feat(specialist): zeromaxing posture and orchestrate plan execution - #829
feat(specialist): zeromaxing posture and orchestrate plan execution#829gnanam1990 wants to merge 128 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. |
…ents A bare Task delegation inherited the parent's model unconditionally, so under zeromaxing every sub-agent ran on the session model no matter what it was for. A Task that names no model is now routed to the configured role pin — the plan path's own classifier and pins, the grant outranking the prose — gated on the posture AND the autoAssign config, so a posture-off spawn stays byte-identical. Four defects found and closed while verifying the seam end to end: - A resumed task drifted back to the parent's model. runResume applied neither the call's model nor the assigned one; it now recovers the model the session actually ran on from the session store, with an explicit model on the resume call winning exactly as on a fresh launch. - A plan task could be second-guessed at dispatch. Plan tasks flow through the same executor, and a model the plan tool deliberately left empty (a reported decision, or auto_assign:false) would have been re-assigned blind — including to a pin the plan level had passed over as not served. autoTaskModel now refuses plan-authored manifests, keyed on the provenance the manifest already carries. - Naming an uncurated model killed the spawn. applyTaskModel forwarded the posture's raised reasoning effort unconditionally; the child only clamps efforts for models the registry knows, so providers that reject the parameter refused the whole request — a real orchestrator lost three children to it and retreated to inherit-everything. The effort now forwards only where the registry can vouch, the same gate the plan path already applies. - A stale pin after a provider switch killed the spawn. Pins named Ollama models while the session ran on xai, and three children died with not-found. A pin now fires only when the provider's own listing carries both the pin and the session's model (the plan path's provider-mismatch guard); anything uncertain inherits. One listing is cached for two minutes, so a five-agent fan-out costs one probe. Every load-bearing line is mutation-checked: the pin lookup, the manifest-model guard, the posture gate, the session-model recovery, the explicit-resume-model apply, the plan-provenance skip, the effort vouch gate and the served-set guards each have a test that goes red when the line is gutted.
…te-aware animation, settled cancels While the posture is on, the workspace wears it; off, every surface is byte-identical to before (each gate mutation-checked). MODELS panel: the sidebar grows a section showing the live mix of models the fleet runs on — a proportional mix bar plus a row per model with live counts, colours stable per model name. Absent until an agent runs on a known model, so a plain session's layout is untouched. The skin: an electric gradient (theme blue blended into the accent — deliberately two-tone) paints the always-on surfaces: the composer box on all four edges, the chat|sidebar divider as a full-height rail, the sidebar section headers, and the Working ripple. The gradient sweep is mirrored, so no seam ever reads as "half painted". No new timers: every walk reads the spinner clock that already runs during a turn, and reduced motion pins all of it. State-aware animation: thinking drifts, writing races (4x), and orchestrating gets a scanner — a bright band sweeping the composer bar while plan tasks are actually running. The Working ripple's wavelength follows the same states. Plan bars join the skin: done work fills with the gradient, the running head burns accent, failure stays findable in calm dimmed red, and the update_plan checklist gets a progress bar of its own (the orchestrate bar's renderer, so the two cannot drift). Click offsets for plan steps account for the inserted bar row, test-pinned against the rendered sidebar. Settled cancels: cancelling the run kills the children but the trackers were never told — agents kept their spinner, ticking clock and "live" mark in MODELS beside "Run cancelled." in the transcript. cancelRun now marks running specialists and orchestrate tasks cancelled (not failed — stopping a run is not a defect), freezing their clocks and dropping the live counts.
The stock #50fa7b green and #ff5555 red are the highest-chroma signals in the whole registry, and in a zeromaxing session — success ticks and error rows arriving constantly — they read as alarms, not status. The success and failure roles now wear Dracula's signature cyan and pink, which sit naturally beside the purple accent and stay clear of amber's permission meaning. The palette token names stay green/red (every consumer binds to them as success/failure); diff colours keep their conventional hues — a diff is a diff on every theme. The registry-wide contrast floors still hold.
The status card stacked three renderers — a labelled list, the resolved
posture line, and the delta sentence — so the profile name appeared
twice and the effort and turn budget three times each. It is now a
single sentence of key-value clauses: each fact once, the transitions
as clauses on the row they qualify ("was 80", "raised by the posture",
"NOT raised — high is unsupported on this model"), no section header
and no hint advertising the command the user just typed.
Every honesty guarantee the old card's tests enforced survives on the
one line: the refused effort level is named, the self-correct claim
tracks live state ("your /selfcorrect off overrides the posture"),
exactly one effort-transition claim exists, and the escalation row keeps
the headless-only asymmetry. /effort still carries the shared delta
sentence verbatim, so the switch notice and that surface cannot
disagree. Also learned and recorded: compactCommandOutputText collapses
every whitespace run, so padded column alignment can never survive this
renderer.
…roots The exec wiring handed children execScope.Roots() — the parent workspace root included — as extra write roots. For a worktree-isolated plan that defeats the isolation outright: the worktree exists so a write-capable plan cannot touch the parent tree, and this line was handing the parent tree back as a writable --add-dir. The TUI has always passed only scope.ExtraRoots; the two surfaces now agree, through a named helper the regression test drives with a real sandbox scope.
Entering the posture through /effort zeromaxing is guarded by the idle-session rule; leaving it through /effort auto reached revertExecProfile — moving the turn budget, the self-correct setting and the shared orchestrate gate — with no guard at all, leaving one turn running under two different budgets. The same rule now guards both doors. Plain /effort auto outside the posture keeps working mid-run: it only clears the effort selection, which mutates no shared budget.
A saved plan's prompts are model-authored text read back from disk — the same trust level as a live task's — and planTaskSummaryLine only cut at the first newline, so control characters (ESC included, the byte that starts an ANSI sequence) reached the display. It now routes through sanitizeCardText, the chokepoint the live path already uses.
absolutePathPattern only matched POSIX-style paths, so on Windows the reach check silently never fired: prompts naming C:\ paths contributed no candidates, and four tests showed it. The pattern now takes both separators and an optional drive letter, plus '~' for 8.3 short names (RUNNER~1), which real CI paths contain. pathInsideRoot keyed on the single platform separator, answering false for every /-separated pair on Windows; both separators are boundaries on every platform now. Also makes the completed-session retention test cross-platform (cmd /c on Windows) instead of assuming /bin/sh — the behaviour under test is OS-agnostic, so it runs everywhere rather than skipping.
make deadcode (-test=false) flags production functions whose only callers are tests: ExecutePlan, withDependencyBriefing and autoAssignModels in specialist, markDone and joinColumns in tui. Each was a deliberate convenience seam with documented rationale; they keep that rationale and move to export_test.go, the repo's home for test-only helpers, so the production binary carries none of them. SortedPlanSizeNames had no callers anywhere and is deleted. Also clears the three staticcheck findings the advisory lint reported in PR-owned code: a tagged switch in plan_model_assign, time.Since in a test, and a self-comparison rewritten as two named renders.
NTFS ignores trailing dots, so Lstat("…/streamer.go.") succeeds for the
file named "…/streamer.go" — the exact-hit branch returned the
sentence's full stop as part of the path, and the reach check compared
the wrong spelling. When the trimmed spelling names the SAME file, the
canonical name wins; a POSIX file genuinely named with a trailing dot
keeps its exact spelling, which the new cases pin (dotted-only, distinct
dotted sibling, and the plain trim), with the unconditional-rewrite
mutation killed by them.
A plan fans out, every task reports, and the report carries what they said — under zeromaxing that is five or more sub-agents' claims, each written by a model that cannot see the others' work and will never be asked about it again. The bundled research plan ends in a refute step for exactly this reason and planTaskSystemPrompt's evidence rules exist to make one possible, but both are habits: a plan whose author forgot a verifier finished green with nothing checked, and a wrong "looks fine" is indistinguishable from a right one once it reaches the report. Under the posture, a multi-task plan that names no verification now gets one appended — a task depending on every other, told to REFUTE and to default to refuted when uncertain, because a verifier that sets out to agree always does. It classifies as verify, so the user's verify pin routes it to their strongest model, and it works on the ARGS before ParsePlan, so it is validated by the same constructor as a hand-written task, round-trips into a saved plan, and re-admits on resume. THE APPEND MUST NEVER BE THE REASON A PLAN IS REFUSED, and two ceilings can do that. The size tier caps task COUNT. The budget floor is the one the existing suite caught: refuseImplausibleBudget requires max_tokens to fund 50,000 per task, so one more task raises the bar by 50,000 and a plan whose budget exactly funded its own tasks was rejected outright — for a task its author never wrote. Both have a guard and a test. It also names NO tools: planToolGrant then resolves the read-only intersection of the parent's grant, so it can neither modify anything nor flip the plan into worktree isolation (isolation is plan-level, the measured failure plan_workspace_reach.go exists to catch). It defers entirely to a plan that already verifies, moves its id aside on a collision, leaves a malformed task list to ParsePlan, and reports itself in the run's output so a task nobody wrote never appears unexplained. Each guard is mutation-checked: gutting the posture gate, either ceiling, the already-verifies check or the id probe fails its own test.
…le field Ranking spanned everything the provider serves, so buildModelTiers read the cheap tier off the SMALLEST model on the account and the router was handed every candidate to choose between. On a provider listing twenty models that is a toy running a scan while nineteen better ones sit unused — the reason 20b-class models kept turning up on tasks that had a 120b available. rankedEligibleModels now keeps the most capable N (default 10) after exclusions and the size floor, so the tiers span the good models rather than the full catalogue and the router chooses from a shortlist worth choosing from. Configurable as planModels.topModels for anyone whose provider warrants a wider or narrower field. THE TAIL, NOT THE HEAD: the list is sorted least-to-most capable, so the shortlist is its end — taking the front would keep exactly the toys this drops. Fail-open throughout: a provider offering fewer than the cap keeps all of them, and a zero or negative cap means the default rather than none, because a zero value must never leave a plan with nothing to assign. A PIN IS UNAFFECTED. Pins are validated against what the provider serves, never against this list, so naming a model outside the top N still routes to it — an explicit instruction outranks a heuristic. Tested, along with the fail-open cases; gutting the shortlist, taking the head instead of the tail, or reading a zero cap as none each fails its own test.
…empt A measured ten-task run lost one task to a bare "Internal Server Error" after 6 tool calls and 42,524 tokens, and everything depending on it went with it. Nothing about the task produced that. Two layers each declined to retry, and only one of them was right to. The transport does not replay a 500/502/504 on purpose: unlike 429/503/ 529 they do not guarantee the request had no effect, so replaying the same POST could pay for the same completion twice (providerio.ShouldRetryStatus). That is correct for a replay. The plan's own loop then returned on "anything other than a stall is an ANSWER, including a failure" — true of a task that read the code and concluded wrongly, which will conclude the same thing, and not true of an HTTP 500, which is a coin flip. A fresh child is not that replay; it is a new request the provider is very likely to answer. So the loop grows a third carve-out beside the two that already exist for "not the task's fault" (ModelRejected, Declined): one more attempt when the child died on the provider, recognised from its exit code the way Declined is — a flag, never a message match. READ-ONLY TASKS ONLY, and that bound is the whole safety argument. A write task may have applied part of its change before the provider died and no exit code says how far it got, so re-running it could apply that change twice. A read-only task re-reads, which costs tokens and nothing else — the same asymmetry the no-tool-call check rests on. Bounded at exactly one by its own flag (a provider failing twice is having an outage), gated by the same cancellation and wall-clock stops as every other retry here, and NOT gated by max_retries, which bounds stall thrashing rather than this. Both attempts' spend is reported. Five mutations, five tests: removing the branch, dropping the write-task gate, dropping the one-retry bound, never setting the flag from the exit code, and drifting the duplicated constant each fail. The flag test is separate on purpose — gutting the classification left the four executor tests green, which is the layer-join defect this package keeps relearning, so one test drives the real runner. The constant is now pinned on BOTH sides; pinning only cli's left specialist's free to move silently.
Two defects a high-effort review found in the top-ranked shortlist. THE SHORTLIST INVERTED COST ON PRICED PROVIDERS. The ranked list is sorted least-to-most capable, but "capable" is measured by SIZE on a free provider and by COST on a priced one — so taking the tail meant "keep the biggest" in the first case and "keep the DEAREST" in the second. Measured on a 12-model priced fixture: the cheap tier moved from cost 1 to cost 3, and on a real 30-model account every scan-role task in every auto-assigned plan silently moved an order of magnitude up in price, reported only as "scan -> <model>". A priced catalogue is now never narrowed. The asymmetry is the point: on a free provider the small models cost nothing and buy nothing, so dropping them is pure gain; on a priced one the cheap end is exactly what the cheap tier exists to spend. catalogueIsPriced is extracted as one authority so the ranking and the shortlist cannot disagree about what a free provider is. A TRILLION-PARAMETER MODEL PARSED AS SIZE ZERO. modelSizeToken accepted only the b/m suffixes, so "kimi-k2:1t" read as unknown and sorted BELOW gpt-oss:20b on a free provider — the largest model on the account became the cheap tier every scan task was routed to, and the first thing the shortlist discarded. The suffix class now covers t. Both mutation-checked: re-narrowing a priced catalogue, and dropping the t suffix, each fail their own test.
…or's plan Two more defects in the auto-appended verification task, both of the same shape: a task the author never wrote changing the outcome of the plan they did write. A THIRD BUDGET CEILING. The headroom check covered refuseImplausibleBudget (max_tokens vs 50,000 per task) but not refuseUnreachablePerTaskCap (max_tokens vs max_tokens_per_task x taskCount), so the extra task raised that bar by one whole per-task cap. A 4-task plan with max_tokens 400,000 and max_tokens_per_task 100,000 parsed fine and was then refused outright — naming a task count and a budget its author never chose. Any plan whose per-task cap exceeds the 50,000 floor falls in that window, so the floor check alone never covered it. THE VERIFIER'S FAILURE FAILED THE WHOLE PLAN. It runs last, on the strongest tier, with the largest dependency briefing — the task most likely to stall or exhaust its provider retry — and terminalStatus counted it like any other, so three succeeded author tasks plus one stalled verifier returned StatusError and the orchestrating model re-ran a plan whose work was already done. When the appended task is the ONLY thing that did not succeed, the call now stays OK and says plainly that the claims were not independently verified and that re-running would repeat completed work. The report stays honest either way: Failed still counts it and the summary still names it; only the verdict on the author's plan is left to the author's tasks. appendVerifyStage returns the id actually used, so a suffixed id cannot match the wrong task. Both mutation-checked.
CANCEL CLOBBERED A LIVE BACKGROUND PLAN. cancelRun settled the trackers unconditionally, with none of the BackgroundPlanLive guard beginRun makes a few lines above — so stopping a foreground turn marked a background plan's tasks and sub-agents cancelled while they kept spending tokens and writing files. The mirror image of the defect the background-status tests exist to prevent. Foreground children still die with the run context and are still settled. A RESUME BLIND-PINNED A STALE SESSION MODEL. runResume applied session.ModelID with no served-check, so a sub-agent recorded on one provider and resumed after a switch was spawned with a model that provider has never heard of and died at spawn — the failure autoTaskModel already guards against. The recorded model is now REJECTED ON EVIDENCE rather than requiring proof, the opposite direction to autoTaskModel and deliberately so: this restores a model the session demonstrably ran on, so no discoverer, a failed listing or an empty one all leave the recovery in place. THE PROVIDER RETRY FIRED ON PERMANENT FAILURES. Exit code 3 means "the provider failed", but internal/cli returns it for every agent-run error — an expired key, an unknown model, a quota wall — so an expired key cost a ten-task plan twenty spawns to produce the same ten auth errors. The budget is now PLAN-WIDE (two, mutex-guarded because tasks dispatch concurrently) instead of per-task: a genuine blip is still retried, a systemic failure costs two extra children for the whole plan. THE SESSION BUDGET CHARGED FOR RESUMES AND REFUSED CALLS. admit() ran ahead of prompt validation and ahead of the resume branch, so iterating on one sub-agent's answer thirty times consumed thirty of the session's slots while spawning nothing. It now runs on the fresh path only, after the checks that can refuse the call outright and still before every side effect. NON-LATIN LABELS WERE REJECTED AS NON-WORDS. hasNameLetter tested ASCII ranges, so every CJK, Cyrillic, Greek and Arabic token was dropped and the sidebar row lost its label — a silent degradation for anyone not writing in English. unicode.IsLetter/IsDigit is the same question asked correctly. DRACULA'S SUCCESS COLOUR COLLIDED WITH ITS BLUE. The recolour set green to #8be9fd, byte-identical to that palette's blue, collapsing two of the four series colours modelMixPalette cycles through so two models in the MODELS bar could render indistinguishably. Success moves to a teal in the same cool family; the registry-wide contrast tests still pass. Also drops TestZeromaxingSkinPreview, a visual preview with no assertion that could not fail. Each fix is mutation-checked: gutting any one fails its own test.
…ot read The ranking sorts an unparseable id as size 0 — the least-capable end — so taking a plain tail deleted exactly the models whose names do not state a parameter count. On a real ollama-cloud account that removed kimi-k2.6, glm-5.2 and deepseek-v4-flash from routing while gpt-oss:20b survived, and the operator's own router guidance calls the first two the strongest reasoners on that machine. Dropping a model because its name is uninformative is a parsing accident, not a capability judgement — and applyMinSizeFloor in this same file already follows exactly that fail-open rule. The cut now applies to SIZED models only: every unknown-size model is kept, and the top N of the ones that can actually be compared join them, with order preserved so cheap/balanced/strong still read off the ends. On the fourteen-model catalogue this was measured against, the shortlist now drops only phi4:14b — small AND sized — and the cheap tier becomes deepseek-v4-flash rather than gpt-oss:20b.
…t a size
Two regressions the review found in the previous two commits, plus the
hole that made one of them reachable.
THE CAP WENT INERT ON ANY PRICED CATALOGUE. Exempting priced catalogues
from the cut fixed the cost inversion by removing the bound entirely, and
catalogueIsPriced is an ANY test — so one priced model among three
hundred disabled the cut for all three hundred, planModels.topModels
contradicted its own documentation, and the router prompt lost its only
bound (measured 2,194 -> 22,985 characters on a 300-model catalogue, paid
once per plan on the strongest model).
The cut now keeps BOTH ENDS of the ranking instead of exempting anything:
a third from the least-capable end, the rest from the most-capable. That
satisfies every reader of the list at once — the cheap tier reads
eligible[0], the strong tier reads the last, and the router gets a
bounded shortlist that still spans the real range. It also preserves the
unsized models without a special case, since an id whose size cannot be
read sorts at the cheap end.
VERSION TEXT WAS READ AS A PARAMETER COUNT. The suffix boundary was
[^a-z], which a DIGIT satisfies, so "deepseek-r1t2-chimera" matched
"1t" followed by "2" and reported a trillion-parameter model — on a
free provider that would have made it the strong tier every judgement
task was routed to. The b/m suffixes always had the same hole
("r1b2-x" read as 1B); adding t is what made it reachable on a real id,
so the boundary excludes digits for all three rather than for the one
that was reported.
Both mutation-checked, and the boundary now has a test naming the exact
reported id — reverting it alone previously failed nothing.
…plan runs The fix for "cancel must not stop a background plan" over-corrected into its own opposite. The guard wrapped BOTH cancelRunning calls in BackgroundPlanLive, and the specialist tracker holds foreground and background children TOGETHER — so one live background plan left every FOREGROUND sub-agent spinning forever: still specialistRunning, still showing its current tool, still counted live in MODELS, over a process that died with the run context. Nothing else settles it, because a late completion is dropped by the stale-run guard once cancelRun has zeroed activeRunID. That is verbatim what TestCancelSettlesEveryRunningAgentAndTask exists to prevent, and the background test missed it because it had no foreground child. Children now carry the distinction themselves. specialistInfo gains a background marker, set from the two places that can know: a background plan's task-start message carries the flag, and a specialistRebindMsg is only ever emitted for a background Task spawn. cancelRunning skips marked rows and settles the rest. The orchestrate panel keeps its whole-panel guard, because one plan runs at a time and there is no mixed case there. The new test is the mixed state neither existing test covered: a foreground child and a background child under a live background plan, asserting the first settles and the second does not. The background test now marks its row the way production does, rather than passing because nothing settled at all. specialistInfo's new field is added to specialistCacheFingerprint, as the render-cache guard test requires — without it a card that changed through the field would keep serving its previous render.
Rebasing onto main flattened the two merge commits this branch carried, and in doing so replayed an older value of postureOffDefinitionsFingerprint over the current one — reverting it to the pre-Gitlawb#838 hash and dropping the comment recording why it had moved. The constant is a frozen golden over the tool DEFINITION bytes, and main's Gitlawb#838 reworded four tool descriptions (glob, grep, list_directory, read_file), so the correct value on this base is the one the branch already had. TestPostureOffToolDefinitionsMatchGolden proves it: with the reverted constant it fails with got 51ffc34f… want 3693b93a…, and the 'got' is what the fixture's tools actually hash to now. Restored with its rationale intact, since that comment is the thing that tells the next reader the move was main's schema change rather than a posture leak from this branch.
Gitlawb#867 landed on main while this branch was being rebased, reshaping read_file's schema to the canonical path/offset/limit contract and rewording its description. The golden hashes the tool DEFINITION bytes, so a description change on main moves it — the same way Gitlawb#838's rewording did before. The new value is what the fixture's tools actually hash to on this base, and it is the value CI computed independently: the macOS and Ubuntu smoke jobs both reported got ea7b4e64… against the old constant, which is the figure recorded here now. This is main's schema change, not a posture leak from this branch. The byte-identical guarantee is unaffected: what the guard protects is that a posture-off run's definitions match a binary built from the same main, and both sides moved together.
4bc2857 to
41fa169
Compare
|
@gnanam1990 separate from the two findings above, and worth doing first: please merge main into this branch. This branch cannot be hand-tested on Windows right now. Copying any transcript output panics the whole TUI: That is your own #876, merged to main as cae0269. This branch's last merge from main brought in up to cd0eb19, which is just before it, so the fix is missing here and the row prefixes that trigger it ( It hit us twice today trying to exercise the posture by hand. I ended up cherry-picking cae0269 onto 2f399f7 locally just to get a binary that survives a copy, which is when it became obvious this is worth telling you rather than working around. It matters more than usual for this PR specifically: most of what changed here is TUI, so "run it and look at it" is the main way to review it, and right now that path ends in a panic for anyone on Windows. |
|
Behavioural note from hand-testing this branch. Not a code defect, and not necessarily attributable to anything in the diff. Filing it here because it is evidence about how the posture behaves in practice, which is hard to come by otherwise. Under
The timestamps say otherwise. The workspace was seeded at 21:41:12 with only Two reasons this is worth having on this PR rather than shrugged off as model noise. The posture's whole pitch is more rigour, so a confidently false completion report is the failure mode it should be least prone to, and a verify stage that checks the work but not the report will never catch it. And the phrasing dresses the confabulation as scrupulousness, "one honest caveat", which is worse than a plainly wrong answer because it reads as trustworthy. Smaller observation from the same run: the task was written to fan out, three independent files then one depending on all three, and no plan was produced at all. It ran as a single turn of tool calls. Orchestrate never engaged on a task shaped to trigger it. That may well be intended, but it does mean the plan path is harder to exercise by hand than I expected, which is worth knowing given how much of this PR only shows up when a plan runs. |
|
@CodeRabbit full review |
|
detectShellRuntime probes PowerShell by starting it in this process, which is
unrestricted, and the command then runs inside the sandbox, which is not. On
Windows the sandbox wraps commands in a WRITE_RESTRICTED token, and PowerShell
is a .NET program whose crypto initialization fails there:
System.DllNotFoundException: Unable to load DLL 'BCrypt.dll'
or one of its dependencies (0x8007045A)
So PowerShell answers "usable" when asked in the parent, is selected, and is
then unable to run anything at all: exec_command and bash could not execute so
much as `echo` under the sandbox. The probe was asking a different environment
than the one that had to answer.
buildBashCommand is the single execution site for both tools, so it now resolves
the shell through the engine that will wrap the command. When PowerShell cannot
start there, detection falls through to cmd.exe, which does survive the token
and was already the documented fallback -- no new fallback path, just an honest
probe reaching the existing one. The sandboxed answer is cached separately from
the unsandboxed one because the two environments genuinely disagree; sharing one
cache would let whichever question was asked first answer both.
This is the same restricted-token incompatibility class already recorded for
Schannel TLS and MSYS2/Cygwin in internal/sandbox/windows_command_runner_windows.go.
Verified by running the feature on all three platforms against the real binary,
same commit, A/B: Windows 6/7 -> 7/7, macOS 7/7 and Linux 7/7 unchanged. The
added test pins the contract the fix relies on -- that a failed probe reaches
the cmd.exe fallback -- but not the wiring itself, which would need lookPath
injected into buildBashCommand; the cross-platform run is what covers that.
Known gap, not addressed here: the bash/exec_command tool descriptions still
advertise PowerShell syntax, because they are built in NewScopedBashTool, which
receives no engine. Sandboxed Windows now runs cmd.exe while the model is told
otherwise. shellGuidanceForRuntime already contains the correct cmd.exe wording;
reaching it means threading the engine through tool construction.
4 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