feat: delegate task#3549
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service review: one convention issue on the newly added TaskOrchestrator service.
Posted via Macroscope — Effect Service Conventions
| export interface TaskOrchestratorShape { | ||
| /** | ||
| * Spawn a child thread per sub-task and start its turn. Returns immediately | ||
| * with a handle per task (`status: "running"`, or `"error"` if spawning | ||
| * failed). Never blocks on completion. | ||
| */ | ||
| readonly startTasks: (input: TaskOrchestratorStartInput) => Effect.Effect<DelegateTasksResult>; | ||
| /** | ||
| * Poll the given child threads up to `waitMs` and return their results; | ||
| * children still running at the deadline come back as `status: "running"`. | ||
| */ | ||
| readonly collectTasks: ( | ||
| input: TaskOrchestratorCollectInput, | ||
| ) => Effect.Effect<DelegateTasksResult>; | ||
| /** True if `threadId` was spawned as a delegated child during this process. */ | ||
| readonly isDelegatedChild: (threadId: ThreadId) => Effect.Effect<boolean>; | ||
| } |
There was a problem hiding this comment.
The service interface is declared as a standalone TaskOrchestratorShape and then passed into Context.Service. Per the Effect service conventions, define the interface inline in the Context.Service declaration and refer to the inferred shape as TaskOrchestrator["Service"] rather than retaining a FooShape type.
Fix: inline these members into the tag declaration (Context.Service<TaskOrchestrator, { readonly startTasks: ...; readonly collectTasks: ...; readonly isDelegatedChild: ... }>()) and remove TaskOrchestratorShape. Then in Layers/TaskOrchestrator.ts drop the type TaskOrchestratorShape import and change TaskOrchestratorShape["startTasks"], TaskOrchestratorShape["collectTasks"], and TaskOrchestratorShape["isDelegatedChild"] to TaskOrchestrator["Service"][...]. The input types (TaskOrchestratorStartInput/TaskOrchestratorCollectInput) can stay as standalone helper types.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 7 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| }); | ||
|
|
||
| const isDelegatedChild: TaskOrchestratorShape["isDelegatedChild"] = (threadId: ThreadId) => | ||
| Ref.get(registry).pipe(Effect.map((current) => current.has(threadId))); |
There was a problem hiding this comment.
Depth guard ignores persisted parent
High Severity
The TaskOrchestrator uses an in-memory registry for delegated task metadata that isn't durable across server restarts. This can bypass the depth-1 delegation guard, allowing sub-tasks to delegate further. It also prevents collect_delegated_tasks from finding and collecting results for existing delegated tasks if threadIds are not explicitly provided.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| ...base, | ||
| status: "completed", | ||
| message: finalAssistantText(detail) ?? "", | ||
| } satisfies DelegateTaskResult; |
There was a problem hiding this comment.
Interrupted sub-tasks marked completed
Medium Severity
The resultFromDetail function incorrectly treats interrupted sub-task turns as completed. This means a parent agent will see stopped or cancelled sub-tasks as successfully finished, potentially including an assistant message, masking their true outcome.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| targetIds, | ||
| (threadId) => | ||
| pollUntilDone(threadId, deadlineMs).pipe( | ||
| Effect.map((detail) => resultFromDetail(threadId, current.get(threadId), detail)), |
There was a problem hiding this comment.
Collect allows foreign thread IDs
Medium Severity
The collectTasks function processes input.threadIds without verifying they are delegated children of the input.parentThreadId or managed by this orchestrator instance. This allows a caller to query the status and final assistant message of arbitrary threads, potentially exposing information from other sessions.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| "vite-plus": "catalog:" | ||
| }, | ||
| "productName": "T3 Code (Alpha)" | ||
| "productName": "T3 Code Baha" |
There was a problem hiding this comment.
Unrelated Baha desktop branding
High Severity
The change to productName renames the desktop app to "T3 Code Baha" and implicitly changes its bundle ID. This alters production packaging, app updates, and macOS passkey entitlements, which seems outside the PR's stated scope.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| return { results: [] }; | ||
| } | ||
| const settings = yield* settingsService.getSettings.pipe(Effect.orDie); | ||
| const tasks = input.tasks.slice(0, MAX_DELEGATED_TASKS); |
There was a problem hiding this comment.
Extra tasks dropped silently
Medium Severity
startTasks truncates input.tasks with slice(0, MAX_DELEGATED_TASKS) but returns a normal success payload. Requests with more than the maximum number of sub-tasks run only the first batch with no error or count indicating the rest were ignored.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| Effect.gen(function* () { | ||
| const parent = yield* readDetail(input.parentThreadId); | ||
| if (parent === undefined) { | ||
| return { results: [] }; |
There was a problem hiding this comment.
Missing parent yields empty success
Medium Severity
If the lead thread is not found in the projection, startTasks returns { results: [] } instead of failing. delegate_tasks can succeed with zero results even when the caller supplied sub-tasks, which looks like a no-op rather than an error.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| ? input.threadIds | ||
| : Array.from(current.entries()) | ||
| .filter(([, meta]) => meta.parentThreadId === input.parentThreadId) | ||
| .map(([id]) => ThreadId.make(id)); |
There was a problem hiding this comment.
Default collect ignores running filter
Medium Severity
Omitting threadIds on collect_delegated_tasks enqueues every child ever recorded in the registry for that lead, not only those still running. Completed or errored sub-tasks are polled again and returned, contrary to the tool description.
Reviewed by Cursor Bugbot for commit c5e06ec. Configure here.
| ), | ||
| { concurrency: Math.max(1, Math.min(targetIds.length, MAX_DELEGATED_TASKS)) }, | ||
| ); | ||
| return { results }; |
There was a problem hiding this comment.
🟡 Medium Layers/TaskOrchestrator.ts:268
Completed and errored children are never removed from registry. When collectTasks is called without threadIds, it builds targetIds from every registry entry for the lead (lines 242-247), so collect_delegated_tasks keeps returning old finished sub-tasks instead of only the still-running children promised by the request contract. Consider removing entries from registry once their final status is known (either completed or error) so that subsequent collection calls only see still-running children.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/TaskOrchestrator.ts around line 268:
Completed and errored children are never removed from `registry`. When `collectTasks` is called without `threadIds`, it builds `targetIds` from every registry entry for the lead (lines 242-247), so `collect_delegated_tasks` keeps returning old finished sub-tasks instead of only the still-running children promised by the request contract. Consider removing entries from `registry` once their final status is known (either completed or error) so that subsequent collection calls only see still-running children.
| collect_delegated_tasks: (input) => | ||
| Effect.gen(function* () { | ||
| const scope = yield* McpInvocationContext.McpInvocationContext; | ||
| const orchestrator = yield* TaskOrchestrator; | ||
| return yield* orchestrator.collectTasks({ | ||
| parentThreadId: scope.threadId, | ||
| threadIds: input.threadIds, | ||
| waitMs: INLINE_WAIT_MS, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🟠 High delegation/handlers.ts:52
collect_delegated_tasks passes input.threadIds directly to orchestrator.collectTasks without validating they belong to the current thread. This lets callers pass any threadId they know (including other users' delegated children) and receive those task results. The parentThreadId parameter only scopes results when threadIds is omitted, so explicit thread IDs bypass the isolation check.
- return yield* orchestrator.collectTasks({
- parentThreadId: scope.threadId,
- threadIds: input.threadIds,
- waitMs: INLINE_WAIT_MS,
- });
+ // Omit threadIds to let orchestrator return only this thread's delegated children
+ return yield* orchestrator.collectTasks({
+ parentThreadId: scope.threadId,
+ waitMs: INLINE_WAIT_MS,
+ });🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/mcp/toolkits/delegation/handlers.ts around lines 52-61:
`collect_delegated_tasks` passes `input.threadIds` directly to `orchestrator.collectTasks` without validating they belong to the current thread. This lets callers pass any `threadId` they know (including other users' delegated children) and receive those task results. The `parentThreadId` parameter only scopes results when `threadIds` is omitted, so explicit thread IDs bypass the isolation check.
There was a problem hiding this comment.
🟡 Medium
t3code/packages/contracts/src/settings.ts
Line 532 in c5e06ec
taskRouting was added to ServerSettings but is missing from ServerSettingsPatch, so any taskRouting value sent through server.updateSettings is silently dropped because the RPC validates against the patch schema and applyServerSettingsPatch only merges known fields.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/settings.ts around line 532:
`taskRouting` was added to `ServerSettings` but is missing from `ServerSettingsPatch`, so any `taskRouting` value sent through `server.updateSettings` is silently dropped because the RPC validates against the patch schema and `applyServerSettingsPatch` only merges known fields.
| }) | ||
| .pipe(Effect.orDie); | ||
|
|
||
| yield* Ref.update(registry, (current) => { |
There was a problem hiding this comment.
🟡 Medium Layers/TaskOrchestrator.ts:199
If thread.create succeeds but thread.turn.start fails, the child thread is created but never added to registry. This leaves a real delegated child that collectTasks cannot find and isDelegatedChild incorrectly reports as false, allowing the depth-1 recursion guard to be bypassed from that orphaned child.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/TaskOrchestrator.ts around line 199:
If `thread.create` succeeds but `thread.turn.start` fails, the child thread is created but never added to `registry`. This leaves a real delegated child that `collectTasks` cannot find and `isDelegatedChild` incorrectly reports as `false`, allowing the depth-1 recursion guard to be bypassed from that orphaned child.
ApprovabilityVerdict: Needs human review 4 blocking correctness issues found. This PR introduces a substantial new task delegation feature enabling parallel child thread spawning. Multiple unresolved review comments identify security concerns (arbitrary thread access), correctness issues (depth guard bypass), and unrelated desktop branding changes affecting the bundle ID. Human review is warranted for this scope of new capability with open issues. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Introduces parallel child agent runs sharing a worktree and new orchestration/MCP paths; mis-coordinated edits or routing mistakes could amplify cost or conflicts, though depth-1 delegation and bounded waits limit blast radius.
Overview
Adds agent task delegation: lead threads can fan out independent sub-tasks to child threads through new MCP tools
delegate_tasksandcollect_delegated_tasks, wired into the HTTP MCP server with aTaskOrchestratorthat spawns children in the same worktree, starts turns, and polls completion within bounded waits so tool calls stay under client timeouts.Server & contracts: New
@t3tools/contractsdelegation schemas (DelegateTaskInput, results, limits), optionalparentThreadId/taskLabelon thread create/events/projections, DB migration033_ProjectionThreadDelegation, and configurabletaskRoutingrules plusTaskModelRouterfor per-sub-task model selection (modelHint, label rules, explicit override).MCP behavior:
delegate_tasksstarts up to 8 tasks with concurrency caps, optionally waits ~30s inline, then returnsrunning/completed/error; children cannot delegate again (depth-1).collect_delegated_tasksfinishes polling running handles.UI: Sidebar nests delegated children under their lead (
nestDelegatedChildren) with indent and a sub-task marker.Also in diff: Desktop packaging rebrands production app to T3 Code Baha /
com.t3tools.t3code.baha(product name, app id, signing tests)—separate from delegation.Reviewed by Cursor Bugbot for commit c5e06ec. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add task delegation MCP tools with child thread spawning and sidebar grouping
delegate_tasksandcollect_delegated_tasksMCP tools via a newDelegationToolkit, allowing providers to spawn and collect results from delegated child threadsTaskOrchestratorservice that spawns child threads via the orchestration engine, polls for completion with a 30s bound, and guards against recursive delegation viaisDelegatedChildtaskRoutingsection of server settings, with fallback to the parent modelparentThreadIdandtaskLabelon threads via a new DB migration (033) and propagates these fields through contracts, projections, and snapshot queries⤷marker andtaskLabelas title📊 Macroscope summarized c5e06ec. 22 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.