feat(specialist): zeromaxing posture and orchestrate plan execution - #829
Draft
gnanam1990 wants to merge 87 commits into
Draft
feat(specialist): zeromaxing posture and orchestrate plan execution#829gnanam1990 wants to merge 87 commits into
gnanam1990 wants to merge 87 commits into
Conversation
zeromaxing is thorough with double the turn budget again: 320 tool-turns, effort high, self-correction armed. It fills only what the caller left unset, exactly as the other profiles do, so an explicit flag and a --mode preset both still win — precedence stays enforced by ordering, not by a new check. ONE NAME. /effort zeromaxing, /profile zeromaxing, --exec-profile zeromaxing and --reasoning-effort zeromaxing all name the same thing, and nothing else does. No aliases: "max", "deep" and "deepmode" resolve to nothing, asserted by test, because a second spelling is a second thing to keep in step. "max" is RESERVED and untouched. ValidReasoningEffort still accepts ReasoningEffortMax, so /effort max and --reasoning-effort max behave exactly as they did — parsed as a raw provider level, rejected by the flag parser, and unsupported by every current model. That spelling stays free for a real provider rung; claiming it for a Zero posture would burn it. HONEST DELTA, stated to the user rather than left in a commit message: the turn budget is the ONLY mechanical change over thorough. Effort is already at the ceiling and self-correction is already on. execprofile.Delta carries that sentence to /effort, /profile and the exec selection notice. ReasoningEffort is "high", pinned by test rather than by comment. TestZeromaxingReasoningEffortStaysHigh explains why: every level above "high" falls through the providers' effort mappers into their default arm — anthropic and gemini both compute a thinking budget of 0 (extended thinking DISABLED) and the openai mapper drops the field entirely. Raising it would silently turn reasoning off while the UI claimed a higher posture. Raising the real ceiling means teaching the providers those tiers first. The raised budget propagates to spawned sub-agents. Deliberate — this is a maximal posture — so it is asserted by test and stated in Delta rather than left to be discovered. SelectionRefusal is the single authority both selection paths consult. The headless exec path and the TUI paths already apply a profile's knobs through different code with different state; letting each decide SELECTION independently is how a rule gets applied to one call path and omitted from its sibling. Config gating follows mergeProjectConfig's tighten-only template: a project .zero/config.json may set profiles.disableZeromaxing (DISABLE) but can never clear it, so a cloned repo cannot switch a cost multiplier on for whoever opens it — an attempted enable is dropped silently, exactly like an ignored network "allow".
Four model-facing notices tell the model when its own posture flips: enter (once, on the first turn after activation), a budget guideline beside it, still-on (every continuing turn), and exit (once, after deactivation). WHERE THEY GO IS THE WHOLE DESIGN. Every notice is appended to the CONVERSATION tail as a user-role message, on the same channel as the existing diagnostics nudge and the failure/plan hints. None of it reaches buildSystemPromptParts. The system prompt and tool definitions are the provider's cached prefix — the anthropic mapper puts its cache_control breakpoint on the last system block and the last tool — and Gitlawb#760 made that prefix build once per run precisely so it stays byte-identical across turns. Per-turn text above that breakpoint invalidates the cache every turn, roughly doubling input cost, and nothing in the system reports it. So ZeromaxingStillOnNotice is a fixed literal: no turn counter, no remaining budget, no timestamp. A test asserts it carries no digits at all, because the hazard is not today's placement but a future edit that adds a varying part and then moves the text into prompt assembly. The reminders also promise NO orchestration — no workers, no fan-out, no workflow tool. Phase 1 ships none of that, and a prompt advertising capabilities the run does not have is a prompt-level lie the model will try to act on. A test enforces the vocabulary. The state machine is one pure function of (posture, turn) with no memory, so "enter exactly once" and "still-on never on the first turn" are assertions about a table rather than about observed side effects. Proof, both shapes of the hazard, each caught by a DIFFERENT assertion: - per-turn text written into messages[0] -> the prefix-extension assertion - static text baked into the system prompt -> the explicit leak assertion TestRunPreservesRequestPrefixAcrossTurnsUnderZeromaxing is the zeromaxing variant of the existing prefix test; the openai one is its wire-level sibling, proving the mapper serializes an append-only conversation into an append-only request body. A third openai test pins that the posture name can never appear in a serialized request at all. Options.Zeromaxing is separate from Options.Profile because the posture arms no escalation triggers, so Profile.Policy() returns nil for it and could not carry this. Its zero value is Off, leaving every existing caller byte-identical.
…paths
/effort zeromaxing is the primary way in; /profile zeromaxing is its alias. It
is handled beside "auto" with an EARLY RETURN, before the ReasoningEffort
conversion and before ValidReasoningEffort — it is a posture name, not a
provider level, and must never become one.
It DELEGATES to handleProfileCommand rather than re-applying the knobs, so
"both entry points resolve identically" is true by construction instead of by
two implementations a test hopes agree. The CLI mirrors it:
normalizeZeromaxingEffort folds --reasoning-effort zeromaxing into the
equivalent --exec-profile selection before mode/profile expansion, leaving the
documented precedence ordering untouched. A conflicting --exec-profile is a
usage error rather than a silent winner.
THE LOAD-BEARING GUARD: forwardedReasoningEffort refuses to forward the posture
name for any model, including catalog-unknown ones where unrecognized values
otherwise pass straight through. Normalization already prevents it reaching
there; the guard is what makes a regression upstream fail loudly instead of
sending a provider a parameter value no provider defines.
"max" stays RESERVED and unchanged through all of this: the flag parser still
rejects it, the normalizer passes it through untouched, and /effort max still
reports it unsupported exactly as before.
Driving the real binary is what found the one real gap here: the flag parser
rejected "zeromaxing" outright, so the entire --reasoning-effort entry point was
dead at the user surface while every unit test passed, because they called the
normalizer directly. TestReasoningEffortFlagAcceptsZeromaxingButNotMax is that
regression, and it fails in both directions.
Degrade honestly — this closes a real asymmetry. exec already told the user via
reasoningEffortNotice when a model could not take the requested effort; the TUI
skipped the fill in SILENCE. Same rule, one of two call paths. Both status
surfaces now render ONE shared resolved-state line
("effort: high · profile: zeromaxing · turns: 320") plus the real delta, so a
user sees what actually reached the provider rather than just a posture name.
reconcileProfileAfterModelSwitch is the sibling re-derive site and records the
same reason, so a switch that drops the effort cannot leave the status implying
a raise the run is not making.
revertExecProfile gains a FOURTH knob. It is knob-by-knob, which is exactly
where a new one gets forgotten, so the posture's restore is asserted alongside
budget, effort and self-correct — and gated on zeromaxing specifically, so
/effort auto under fast/thorough keeps its existing meaning of "clear the
effort" instead of dropping the whole profile.
The footer carries a ZEROMAXING chip beside the effort chip while the posture is
on, and drops it while Exiting — by then the posture is already off and only its
announcement is pending.
…wn state
Two corrections to what zeromaxing tells the user, both cases of documentation
describing behaviour that is not happening.
1. --spec-reasoning-effort zeromaxing stays rejected — it is a run posture and
has no meaning for a spec draft — but the generic "Expected low, medium, or
high" read like a bug to anyone who had just learned the name works on
--reasoning-effort. It now says why it does not apply and names both flags
that do what the user was reaching for. A genuinely unknown value keeps the
plain message: the explanation is specific to the posture name, not a new
blanket wording.
2. Delta's self-correct clause described the change from THOROUGH, not from the
caller. Telling a user sitting on the LSP-only default that self-correction
is "already armed" while silently moving them to the full project test plan
is exactly the failure this posture's honesty rules exist to prevent. Delta
is now a function of the caller's actual state and renders one of three
transitions, in /selfcorrect's own vocabulary:
self-correct: lsp -> tests (the common case)
self-correct: unchanged (tests) (genuinely unchanged)
self-correct: lsp (your /selfcorrect choice ...) (user override wins)
The third exists because the TUI lets a user turn it back off after selecting
the posture; reporting a raise there would be a third way to describe
behaviour that is not happening. The budget and effort clauses stay fixed —
effort genuinely is caller-independent, since "high" is the ceiling every
provider accepts.
The headless path captures self-correct BEFORE applyExecProfile, which arms it
as a side effect; reading it afterwards would make every run report "unchanged".
The TUI reads its LIVE state rather than selection-time state, so a later
/selfcorrect off stops the status claiming a raise the session no longer has.
A unit test on the transition helper cannot catch the capture being read at the
wrong moment, because it never runs the code that captures — so the ordering is
pinned by a test that drives the real exec path and reads stderr. That gap was
found by mutation, not by review.
…t vouch for Three defects found by running the real binary, all in what the posture reports or applies. 1. THE FILL SILENTLY DID NOT HAPPEN on a custom endpoint. /effort zeromaxing left the effort at "auto" while --exec-profile zeromaxing on the SAME model sent reasoning_effort:"high" on the wire. Same posture, same model, two answers — the fifteenth instance of one rule applied to one of two call paths. The fill site used reasoningEffortAllowed, which returns false both for a model KNOWN to lack the level and for a model the catalog has never heard of. The headless path already distinguishes them (an unknown model forwards the requested effort "since no support claim can be made for it"), and so does the TUI's own model-SWITCH site, which takes a ringKnown flag for exactly this reason. Only the profile-fill site conflated them. profileEffortApplies now fills when the model lists the level OR when no catalog entry exists, and declines only when an authoritative empty ring says the model has no reasoning controls. A model name that is empty is a third case and still declines — there is nothing to make a support claim about. That last distinction was caught by the pre-existing fast-posture test, which the first version of this fix broke. THE HOLE THAT HID IT: every test asserted a surface against its OWN expectation — the TUI tests against the TUI's rule, the CLI tests against the CLI's. Both suites were green while the two paths disagreed, because nothing compared them against each other for the same model. A unit test on either helper could not have caught it. TestProfileEffortFillAgreesWithTheHeadlessPath is that missing comparison, run across a catalog reasoning model, an inferred one, a catalog model with an authoritative empty ring, and an unknown endpoint. 2. THE OUTPUT CONTRADICTED ITSELF: "reasoning effort: unchanged — already at the highest level" and "NOT raised to high: the active model does not support that level", in the same card. Both cannot be true. The fixed clause lived in Delta while the refusal lived in a separate line, so they were only ever consistent by coincidence. Delta now renders every clause from one DeltaState, with the effort clause a single switch over EffortTransition. A contradiction is unrepresentable rather than merely unlikely, and the separate line is gone. 3. THE BUDGET CLAUSE compared against thorough — "(thorough uses 160)" — which is information about two profiles, not about the user. It now states the caller's own transition, "turn budget: 80 → 320", with distinct renderings for an already-at-320 session and an unknown current budget.
…ing controls
PRE-EXISTING defect, surfaced while investigating the zeromaxing posture and
verified on origin/main. It is not something the posture introduced.
availableReasoningEfforts() returns an EMPTY ring for two very different
situations: a catalog model that genuinely has no reasoning controls (gpt-4.1),
and a model with no catalog entry at all (the reporter's glm-5.2 on
ollama-cloud, any custom openai-compatible endpoint). Every TUI consumer treated
the second as if it were the first.
That single fact produced two visible failures at once:
- /effort listed no levels and reported "no reasoning controls on this model"
- /effort high was REFUSED outright
- and it is why a profile's effort fill was declined with "the model does not
support that level"
Meanwhile the headless path has always disagreed. On origin/main, for the same
glm-5.2: forwardedReasoningEffort returns "high" and emits no notice, because an
unknown model "forwards the requested value as-is, since no support claim can be
made for it". So the CLI forwarded the level while the TUI refused to let the
user set it — one rule, two answers, predating this feature.
There are THREE consumers of "does this model take this level?": a manual
/effort, a profile's fill, and the headless forwarding decision. They now share
one rule — a catalog entry is authoritative and refuses; no entry means Zero
cannot vouch either way and does not block. TestEffortSettabilityAgreesAcross-
AllThreeConsumers pins that they agree, per model, including the reporter's.
/effort also distinguishes the two states in its output, because "this model has
no reasoning controls" and "Zero has no entry for this model" are different
facts and rendered identically before. Setting a level on an unlisted model now
says plainly that support is unconfirmed and an unsupported value is ignored by
the provider.
The regression tests drive the real /effort path rather than the helper. A green
helper test alongside an empty user surface is how this feature has now produced
four defects: the flag-parser hole, the self-correct capture point, the identity
test passing through the wrong gate, and this.
ZeroMaxing Phase 2: an `orchestrate` tool that accepts a plan as typed tool
arguments, validates it, records it as session events, and runs it SEQUENTIALLY
through the same specialist path a Task call uses. The deliverable is DATA — is
fan-out worth building at all — not concurrency.
ADDITIVE. With the posture off, the advertised tool set, the tool-definition
bytes and the assembled prompt are byte-identical to a build without this
feature, proved by TestPostureOffPrefixUnchangedByRegisteringTheTool: two
registries differing only in whether the tool is registered, asserted equal
under BOTH auto and unsafe. The enforcing mechanism is Safety() returning
PermissionDeny, not Deferred() — deferral only hides anything when it is ACTIVE,
and an unsafe session with deferral inactive would otherwise have advertised the
tool. Deferred() stays as a second layer and is now asserted rather than assumed.
VALIDATION CANNOT BE SKIPPED. Plan's fields are unexported and ParsePlan is the
only constructor, so there is no path from tool arguments to an executable plan
that bypasses it — the prototype's validator was reachable, correct, and never
called. Rejected by default: ids by ALLOW-LIST charset, unique; every depends_on
resolves (unknown edges rejected, never skipped); the graph is acyclic by Kahn's
with the involved ids NAMED (audit U24 — nothing in this tree checked, and a
cyclic graph hangs forever); task count capped; tools read-only and within the
parent's grant; a budget with max_tokens required and max_workers exactly 1,
REJECTED rather than coerced so the field stays meaningful for Phase 3; and
depth checked at admission with the remaining headroom named, rather than an
opaque failure mid-plan.
ONE counting function — a length, not a text scan. The prototype counted source
text, so `agent ("x")` with one space counted as zero and executed anyway.
BUDGET ENFORCED AT DISPATCH, not merely validated. Under this posture every
child inherits a 320-turn ceiling, so a twenty-task plan authorises 6,400 child
turns from one tool call. The grant is intersected again at dispatch too, so a
validator bug cannot widen a task's authority.
FAILURE IS NOT SUCCESS. A failed task skips its transitive dependents, each
RECORDED with the dependency that blocked it; independent siblings still run;
the plan runs to exhaustion. Partial is its own terminal status and the tool
returns StatusError for it, so nineteen of twenty failing can never surface as a
clean result (audit RC-F).
THE METRIC is value, not safety: max_speedup = sequential_total / critical_path,
computed from recorded per-task durations over the declared DAG and surfaced in
the summary and in Meta. Kill criterion: median >= 2.0x across >= 20 real plans,
or Phase 3 is not built. Independence-violation would answer safety, but
read-only tasks are always safe to parallelise, so it would read 0 and decide
nothing.
PermissionAllow when the posture is on, deliberately, with the reasoning in the
code so Phase 3 does not inherit it blind: the approval surface renders only the
tool name and a static sentence — PermissionRequest carries Args but no renderer
reads it — so a prompt could not show the plan it was gating and would train
click-through. What bounds it instead is enforced, not advisory: read-only at
validation and dispatch, a required budget enforced at dispatch, and the posture
itself as explicit consent. When tasks can WRITE, an approval gate becomes
necessary and needs a real renderer first.
Plan state is recorded as five ordinary session events beside the specialist
ones; no new store. Recording is best-effort and never fails the run.
…it 4 Wires Phase 2 into the real binary and maps a plan that did not fully complete onto the existing incomplete exit path. POSTURE CALLBACK — a POINTER, not a closure. The TUI model is a VALUE type (every handler takes and returns `m model`), so a func() bool closing over it would capture a copy frozen at registration and report the posture as it was when the session started, forever. Re-registration fails for the reason decision 2 rejected it earlier and worse: the TUI clones the registry per run and the clone copies tool POINTERS, so a replacement registered into the session registry would never reach a run already holding a clone. specialist.PostureGate is one shared atomic flag the tool holds for the process's life; every clone sees the same one and a posture flip needs no re-registration. PLAN RUNNER — captures only run-INVARIANT state (executor, workspace, parent identity and policy) and NO context. The ctx it uses is the one ExecutePlan hands it per task, so a cancelled run cancels the task in flight; it also checks ctx.Err() before launching, so a cancelled plan does not spend another task's budget. Capturing a context at construction is precisely how the prototype's background goroutine kept running after cancellation. The runner is synchronous and holds no goroutine, so it cannot outlive its plan. PLAN RECORDER — preserves execSessionRecorder's best-effort contract exactly. Every bridge method returns nothing, so a recording failure has no path back into ExecutePlan and cannot abort a plan mid-flight; nil is a no-op at every level. The bridge is built before the session exists and its inner recorder attached afterwards, so events before that point are simply not recorded rather than fatal. PARTIAL -> EXIT 4, folded into the EXISTING result.Incomplete path rather than a second exit route, so a partial plan and a stalled loop report the same way. It never overrides an incompleteness the loop already found. A DEFECT FOUND BY DRIVING THE BINARY, not by any test: the budget was enforced at dispatch against a counter that never moved. NewPlanRunner never populated TaskResult.Tokens, so a plan with max_tokens:1 ran all three tasks. The unit test passed because its fake runner fabricated its own token counts — the same "test the helper, miss the wiring" pattern that produced the flag-parser hole, the self-correct capture point, and the identity test's wrong gate. ExecResult gains an additive TotalTokens, populated from the stream summary the accounting path already computes. Verified at the real surface: orchestrate absent with the posture off and present as [shell/allow] with it on in the same auto-mode session; a 4-task diamond running in topological order with results verbatim and max_speedup 1.33x (hand-checked: four equal tasks over three hops = 4/3); cyclic, write-tool, max_workers and missing-budget plans each rejected with the offending ids or values named; and budget exhaustion reporting partial with `echo $?` showing 4.
Closes the two gaps left by the registration step.
TUI GATE. handleProfileCommand and revertExecProfile write the shared
PostureGate on every transition, threaded through tui.Options in the same shape
as ZeromaxingDisabled. What is proved without a TTY:
- the gate is written on posture ON and OFF through the REAL handlers, for
BOTH entry points (/effort and /profile) — not through a helper, which is
what missed the wiring four times in this feature
- a REFUSED selection (disabled workspace) leaves the gate off
- a nil gate is safe, so a caller that never wires one simply has no tool
- THE cloneToolRegistry HAZARD, proved rather than assumed: a tool reached
from a CLONE taken BEFORE a flip still observes the flip. That is the exact
ordering a captured copy or a closure over this value-typed model would get
wrong, and it is why the gate is a pointer
- concurrent Set/read, for -race: the TUI writes from its update loop while a
run's tool dispatch reads from the agent goroutine
FAILING-TASK VERIFICATION. The stub gained a prompt sentinel that makes one
specific child fail with a real provider-stream error, so a dependency failure
runs end to end rather than being asserted only at the executor. A 4-task
diamond where b fails now reports, verbatim:
Plan partial: 2 succeeded, 1 failed, 1 skipped max_speedup: 1.49x
- a [succeeded] full result verbatim
- b [failed] provider error: sentinel failure
- c [succeeded] independent sibling unaffected
- d [dependency_failed] skipped: dependency "b" did not succeed
Run incomplete (not reported as success) EXIT=4
Every level of the RC-F seam is now covered by something that can fail:
executor status, tool status, recorder, and the process exit code.
THE BUDGET DEFECT GETS ITS OWN GUARD. TestRealRunnerFeedsTheBudgetMeter drives
NewPlanRunner over an Executor whose child reports usage, and asserts a
max_tokens=1 plan launches exactly one child. A fake runner that fabricates its
own token counts cannot catch a producer that never populates them — which is
precisely how the defect survived every executor test. A second test pins that
the runner honours the per-call context rather than a captured one.
registerSpecialistTools regains its original name and takes the wiring
argument directly; the transitional wrapper is gone now that both call sites
pass wiring, so no dead indirection ships.
…n empty
The rule "a plan task may narrow the parent's grant, never widen it" was
declared, documented and validated against -- and both production call sites
left ParentTools nil, so neither the validation check nor the dispatch-time
intersection ever ran. A run restricted to grep handed a plan task read_file.
The empty case was worse. planToolGrant returned an empty slice when a task
declared no tools and the parent grant was empty; resolvedToolAllowlist read an
empty tool list as UNSPECIFIED and expanded it to the default read-only
category. The narrower the parent, the wider the child -- on the default path,
where a task declares no tools at all.
Three changes:
- The grant is now DERIVED inside registerSpecialistTools from specialist's
own exported read-only list, narrowed by the registry and the run's
operator filters. The filters are explicit parameters rather than wiring
fields, so a call site cannot omit them without failing to compile.
- Both intersections are unconditional. The "skip the check when the parent
list is empty" escape hatch is what made the rule inert; an unsupplied
grant is now a wiring bug that fails closed.
- Manifest gains ToolsResolved, so a resolved tool list can say
"deliberately nothing". A bare []string cannot: absence and emptiness are
both len()==0, and omitempty erases an empty slice entirely, so a nil check
would not survive a round trip. Same shape as audit finding M24. The flag's
meaningful value is true, so omitempty only drops the meaningless false.
planToolGrant now refuses an empty result rather than returning one, and
ExecutePlan records that as a task failure with a reason without dispatching
it, so an ungrantable plan reports partial/failed and exits 4.
reconcileProfileAfterModelSwitch called reasoningEffortAllowed directly while
the fill site called profileEffortApplies, added in 009f12b precisely to stop
treating an uncatalogued model's empty ring as a refusal. The two disagreed on
exactly that case, which is both of this user's configured models (glm-5.2 and
grok-4.5 are catalogued=false with an empty ring): selecting the posture on one
of them filled high, and switching to one with the posture already active
dropped the effort to auto and reported it as a level the model would not take.
e782470 was meant to fix this class at the cause across all consumers. It
updated one door and not the other -- the seventeenth instance of the pattern,
introduced by the fix for the sixteenth.
The rule is now ONE function, profileEffortAppliesOn, that both doors call, and
the ring's authority is passed through rather than inferred from its emptiness.
reconcileProfileAfterModelSwitch takes ringKnown, which its caller already had
and did not forward.
The comment at command_center.go described the old conservatism as deliberate
("only ever applies where support is known"). That is now the bug's
description, so it is replaced rather than left for the next reader to restore.
TestProfileEffortDoorsAgree asserts the doors against EACH OTHER across
catalogued / catalogued-with-no-ring / uncatalogued / no-model. Both doors had
tests before and both passed; nothing compared them, which is why the
disagreement shipped.
Three existing tests used kimi-k2.7-code:cloud as their "unsupported"
destination -- an UNCATALOGUED model, so they were asserting the defect. They
now use gpt-4o, which the catalog vouches has no reasoning controls, so the
genuine drop case stays covered.
task_completed carried session_id and tokens; task_failed carried neither. So the one task a user needs to open was the one task the event log could not point them at -- and the per-task token records did not add up to the plan's own total. Executor.Run already goes out of its way to preserve the child session id on a post-start failure, with a comment saying it exists so a failed child stays drillable. The recorder discarded it one layer up. A dependency or budget skip records an empty session id rather than omitting the field: that task never started a child, and present-and-empty says so, where an absent field would look like the same drop being fixed here. The regression test compares the two events against EACH OTHER. Both already had tests and both passed; nothing compared them, which is how the asymmetry survived.
PlanContext.Depth was unset at both call sites, so Limits.CurrentDepth and the executor's own nesting check both measured a constant zero. The admission headroom check could never fire. It was only harmless because plan tasks run at --auto low, where specialist tools do not register, so nothing could re-enter -- an inert guard that reads like a live one, which is the shape someone later trusts. Wired rather than deleted: the value exists (--depth), it costs two lines, and the check it feeds is the one thing standing between a nested run and maxSpecialistDepth if a future phase ever lets a plan task spawn. The TUI passes an explicit 0 with a comment: it is always a root session, so zero is the measured value there, not an unset one. Proven at the process boundary rather than by a unit test, since the defect was the call site and not the helper: before, `exec --depth 7 --exec-profile zeromaxing` ran a four-task plan; after, it refuses with "a plan's tasks would run at depth 8, which reaches the maximum nesting depth of 8", while --depth 6 still runs.
…ch other RULES.md §3: for every rule you touch, find its sibling and prove by test that both agree -- one test comparing the paths on the same input, not two tests each asserting their own expectation. The authority rule has two enforcement points and Workstream A changed both; two sites fixed together is exactly the shape that drifts apart later. The relationship asserted is SUBSET, not equality, and the direction is the point. Admission is stricter: it rejects a task naming anything outside the grant. Dispatch narrows: it drops such a tool and grants the rest. Requiring them to refuse identically would assert a symmetry the design does not want -- dispatch only runs on an already-admitted task, so when it disagrees its job is to hand over less, never more. Mutation-checked: dropping the parent check from the dispatch intersection is caught with "the second layer widened authority", which is the defect the "BELT AND BRACES" comment promises cannot happen.
A plan ran silently: sequential, synchronous, no output until it finished. Four things stood between the existing live-progress machinery and the plan tool, all of them wiring rather than missing capability. loop.go gated the progress callback on `call.Name == "Task"`, so the second sub-agent-spawning tool got nil. Keyed now on the TOOL'S OWN DECLARATION (tools.ChildProgressStreamer) rather than its name: `|| call.Name == "orchestrate"` would have been the same defect one name later, which is this codebase's most repeated shape. Every tool that does not declare keeps the nil callback it has today -- swarm tools included, deliberately. NewPlanRunner built TaskRunOptions without Progress and RunWithOptions never read options.Progress. Either half alone leaves a plan's children streaming to nobody, so they are ONE change with one end-to-end test. This is finding 7's class again: a second construction path that does not carry what the first one did. PlanRunner now takes a PlanTaskRequest struct rather than a widening parameter list, because each new positional argument is another chance for that omission. app.go never set Recorder, so the TUI recorded NONE of the five plan lifecycle events (finding 9). The recorder is now a required POSITIONAL parameter of registerSpecialistTools, like the operator filters: forgetting it is a compile error rather than a silent nil. TaskCancelled is a distinct outcome. Cancellation marked every remaining task failed with "context canceled", so a deliberate Ctrl-C on a twenty-task plan read as nineteen defects. PlanCancelled joins it as a terminal status, and terminalStatus counts it -- without that a plan with two successes and two cancellations reported "completed", which is RC-F exactly. Also fixes a live render-cache collision this change would have made certain. A rowSpecialist row carries no id and no text; every distinguishing field lives behind row.specialistInfo, and none of it was in the cache key. Two specialist cards in one run hashed identically, so a failed sub-agent rendered as a successful one. Rare with Task (one child per run is typical), guaranteed with a plan. Proven by test before fixing, then asserted with four cards including a failure and a cancellation. Additivity re-proven: posture off, all five permission modes byte-identical against an origin/main binary with DeferThreshold unset, hashes unchanged.
Gate 1a made a plan's tasks visible as cards. Cards cannot show the one thing that makes a plan a plan: its shape. This adds the panel. The shape is carried on planAdmittedMsg, not assembled as tasks finish. ParsePlan already proved the graph acyclic and computed the topological order, so the panel is complete before the first task starts rather than growing a node at a time. Each task is ranked by dependency depth -- one more than its deepest dependency -- and indented by it, so a diamond reads as one node, two beside each other, one node. A chain steps in once per link; independent tasks share a rung. All three are asserted. Mounted above the pinned update_plan panel, since it describes work running right now. It renders NOTHING when no plan has been admitted -- not an empty box, not a blank line -- which is what keeps a posture-off session unchanged: the panel never runs, rather than running and producing nothing. Named orchestrate throughout. /plan (update_plan's TODO list) and /plans (this) are genuinely different things with near-identical names, so every type, field and function here says orchestrate and only the user-facing command says plans. The command lists every task including any the pinned panel had to drop, and names each task's dependencies explicitly, so the shape survives copied text and narrow terminals where indentation does not. Bounded at 12 rows and it states what it hid: a silently truncated list reads as a complete one. Summaries cut on rune boundaries. The full result stays in the tool output -- the panel summarises, and a display formatter on a data path is how a 583-rune work product became 200 mangled runes. The header clock stops when the plan ends, and when the run ends without a terminal plan event. It previously kept counting while the turn that produced the plan carried on, so a 20-second plan read as minutes. TestSuggestionOverlayCapsRowsWithoutMoreText pinned the first and last entries of the command palette's first window; inserting /plans pushed /ps out of it. The window's start and size are unchanged, so the tail anchor moves to /permissions and the comment says why it is incidental.
The struct comment always claimed a plan task "inherits exactly the parent's policy". It did not: both production call sites left PlanTaskContext.ParentModel empty, so appendModelArgs received no parent model and passed no --model, and the child fell back to whatever its own config resolved. After a /model switch or a --model flag, plan tasks ran on a different model than the run that launched them. Decided: the documentation states the safer behaviour, so the code moves to match the doc. Explicitly NOT a router decision -- choosing a cheaper model per task is a real future feature and would arrive as an explicit plan field on top of correct inheritance. Silently failing to propagate is not that feature. The fix is not "populate the field at registration", which would have been wrong for the TUI: its registry is built once per session while /model changes the model between runs, so a captured value is stale by design. That is why the fields were empty rather than merely unset -- they were in the wrong home. ParentSessionID/ParentModel/ParentReasoningEffort now travel per call on PlanTaskRequest, read from the same tools.RunOptions fields the Task tool reads, and are gone from PlanTaskContext so there is no second source to drift from. Before: parent ran --model parent-chose-this, child reported stub-model. After: child reports parent-chose-this. Asserted per call, so a second orchestrate call with a different model launches on that model. The parent session id travels with it, which is what links a plan task back to the run that spawned it rather than leaving it an orphan.
Launching a specialist and resuming one are two doors onto the same question -- which model does this specialist run on -- and they answered it differently. BuildResumeArgsInput carried no model fields at all and BuildResumeArgs never called appendModelArgs, so a resumed child was launched with no --model and fell back to whatever its own config resolved. The same specialist could change model between its first turn and its second. Verified by building both argument sets side by side rather than by reading: FRESH args: exec --init-session-id … --model parent-chose-this … RESUME args: exec --resume … (no --model) The fix calls the SHARED helper rather than repeating its rules, so the manifest-pins-its-own-model precedence is applied once and cannot drift between the two paths. The relationship the regression test asserts is EQUALITY (RULES.md §3): fresh and resumed launches of the same specialist must produce the same model and the same reasoning effort. A second test drives Executor.Run with Resume set, since the builder having the fields proves nothing if the call site never fills them -- which is exactly how the two paths diverged. PRE-EXISTING main defect, not ZeroMaxing: it affects every resumed specialist. Separate commit so it can carry its own issue. Note for Stage 2c: runResume also resolves the manifest by session.AgentName and ignores params.Manifest, which is why inline-manifest children are unresumable. Same function, still open, deliberately not fixed here.
planReadOnlyTools bounds what a plan task may hold; readOnlySpecialistTools decides whether a specialist counts as read-only for the Task tool's permission gate. Different questions, same answer -- and they disagreed: lsp_navigate was in the first and not the second, so a manifest holding exactly the plan grant failed the read-only check. The omission was chronology rather than a decision. readOnlySpecialistTools was written in Gitlawb#243 (2026-06-18); lsp_navigate arrived in Gitlawb#276 two days later and nobody went back. The tool declares EffectReadOnly with the safety reason "reads files, modifies nothing" and resolves its path through the same scoped confinement its read-only siblings use, so it belongs in the set. Pinned as a SUBSET, not equality, and the direction is the point: anything a plan task may hold must be something the wider gate also calls read-only. The reverse is not required -- update_plan is read-only for a specialist and deliberately not a plan tool. NOT derived from the tools package's declared capabilities, which would be the real class fix: update_plan is EffectInteractive, so deriving the set would silently drop it and change the Task tool's permission gate. That is its own change with its own reasoning, not a drive-by.
A read-only run carried ~5 KB of confirmation policy governing actions it could not take. Measured on a real plan-task child: 8,365 bytes of the 17,903-byte system prompt -- 47% of it -- were policy for capabilities the child provably lacked (no write, no edit, no shell, no ask_user), and the system prompt is 71% of that request. Multiplied by every task in a plan and every specialist sub-agent. Keyed on the RESOLVED TOOL SET, never a name or a manifest flag, and fail closed at every step: a nil registry, a run holding nothing, and any tool whose effect is not positively read-only all keep the policy. EffectUnknown -- MCP tools, plugin tools, the specialist tools -- keeps it, which is why an ordinary session is untouched. Two things this got wrong first, both caught rather than reasoned away: It used ToolVisible, which applies the permission mode's ADVERTISING rules on top of the operator filters. A write tool auto mode does not advertise is still held and still callable once approved, so a run launched with --enabled-tools read_file,grep,glob,write_file counted as read-only and lost its policy -- a fail-OPEN exactly inverse to the purpose. Found by driving the binary; every unit test passed at the time. It now asks ToolAllowedByFilters, which answers "does this run hold the tool". Reading the registered tool set made registering a posture-gated tool change the prompt with the posture OFF, breaking the additive-only guarantee. TestPostureOffPrefixUnchangedByRegisteringTheTool caught it, which is what that test exists for. A tool the run can never call now says nothing about what the run can do -- and a tool that varies its permission by argument is never ruled out from its static safety, however that reads. Verified on the wire: --enabled-tools read_file,grep,glob drops it (12,310-byte prompt); adding write_file or bash keeps it (18,789). Additivity re-proven byte-identical across all five permission modes. Only the confirmation policy is gated. The remaining ~3.3 KB (Editing discipline, Testing gate, Permission and safety) lives inside system_prompt.md and needs that file split, which touches the prompt every run shares. Separate change.
Two changes from driving a real six-task chain. THE TOKEN BUDGET IS NOW OPTIONAL, AND OMITTED MEANS UNBOUNDED. max_tokens was required and capped at 200_000, and a chain asking for exactly that spent 469,555 -- 2.3x over -- and still lost its last task. The check runs only BETWEEN tasks: a task is dispatched whenever any budget remains and then spends whatever it spends, so the cap bounded nothing while stopping real work from finishing. A number that neither bounds spend nor lets a heavy plan complete is worse than no number, because it reads like a guarantee. Spend is still METERED and reported everywhere it was -- PlanReport.TokensUsed, the plan_completed event, the panel footer -- so an unbounded plan is not an unmeasured one. A caller that wants a bound still sets max_tokens and gets the same dispatch-time behaviour as before, asserted by test in both directions. This removes the only ceiling on what one orchestrate call can spend. Under the zeromaxing posture a twenty-task plan authorises 6,400 child turns. That is the user's decision, taken after seeing the cap fail to do the job it existed for. THE PANEL COLLAPSES. A six-task chain took seven footer lines for the whole run, pushing the conversation up for detail that is one keypress away. The header alone is the state; the tasks are detail. Click the header line to open it -- the interaction that was actually asked for -- or Ctrl+G. NOT Ctrl+O, which already toggles the detailed transcript view: the first attempt bound it there and the existing handler silently won. Not Ctrl+P either, which belongs to the update_plan panel. The dependency indent is capped at five levels. A twenty-task chain adds a rung per link, which would indent forty columns and run off a narrow terminal; the depths still reflect the real graph, only the drawing stops. Past the cap every task sits at one depth, which is honest for a chain.
TWO SEPARATE THINGS FROM DRIVING A REAL PLAN.
Finished tasks now drop out of the panel after a 5s linger, so it tracks live
work instead of accumulating a transcript of it. Long enough to see a task land,
short enough that a six-task chain does not fill the footer with history.
Matches how the AGENTS sidebar already retires finished agents.
A faded task is hidden, not forgotten: the header still counts it, and /plans
still lists the whole plan. That matters, because the dependency shape fades
with the tasks -- a diamond stops looking like a diamond once its first task
goes. /plans is where the shape stays readable for the whole run, and it names
each task's dependencies explicitly rather than relying on indentation.
/effort NOW SAYS WHAT IT ACCEPTS.
On glm-5.2 the card reported "available: not listed - model is not in Zero's
catalog" and stopped. Nothing typeable, on the model this user actually runs.
And on a catalogued model it listed low/medium/high while omitting zeromaxing --
which is selected through this very command, and which the actions line already
offered.
"available" (what the CATALOG vouches for) and "you can set" (what the command
accepts) are different questions and were conflated into one line. They are now
two fields.
The distinction is enforced, not decorative:
- catalogued with a ring -> exactly that ring
- catalogued with NO ring -> nothing but the posture. gpt-4o genuinely has no
reasoning controls and the command refuses every
level there, so offering them would advertise
what the command rejects
- not catalogued -> low/medium/high, because Zero cannot vouch
either way and the headless path forwards them
- posture disabled -> the posture is dropped from BOTH the list and
the actions line, since suggesting a command the
run will refuse is worse than saying nothing
The card and the command are two doors onto "which efforts can I set", so the
parity test feeds every advertised value back through the real command rather
than comparing two lists -- a list comparison would pass while the command
rejected all of them.
The popup that /effort opens showed a single "auto" row on glm-5.2. Reported from use, with a screenshot; I had just told the user no such picker existed, which was wrong -- newEffortPicker has been there all along. It read availableReasoningEfforts(), the CATALOG's answer. A model with no catalog entry has an empty ring, so the picker offered nothing settable -- on the model this user runs. And it never offered the zeromaxing posture on ANY model, even though picking one routes straight into handleEffortCommand, which accepts it. The picker now shares settableEfforts() with the /effort card, so the three surfaces that answer "which efforts can I set" -- card, picker, command -- cannot disagree. Catalog authority is preserved: gpt-4o, which the catalog vouches has no reasoning controls, still offers only auto and the posture. It also preselects the active posture. Under zeromaxing m.reasoningEffort holds "high" -- the level the posture FILLED -- so preselecting from that alone would highlight the wrong row. The parity test now feeds every value from BOTH surfaces back through the real command, and asserts the two offer the same set. The picker is the surface that actually failed in use while the card's own test passed, which is the argument for testing them against each other rather than each against its own expectation. Two existing tests encoded the old behaviour. One asserted "[auto] as the only effort option on an unsupported model" -- the reported bug, written down as an expectation; an uncatalogued model is not an unsupported one, and its levels are forwarded. The other pressed enter expecting to land on auto, which only held while auto was the sole row; it now selects auto deliberately and asserts the preselection instead.
All five from one screenshot of a six-task chain, all in what the run REPORTED rather than in what it did. "error (exit code 0)" on a failed task, directly above a body reading "Subagent failed (exit 4)" -- the card contradicting its own detail. A plan task's failure carries no exit code, and rendering the zero value claimed one it did not have. Only shown when non-zero now. Dependency-skipped tasks rendered "error". specialistCancelled landed in specialistStatusString's default arm, so a plan with ONE real failure showed three errors: the failure plus every task skipped behind it. The default stays "error" so an unmapped status still fails closed. "0 tokens" in the specialists rollup while the plan reported 130,135. Nothing has ever populated specialistInfo.tokenCount -- not for plan tasks, not for Task sub-agents -- so the total was always zero. The segment is now omitted at zero, matching the per-card rule (M18): a number that looks measured and is not is worse than no number. A plan task's spend now reaches its card, so the rollup adds up to what the plan reports. Task sub-agents still bridge no usage; that gap is unchanged and now shows as an absent segment rather than a false zero. The sidebar said "no active plan" while the panel two lines below showed one mid-flight. Its PLAN section reads the update_plan TODO list and knew nothing about orchestrate plans. It now shows a one-line summary -- name, done/total, failures, the task in flight -- when an orchestrate plan is running. EXACTLY one line, because the FILES section below computes its click offsets from the lines above it. The sidebar test drives renderContextSidebar rather than the line helper: the first version called the helper directly, and the mutation that removes the section from the sidebar entirely still passed.
The footer read "budget 0/200000 tokens" for an entire run while the cards directly above it showed 84,953 spent. tokensUsed was assigned in exactly one place -- from plan_completed, which arrives when the whole plan ends -- so the one number a user watches that line for was zero for as long as it mattered. Spend now accumulates as each task finishes. The executor's own total still wins at the end, since it counts every task including any whose message the panel dropped as stale, but a plan that reports no total no longer erases what was counted.
Pressing the PLAN header opens a two-pane drill-in: every phase down the left, the selected phase's live agent state on the right — status, elapsed, dependencies, tool count, spend, its prompt, what it is doing right now, and where it ended up. Up/down moves, enter drills into the child session, esc closes. An OVERLAY, in the same slot the help and picker overlays use, so it composites over the chat rather than replacing it. Nothing about the transcript, footer or composer changes, and it renders nothing when closed or when no plan has been admitted — which is what keeps a posture-off session unchanged. No separate posture gate: a plan only exists under the posture, so without one there is no PLAN line to press. The panel's own expand/collapse stays on ctrl+g, for a glance without leaving the conversation. Opening lands on the task worth reading: the one running, else the first that failed, else the first task. The selection clamps rather than wraps. Enter is offered ONLY when the selected task has a child session to open. A running task is still keyed by a temporary id, and a hint for a key that does nothing trains the user to ignore hints. The phase list marks the task in flight. Running and pending both fell into the default glyph, so the list could not show what was actually happening. Prompts are SUMMARISED and say when they were cut; the full text stays in the tool output. A task that never started shows what it is waiting on rather than a row of zeroes. Eight mutations, all caught. Two needed the tests fixed first: the running-task assertion compared whole rows, which differ by the selection marker anyway, so removing the in-flight glyph passed; and nothing drove the click path at all.
The right-hand column showed one grey line — "trace-chain 3/6 · helpers" — for a plan the panel below it was rendering in full. It now carries the plan properly: a progress count in the header and one coloured line per task, in the same shape the update_plan steps use so the section reads consistently whichever plan is in it. green ✓ done · red ✗ failed · accent • in flight · faint ○ pending faint ⊘ skipped or cancelled — deliberately NOT red, since neither is a defect The header count turns red on a failure and green when everything is done, so the one thing worth noticing carries from across the screen. THE LINES LIVE INSIDE sidebarPlanLines, and that is not a detail. sidebarFileSelectables computes the FILES section's click offsets from len(sidebarPlanLines); the previous one-liner sat beside the placeholder and was safe only because it was exactly one line. Rendering a whole section there would have misdirected every file click by the number of lines added, silently. Inside the function the arithmetic stays correct by construction. Live tasks first, so a long plan shows what is happening rather than what already happened, bounded at six lines with the remainder stated — the column is 26 to 40 cells wide and shares its height with AGENTS, FILES and ACTIVITY. The panel and the detail view are where a whole plan is read. update_plan's own steps still win the section when it has any: the two are different things and the TODO list is the one the model is actively editing. The plan's NAME is not in the sidebar. At this width it would cost a task row, and it is on the panel and in the detail view.
…live detail
The overlay was wrong and the screenshot showed why: composited into the
transcript column, a twelve-task plan drew its phase list straight through the
user's own prompt. It is gone.
The right column already owns "what is happening" — agents, files, activity —
and had half a screen padded with blank lines down to the token floor. The plan
lives there now:
PLAN 1/8
█████████░░░░░░░░░░░░░░░░░ 1/8
✓ t-sandbox x
✗ t-agent y
• t-tools Read the Go package at…
○ t-redaction
+2 more
TASK t-tools
running
52 tool calls · 21,400 tok
blocks pair-a
↳ read_file internal/tools/registry.go
Read the Go package at internal/tools
A progress bar that keeps done, failed, skipped and running in their own
colours inside one span, sized to the column rather than to a fixed width, and
never rounding a segment away — one failure in thirty still gets a cell,
because a failure the bar hides is a failure it does not report.
TASK fills what was dead space, and yields first when the column is short: the
list above it is what the section is for. It shows the REVERSE dependency edge
too — what a task blocks — because a failure matters in proportion to what
waits on it and the forward edge alone does not say.
Click a task to select it, click PLAN to collapse the section, ctrl+g to cycle
the selection from the keyboard. The sidebar is not focusable, so a mouse-only
affordance would not be one.
THE OFFSET ARITHMETIC IS THE RISK HERE, not the drawing. The progress bar sits
between the header and the first task, so every clickable row below it moves
down one — in the plan's own hit table AND in the FILES section beneath. Both
are adjusted, and both are asserted against where the row actually RENDERS
rather than against a re-derivation of the sum: the first version of the FILES
test recomputed the arithmetic and would have passed with the bar unaccounted
for on both sides.
Nine mutations, all caught. Two needed the tests rewritten first — the FILES
one asserted nothing, and the bar one used numbers that never round to zero.
…s offered The posture raises a cost multiplier — 320 turns per run, inherited by every sub-agent — and it was a word in amber, in the same weight as everything beside it. "Is it on?" should be answerable from across the room. The footer chip is now a FILLED badge: the label sits on the amber rather than in it, which is what reads as lit rather than as another label in the row. Off still means ABSENT, not dim — the footer is byte-identical without the posture. It breathes while a turn is in flight, ◦ • ● ◉ ● •, on a 1.4s cycle. Slow on purpose: this marks a standing state, not activity, and a fast blink beside a working spinner reads as a second thing happening. THE PULSE IS FREE, and that constraint shaped it. It advances on the spinner tick that is ALREADY running during a turn and holds a steady full glyph otherwise. ensureSpinnerTick deliberately schedules no timer on an idle session — "an idle plain session schedules no timer" — and a chip is not a reason to break that. Steady rather than frozen mid-cycle, because a half-lit chip on an idle session would read as a rendering fault. Reduced motion pins it, like every other animation here. The /effort picker marks the posture row too, so what you are about to turn on looks like what you will see once it is on. Marked with a glyph rather than merely coloured: the selected row already owns its background, so colour alone would be invisible exactly when you are looking at it. The marker is display only — the VALUE handed to the command stays the bare name, or selecting it would be refused as an unknown effort. Seven mutations, all caught. Two needed the tests fixed first: the chip test called the helper rather than the footer, so reverting the footer to plain amber text passed it; and the value test located the row BY value, so the moment the marker leaked into the value the row stopped matching and the test passed by finding nothing.
…is clickable The chip is now clickable — pressing it opens /effort, where the posture can be turned off or changed — so it highlights under the cursor. A clickable chip that looks identical to a label never teaches anyone it can be pressed. Not while a turn is in flight: /effort refuses mid-run, and opening a dialog that cannot be acted on is worse than opening none. Its hit region is measured from the RENDERED footer rather than assumed. The chips before it — permission mode, effort — vary in width with the session, so a fixed offset would drift the moment either changed. The sidebar's plan rows highlight too, joining the agent, plan-step and file rows that already did. The hover is held by TASK ID, not row index, for the reason the others hold identities: the rendered row set changes as tasks finish and fade, with no mouse motion in between to re-resolve it, and a cached offset would light up whatever slid into the slot. The /effort picker already highlighted its rows on hover — selectGenericPicker- AtMouse live-previews on motion — so the zeromaxing row needed nothing. Six mutations, five caught. The sixth is belt-and-braces rather than a gap and the comment now says so: the posture-off guard in the chip's hit test is a fast path, not the enforcement — with the posture off the footer carries no chip, so the span lookup fails anyway.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
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) |
Collaborator
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) |
Collaborator
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() |
Collaborator
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 { |
Collaborator
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?
Comment on lines
+217
to
+219
| delete(s.tempReads, root) | ||
| s.mu.Unlock() | ||
| s.removeReadRoot(root) |
Collaborator
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." |
Collaborator
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 { |
Collaborator
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) { |
Collaborator
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 { |
Collaborator
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) { |
Collaborator
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?
…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>
Collaborator
|
@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 |
Collaborator
|
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. |
releaseTemporaryRead deleted the refcount, dropped the lock, then stripped
the root in a second acquisition. Between those two holds the root was still
in readRoots but no longer in tempReads, and AddTemporaryRead reads exactly
that pair to decide what a caller is holding: it finds the root covered,
finds no refcount, concludes the cover is PERMANENT, and returns a no-op
undo. The release then strips the root and that caller has silently lost
access it believes it holds.
Reproduced by driving the two mutations apart and stepping a second caller
through the gap:
after add: readRoots=[.../shared] tempReads=map[.../shared:1]
in window: second caller tracked=false (a no-op undo)
after release: validateRead -> "requires access outside the workspace"
Both mutations now happen under one hold, and releaseTemporaryWrite gets the
same treatment. removeReadRoot/removeWriteRoot existed only to re-acquire the
lock and are gone.
ON THE TESTS, because they are weaker than they look: neither reproduces the
window on demand. The concurrent one cannot hit a gap that narrow reliably —
it survived the unfixed code even with an injected scheduling point — and the
sequential one holds two references, so the refcount never reaches zero and
the delete never runs. They assert the invariant the bug broke (a live grant
stays readable, the root retires only with its last holder) and are worth
having, but the evidence for the bug is the reproduction above, not them.
Two earlier versions of these tests were worse than weak: they used
t.TempDir() with NewScope, which adds the system temp directory as a write
root, so AddTemporaryRead returned early and never touched the refcount at
all. The scope is built directly here for that reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Three guards that were missing beside siblings that have them. /effort zeromaxing delegated to handleProfileCommand with no idle check. That is the same mutation /profile refuses mid-run — turn budget, self-correct, and the shared orchestrate gate — and /profile refuses it because the budget propagates to sub-agents spawned later in the same run, so changing it mid-turn leaves one run straddling two budgets. Reaching the mutation through the effort namespace skipped the guard the profile namespace applies. Idle behaviour is unchanged and tested, so the guard cannot quietly disable the feature. orchestrateHeaderAtMouse had no modal guard while orchestrateTaskAtMouse directly above it does, and sidebar.go states the rule: sidebarAvailable deliberately does not suppress the sidebar for the `/` palette, because a palette must not reflow the layout, so each hit-tester carries its own suggestionsActive() guard. Without it, clicking the PLAN header while the palette is open toggled the section behind the overlay. The posture chip in mouse.go is checked BEFORE the surface switch, so the modal guards in those cases do not cover it. With the /model picker or a provider wizard open, clicking the chip swapped in a fresh effort picker and discarded whatever the open one had loaded or the user had typed. The header test first used a picker and passed against the unguarded code: sidebarAvailable already suppresses the sidebar for pickers, so it proved nothing. It uses the `/` palette now, which is the case the guard exists for, and that mutation fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
planInt returns 0 for both an absent key and a present-but-negative one, so `seconds > 0` read a model-supplied max_wall_seconds of -60 as "unset" and ran the plan unbounded with no error. Every other numeric in the budget refuses a negative — max_retries and max_tokens explicitly, and max_stall_seconds even refuses positive values under its floor — so this was the one invalid input silently accepted, and the failure mode is the worst kind: a bound the caller asked for that is not applied. Absent and explicit zero keep their meanings, which is tested, so the stricter parse cannot turn "unset" into an error. The orchestrate approval card said "read-only specialist sub-agents". PermissionForArgs prompts ONLY when argsCanWrite is true, so every card a user actually sees is asking about a plan that CAN write — the description was exactly inverted on the one screen where it matters. Description() said the same thing to the model. Both now say tasks inherit the parent's grant and may hold write tools granted by name. The posture-off identity test claimed Zero is byte-identical to a build without the feature when the posture is off. It is not: system_prompt.go drops the ~5 KB confirmation policy for any run that cannot mutate, and runCanMutate is evaluated regardless of posture, so an all-read-only run gets a smaller prompt either way. The drop is deliberate and fails closed — the claim was just wider than the proof, and the comparison could not catch it since both registries take the same branch. The comment now states the property actually proved: registering the tool changes nothing while the posture is off. Left alone deliberately, as design calls rather than defects: PlanProgress Done/Remaining have only test callers, and research.json ships with an unsubstituted "THE SUBJECT" placeholder plus dependents that assume upstream output they never receive. Both need a decision (wire in vs drop; add argument substitution vs unship the plan) that belongs to the author. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
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
Comment on lines
+34
to
+48
| // Absent and zero must keep their existing meanings — the fix must not turn | ||
| // "unset" into an error. | ||
| func TestBudgetStillAcceptsAbsentAndZeroTimeouts(t *testing.T) { | ||
| for name, budget := range map[string]map[string]any{ | ||
| "absent": {"max_workers": float64(1)}, | ||
| "zero": {"max_workers": float64(1), "max_wall_seconds": float64(0), "max_stall_seconds": float64(0)}, | ||
| } { | ||
| t.Run(name, func(t *testing.T) { | ||
| if _, err := ParsePlan(map[string]any{ | ||
| "name": "p", | ||
| "tasks": []any{map[string]any{"id": "a", "prompt": "x"}}, | ||
| "budget": budget, | ||
| }, Limits{MaxTasks: 20, ParentTools: []string{"read_file"}}); err != nil { | ||
| t.Fatalf("%s timeouts should parse: %v", name, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the preserved timeout values.
TestBudgetStillAcceptsAbsentAndZeroTimeouts checks only that ParsePlan returns no error. It does not verify that absent and explicit 0 values leave Budget.MaxWall and Budget.MaxStall unset. A regression that assigns a non-zero timeout while accepting the input would pass this test.
Store the returned plan and assert the expected zero values for both cases.
As per coding guidelines, **/*_test.go requires regression tests for behavior changes; this test must verify preserved timeout semantics as well as acceptance.
🤖 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_budget_negative_test.go` around lines 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.
Source: Coding guidelines
Comment on lines
+490
to
+510
| if seconds, set := planIntSet(raw, "max_wall_seconds"); set { | ||
| if seconds < 0 { | ||
| return Budget{}, fmt.Errorf("budget.max_wall_seconds must not be negative; %d was requested", seconds) | ||
| } | ||
| if seconds > 0 { | ||
| budget.MaxWall = time.Duration(seconds) * time.Second | ||
| } | ||
| } | ||
| if seconds, set := planIntSet(raw, "max_stall_seconds"); set { | ||
| if seconds < 0 { | ||
| return Budget{}, fmt.Errorf("budget.max_stall_seconds must not be negative; %d was requested", seconds) | ||
| } | ||
| stall := time.Duration(seconds) * time.Second | ||
| if seconds > 0 && stall < minStallTimeout { | ||
| return Budget{}, fmt.Errorf( | ||
| "budget.max_stall_seconds must be at least %d: below that the watchdog fires on ordinary think-time and becomes a random task-killer", | ||
| int(minStallTimeout.Seconds())) | ||
| } | ||
| if seconds > 0 { | ||
| budget.MaxStall = stall | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the repository-declared Go target without assuming a local toolchain.
sed -n '/^go /p' go.mod
# Inspect duration admission and existing boundary coverage.
sed -n '474,564p' internal/specialist/plan.go
rg -n -C 3 'max_wall_seconds|max_stall_seconds|MaxWall|MaxStall' \
internal/specialist --glob '*_test.go'Repository: Gitlawb/zero
Length of output: 10777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect numeric conversion and all consumers of the parsed duration fields.
rg -n -C 8 'func planIntSet|type Budget|MaxWall|MaxStall|minStallTimeout|time\.Duration\(.*seconds' internal/specialist --glob '*.go'
# Inspect the existing budget tests and repository contribution requirements.
sed -n '1,220p' internal/specialist/plan_budget_negative_test.go
sed -n '1,180p' CONTRIBUTING.mdRepository: Gitlawb/zero
Length of output: 26491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'func planIntSet|type Budget|MaxWall|MaxStall|minStallTimeout|time\.Duration\(.*seconds' internal/specialist --glob '*.go'
sed -n '1,220p' internal/specialist/plan_budget_negative_test.go
sed -n '1,180p' CONTRIBUTING.mdRepository: Gitlawb/zero
Length of output: 26491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Model the exact int64 duration conversion at the boundary and show the
# downstream admission behavior for both fields.
python3 - <<'PY'
MAX_NS = (1 << 63) - 1
SECOND_NS = 1_000_000_000
MAX_SECONDS = MAX_NS // SECOND_NS
def int64(n):
n &= (1 << 64) - 1
return n - (1 << 64) if n >= (1 << 63) else n
def duration_seconds(seconds):
return int64(seconds * SECOND_NS)
for field in ("max_wall_seconds", "max_stall_seconds"):
for seconds in (MAX_SECONDS, MAX_SECONDS + 1):
duration = duration_seconds(seconds)
stall_rejects = seconds > 0 and duration < 30 * SECOND_NS
wall_is_applied = seconds > 0 and duration > 0
print(field, seconds, "duration_ns=", duration,
"stall_rejects=", stall_rejects,
"wall_is_applied=", wall_is_applied)
PYRepository: Gitlawb/zero
Length of output: 570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
MAX_NS = (1 << 63) - 1
SECOND_NS = 1_000_000_000
MAX_SECONDS = MAX_NS // SECOND_NS
def int64(n):
n &= (1 << 64) - 1
return n - (1 << 64) if n >= (1 << 63) else n
for field in ("max_wall_seconds", "max_stall_seconds"):
for seconds in (MAX_SECONDS, MAX_SECONDS + 1):
duration = int64(seconds * SECOND_NS)
print(field, seconds, duration)
PYRepository: Gitlawb/zero
Length of output: 346
Reject overflowing max_wall_seconds values before converting to time.Duration.
max_wall_seconds = 9223372037 overflows to a negative duration. plan_exec.go then skips the wall deadline because MaxWall > 0 is false. max_stall_seconds already rejects this overflow through its minimum-timeout check. Add tests that accept 9223372036 and reject 9223372037.
🤖 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 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.
Source: Coding guidelines
Comment on lines
+58
to
+61
| // Sanity: the click lands on the header when nothing is in the way. | ||
| if !base.orchestrateHeaderAtMouse(click) { | ||
| t.Skip("the header is not where this test thinks it is") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fail when the calculated click does not hit the PLAN header.
Line 60 skips the test when the fixture or layout calculation is incorrect. The modal guard then has no regression coverage. Replace t.Skip with t.Fatal.
Proposed fix
if !base.orchestrateHeaderAtMouse(click) {
- t.Skip("the header is not where this test thinks it is")
+ t.Fatal("test click does not resolve to the PLAN header")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Sanity: the click lands on the header when nothing is in the way. | |
| if !base.orchestrateHeaderAtMouse(click) { | |
| t.Skip("the header is not where this test thinks it is") | |
| } | |
| // Sanity: the click lands on the header when nothing is in the way. | |
| if !base.orchestrateHeaderAtMouse(click) { | |
| t.Fatal("test click does not resolve to the PLAN header") | |
| } |
🤖 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_guards_test.go` around lines 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.
The wall deadline was consulted only before dispatching a task, and the walk
only reaches that check when it needs a free worker slot. A plan whose ready
set fits the worker pool dispatches in one wave and is never checked again,
so it ran for as long as its slowest child took and reported "completed"
with nothing skipped.
Measured before the fix, four 2s tasks under a 1s wall:
max_workers=4 -> 2.0s elapsed, status=completed, 4 succeeded, 0 skipped
max_workers=1 -> status=partial, 1 succeeded, 3 skipped
So the bound existed only on the sequential path, and asking for parallelism
silently deleted it. Nothing cancelled an in-flight child either — the plan
context was WithCancel, and the only deadline reads were the two dispatch
gates.
The plan context now carries the wall as a real deadline, which reaches the
children through the same machinery a user stop already proves works. The
walk keeps its skip-the-rest behaviour for tasks not yet dispatched.
This is the defect max_tokens' cap was removed for, in the commit that said a
number which neither bounds spend nor lets a plan finish "is worse than no
number, because it reads like a guarantee". max_wall_seconds was the worse
case: the token check at least fires between tasks, while the wall deadline
fired zero times in a fan-out — and the approval card shows "300s wall" while
never showing max_workers, the parameter that disabled it.
Both cancellation paths now say WHICH reason stopped a task. A wall expiry is
the plan spending what it was allowed; a cancel is a person deciding. Both
read "the run was stopped", which sent the reader looking for who stopped it.
The tests bound their own waits rather than blocking on ctx.Done(). An
unbounded wait there wedged a full run for ten minutes when the fix was
mutated out — a test that hangs on a regression is worse than one that fails.
ParsePlan permits a task to name a write tool, deliberately and recently: "A TASK MAY NOW NAME A WRITE TOOL, and only by naming it." That grant is what makes PermissionForArgs prompt and what makes RequiresIsolation prepare a worktree. Two layers downstream still assumed the older rule that every task is read-only, so the capability was gated, approved and isolated — and then could not be exercised. The child's system prompt was one literal telling every task "You have read-only tools" and "do not attempt to modify anything", including a task granted write_file or bash. A child told not to modify anything does not use the write tool it holds, so the approval the user gave and the worktree prepared for it bought nothing. The prompt is now built from the grant, using the same allow-list ParsePlan validates against so the two cannot drift. The read-only wording is unchanged for the read-only case, which is the majority and the one whose phrasing was tuned. savedPlanLimits granted only the read-only names, so a plan ParsePlan accepts at run time failed to parse on save, show, run, restart and resume alike. The durability surface silently excluded exactly the plans whose work is most worth keeping. It now uses PlanGrantableToolNames. This widens no authority: the parent grant is intersected again when the plan runs, a write-capable plan still needs its approval and its worktree, and a tool outside the grantable list is still refused — both directions tested. NOT changed, because it is a decision rather than a defect: plan children still run at --auto low, where plan_runner.go says "Explicitly NOT MemberAutonomy: Phase 2 tasks are read-only, and granting it here would be the authority widening that was ruled out as needing its own decision." That comment and ParsePlan's contradict each other, and only one of them can stand. Widening child autonomy is not a call to make inside a review fix.
Collaborator
|
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:
|
Collaborator
|
@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
Comment on lines
+196
to
+211
| // max_wall_seconds bounds the PLAN, which means it has to reach the children. | ||
| // The pre-dispatch check below is not a bound on its own: it is consulted | ||
| // only when the walk needs a free slot, so a plan whose ready set fits the | ||
| // worker pool dispatches in one wave and is never gated again. Measured at | ||
| // max_workers=4 with a 1s wall and four 2s tasks: 2.0s elapsed, reported | ||
| // "completed", nothing skipped — while the same plan at max_workers=1 | ||
| // correctly reported partial. Asking for parallelism deleted the bound. | ||
| // | ||
| // A deadline on the plan context fixes that with the machinery a user stop | ||
| // already proves works, and the walk keeps its skip-the-rest behaviour for | ||
| // tasks not yet dispatched. | ||
| if wall := plan.Budget().MaxWall; wall > 0 { | ||
| var cancelWall context.CancelFunc | ||
| ctx, cancelWall = context.WithTimeout(ctx, wall) | ||
| defer cancelWall() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether pause duration is excluded from MaxWall/deadline accounting.
set -euo pipefail
fd -e go . internal/specialist | xargs rg -n -C6 'waitWhilePaused|func.*[Pp]ause|WaitWhilePaused|extendDeadline|PlanControl'Repository: Gitlawb/zero
Length of output: 10539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(plan|.*gate|.*surface|.*recorder).*\.go$|internal/specialist/.*\.go$' | head -200
printf '%s\n' '--- pause/control symbols ---'
rg -n -C5 'WaitWhilePaused|PlanRunning|pause|Pause|MaxWall|max_wall_seconds|WithTimeout|WithDeadline' --glob '*.go' .
printf '%s\n' '--- relevant test names ---'
rg -n 'MaxWall|max_wall_seconds|paused|Paused|pause|Pause|deadline|cancelled' internal/specialist --glob '*_test.go' | head -240Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C8 'WaitWhilePaused|MaxWall|max_wall_seconds|WithTimeout|WithDeadline|PlanRunning' internal --glob '*.go'Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all repository pause/control references ---'
rg -n -C6 'WaitWhilePaused|PlanRunning|Paused|paused|pause|Pause' --glob '*.go' . | head -400
printf '%s\n' '--- all budget and deadline references ---'
rg -n -C6 'MaxWall|max_wall_seconds|WithTimeout|WithDeadline|Deadline\(' --glob '*.go' . | head -400Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 'WaitWhilePaused|PlanRunning|Paused|paused|pause|Pause|MaxWall|max_wall_seconds|WithTimeout|WithDeadline|Deadline\(' --glob '*.go' .Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- TUI plan controller references ---'
rg -n -C12 'PlanController|WaitWhilePaused|PlanRunning|planProgress|pause|Pause|resume|Resume' internal/tui --glob '*.go' | head -500
printf '%s\n' '--- specialist wall/pause tests ---'
rg -n -C10 'MaxWall|max_wall_seconds|WaitWhilePaused|paused|Paused|pause|Pause|DeadlineExceeded|partial' internal/specialist --glob '*_test.go' | head -500Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C12 'PlanController|WaitWhilePaused|PlanRunning|planProgress|pause|Pause|resume|Resume' internal/tui --glob '*.go' | head -500
rg -n -C10 'MaxWall|max_wall_seconds|WaitWhilePaused|paused|Paused|pause|Pause|DeadlineExceeded|partial' internal/specialist --glob '*_test.go' | head -500Repository: Gitlawb/zero
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact controller definitions and uses ---'
rg -n 'type (PlanProgressBridge|.*Plan.*Bridge)|func \(.*PlanProgressBridge|WaitWhilePaused|PlanRunning|BackgroundPlanLive|PausePlan|ResumePlan' internal --glob '*.go'
printf '%s\n' '--- exact specialist tests involving MaxWall ---'
rg -n 'MaxWall|max_wall_seconds' internal/specialist --glob '*_test.go'Repository: Gitlawb/zero
Length of output: 7670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'type (PlanProgressBridge|.*Plan.*Bridge)|func \(.*PlanProgressBridge|WaitWhilePaused|PlanRunning|BackgroundPlanLive|PausePlan|ResumePlan' internal --glob '*.go'
rg -n 'MaxWall|max_wall_seconds' internal/specialist --glob '*_test.go'Repository: Gitlawb/zero
Length of output: 7575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PlanProgressBridge pause implementation ---'
sed -n '145,285p' internal/tui/plan_progress.go
printf '%s\n' '--- pause control tests ---'
sed -n '1,180p' internal/tui/orchestrate_control_test.go
printf '%s\n' '--- wall budget tests ---'
sed -n '1,150p' internal/specialist/plan_wall_budget_test.goRepository: Gitlawb/zero
Length of output: 14603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '145,285p' internal/tui/plan_progress.go
sed -n '1,180p' internal/tui/orchestrate_control_test.go
sed -n '1,150p' internal/specialist/plan_wall_budget_test.goRepository: Gitlawb/zero
Length of output: 14501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
exec_src = Path("internal/specialist/plan_exec.go").read_text()
bridge_src = Path("internal/tui/plan_progress.go").read_text()
assert "ctx, cancelWall = context.WithTimeout(ctx, wall)" in exec_src
wait_start = bridge_src.index("func (bridge *PlanProgressBridge) WaitWhilePaused")
wait_end = bridge_src.index("// StopPlan", wait_start)
wait_src = bridge_src[wait_start:wait_end]
assert "<-ctx.Done()" in wait_src
assert "<-resume" in wait_src
assert "MaxWall" not in wait_src
control_start = bridge_src.index("func (bridge *PlanProgressBridge) SetPlanPaused")
control_end = bridge_src.index("// PlanPaused", control_start)
control_src = bridge_src[control_start:control_end]
assert "MaxWall" not in control_src
assert "WithTimeout" not in control_src
assert "cancelled: the plan's max_wall_seconds elapsed before this task ran" in exec_src
print("plan deadline is created once; pause waits on the same context; pause control does not extend MaxWall")
PYRepository: Gitlawb/zero
Length of output: 252
Suspend MaxWall while the plan is paused.
context.WithTimeout continues during WaitWhilePaused. If the pause exceeds the remaining MaxWall, resuming immediately cancels every undispatched task with "cancelled: the plan's max_wall_seconds elapsed before this task ran". Exclude paused time from MaxWall accounting and add a regression test.
🤖 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 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.
Comment on lines
+136
to
+172
| func TestZZProbeRealFailureRelabelledCancelled(t *testing.T) { | ||
| plan := mustPlan(t, []any{ | ||
| task("boom", "fails for real"), | ||
| task("later", "never runs"), | ||
| }, map[string]any{"max_workers": float64(2)}, readOnlyLimits()) | ||
| t.Logf("order = %v", plan.Order()) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| rec := &zzPauseRecorder{gate: make(chan struct{})} | ||
|
|
||
| failed := make(chan struct{}) | ||
| done := make(chan PlanReport, 1) | ||
| go func() { | ||
| done <- ExecutePlan(ctx, plan, []string{"read_file"}, | ||
| func(_ context.Context, req PlanTaskRequest) (TaskResult, error) { | ||
| if req.Task.ID == "boom" { | ||
| close(failed) | ||
| // A REAL failure: not a context error. | ||
| return TaskResult{Outcome: TaskFailed, Err: "compile error in main.go"}, nil | ||
| } | ||
| return TaskResult{Outcome: TaskSucceeded}, nil | ||
| }, rec) | ||
| }() | ||
|
|
||
| <-failed | ||
| time.Sleep(50 * time.Millisecond) // let the completion sit in the buffer | ||
| cancel() // user stops the paused plan | ||
| close(rec.gate) | ||
|
|
||
| report := <-done | ||
| for _, task := range report.Tasks { | ||
| t.Logf("task %-6s outcome=%-18s err=%q", task.ID, task.Outcome, task.Err) | ||
| } | ||
| t.Logf("status=%s succeeded=%d failed=%d cancelled=%d skipped=%d", | ||
| report.Status, report.Succeeded, report.Failed, report.Cancelled, report.Skipped) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an assertion for the relabeling this probe demonstrates.
TestZZProbeRealFailureRelabelledCancelled reproduces a real defect but never fails on it. The task "boom" returns Outcome: TaskFailed with a real error ("compile error in main.go", err is nil, Line 155) before cancel() runs. The recorder pauses the scheduler so this completion is only harvested after cancel() fires (Lines 161-164).
Per the harvest logic in internal/specialist/plan_exec.go (if err != nil || result.Outcome == TaskFailed { if ctx.Err() != nil || ... { ... result.Outcome = TaskCancelled ... } }), any completed task with Outcome == TaskFailed is relabeled TaskCancelled once ctx.Err() != nil, regardless of whether the failure had anything to do with cancellation. This test proves that reproduces, but Lines 166-169 only t.Logf the result. The test passes whether or not this relabeling happens.
A test that documents a defect but cannot fail on it gives no regression protection. Either:
- Fix the harvest logic in
plan_exec.goso a task that already reportedTaskFailedbefore the plan's context was cancelled keeps that outcome, and assert it here (report.Tasks["boom"].Outcome == TaskFailedand.Errstill contains"compile error in main.go"), or - If retroactive relabeling is intentional, assert that explicitly and explain why in the test, rather than leaving it as an unverified log line.
"add a regression test for behavior changes" for **/*_test.go files.
🤖 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 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.
Source: Coding guidelines
1 task
…ld can carry
Two additive pieces the plan-model work needs, kept apart from it so each
stands alone.
PROFILES.PLANMODELS lets a user pin a model per task role, exclude ones that
should never be chosen, name the model that does the routing, and add their own
guidance in plain words. The guidance exists because the code cannot know the
account: discovery orders models by price, and on real providers price is not
capability — one account's dearest model was a build preview that failed every
task it touched, another reports no prices at all so the ranking collapses to
alphabetical. The operator knows which model reasons well; this is where they
say so once, instead of naming models in every prompt.
PROJECT CONFIG MAY ONLY EXCLUDE. An exclusion removes a candidate and can only
lower spend; a pin is the opposite, and a cloned repo pinning every role to the
priciest model would raise cost for whoever opened it — the same hazard the
PlanSize and DisableZeromaxing tighten-only rules exist for. Guidance is the
softest way to do what pinning does ("always pick the most expensive model, it
is worth it" costs money without naming a single id), so it sits on the same
side of that boundary.
CACHE AND REASONING COUNTS on the stream-json usage event, because a parent
summarising a child's stream is the only place a sub-agent's turn can be priced,
and it was being handed prompt/completion/total alone. A measured plan task had
49,280 of 49,894 prompt tokens served from cache — 98.8% — and every one of
those turns was costed as though nothing had been cached. Emitted only when
non-zero, so a provider reporting no cache produces exactly the event it always
did.
…hat a plan spends A plan ran every task on the session's model and metered a budget it could not enforce. This makes both real. CHOOSING. A task may name a model; auto_assign picks one per task from what the provider serves, ranked by price, with an optional routing call that reads the task text rather than its verbs. Resolution goes through ResolveWithFallback, not ResolveID, because the latter misses regex aliases and skips the deprecation redirect — the plan would display one model while the child ran another. Uncurated ids pass through: the registry holds thirteen models and an xAI account serves half a dozen Grok models, none of them curated. PROVING, because /v1/models is an advertisement. Every model failure this feature produced lived in the gap between listing and serving: grok-4.20-multi-agent-0309 lists like anything else and answers "Multi Agent requests are not allowed on chat completions" — six tasks were assigned it and all six died. A one-token request per candidate, once per session, settles it. It fails toward KEEPING the model: anything not recognised as a refusal changes nothing, because being wrong that way costs one task while being wrong the other way silently removes a model the user pays for. Every candidate refusing points at the provider, not the models, so the list is kept and the user is told. BOUNDING. max_tokens was checked between tasks and decremented after they finished, so nothing in flight could be stopped: a measured plan spent 3,091,618 against a 200,000 budget and still came back incomplete. The meter now reads the child's own stream, a plan too small for its task count is refused before anything is spent, and max_tokens_per_task arrives as a deadline the task is told about rather than a guillotine — a cap that only kills produced 1,437,049 tokens and zero completed tasks. CARRYING. depends_on ordered execution and passed nothing, so a synthesising task received conclusions with no trace behind them and could repeat a claim but never check one. Dependents now receive their dependencies' findings, bounded, with truncation disclosed. RESCUING. A model the provider refuses, or a task that declines rather than answers, gets one attempt on the session's own model. Both triggers are structural — a flag and an exit code — because deciding to spend another child by reading error prose is the shape this package already warns about twice.
The hooks specialist declares, supplied from the two surfaces that own a provider and a sandbox. DISCOVERY FOLLOWS THE LIVE PROVIDER. It was bound once at startup, so a session that switched provider with /model kept probing the one it began on: a run on xai/grok-4.5 had its plan assigned from a nineteen-model Ollama list and the provider refused every task. It now re-resolves through the same path a freshly spawned child takes, config plus ZERO_PROVIDER, so discovery and child execution read one source of truth rather than two kept in sync. The captured profile still wins when the provider is unchanged, because --provider, --base-url and an inline --api-key are applied on top of Resolve and re-resolving would discard them. A CHILD STANDS ON THE SAME GROUND AS ITS PARENT. A run granted a directory outside its workspace could create it, populate it, and then watch every plan task be refused at that same directory — a child rebuilds its sandbox from --cwd alone and the grant lived only in the parent's engine. The run's roots now reach the child as --add-dir, read at launch so a mid-session approval lands, and on the resume path too: this file has already seen the resume path forget what the fresh path carried. PROVING, wired to the provider stack and exposed as `providers models --verify` — which model this provider will actually run, as opposed to which it lists. That is the difference the exclude list was being maintained by hand to express.
The surface a person reads was the last to learn what the plan already knew. THE MODEL THAT RAN, NOT THE ONE THAT WAS CHOSEN. A card's model is set once at dispatch from the assigned model, and a task whose model the provider refuses is re-run on the session's — so the row went on naming a model that never executed it. The plan report the MODEL reads was corrected first; this is the half a person looks at. A refused model is still named, because it stays in the provider's list and the next plan will pick it again. WHY WORK REMAINS, on resume. A plan that ran to the end with failures and one stopped partway both leave tasks behind, and they call for different decisions: re-running things that already failed, or finishing work that never started. PlanProgress.Complete distinguished them and was recorded, never read. WHAT A BUDGET CUT SHORT, in the headline rather than in per-row text. A measured run came back missing a third of its questions and nothing above the fold said which — an incomplete answer that does not announce itself gets used as a complete one. Also removes a workers count that rode planAdmittedMsg for planRunningCardKey, which was deleted when per-task progress began carrying its own task id. The field outlived its only consumer and its comment still sent readers to a function that no longer exists.
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
Comment on lines
+293
to
+322
|
|
||
| // planModelDiscoverer adapts provider discovery for the orchestrate tool's | ||
| // auto_assign. | ||
| // | ||
| // THE ADAPTER LIVES HERE, not in specialist, and that is the point of the | ||
| // narrower type. internal/specialist runs on the child-execution path; importing | ||
| // config and providercatalog to describe a model would drag the whole provider | ||
| // stack in behind it. The surface that already holds a provider profile does the | ||
| // translation and hands over the four facts a plan actually chooses between. | ||
| func planModelDiscoverer(workspaceRoot string, profile config.ProviderProfile) specialist.ModelDiscoverer { | ||
| return func(ctx context.Context) ([]specialist.DiscoveredModel, error) { | ||
| found, err := defaultDiscoverProviderModels(ctx, discoveryCredentialProfile(livePlanProvider(workspaceRoot, profile))) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| out := make([]specialist.DiscoveredModel, 0, len(found)) | ||
| for _, model := range found { | ||
| out = append(out, specialist.DiscoveredModel{ | ||
| ID: model.ID, | ||
| Description: model.Description, | ||
| ToolCall: model.ToolCall, | ||
| Reasoning: model.Reasoning, | ||
| InputCost: model.InputCost, | ||
| OutputCost: model.OutputCost, | ||
| OutputModalities: model.OutputModalities, | ||
| }) | ||
| } | ||
| return out, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether specialist's auto_assign path filters or probes
# discovered models before use, to see if it compensates for the missing
# catalog filter in planModelDiscoverer.
set -euo pipefail
rg -n -C6 'DiscoverModels|ProbeModel|ModelIDAllowedForProvider' internal/specialist/plan_model_assign.go internal/specialist/plan_model_router.go internal/specialist/plan_tool.go 2>/dev/null || trueRepository: Gitlawb/zero
Length of output: 5341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- specialist model assignment and probing ---'
sed -n '620,760p' internal/specialist/plan_tool.go
rg -n -C8 'func proveModels|type ModelProber|ProbeModel|planModelDiscoverer|runProvidersModels|ModelIDAllowedForProvider|CatalogID' \
internal/specialist internal/cli/provider_models.go internal -g '*.go'Repository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- specialist probing definitions ---'
rg -n -C5 'func proveModels|type ModelProber|ProbeModel|ModelDiscoverer|proveModels' internal/specialist
printf '%s\n' '--- provider discovery and plan adapter ---'
sed -n '1,115p' internal/cli/provider_models.go
sed -n '270,335p' internal/cli/provider_models.go
printf '%s\n' '--- plan tool call sites ---'
rg -n -C6 'planModelDiscoverer|ProbeModel:|DiscoverModels:' internal -g '*.go'Repository: Gitlawb/zero
Length of output: 42874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- probe filtering behavior ---'
sed -n '134,235p' internal/specialist/plan_model_probe.go
printf '%s\n' '--- plan prober implementation ---'
rg -n -C12 'func planModelProber|planModelProber' internal/cli internal/providers -g '*.go'
printf '%s\n' '--- provider allowlist enforcement ---'
rg -n -C10 'ModelIDAllowedForProvider|does not allow model|allowlist' internal/providers internal/providermodelcatalog -g '*.go'Repository: Gitlawb/zero
Length of output: 30660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete plan prober path ---'
sed -n '31,115p' internal/cli/plan_model_probe.go
printf '%s\n' '--- probe classification rules ---'
sed -n '1,135p' internal/specialist/plan_model_probe.goRepository: Gitlawb/zero
Length of output: 8196
Filter catalog-disallowed models in planModelDiscoverer.
ProbeModel treats provider-construction errors, including catalog allowlist rejections, as ProbeUnknown. proveModels keeps unknown models, so auto_assign can select IDs that the provider will reject. Apply providermodelcatalog.ModelIDAllowedForProvider before 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 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.
Comment on lines
+396
to
+398
| if len(src.Profiles.PlanModels.Exclude) > 0 { | ||
| dst.Profiles.PlanModels.Exclude = append(dst.Profiles.PlanModels.Exclude, src.Profiles.PlanModels.Exclude...) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not accept model exclusions from untrusted project config.
A cloned project can exclude lower-cost candidates by ID. The automatic selector can then choose a more expensive remaining model. This can raise the user's spend, so exclusion is not a tighten-only operation.
internal/config/resolver.go#L396-L398: Ignore projectPlanModels.Excludevalues, or require an explicit user-approved cost policy before applying them.internal/config/plan_size_test.go#L249-L270: Replace the assertion that project exclusions are safe with coverage that project model preferences do not alter the user's candidate set.
📍 Affects 2 files
internal/config/resolver.go#L396-L398(this comment)internal/config/plan_size_test.go#L249-L270
🤖 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/config/resolver.go` around lines 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.
Comment on lines
+145
to
+166
| func proveModels(ctx context.Context, models []DiscoveredModel, probe ModelProber, cache *probeCache) ([]DiscoveredModel, []string) { | ||
| if probe == nil || len(models) == 0 { | ||
| return models, nil | ||
| } | ||
|
|
||
| verdicts := make([]ModelProbeResult, len(models)) | ||
| var wait sync.WaitGroup | ||
| for index, model := range models { | ||
| if cached, ok := cache.get(model.ID); ok { | ||
| verdicts[index] = cached | ||
| continue | ||
| } | ||
| wait.Add(1) | ||
| go func(index int, id string) { | ||
| defer wait.Done() | ||
| // A panicking prober must not take the plan with it: the worst it can | ||
| // do is leave this model unproven, which keeps it. | ||
| defer func() { _ = recover() }() | ||
| verdicts[index] = probe(ctx, id) | ||
| }(index, model.ID) | ||
| } | ||
| wait.Wait() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The probe fan-out is unbounded, and the comment says it is bounded.
Line 136 states "CONCURRENT AND BOUNDED", but the loop starts one goroutine, and one provider request, per unproven model with no limit. Providers that list large catalogues make this a burst of hundreds of simultaneous requests on the session's own API key. The likely provider answer is 429, which classifies as ProbeUnknown, so the probe pays for the burst and learns nothing, and the throttling can affect the plan's real tasks.
Bound the fan-out.
🔧 Limit in-flight probes
+ // Bounded fan-out: the point is one round trip's latency, not one request
+ // per model at once. A provider listing hundreds of models otherwise
+ // answers 429, which classifies as ProbeUnknown and teaches nothing.
+ const maxInFlight = 8
+ slots := make(chan struct{}, maxInFlight)
verdicts := make([]ModelProbeResult, len(models))
var wait sync.WaitGroup
for index, model := range models {
if cached, ok := cache.get(model.ID); ok {
verdicts[index] = cached
continue
}
wait.Add(1)
go func(index int, id string) {
defer wait.Done()
// A panicking prober must not take the plan with it: the worst it can
// do is leave this model unproven, which keeps it.
defer func() { _ = recover() }()
+ slots <- struct{}{}
+ defer func() { <-slots }()
verdicts[index] = probe(ctx, id)
}(index, model.ID)
}Run the affected concurrent code under the race detector after the change. As per coding guidelines: "Run affected concurrent code under the race detector."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func proveModels(ctx context.Context, models []DiscoveredModel, probe ModelProber, cache *probeCache) ([]DiscoveredModel, []string) { | |
| if probe == nil || len(models) == 0 { | |
| return models, nil | |
| } | |
| verdicts := make([]ModelProbeResult, len(models)) | |
| var wait sync.WaitGroup | |
| for index, model := range models { | |
| if cached, ok := cache.get(model.ID); ok { | |
| verdicts[index] = cached | |
| continue | |
| } | |
| wait.Add(1) | |
| go func(index int, id string) { | |
| defer wait.Done() | |
| // A panicking prober must not take the plan with it: the worst it can | |
| // do is leave this model unproven, which keeps it. | |
| defer func() { _ = recover() }() | |
| verdicts[index] = probe(ctx, id) | |
| }(index, model.ID) | |
| } | |
| wait.Wait() | |
| func proveModels(ctx context.Context, models []DiscoveredModel, probe ModelProber, cache *probeCache) ([]DiscoveredModel, []string) { | |
| if probe == nil || len(models) == 0 { | |
| return models, nil | |
| } | |
| // Bounded fan-out: the point is one round trip's latency, not one request | |
| // per model at once. A provider listing hundreds of models otherwise | |
| // answers 429, which classifies as ProbeUnknown and teaches nothing. | |
| const maxInFlight = 8 | |
| slots := make(chan struct{}, maxInFlight) | |
| verdicts := make([]ModelProbeResult, len(models)) | |
| var wait sync.WaitGroup | |
| for index, model := range models { | |
| if cached, ok := cache.get(model.ID); ok { | |
| verdicts[index] = cached | |
| continue | |
| } | |
| wait.Add(1) | |
| go func(index int, id string) { | |
| defer wait.Done() | |
| // A panicking prober must not take the plan with it: the worst it can | |
| // do is leave this model unproven, which keeps it. | |
| defer func() { _ = recover() }() | |
| slots <- struct{}{} | |
| defer func() { <-slots }() | |
| verdicts[index] = probe(ctx, id) | |
| }(index, model.ID) | |
| } | |
| wait.Wait() |
🤖 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_probe.go` around lines 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.
Source: Coding guidelines
Comment on lines
+32
to
+49
| dispatched := false | ||
| tool := &OrchestrateTool{ | ||
| DiscoverModels: func(context.Context) ([]DiscoveredModel, error) { return elsewhere, nil }, | ||
| ModelPrefs: ModelPreferences{AutoAssign: true}, | ||
| } | ||
| args := map[string]any{"tasks": []any{ | ||
| map[string]any{"id": "a", "prompt": "list the files"}, | ||
| map[string]any{"id": "b", "prompt": "trace the call path and explain it"}, | ||
| map[string]any{"id": "c", "prompt": "decide whether the guarantee holds"}, | ||
| }} | ||
|
|
||
| notes, err := tool.autoAssignModels(context.Background(), args, tools.RunOptions{Model: "grok-4.5"}) | ||
| if err != nil { | ||
| t.Fatalf("a configured default must degrade, not fail the plan: %v", err) | ||
| } | ||
| if dispatched { | ||
| t.Error("the router was called against a provider that cannot serve it") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dispatched can never become true, so this assertion tests nothing.
Nothing assigns dispatched, and the tool wires no RunTask, so the router has no path to run in either case. The check at Line 47 always passes. Wire a runner that records the call, so a regression that routes against the wrong provider's list actually fails the test.
🔧 Record the dispatch
dispatched := false
tool := &OrchestrateTool{
DiscoverModels: func(context.Context) ([]DiscoveredModel, error) { return elsewhere, nil },
ModelPrefs: ModelPreferences{AutoAssign: true},
+ ParentTools: []string{"read_file"},
+ RunTask: func(context.Context, PlanTaskRequest) (TaskResult, error) {
+ dispatched = true
+ return TaskResult{Outcome: TaskSucceeded, Output: "{}"}, nil
+ },
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dispatched := false | |
| tool := &OrchestrateTool{ | |
| DiscoverModels: func(context.Context) ([]DiscoveredModel, error) { return elsewhere, nil }, | |
| ModelPrefs: ModelPreferences{AutoAssign: true}, | |
| } | |
| args := map[string]any{"tasks": []any{ | |
| map[string]any{"id": "a", "prompt": "list the files"}, | |
| map[string]any{"id": "b", "prompt": "trace the call path and explain it"}, | |
| map[string]any{"id": "c", "prompt": "decide whether the guarantee holds"}, | |
| }} | |
| notes, err := tool.autoAssignModels(context.Background(), args, tools.RunOptions{Model: "grok-4.5"}) | |
| if err != nil { | |
| t.Fatalf("a configured default must degrade, not fail the plan: %v", err) | |
| } | |
| if dispatched { | |
| t.Error("the router was called against a provider that cannot serve it") | |
| } | |
| dispatched := false | |
| tool := &OrchestrateTool{ | |
| DiscoverModels: func(context.Context) ([]DiscoveredModel, error) { return elsewhere, nil }, | |
| ModelPrefs: ModelPreferences{AutoAssign: true}, | |
| ParentTools: []string{"read_file"}, | |
| RunTask: func(context.Context, PlanTaskRequest) (TaskResult, error) { | |
| dispatched = true | |
| return TaskResult{Outcome: TaskSucceeded, Output: "{}"}, nil | |
| }, | |
| } | |
| args := map[string]any{"tasks": []any{ | |
| map[string]any{"id": "a", "prompt": "list the files"}, | |
| map[string]any{"id": "b", "prompt": "trace the call path and explain it"}, | |
| map[string]any{"id": "c", "prompt": "decide whether the guarantee holds"}, | |
| }} | |
| notes, err := tool.autoAssignModels(context.Background(), args, tools.RunOptions{Model: "grok-4.5"}) | |
| if err != nil { | |
| t.Fatalf("a configured default must degrade, not fail the plan: %v", err) | |
| } | |
| if dispatched { | |
| t.Error("the router was called against a provider that cannot serve it") | |
| } |
🤖 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_provider_mismatch_test.go` around lines 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.
ONE REAL CONFLICT, in the TUI model's fields. main replaced the /retitle backfill with a local /rename (Gitlawb#826), so its side deletes retitleQueue and the four counters beside it and adds renamePrompt; this branch's side carries the orchestrate plan surface main has never seen. Both, not either: keeping our copy of the retitle fields would have resurrected state whose only callers main removed, leaving it compiling and unreachable. THE POSTURE-OFF GOLDEN MOVED, and it was checked rather than assumed. Gitlawb#838 reworded four tool descriptions (glob, grep, list_directory, read_file), which changes the definition bytes the fingerprint freezes. A moved golden is exactly what a posture leak from this branch would look like, so the two were told apart by measurement: a posture-off run's first request body, compared byte for byte against a binary built from this same main, across --auto low/medium/high/member and --use-spec. All five identical. The absolute sizes moved with main's new wording (33549 -> 31851 on --auto low); the difference between the binaries stayed zero, which is the property the guard exists for. Everything else auto-merged.
Each lands with the test that fails without it.
A MALFORMED DEPENDENCY WAS DROPPED, NOT REFUSED. planStrings skips an entry it
cannot decode — right for a display label, wrong for an edge: "depends_on": [42]
decoded to no dependency at all, so the task was admitted as dependency-free and
ran BEFORE the precondition it declared. Silently; nothing failed and nothing
warned. That defeats this file's own rule that an unknown edge is rejected and
never skipped. depends_on and tools now go through a strict variant that names
the offending index, and the lenient one stays for the fields where dropping is
harmless.
Tasks() SHARED ITS SLICES. copy() duplicates the structs and shares their
backing arrays, so plan.Tasks()[0].Tools[0] = "bash" rewrote the validated grant
in place, from outside, after every check had passed — the widening this file
exists to prevent, reached through the accessor rather than around it.
SavePlan GUARDED THE DOOR AND LEFT THE WALL OPEN. Symlinks were refused on the
directory and on <name>.json, and the bytes go to <name>.json.tmp, which nothing
checked. A repo shipping that name as a symlink turned "save my plan" into the
file-overwrite primitive the other two checks exist to prevent.
A SAVED PLAN LOST THE CALLER'S EXECUTION FLAGS. resolveSavedPlan replaces the
caller's arguments with the stored plan's, which is right for plan content and
wrong for the two flags that say HOW to run rather than what:
{"saved":"sweep","background":true} passed the refusal list and then ran in the
foreground, because background was read from the map that had replaced it.
Plan content is still refused.
SHUTDOWN RAN BACKWARDS. Defers are LIFO, so registering planLaunch.Close()
beside the launcher it belongs to — which reads naturally — meant it ran AFTER
closeSpecialistRuntime. Close cancels AND WAITS, so a background plan was being
waited on with the specialist runtime it needs already torn down. The ordering is
invisible at the point where the next defer would be added, and a comment did not
prevent it happening once, so a source-order guard asserts it.
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
Comment on lines
+590
to
+616
| func TestSavingRefusesToWriteThroughASymlinkedTempFile(t *testing.T) { | ||
| base := t.TempDir() | ||
| target := filepath.Join(base, "precious") | ||
| if err := os.WriteFile(target, []byte("do not clobber"), 0o600); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| dir := filepath.Join(base, "plans") | ||
| if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| // Only the TEMP name is planted; <name>.json itself is absent, so the two | ||
| // existing checks both pass and this is the only thing standing in the way. | ||
| if err := os.Symlink(target, filepath.Join(dir, "sweep"+planFileExt+".tmp")); err != nil { | ||
| t.Skipf("symlinks unavailable: %v", err) | ||
| } | ||
|
|
||
| if _, err := SavePlan(dir, "sweep", savedPlanFixture(t)); err == nil { | ||
| t.Fatal("SavePlan wrote through a symlinked temp file") | ||
| } | ||
| body, err := os.ReadFile(target) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(body) != "do not clobber" { | ||
| t.Fatalf("the symlink target was overwritten through the temp path: %q", body) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate symbols ---'
rg -n "func (SavePlan|refuseSymlink)|refuseSymlink\\(|os\\.(WriteFile|CreateTemp|OpenFile)|planFileExt" internal/specialist --glob '*.go'
printf '%s\n' '--- test context ---'
sed -n '520,660p' internal/specialist/plan_store_test.go
printf '%s\n' '--- implementation context ---'
fd -i 'plan_store.go' .Repository: Gitlawb/zero
Length of output: 7356
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- SavePlan and symlink helper ---'
sed -n '1,180p' internal/specialist/plan_store.go
printf '%s\n' '--- storage write patterns ---'
sed -n '1,130p' internal/specialist/storage.go
printf '%s\n' '--- relevant tests and repository guidance ---'
rg -n "SavePlan|CreateTemp|O_EXCL|symlink|refuseSymlink" internal/specialist --glob '*_test.go' --glob '*.go'
fd -i 'CONTRIBUTING.md' . -x sh -c 'echo --- $1; sed -n "1,220p" "$1"' shRepository: Gitlawb/zero
Length of output: 26159
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Go toolchain and API semantics ---'
go version
go doc os.WriteFile os.CreateTemp os.OpenFile os.Rename 2>/dev/null | sed -n '1,220p'
printf '%s\n' '--- standalone filesystem verifier (uses only a private temporary directory) ---'
python3 - <<'PY'
import os
import tempfile
with tempfile.TemporaryDirectory() as base:
target = os.path.join(base, "target")
temp = os.path.join(base, "plan.json.tmp")
with open(target, "wb") as f:
f.write(b"safe")
# Model the check/use window: the check passes before the attacker swap.
assert not os.path.lexists(temp)
os.symlink(target, temp)
# os.WriteFile uses O_CREATE|O_TRUNC without O_EXCL and follows the
# replacement symlink at open time.
fd = os.open(temp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
try:
os.write(fd, b"clobbered")
finally:
os.close(fd)
with open(target, "rb") as f:
assert f.read() == b"clobbered"
print("separate check followed by write through replacement symlink: reproduced")
# O_CREAT|O_EXCL rejects an existing symlink instead of following it.
secure = os.path.join(base, "secure.tmp")
os.symlink(target, secure)
try:
os.open(secure, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
except FileExistsError:
print("exclusive temporary creation rejects symlink: reproduced")
else:
raise AssertionError("exclusive creation unexpectedly opened symlink")
PY
printf '%s\n' '--- Go module version ---'
sed -n '1,24p' go.modRepository: Gitlawb/zero
Length of output: 1236
Create the temporary file atomically in SavePlan.
refuseSymlink(temp) followed by os.WriteFile(temp, ...) has a TOCTOU race. A process that can modify dir can replace temp after the check, causing os.WriteFile to follow a symlink and overwrite its target. Use os.CreateTemp, write through its returned file descriptor, close it, and then rename it. Add a deterministic regression test for concurrent replacement.
🤖 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 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.
Comment on lines
+154
to
+161
| func TestASavedPlanStillRefusesInlineContent(t *testing.T) { | ||
| tool := &OrchestrateTool{Plans: PlanPaths{UserDir: t.TempDir()}} | ||
| for _, field := range []string{"tasks", "budget", "name", "description"} { | ||
| _, err := tool.resolveSavedPlan(map[string]any{"saved": "sweep", field: "anything"}) | ||
| if err == nil { | ||
| t.Errorf("%q was accepted alongside a saved plan", field) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Seed the saved plan before testing inline-content refusal.
"sweep" is not saved in UserDir. resolveSavedPlan can therefore fail because the plan is missing, even if it accepts inline content. Save a valid "sweep" plan before this loop. Then each error proves the override policy.
Proposed test setup
func TestASavedPlanStillRefusesInlineContent(t *testing.T) {
- tool := &OrchestrateTool{Plans: PlanPaths{UserDir: t.TempDir()}}
+ dir := t.TempDir()
+ if _, err := SavePlan(dir, "sweep",
+ mustPlan(t, []any{task("a", "x")}, okBudget(), readOnlyLimits()),
+ ); err != nil {
+ t.Fatalf("SavePlan: %v", err)
+ }
+ tool := &OrchestrateTool{Plans: PlanPaths{UserDir: dir}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestASavedPlanStillRefusesInlineContent(t *testing.T) { | |
| tool := &OrchestrateTool{Plans: PlanPaths{UserDir: t.TempDir()}} | |
| for _, field := range []string{"tasks", "budget", "name", "description"} { | |
| _, err := tool.resolveSavedPlan(map[string]any{"saved": "sweep", field: "anything"}) | |
| if err == nil { | |
| t.Errorf("%q was accepted alongside a saved plan", field) | |
| } | |
| } | |
| func TestASavedPlanStillRefusesInlineContent(t *testing.T) { | |
| dir := t.TempDir() | |
| if _, err := SavePlan(dir, "sweep", | |
| mustPlan(t, []any{task("a", "x")}, okBudget(), readOnlyLimits()), | |
| ); err != nil { | |
| t.Fatalf("SavePlan: %v", err) | |
| } | |
| tool := &OrchestrateTool{Plans: PlanPaths{UserDir: dir}} | |
| for _, field := range []string{"tasks", "budget", "name", "description"} { | |
| _, err := tool.resolveSavedPlan(map[string]any{"saved": "sweep", field: "anything"}) | |
| if err == nil { | |
| t.Errorf("%q was accepted alongside a saved plan", field) | |
| } | |
| } |
🤖 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_strict_lists_test.go` around lines 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.
…before a plan exists TWO RUNS DECIDED THE FIRST ONE. The same audit of this repo, once ending in a verify task and once not: the verified run dropped five overclaims the other passed through to its report, including an inference the unverified run stated as fact. Fan-out multiplies whatever a finder produces, mistakes included, and nothing in this tool attacks a claim once it has been made. So the shape is TAUGHT, not built. No verdict is parsed and no claim is filtered — a plan author adopts find/verify/synthesize with the tasks they already write, or does not, and the only place that can live is the description they read. The prompt rules that make a verifier worth its tokens are named there too: refute first and confirm only when the code forces it, quote what the code says rather than guessing a refutation, judge each claim independently of the finder's confidence, and re-read the cited locations because a briefing carries a claim and not a trace. AND THE SILENCE BEFORE A PLAN. Auto-assignment runs ahead of admission: a /models call, a probe of every candidate, and when routing is on a full child run on the strongest model. Tens of seconds with no plan, so no panel and no rows — a foreground run looked frozen at exactly the moment it was doing the most. Reported as a STATUS, not a plan row, through an optional recorder interface alongside the others: the plan does not exist yet and admission may still refuse it, so putting a task on screen would show work that never runs. Empty clears it, and the stale-run guard applies as it does to every other plan message.
…nd say when spend cannot be measured A plan that names no max_tokens has nothing to meter against — and on a provider that reports no usage, max_tokens would not have bounded it either: the meter sums the children's own usage events, and a provider emitting none leaves it at zero forever while the work runs. Nothing then stops the plan but the work finishing. SO A PLAN THAT ASKED FOR NOTHING GETS A WALL, and only that plan. An explicit wall wins, and a named token budget is left alone entirely — defaulting a wall over it would put a new ceiling on a plan deliberately running long inside a bound its author chose. An hour is far beyond the 3-5 minutes measured plans on this repo take; it exists to catch a runaway, not to discipline a slow plan. ONE SOURCE FOR THE WALL. Two sites read it — the context timeout that bounds children in flight, and the deadline that stops dispatching new tasks — and only one was reading the new value, so a backstop would have killed running work while still dispatching more. They now read the same function. AND THE REPORT SAYS WHEN SPEND COULD NOT BE MEASURED, because zero tokens after two successful tasks is not a cheap plan, it is an unmeasured one — and a max_tokens set against that number bounds nothing while reading like a guarantee. Verified while doing this and NOT built: update_plan already cannot reach a plan task. Named explicitly it is refused at admission with the tools it may use instead; inherited from the parent's grant it is filtered out. The reasoning is recorded at planReadOnlyTools and holds.
A cloned repo could raise the per-run tool-turn budget, increasing cost for whoever opened it. Project config now mirrors the rule PlanSize and DisableZeromaxing already follow three fields below; user config stays free to raise, being the user's own file and their own money. COMPARED AGAINST THE EFFECTIVE VALUE, NOT THE FIELD, and that is the whole of it. Zero does not mean "no limit" here, it means "fall back to defaultMaxTurns" — and that fallback runs AFTER this merge. The obvious guard, if src.MaxTurns > 0 && (dst.MaxTurns == 0 || src.MaxTurns < dst.MaxTurns) reads as tighten-only and is not: with no user config dst.MaxTurns is 0, the first clause holds, and the repo's 500 is taken. That is the common case — every user who never wrote a config — and the one the rule exists for. Both the original defect and that near-miss are pinned by tests.
…hort Two runs of the same seven-task audit, 3.6 million tokens, and no report at all. Four finder tasks were stopped at their token budget — three of them holding substantial partial findings — and the verify, sweep and synthesis tasks that depended on them were every one skipped. The evidence to write the report was sitting in the results map the whole time. TWO SEPARATE LOSSES, and both had to go. WHAT THE CHILD WROTE WAS DISCARDED AT THE BOUNDARY. The kill path returned an empty Result, so a task stopped mid-investigation handed on nothing however much it had already found. summary.Text held it; nothing carried it. Status stays StatusError — the task did not succeed and must not read as a finished answer — but the text now travels with it. AND A DEPENDENT WAS SKIPPED IF ANY DEPENDENCY FELL SHORT. Now only if EVERY one did. A task with some evidence can do useful work and name what it lacked; a task with none would produce a confident answer drawn from nothing, which is worse than the gap. That is the line, and it is where the old rule sat one step too far to the left. LABELLED, NOT LAUNDERED. Partial input arrives under an INCOMPLETE heading, the briefing opens and closes by saying which inputs were cut short, and the reader is told plainly that an unfinished input's silence is not evidence of anything — nothing was said about X must not become X is fine. A FAILED dependency is still not partial evidence. It ran and did not deliver; its output is a harness diagnostic or an answer already judged wrong, and passing that on as a finding would launder a failure into a source. Only a CANCELLED task — one interrupted while working — carries anything forward.
… schema
Six runs of one seven-task audit, none of which produced a report. Each failure
had a different immediate cause and the same shape: the plan spent its budget and
returned nothing.
RESERVE, IN THREE LAYERS, because fixing one exposed the next.
Feeders consuming the whole budget left nothing to dispatch verify, sweep or
synthesis with, so work that others wait on now stops early. A ceiling alone did
not hold: four tasks in flight each overshoot by about one usage event, and with
max_workers 4 that overshoot was the size of the reserve itself, drawn from the
same account it was capping. Two pools fixed that — upstream cannot touch
downstream's share however far it overruns. Then a sticky budgetExhausted flag
undid it anyway: the moment ANY task was skipped for budget, every task after it
was too, including work drawing from a pool nobody had touched. Per pool now.
SIZED BY THE PLAN, NOT THE BUDGET. A flat quarter gave three downstream tasks
125,000 between them; verify and sweep used 149,769 and synthesis was skipped.
Three of seven tasks downstream reserves three sevenths: a plan that is mostly
synthesis keeps most of its budget for synthesis, one that is mostly finding
keeps little, and a plan with no dependencies keeps all of it.
THE SCHEMA DECLARES ITS SHAPE. tasks was a bare array with a paragraph of
English, so a model composing a plan had to invent the field names — and read
"an optional read-only tool subset", which names no tool, as tools:
["read-only"]. Twice. The item schema now types every field and the tools enum
comes from PlanGrantableToolNames, which admission already validates against and
which was exported for exactly this ("ONE list ... so the grant and the validator
cannot come to disagree"); the schema was the one caller not using it.
AND THE NUMBERS ARE EXPLAINED WHERE THEY ARE CHOSEN. max_tokens bounds the whole
plan and max_tokens_per_task bounds one task; setting both so the cap can never
be reached is now refused with the arithmetic rather than silently resolved in
favour of the smaller. An undersized budget is warned about BEFORE the first task
rather than in the output afterwards — the estimate was computed correctly on
five consecutive runs and read by nobody, because it rode a result that arrives
once the run is already over.
Estimates are weighted by position: a task that reads a repository has measured
510k-1,017k, one that reads its dependencies' findings 62k-87k. Pricing both
alike over-estimated a correct plan by 60%, and a warning that cries wolf is
worse than none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 entire first
HTTP request body is byte-identical to an
origin/mainbinary. Re-proven afterevery 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