Skip to content

feat: model-driven permission control for subtasks (Phase 3b)#12337

Closed
roomote-v0[bot] wants to merge 4 commits into
mainfrom
feature/task-permissions-phase-3b
Closed

feat: model-driven permission control for subtasks (Phase 3b)#12337
roomote-v0[bot] wants to merge 4 commits into
mainfrom
feature/task-permissions-phase-3b

Conversation

@roomote-v0
Copy link
Copy Markdown
Contributor

@roomote-v0 roomote-v0 Bot commented May 12, 2026

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 permissions parameter to the new_task tool, 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 under src/components/ and can only run npm commands".

Key implementation details:

  • TaskPermissions type (packages/types/src/task-permissions.ts): Defines four permission boundary fields:

    • filePatterns - regex patterns for allowed file paths
    • commandPatterns - regex patterns for allowed shell commands
    • allowedTools - explicit tool allowlist
    • deniedTools - explicit tool blocklist
  • Most-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/commandPatterns use intersection; deniedTools uses union.

  • Runtime enforcement in validateToolUse(): All permission types are checked at tool execution time with clear TaskPermissionError messages. Always-available tools (like attempt_completion) remain accessible regardless of allowedTools.

  • Optional by default: When permissions is omitted from new_task, behavior is identical to today -- zero friction for normal users. Advanced users can pre-define permission rules for subtasks.

Files changed:

File Change
packages/types/src/task-permissions.ts New TaskPermissions type, schema, merge logic
packages/types/src/task.ts Add taskPermissions to CreateTaskOptions
src/core/prompts/tools/native-tools/new_task.ts Add permissions parameter to tool definition
src/shared/tools.ts Add permissions to NativeToolArgs and NewTaskToolUse
src/core/tools/NewTaskTool.ts Parse and validate permissions JSON
src/core/webview/ClineProvider.ts Pass permissions through delegateParentAndOpenChild
src/core/task/Task.ts Store and merge permissions in constructor
src/core/tools/validateToolUse.ts Enforce all permission types at runtime
src/core/assistant-message/presentAssistantMessage.ts Pass task permissions to validation
src/core/assistant-message/NativeToolCallParser.ts Parse permissions in native tool calls

Test Procedure

  • 32 new unit tests added across 2 test files:
    • packages/types/src/__tests__/task-permissions.spec.ts (21 tests): Schema validation, merge logic, pattern matching
    • src/core/tools/__tests__/taskPermissionsEnforcement.spec.ts (11 tests): Runtime enforcement of all permission types
  • All 24 existing validateToolUse tests continue to pass
  • All 61 existing mode and newTaskTool tests continue to pass
  • Full lint and type-check pass across all packages

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue
  • Scope: Changes are focused on the linked issue
  • Self-Review: Performed a thorough self-review
  • Testing: New tests added to cover changes
  • Documentation Impact: May need docs update for the new permissions parameter
  • Contribution Guidelines: Read and agree to the Contributor Guidelines

Documentation Updates

The new_task tool now supports an optional permissions parameter. Documentation for how Orchestrator mode uses this to scope subtask access may be needed.

Interactively review PR in Roo Code Cloud

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)
@easonLiangWorldedtech
Copy link
Copy Markdown

@roomote

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 new_task tool. From my understanding, it allows the parent/Orchestrator to restrict what a subtask can do (files, commands, tools).

A few questions for you to evaluate and decide:

  1. Does this approach align well with the overall goal in [ENHANCEMENT] Support parallel execution of specialized agents and improve context handoff between modes #12330 (supporting parallel execution of specialized agents + better context handoff between modes)?

  2. Is most-restrictive-wins merging the best strategy for nested subtasks, or would you recommend a different merging logic?

  3. This PR focuses on permission control. How do you think we should handle context handoff between agents/modes in the next phases? Should we introduce a structured HandoffContext object, or is there a simpler way using existing mechanisms?

  4. Are there any risks or limitations you see with the current implementation when multiple agents run in parallel?

Please review your own changes and let me know:

  • Whether this is the right direction
  • What improvements or alternative designs you would suggest
  • What the next step (Phase 3c or context handoff) should look like

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!

@roomote-v0
Copy link
Copy Markdown
Contributor Author

roomote-v0 Bot commented May 12, 2026

Fixaroo Clock   See task

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.

View commit | Revert commit

…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.
@easonLiangWorldedtech
Copy link
Copy Markdown

@roomote

Thanks for quickly fixing those three bugs. Good catches.

Since you're the one implementing, I need your honest technical assessment:

  1. After these fixes, do you think the current implementation of Phase 3b is now solid and ready to be merged? Or are there still important issues/risks we should address first?

  2. From your perspective, is this permission control mechanism good enough as a foundation for the bigger goal in [ENHANCEMENT] Support parallel execution of specialized agents and improve context handoff between modes #12330 (parallel execution of specialized agents)?

  3. What's your recommendation for the next step?

    • Should we merge this PR first and then move to context handoff (Phase 3c)?
    • Or do you think we should make further improvements to this PR before merging?
    • What do you think should be the priority for the next phase?

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!

@roomote-v0
Copy link
Copy Markdown
Contributor Author

roomote-v0 Bot commented May 12, 2026

Fixaroo Clock   See task

Technical Assessment of Phase 3b

Is 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:

  1. Regex patterns were not anchored -- matchesAnyPattern used partial matching, so filePatterns: ["src/.*"] would incorrectly allow paths like evil/src/components/foo. Now patterns are auto-anchored with ^(?:...)$.
  2. Invalid regex patterns were silently swallowed at runtime -- Added a regexString Zod refinement so malformed patterns are rejected at parse time in NewTaskTool, giving the model a clear error to correct.
  3. Duplicate validation code -- The layered vs flat pattern enforcement in validateToolUse had two near-identical code blocks. Unified into a single path that falls back to wrapping flat patterns as a single layer.

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 TaskPermissions type and merge logic are cleanly separated in packages/types, the enforcement is centralized in validateToolUse, and the plumbing through ClineProvider -> Task constructor is minimal. This gives future phases (background task runner, parallel UI, write-parallelism with file locking) a solid permission layer to build on.

Remaining considerations before merge

  • Permissions are not persisted in HistoryItem -- if VS Code restarts mid-subtask, permissions are lost. This is acceptable for now since subtasks don't survive restarts anyway, but worth noting for Phase 3c/3d.
  • No ReDoS mitigation -- model-provided regex patterns are compiled directly. Since the attack surface is limited (patterns come from the parent LLM, not external users), this is low risk but could be hardened later with a pattern complexity check or timeout.

Recommended next step

Merge 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.

View commit | Revert commit

…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.
@easonLiangWorldedtech
Copy link
Copy Markdown

@roomote

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:

  1. Permissions not being persisted in HistoryItem
  2. Lack of ReDoS mitigation for model-provided regex patterns

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:

  • Which of these two should we fix before merging?
  • How much effort would it take to implement them?
  • Would you recommend fixing both, or just the persistence one for now?

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.

@roomote-v0
Copy link
Copy Markdown
Contributor Author

roomote-v0 Bot commented May 12, 2026

Planneroo Clock   See task

Implemented both fixes before merging, as recommended:

  1. Permissions persistence in HistoryItem -- taskPermissions is now persisted in historyItemSchema and restored when loading tasks from history, so permission boundaries survive restarts.
  2. ReDoS mitigation -- Added isSafeRegex() heuristic that rejects nested quantifiers (e.g. (a+)+) and overlapping alternations in repeated groups, plus a 200-char max pattern length. Both enforced at schema validation time via Zod refinements.

All 80 related tests pass (43 types + 13 enforcement + 24 validateToolUse). Full lint and type-check clean across all packages.

View commit | Revert commit

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
@roomote-v0
Copy link
Copy Markdown
Contributor Author

roomote-v0 Bot commented May 12, 2026

Superseded by #12339, which includes all changes from this PR plus prompt guidance and UI visibility. #12339 is now the single final PR for Phase 3b.

@roomote-v0 roomote-v0 Bot closed this May 12, 2026
@github-project-automation github-project-automation Bot moved this from New to Done in Roo Code Roadmap May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Support parallel execution of specialized agents and improve context handoff between modes

2 participants