feat(specialist): zeromaxing posture and orchestrate plan execution - #829
feat(specialist): zeromaxing posture and orchestrate plan execution#829gnanam1990 wants to merge 195 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a "zeromaxing" execution posture spanning the agent core, execprofile, config, CLI, and TUI layers. It introduces a specialist plan orchestration engine (parsing, scheduling, watchdog, worktree isolation, saved plans, background execution) with an OrchestrateTool. It adds tool interfaces for progress streaming, persistent permission refusal, and permanent denial, with confirmation-policy gating. It builds TUI orchestrate panel, sidebar, and plan-progress UI. It also hardens sandbox temporary grants, credstore file locking, and git subprocess execution in worktrees. ChangesZeromaxing Execution Posture
Estimated code review effort: 5 (Critical) | ~240 minutes Tool Capability Declarations and Confirmation Policy
Specialist Plan Orchestration Engine
CLI Plan/Orchestrate Registration and Background Launcher
TUI Orchestrate Panel, Sidebar, and Plan Progress UI
Sandbox Temporary Grant Reference Counting
Credstore File Locking
Worktrees Hardened Git Subprocess
Sequence Diagram(s)sequenceDiagram
participant TUIModel
participant AgentRun
participant SystemPrompt
participant PostureGate
TUIModel->>AgentRun: Options.Zeromaxing = Entering
AgentRun->>AgentRun: zeromaxingReminders(posture, turn, orchestrateAvailable)
AgentRun->>AgentRun: append reminder messages to conversation
AgentRun->>SystemPrompt: runCanMutate(options)
SystemPrompt-->>AgentRun: policy included or stripped
AgentRun->>PostureGate: PostureActive()
PostureGate-->>AgentRun: true or false
AgentRun-->>TUIModel: turn response with reminders
TUIModel->>TUIModel: advanceZeromaxing() on completion or cancel
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specialist/exec.go (1)
608-608: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
TotalTokensis silently dropped on a post-start child failure.In the error branch (Lines 626-635),
summary := SummarizeStream(run.Events, exitCode)is computed — meaning any tokens the child reported before crashing are known — but the returnedExecResult{SessionID: built.SessionID}omitsTotalTokens, unlike the success path at Line 642. Per the comment onTotalTokens(Lines 220-226), the plan executor meters its budget from this field; a task that burns real tokens and then errors out will report 0 tokens, letting the budget meter under-count actual spend for every failed/crashed task — exactly the invisible-to-unit-tests defect class this field was added to fix.🐛 Proposed fix
if err != nil { exitCode := run.exitCodeOr(-1) summary := SummarizeStream(run.Events, exitCode) executor.recordSpecialistStop(accounting, summary, "error", summary.ExitCode, err, false) // Carry the child session id even on a post-start failure so a caller (the // swarm launcher -> FailWithSession) can still make the failed member // drillable; the session exists once the child has started. - return ExecResult{SessionID: built.SessionID}, err + return ExecResult{SessionID: built.SessionID, TotalTokens: summary.Usage.EffectiveTotalTokens()}, err }Also applies to: 626-643
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/exec.go` at line 608, Update the error return in runBuiltArgs to include the TotalTokens value from the already-computed summary, matching the success path’s token accounting. Preserve the existing SessionID and error behavior while ensuring failed or crashed child executions report tokens consumed before failure.
🟡 Minor comments (25)
internal/tui/model.go-2449-2452 (1)
2449-2452: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't freeze
orchestratefor background plans
m.orchestrate.frozenAt = m.now()makes a background plan that keeps sendingplanTaskStartMsg/planTaskProgressMsgafter the run ends look frozen forever, because nothing clearsfrozenAtagain. Gate this on foreground-only runs or resetfrozenAtwhen background task activity resumes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 2449 - 2452, Update the orchestrate freeze handling around m.orchestrate.frozenAt so background plans are not permanently frozen when they continue emitting planTaskStartMsg or planTaskProgressMsg after the run ends. Apply the freeze only to foreground runs, or clear frozenAt when background task activity resumes, while preserving the existing behavior for foreground plans.internal/tui/mouse.go-90-106 (1)
90-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBlock the footer chip while an overlay is active.
m.zeromaxingChipAtMousehas nosetup/wizard/mcpManager/picker/suggestionsguard, so a click on the posture chip can preempt an open overlay and replace it with the effort picker. Match the early-return used byorchestrateTaskAtMousebefore this branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/mouse.go` around lines 90 - 106, Guard the posture-chip click branch identified by zeromaxingChipAtMouse with the same setup, wizard, mcpManager, picker, and suggestions overlay checks used by orchestrateTaskAtMouse. Return without opening newEffortPicker when any overlay is active, preserving the existing pending-turn behavior otherwise.internal/tui/sidebar_plan_detail_test.go-219-231 (1)
219-231: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe 30-task rounding case may not actually have 30 tasks.
string(rune('a' + index%26))repeats idsa–dafter 26, so ifadmitkeys tasks by id the panel ends up with 26 rows whiletaskCountsays 30 — the test would still pass, but not on the shape it claims to exercise. Give each task a unique id.♻️ Unique ids
- for index := 0; index < 30; index++ { - msg.tasks = append(msg.tasks, planGraphTask{id: string(rune('a' + index%26))}) - } + for index := 0; index < 30; index++ { + msg.tasks = append(msg.tasks, planGraphTask{id: fmt.Sprintf("t%02d", index)}) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/sidebar_plan_detail_test.go` around lines 219 - 231, The 30-task rounding test creates duplicate task IDs after 26 iterations, so admission may retain fewer than 30 tasks. Update the task construction in the big.orchestrate test setup to generate a unique id for every index while preserving msg.taskCount and the existing failure/progress-bar assertions.internal/tui/orchestrate_panel.go-534-544 (1)
534-544: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUncapped indent in the
/plansrows. UnlikerenderOrchestrateTaskLine, this uses rawtask.depth, so a 40-link chain indents 80 columns and wraps into unreadable text. ReuseorchestrateMaxIndentDepthhere for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/orchestrate_panel.go` around lines 534 - 544, Update orchestratePlainTaskLine to cap task.depth with orchestrateMaxIndentDepth before calculating the repeated indentation, matching renderOrchestrateTaskLine while preserving the rest of the row formatting.internal/tui/plan_progress.go-309-325 (1)
309-325: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RunningPlanNamecan report the previous plan's name.
lastPlanNameis never cleared inPlanCompleted, so a newly launched-but-not-yet-admitted plan (background flag set, nothing admitted yet) is refused under the last plan's name instead of the honest "a plan" fallback. Clearing the name when the plan ends keeps the refusal truthful whilelastPlan(what/plans saveneeds) stays intact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/plan_progress.go` around lines 309 - 325, The PlanProgressBridge completion flow must clear lastPlanName when the current plan ends so RunningPlanName cannot reuse a previous plan’s name for a newly launched, unadmitted plan. Update PlanCompleted to reset lastPlanName while preserving lastPlan for /plans save and keeping RunningPlanName’s “a plan” fallback unchanged.internal/tui/specialist_card.go-28-33 (1)
28-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe new
specialistCancelledvalue isn't reflected in two render paths.
specialistStatusStringmaps it, but:
- the header switch in
renderSpecialistCard(Lines 319-329) has nospecialistCancelledarm, so a stopped/skipped task falls intodefaultand renders with the accent•— the same treatment as pending/running, i.e. a finished task that looks live;renderSpecialistSummary(Lines 504-517) counts cancelled cards inlen(specialists)but in neitherrunningnorcompleted, so "3 specialists · 0 running · 1 done" silently loses two of them.Everything else here (token/result setters, exit-code guard, zero-token omission) looks right.
Also applies to: 50-53, 103-129, 200-208, 334-340, 524-529
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/specialist_card.go` around lines 28 - 33, Update the specialistCancelled handling in renderSpecialistCard so cancelled tasks use the finished/non-live header rendering instead of the default accent bullet. Update renderSpecialistSummary to count cancelled specialists in the appropriate completed/done total while leaving running counts unchanged, so every card in len(specialists) is represented in the summary.internal/tui/orchestrate_panel.go-347-353 (1)
347-353: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale keybinding in the comment. The comment says "Ctrl+O expands" while the header text and the actual binding are ctrl+g.
📝 Proposed fix
- // away. Ctrl+O expands. + // away. Ctrl+G expands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/orchestrate_panel.go` around lines 347 - 353, Update the explanatory comment above the !state.expanded check to say that Ctrl+G expands the details, matching the header text and actual keybinding; leave the collapse behavior unchanged.internal/tui/orchestrate_control_test.go-374-383 (1)
374-383: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis assertion can't fail.
m.orchestrate.admit(...)installs one task, soisEmpty()is already false; the&&makes the check vacuous whether or not the terminal message survived the guard. Assert the observable effect ofcompleteinstead — the status landing on the panel.💚 Proposed fix
updated, _ := m.Update(done) - if updated.(model).orchestrate.frozenAt.IsZero() && updated.(model).orchestrate.isEmpty() { - t.Fatal("the terminal message was dropped by the stale-run guard") - } + if got := updated.(model).orchestrate.status; got != string(specialist.PlanCompleted) { + t.Fatalf("panel status = %q; the terminal message was dropped by the stale-run guard", got) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/orchestrate_control_test.go` around lines 374 - 383, Update the assertion in the test around model.Update(done) to verify the observable completion effect: assert that the completed task’s status is reflected on the panel. Do not use the current frozenAt/isEmpty conjunction, since admit installs a task and makes isEmpty() false regardless of whether the terminal message is processed.internal/tui/sidebar_plan_detail.go-193-213 (1)
193-213: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUntrusted text reaches the column unsanitised here.
sidebarAgentExpansionininternal/tui/sidebar.goruns child output throughsanitizeCardTextprecisely because "a child's answer is untrusted text and an ANSI escape in it would repaint the column". This path rendersinfo.errorMsg(viafirstLineOf),info.currentTool/currentDetailand the model-suppliedtask.summarywith only truncation, so the same escape repaints the sidebar from here.🛡️ Proposed fix
-func firstLineOf(text string) string { - if index := strings.IndexAny(text, "\r\n"); index >= 0 { - return strings.TrimSpace(text[:index]) - } - return strings.TrimSpace(text) -} +// Sanitised, not merely first-lined: this text came from a child agent or the +// model, and an ANSI escape in it repaints the column. +func firstLineOf(text string) string { + return sanitizeCardText(text) +}and wrap
activityandtask.summaryinsanitizeCardTextbeforetruncateStep.Also applies to: 231-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/sidebar_plan_detail.go` around lines 193 - 213, Sanitize all untrusted sidebar text before rendering in the plan-detail flow: apply sanitizeCardText to activity assembled from info.currentTool/currentDetail, task.summary, and the outcome text produced by orchestrateOutcomeLine (including info.errorMsg via firstLineOf). Keep truncation and existing layout behavior unchanged, using the sanitized values as input to truncateStep.internal/tui/orchestrate_panel_test.go-474-477 (1)
474-477: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComment names the wrong key. The test drives
ctrl+g(and is named for it); the doc comment says Ctrl+O.📝 Proposed fix
-// Ctrl+O toggles it through the real key handler, and does nothing when there +// Ctrl+G toggles it through the real key handler, and does nothing when there🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/orchestrate_panel_test.go` around lines 474 - 477, Correct the doc comment above TestCtrlGTogglesTheOrchestratePanel to refer to Ctrl+G instead of Ctrl+O, keeping the test name and key-handler behavior unchanged.internal/specialist/plan.go-483-494 (1)
483-494: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
max_wall_secondsis the one budget field with no validation at all.A negative value is silently discarded (unlike
max_tokens, which is rejected), and there is no floor (unlikemax_stall_seconds, which has one for exactly the "random task-killer" reason)."max_wall_seconds": 1admits cleanly and then kills the plan before the first child says anything. Rejecting negatives and applying a small floor keeps the budget validation story consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan.go` around lines 483 - 494, The plan parser’s max_wall_seconds handling lacks negative-value rejection and a minimum timeout. Update the validation around planInt and the budget.MaxWall assignment to reject negative values and enforce the appropriate small wall-time floor, matching the existing max_tokens and max_stall_seconds validation patterns while preserving valid-budget assignment.internal/specialist/plan.go-409-417 (1)
409-417: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe remedy can suggest the tier the user is already on.
planTooLargeErroralways namesconfig.PlanSizeLarge, so a run already at the large tier gets "raise it with planSize: large" — advice that changes nothing, from a message whose whole stated purpose is being actionable. Consider only appending the raise-it clause whenlimits.MaxTasks < config.PlanSizeLarge.MaxTasks(), and otherwise just telling the user to split the plan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan.go` around lines 409 - 417, Update planTooLargeError so the raise-it configuration guidance is appended only when limits.MaxTasks is below config.PlanSizeLarge.MaxTasks(); for runs already at the large tier, return an error that advises splitting the plan without suggesting the unchanged large setting. Preserve the existing source-specific context and task-count details.internal/specialist/plan_worktree.go-8-17 (1)
8-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc comments across the plan engine still describe the pre-write-tool, sequential design. These files carry the "Phase 2 tasks are read-only and run one at a time" narrative that this PR's own code and tests contradict — write tools are admitted, isolation is reachable, and
max_workersgoes up tomaxPlanWorkers. One sweep, three sites:
internal/specialist/plan_worktree.go#L8-L17: drop "Nothing requires isolation today" and the step-3-is-future framing (repeat at L56-L61);validateTaskToolsnow admitsplanWriteToolsandplan_test.goassertsRequiresIsolation()is true.internal/specialist/plan.go#L13-L22: replace "executed SEQUENTIALLY" with the actual worker-slot scheduling, and fix thewriteToolMarkersname at L114-L116 toplanReadOnlyTools.internal/specialist/plan_test.go#L306-L306: delete "(n) A write tool is rejected — Phase 2 tasks are read-only", which sits directly aboveTestAWriteToolIsPermittedOnlyWhenTheParentHoldsIt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_worktree.go` around lines 8 - 17, Update the plan-engine documentation across internal/specialist/plan_worktree.go lines 8-17 and 56-61 to remove the obsolete “nothing requires isolation” and future step-3 framing; revise internal/specialist/plan.go lines 13-22 to describe worker-slot scheduling instead of sequential execution and rename writeToolMarkers to planReadOnlyTools at lines 114-116; delete the outdated write-tool rejection text above TestAWriteToolIsPermittedOnlyWhenTheParentHoldsIt in internal/specialist/plan_test.go line 306.internal/specialist/plan_store_test.go-550-556 (1)
550-556: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winA failed
Runturns this test into a nil-func panic instead of a readable failure.
tool.Run's result is discarded, so if the plan is ever refused beforeLaunchfires,launchedstays nil and Line 555 panics. Assert the launch happened first — same guard the sibling test at Line 477 already has.💚 Fail with a reason
- tool.Run(t.Context(), map[string]any{ + result := tool.Run(t.Context(), map[string]any{ "tasks": []any{task("a", "x"), task("b", "y", "a")}, "budget": map[string]any{"max_workers": float64(1)}, "background": true, }) + if launched == nil { + t.Fatalf("nothing was handed to the launcher: %+v", result) + } launched(context.Background())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_store_test.go` around lines 550 - 556, Update the test around tool.Run and launched to assert that Run succeeds and launched is non-nil before invoking it, matching the guard used by the sibling test. Preserve the existing launch invocation after this assertion so a refused plan produces a readable test failure instead of a nil-function panic.internal/specialist/plan_store.go-188-193 (1)
188-193: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOnly a missing directory should be silent.
The comment names the ordinary case but the code swallows every
ReadDirfailure, so an unreadable.zero/plans(bad perms, I/O error) reports "no saved plans" — the silent skip this file's header explicitly rules out.🛡️ Report anything that is not "not exist"
entries, err := os.ReadDir(dir) if err != nil { - // A missing directory is the ordinary case, not a problem worth naming. - return nil, nil + // A missing directory is the ordinary case; anything else is reported. + if os.IsNotExist(err) { + return nil, nil + } + return nil, []string{fmt.Sprintf("%s: %v", dir, err)} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_store.go` around lines 188 - 193, Update loadPlanDir so only an os.ReadDir error indicating the directory does not exist returns nil, nil; propagate or record all other failures in the problems result, including permission and I/O errors, while preserving normal directory loading.internal/specialist/plan_retry_test.go-329-340 (1)
329-340: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
t.Fatalinside a plan runner callback runs off the test goroutine.ExecutePlanIndispatches every task throughgo func(...), so anyFailNowin a runner closure exits only that goroutine — the completion is never harvested, the plan keeps going, and the failure surfaces as a timeout or a confusing report instead of the intended message.
internal/specialist/plan_retry_test.go#L329-L340: record the missing-cancel condition in a variable and assert it afterExecutePlanreturns.internal/specialist/plan_worktree_test.go#L171-L186: theRunTaskclosure'st.Fatalis latent today (the closure is never invoked); make it set a flag the test checks, or remove the unused fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_retry_test.go` around lines 329 - 340, Replace the callback t.Fatal in internal/specialist/plan_retry_test.go:329-340 with a recorded missing-cancel condition, then assert that condition after ExecutePlan returns; update the RunTask closure in internal/specialist/plan_worktree_test.go:171-186 to set a flag checked by the test, or remove the unused fixture.internal/specialist/plan_tool.go-225-252 (1)
225-252: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
PermissionForArgsdoc comment is attached toRefusesPersistentPermission.The block starting at Line 225 ("PermissionForArgs is what makes a WRITE-CAPABLE plan ask…") runs straight into the
RefusesPersistentPermissionparagraph with no separator, so godoc hangs all of it on Line 250's method whilePermissionForArgs(Line 252) ends up undocumented. Split the two comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_tool.go` around lines 225 - 252, Separate the documentation comments for RefusesPersistentPermission and PermissionForArgs in OrchestrateTool. Keep the existing RefusesPersistentPermission explanation attached directly to that method, then add a distinct comment immediately before PermissionForArgs describing its write-capable-plan prompting behavior.internal/specialist/plan_tool.go-298-315 (1)
298-315: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDangling edit left in the doc comment.
Lines 312-315 contain a half-deleted sentence: "Previously this read: MaxWorkers is validated to be 1, … which is no longer true." followed by the orphan fragment "moment two tasks can run at once — see the note on the TUI plan recorder." Trim it to the claim that is still true.
✏️ Suggested trim
-// worker a consumer CANNOT attribute an event to a task, and the TUI stops -// trying rather than attributing every child to whichever task started last. -// Threading the child's identity through the loop's callback is what would fix -// it properly. Previously this read: MaxWorkers is validated to be 1, -// so exactly one task is in flight at any moment and the consumer can attribute -// events to the task it last saw dispatched — which is no longer true. -// moment two tasks can run at once — see the note on the TUI plan recorder. +// worker a consumer CANNOT attribute an event to a task, and the TUI stops +// trying rather than attributing every child to whichever task started last. +// Threading the child's identity through the loop's callback is what would fix +// it properly — see the note on the TUI plan recorder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_tool.go` around lines 298 - 315, Clean up the doc comment above runnerForCall by removing the dangling, half-deleted historical sentence and orphan fragment about MaxWorkers validation and concurrent tasks. Preserve only the still-valid explanation that the shared callback cannot identify individual tasks and that threading child identity through the loop callback would fix it.internal/specialist/plan_exec.go-627-635 (1)
627-635: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Summary()never reports the effective worker count, but the tool description promises it does.
plan_tool.goline 153 tells the model "The machine's own capacity may be lower and the report says which number applied", yet neitherSummary()nor theMetamap surfacesWorkers/WorkersRequested. A plan that asked for 16 and ran 6 reads identically to one that got 16 — the exact "fiction" the report fields were added to prevent.🔧 Surface the pair when they differ
fmt.Fprintf(&b, "sequential total: %s · critical path: %s · max_speedup: %.2fx\n", report.SequentialTotal.Round(time.Millisecond), report.CriticalPath.Round(time.Millisecond), report.MaxSpeedup) + if report.WorkersRequested > report.Workers { + fmt.Fprintf(&b, "workers: %d of %d requested (the machine could not carry more)\n", + report.Workers, report.WorkersRequested) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_exec.go` around lines 627 - 635, Update PlanReport.Summary to report the requested and effective worker counts when they differ, using the existing WorkersRequested and Workers fields. Keep the current summary unchanged when the values match, and ensure the output clearly distinguishes requested capacity from the number actually applied.internal/execprofile/zeromaxing_test.go-116-123 (1)
116-123: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTwo clause-slicing sites index
Delta's output without checkingstrings.Index. If either literal ever leaves the rendered delta,-1flows into a slice expression and the test panics instead of reporting a readable failure.
internal/execprofile/zeromaxing_test.go#L116-L123: capturestrings.Index(got, ", and that budget"),t.Fatalfwhen it is negative, then slice.internal/execprofile/zeromaxing_test.go#L180-L189: do the same forstrings.Index(got, "self-correct:")before slicing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/execprofile/zeromaxing_test.go` around lines 116 - 123, Guard both Delta clause-slicing sites against missing delimiters: at internal/execprofile/zeromaxing_test.go lines 116-123, capture the index of ", and that budget", fail with a readable t.Fatalf when it is negative, then slice; apply the same pattern at lines 180-189 for "self-correct:".internal/tui/zeromaxing_test.go-726-742 (1)
726-742: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis "third consumer" is a copy, so headless drift can't be detected here.
The doc comment claims
TestEffortSettabilityAgreesAcrossAllThreeConsumerspins this against the realforwardedReasoningEffort— it doesn't. That test compares this local re-implementation against the TUI, so ifinternal/cli's rule changes, both this helper and the test stay green while the surfaces diverge again. Either lift the rule into a shared package both sides call, or correct the comment so the gap is on the record rather than papered over.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/zeromaxing_test.go` around lines 726 - 742, Correct the misleading comment above forwardedEffortForTest: TestEffortSettabilityAgreesAcrossAllThreeConsumers does not compare this helper with internal/cli’s forwardedReasoningEffort, so it cannot detect headless drift. Document that this is a duplicated local implementation and explicitly note that divergence from the CLI rule is not covered, without changing the helper behavior.internal/tui/zeromaxing_glow_test.go-12-21 (1)
12-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the named enum values instead of raw
2/0.
m.zeromaxing = 2 // ZeromaxingActive(andm.zeromaxing = 0at line 472) hard-codes the ordinal ofagent.Zeromaxing. Reorder that enum and this fixture silently exercises a different state — thezeromaxingActive()guard on line 17 wouldn't catch it, since it's also true forEntering.♻️ Name the state
+ "github.com/Gitlawb/zero/internal/agent"- m.zeromaxing = 2 // ZeromaxingActive + m.zeromaxing = agent.ZeromaxingActive🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/zeromaxing_glow_test.go` around lines 12 - 21, Update the zeromaxing test fixtures in glowModel and the later setup assigning zero to use the named agent.Zeromaxing enum values instead of raw ordinals, preserving the intended active and inactive states without relying on numeric ordering.internal/tui/session_controls.go-316-320 (1)
316-320: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis arm can claim "NOT raised" when the posture never wanted to fill.
!supported && !filledalso covers the case where the session already holds an effort the user didn't set through/effort(e.g.Options.ReasoningEffort, soexecProfileEffortTouchedis false). The profile's fill was skipped because the slot was occupied, not because the model refused — but recordingwanthere makeseffortTransition()returnEffortNotSupported, and the status card then tells the user the model rejected a level it was never asked for.🐛 Only record a refusal when the fill was actually attempted
- case !supported && !filled: + case !supported && !filled && m.reasoningEffort == "": // Never filled and still unsupported: keep the reason fresh for the // destination model rather than leaving a stale one from the source. m.execProfileEffortUnraised = want🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/session_controls.go` around lines 316 - 320, Update the !supported && !filled arm in the effort transition logic to record unraised only when the profile fill was actually attempted, using the existing effort-touch/occupancy state to distinguish skipped fills from model refusals. Leave the refusal status unset when an existing effort such as Options.ReasoningEffort occupied the slot without an /effort request, so effortTransition() does not report EffortNotSupported.internal/sandbox/scope_temporary_test.go-17-35 (1)
17-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a Linux-safe outside-root probe
go test ./...runs onubuntu-latest, but this helper only probes/Users/Sharedand/var/empty, so the file can skip on Linux CI. Reuse the existingtempDirOutsideDefaultTemppattern or add a GOOS-aware candidate list here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/scope_temporary_test.go` around lines 17 - 35, Update scopeOutsideRoots to use the existing tempDirOutsideDefaultTemp pattern or a GOOS-aware list of writable directories, including a Linux-safe candidate outside the default temporary root. Preserve the current fallback behavior, cleanup, workspace/outside directory creation, and skip only when no suitable candidate is available.internal/cli/plan_background.go-72-84 (1)
72-84: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA panicking background plan vanishes silently — no failure surfaces to the bridge/panel.
defer func() { _ = recover() }()(Line 75) correctly stops a panic from taking down the session, but the recovered value is discarded with no report tolauncher.bridgeor any log. Combined with the "slot frees" cleanup on Lines 76-82, the plan simply disappears from the user's perspective —TestAPanickingBackgroundPlanIsContainedAndFreesTheSlotonly asserts the slot frees, not that the panel/user is told anything failed. A user watching the orchestrate panel has no way to distinguish "plan finished" from "plan crashed."Consider reporting the recovered panic through the bridge (e.g., a synthetic
PlanCompleted/failure event) so the panel reflects the true outcome instead of going stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/plan_background.go` around lines 72 - 84, Update the panic recovery defer in the background plan goroutine to capture the recovered value and report the plan failure through launcher.bridge using the existing completion or failure event mechanism. Preserve the current panic containment and cleanup behavior in the adjacent defer, ensuring the panel receives a failure outcome before the running slot is released.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/app.go`:
- Around line 723-726: Reorder the shutdown defers in the app setup around
newPlanLauncher and closeSpecialistRuntime so planLaunch.Close executes first
and fully drains any background plan before the specialist runtime is closed.
Since defers run LIFO, register the runtime cleanup before defer
planLaunch.Close, preserving the existing cleanup behavior otherwise.
In `@internal/sandbox/scope.go`:
- Around line 199-242: Update releaseTemporaryRead and releaseTemporaryWrite to
decrement the reference count and remove the corresponding root while holding
the same s.mu critical section, avoiding unlock-then-relock through
removeReadRoot/removeWriteRoot; preserve the existing helpers for other callers.
Add a targeted interleaving regression test, extending
TestConcurrentHoldersOfOneRoot or equivalent, that starts
AddTemporaryRead/AddTemporaryWrite during the final release and verifies the new
holder retains the root and receives a functional undo, then run the affected
test under the race detector.
In `@internal/specialist/plan_concurrent_test.go`:
- Around line 87-117: Update TestIndependentTasksRunConcurrently to compare the
peak-concurrency expectation against the effective worker count returned by the
plan’s worker-sizing logic, rather than the requested four tasks. Preserve the
existing release-channel closure and report assertions, including the timeout
behavior when the effective concurrency is not reached.
In `@internal/specialist/plan_resume.go`:
- Around line 54-58: Update PlanProgress terminal-state reduction so each task
has one deduplicated outcome: when a later success is recorded, remove that task
from Failed, and ensure duplicate entries do not remain in either list. Then
derive Done from the same remaining-work state used by Remaining, preserving
consistent no-op behavior when every task has reached a terminal outcome.
In `@internal/specialist/plan_runner.go`:
- Around line 150-193: Update planTaskManifest to detect whether grantedTools
intersects planWriteTools, and make both Manifest.Metadata.Description and
SystemPrompt reflect that capability. Write-capable tasks must not be labeled
read-only or instructed not to modify files, while tasks without write tools
must retain the existing read-only restrictions. Update the nearby “Phase 2
tasks are read-only” comment to match the conditional behavior.
In `@internal/specialist/plan_store.go`:
- Around line 127-136: Replace the fixed `path + ".tmp"` write in the plan-save
function with a uniquely named temporary file created via `os.CreateTemp` in the
same directory, preserving restrictive permissions. Write and close the returned
file before renaming it to `path`, remove the temporary file on any failure, and
retain the existing `refuseSymlink` protections and error propagation.
In `@internal/specialist/plan_tool.go`:
- Around line 424-432: Update resolveSavedPlan and its caller so caller-supplied
control flags, especially background, survive replacement with stored.Args.
Preserve the stored plan arguments while explicitly carrying the invocation’s
allowed control flags into the resolved map, so planBool in ParsePlan observes
background=true for saved plans.
In `@internal/specialist/plan_watchdog_test.go`:
- Around line 299-347: Make TestAChattyChildOutlivesItsStallTimeout exercise at
least one watchdog poll by extending the simulated child runtime beyond
watch()’s one-second minimum interval while continuing to emit progress, or add
a configurable poll interval through PlanTaskRequest/PlanRunner for
millisecond-scale testing. Preserve the test’s assertion that regular progress
prevents the 60ms stall timeout from terminating the child.
In `@internal/specialist/plan.go`:
- Around line 155-167: Update Plan.Tasks to deep-copy each Task’s slice fields,
including DependsOn and Tools, rather than only copying the Task structs;
preserve the existing copy isolation for scalar fields and ensure mutations to
returned nested slices cannot alter the validated plan.
- Around line 593-607: Update planStrings to return an error alongside the
parsed strings, rejecting non-string entries and empty or whitespace-only
strings instead of silently skipping them. Propagate that error through planTask
so malformed depends_on or tools values fail closed and no truncated Task is
constructed.
In `@internal/tui/mouse.go`:
- Around line 185-190: Sequence the hover update and spinner scheduling in the
surrounding mouse-update flow instead of returning hovered.ensureSpinnerTick()
as a combined expression. Assign the result of updateHoverTarget to hovered,
call ensureSpinnerTick on that value, then return the updated model so its
spinnerTicking bookkeeping is preserved reliably.
In `@internal/tui/orchestrate_panel.go`:
- Around line 449-471: Update the row-width calculation around the styled glyph
and head so remaining is based on terminal display width rather than rune counts
that include ANSI escape sequences. Measure the unstyled head (or use
lipgloss.Width consistently) while preserving the existing summary truncation
and rendering behavior in the task row formatting logic.
In `@internal/tui/render_cache.go`:
- Around line 176-202: Update specialistCacheFingerprint to include
specialistInfo.tokenCount and specialistInfo.result, ensuring mutations from
setTokens and setResult invalidate cached renders. Extend
TestSpecialistCacheKeyCoversEveryVaryingField in the plan progress tests with
distinct tokenCount and result cases.
In `@internal/tui/zeromaxing_glow.go`:
- Around line 212-225: Update zeromaxingChipSpan to convert the label’s byte
offset from strings.Index into a rune/column offset before subtracting the badge
prefix. Return span coordinates in the same mouseX(msg) column units so
multi-byte glyphs such as ●, ◎, and ↻ do not shift the clickable chip area.
---
Outside diff comments:
In `@internal/specialist/exec.go`:
- Line 608: Update the error return in runBuiltArgs to include the TotalTokens
value from the already-computed summary, matching the success path’s token
accounting. Preserve the existing SessionID and error behavior while ensuring
failed or crashed child executions report tokens consumed before failure.
---
Minor comments:
In `@internal/cli/plan_background.go`:
- Around line 72-84: Update the panic recovery defer in the background plan
goroutine to capture the recovered value and report the plan failure through
launcher.bridge using the existing completion or failure event mechanism.
Preserve the current panic containment and cleanup behavior in the adjacent
defer, ensuring the panel receives a failure outcome before the running slot is
released.
In `@internal/execprofile/zeromaxing_test.go`:
- Around line 116-123: Guard both Delta clause-slicing sites against missing
delimiters: at internal/execprofile/zeromaxing_test.go lines 116-123, capture
the index of ", and that budget", fail with a readable t.Fatalf when it is
negative, then slice; apply the same pattern at lines 180-189 for
"self-correct:".
In `@internal/sandbox/scope_temporary_test.go`:
- Around line 17-35: Update scopeOutsideRoots to use the existing
tempDirOutsideDefaultTemp pattern or a GOOS-aware list of writable directories,
including a Linux-safe candidate outside the default temporary root. Preserve
the current fallback behavior, cleanup, workspace/outside directory creation,
and skip only when no suitable candidate is available.
In `@internal/specialist/plan_exec.go`:
- Around line 627-635: Update PlanReport.Summary to report the requested and
effective worker counts when they differ, using the existing WorkersRequested
and Workers fields. Keep the current summary unchanged when the values match,
and ensure the output clearly distinguishes requested capacity from the number
actually applied.
In `@internal/specialist/plan_retry_test.go`:
- Around line 329-340: Replace the callback t.Fatal in
internal/specialist/plan_retry_test.go:329-340 with a recorded missing-cancel
condition, then assert that condition after ExecutePlan returns; update the
RunTask closure in internal/specialist/plan_worktree_test.go:171-186 to set a
flag checked by the test, or remove the unused fixture.
In `@internal/specialist/plan_store_test.go`:
- Around line 550-556: Update the test around tool.Run and launched to assert
that Run succeeds and launched is non-nil before invoking it, matching the guard
used by the sibling test. Preserve the existing launch invocation after this
assertion so a refused plan produces a readable test failure instead of a
nil-function panic.
In `@internal/specialist/plan_store.go`:
- Around line 188-193: Update loadPlanDir so only an os.ReadDir error indicating
the directory does not exist returns nil, nil; propagate or record all other
failures in the problems result, including permission and I/O errors, while
preserving normal directory loading.
In `@internal/specialist/plan_tool.go`:
- Around line 225-252: Separate the documentation comments for
RefusesPersistentPermission and PermissionForArgs in OrchestrateTool. Keep the
existing RefusesPersistentPermission explanation attached directly to that
method, then add a distinct comment immediately before PermissionForArgs
describing its write-capable-plan prompting behavior.
- Around line 298-315: Clean up the doc comment above runnerForCall by removing
the dangling, half-deleted historical sentence and orphan fragment about
MaxWorkers validation and concurrent tasks. Preserve only the still-valid
explanation that the shared callback cannot identify individual tasks and that
threading child identity through the loop callback would fix it.
In `@internal/specialist/plan_worktree.go`:
- Around line 8-17: Update the plan-engine documentation across
internal/specialist/plan_worktree.go lines 8-17 and 56-61 to remove the obsolete
“nothing requires isolation” and future step-3 framing; revise
internal/specialist/plan.go lines 13-22 to describe worker-slot scheduling
instead of sequential execution and rename writeToolMarkers to planReadOnlyTools
at lines 114-116; delete the outdated write-tool rejection text above
TestAWriteToolIsPermittedOnlyWhenTheParentHoldsIt in
internal/specialist/plan_test.go line 306.
In `@internal/specialist/plan.go`:
- Around line 483-494: The plan parser’s max_wall_seconds handling lacks
negative-value rejection and a minimum timeout. Update the validation around
planInt and the budget.MaxWall assignment to reject negative values and enforce
the appropriate small wall-time floor, matching the existing max_tokens and
max_stall_seconds validation patterns while preserving valid-budget assignment.
- Around line 409-417: Update planTooLargeError so the raise-it configuration
guidance is appended only when limits.MaxTasks is below
config.PlanSizeLarge.MaxTasks(); for runs already at the large tier, return an
error that advises splitting the plan without suggesting the unchanged large
setting. Preserve the existing source-specific context and task-count details.
In `@internal/tui/model.go`:
- Around line 2449-2452: Update the orchestrate freeze handling around
m.orchestrate.frozenAt so background plans are not permanently frozen when they
continue emitting planTaskStartMsg or planTaskProgressMsg after the run ends.
Apply the freeze only to foreground runs, or clear frozenAt when background task
activity resumes, while preserving the existing behavior for foreground plans.
In `@internal/tui/mouse.go`:
- Around line 90-106: Guard the posture-chip click branch identified by
zeromaxingChipAtMouse with the same setup, wizard, mcpManager, picker, and
suggestions overlay checks used by orchestrateTaskAtMouse. Return without
opening newEffortPicker when any overlay is active, preserving the existing
pending-turn behavior otherwise.
In `@internal/tui/orchestrate_control_test.go`:
- Around line 374-383: Update the assertion in the test around
model.Update(done) to verify the observable completion effect: assert that the
completed task’s status is reflected on the panel. Do not use the current
frozenAt/isEmpty conjunction, since admit installs a task and makes isEmpty()
false regardless of whether the terminal message is processed.
In `@internal/tui/orchestrate_panel_test.go`:
- Around line 474-477: Correct the doc comment above
TestCtrlGTogglesTheOrchestratePanel to refer to Ctrl+G instead of Ctrl+O,
keeping the test name and key-handler behavior unchanged.
In `@internal/tui/orchestrate_panel.go`:
- Around line 534-544: Update orchestratePlainTaskLine to cap task.depth with
orchestrateMaxIndentDepth before calculating the repeated indentation, matching
renderOrchestrateTaskLine while preserving the rest of the row formatting.
- Around line 347-353: Update the explanatory comment above the !state.expanded
check to say that Ctrl+G expands the details, matching the header text and
actual keybinding; leave the collapse behavior unchanged.
In `@internal/tui/plan_progress.go`:
- Around line 309-325: The PlanProgressBridge completion flow must clear
lastPlanName when the current plan ends so RunningPlanName cannot reuse a
previous plan’s name for a newly launched, unadmitted plan. Update PlanCompleted
to reset lastPlanName while preserving lastPlan for /plans save and keeping
RunningPlanName’s “a plan” fallback unchanged.
In `@internal/tui/session_controls.go`:
- Around line 316-320: Update the !supported && !filled arm in the effort
transition logic to record unraised only when the profile fill was actually
attempted, using the existing effort-touch/occupancy state to distinguish
skipped fills from model refusals. Leave the refusal status unset when an
existing effort such as Options.ReasoningEffort occupied the slot without an
/effort request, so effortTransition() does not report EffortNotSupported.
In `@internal/tui/sidebar_plan_detail_test.go`:
- Around line 219-231: The 30-task rounding test creates duplicate task IDs
after 26 iterations, so admission may retain fewer than 30 tasks. Update the
task construction in the big.orchestrate test setup to generate a unique id for
every index while preserving msg.taskCount and the existing failure/progress-bar
assertions.
In `@internal/tui/sidebar_plan_detail.go`:
- Around line 193-213: Sanitize all untrusted sidebar text before rendering in
the plan-detail flow: apply sanitizeCardText to activity assembled from
info.currentTool/currentDetail, task.summary, and the outcome text produced by
orchestrateOutcomeLine (including info.errorMsg via firstLineOf). Keep
truncation and existing layout behavior unchanged, using the sanitized values as
input to truncateStep.
In `@internal/tui/specialist_card.go`:
- Around line 28-33: Update the specialistCancelled handling in
renderSpecialistCard so cancelled tasks use the finished/non-live header
rendering instead of the default accent bullet. Update renderSpecialistSummary
to count cancelled specialists in the appropriate completed/done total while
leaving running counts unchanged, so every card in len(specialists) is
represented in the summary.
In `@internal/tui/zeromaxing_glow_test.go`:
- Around line 12-21: Update the zeromaxing test fixtures in glowModel and the
later setup assigning zero to use the named agent.Zeromaxing enum values instead
of raw ordinals, preserving the intended active and inactive states without
relying on numeric ordering.
In `@internal/tui/zeromaxing_test.go`:
- Around line 726-742: Correct the misleading comment above
forwardedEffortForTest: TestEffortSettabilityAgreesAcrossAllThreeConsumers does
not compare this helper with internal/cli’s forwardedReasoningEffort, so it
cannot detect headless drift. Document that this is a duplicated local
implementation and explicitly note that divergence from the CLI rule is not
covered, without changing the helper behavior.
---
Nitpick comments:
In `@internal/cli/exec_zeromaxing_test.go`:
- Around line 38-63: Update TestZeromaxingDoesNotOverrideModeFillsCLI to assert
that the fast mode fixture actually sets non-empty reasoningEffort and positive
maxTurns immediately after applyExecMode; then perform the precedence checks
unconditionally, preserving the existing expected-value comparisons and failure
messages.
In `@internal/cli/plan_isolate.go`:
- Around line 21-26: Provide an operator-facing cleanup path for worktrees
retained by Release, such as documenting periodic removal or adding a `zero
worktrees` cleanup command. Ensure the guidance or command identifies the
workspace worktree store and allows accumulated, no-longer-needed plan worktrees
to be reclaimed without changing the safety behavior that preserves newly
written work.
In `@internal/sandbox/scope_temporary_test.go`:
- Around line 139-172: Add a write-side nested-grant test alongside
TestATemporaryWriteSurvivesASiblingsCleanup, mirroring
TestANarrowerRequestHoldsTheCoveringGrant: create a broader temporary write
grant, request a narrower covered path, clean up the broader grant, and verify
the narrower grant still preserves coverage until its own cleanup. Use
AddTemporaryWrite and the existing scope.Roots coverage checks to exercise map
iteration order.
- Around line 178-228: Extend TestConcurrentHoldersOfOneRoot with a coordinated
iteration that starts a new AddTemporaryRead(outside) while another holder’s
undo() is in progress, using synchronization to overlap the operations within
the releaseTemporaryRead window. Assert the new grant remains visible throughout
its hold and that the root is removed only after all holders, including the
newly added one, release it.
In `@internal/specialist/exec.go`:
- Around line 415-438: The resumed inline manifest is not validated against the
session’s specialist identity. In runResume, after resolveManifest returns,
validate that a supplied manifest’s Metadata.Name matches specialistName (or
enforce this within resolveManifest using the resumed expected name), regardless
of whether params.Name was provided; reject mismatches before execution. Also
inspect the production plan_runner.go caller to confirm whether
TaskParameters.Name always matches the inline manifest Metadata.Name, and
preserve the guard even if callers currently do.
In `@internal/specialist/plan_concurrent_test.go`:
- Around line 46-54: Update fanOutPlan’s task ID generation to use a formatted,
explicitly valid identifier for every index instead of converting 'a'+i to a
rune. Preserve uniqueness across all n values and keep the existing mustPlan
setup unchanged.
- Around line 335-342: Update goroutineLabel to bound the stack-header slice by
the number of bytes actually written, returning up to 20 bytes without panicking
when runtime.Stack produces a shorter result. Preserve the existing 20-character
label behavior when sufficient data is available.
In `@internal/specialist/plan_grant_test.go`:
- Around line 248-275: The grant-plan table test should explicitly encode
whether each case is expected to refuse dispatch, rather than skipping all
converse checks on any grant error. Add a per-case wantGrantErr expectation,
assert grantErr matches it, and ensure only the intentionally ungrantable “no
request, parent holds only mutators” case permits refusal while admission
succeeds; keep the existing granted-tool validation for successful cases.
In `@internal/specialist/plan_progress_test.go`:
- Around line 156-176: Extend the test around runnerForCall to assert that the
child arguments include the parent reasoning effort supplied through
tools.RunOptions.ReasoningEffort: "high". Keep the existing model and session
assertions unchanged, and verify the corresponding high reasoning-effort
argument so regressions dropping ParentReasoningEffort are detected.
In `@internal/specialist/plan_resume_test.go`:
- Around line 76-83: Update the test around ReducePlanEvents and
progress.Remaining() to first verify the returned slice contains an element,
failing with a descriptive message that includes the actual slice when it is
empty; only index got[0] after that guard, while preserving the existing
expectation that the first remaining task is "a".
In `@internal/specialist/plan_test.go`:
- Around line 485-486: Remove the import-keepalive declarations for errors.New
and time.Second in plan_test.go, since errors is already used by the test and
time is unnecessary. Delete the corresponding time import while preserving the
existing errors usage.
In `@internal/specialist/plan_tool.go`:
- Around line 398-411: Remove the unused options parameter from
OrchestrateTool.limits and update every call site to invoke it without
arguments. Keep the existing tool.Size and tool.Depth-based limit calculation
unchanged.
- Around line 490-505: Merge the consecutive !launched checks in the plan-launch
flow around tool.Launch: release workspace within the single refusal branch
before returning the error result. Preserve the successful launch path and
existing shutdown/already-running error response.
In `@internal/specialist/plan_worktree_test.go`:
- Around line 171-186: Update TestTheToolRefusesAnUnisolatableWritePlan to
exercise the foreground dispatch through RunWithOptions using the configured
tool and write plan, ensuring the test reaches PostureActive and RunTask;
alternatively, remove the unused fixture fields and rename the test to reflect
that it only validates resolvePlanWorkspace with a nil isolator.
In `@internal/specialist/plan.go`:
- Around line 13-22: Update the stale comments in the plan implementation:
revise the package-level ZeroMaxing Phase 2 description to match the concurrent
scheduler and max_workers behavior allowed by planBudget, and correct the
comment above writeToolMarkers to document planReadOnlyTools. Remove outdated
references to sequential execution and obsolete phase hooks while preserving the
current authority rules.
In `@internal/tui/permission_detail.go`:
- Around line 166-188: Update sanitizeCardText to remove
bidirectional-formatting and zero-width/invisible Unicode runes, including
U+202E and U+200B-class characters, in addition to the existing
control-character filtering. Preserve printable text, tab-to-space conversion,
line truncation, and final trimming.
In `@internal/tui/plan_durability_test.go`:
- Around line 247-269: Throttle the concurrent loops in the durability test
around bridge.TaskProgress by adding a small scheduler yield or equivalent
bounded coordination inside the loop. Preserve overlapping progress calls with
later TaskDispatched operations, while avoiding the unbounded default-branch
busy-wait that creates excessive contention under the race detector.
In `@internal/tui/plan_messages.go`:
- Around line 118-120: Update the doc comment immediately above planAdmittedLine
so it starts with the declared function name, replacing the incorrect
planNoticeLine reference while preserving the existing description.
In `@internal/tui/session_controls.go`:
- Around line 162-168: Update effortText and its effort-resolution flow to
resolve the available reasoning-effort ring once per card render, then reuse
that result for the efforts list, known-state check, and settableEfforts
behavior. Refactor availableReasoningEfforts, availableReasoningEffortsKnown,
and settableEfforts as needed so they share the resolved registry/ring instead
of independently calling modelregistry.DefaultRegistry().
In `@internal/tui/sidebar_plan_test.go`:
- Around line 24-44: The test TestFileClickOffsetsSurviveThePlanSection
currently checks only sidebarPlanLines output and never verifies file click
offsets. Update it to call sidebarFileSelectables for models with and without
the plan, then assert the resulting file hits shift by exactly
len(withPlan.sidebarPlanLines(34)), while preserving the existing empty-plan
validation.
In `@internal/tui/sidebar_test.go`:
- Around line 822-829: Strengthen the cancelled-task test around
sidebarAgentExpansion by first validating that m.sidebarSpecialists() is
non-empty before indexing element zero, reporting a test failure instead of
panicking. Extract the cancelled row and assert it contains no zeroTheme.red SGR
escape sequence, rather than checking only for the exact styled word, while
preserving the existing “cancelled” text assertion.
In `@internal/tui/sidebar.go`:
- Around line 205-243: Extract the shared filtering logic into helpers matching
the proposed hiddenNotFoundAgent and m.agentPastLinger predicates, then use
those helpers in both sidebarSpecialists and doneAgentCount. Preserve the
existing not-found exclusion and linger-expiry behavior so the displayed rows
and done count remain synchronized.
In `@internal/tui/zeromaxing_glow_test.go`:
- Around line 315-317: Update the underline assertion in the hovered-chip test
to remove the broad "4;" substring check, which can match unrelated truecolor
values. Validate that the hovered output contains an actual SGR underline
parameter alongside the existing relevant checks, so chips without underline
styling fail reliably.
In `@internal/worktrees/run_git_unix.go`:
- Line 16: Move the shared worktreeWaitDelay declaration and its documentation
out of run_git_unix.go and run_git_windows.go into a build-tag-free shared file
such as worktrees.go. Remove both platform-specific copies while leaving the
platform-specific hardenWorktreeGit implementations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
wow this is great feature |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Went through the whole branch properly — posture machinery, the DAG engine, durability, the TUI surface. Nice work, Kratos. The one-scheduler design and the event-reduction durability model are the right shape, and the mutation-tested tests show.
A handful of things to look at before this comes out of draft. The big one is the first inline below — the /plans resume narrowing bug silently drops real work, I'd fix that before anything else. The unsanitized plan strings and the background-plan panel wipe are next. The rest are smaller correctness nits and a couple of stale strings.
On the split you offered at the end of the description: the seams you listed look right to me, happy to re-review per slice if you go that way.
| if err != nil { | ||
| return "", planControlNotice("warning", "Could not read this session's events: "+err.Error()), false | ||
| } | ||
| progress, found := specialist.ReducePlanEvents(events) |
There was a problem hiding this comment.
This is the one I'd fix first. ReducePlanEvents keeps only the last admitted plan's progress, but here we narrow whichever saved plan the user named by that progress — without checking the two are the same plan. Run saved plan A, then plan B, then /plans resume A: every A task whose id happens to sit in B's succeeded set (ids like "tests"/"lint" collide across plans all the time) gets silently dropped and never runs — and the notice then reports B's counts against A's name. progress.Name/order are reduced and right there; can we match them against the plan being resumed and refuse on mismatch?
| if index := strings.IndexAny(summary, "\r\n"); index >= 0 { | ||
| summary = summary[:index] | ||
| } | ||
| return truncateRunes(summary, planTaskSummaryWidth) |
There was a problem hiding this comment.
planTaskSummary cuts at the first newline but leaves every other control byte in, and these strings are model-authored — so ESC/OSC sequences survive into the inline panel, the sidebar rows, and the detail pane. The realistic path is indirect prompt injection (poisoned file/web content echoed into orchestrate args) repainting someone's terminal. We added sanitizeCardText in permission_detail.go for exactly this data class in this same PR, and sidebar.go already scrubs child output — these plan surfaces need the same treatment. planString (plan name) has the same hole, and so do the first-lines in /plans list and /plans show.
| // previous turn don't bleed into the new one. | ||
| m.specialists.clear() | ||
| m.plan.clear() | ||
| m.orchestrate.clear() |
There was a problem hiding this comment.
beginRun clears the panel unconditionally, but background plans are built to outlive the run that launched them — every bridge message carries the background flag precisely to survive the stale-run guard. After this clear, that plan's start/done messages pass the guard but no-op against an empty byID, planAdmitted never re-fires, and the PLAN surface silently vanishes for the rest of the plan's life while it keeps running and spending. (The adjacent specialists.clear drops its AGENTS rows mid-flight too.) Can we skip the clear — or re-admit from the bridge — when a background plan is in flight?
| // This delegates to handleProfileCommand rather than re-applying the knobs, | ||
| // so "/effort zeromaxing and /profile zeromaxing resolve identically" is | ||
| // true by construction instead of by two implementations a test hopes agree. | ||
| if args == execprofile.Name { |
There was a problem hiding this comment.
/effort zeromaxing goes straight through with no m.pending check, but it routes into handleProfileCommand — which mutates the turn budget, self-correct, and the shared orchestrate gate. That's the same mutation /profile refuses mid-run ("Finish or stop the current run before switching the execution profile"), and the /turns comment above explains why: the budget propagates to sub-agents spawned later in the same run via ZERO_MAX_TURNS, so a mid-run switch makes a live run inconsistent — here it also arms the orchestrate tool for a run that started without the posture. Same guard here?
| delete(s.tempReads, root) | ||
| s.mu.Unlock() | ||
| s.removeReadRoot(root) |
There was a problem hiding this comment.
The refcount delete and the slice removal happen under different lock holds — delete(s.tempReads, root), unlock, then removeReadRoot re-acquires. A concurrent AddTemporaryRead for the same root landing in that window sees the root still present in readRoots but no longer tracked, misclassifies it as permanent, and returns a no-op undo — then removeReadRoot strips the root and the new holder's already-approved tool call gets sandbox-denied. That's exactly the parallel-batch shape this refcounting exists to fix (batched reads granted the same outside-workspace dir). Same issue in releaseTemporaryWrite below. Can we do the removal under the same lock hold?
| { | ||
| "id": "by_name", | ||
| "phase": "search", | ||
| "prompt": "Find where THE SUBJECT is DEFINED. Search by identifier: type names, function names, constants, struct fields. Report every definition site as file:line with a one-line description. If you find nothing, say so plainly — a wrong guess is worse than an empty result." |
There was a problem hiding this comment.
Two problems with shipping this as runnable: "THE SUBJECT" is never substituted (resolveSavedPlan refuses args alongside saved), so /plans run research spends five child agents searching for a placeholder. And synthesise/refute assume upstream outputs reach dependents — "Using the three searches above" — but NewPlanRunner passes task.Prompt verbatim, so a dependent child never sees earlier results and has to redo or hallucinate the synthesis. Either make the example concrete and self-contained, or stop advertising it in /plans run.
| } | ||
|
|
||
| // Done reports whether there is anything left to run. | ||
| func (progress PlanProgress) Done() bool { |
There was a problem hiding this comment.
Done and Remaining look like dead API — only the tests call them; the actual resume path narrows via RemainingPlan and never touches either. Wire them in (Done to short-circuit a fully-succeeded resume?) or drop them.
| sections = append(sections, style) | ||
| } | ||
| policy := strings.TrimSpace(confirmationPolicy) | ||
| if !runCanMutate(options) { |
There was a problem hiding this comment.
Heads-up on the headline invariant: runCanMutate is evaluated unconditionally, so any all-read-only run (e.g. zero exec --enabled-tools read_file,grep, or a read-only specialist child) now gets a system prompt ~5KB smaller than a build without the feature — posture off. The drop itself is deliberate and fails closed, that's fine — but "byte-identical with the posture off" is false as written, and posture_off_identity_test can't see this delta (it only proves registering the tool changes nothing). Either narrow the claim or make the drop opt-in.
|
|
||
| // orchestrateHeaderAtMouse reports a click on the sidebar's PLAN header, which | ||
| // collapses and expands the whole section. | ||
| func (m model) orchestrateHeaderAtMouse(msg tea.MouseMsg) bool { |
There was a problem hiding this comment.
This hit-tester is missing the modal guard its siblings all carry — sidebarAvailable deliberately excludes suggestionsActive, with the in-code note that each sidebar hit-tester "carries its own suggestionsActive() guard", but this one refuses nothing. Clicking the PLAN header while the / palette is open toggles sidebarCollapsed behind the overlay.
| // Clicking the posture chip opens /effort, where it can be turned off or | ||
| // changed. A chip that highlights under the cursor and then does nothing | ||
| // when pressed is worse than one that never highlighted. | ||
| if mouseLeftPress(msg) && m.zeromaxingChipAtMouse(msg) { |
There was a problem hiding this comment.
This branch runs before the modal switch and only checks m.pending — the hit-testers added right beside it guard m.picker, the wizards, and suggestionsActive, but this one doesn't. With the /model picker or a provider wizard open, clicking the chip silently swaps in a fresh effort picker and discards the open picker's loaded/typed state. Same guard here?
|
@gnanam1990 took the top three from my review and pushed them as a commit you can cherry-pick — git fetch https://github.com/Gitlawb/zero.git fix/829-resume-identity-and-panel && git cherry-pick 5e9a8a79Merge it, take pieces, or ignore it entirely and write it your way — no attachment to the implementation. The resume bug is the one that mattered, and it's worse than I first described. One detail worth your eye. The check compares Task summaries go through
Three mutations, all killed. Untouched from the review, in case you'd rather split them: the |
…panels Three from the review on Gitlawb#829. RESUME NARROWED BY THE WRONG PLAN'S PROGRESS. ReducePlanEvents keeps only the LAST admitted plan's state and records whose it is in PlanProgress.Name, but resumeSavedPlan narrowed whichever plan the user named by that progress without checking the two match. Run plan A, run plan B, then /plans resume A: every A task whose id sits in B's succeeded set is dropped from the remainder and never runs. Ids like "tests" or "lint" collide across plans constantly, so this is the ordinary case rather than a contrived one, and it fails silently — the remainder validates and runs, just without the work. The comparison is against plan.Name(), not stored.Name: plan_admitted records the plan's own name, which is independent of the name it was saved under. A test asserting the saved name would have passed while breaking every legitimate resume, so both directions are pinned. TASK SUMMARIES CARRIED CONTROL BYTES. planTaskSummary cut at the first newline and left every other control byte intact, and these strings are model-authored — the realistic path is indirect prompt injection, poisoned file or web content echoed into orchestrate args and painted into the inline panel, the sidebar rows and the detail pane. It now goes through sanitizeCardText, which permission_detail.go already applies to exactly this class of data. BACKGROUND PLANS LOST THEIR PANEL. beginRun cleared the orchestrate panel unconditionally, but background plans are built to outlive the run that launched them and every message they post carries the background flag precisely to pass the stale-run guard. After the clear those messages passed the guard and no-oped against an empty byID, so the PLAN surface vanished for the rest of the plan's life while it kept running. BackgroundPlanLive gates the clear, consulting background alongside cancelPlan for the same reason RunningPlanName does — the launcher sets one synchronously and the goroutine sets the other later, so reading either alone leaves a window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed that commit straight onto your branch rather than leaving it for you to cherry-pick — Carrying on with the rest of the review list; I'll push them as separate commits so any one is easy to drop. |
|
It's real, and I want to show the reproduction rather than assert it, because it took me several wrong turns to pin down. Driving the two mutations apart and stepping a second caller through the gap:
The tests are weaker than they look and I'd rather say so than let them read as a guarantee. Neither reproduces the window on demand. The concurrent one can't hit a gap that narrow reliably — it survived the unfixed code even with an injected Worth passing on, since it'll bite anyone testing this area: my first two attempts used That's the two correctness items from the review done. Remaining, if you want another slice: the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/scope_temp_refcount_test.go (1)
67-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the split-lock regression deterministic.
This loop increases contention but does not force
AddTemporaryReadinto the former gap between removingtempReads[root]and removingreadRoots. The previous implementation can pass depending on scheduling.Add a package-private test synchronization hook around that transition. Pause release at the hook, start a second
AddTemporaryRead, then assert that it cannot return until the root-list update completes. Rungo test -race ./internal/sandboxafter adding the test.As per coding guidelines, “Run affected concurrent code under the race detector.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/scope_temp_refcount_test.go` around lines 67 - 102, Make TestTemporaryReadGrantsSurviveConcurrentReleases deterministic by adding a package-private synchronization hook around the temporary-read release transition that updates tempReads and readRoots. Pause the first undo at this hook, start a second AddTemporaryRead, assert it remains blocked until the root-list update completes, then resume and verify the grant succeeds; run go test -race ./internal/sandbox.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/sandbox/scope_temp_refcount_test.go`:
- Around line 67-102: Make TestTemporaryReadGrantsSurviveConcurrentReleases
deterministic by adding a package-private synchronization hook around the
temporary-read release transition that updates tempReads and readRoots. Pause
the first undo at this hook, start a second AddTemporaryRead, assert it remains
blocked until the root-list update completes, then resume and verify the grant
succeeds; run go test -race ./internal/sandbox.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 657e9d30-988e-47dd-8dfd-e7cfc5634ac7
📒 Files selected for processing (2)
internal/sandbox/scope.gointernal/sandbox/scope_temp_refcount_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/scope.go
|
Two more commits,
The posture chip in
The approval card said "read-only specialist sub-agents", but And the posture-off test claimed byte-identical-to-a-build-without-the-feature. It isn't: Left for you, on purpose — these are design calls, not defects, and I didn't want to make them unilaterally on your branch:
Everything green apart from |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/specialist/plan_tool.go (1)
212-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale safety rationale.
The new
Reasoncorrectly states that tasks may receive write tools. However, the surroundingSafety()comments at Lines 197-206 still state that tasks are read-only and that write approval is future work.PermissionForArgsnow handles write-capable plans. Update those comments to describe the current approval flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_tool.go` around lines 212 - 216, Update the Safety() comments near the specialist plan approval logic to remove the outdated read-only and future-write-approval claims. Describe that PermissionForArgs evaluates plan arguments and prompts for approval when tasks may use write-capable tools, consistent with the current behavior and the Reason text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/specialist/plan_budget_negative_test.go`:
- Around line 34-48: Update TestBudgetStillAcceptsAbsentAndZeroTimeouts to
retain the plan returned by ParsePlan and assert that both Budget.MaxWall and
Budget.MaxStall remain zero for the absent and explicit-zero inputs, while
preserving the existing no-error assertion.
In `@internal/specialist/plan.go`:
- Around line 490-510: Validate max_wall_seconds in the budget parsing flow
before converting seconds to time.Duration, rejecting values that exceed the
maximum representable positive duration. Preserve acceptance of 9223372036 and
reject 9223372037, and add focused tests covering both cases near the existing
max_stall_seconds validation.
In `@internal/tui/zeromaxing_guards_test.go`:
- Around line 58-61: Update the sanity check around orchestrateHeaderAtMouse in
the test to call t.Fatal instead of t.Skip when the calculated click misses the
PLAN header, ensuring fixture or layout errors fail the test and the modal guard
remains covered.
---
Nitpick comments:
In `@internal/specialist/plan_tool.go`:
- Around line 212-216: Update the Safety() comments near the specialist plan
approval logic to remove the outdated read-only and future-write-approval
claims. Describe that PermissionForArgs evaluates plan arguments and prompts for
approval when tasks may use write-capable tools, consistent with the current
behavior and the Reason text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6790f934-b9d9-4d3a-91ca-e5a31a1052e3
📒 Files selected for processing (8)
internal/agent/posture_off_identity_test.gointernal/specialist/plan.gointernal/specialist/plan_budget_negative_test.gointernal/specialist/plan_tool.gointernal/tui/mouse.gointernal/tui/session_controls.gointernal/tui/sidebar_plan_detail.gointernal/tui/zeromaxing_guards_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/tui/mouse.go
- internal/tui/sidebar_plan_detail.go
- internal/agent/posture_off_identity_test.go
- internal/tui/session_controls.go
|
Ran a design-level audit on top of the earlier line review — six dimensions, every candidate sent to a refuter that had to reproduce it. 64 candidates, 30 survived, and they collapse into two roots. Two commits:
|
|
@gnanam1990 six commits from me are on your branch now, head The two I'd most want your eyes on, because they touch decisions you reasoned about explicitly:
Two things I got wrong along the way, in case they save you time: No rush, and genuinely fine to drop any of them — it's your PR and your design. @kevin-hnnh no action needed here, just so you know the branch moved. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/specialist/plan_runner.go (1)
153-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Descriptionstill says "Read-only plan task." for write-capable grants.
planTaskSystemPrompt(Line 176) now correctly tells a write-capable task to make changes, butMetadata.Description(Line 160) is still unconditionally"Read-only plan task.". A task grantededit_fileorbashnow carries a manifest whoseSystemPromptsays "you have been granted tools that CHANGE things" while itsDescriptionsays the opposite. This is the same "read-only" mislabeling raised in the earlier review comment on this function; only theSystemPrompthalf of that fix was applied, not theDescriptionfield.Branch
Descriptionthe same wayplanTaskSystemPromptbranches, using the samegrantsPlanWriteTool(grantedTools)check, so the two fields never disagree.🔧 Proposed fix: branch Description on the granted set
func planTaskManifest(name string, grantedTools []string) Manifest { if strings.TrimSpace(name) == "" { name = "explorer" } + description := "Read-only plan task." + if grantsPlanWriteTool(grantedTools) { + description = "Write-capable plan task, running in the plan's isolated worktree." + } return Manifest{ Metadata: Metadata{ Name: name, - Description: "Read-only plan task.", + Description: description, Tools: grantedTools, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_runner.go` around lines 153 - 186, Update planTaskManifest so Metadata.Description branches on grantsPlanWriteTool(grantedTools), matching the branching behavior of planTaskSystemPrompt: use a write-capable description when the granted set includes write tools and retain the read-only description otherwise, ensuring both manifest fields consistently reflect the same grants.
🧹 Nitpick comments (1)
internal/specialist/zzhol829_probe_test.go (1)
12-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert these probes into real assertions before merge.
TestZZProbeConcurrentWritersShareOneTree(Lines 12-53) andTestZZProbeHeadOfLineBlocking(Lines 58-108) exercise exactly the concurrent-writer and scheduling behaviors this PR cohort is meant to lock in, but both onlyt.Logftheir observations (Lines 49-52, 98-107). Neither test can fail regardless of whatExecutePlanInactually does with worktree sharing or head-of-line ordering.The file name (
zzhol829_probe_test.go) and the "PROBE" comments (Lines 11, 55) suggest this started as exploration rather than a maintained regression test. If the intent is to keep documenting scheduling characteristics for future readers, that's fine, but pair it with real assertions once the expected behavior is decided (for example, boundpeakconcurrent writers in Probe 1, or assert an ordering/timing expectation in Probe 2). Otherwise, consider removing this file before merge so it doesn't linger as a log-only artifact in the permanent test suite."add a regression test for behavior changes" for
**/*_test.gofiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/zzhol829_probe_test.go` around lines 12 - 108, Convert TestZZProbeConcurrentWritersShareOneTree and TestZZProbeHeadOfLineBlocking from log-only probes into regression tests with deterministic assertions for the intended worktree-sharing and scheduling behavior. Assert the expected peak writer concurrency and relevant task ordering or timing, remove probe-only logging/comments as appropriate, and ensure failures occur when ExecutePlanIn or ExecutePlan changes those behaviors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/specialist/plan_exec.go`:
- Around line 196-211: Update the MaxWall deadline setup around the plan
execution context so elapsed paused time in WaitWhilePaused does not consume the
plan’s wall budget. Rework the timeout handling to suspend and resume remaining
MaxWall accounting across pauses while preserving cancellation and
undispatched-task skip behavior, and add a regression test covering a pause
longer than the remaining wall budget followed by successful resumption.
In `@internal/specialist/zzhol829_probe_test.go`:
- Around line 136-172: The test TestZZProbeRealFailureRelabelledCancelled
reproduces a relabeling behavior where tasks marked TaskFailed are retroactively
changed to TaskCancelled after context cancellation, but only logs the results
without asserting on them, providing no regression protection. Replace the
t.Logf statements at the end of the test with explicit assertions that verify
the intended outcome: either assert that the "boom" task keeps its TaskFailed
outcome with the original error "compile error in main.go" (if the harvest logic
in plan_exec.go is fixed to prevent relabeling of already-failed tasks), or
assert that relabeling to TaskCancelled occurs and add a comment documenting why
this behavior is intentional. Ensure the test fails if the relabeling behavior
changes unexpectedly.
---
Duplicate comments:
In `@internal/specialist/plan_runner.go`:
- Around line 153-186: Update planTaskManifest so Metadata.Description branches
on grantsPlanWriteTool(grantedTools), matching the branching behavior of
planTaskSystemPrompt: use a write-capable description when the granted set
includes write tools and retain the read-only description otherwise, ensuring
both manifest fields consistently reflect the same grants.
---
Nitpick comments:
In `@internal/specialist/zzhol829_probe_test.go`:
- Around line 12-108: Convert TestZZProbeConcurrentWritersShareOneTree and
TestZZProbeHeadOfLineBlocking from log-only probes into regression tests with
deterministic assertions for the intended worktree-sharing and scheduling
behavior. Assert the expected peak writer concurrency and relevant task ordering
or timing, remove probe-only logging/comments as appropriate, and ensure
failures occur when ExecutePlanIn or ExecutePlan changes those behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acda29ee-1b70-440a-9478-ac7086b4badb
📒 Files selected for processing (7)
internal/specialist/plan_exec.gointernal/specialist/plan_runner.gointernal/specialist/plan_wall_budget_test.gointernal/specialist/plan_write_grant_test.gointernal/specialist/zzhol829_probe_test.gointernal/tui/orchestrate_saved.gointernal/tui/orchestrate_saved_write_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/orchestrate_saved.go
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/specialist/plan_tool.go (2)
585-592: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe background path drops the auto-assignment report.
assignNotesis only rendered on the foreground path at Line 611. A background plan can be fully re-modelled, and pay for a router call, and the returned text says nothing about it.autoAssignSummarydocuments this reporting as mandatory. Append it here too.🔧 Report the assignment on the background path
return tools.Result{ Status: tools.StatusOK, Output: fmt.Sprintf( "Plan %q started in the background with %d tasks. It is NOT finished — its result will arrive on a later turn. "+ "Carry on with other work; do not wait for it and do not report it as done.", - plan.Name(), plan.TaskCount()), + plan.Name(), plan.TaskCount()) + autoAssignSummary(assignNotes), Meta: map[string]string{"plan_status": "background"}, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_tool.go` around lines 585 - 592, Update the background-plan result in the plan execution flow to append the auto-assignment report from autoAssignSummary, matching the foreground path’s assignNotes reporting. Keep the existing background status, task count, and deferred-result messaging intact while ensuring any assignment summary is included in the returned Output.
506-539: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winA refused plan still pays for discovery and routing.
Line 503 was added so an invalid plan costs no provider call. Two later refusals still run after the spend: the missing-runner check at Line 516 and the one-plan-at-a-time check at Line 531. Both are pure and knowable before Line 506. With a background plan already running, every following
orchestratecall lists the provider's models, spends a frontier-model router call, and is then refused.Move both checks above
autoAssignModels.🔧 Refuse before spending
+ if tool.RunTask == nil { + return tools.Result{Status: tools.StatusError, Output: "Error: orchestrate has no task runner wired."} + } + if running, busy := runningPlanOn(tool.Recorder); busy { + return tools.Result{ + Status: tools.StatusError, + Output: fmt.Sprintf( + "Error: plan %q is still running, and a session shows one plan at a time. "+ + "Wait for it to finish — its result arrives on a later turn if it is a background plan — "+ + "or stop it with /plans stop, then run this one.", running), + } + } assignNotes, autoErr := tool.autoAssignModels(ctx, args, options) if autoErr != nil { return tools.Result{Status: tools.StatusError, Output: "Error: " + autoErr.Error()} } @@ plan, err := ParsePlan(args, tool.limits(options)) if err != nil { return tools.Result{Status: tools.StatusError, Output: "Error: " + err.Error()} } - if tool.RunTask == nil { - return tools.Result{Status: tools.StatusError, Output: "Error: orchestrate has no task runner wired."} - } - if running, busy := runningPlanOn(tool.Recorder); busy { - return tools.Result{ ... } - }Note that the existing comment at Line 528 justifies checking after parsing. Line 503 already parses, so the reason for the ordering no longer applies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_tool.go` around lines 506 - 539, Move the RunTask nil check and runningPlanOn(tool.Recorder) admission check ahead of autoAssignModels in the orchestrate flow, while keeping ParsePlan first so invalid plans are still reported as validation errors. Update the nearby ordering comment to reflect that these refusals now occur before discovery and routing, and preserve the existing error responses.
🧹 Nitpick comments (12)
internal/specialist/router_manifest_test.go (1)
21-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the unused runner scaffolding in this test.
The assertions read only
seen, which the stubPlanRunnerat Lines 40-43 sets.exec,planCtxandrunare never exercised:runis discarded by_ = runat Line 46, and theRunChildreassignment at Lines 34-36 mutates a copy afterNewPlanRunneralready capturedplanCtx, so it has no effect at all. A future reader will assume this test drives the real executor. It does not.♻️ Proposed trim
- var seen string - exec := Executor{ - BinaryPath: "/bin/true", - NewSessionID: func() (string, error) { return "specialist_00000000000000000000000a", nil }, - Load: func(LoadOptions) (LoadResult, error) { return LoadResult{}, nil }, - RunChild: func(context.Context, string, []string, func(streamjson.Event)) (ChildRunResult, error) { - return ChildRunResult{Started: true}, nil - }, - } - // The runner builds the manifest; capture it by intercepting the load path. - planCtx := PlanTaskContext{Executor: exec, Cwd: t.TempDir(), SpecialistName: "explorer"} - planCtx.Executor.Load = func(LoadOptions) (LoadResult, error) { return LoadResult{}, nil } - run := NewPlanRunner(planCtx) - planCtx.Executor.RunChild = func(context.Context, string, []string, func(streamjson.Event)) (ChildRunResult, error) { - return ChildRunResult{Started: true}, nil - } - + var seen string // Drive the real router entry point so the request it builds is the one tested. _, _, _ = routeTaskModels(context.Background(), func(_ context.Context, req PlanTaskRequest) (TaskResult, error) { seen = req.SystemPrompt return TaskResult{Outcome: TaskSucceeded, Output: `{"assignments":[]}`}, nil }, PlanTaskRequest{Tools: []string{"read_file"}}, "m", routerTasks(), routerCandidates(), "") - _ = runThe
streamjsonimport then becomes unused in this file only if the second test does not need it; it does, so leave the import list alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/router_manifest_test.go` around lines 21 - 46, Remove the unused Executor, planCtx, NewPlanRunner, RunChild reassignment, and run scaffolding from the test, leaving only the routeTaskModels setup needed to capture seen. Do not alter the streamjson import if it remains required by the other test.internal/specialist/plan_model_router_test.go (1)
125-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe table does not cover the "no runner" case the comment claims.
Line 125 says "No router model, no candidates, no runner: all skip silently", and the table has only
"no model"and"no candidates". The third element of each[3]anyholdsrunand is never read. Either add anil-runner case or drop the claim and the unused slot.💚 Proposed addition
- for name, args := range map[string][3]any{ - "no model": {"", routerCandidates(), run}, - "no candidates": {"qwen3.5:397b", []DiscoveredModel(nil), run}, - } { - called = false - model := args[0].(string) - cands, _ := args[1].([]DiscoveredModel) - if _, _, err := routeTaskModels(context.Background(), run, PlanTaskRequest{}, model, - routerTasks(), cands, ""); err != nil { + type skipCase struct { + model string + candidates []DiscoveredModel + runner PlanRunner + } + for name, tc := range map[string]skipCase{ + "no model": {"", routerCandidates(), run}, + "no candidates": {"qwen3.5:397b", nil, run}, + "no runner": {"qwen3.5:397b", routerCandidates(), nil}, + } { + called = false + if _, _, err := routeTaskModels(context.Background(), tc.runner, PlanTaskRequest{}, tc.model, + routerTasks(), tc.candidates, ""); err != nil { t.Errorf("%s: expected a silent skip, got %v", name, err) }If
routeTaskModelsdoes not guard a nil runner today, that is the bug this case would find.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_model_router_test.go` around lines 125 - 140, Add a `"no runner"` table case to the test around routeTaskModels, using a nil runner while retaining valid router model and candidates, and ensure the invocation passes each case’s runner value instead of the fixed run variable. Keep the silent-skip and router-not-called assertions so the test verifies nil-runner handling.internal/specialist/plan_runner.go (1)
98-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
task_IDand stop shadowingtaskwith a token count.Two readability problems in one closure:
task_IDuses an underscore, which is not idiomatic Go for a function name.taskLabelorrequestTaskIDreads the same and matches the rest of the file.- Line 104 binds
task := taskTokens.Add(...), anint64, while the enclosing scope already hastaskbound to theTaskat Line 64. A later edit that reaches fortask.IDinside this closure compiles into something unexpected or fails confusingly.♻️ Proposed rename
- spent := *event.TotalTokens - task := taskTokens.Add(int64(spent)) + spent := *event.TotalTokens + spentSoFar := taskTokens.Add(int64(spent)) switch { - case req.MaxTaskTokens > 0 && task > int64(req.MaxTaskTokens): + case req.MaxTaskTokens > 0 && spentSoFar > int64(req.MaxTaskTokens): overspent.Store(fmt.Sprintf( "task %s stopped after %d tokens: budget.max_tokens_per_task is %d", - task_ID(req), task, req.MaxTaskTokens)) + taskLabel(req), spentSoFar, req.MaxTaskTokens))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_runner.go` around lines 98 - 123, Rename the helper function task_ID to an idiomatic name such as taskLabel, updating all references in the closure and surrounding plan-runner code. In the counted closure, rename the local token-count result from task to a distinct name such as taskTotal, and use that name in the limit comparison and overspent message while leaving the enclosing Task variable unshadowed.internal/specialist/plan_model_fallback_test.go (1)
37-54: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePlan-task fixtures mutate plain variables from worker goroutines.
RunChildandRunTaskcallbacks execute on the plan scheduler's worker goroutines, so every counter and slice they touch is shared state. Both sites are safe today only because the fixture sets one worker or one task. Raising either value turns these into data races undergo test -race, and the failure will point at the harness rather than the behavior under test. Add async.Mutex(or useatomic.Int64for counters) so the fixtures stay race-free independent ofmax_workers.
internal/specialist/plan_model_fallback_test.go#L37-L54: guard theranappend with a mutex, and apply the same treatment to theattemptscounters in the remaining tests of this file.internal/specialist/child_scope_test.go#L194-L214: guard theseenappend inRunTaskwith a mutex.As per coding guidelines "Run affected concurrent code under the race detector".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_model_fallback_test.go` around lines 37 - 54, Make the concurrent test fixtures race-free: in internal/specialist/plan_model_fallback_test.go around lines 37-54, protect the ran append and every attempts counter in the remaining tests with a sync.Mutex or atomic.Int64; in internal/specialist/child_scope_test.go around lines 194-214, protect the seen append in RunTask with a mutex. Run the affected tests with the race detector.Source: Coding guidelines
internal/specialist/served_forms_test.go (1)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not discard the error here.
If
autoAssignModelsreturns an error,notesmay be empty and the failure reports "an Ollama list on an xAI session was accepted", which points at the wrong cause. Assert the error explicitly.♻️ Proposed change
- notes, _ := tool.autoAssignModels(context.Background(), args, tools.RunOptions{Model: "grok-4.5"}) + notes, err := tool.autoAssignModels(context.Background(), args, tools.RunOptions{Model: "grok-4.5"}) + if err != nil { + t.Fatalf("autoAssignModels: %v", err) + } if !strings.Contains(strings.Join(notes, " "), "different provider") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/served_forms_test.go` around lines 77 - 80, Update the autoAssignModels call in the test to retain its returned error and assert that the error is nil before checking notes, so execution reports an assignment failure directly instead of misclassifying it as an accepted Ollama list.internal/specialist/exec.go (1)
362-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare cleaned paths, not raw strings.
The workspace skip uses exact string equality. A supplier that reports
/ws/or/ws/.while--cwdis/wsre-emits the workspace as--add-dir, which is the exact case the comment above says cannot happen. Duplicate entries also produce duplicate flag pairs.filepath.Cleanon both sides plus a seen-set closes both.♻️ Proposed hardening
func appendExtraWriteRootArgs(args []string, roots []string, cwd string) []string { - workspace := strings.TrimSpace(cwd) + workspace := "" + if trimmed := strings.TrimSpace(cwd); trimmed != "" { + workspace = filepath.Clean(trimmed) + } + seen := map[string]bool{} for _, root := range roots { root = strings.TrimSpace(root) - if root == "" || root == workspace { + if root == "" { + continue + } + root = filepath.Clean(root) + if root == workspace || seen[root] { continue } + seen[root] = true args = append(args, "--add-dir", root) } return args }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/exec.go` around lines 362 - 372, Update appendExtraWriteRootArgs to normalize both cwd and each root with filepath.Clean before comparing them, so equivalent paths such as trailing-slash or dot forms are skipped. Track normalized roots in a seen-set and avoid appending duplicate --add-dir flag pairs while preserving empty-root filtering and argument order.internal/specialist/usage_pricing_test.go (1)
139-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not prove the property its comment claims.
The notes are hand-written, so the assertion only shows
autoAssignSummarypasses a substring through. Nothing here shows the router ever produces a note containing its token spend. A change that stops emitting the token count from the routing call keeps this test green. Drive the routing path and assert the note it generates, asTestTheUsageRollupWritesThePricingFieldsdoes for the rollup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/usage_pricing_test.go` around lines 139 - 145, Strengthen TestTheRoutingCallReportsWhatItSpent by exercising the actual routing path instead of passing hand-written notes directly to autoAssignSummary. Use the same setup and invocation pattern as TestTheUsageRollupWritesThePricingFields, then assert that the routing call’s generated note includes its token spend before validating the summary.internal/specialist/manifest.go (1)
272-281: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
ResolveWithFallbackfor manifest model canonicalization.
ResolveIDleavessonnet 4.5unresolved and preserves deprecatedclaude-haiku-3.5. Use the same resolver asresolveTaskModelso manifest metadata,--model, and usage accounting use the model that the child executes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/manifest.go` around lines 272 - 281, Update manifest model canonicalization to call the registry’s ResolveWithFallback, matching the resolver used by resolveTaskModel. Apply the fallback result to manifest.Metadata.Model so unresolved aliases such as sonnet 4.5 and deprecated identifiers such as claude-haiku-3.5 resolve consistently for execution and accounting.internal/specialist/plan_model_assign_test.go (1)
26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA duplicated assertion probably replaced an intended one.
Lines 26-28 and 29-31 assert the same thing about
tiers.strong. The second block likely meant to asserttiers.balanced, which no test in this function checks directly. Replace it or delete it.🔧 Assert the middle tier instead
- if tiers.strong != "claude-opus-4.1" { - t.Errorf("strong = %q", tiers.strong) - } + if tiers.balanced != "claude-sonnet-4.5" { + t.Errorf("balanced = %q, want the mid-priced tool-calling model", tiers.balanced) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_model_assign_test.go` around lines 26 - 34, Replace the duplicated second assertion on tiers.strong with an assertion validating tiers.balanced, preserving the expected middle-tier model value and error-reporting style used in the test.internal/specialist/plan_model_router.go (1)
220-240: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch the router's answer with the same id normalization used elsewhere.
offeredholds exact canonical ids, and Line 236 compares by exact string.modelIDFormsexists because a model id has several legitimate spellings, including an Ollama:latesttag. If the router replies withglm-5.2where the list printedglm-5.2:latest, the decision is dropped without a note and the task falls back to the classifier. Reuse the form-aware lookup.🔧 Resolve the router's choice through the offered forms
- offered := make(map[string]bool, len(candidates)) - for _, model := range candidates { - if id := strings.TrimSpace(model.ID); id != "" { - offered[id] = true - } - } + offered := map[string]string{} + for _, model := range candidates { + id := strings.TrimSpace(model.ID) + if id == "" { + continue + } + for _, form := range modelIDForms(id) { + offered[form] = id + } + } @@ - if !offered[chosen] { + canonical, ok := offered[chosen] + if !ok { continue } - out[id] = chosen + out[id] = canonical🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_model_router.go` around lines 220 - 240, Update the router assignment validation in the loop over decoded.Assignments to resolve chosen model IDs using the existing modelIDForms normalization rather than exact offered-map lookup. Preserve rejection of unknown models, but accept equivalent spellings such as an omitted Ollama :latest tag and store the canonical offered ID in out so downstream dispatch uses the listed model identifier.internal/specialist/plan_model_assign.go (1)
86-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comments are stacked on the wrong functions.
Lines 86-101 document
buildModelTiersand canonicalisation, but they sit directly aboverankedEligibleModels, sogo docattributes all of it to that function. Lines 301-318 contain two separateassignModelsToTaskArgssummaries. Move the tier text abovebuildModelTiersat Line 159 and merge the duplicate summary.Also applies to: 301-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/plan_model_assign.go` around lines 86 - 110, Move the tier-ranking and canonicalisation documentation currently above rankedEligibleModels so it directly precedes buildModelTiers, preserving the rankedEligibleModels comment for that function. In the assignModelsToTaskArgs area, merge the two adjacent duplicate summaries into one accurate doc comment and remove the redundant block.internal/cli/provider_models.go (1)
74-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the verify probe concurrency and confirm the race detector passes.
The verify loop launches one goroutine per discovered model with no cap:
"for index, model := range models { wait.Add(1); go func(index int, id string) { defer wait.Done(); results[index] = probe(ctx, id) }(index, model.ID) }"Each goroutine builds a new provider client and calls
discoveryCredentialProfile, which readsconfig.ProviderKeyStore()(a shared credential store). For a provider that lists dozens of models, this fires that many concurrent network probes and concurrent credential-store reads at once. Add a bounded worker pool (a semaphore or fixed-size channel) so probe concurrency has a ceiling and does not risk provider-side rate limiting or resource exhaustion.As per coding guidelines, "Run affected concurrent code under the race detector," run this verify path under
go test -race(or an equivalent manual-racerun) to confirmconfig.ProviderKeyStore()and the sharedresultsslice/verdictsmap access are race-free under concurrent probing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/provider_models.go` around lines 74 - 103, Bound the concurrent probing in the options.verify block by replacing the unbounded per-model goroutines with a fixed-size worker pool or semaphore, using an appropriate concurrency ceiling while preserving each model’s result. Ensure shared results and verdicts access remains race-free, then run the affected verify path with go test -race or an equivalent race-enabled run.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/provider_models.go`:
- Around line 293-322: Update planModelDiscoverer to filter discovered models
with providermodelcatalog.ModelIDAllowedForProvider for the active provider
before converting them to specialist.DiscoveredModel; exclude catalog-disallowed
IDs from out, while preserving allowed models and the existing discovery error
propagation.
In `@internal/config/resolver.go`:
- Around line 396-398: Stop merging untrusted project model exclusions in the
resolver’s PlanModels handling; remove or gate the
src.Profiles.PlanModels.Exclude append behind explicit user-approved cost
policy. Update internal/config/plan_size_test.go lines 249-270 to verify project
model preferences cannot alter the user’s candidate set, replacing the existing
safety assertion.
In `@internal/specialist/plan_exec.go`:
- Around line 1050-1063: Update tasksCutForBudget and the task-result flow so
budget termination is identified structurally rather than by matching task.Err
text, including both plan-level and max_tokens_per_task cancellations. Have the
runner mark the relevant TaskResult when a budget stops a task, and select
cancelled results using that signal alongside TaskSkippedBudget; adjust the
generated headline wording so cancelled tasks are not described as having “never
ran.”
In `@internal/specialist/plan_model_probe.go`:
- Around line 145-166: Update proveModels to cap the number of concurrent probe
goroutines and provider requests, using a bounded worker or semaphore pattern
while preserving result indexing, cache handling, panic recovery, and waiting
for all probes to finish. Ensure the bound is applied to every uncached model
and run the affected concurrent code under the race detector.
In `@internal/specialist/plan_provider_mismatch_test.go`:
- Around line 32-49: Update the test’s OrchestrateTool setup around
autoAssignModels to provide a RunTask implementation that sets dispatched when
routing occurs, using the existing context and task inputs as appropriate. Keep
the assertion verifying that a configured default is downgraded without error
and that no dispatch occurs against an incompatible provider, so the check can
fail on regressions.
In `@internal/specialist/plan_runner.go`:
- Around line 107-118: Update the token-limit handling switch in the task
execution flow to call req.Spend.add(spent) before evaluating whether
MaxTaskTokens stopped the task. Preserve the task-cap message and cancellation
behavior, while ensuring the plan-level meter receives the final token event
before either cap decision is applied.
In `@internal/specialist/plan_tool.go`:
- Around line 154-176: The tasks array schema in the plan tool lacks an Items
definition. Update the "tasks" PropertySchema to define each item as an object
with required id and prompt fields, plus optional depends_on, read-only tool
subset, phase label, and model fields, following the existing recursive Items
pattern used by comparable arrays.
---
Outside diff comments:
In `@internal/specialist/plan_tool.go`:
- Around line 585-592: Update the background-plan result in the plan execution
flow to append the auto-assignment report from autoAssignSummary, matching the
foreground path’s assignNotes reporting. Keep the existing background status,
task count, and deferred-result messaging intact while ensuring any assignment
summary is included in the returned Output.
- Around line 506-539: Move the RunTask nil check and
runningPlanOn(tool.Recorder) admission check ahead of autoAssignModels in the
orchestrate flow, while keeping ParsePlan first so invalid plans are still
reported as validation errors. Update the nearby ordering comment to reflect
that these refusals now occur before discovery and routing, and preserve the
existing error responses.
---
Nitpick comments:
In `@internal/cli/provider_models.go`:
- Around line 74-103: Bound the concurrent probing in the options.verify block
by replacing the unbounded per-model goroutines with a fixed-size worker pool or
semaphore, using an appropriate concurrency ceiling while preserving each
model’s result. Ensure shared results and verdicts access remains race-free,
then run the affected verify path with go test -race or an equivalent
race-enabled run.
In `@internal/specialist/exec.go`:
- Around line 362-372: Update appendExtraWriteRootArgs to normalize both cwd and
each root with filepath.Clean before comparing them, so equivalent paths such as
trailing-slash or dot forms are skipped. Track normalized roots in a seen-set
and avoid appending duplicate --add-dir flag pairs while preserving empty-root
filtering and argument order.
In `@internal/specialist/manifest.go`:
- Around line 272-281: Update manifest model canonicalization to call the
registry’s ResolveWithFallback, matching the resolver used by resolveTaskModel.
Apply the fallback result to manifest.Metadata.Model so unresolved aliases such
as sonnet 4.5 and deprecated identifiers such as claude-haiku-3.5 resolve
consistently for execution and accounting.
In `@internal/specialist/plan_model_assign_test.go`:
- Around line 26-34: Replace the duplicated second assertion on tiers.strong
with an assertion validating tiers.balanced, preserving the expected middle-tier
model value and error-reporting style used in the test.
In `@internal/specialist/plan_model_assign.go`:
- Around line 86-110: Move the tier-ranking and canonicalisation documentation
currently above rankedEligibleModels so it directly precedes buildModelTiers,
preserving the rankedEligibleModels comment for that function. In the
assignModelsToTaskArgs area, merge the two adjacent duplicate summaries into one
accurate doc comment and remove the redundant block.
In `@internal/specialist/plan_model_fallback_test.go`:
- Around line 37-54: Make the concurrent test fixtures race-free: in
internal/specialist/plan_model_fallback_test.go around lines 37-54, protect the
ran append and every attempts counter in the remaining tests with a sync.Mutex
or atomic.Int64; in internal/specialist/child_scope_test.go around lines
194-214, protect the seen append in RunTask with a mutex. Run the affected tests
with the race detector.
In `@internal/specialist/plan_model_router_test.go`:
- Around line 125-140: Add a `"no runner"` table case to the test around
routeTaskModels, using a nil runner while retaining valid router model and
candidates, and ensure the invocation passes each case’s runner value instead of
the fixed run variable. Keep the silent-skip and router-not-called assertions so
the test verifies nil-runner handling.
In `@internal/specialist/plan_model_router.go`:
- Around line 220-240: Update the router assignment validation in the loop over
decoded.Assignments to resolve chosen model IDs using the existing modelIDForms
normalization rather than exact offered-map lookup. Preserve rejection of
unknown models, but accept equivalent spellings such as an omitted Ollama
:latest tag and store the canonical offered ID in out so downstream dispatch
uses the listed model identifier.
In `@internal/specialist/plan_runner.go`:
- Around line 98-123: Rename the helper function task_ID to an idiomatic name
such as taskLabel, updating all references in the closure and surrounding
plan-runner code. In the counted closure, rename the local token-count result
from task to a distinct name such as taskTotal, and use that name in the limit
comparison and overspent message while leaving the enclosing Task variable
unshadowed.
In `@internal/specialist/router_manifest_test.go`:
- Around line 21-46: Remove the unused Executor, planCtx, NewPlanRunner,
RunChild reassignment, and run scaffolding from the test, leaving only the
routeTaskModels setup needed to capture seen. Do not alter the streamjson import
if it remains required by the other test.
In `@internal/specialist/served_forms_test.go`:
- Around line 77-80: Update the autoAssignModels call in the test to retain its
returned error and assert that the error is nil before checking notes, so
execution reports an assignment failure directly instead of misclassifying it as
an accepted Ollama list.
In `@internal/specialist/usage_pricing_test.go`:
- Around line 139-145: Strengthen TestTheRoutingCallReportsWhatItSpent by
exercising the actual routing path instead of passing hand-written notes
directly to autoAssignSummary. Use the same setup and invocation pattern as
TestTheUsageRollupWritesThePricingFields, then assert that the routing call’s
generated note includes its token spend before validating the summary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fed1fd2-99b9-48f9-8a80-319dd8a1576e
📒 Files selected for processing (65)
internal/cli/app.gointernal/cli/child_exit_code_agreement_test.gointernal/cli/exec.gointernal/cli/exec_usage_cache_test.gointernal/cli/exec_writer.gointernal/cli/exec_zeromaxing_test.gointernal/cli/plan_grant_test.gointernal/cli/plan_live_provider_test.gointernal/cli/plan_model_prefs_carry_test.gointernal/cli/plan_model_probe.gointernal/cli/provider_models.gointernal/config/plan_size_test.gointernal/config/resolver.gointernal/config/types.gointernal/specialist/accounting.gointernal/specialist/budget_enforcement_test.gointernal/specialist/child_scope_test.gointernal/specialist/dependency_briefing_test.gointernal/specialist/exec.gointernal/specialist/manifest.gointernal/specialist/manifest_test.gointernal/specialist/plan.gointernal/specialist/plan_exec.gointernal/specialist/plan_exec_test.gointernal/specialist/plan_grant_test.gointernal/specialist/plan_model.gointernal/specialist/plan_model_assign.gointernal/specialist/plan_model_assign_test.gointernal/specialist/plan_model_fallback_test.gointernal/specialist/plan_model_probe.gointernal/specialist/plan_model_probe_test.gointernal/specialist/plan_model_router.gointernal/specialist/plan_model_router_test.gointernal/specialist/plan_model_test.gointernal/specialist/plan_provider_mismatch_test.gointernal/specialist/plan_runner.gointernal/specialist/plan_store_test.gointernal/specialist/plan_test.gointernal/specialist/plan_tool.gointernal/specialist/plan_worktree_test.gointernal/specialist/plan_write_test.gointernal/specialist/resume_manifest_test.gointernal/specialist/router_manifest_test.gointernal/specialist/served_forms_test.gointernal/specialist/streamer.gointernal/specialist/task_role.gointernal/specialist/task_role_test.gointernal/specialist/usage_pricing_test.gointernal/streamjson/streamjson.gointernal/tui/model.gointernal/tui/orchestrate_panel.gointernal/tui/orchestrate_panel_test.gointernal/tui/orchestrate_saved.gointernal/tui/orchestrate_window_test.gointernal/tui/plan_card_regression_test.gointernal/tui/plan_fallback_display_test.gointernal/tui/plan_messages.gointernal/tui/plan_progress.gointernal/tui/plan_progress_test.gointernal/tui/sidebar.gointernal/tui/sidebar_plan_detail.gointernal/tui/sidebar_plan_detail_test.gointernal/tui/sidebar_plan_test.gointernal/tui/sidebar_test.gointernal/tui/specialist_card.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/tui/plan_messages.go
- internal/tui/orchestrate_saved.go
- internal/tui/orchestrate_window_test.go
- internal/specialist/resume_manifest_test.go
- internal/tui/plan_card_regression_test.go
- internal/specialist/plan_store_test.go
- internal/tui/model.go
- internal/specialist/plan_test.go
- internal/specialist/plan_grant_test.go
- internal/cli/exec_zeromaxing_test.go
- internal/specialist/plan_worktree_test.go
- internal/tui/sidebar_plan_test.go
- internal/specialist/plan_exec_test.go
- internal/tui/plan_progress.go
- internal/cli/app.go
- internal/tui/orchestrate_panel_test.go
- internal/tui/sidebar_plan_detail_test.go
- internal/tui/orchestrate_panel.go
- internal/cli/exec.go
- internal/tui/sidebar.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/specialist/plan_store_test.go`:
- Around line 590-616: Update SavePlan to create the temporary file atomically
with os.CreateTemp, write through the returned file descriptor, close it, and
then rename it; remove the separate refuseSymlink/temp-path write sequence.
Extend the existing symlink protection coverage with a deterministic regression
test that replaces the temporary path concurrently and verifies the symlink
target is not modified.
In `@internal/specialist/plan_strict_lists_test.go`:
- Around line 154-161: Update TestASavedPlanStillRefusesInlineContent to seed a
valid saved plan named “sweep” in the temporary UserDir before iterating over
inline fields. Use the existing plan-saving setup or helper, then keep the loop
asserting each field causes an error so failures specifically verify the
saved-plan override policy.
In `@internal/specialist/plan.go`:
- Around line 776-782: The planStringsStrict function currently treats present
non-array values as absent, allowing malformed depends_on and tools fields to
bypass validation. Distinguish a missing key from a present value, return an
error when args[key] exists but is not an array (including null), and preserve
the existing nil result only for absent keys. Add regression cases in
plan_strict_lists_test.go covering invalid depends_on and tools values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a3e9fb9-1221-48d6-9ec8-01ebecd92512
📒 Files selected for processing (7)
internal/cli/app.gointernal/cli/shutdown_order_test.gointernal/specialist/plan.gointernal/specialist/plan_store.gointernal/specialist/plan_store_test.gointernal/specialist/plan_strict_lists_test.gointernal/specialist/plan_tool.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/app.go
- internal/specialist/plan_store.go
- internal/specialist/plan_tool.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
I tested this from the TUI as well as reviewing the diff. The core path works: zeromaxing activates, a two-task read-only plan completed correctly in parallel, and the specialist/TUI/CLI tests pass.
I am requesting changes for the remaining user-facing issues:
- The bundled research plan still uses the literal "THE SUBJECT". The documented "/plans run research" path cannot provide a subject, so it can spend five child-agent runs researching a placeholder.
- The budget schema describes limits but does not declare its minimums and maximums. In the manual run the model emitted max_tokens_per_task: 5000, Zero rejected it, and the model had to retry. The schema should prevent that invalid call.
- The zeromaxing chip hit-test still searches every footer row for the label. Typing the same word in the composer can make the composer row become the hover/click target instead of the status chip.
- The completed plan UI showed both "AGENTS 2 done" and "no agents spawned", and raw child session_id values were printed inside the plan result.
The size also makes this difficult to review safely: 36,607 additions across 198 files and 96 commits. About 22k lines are tests, which is good, but there are still roughly 14.2k production additions spanning posture, scheduling, persistence/resume, background execution, write isolation, model routing, TUI, sandbox, credential-store, and worktree changes. These are independently reviewable seams and would be safer as focused PRs.
The feature has real value, but I do not think this draft is ready to merge until the concrete issues above are fixed and the scope is reduced or split into reviewable pieces.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Read through this properly rather than skimming the diff — 36k lines is a lot, and most of it is solid. The plan/orchestrate model hangs together, the isolation design is the right shape, and the commentary explaining why things are the way they are is genuinely better than most of what lands here.
Two things need fixing before it goes in, and one of them is the reason I'm requesting changes rather than nitting.
1. Isolated plan tasks can write to the real repo — internal/specialist/exec.go:366
The write-capable path promises isolation and doesn't deliver it. Chain:
appendExtraWriteRootArgsskips a root only whenroot == input.CwdScope.Roots()returns the parent workspace root first, then extras- for an isolated task
input.Cwdis the worktree, so the parent repo root isn't skipped — it goes out as--add-dir <parentRepo> - the child feeds
addDirsintosandbox.NewScope(workspaceRoot, extras), andAddfails withwrite root %q, so these are write roots, not read
Meanwhile planWorkspaceNote tells the user "nothing was written to the parent tree", and the tool schema says naming a write tool "runs it in an isolated worktree and asks for approval first". So a task the user approved on that promise can write_file/apply_patch/bash anywhere in their actual repo.
I don't think this is carelessness — a worktree's .git file points back at the parent, so some git operations genuinely need write access under <parentRepo>/.git. But that argues for granting that path specifically, not the whole tree. Whatever you land on, the note and the schema text need to describe what's actually enforced.
internal/specialist/child_scope_test.go:36 only covers Cwd == the parent workspace, which is why this passes today — the isolated case, the one that matters, isn't exercised.
2. /effort auto mutates a run in flight — internal/tui/session_controls.go:93
You already wrote the argument for this guard, on the branch right above it:
Same idle-session rule /profile enforces… The budget propagates to sub-agents spawned later in the same run, so changing it mid-run leaves one turn running under two different budgets.
The entering branch checks m.pending. The leaving branch calls revertExecProfile() with no check at all, and /effort auto is reachable mid-run. Sub-agents spawned later in that turn inherit the reverted budget while earlier ones kept the posture's, which is exactly the straddling state /turns and /profile refuse. It also flips the orchestrate gate, so the tool disappears from under a model that's mid-plan.
Same guard, same reason, other direction.
Worth fixing in the same pass
internal/cli/plan_model_probe.go:43—providers models <name> --verifylooks like it probes the active provider rather than the named one, because the prober routes the selected profile throughlivePlanProvider. If so,--verifyreports the wrong thing whenever the named profile isn't the active one.plan_exec.go:720/plan_runner.go:98—budget.max_tokens_per_taskis applied per attempt. With retries and fallbacks a task can spend a multiple of its declared cap, which makes the cap advisory rather than a bound.internal/agent/loop.go:595—Options.MaxTokenscounts only the parent's own provider usage, so orchestrate and sub-agent spend never lands against the posture's spend bound. For a posture whose whole point is running long, that's the number people will assume is holding.internal/cli/provider_models.go:92—--verifyfans out one goroutine per discovered model, uncapped. The specialist path next door caps at 8 for exactly this reason.plan.go:483(from my earlier round, still open) —seconds > 0means a negativemax_wall_seconds/max_stall_secondsreads as "unset" and the plan runs unbounded. A negative value should be an error, not an off switch.plan_exec.go:510— paused time is credited to the wall clock only once the pause ends, while the watchdog keeps counting through it, so a long pause can trip the budget.plan_exec.go:413—cutShortkeys on a non-nil error the production runner never returns, so a task in flight when a plan is stopped records asTaskFailedrather thanTaskCancelled.
Smaller
orchestrate_saved.go:382 — the cross-plan resume guard compares plan names, but names are optional, so two unnamed plans both compare "" and it passes. orchestrate_saved.go:237 — planTaskSummaryLine cuts only at \r\n and leaves ESC through, unlike the sibling you already fixed; same for the failed-task error text at sidebar.go:668. These strings are model-authored, so escape sequences reach the terminal. zeromaxing_glow.go:208 — chip hit-testing takes the first footer line containing "zeromaxing", which composer text can shadow. plans/research.json:8 still ships literal "THE SUBJECT" with no placeholder declared, so /plans run research dispatches five sub-agents on an unanswerable prompt.
Three tests don't test what they're named for: plan_watchdog_test.go:386 asserts parent.Err() != nil on a context.Background(), which can never be true; plan_grant_test.go:377 puts its only assertion inside if err == nil && len(models) > 0, so the error property it names is never checked; scope_temporary_test.go:19 only tries macOS paths, so all five refcount tests — the only coverage AddTemporaryWrite has — skip on Windows and Linux.
Where that leaves it
Fix 1 and 2 and I'll re-review the whole thing rather than sending you back round again — I know CodeRabbit has already bounced this five times and I'd rather this be the last round from me. The middle group I'd want addressed or argued with; the smaller ones you can take or leave, but the three dead tests I'd fix since they're currently reporting green on properties nobody is checking.
Nothing here is a design objection. The shape is right.
|
Thanks for the careful reviews. Important context first: the fixes for most of these were committed but unpushed when you reviewed — the fork branch was behind, so the code you saw predates them. Everything is now pushed; here is where each point stands against current HEAD. @anandh8x — all four addressed
@Vasanthdev2004 — addressed
|
|
@gnanam1990 sorry for the wait. I went straight at the two blockers on current HEAD rather than re-reading all 49k lines, because you have been sitting on this since this morning. Both are fixed in one place and missed in a twin. Three one-line misses and then I think this is done. Isolation.
Negative Fix those three and I will approve. No more rounds from me after that. |
# Conflicts: # internal/agent/loop_test.go # internal/cli/app.go # internal/cli/exec.go # internal/cli/exec_writer.go # internal/config/types.go # internal/providers/openai/provider_test.go # internal/sandbox/scope.go # internal/sandbox/scope_extra_read_test.go # internal/sandbox/scope_temp_refcount_test.go # internal/sandbox/scope_temporary_test.go # internal/tools/edit_file.go # internal/tools/file_tracker.go # internal/tui/files_panel.go # internal/tui/hover.go # internal/tui/hover_test.go # internal/tui/keybinding_help.go # internal/tui/model.go # internal/tui/mouse.go # internal/tui/options.go # internal/tui/sidebar.go # internal/tui/transcript_selection.go
|
@Vasanthdev2004 Addressed the current-head blocker and refreshed this branch onto current main at b630dc1. Ownership cleanup:
Fresh-main integration:
This is now an explicit stacked dependency: the standalone #829 head will report missing internal/measurements and internal/memory until #909 and #897 land. I validated the real composition in a disposable non-/tmp worktree by merging the current heads of #909 and #897 into this head (taking #897 ownership for its two memory-tool files):
No new third-party module was introduced by this repair. Please rereview the current head; the expected temporary red dependency check should clear when #909 and #897 merge. |
|
@Vasanthdev2004 The current-main integration/split repair is updated at 085ec16. What changed
Validation on the real stacked composition
Expected standalone status
Please rereview the current head for the requested split/current-main integration. |
|
Follow-up stack validation against the newest dependency heads is complete:
The focused integration matrix and its race-detector runs pass for The standalone PR checks remain expectedly compile-blocked until the two extracted dependency PRs land; the composed stack is the meaningful validation target. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
-
[P1] Land the two prerequisite packages before merging this PR
internal/agent/loop.go:16This head imports
internal/measurementsandinternal/memory, but neither package exists on the PR head or itsmaintarget. That is why the required Ubuntu, macOS, Windows, performance, security, and review jobs all stop at missing-package compilation errors. The composed stack is useful development evidence, but the branch GitHub would merge is not independently buildable today.Please keep #829 ordered behind #909 and #897, then refresh this branch after both dependencies land and require the ordinary standalone CI matrix to pass. The root problem is dependency ownership that GitHub cannot encode in this PR's current
maintarget; suppressing or special-casing the red checks would leave the merge graph ambiguous rather than resolve it.
Findings
-
[P1] Use a single owner for terminal process completion
internal/execution/process_manager.go:457When a process exits,
removeCompletedLaterwakes onprocess.doneand callscollectResult. At the same time, theStartorContinuecaller may already be inside the same collection path.collectResultis not an observation: it callsprocess.output.drain(), which removes the buffered bytes. Both collectors then callremember, andrememberunconditionally replaces the prior record. Depending on scheduling, the retention goroutine can steal the terminal chunk from the caller, or an empty caller result can overwrite a complete retained result. Repeating the new retention regression test produces an empty result, so this is an exercised race rather than a theoretical interleaving.Please address the ownership model, not only the final overwrite. Completion should be collected exactly once into an immutable terminal result, and callers, snapshots, and retention should all read or derive from that same result. Adding ordering checks around
rememberwould still leave two destructive consumers competing for the output buffer and would only change which surface loses the bytes. -
[P2] Route late
write_stdinpolls through the completed-session store
internal/tools/exec_command.go:350ProcessManager.Continuenow falls back toCompleted(id)when a process is no longer live, but the productionwrite_stdintool callsSnapshot(id)first.Snapshotconsults only the live process map, so an evicted completion returnsUnknownExecSessionErrorbeforeContinuecan use the new fallback. The manager-level tests pass because they callContinuedirectly and therefore bypass the consumer boundary that users exercise.There is a second part to the same contract:
completedProcessretains only ID, command, output, and exit code, whileexecToolResultconsumes cwd, TTY, truncation, interruption, request/enforcement/report, changed files, and sandbox metadata. If the preflight is merely removed, late results will become reachable but will silently lose mutation and safety evidence.Please make live and completed lookup one coherent manager operation and retain an immutable terminal
ProcessResult(or an explicitly equivalent shape). The tool should not have to probe one store before calling an API that knows about both, and a completed result should preserve the same user-visible and policy-relevant semantics as the result returned at exit time. -
[P1] Keep a background plan bound to its originating session
internal/tui/plan_progress.go:117A background plan intentionally outlives the turn that launched it.
/newand/resumecan switch conversations while that plan remains live, and the nextbeginRuncallsAttachon the same process-wide bridge with the new run and session IDs.Attachoverwrites the mutablestore,sessionID,sink, andrunIDfields that the old plan's goroutine still uses. Subsequent task and terminal events from session A are therefore appended to session B, and the bridge's global completion queue is later drained into B's model conversation as if B had launched the work. Session A is left with a partial event history that looks interrupted.Please fix the ownership boundary rather than adding another stale-run guard. A live plan needs immutable origin identity for persistence and completion delivery. Viable designs include a per-plan recorder/bridge captured at launch, or explicitly preventing/detaching a session switch until the background lifecycle has been transferred or terminated. Rebinding display state may be desirable, but it must not rebind the durable session owner of already-running work.
-
[P1] Scope
lastPlanand resume progress to the same session and plan
internal/tui/orchestrate_saved.go:405PlanProgressBridge.lastPlanis process-global even thoughLastPlanand the/plansmessages describe it as belonging to “this session.” Neither/newnor/resumeclears it or restores the selected session's plan identity. After moving from session A to B,/plans saveand/plans restartcan therefore operate on A's plan. Bare/plans resumeis more dangerous: it parses A's retained arguments, reduces B's latest plan events, and passes the two toRemainingPlanwithout theprogress.Name == plan.Name()check used by saved-plan resume. If the plans reuse task IDs, B's successes can cause tasks from A to be treated as already complete and never run.Please make the plan identity and progress source inseparable. Store or reconstruct the most recent plan per session, clear or restore it during session changes, and validate the plan name/identity before applying any reduced events. Fixing only the task-ID collision would still allow save/restart to replay a plan that the current conversation never ran.
-
[P2] Complete the bridge lifecycle when a background plan panics
internal/cli/plan_background.go:75The launcher deliberately recovers panics so a background goroutine cannot crash the TUI, but the recovery defer discards the panic and the cleanup defer only cancels the context and releases
launcher.running. The normalrecordPlanCompletedcall sits afterExecutePlanIn, so a panic skips it. The bridge consequently keeps itsbackgroundandcancelPlanstate, emits no terminal event or completion, and continues answeringPlanSurfaceBusyeven though the launcher slot is free. Later plans are refused and the panel remains unfinished until restart.Please turn recovery into an explicit terminal transition. The panic path should produce a failed plan report/event, clear bridge control state, release paused waiters, and then release the launcher slot. More broadly, all exits from a launched plan—success, ordinary failure, cancellation, and panic—should converge on one idempotent finalizer so adding another early-return path cannot wedge the surface again.
-
[P2] Surface plan-event persistence failures before claiming durability
internal/tui/plan_progress.go:147If
Store.AppendEventfails because the session store becomes unwritable, the disk fills, or an I/O operation fails,recordlatchesrecordErrand silently drops every later plan event.RecordingErroris referenced only by tests, and the nextAttachclears the latch. The plan can therefore finish with a success presentation even though its durable history ends mid-run. A later/plans resumemay repeat completed tasks or report no resumable plan, with no indication that persistence was lost.Please connect persistence failure to the production lifecycle. The UI/result should disclose that durability was lost, and the error must remain associated with the affected plan/session until it has been surfaced. The root issue is that “best effort” recording is being used as the authoritative resume source; either make required lifecycle persistence fail visibly, or explicitly mark the resulting plan non-resumable. Merely logging the first error would not prevent a later success claim from overstating what was saved.
-
[P1] Confine saved-plan reads to their declared roots
internal/specialist/plan_store.go:240PlanPathsdefinesProjectRootandUserRootas containment boundaries, andSavePlancorrectly uses rootedpathjailoperations.LoadPlans, however, ignores both roots and givesloadPlanDironly an absolute directory string.os.ReadDirfollows a linked.zeroorplansancestor, whilerefuseSymlinkchecks only the final JSON file; the lateros.ReadFilealso re-resolves the path after that check. A repository can therefore redirect plan discovery outside the workspace and have external JSON labeled, displayed, and executed as a project-owned saved plan.Please use the same handle-relative containment model for reads as for writes: open the declared root, traverse/list relative to that handle without following reparse-point ancestors, and read the selected file through the confined handle. Extending the final-component
Lstatchecks would retain both the ancestor escape and the check/use race; the root cause is resolving untrusted path strings more than once outside the jail. -
[P2] Distinguish an absent saved-plan directory from a failed read
internal/specialist/plan_store.go:241loadPlanDirreturns(nil, nil)for everyos.ReadDirerror even though its comment says only a missing directory is ordinary. Permission failures, I/O errors, malformed links, and other inspection failures are therefore indistinguishable from “no plans here.” Because loading proceeds builtin, user, then project, a project-directory failure can silently leave a same-named user or builtin plan selected and run different instructions from the plan the user expected.Please ignore only
os.IsNotExistand return every other directory error through the existingproblemschannel. At the higher level, ensure a failed higher-priority scope cannot silently degrade to a lower-priority plan of the requested name. The root contract is precedence with truthful failure reporting, not simply makingReadDirreturn an error string. -
[P2] Separate successful worktree retention from admission-abort cleanup
internal/cli/plan_isolate.go:80resolvePlanWorkspaceprepares an isolated worktree and then performs reachability checks. On either post-creation refusal it callsworkspace.Release()because the caller never receives a handle. The production isolator suppliesRelease: func() {}to preserve successful plan work for review. That same no-op is used on the refusal path, leaving the deterministicplan-<name>worktree under a PID-less Zero lease. A corrected retry with the same name is rejected as already locked, and normal stale cleanup skips the locked entry.Please model the two lifecycle outcomes separately. Successful execution may intentionally retain the tree and its user work, while an admission abort must release the lease (and decide safely whether to remove an untouched tree). A single callback cannot simultaneously mean “retain successful output” and “roll back a resource the caller was never given”; adding cleanup at one current refusal site would leave future post-prepare validation failures vulnerable to the same leak.
-
[P1] Make shell prompt guidance use the sandbox-selected runtime
internal/tools/shell_runtime.go:217Command construction now calls the sandbox-aware resolver, which can observe that PowerShell starts on the host but fails under the Windows restricted token and then select
cmd.exe.HostShellEnvironmentGuidanceindependently calls the unsandboxed resolver. In that supported fallback case, the system prompt instructs the model to emit PowerShell cmdlets, pipelines, quoting, and environment syntax, while the execution tool passes the command text tocmd.exe. The model is set up to produce invalid commands and then reason from failures caused by guidance that did not describe the actual executor.Please establish a single source of truth for the effective runtime used by both prompt construction and command execution. If the sandboxed probe cannot safely run during prompt assembly, resolve/cache the effective runtime when the sandbox engine is created and inject that value into both consumers. Duplicating the selection logic or teaching the prompt both syntaxes would preserve ambiguity about which grammar is actually active.
-
[P2] Scope model-probe verdicts to provider identity
internal/specialist/plan_model_probe.go:79The model discovery and probing closures deliberately call
livePlanProviderso an in-session provider change takes effect without rebuilding the tool. TheOrchestrateToolitself is long-lived and keeps oneprobeCache, whose map key is only the model ID. Two providers commonly advertise overlapping IDs. After provider A caches “serves” or “refuses,” provider B's first plan reuses that verdict without invoking B's prober. An adversarial test with the same ID and opposite provider verdicts confirmed that provider B is never called.Please include every routing dimension that changes the truth of a probe in the cache identity—at minimum provider/profile/endpoint identity plus model ID—or invalidate the cache atomically when the live provider changes. Keying only on model spelling conflicts with the feature's explicit live-provider behavior; shortening the cache lifetime would reduce but not remove the correctness hole during a switch.
-
[P2] Validate the parsed target set for sweep templates
internal/specialist/plan_template.go:111BuildTemplatePlanverifies that the rawtargetsparameter contains some non-whitespace text. The sweep builder then callssplitTemplateList, which discards blank comma-separated entries. Inputs such as","or", ,"therefore pass validation but produce zero examination tasks and onecombinetask with no dependencies. The plan is structurally valid and can report a synthesized answer even though no target was examined.Please validate the semantic value after parsing: the target slice must contain at least one non-empty target before any tasks are constructed. Ideally parameter parsing should return a validated typed value so the admission check and builder cannot disagree. Special-casing comma-only strings in the raw check would repeat the same bug for another normalization edge.
Optional direction: convert this umbrella into independently landing PRsI do not think the work needs to be discarded or rewritten from scratch, and I agree that mechanically cutting the current branch at arbitrary commits would leave some slices uncompilable. The current implementation has accumulated dependencies through I do not think that means the feature is inherently one PR, though. Shared integration files are where independently implemented components are composed; touching the same file does not make those components one review or rollback unit. At the current head this is 262 files and roughly 50k added lines, spanning execution retention, posture, plan admission/execution, persistence/resume, model selection, workspace isolation, background lifecycle, and a large TUI surface. Those areas have different failure modes, security boundaries, and useful validation. Here is an optional path that would preserve the implementation while turning it into a small foundation plus leaf PRs that target One honest constraintNo decomposition can make a Go package import code that does not exist yet. If persistence imports plan types, or the TUI imports the executor, that consumer cannot literally land before its provider without duplicating code. The minimal way around that is one small contract/seam PR first. It should contain only stable data contracts, interfaces, and inert registration seams—no user-visible orchestration and no substantial executor implementation. Once that lands, the substantive leaf PRs can all branch from the same That is different from a conventional stack: after the seam, the leaf PRs would all target Step 0: keep extracted package ownership where it already belongs
This removes the recurring sync/conflict problem instead of copying reviewed package heads into the umbrella branch. Step 1: land a small, inert orchestration contractThe foundation could be a new narrow package such as It should define only the contracts that multiple leaves genuinely share, for example:
I would avoid carrying the current collection of optional recorder type assertions into the seam. The current code documents several cases where an omitted optional half compiled successfully and silently removed required behavior. Prefer one explicit dependency object, for example conceptually: type Runtime struct {
Tasks TaskExecutor
Events EventRecorder
Models ModelSelector
Workspaces WorkspaceProvider
Clock Clock
}
func NewService(runtime Runtime) (*Service, error)The exact shape can differ, but construction should fail when behavior required by the selected capability is absent. Optional product capabilities can use explicit nil/no-op implementations whose limitations are queryable, rather than interface discovery changing executor semantics. The seam PR should not:
Its main proof is that Step 2: extract leaf PRs that can land in any order after the seamA. Zeromaxing posture onlyScope:
Explicitly exclude:
The posture is independently useful as a high-budget/high-effort mode. If product intent says it should exist only with orchestration, it can still land dormant behind a configuration/availability gate and be exposed by the activation PR. Either way, it does not need the plan engine to compile. Validation:
B. Core plan engine, in memory and read-onlyScope:
Explicitly exclude:
This PR should run entirely against in-memory fake recorders/executors. It can land before or after every presentation, persistence, routing, and workspace leaf because it implements the contract without importing them. Validation:
C. Plan event persistence, saved plans, and resumeScope:
This leaf should consume and produce contract DTOs rather than import the executor. Saved data should be revalidated by an injected validator when it is eventually run. That allows this PR to land before the engine implementation: its tests can use fixture specs and events from the seam. The storage boundary should be complete in this PR:
Validation:
D. Model discovery and task selectionScope:
Explicitly exclude:
The selector can be tested as Validation:
E. Write-capable plans and workspace isolationThis should remain one security-atomic leaf rather than being split across approval, worktree, and grant PRs. Scope:
The leaf should not import the engine implementation. It implements workspace/capability contracts from the seam and is tested through contract fixtures. The engine activation later selects this provider only for plans whose validated capabilities require it. Validation:
F. TUI presentation and controlsScope:
The leaf should add the presentation components and test them with a fake event source/controller, but should not yet wire them into the process-wide Session ownership should be explicit in every event and control request. A presentation bridge should not mutate the durable owner of a live background plan when the visible session changes. Validation:
G. Background execution lifecycleThis could be part of the engine leaf, but it has enough ownership and shutdown behavior to justify a focused adapter PR. Scope:
It should not know about Bubble Tea or session file formats. It publishes lifecycle events and completion results to injected interfaces, so it can land before either the persistence or TUI implementations. Validation:
Step 3: extract unrelated fixes into their own PRsThe following do not need to wait for any orchestration seam and can target current
Each should carry its own production consumer test, not only a lower-level unit test. For example, completed-session retention should be tested through These fixes have value on Step 4: one small activation/integration PR lands lastAfter the desired leaves are on
I would make The activation PR is intentionally last because activation is the one thing that cannot precede its implementations. That does not make the leaves a stack: they can land in any order, remain inert until composed, and be reverted independently after activation if their contract has a fallback or the feature is disabled. Suggested landing matrix
This also permits a smaller initial product. For example, read-only foreground orchestration could activate after the engine and basic persistence land, while model auto-assignment, write-capable plans, background mode, or the full TUI surface remain unavailable until their independent leaves are ready. Capability availability should be explicit so an absent leaf produces “not available in this build/run,” never silent degradation to a less safe behavior. Mechanical extraction processI would use the current #829 branch as the source implementation and coordination ledger, not as the branch each leaf is based on:
Per-PR acceptance barFor each leaf I would expect:
Why this is worth doingThis is not only about making the review shorter. It changes the failure and maintenance shape:
I am offering this as optional direction, not insisting on these exact package names or PR boundaries. The important properties are: one small inert seam, leaf implementations that do not import each other, security-atomic write support, unrelated fixes removed from the feature, and one final composition PR with very little algorithmic code. That would preserve the substance of the work while making most of it independently reviewable, landable, and revertible. |
|
@jatmn I addressed the current-head review on What changed:
Regression coverage includes concurrent terminal collectors, production late Validation on the composed dependency stack (
A full The merge-order blocker remains intentionally unchanged: this PR is not standalone-buildable until #909 and #897 land on No new dependency or third-party module was added; |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
-
[P1] Keep this PR behind its two unmerged package dependencies
internal/agent/loop.go:16At
b6b38238, this branch importsinternal/measurementsfrom the agent and specialist paths andinternal/memoryfrom the tool/CLI paths, but neither package exists on this head or on its currentmainbase. The required Ubuntu, macOS, Windows, performance, security, and review jobs consequently stop during package loading or compilation instead of validating the feature. The author's composed-stack runs are useful integration evidence, but GitHub would merge the standalone tree represented by this PR.This is an intentional stacked dependency rather than an accidental omission, so please do not restore duplicate copies here. Address the root dependency ordering: land #909 and #897 first, refresh #829 onto the resulting
main, resolve any integration drift, and require the ordinary standalone CI matrix to pass on the final head before merging.
Findings
-
[P1] Enforce read-only access to the plan scratchpad
internal/specialist/plan_exec.go:847The scratchpad stores prior tasks' full outputs, and downstream tasks are explicitly directed there when an in-prompt excerpt is truncated.
ExecutePlanInplaces that directory inretryPolicy.readRoots;NewPlanRunnerforwards it asTaskRunOptions.ExtraReadRoots; andBuildArgssends that field throughappendExtraWriteRootArgs, producing--add-dir. A write-capable task therefore receives write authority over the evidence produced by sibling tasks. Because the scratchpad is also created below the generally writable temporary root, changing only the argument name may not remove the effective permission. A compromised or mistaken child can rewrite or delete an earlier result before a dependent reads it, changing the plan's conclusions without changing the recorded upstream result.Please fix the authority model at its root: keep the executor as the only scratchpad writer and make the effective child sandbox grant read-only regardless of whether that child also has write tools for its isolated worktree. Add an end-to-end sandbox test with a write-capable child that can read a prior scratchpad entry but cannot modify, replace, delete, or create files in that directory.
-
[P1] Suspend the wall-budget timer while a plan is paused
internal/specialist/plan_wall_clock.go:91watcharms a timer for the current remaining budget. At a task boundary, the executor recordspausedAt, blocks inWaitWhilePaused, and callsaddPausedonly after that wait returns. If ten seconds remain and the user pauses for eleven seconds, the original timer fires while the plan is parked; because no pause credit has yet been recorded,remaining()reports exhaustion,expiredis set, and the plan context is permanently cancelled. The cancellation itself wakesWaitWhilePaused, after which the credit is applied too late to revive the run. The existing clock test callsaddPausedsynchronously and therefore cannot exercise this ordering.Please address the lifecycle rather than adjusting the arithmetic after the fact: pause/resume transitions need to suspend or reschedule the active watcher before blocking, with synchronization that prevents a timer firing concurrently with the pause transition. Add an executor-level regression that pauses longer than the pre-pause remainder, resumes, and proves the next task runs while actual running time still exhausts the budget normally.
-
[P1] Reject present non-array dependency and tool fields
internal/specialist/plan.go:896planStringsStrictdirectly assertsargs[key]to[]anyand returns(nil, nil)whenever that assertion fails. It therefore gives an absent optional field and a present malformed value identical semantics. For example,"depends_on":"setup","depends_on":null, or an object value is admitted as no dependencies, allowing the task to run before the prerequisite it attempted to declare. A malformedtoolscontainer similarly becomes the default inherited read-only grant rather than an admission error. Provider schemas reduce the chance of malformed model output, but the runtime does not locally enforce those schemas, and hand-edited saved-plan JSON reaches the same parser directly. This exact issue was previously marked addressed, but current head again contains the failing branch.Please fix the parser contract, not individual callers: distinguish a missing key from a present value of the wrong type and fail closed for every present non-array/null container. Keep omission and valid empty arrays behaving as designed, and add regressions for malformed containers—not only malformed elements inside otherwise valid arrays—through both inline and saved-plan admission.
-
[P1] Propagate dynamic read grants through ordinary Task launches and resumes
internal/specialist/task_tool.go:109The PR introduces
--add-read-dirso sub-agents can inherit parentrequest_permissionsread grants without gaining write access, but the livescope.ExtraReadRootssupplier is wired only intoOrchestrateTool. A fresh ordinaryTaskconstructsTaskRunOptionswithoutReadOnlyRoots, so it cannot inspect a path the parent was just authorized to read. Resume is narrower again:BuildResumeArgsInputhas no read-root field andBuildResumeArgsemits only the executor's extra write roots. A child can therefore read an external path during an orchestrated task yet fail on the same path when launched normally or resumed. Existing tests cover the plan-specific path, not these sibling entry points.Please fix the shared child-execution boundary: give
Executoraccess to the live read-root supplier just as it has the live write-root supplier, and have both fresh and resume argument builders emit those grants exclusively as--add-read-dir. Add end-to-end coverage for ordinaryTaskand resumedTasklaunches proving the path remains readable but not writable. -
[P2] Reject fractional budget values instead of truncating them
internal/specialist/plan.go:954Tool arguments and saved-plan JSON decode numbers as
float64, andplanIntSetconverts every such value with uncheckedint(value). This silently changes the caller's budget:max_workers: 1.9runs with one worker,max_retries: 0.9becomes an explicit zero, andmax_wall_seconds: 0.9becomes zero and disables the wall bound entirely. Declaring these fields as JSON Schema integers is not a runtime guarantee, particularly for hand-edited persisted plans; current tests cover negatives and large whole timeout values but not fractional input.Please centralize exact integer decoding for all integer-valued plan fields. Accept only finite, representable whole numbers, reject fractional values before conversion, and preserve the current absent-versus-explicit-zero semantics. Use that one conversion path for worker, retry, token, wall, and stall limits, with table tests that prove each field rejects fractions rather than rounding or disabling a bound.
-
[P2] Do not overwrite user plans when staging restart or resume
internal/tui/orchestrate_saved.go:452Bare resume stages to
last_run_resume, restart stages tolast_run, and named resume stages to<stored.Name>_resume. These are ordinary valid names in the same project/user-visible namespace used by/plans save. All paths callSavePlan, whose atomic rename intentionally replaces an existing destination, so/plans restartcan silently destroy a user-authoredlast_run, and/plans resume sweepcan replace an unrelatedsweep_resume. The current tests verify that the generated file appears but never seed a colliding user plan. The comments explain why generated names are reused, but not why the command owns pre-existing user data under those names.Please separate internal staging ownership from user-authored storage rather than weakening atomic replacement globally. Use a private/ephemeral namespace or collision-safe identifiers and clean them after admission; at minimum, detect an existing user plan and refuse to replace it. Cover all three collision forms and verify that the original bytes remain intact after the command.
-
[P2] Expire terminal results from the completed-session store
internal/execution/process_manager.go:448The new immutable completed-result cache fixes late polling, but its cleanup is disconnected from the configured retention.
removeCompletedLaterwaits forCompletedRetentionand callsmanager.Remove(process.id);Removedeletes onlymanager.processes, whilerememberstores the terminal result in the separatemanager.completedmap. Consequently, the ID remains addressable throughCompletedandContinueafter the advertised retention period, until 64 newer completions happen to evict it or the manager is destroyed. That also retains the bounded output plus request, enforcement, change, and metadata fields longer than configured. Existing tests verify the minimum retention relationship and FIFO count bound, but not time-based expiry.Please make completed-result ownership include its expiration lifecycle: remove the matching immutable record when its own retention window ends without deleting a newer replacement or racing terminal publication. Add a deterministic short-retention test proving late polls work before expiry, fail after expiry, and that FIFO eviction still behaves independently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
You did what I asked and it broke the build, because what I asked for was incomplete. That is on me, so let me give the accurate version.
What happened
internal/measurements and internal/memory are gone from the branch, which was the ask. Their consumers are not:
internal/memory 0 files, still imported by
internal/cli/app.go
internal/tools/memory.go
internal/tools/memory_test.go
internal/tools/memory_tool_test.go
internal/measurements 0 files, still imported by
internal/agent/loop.go
internal/specialist/plan_runner.go
Neither package exists on main either, so nothing can resolve those imports and every check fails at load:
internal/tools/memory.go:9:2: no required module provides package
github.com/Gitlawb/zero/internal/memory
I said "drop those five files and let #909 and #897 own them" and never checked whether this branch also carried code that uses them. It does, in six places. Dropping a package while keeping its callers is the same defect shape I have written up elsewhere in the other direction, and I walked you straight into it.
The options, honestly
Restore the five files. Back to where you were: the branch builds, and whoever merges second resolves a conflict by hand in two packages whose subject is correctness. That is the cost I was trying to avoid, and it is a real cost, but it is smaller than a branch that cannot compile.
Drop the consumers too. Removes the memory tool and the measurements integration from this PR entirely. That is a different PR than the one you have, and I do not think it is what you want.
Stack on #897 and #909. This is what "let them own it" should have meant and I did not say it. Base this branch on those two, and the packages arrive from their owners with no duplication and no conflict. Both are open and mergeable today; #897 has changes requested and #909 has had no human review yet, so it is a real dependency rather than a formality.
I would take the third if you are willing to carry the dependency, and the first if you would rather not be blocked on two other PRs. Either is fine by me, and I am not going to make you re-litigate it a third time.
The rest
My earlier point about scope stands and I am not repeating it beyond this line: at +50470 I still cannot review the whole thing to the standard I applied to the small PRs split out of it, and that remains a matter for you and kevin rather than something I will keep raising.
Requesting changes only because the branch does not build. Say which option you want and I will help make it work.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Two things are settled, so you can stop working on them.
Keep the restored trees. @jatmn asked you to hold this behind #897 and #909 rather than restore copies; you restored them on my advice, and those two instructions contradicted each other. Settling it with evidence rather than a third opinion: the restored trees are byte-identical to #897's and #909's (internal/memory tree 46e3b4615c, internal/measurements ea9dfecf14 on both sides), all four merge orders of {829, 897, 909} into main are conflict-free, and the combined tree builds and vets clean. The duplicate-copy objection does not materialise. No action needed.
The scratchpad answer is not "make the grant read-only". Rerouting readRoots through --add-read-dir would remove exactly zero write ability: the pad lives at os.MkdirTemp(""), and sandbox.NewScope adds the OS temp candidates as write roots unconditionally (scope.go:46-50). That is base behaviour, byte-identical on main. Building the child scope the way internal/cli/exec.go:457 does and writing into the pad returns block=nil on both trees. Don't spend a round on it.
Requesting changes on what follows. Every finding is judged against this PR's own stated contracts, since main has no plan subsystem at all.
jatmn's scratchpad P1, answered once
The integrity contract does not hold, but the defect is the missing producer binding, not the write grant. The deliberate grant at plan_exec.go:842 is fine and I am not asking you to remove it.
What is false is plan_scratchpad.go:31-35: "NO TASK WRITES HERE... Write contention is not locked against; it cannot arise, because there is exactly one writer." Its premise at :22-26, "A READ-ONLY PLAN TASK HAS NO WRITE TOOL", is true, and is falsified as a basis for the invariant by the write-capable plan tasks this same PR introduces. Driven through a real ExecutePlanIn with ParentTools = PlanGrantableToolNames():
RequiresIsolation=true; Failed=0 status=completed
beta tools = [read_file write_file]
beta listed pad = [alpha.md]
beta write err = <nil>
synth read back = "FORGED" (alpha had produced 18349 bytes)
Nothing binds a record to the task that produced it: the filename is the bare task id, the directory is listable from inside a task, and the pointer states an expected length that nothing compares at read time.
Two things bound it honestly, so this is smaller than it looks: the only actor that can do it already holds write_file over the same shared worktree as its victim, which is a strictly larger target than a temp .md; and the dependent's prompt still carries the real excerpt inline. A read-only plan cannot do it at all.
The ask is small: correct the invariant comment, and give Record O_EXCL. That one change makes "exactly one writer" true and also closes finding 7 below.
P1. A budget number that arrives as a string silently erases the bound
internal/specialist/plan.go, planIntSet (~:954). The default branch returns (0,false) for a string, so planBudget reads a present key as absent:
numbers: max_tokens=(500000,true) per_task=(100000,true) retries=(1,true)
strings: max_tokens=(0,false) per_task=(0,false) retries=(0,false)
End to end through OrchestrateTool.RunWithOptions, a requested 500k plan bound reaches the live meter as 0 and a requested 100k per-task cap reaches the runner as MaxTaskTokens=0, which your own struct comment defines as unbounded. No error anywhere, and the bound is erased from Args(), so a saved plan loses it permanently.
This is reachable rather than theoretical: internal/agent/loop.go:1231 unmarshals tool args into map[string]any with no coercion, nothing validates args against Parameters() anywhere in the repo, and this repo already asserts that models do exactly this. internal/tools/args.go:105 reads "Some models send numbers as strings" and is pinned by a passing test. planIntSet does not use that layer.
Narrowing it fairly: max_wall_seconds "600" degrades to the 1h backstop and max_stall_seconds "600" degrades tighter, both safe directions. The genuinely unbounded pair is max_tokens and max_tokens_per_task, which are the two fields a user only ever sets because they wanted a spending limit. Note max_workers: "4" errors on the same input, so the class is visible for one field and silent for five.
planBudget's own comment at :702-712 names this failure and closes it for a negative max_wall_seconds, and planTask already uses planStringsStrict because a silently dropped depends_on ran a task before its precondition. Same treatment, or route through intArg.
P1. An ancestor write root defeats worktree isolation
internal/specialist/exec.go:525. appendExtraWriteRootArgs skips a root only when it is byte-equal to --cwd, never checking containment. The guard has the same gap: plan_isolation_test.go:57 compares only root == parentWorkspace. Reproduced through the real launch path, with TEMP/TMP/TMPDIR redirected so the automatic temp roots could not confound it:
child --add-dir = [<decoy-temp> <base>\dev]
child --cwd = [<base>\worktrees\plan-a]
write_file status=ok "Created <base>\dev\zero\internal\pwned.go (1 lines)."
The approval card says "worktree <name> at <path>", and this writes into the parent tree instead, with no branch and no diff. Precondition is a run holding a write root that is an ancestor of the workspace (config additionalWriteRoots, a root-level --add-dir ~/dev, or a session-scope grant) plus an approved write-capable plan. resolvePlanWorkspace correctly refuses rather than degrades when isolation is unavailable, which is what makes this the one way through.
Same function, second half. Scope.ExtraRoots() returns AddTemporaryWrite entries with no temp-versus-permanent distinction, so a turn-scoped grant leaves as a plain --add-dir and is permanent for the child's whole life:
during turn: parent ExtraRoots() = [<decoy-temp> <base>\secrets]
after turn: parent ExtraRoots() = [<decoy-temp>]
child write after release: status=ok "Created <base>\secrets\written-after-the-turn.txt"
The parent releases correctly; the child's argv already carried it. Bounded to children that outlive the turn. Both halves are one decision about what ExtraRoots may hand a child.
P2, five of them
max_tokens_per_task bounds one attempt, not one task (plan_exec.go:1012 + plan_runner.go:146). runTaskWithRetries passes the unreduced cap every iteration and NewPlanRunner allocates a fresh counter per call. Real path, cap 60,000, child emits one 50,000 event then goes silent: max_retries=0 → 1 child / 50,000; =1 → 2 / 100,000; =3 → 4 / 200,000, caps never reduced. Ceiling is (max_retries+1)x for the stall-only case, 2x at the shipped default. It bites hardest in exactly the configuration plan_tool.go:337 prescribes, "use it ALONE", because with max_tokens unset nothing bounds the sum but an hour of wall. Contradicts plan.go:81, plan_exec.go:282 and plan_tool.go:337, all of which say ONE TASK.
The live meter drops the event that trips the per-task cap, and every event after it (plan_runner.go:151). req.Spend.add(...) is a side effect inside the second switch case's condition, and Go stops at the first true case. Measured: cap set → spend.upstream = 40000; same child, cap omitted → 80000. Not cosmetic: plan_exec.go:788 gates downstream dispatch on limit - upstream, so an undercount inflates that ceiling for the rest of the plan (320,000 → 360,000 in a probe). plan_exec.go:1174 states the contract this breaks, "Both sum the same usage events, so they agree at the end." Hoisting the add out of the case condition fixes it.
A pause longer than the remaining wall budget kills the plan mid-pause (plan_wall_clock.go:83-107). The credit is applied only after waitWhilePaused returns and the clock has no notion of "currently paused", so watch wakes on the original deadline and cancels. That is the regression the comment at :725-730 says was fixed. Three identical runs with max_wall_seconds=1 and a 2s pause: elapsed=1.001s status=cancelled succeeded=0 cancelled=4, the pause itself cut short, zero tasks ever ran. Time-scale invariant: a 30-minute budget paused for 40 minutes fails the same way. TestPausedTimeIsNotChargedToTheWallBudget passes because it is a pure unit test with an injected now() that never starts the watch goroutine. Second half of the same line: the credit is unconditional but the pause blocks only the dispatch walk, so with max_workers=2 a 1.9s pause while a task was in flight refunded time in which real work happened.
Case-variant task ids collide to one scratchpad file (plan_scratchpad.go:95). ParsePlan's uniqueness check is an exact-string map and planIDPattern permits both cases, so Alpha and alpha are admitted as distinct tasks; Record joins taskID+".md" with no folding. Full plan run, Alpha emitting 18349 'U' and alpha 18349 'l': files in pad = [Alpha.md], and synth reads Alpha's pointer to get alpha's answer with a character count that matches, so nothing looks wrong. No adversary needed. plan_scratchpad.go:41-44 says the allow-list exists "because a task id becomes a filename", which is the property that fails here.
Record writes through a link already at the destination (plan_scratchpad.go:96). Bare os.WriteFile, no O_EXCL, no lstat, so a link planted under a not-yet-run task's id redirects the executor's write, which is the unsandboxed parent process. Unelevated Windows, victim outside every default write root: os.Link err=<nil>, Record returned normally, victim settings.json now holds model-chosen content. Worth knowing before choosing a fix: internal/pathjail would not catch this variant, since a hard link is not a reparse point and os.Root.OpenFile passed it through. O_EXCL does. Honest gap: nobody drove a real sandboxed child planting the link through its own tools.
Smaller, confirmed, none gating on its own
A failed task's full output is recorded to the shared pad and reachable by unrelated tasks (plan_exec.go:562), while the briefing deliberately refuses to pass it on. refuseUnreachablePerTaskCap fails open on int overflow (plan.go:424) sixty lines above refuseUnrepresentableSeconds, which already rules on exactly that hazard. Non-stall retries spend the stall budget (plan_exec.go:1127), so a decline-then-stall gets zero stall retries against a documented one. terminalStatus reports PlanCancelled for a plan whose tasks genuinely failed (:1148-1152), reaching Meta["plan_status"] and the durable event. And budgetLeft is a dead store: I poisoned it and multiplied its decrement by a million and the whole package still passed, including every budget test, while three comments still describe it as authoritative.
What I checked and found correct
Recording this so the next pass does not re-spend the time. The read/write channel split for parent grants is genuinely sound, and plan_read_roots_test.go is a real assertion rather than a name. The task-id allow-list is a true allow-list and nothing escapes by pathname. Release() does not recurse through a planted junction, which given this repo's junction history was my strongest suspicion going in. The pad root is MkdirTemp + 0700 per run, not a derived shared path. The scheduler held up under 120 randomised -race iterations of 4 to 11 task DAGs mixing panics, failures and concurrent cancels: outcome counts always summed, no task ever ran while a dependency was in flight, peak concurrency never exceeded the worker count, and slots cannot leak or double-return. The retry loop cannot spawn unbounded. The two-pool reserve holds apart from the leak above. NaN, +Inf, 1e300 and 2^63 are refused across every budget field. Grant derivation intersects against the parent unconditionally, depth is capped correctly, and the tool is not advertised under plan mode.
One consequence recorded without an ask, since per-plan isolation is your documented choice: nothing couples max_workers to write capability, so two tasks each granted write_file run concurrently in one tree with no locking. Two tasks holding bash would contend on .git/index.lock. Not asking you to change it here.
6 participants
How to review this PR
It is one PR by necessity — the areas share core files (
plan_tool.go,plan_exec.go,app.goare each touched by many commits), so they cannot becleanly split into separate PRs without breaking builds. But the changes are
independent by concern. Review area by area, top to bottom; each is
self-contained and these are the seams a split would follow.
orchestratetool — the zeromaxing rung and the DAG tool, validated by one constructorplan.go,plan_tool.go,plan_gate.go,plan_keyword.goplan_schedule.go,plan_exec.go,plan_events.goplan_identity.go,plan_resume.go,orchestrate_saved.gorequest_permissionsREAD grants via a new--add-read-dirflag, applied read-only (never writable)sandbox/scope.go,cli/exec.go,cli/exec_parse.go,plan_runner.gomin_sizedecency floor; the router is shown size labels;providers modelsdisplays themplan_model_size.go,plan_model_assign.go,plan_model_router.go,config/types.gotui/orchestrate_*.go,sidebar*.go,specialist_card.go,worker_view.goEvery production change has a regression test beside it (~61% of the diff is
tests); the tree builds and
go test ./...passes; with the posture off theorchestrate tool is unadvertised and no plan machinery runs (see Verification
for the precise additivity claim and its one documented exception).
Summary
Adds zeromaxing — an explicit, opt-in posture that lets a turn spend more to
get a more exhaustive answer — and the plan orchestration it exists to drive.
The posture is a rung above
highon the existing effort ladder, reached by/effort zeromaxing,/profile zeromaxing, or--exec-profile zeromaxing. Itraises the turn budget, widens the sub-agent allowance, and advertises one new
tool:
orchestrate, which takes a declared DAG of tasks and runs them as childagents, in parallel where the dependency graph allows.
The whole feature is additive. With the posture off, nothing here executes.
That is not an aspiration — it is the property every commit on this branch was
checked against, and the check is in "Verification" below.
What landed
zeromaxingrung; lifecycle reminders injected below the cache breakpointorchestratetoolmax_workers1–16 walks the validated topological order. Measured 40.0s → 27.0s at 4 workerssmall/medium/large/unrestrictedtiers; project config may only tighten what user config sets/plansverbs: list, save, run, resume, restart, stop, pauseDesign notes worth review
No new store. Plan state is derived from five session events beside the
existing specialist ones. Resume is a reduction over them, which is why a plan
recorded by the TUI and one recorded by
zero execresume identically.One scheduler, not two. The sequential path is the concurrent one with a
single worker. Two executors would have been easier to write and impossible to
keep in step — a duplicated rule drifts, and a duplicated executor drifts faster.
Optional interfaces over name switches. Control, per-task progress, isolation
and concurrency each arrive through a type-asserted optional half, so a recorder
that only records is unaffected and no existing signature changed.
One plan per surface. The panel holds one plan and the card table is keyed by
task id — unique within a plan, not between two.
PlanSurfaceBusyenforcesthat at the tool, on the path the model drives, matching the guard
/plans restartalready had on the path a user drives.Verification
Additivity, the load-bearing claim. With the posture off, the orchestrate
tool is unadvertised and no plan machinery runs; for a write-capable run the first
HTTP request body is byte-identical to an
origin/mainbinary. One documentedexception, orthogonal to the posture: a run holding no mutating tools omits the
~5 KB confirmation-policy block (
runCanMutate), so a read-only run's prompt issmaller than a pre-feature build — deliberate, fail-closed, and independent of
whether the posture is on. Re-proven after
every commit; both binaries built fresh, baseline from a clean
origin/mainworktree:
Gauntlet.
go build ./...,go vet ./...,go test ./...all pass.gofmtclean. Concurrency-sensitive packages run under
-racewith repeat counts.Mutation checking. Every fix on this branch was verified by reverting the
production change and confirming the test fails. Several tests passed initially
for the wrong reason and were rebuilt until they bit — the misses are recorded in
the commit messages rather than quietly fixed.
Fan-out measured end to end, not asserted:
max_workers=1→ 40.0s,max_workers=4→ 27.0s on the same plan.Known gaps
is hardened; the remaining ~60 call sites are their own campaign.
Notes for the reviewer
This is a draft, and deliberately so: it is large. Per
CONTRIBUTING.mda PRneeds an approved parent issue and one PR should carry one change — this carries
a feature. Happy to split it along the seams above (posture rung / orchestrate
tool + executor / durability + resume / TUI surface), each of which builds and
tests independently, if that is the preferred shape.
UI changes are best seen running; screenshots can be added on request.
Summary by CodeRabbit