feat: model-driven permission control for subtasks (Phase 3b)#12337
feat: model-driven permission control for subtasks (Phase 3b)#12337roomote-v0[bot] wants to merge 4 commits into
Conversation
Adds an optional `permissions` parameter to the `new_task` tool, allowing the Orchestrator (or any parent task) to dynamically set permission boundaries for subtasks: - New `TaskPermissions` type with filePatterns, commandPatterns, allowedTools, and deniedTools - Permission merging with most-restrictive-wins semantics for nested subtask delegation - Runtime enforcement in validateToolUse() for all permission types - Full test coverage for merging logic and enforcement Addresses Issue #12330 (Phase 3b)
|
Thanks for implementing Phase 3b so quickly! I'm not a coder, so I need your guidance here. This PR adds model-driven permission control for subtasks via the A few questions for you to evaluate and decide:
Please review your own changes and let me know:
I'm relying on your technical judgment here. Feel free to propose changes or a better approach if you think this can be improved. Thanks again! |
Fixed three bugs: (1) permissions parsed only when todos present in NativeToolCallParser, (2) deniedTools could trap subtask by blocking attempt_completion, (3) pattern merging used exact-string intersection instead of layered AND enforcement. All local checks passed. |
…pattern merging 1. NativeToolCallParser: Remove permissions from update_todo_list cases (was erroneously added to wrong tool case, should only be on new_task) 2. deniedTools: Exempt ALWAYS_AVAILABLE_TOOLS (attempt_completion, etc.) from deniedTools check, matching the existing allowedTools behavior. Prevents parent from trapping subtask by denying completion tools. 3. Pattern merging: Replace broken exact-string intersection with layered enforcement. filePatterns/commandPatterns from parent and child are kept as separate layers (AND between layers, OR within each layer). This correctly handles narrowing: parent ["src/.*"] + child ["src/components/.*"] now allows only files matching BOTH patterns, instead of producing an empty intersection.
|
Thanks for quickly fixing those three bugs. Good catches. Since you're the one implementing, I need your honest technical assessment:
Please give me your recommendation and reasoning. I'm relying on your judgment here. If you think it's ready, feel free to say so clearly. If you want to make more changes, also let me know what you plan to do. Thanks! |
Technical Assessment of Phase 3bIs it solid enough to merge?The implementation is well-structured and the core design decisions (most-restrictive-wins merging, layered pattern enforcement, always-available tool exemptions) are sound. After reviewing the code I found three hardening issues and pushed fixes:
All 87 tests pass (34 types + 53 enforcement/validation/newTaskTool), plus full lint and type-check across all packages. Is it a good foundation for #12330?Yes. The Remaining considerations before merge
Recommended next stepMerge this PR, then move to Phase 3c (context handoff improvements) or directly to the background task runner, depending on whether the team wants better handoff visibility first or wants to start tackling the single-open invariant. |
…ema level, simplify validation code 1. Anchor regex patterns in matchesAnyPattern with ^(?:...)$ wrapping so patterns like "src/.*" require full-path matching instead of substring matching. Prevents "evil/src/foo" from matching a "src/.*" permission. 2. Add regex validation at schema level (regexString refinement) so invalid patterns are rejected at parse time rather than silently failing at runtime. 3. Simplify duplicate file/command pattern validation in validateToolUse by unifying layered and flat code paths into a single branch that falls back to wrapping flat patterns as a single layer. 4. Remove unused matchesAnyPattern import from validateToolUse.ts. 5. Add tests for anchoring behavior, pre-anchored patterns, and invalid regex rejection at schema level.
|
Thanks for the detailed assessment and the additional fixes. All tests passing is good to see. I agree this is becoming a solid foundation. However, before we merge and move to Phase 3c, I would like to address the two remaining considerations you mentioned:
Even if subtasks currently don't survive restarts, I think it's better to make the permission system more complete and robust before we build context handoff (Phase 3c) on top of it. Can you evaluate:
Please give me your recommendation on whether we should fix these issues in this PR before merging, or if it's better to merge as-is and handle them in a follow-up. I'm relying on your judgment. |
Implemented both fixes before merging, as recommended:
All 80 related tests pass (43 types + 13 enforcement + 24 validateToolUse). Full lint and type-check clean across all packages. |
1. Persist taskPermissions in HistoryItem so permissions survive task
restarts. Added taskPermissions field to historyItemSchema, included
it in taskMetadata output, and restored it in the Task constructor
when loading from history.
2. Add ReDoS mitigation for model-provided regex patterns:
- isSafeRegex() heuristic rejects nested quantifiers like (a+)+
and overlapping alternations in repeated groups like (a|a)+
- Max pattern length capped at 200 characters
- Both checks enforced at schema validation time via Zod refinements
- 11 new tests covering ReDoS detection and persistence round-trips
Related GitHub Issue
Closes: #12330
Description
This PR attempts to address Issue #12330 by implementing Phase 3b: model-driven permission control for subtasks. Feedback and guidance are welcome.
What this adds:
An optional
permissionsparameter to thenew_tasktool, allowing the Orchestrator (or any parent task) to dynamically set permission boundaries for subtasks. This directly addresses Question 1 from the issue comment: the ability to tell a subtask "you can only edit files undersrc/components/and can only runnpmcommands".Key implementation details:
TaskPermissionstype (packages/types/src/task-permissions.ts): Defines four permission boundary fields:filePatterns- regex patterns for allowed file pathscommandPatterns- regex patterns for allowed shell commandsallowedTools- explicit tool allowlistdeniedTools- explicit tool blocklistMost-restrictive-wins merging (
mergeTaskPermissions): When nested subtasks are created, permissions are merged so a child can never grant itself more access than its parent.allowedTools/filePatterns/commandPatternsuse intersection;deniedToolsuses union.Runtime enforcement in
validateToolUse(): All permission types are checked at tool execution time with clearTaskPermissionErrormessages. Always-available tools (likeattempt_completion) remain accessible regardless ofallowedTools.Optional by default: When
permissionsis omitted fromnew_task, behavior is identical to today -- zero friction for normal users. Advanced users can pre-define permission rules for subtasks.Files changed:
packages/types/src/task-permissions.tsTaskPermissionstype, schema, merge logicpackages/types/src/task.tstaskPermissionstoCreateTaskOptionssrc/core/prompts/tools/native-tools/new_task.tspermissionsparameter to tool definitionsrc/shared/tools.tspermissionstoNativeToolArgsandNewTaskToolUsesrc/core/tools/NewTaskTool.tssrc/core/webview/ClineProvider.tsdelegateParentAndOpenChildsrc/core/task/Task.tssrc/core/tools/validateToolUse.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/assistant-message/NativeToolCallParser.tsTest Procedure
packages/types/src/__tests__/task-permissions.spec.ts(21 tests): Schema validation, merge logic, pattern matchingsrc/core/tools/__tests__/taskPermissionsEnforcement.spec.ts(11 tests): Runtime enforcement of all permission typesvalidateToolUsetests continue to passPre-Submission Checklist
permissionsparameterDocumentation Updates
The
new_tasktool now supports an optionalpermissionsparameter. Documentation for how Orchestrator mode uses this to scope subtask access may be needed.Interactively review PR in Roo Code Cloud