mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 23:40:28 -10:00
fd4231bf474ee8b8564a2da71d1c04be39239faf
203
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 KRATOS
|
7595c82afa |
feat(specialist): a plan task may write, if it asks and is isolated (write-task arc, 3/3)
The last step, and the only one that changes what a plan may DO. It is safe to make because the previous two exist: an approval that can show the plan (b52ed77) and a worktree of the plan's own (b2673f8). Making it first would have been asking a user to approve something the screen could not describe, into a tree they had not agreed to have changed. WRITING IS NAMED, NEVER INHERITED. A task that names no tools still gets the read-only grant and nothing else; a task that wants to change something says which tool. Otherwise every unqualified task in every plan becomes write-capable the day the parent grant widens — and the grant DID widen here, which is exactly when that would have happened. AN ALLOW-LIST, not "anything not read-only": write_file, edit_file, apply_patch, bash, exec_command. The negative form would hand a plan task every future tool the moment it is registered, including ones nobody considered when this was written. Every deny-list in this repository has leaked. BOTH GRANT SITES MOVED TOGETHER. validateTaskTools permits a named write tool and planToolGrant delivers one; relaxing admission alone would have produced a task that validated and then ran with less than it asked for, silently. That pair is the sibling rule this feature exists to respect. PROMPTING IS ARGUMENT-AWARE because it has to be: Safety() sees no arguments, so it can only describe the tool, and "this plan writes" is a property of the PLAN. A read-only plan keeps PermissionAllow — friction with nothing to show for it is the click-through problem. An unreadable plan, or a `saved` one whose tasks are in a file the check has not opened, errs toward ASKING: a wrong guess that way costs one prompt, the other way runs write tasks unasked. THE ADDITIVITY GUARD THIS NEEDED. Implementing ArgsPermissioner makes permanentlyDenied return false — deliberately, since only a tool with no per-argument override can be ruled out from its static permission. But orchestrate is gated on the POSTURE, which is a session state and not an argument, so it would have counted as held in every run merely for implementing the interface, and the confirmation policy would have appeared in read-only runs that never turned the feature on. tools.PermanentDenier lets a tool say what ArgsPermissioner cannot, and it is consulted first. Five modes still byte-identical. Seven mutations, all caught. Two existing tests encoded the OLD rule and were rewritten to the new one rather than around it — and TestBothGrantEnforcement PointsAgree now CALLS validateTaskTools instead of restating it, so the next time the rule moves the test moves with it rather than failing against correct code. Verified in the real binary: a write-capable plan outside a git repository is refused with the reason, and inside one it gets a worktree named for the plan. |
||
|
KRATOS
|
f9d6579291 |
feat(specialist): a write-capable plan gets a worktree, or does not run (write-task arc, 2/3)
STEP 2 OF THREE. Step 1 gave the approval prompt something to show; this gives a write-capable plan somewhere to write that is not the user's tree; step 3 allows write tools with both as preconditions. THE REQUIREMENT IS DERIVED FROM THE PLAN, not declared by a caller, and that is what makes step 3 safe by construction. RequiresIsolation reads the plan's own tools: a task naming anything outside the read-only set can change something, which is exactly the condition isolation exists for. Today validateTaskTools rejects such a task so it is always false; the moment step 3 stops rejecting them it becomes true, with no second place needing to be updated. Three defects in this feature have been a rule that lived in one place and a condition that lived in another. PER PLAN, NOT PER TASK. Per-task isolation is more isolation and less use: this phase is sequential and a later task routinely needs an earlier one's output, so per-task trees would either hide that or need a merge step nobody wrote. A plan is the unit a user reviews, so a plan is the unit that gets a tree — one branch, one diff. FAIL CLOSED, four ways, because a precondition that degrades is not one. No isolator, a prepare that errors, an isolator reporting success with no path, and one reporting a path it did not isolate are all REFUSED — never run in the parent tree with a warning. The third and fourth are what a buggy isolator produces, and honouring either would put write tasks exactly where this exists to keep them out of. BOTH DISPATCH PATHS. Foreground and background resolve the same workspace by the same rule, and the background path resolves it BEFORE launching so a plan that cannot be isolated is refused now, with a reason the model reads, rather than failing invisibly on a later turn. It reuses internal/worktrees — the package `zero exec --worktree` already uses — rather than shelling out to git again. Release does NOT delete the tree: a plan that wrote produced work nobody has reviewed, and removing it would remove the only copy. Eleven mutations. I7 initially MISSED and it is the same shape as the Stalled flag: every test drove ExecutePlanIn with a FAKE runner that reads req.Cwd, so all of them passed against a real runner ignoring it. Fixed with a test that drives NewPlanRunner and asserts on the `--cwd` argument the child actually receives — both directions, since a runner that used only the override would put read-only children nowhere. A read-only plan still runs in the parent tree, verified with the real binary: no worktree is created for one. |
||
|
KRATOS
|
1d73e0d8ee |
fix(specialist): drop update_plan from the plan grant, which cost every task ~6.5KB
The last open decision, settled by measuring instead of reasoning — and the
premise I had been carrying was wrong.
I had recorded this as "update_plan blocks the prompt saving; drop it or add a
session-only capability". Driving a real plan and dumping the CHILD request
showed the saving was not reaching plan tasks at all, and the reason was neither
option: update_plan declares readOnlySafety("Updates in-memory planning state
only") and sets NO capabilities, so CapabilitiesOf reports EffectUnknown — the
fail-closed default — while its safety string says read-only. runCanMutate reads
the CAPABILITY. One unclassified tool in an otherwise read-only grant made the
whole child look mutating.
Measured at the wire, same plan, before and after:
child system prompt 18,118 -> 11,639 chars (-36%)
confirmation policy present -> absent
That is per TASK, multiplied by every task of every plan.
DROPPED FROM THE GRANT rather than reclassified, and the distinction matters:
the same capability drives concurrency eligibility, so promoting update_plan to
EffectReadOnly would make it eligible for parallel execution it is not safe for
— it REPLACES the whole plan list on every call, so concurrent callers race and
the last one wins. The safety/capability mismatch is a pre-existing main defect
and is filed as one, not fixed here.
It also has no business in the grant on its own merits. A plan task is an
investigator that reports to the parent; the parent owns the plan list.
WHY NO TEST CAUGHT THIS. internal/agent's readOnlyRegistry enumerated its own
copy of the plan grant, and the copy differed from the authoritative list by
exactly one entry — update_plan, the entry that broke it. So
TestAReadOnlyRunDropsTheConfirmationPolicy asserted the policy was dropped for a
grant that was not the grant, and passed green for the whole period real plan
children carried it. Invariant 5, in a test rather than in production. The
helper now reads specialist.PlanReadOnlyToolNames.
Three mutations. U1 — putting update_plan back — is MISSED by the agent tests
and the fixture now says why in place: a grant name outside the scoped core set
is invisible to it. That half proves "these read-only tools drop the policy";
the other half, that every name in the grant is read-only BY CAPABILITY, is
TestEveryPlanGrantToolIsReadOnlyByCapability against the real registry, where
the same mutation is CAUGHT. Neither alone is the guarantee.
|
||
|
KRATOS
|
f7aeb9bf1f |
feat: run a plan in the background and report it on a later turn (gap report §5.3)
Opt-in via `background: true`, default off, so nothing changes unless asked.
THE CONTRACT IS INJECTION, of the three the report offered. The agent loop
already drains post-edit diagnostics into the conversation tail as a user-role
message — background work reporting into a later turn has a channel here, and a
second one would be a second ordering and a second thing to budget. The
completion goes down the same path, at the same point in the turn.
THE SEAM IS A LAUNCHER, NOT A CONTEXT. A background plan must outlive its tool
call and must not outlive its session, and only the surface running the session
knows where that ends. So the surface supplies a launcher, keeps the context and
drains on exit; the tool asks to be run and is told yes or no. Headless exec's
refusal then falls out of the wiring rather than being special-cased: that
process exits when the turn ends, so it supplies no launcher, and the refusal
says exactly that.
THREE DEFECTS FIXED THAT THE FEATURE WOULD HAVE LANDED ON, all found by sizing
it before writing it:
1. Card keys collided. `dispatched` reset on every Attach, safe only while
every plan lived inside one run; a background plan still dispatching when
the next run attached would restart the counter and hand the new run's
tasks keys the old plan was still using. Now monotonic for the bridge's
life.
2. Progress was silently dropped. The stale-run guard discards anything whose
runID is not the active one — right for a finished run's leftovers, wrong
for a plan still working. The panel would have frozen with no error, no
card, nothing. The guard is now background-aware in all four handlers, and
a test asserts a stale FOREGROUND message is still dropped.
3. The goroutine would have outlived the session. The TUI root was
context.Background(), never cancelled. Now a cancellable session root, and
Close cancels AND WAITS — returning early would let the process exit while
a child was still being torn down, which is how orphans happen.
One at a time, by rule. Two background plans report into one panel keyed by task
id and the second would overwrite the first's rows; refusing the second is
stated, letting the UI silently pick one is a defect.
A background plan returns "NOT finished", never a summary. Returning one would
be reporting work that has not happened, which is this repo's oldest defect
class at the point where it would be least visible.
Eighteen mutations. G7 was a genuine miss — the terminal message's background
mark, which has to be captured BEFORE the flag is cleared and carried down.
Without it a background plan's panel freezes one row from the end: every task
done, the plan never closing. It was caught earlier only because the compiler
complained an unused variable, which is luckier than it deserved to be, and now
has a test. G12 read as a miss and was an inert mutation; re-run as "never
marked at all", CAUGHT.
Verified in the real binary: headless exec refuses it with the reason.
Additivity re-proven, five modes byte-identical. The TUI path needs a user run.
|
||
|
KRATOS
|
5c3649397f |
fix(cli): the model was never told the orchestrate tool exists
Found by the user running the binary: the posture turned on and nothing orchestrated. AgentOptions.OrchestrateAvailable was declared, documented, and consumed by the reminder selector — and set by NEITHER production call site. So the enter notice that names the tool could not fire, in either surface, ever. The tool was advertised in the tool list and the conversation never mentioned it, and a model handed an unfamiliar tool it was never told about does not reach for it. This is invariant 1 for the SECOND time in this feature — the same shape as ParentTools, which was likewise declared, forwarded, consumed by two guards, and populated at neither site. Both were found the same way, and it was not by a test: a unit test on zeromaxingReminders passes the flag in itself, so it stayed green for the whole period the field was set nowhere. Derived from the registry rather than passed as a flag, and set ONCE in the TUI rather than beside each of the three places it assigns Zeromaxing. A bool a caller has to remember is how this happened; the registry is the thing that actually knows. ALSO FIXED, a regression from b2f22d2: adding the `saved` argument dropped both tasks and budget from Required, which emits no "required" key at all — so the model was handed a tool whose every argument looked optional. tools.Schema has no oneOf, so the constraint that is actually true, "tasks and budget, or saved, never neither", cannot be spelled there. It is spelled in Description, which is what a model reads for intent, along with when the tool is worth using at all. Four mutations, all caught, including the two that ARE the reported bug — exec not setting it, and the TUI not setting it. They are caught by driving Run() to the wire and by capturing the tui.Options the TUI is handed, because those are the only levels at which this was ever visible. |
||
|
KRATOS
|
ddeb5ba5e6 |
feat(specialist): save a plan that worked and run it again (gap report §5.7, §5.8)
A plan that took a real sweep to get right could only ever run once. Saved plans live at .zero/plans/<name>.json (project, checked in and shared) or the user config equivalent, with project shadowing user — the same precedence usercommands and the specialist loader already use. A PLAN IS STORED AS ARGUMENTS AND RE-ADMITTED THROUGH ParsePlan. Never as a serialised Plan: the fields are unexported and ParsePlan is the only constructor precisely so nothing can enter execution having skipped validation, and a stored object that deserialises straight into an executable would be that hole with a filesystem in front of it. So a saved plan is re-validated against the CURRENT run — a plan saved when the tier was large is refused when it is small, and one naming a tool this run does not hold is refused rather than granted. Proved with the real binary, not only in tests. RUNNING ONE IS NOT A NEW EXECUTION PATH, which is the deliberate divergence from the report. The report wants saved plans to become /name slash commands; they do not. A plan is not a prompt, and a command that reached into tool execution would need its own copy of the posture gate, the depth check, the grant intersection and the recorder wiring — this feature's whole defect history is second call paths carrying less than the first. Instead orchestrate takes a `saved` argument, /plans run dispatches an ordinary prompt, and by the time ParsePlan sees it a stored plan and a model-supplied one are indistinguishable. A saved plan runs AS IT WAS SAVED: `saved` alongside tasks/budget/name/ description is refused rather than merged, because a half-overridden plan is not the plan whose name is still in the transcript. The name is the PATH GUARD as well as the name guard, and it is an allow-list: no separator, dot or traversal component can be spelled with those characters, so "../../etc/passwd" is refused by the same rule that refuses a space. Symlinks are refused on the file AND the directory, on write and on read — without it "save my plan" is a file-overwrite primitive. A malformed plan file is reported by name rather than skipped, and a lookup past one says the file was unreadable instead of "you have no plan by that name". Save-from-run reads the bridge, not the panel. The panel holds a RENDERING of a plan — ids, statuses, depths — and saving that would write something that merely resembles what ran. Thirteen mutations. N10 could not be killed and the code now says why rather than implying otherwise: the re-admission on the SAVE side is unreachable, because those args came from a Plan that was already admitted. It is handled because ignoring a returned error is how a reachable version goes wrong later, not because it guards anything today. The re-admission that DOES fire is on the way back in, and a hand-edited file that is valid JSON and an impossible plan is now refused there by name. |
||
|
KRATOS
|
fec3276e38 |
feat(config): the plan-size ceiling is a configurable tier (gap report §5.9)
A plan's task count was bounded by ONE hard-coded number with no way to move it. Twenty was too many for someone on a metered provider and too few for a real sweep, and the only way to discover it was to read plan_tool.go. profiles.planSize now names the ceiling: small 5, medium 20, large 50, unrestricted. NAMED rather than numeric so the config file says what it means and so "no ceiling at all" is expressible without a sentinel integer. TWO DELIBERATE DIVERGENCES FROM THE REPORT, both worth stating. It stays a HARD CAP, not the advisory warning the reference implementation has. Under this posture every plan task is a child inheriting a 320-turn ceiling, so fifty tasks authorise 16,000 child turns from a single tool call. A warning the model emits to itself is not a bound on that. What the report actually wanted — that the number be discoverable and movable — is delivered by the rejection naming the tier and the setting instead of printing a bare integer. And medium is 20, not the reference's 15, because 20 is the number defaultPlanMaxTasks already held. Making a bound configurable and moving it in the same change hides a behaviour change inside a mechanism change; the tier mechanism is the deliverable and the number stays where it was. PROJECT CONFIG MAY ONLY TIGHTEN IT. A cloned repo can ask for a smaller ceiling and can never raise one, because raising it raises a spend ceiling for whoever opens the repo. Same privilege boundary as Sandbox.Network's allow/deny rule and DisableZeromaxing's disable-only rule, and a looser tier is dropped silently the way an ignored network "allow" is: the project scope simply does not hold that privilege, which is not a malformed config. Fail closed throughout. An unrecognised tier resolves to the default and never to "no ceiling" — a typo must not be the thing that removes a bound — and the same is true when the early config resolve in exec fails. The mutation sweep found that rank() returned -1 for an unknown tier with a comment claiming that made it safe. It does the opposite: the tighten-only comparison adopts the SMALLER rank, so -1 would have beaten every real tier. Nothing was reachable through it, because ParsePlanSize rejects the name first — but a defence-in-depth value that only holds because of the guard in front of it is not defence in depth. Verified against the real binary, not just the tests: the same six-task plan is admitted with no setting and refused at 5 with planSize small, and a project config asking for unrestricted against a user config of small stays refused. |
||
|
KRATOS
|
7ccae3b9fa |
feat(tui): a plan run in the TUI is durable (gap report §5.2)
The report asked for an append-only journal beside the session store. It is not needed, and ARCHITECTURE.md says why: "Plan state is a deterministic reduction over the five events. No new store." The five events already reached the session log under `zero exec`. The TUI wrote NONE of them — PlanProgressBridge drove the panel and persisted nothing — so a plan was durable or not depending on which surface you ran it from, and only one of the two could ever be resumed. That was the actual gap; a journal would have been a second store papering over it. Events are appended AS THEY HAPPEN, not batched to run end. Batching would lose exactly the crash the durability is for. Written from the TOOL's goroutine, never the Bubble Tea loop — appending is file I/O and the loop must not block on it. Dispatch is written BEFORE the child runs, so a task that was in flight when the process died is distinguishable on resume from one that never started; that ordering IS the guarantee. THE PAYLOADS NOW HAVE ONE AUTHOR. They were built in internal/cli, and a TUI copy would have drifted — with the drift only surfacing when someone resumed a plan recorded by the other surface. specialist.PlanAdmittedEvent and its four siblings are now the single source both recorders append, and a test compares the two by round-tripping through JSON, the form they are actually stored in. Best effort at every level, mirroring execSessionRecorder.append: no store, no session, or a latched earlier failure and it does nothing. The first failure is latched and readable, so a surface can say once that the plan was not fully persisted rather than leaving a user to believe it was. Five mutations, all caught. The fifth needed a test that drives beginRun rather than the bridge: a recorder that records perfectly and is never bound records nothing, which is the shape this feature keeps producing. |
||
|
KRATOS
|
8199c430dc |
feat(tui): make a running plan visible
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. |
||
|
KRATOS
|
3ee471dabd |
fix(cli): measure the run's real depth for plan admission
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. |
||
|
KRATOS
|
e170c6fdcb |
fix(cli): record the failed plan task's session and spend
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. |
||
|
KRATOS
|
e682cb615f |
fix(specialist): make the plan tool's parent grant real and empty mean 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.
|
||
|
KRATOS
|
3b1df93754 |
feat(tui): flip the orchestrate posture gate from the real handlers
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.
|
||
|
KRATOS
|
940aba20e8 |
feat(cli): register the orchestrate tool and map a partial plan to exit 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. |
||
|
KRATOS
|
ac823643ac |
fix(tui): apply the posture's effort fill on models the catalog cannot 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. |
||
|
KRATOS
|
e885d79205 |
fix(execprofile): describe the posture's delta against the caller's own 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.
|
||
|
KRATOS
|
455e7708eb |
feat(tui): make /effort zeromaxing the entry point on both selection 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.
|
||
|
Felix-Ayush
|
58daea7a48 |
feat(providers): add Fireworks AI provider preset (#772)
* feat(providers): add Fireworks AI provider preset Add an OpenAI-compatible Fireworks catalog entry with curated coding models, models.dev alias mapping, onboarding tests, and README setup. * fix(provider): use correct Fireworks DeepSeek V3 model ID Update default and catalog model IDs to deepseek-v3-0324 per Fireworks API. |
||
|
Vasanth T
|
d82b94defe |
test(cli): order the prewarm probe against the turn (#809)
* test(cli): order the prewarm probe against the turn TestRunExecOptimizedSessionUnderGate intermittently sees zero prewarm HEAD probes on the Windows runner. It polled for up to 5s after the run returned, reasoning that the deadline exceeded the 3s prewarm timeout so a slow probe could not flake it. That reasoning does not hold. The probe is issued on the run's context and runExec cancels that context as it returns, because the stop function signalContext hands back is a CancelFunc. Once the run is over the probe is cancelled rather than slow, so polling cannot observe one that has not already landed, at any deadline. Against a mock server the turn finishes in microseconds, which leaves the probe racing teardown. The turn now blocks until the probe arrives, giving it a window while the run is still alive. The wait is bounded, so a probe that never fires still fails the assertion with a useful message. This widens the window rather than closing it: prewarmTimeout is armed inside the probe goroutine, so holding the turn cannot extend it. The comment says so, and names prewarmTimeout as the thing to look at if this ever recurs. * test(cli): fail when the prewarm hold gives up The bounded wait let the turn proceed after its guard expired, so a probe that arrived late still left the count at 1 and the test passed. That proves the probe was eventually received, not that it preceded the turn, which is the only thing this test claims. Raised by CodeRabbit. The expiry is now recorded and checked before the count, since the count cannot tell the two apart. Verified by suppressing the probe, which makes the guard expire: the assertion fires with its own message rather than the count assertion, and the run takes the full guard duration. |
||
|
Felix-Ayush
|
9fbdc34155 |
feat(skills): report hash drift in skill info (#795)
Mirror plugin info: compare on-disk SKILL.md to the skills.lock hash so edited installs are visible. Also drop the nonexistent skills lock command from EXTENDING.md. |
||
|
|
583e6530bd |
fix(lint): resolve static analysis baseline (#769)
* fix(lint): resolve static analysis baseline * fix(lint): clear linux staticcheck findings Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b2ee-7442-8a31-80b2cf9dd270 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Amp <[email protected]> |
||
|
Felix-Ayush
|
fa8734f179 |
fix(cli): include plugins info in shell completions (#794)
zero plugins info landed in #773 but the completions tree still only offered list/add/remove, so generated bash/zsh/fish scripts omitted it. |
||
|
Felix-Ayush
|
2479884dea |
feat(plugins): add zero plugins info command (#773)
* feat(plugins): add zero plugins info command Add plugins info to inspect manifest state, extension counts, and lockfile source/hash with optional hash drift detection. * fix(plugins): derive lock dir from plugin install path for info Remove LockDir from InfoOptions and read the lockfile from filepath.Dir(plugin.PluginDir) so lock metadata matches the resolved plugin location. Consolidate duplicate lock hash output lines. |
||
|
Anandan
|
ab2f9fbd3e |
Add shell completion generation (#764)
* Add shell completion generation Add zero completions for bash, zsh, fish, PowerShell, and Elvish using one shared command tree that covers aliases, nested commands, root flags, and common exec flags. Validate missing and unsupported shell arguments as usage errors and document safe installation examples in command help. Refs #499 * Complete shell target suggestions Expose every supported shell as a completions subcommand candidate and document Elvish activation. Refs #499 * Fix Elvish completion syntax Close generated Elvish conditional branches, use value equality, and validate generated script structure plus native Bash and Zsh syntax. Refs #499 |
||
|
|
ddfdf2878e |
Fix ChatGPT OAuth model discovery and ACP model selection (#724)
* fix: expose authenticated ChatGPT models to ACP Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): persist discovered model selections * fix(acp): normalize model selections --------- Co-authored-by: Amp <[email protected]> |
||
|
Anandan
|
96859c9bd1 |
feat(sandbox): unify command execution and enforcement (#781)
* feat(sandbox): unify command execution and enforcement * test(sandbox): make platform contracts portable * fix(sandbox): close execution lifecycle races Bound retained interactive output, preserve interruption state, and start workspace observation before process launch. Protect active runtime roots with cross-platform leases, capture transient protected-path creation events, and restore process attributes after PTY fallback. * fix(sandbox): restore strict macOS isolation Remove Seatbelt host-local network allowances after native validation showed localhost filters can reach host interfaces. Track process reap completion so retained sessions cannot signal a stale numeric PID. |
||
|
Vasanth T
|
3524f795e5 |
fix(cli): warn when ZERO_PROVIDER overrides a providers-use selection (#767)
Closes #721. `zero providers use <name>` wrote activeProvider to config.json and reported "Active provider set to <name>" even when ZERO_PROVIDER was set, which applyEnv makes win over config.json unconditionally. So the switch was reported as a success while `providers current` still showed the env-pinned provider, a silent no-op. Detect the override and say so: the text path prints a note to stderr naming the env var and the provider that stays active, and the --json path adds effectiveProvider / overriddenByEnv fields so a machine consumer is not misled either. The config write still lands unchanged; only the reporting is honest. The env read is injected via a new appDeps.getenv (os.Getenv in production, left nil by fillAppDeps like exportActiveProvider) so tests stay hermetic against an ambient ZERO_PROVIDER. The related env-derived-profile "not found" case the issue also mentions is already handled by the unpersisted-provider path (#707) and #716. |
||
|
KRATOSandKRATOS
|
a21a052a4a |
feat(sandbox): disable the sandbox via config (#687) (#746)
* feat(sandbox): disable the sandbox via config (#687) Add a sandbox.enabled field (`"sandbox": {"enabled": false}`). ModeDisabled already short-circuits the engine but had no config surface. - SandboxConfig.Enabled *bool (pointer distinguishes an explicit false from an omitted key). - mergeConfig honors it (global config / CLI); applyConfiguredSandboxPolicy maps enabled:false to ModeDisabled. - mergeProjectConfig intentionally does NOT merge it: a cloned repo must not be able to disable the sandbox that constrains it — same posture as AdditionalWriteRoots and the Network tighten-only rule. * fix(config): honor sandbox.enabled from the CLI override path (#687 review) applyOverrides copied BlockUnixSockets/MonitorDenials but not the new Enabled tri-state pointer, so a CLI/programmatic Overrides{Sandbox:{Enabled:&false}} was dropped — contradicting the 'global config / CLI can disable' contract. Preserve the pointer in applyOverrides (runs after all config sources, so an explicit override wins) and add a resolver regression test: an explicit false override disables, and an explicit true override wins over a lower-precedence config false. * fix(config): keep sandbox.enabled out of the provider-command path (#687 review) mergeConfig now copies Sandbox.Enabled, and it is shared by two callers: the global-config merge (trusted) and the provider-command merge (not). A provider command is an arbitrary executable named in config, and LoadProviderCommand parses its stdout into a full FileConfig -- so one returning a valid provider plus {"sandbox":{"enabled":false}} could disable the sandbox meant to constrain it, despite the setting being documented as global-config/CLI only. Confirmed reproducible in both directions before the fix. Clear Sandbox.Enabled from the provider-command config before merging, mirroring the carve-out mergeProjectConfig already applies for the same reason. Done at the call site rather than inside mergeConfig because that helper must keep honouring the setting for the trusted global-config merge. Adds TestResolveSandboxEnabledIgnoredFromProviderCommand covering both directions (cannot disable, cannot enable) and asserting the provider itself is still applied, so the guard cannot be satisfied by dropping the command wholesale. --------- Co-authored-by: KRATOS <[email protected]> |
||
|
PierrunoYT
|
4cbd144d11 |
fix(providers): stop "provider not found" for env-derived profiles (#716)
* fix(providers): stop "provider not found" for env-derived profiles * fix(providers): address env profile review feedback * fix(providers): keep env JSON response schemas stable |
||
|
Anandan
|
2e267bdbee |
feat(tui): add isolated /btw conversations (#748)
* feat(tui): add isolated btw conversations Add /btw with inline-question support, isolated non-resumable session forks, background main-run routing, and safe return behavior. Refs #637 * test(tui): cover btw return controls Preserve drafts when Ctrl+C is pressed during an active BTW run and cover returning to the parent through the documented /btw toggle. * Address BTW isolation review feedback * Reject explicit resumes of side sessions * Harden BTW isolation lifecycle |
||
|
Vasanth T
|
378d538e24 |
perf(agent): execution profiles with one-shot posture escalation (PR10b+PR10c) (#740)
* perf(agent): execution profiles, --exec-profile, bench passthrough, TUI /profile (PR10b+10c) * fix(execprofile): harden profiles against review findings - fast escalates on critical-risk mutations, not high: the sandbox marks every shell command high before command analysis, so a high trigger spent the one-shot on the first go test of any task - escalation can now restore the provider-default effort a profile displaced (PostureEscalation.RestoreDefaultEffort; a plain empty target means leave untouched) - TUI /turns while a profile is active pins the budget: disarms the escalation turn target (mirroring exec's explicit --max-turns rule) and profile reverts use touched bits instead of value equality, so explicit choices survive even when they coincide with profile values - spec-draft runs get the same escalation policy as the main run instead of the displaced budget with no safety net - bench validates and canonicalizes --exec-profile before spawning: an unknown name previously produced an exit-0 report full of millisecond usage-error walls - turn summary prints exec-profile alongside mode; zero exec rejects an inline-empty --exec-profile=; balanced trace label documented and pinned as deliberate attribution * fix(execprofile): explicit effort choices disarm the escalation effort restore An explicit effort set AFTER profile selection could be cleared by a mid-run escalation (the effort analog of the /turns pin, introduced by RestoreDefaultEffort): TUI /effort and Ctrl+T now disarm the restore on a policy clone, and the spec-draft path only arms it when no explicit --spec-reasoning-effort governs the draft. * fix(execprofile): address review findings on the profile surfaces - --use-spec projects away a profile's self-correct knob instead of silently arming an option the draft runner ignores (the explicit --self-correct --use-spec combination is already a parse-time error) - RunTurnBench canonicalizes and validates ExecProfile at the library boundary, so a direct caller's casing/whitespace cannot fork report labels and an unknown name fails before anything runs - reverting a profile whose displaced budget was zero clears ZERO_MAX_TURNS (SetMaxTurnsEnv ignores zero, so sub-agents would have kept the removed profile's budget) - the TUI effort-disarm test fails instead of skipping when the catalog stops filling low effort * fix(tui): reconcile an active profile's effort fill on model switch The fill is per-model, not a session preference: fast selected on a model without low fills nothing, and switching to a model that supports it should behave like selecting the profile there; the reverse switch must not keep sending a level the destination does not support. Both switch paths (the /model command and the cross-provider picker) now re-derive the fill, leaving explicitly touched efforts alone and keeping the escalation's effort restore in step. Also: the revert-env test asserts key removal via LookupEnv, and applyProfileTurnBudget's pinned-budget parameter is named and documented as covering mode presets, not just the flag. * fix(tui): void profile effort bookkeeping when the model-switch drop erases it The /model path's pre-existing unsupported-effort drop ran before the profile reconciliation and left contradictory state behind: a dropped touched override kept blocking the profile from governing, and a dropped profile fill left a stale applied value with the effort restore still armed. The drop itself is load-bearing (the TUI run path forwards the effort to the provider unfiltered), so instead of preserving a known-unsupported level, the voided choice now resets the profile's effort bookkeeping and the reconcile re-derives from scratch. The whole sequence lives in one ring-parameterized helper driven by the switch target's authoritative ring, unit-tested against synthetic rings for both the void-then-refill arc and the supported-override survival. * fix(tui): only drop an explicit effort when the target ring is authoritative A nil ring meant both a catalog model with no effort controls and a live-discovered custom model the catalog cannot vouch for. The drop now requires an authoritative ring (a resolved catalog entry, whose ring may legitimately be empty); an unknown ring preserves an explicit preference, matching the cross-provider picker's behavior, while the profile's own fill still applies only where support is known. |
||
|
Vasanth T
|
60dc84e7a3 |
perf(openai): optimized turn session — background prewarm and prefix telemetry (PR8) (#723)
* trace(perf): add provider prewarm span and prefix-stability counters * feat(openai): optimized turn session with prewarm and prefix telemetry * feat(providers): gate optimized OpenAI turn sessions behind env flag * feat(cli): wire optimized turn sessions into headless exec * docs(readme): document the optimized OpenAI turn-session toggle * fix(openai): include tool descriptions in prefix digest, harden gate and test bounds * fix(openai): isolate prewarm tracing, skip non-reusable transports, fingerprint wire order and cache key * test(openai): pin keep-alive transports in probe-expecting tests * feat(providers): preserve resolved capabilities on switched-model fallback sessions |
||
|
KRATOSandKRATOS
|
e5670c427f |
perf(output): add token-aware semantic output budgeting (PR11) (#717)
* perf(output): add token-aware budget contract * perf(output): add semantic retention policies * perf(output): budget results after redaction * perf(output): propagate tool truncation metadata * perf(output): trace semantic budget decisions * perf(output): classify core tool output * docs(output): describe semantic output budgeting * test(output): make spill fallback portable on windows * fix(output): address semantic budgeting review findings * test(output): make hook budgeting tests portable * fix(output): retain oversized single-line file content * fix(output): apply semantic budgets to captured output * fix(output): preserve hook budgets and search paths * fix(output): preserve alias and truncation metadata * test(trace): validate output budget metadata --------- Co-authored-by: KRATOS <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
9043baedcf |
fix(sandbox): scrub dynamic credential env vars (#682)
* fix(sandbox): scrub dynamic credential env vars * fix(sandbox): tolerate env assignments in configured sensitive keys A config value mistakenly given as a full assignment (e.g. "COMPANY_LLM_SECRET=...") would never match the real env key during scrubbing, silently re-exposing the credential to sandboxed commands. normalizeSensitiveEnvKeys now keeps only the name part before '=' and re-trims it, per PR review. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(sandbox): scrub credentials on direct/degraded command plans directCommandPlan (used when the native sandbox backend is unavailable, disabled, or not required, including the EnforcementDegraded fallback) left Env nil whenever spec.Env was nil, so exec.Cmd inherited the full caller environment unscrubbed. Configured apiKeyEnv credentials and dynamically named ZERO_OAUTH_<NAME>_CLIENT_SECRET variables leaked through that path even though the wrapped sandbox plans already scrubbed them. Route direct plans through scrubSensitiveEnv the same way wrapped plans do, snapshotting os.Environ() when spec.Env is nil so scrubbing still applies. Adds a regression test covering the degraded-fallback path with both a configured and a dynamically named secret. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
|
6b9c2f0c08 |
feat(aimlapi): AI/ML API provider with guided onboarding (top-up + key issuance) (#655)
* feat(aimlapi): AI/ML API guided onboarding (top-up + key issuance) Add the AI/ML API provider integration to the zero CLI: a guided TUI onboarding sub-flow that takes an existing key or an email top-up and writes the issued key into the provider profile, attributed to the Gitlawb rebate partner. - internal/aimlapi: OpenAI-compatible client, partner-checkout top-up, key validation, streaming, and endpoint config (Paths A/B/C). - providercatalog / providermodelcatalog: register the aimlapi provider with default headers and models. - tui: aimlapi onboarding screen wired into the provider wizard and onboarding flow. Default endpoints point at production (auth/app/api/pay/aimlapi.com); each is overridable via AIMLAPI_*_URL env vars. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): address review findings in onboarding flow - topup: use errors.As for the 5xx retry check so a wrapped APIError still retries instead of aborting a paid checkout. - onboarding: a Path-A top-up on an email with no account now lands on a single coherent "key saved, balance not topped up" screen instead of a green "ready" plus a red error. - onboarding: an empty amount field is rejected instead of silently falling through to the $25 default. - collapse the two identical top-up prompt constants into MsgTopUpPrompt and drop the dead amountPrompt() branch. - reuse stdlib maps.Clone instead of a third bespoke header-copy helper. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): retry transient poll errors and sharpen amount validation * fix(aimlapi): retry transient poll errors and surface real amount errors * docs(aimlapi): document exported client API; drop out-of-scope gitignore entry Addresses the two CodeRabbit PR checks on #655: - Add doc comments to the exported aimlapi client API (Client, APIError, the session/checkout/onboarding types and their methods) and ParseAmountUSD, to satisfy the docstring-coverage check. - Remove the unrelated /zero.exe~ .gitignore entry that an earlier review-fix commit added; it is out of scope for the provider work, so the PR no longer touches .gitignore. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(aimlapi): render aimlapi row like other recommended providers * fix(aimlapi): resolve onboarding review findings (Path A funding, base URL, request cancellation) * fix(aimlapi): validate pasted API keys * test(aimlapi): clarify pasted key validation * test(aimlapi): cover key-balance low-balance transition * fix(aimlapi): reject non-finite top-up amounts * fix(cli): scope and canonicalize provider headers * fix(tui): distinguish invalid verification codes * feat(aimlapi): resume account-bound top-ups safely * feat(tui): offer top-up for existing AIMLAPI keys * feat(tui): reuse AIMLAPI env key on first run * feat(tui): reuse configured AIMLAPI profiles * test(aimlapi): trim redundant flow coverage * fix(tui): preserve existing AIMLAPI profile metadata * fix(tui): clone AIMLAPI headers in env fallback * fix(aimlapi): address remaining review findings * fix(aimlapi): harden onboarding lifecycle * fix(aimlapi): preserve validated checkout context * fix(aimlapi): harden checkout retry state * fix(tui): clarify AIMLAPI configured choices * fix(aimlapi): make checkout retries idempotent * fix(aimlapi): validate checkout responses * fix(providers): reconcile aimlapi catalog after rebase --------- Co-authored-by: Lookoff123 <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> |
||
|
euxaristia
|
9acb4113cc |
feat(cli): show ZERO wordmark on --version (#673)
* feat(cli): show ZERO wordmark on --version The TUI's empty-state ZERO ascii art gets exposed via a new tui.Wordmark helper and printed above the version line for `zero -v`/`--version`. Coloring only applies when stdout is a real TTY and NO_COLOR isn't set, so redirected or piped output (scripts parsing the version) stays plain. * fix(release): match --version output by suffix, not exact string The release smoke check compared zero --version output to an exact "zero <version>" string, which broke once --version started printing the wordmark above the version line. Check for a "version: <version>" suffix instead so the smoke check doesn't couple to the wordmark art. * fix(cli,tui): keep --version machine-readable off-TTY, drop wordmark color - When stdout is not a terminal, --version prints exactly "zero <version>" again: pipes, redirects, command substitutions, and the release smoke contract all parse that single record, so the banner is a TTY-only affordance now. - The TTY path keeps the banner but restores "zero <version>" as the version record instead of "version: <version>". - Wordmark renders uncolored: only newModel resolves --theme/ZERO_THEME, so painting the global dark palette here was unreadable on light terminals. The terminal's default foreground works on any background. - smokeVersion goes back to requiring the exact "zero <version>" output instead of the suffix match that papered over the banner. |
||
|
|
d66a9dda69 |
feat(providers): add AI/ML API preset (rebased onto main) (#621)
* feat(providers): add AI/ML API preset * fix(providers): relocate aimlapi preset and drop referral header Addresses the two review asks on #402: - Move the AI/ML API descriptor out of catalog slot #2 (it sat above the first-party OpenAI/Anthropic/Google entries) down next to openrouter in the aggregator cluster. Re-sync expectedCatalogIDs and the TransportOpenAICompat order-list to match. - Drop the always-on X-AIMLAPI-Partner-ID / Integration-Repo / Integration-Version attribution headers. There's no partner arrangement behind the preset, so a non-overridable referral tag on every user's request isn't something to ship on a community preset. aimlapi now rides through plain CopyHeaders like every other provider. Removes HeadersForCatalog (it existed only for this special-case), reverts the factory and discovery call sites to CopyHeaders, and drops the header test. The factory/discovery tests now assert the endpoint, auth, and the user's own custom headers survive while no attribution header is sent. * fix(providers): address CodeRabbit review on #621 - Document AIMLAPIID for docstring coverage. - Apply providerio.CopyHeaders for Anthropic and Google provider paths, matching the OpenAI path introduced for aimlapi header isolation. Co-authored-by: Cursor <[email protected]> * fix(providers): gofmt factory.go to unblock CI smoke Remove mixed tab/space indentation in the openai.New options block so ubuntu-latest fmt-check passes. --------- Co-authored-by: Dmitry Tumanov <[email protected]> Co-authored-by: Vasanthdev2004 <[email protected]> Co-authored-by: Cursor <[email protected]> Co-authored-by: KRATOS <[email protected]> |
||
|
Leonardo Faoro
|
7d579996b4 |
feat(skills): discover shared ~/.agents/skills with multi-root skill loading (#696)
* feat(skills): discover shared ~/.agents/skills with multi-root skill loading Add ~/.agents/skills as a read-only global discovery root after the primary Zero skills dir and before plugin roots. Unify runtime and CLI discovery on one multi-root path (DiscoveryRoots / LoadFromRoots / GlobalRoots) and always register the multi-root skill tool so bare runs and plugin runs share the same load surface. Install/remove/lock still target only the primary Zero skills directory. Fixes #610 * docs(skills): note lock stays on the primary write root Document that install/remove/lock target only the primary Zero skills directory, matching multi-root discovery write-root isolation. * fix(skills): bubble primary root load errors from LoadFromRoots Return non-missing failures from the first non-empty skills root so a broken primary dir is not reported as an empty skill list. Keep fail-open behavior for optional roots (~/.agents/skills, plugins). * fix(skills): treat non-directory skills root as error on Windows Windows maps ENOTDIR to ErrNotExist, so ReadDir on a regular file looked like a missing skills dir. Reclassify existing non-directories and assert the portable load behavior in tests. |
||
|
Vasanth T
|
abe24bcb10 |
Add per-turn tracing and a turn-benchmark harness (Phase 0 baseline) (#700)
* Add per-turn tracing and a turn-benchmark harness (Phase 0 baseline)
Introduce an opt-in `internal/trace` package that attributes a run's wall
time to named spans — prompt build, provider connect/queue, generation,
tool queue/execution, permission wait, verification, compaction,
persistence — plus counters for model requests, tool calls, retries,
reconnects, compactions, completion nudges, acceptance checks, model
switches, and input/cached/output tokens. The recorder is nil-safe (a nil
*Recorder is a no-op), so every stamp site can call it unguarded; an
untraced run is byte-identical to today.
The recorder threads into agent.Run via a new `Options.Trace` field and a
single `trace.WithContext` assignment, so the shared providerio seam
(connect around client.Do, queue around OAuth resolve, first token at the
first non-keepalive SSE payload) reaches it through `trace.FromContext`
without any provider-interface change. All instrumentation is opt-in: when
`Options.Trace` is nil the hot path is untouched.
Headless `zero exec` gains `--trace <path>` (and `ZERO_TRACE=<path>`),
writing an agenteval-contract-compatible NDJSON trace (`-` for stderr).
The trace is pure observation; enabling it does not change behavior.
A new per-turn benchmark harness extends `internal/perfbench` (reusing the
existing TaskSet/NumericStats/subprocess runner shapes): `RunTurnBench`
aggregates per-span median/P95 across iterations, ranks the top three
controllable latency sources by share of attributed time, rolls up per
task class, and totals tokens/counts. A checked-in 42-task manifest across
seven classes (nav, edit, fix, refactor, longproc, longctx, parallel) and
local offline fixtures back it, and `make baseline` runs it against the
built `zero`, writing `internal/perfbench/reports/baseline.json`. The
report is machine-specific and regenerated, not hand-edited.
The manifest is JSON rather than YAML to avoid adding a dependency;
per-task verification targets the specific test (`go test -run <name>`)
so fix tasks are independent. Each task is a fresh `zero exec` process, so
iterations are cold-start samples; a warm in-process path is left for
follow-up.
Verified: `go build ./...`, `go vet ./...`, `gofmt -l` clean on touched
files; `go test -race` green for internal/trace, internal/agent,
internal/perfbench, and cmd/zero-perf-bench.
* test(trace): make TestAttributionRatio clock-independent
The test recorded two synthetic 10ms spans with no real delay and asserted
the attribution ratio stayed <= 1.0. On a precise clock (Linux/macOS CI) the
wall is microseconds while attributed is 20ms, so attributed/wall blows past
1.0 and the test failed — even though the ratio legitimately exceeds 1.0
when spans overlap (parallel tool execution). On Windows the coarse timer
masked it, so the failure only surfaced in CI.
Assert the contract instead: attributed is the deterministic sum of span
durations, and the ratio equals attributed/wall (0 when wall is zero). Add a
zero-wall case for the divide-by-zero guard.
* Address review feedback on the per-turn tracer + benchmark harness
Benchmark runner (internal/perfbench):
- A nonzero agent exit code no longer counts as Passed. The runner set
VerifyErr but then fell through to outcome.Passed = true, so a crashed
run could be recorded as a pass. Return before the verification block
when VerifyErr is already set.
- A task now passes only when every iteration passes. The previous
"any iteration passes" rule let a flaky fail hide behind one good run.
- Each invocation copies its workspace fixture to a fresh temp dir and
runs the agent + verifier there, so mutating tasks (edit/fix/refactor)
can't dirty the checked-in fixtures or bleed into the next iteration.
- Trace load failures (missing/malformed trace file) are surfaced as
result warnings via a new TraceIssue field instead of being swallowed.
Tracer (internal/trace):
- Finish freezes the recorder: Counter, addSpan, and the first-event
stamps are dropped after Finish returns, so a snapshot captures exactly
one turn and late stamps can't mutate it.
- WriteText propagates the first write error instead of silently
swallowing sink failures.
Provider seam (internal/providers/providerio):
- FirstTokenAt is stamped only after a payload is accepted as real model
output, not before handle() decides to drop it as an error payload.
Agent loop (internal/agent):
- The compaction counter increments only when the history actually
shrinks, in both the proactive and reactive paths — paid no-ops no
longer inflate the compaction total.
- Document on Options.Trace that a fresh Recorder is required per Run.
CLI (internal/cli/exec):
- Finish the trace at the agent.Run boundary and defer only the
serialization, so the snapshot covers exactly the turn.
- Reject --trace / ZERO_TRACE for spec-draft runs with a clear error
instead of silently accepting and writing nothing.
Manifest + fixtures:
- Strengthen the edit verifiers: rename and value-bump tasks now assert
the old symbol is gone, and the getter task greps the method signature.
- Add the missing testdata/edit fixture (10 edit tasks referenced a
directory that was never committed, so the whole edit class errored
out at run time).
- The manifest test now stats each fixture so a missing dir fails at
load time instead of at every run.
- longproc benchmark keeps Process live across the loop; README code
fence gets a language tag and documents the per-invocation fixture copy.
- Add tests for the tracing wrapper (nil vs wired recorder, OnUsage
forwarding, token aggregation) and the frozen-recorder contract.
* Address review feedback: exclusive-time attribution + wired spans + fix-suite compile
Rewrite the trace attribution model so concurrent/nested spans no longer
double-count. Spans record wall intervals; the recorder derives a parent for
each by interval containment and computes exclusive time (duration minus the
union of its children). AttributedDuration sums only top-level spans; Coverage
is the union of intervals over wall, capped at 1, the honest ">=95%" metric.
Top-latency ranking uses exclusive time so shares sum to ~1 instead of being
inflated by overlapping provider_connect/generation and nested
permission_wait/tool_execution.
Wire the spans that were named but never stamped: verification around the
self-correct verify pass, and StampFirstVisibleEvent on the first forwarded
text event. Drop the tool_queue/process_wait/persistence constants that had no
stamps, and rename the misnamed near-noise prompt_build span to tool_partition.
Trim RequiredEventKeys to events a healthy short run actually emits; move
conditional phases (tools, permission, compaction, verification) to
OptionalEventKeys so a short trace does not false-fail.
Make ReadNDJSON fail loudly on a corrupt file: require a type:trace header,
error on span/counter lines that appear before it, and error when a header
yields no spans or counters.
Fix the blocking fix-suite compile error: Label's %d format verb on a string
tripped go test's vet before any fix task could verify. Make it a runtime
wrong-prefix bug ("admin-" vs "user-") that still type-checks, so each fix
task is independent again.
Manifest: correct refactor-04's stats.go to main.go, reword fix-07 to the
wrong-prefix bug, add a latency-only description (read-only tasks carry no
verification oracle by design), and frame the report README on exclusive time
and coverage.
Tests updated for the occurrence/exclusive/coverage model; trace, perfbench,
and agent green under -race.
* Fix exclusive-time round-trip, identical-interval nesting, and empty-trace parse
Three correctness bugs in the attribution model found by an adversarial review
of the prior commit, all undermining the no-double-count guarantee:
1. exclusive_ms:0 was overwritten with the inclusive Duration on re-parse. A
parent whose children tile its interval legitimately has exclusive time 0;
WriteNDJSON emits exclusive_ms:0, but ReadNDJSON's "<= 0 means missing"
fallback turned that back into the full duration, so every re-parsed trace
re-introduced the double-counting the model exists to prevent (the benchmark
harness, which ranks re-parsed traces, inherits it). Gate the fallback on
key ABSENCE, so a written 0 is preserved.
2. deriveNesting formed a 2-cycle for two spans with identical [start, end]
intervals: the symmetric containment check made each parent the other,
dropping both from top-level and zeroing AttributedDuration. Break the tie
deterministically — strict containment is an unambiguous parent; for
identical intervals, only the lower-indexed span may parent the other, so
one stays top-level.
3. ReadNDJSON returned a valid empty TurnTrace for empty/blank-only input, so a
run that crashed before emitting a trace (or where --trace was not honored)
masqueraded as a clean zero-attribution sample. Empty input now errors, so
the harness records a TraceIssue.
Also: decode counters via json.Number so int64 values above 2^53 round-trip
exactly instead of losing precision through float64; and drop the harness's
own "exclusive <= 0 -> Duration" ranking fallback, which was the same
double-count on the re-parsed path. Added regression tests covering the
zero-exclusive round-trip, the identical-interval nesting, empty-input
rejection, and counter precision.
* perf(turn-bench): split pass/fail into oracle tiers so the pass rate can't be misread as correctness
The turn benchmark counted any exit-0 task as passed, including the 24
read-only tasks (nav/longproc/longctx/parallel) that carry no verification
oracle — so the JSON tasksPassed / "N/M passed" headline could read as a
correctness verdict it never was. Split pass/fail into three oracle tiers:
- correctness (edit grep + fix go test): the only pass rate that can move
with model quality (tasksVerified / tasksPassed / correctnessPassRate)
- build-only (refactor go build): a non-positive oracle, reported as
buildPassRate and excluded from correctnessPassRate; the manifest declares
buildOnlyClasses: ["refactor"]
- latency-only (no oracle): counted in latencyOnlyTasks, never in any pass
rate
NewTurnExecRunner no longer sets Passed=true for tasks with no
verificationCommand, and the summary headline names each tier explicitly so
an exit-0 read-only run cannot inflate a correctness number. Bumps
TurnSchemaVersion to 2. Class breakdown and tier contract documented in
internal/perfbench/MANIFEST.md and reports/README.md.
Also install the trace first-token/visible-event forwarding handlers in
agent.Run whenever options.Trace is set, not only when a UI OnText /
OnReasoning callback is set, so a headless traced run (zero exec --trace)
still captures FirstTokenAt. forwardedVisibleText stays tied to the user
callback so stall-retry semantics are unchanged. Adds a regression
assertion to the existing traced-run test.
* Address CodeRabbit review on
|
||
|
Felix-Ayush
|
75a78e715b |
feat(cli): wire MCP serve WorkspaceRoot and --add-dir scope (#694)
* feat(cli): wire MCP serve WorkspaceRoot and --add-dir scope Pass the resolved workspace into mcp.ServeOptions and honor --add-dir so resources/list and scoped tools can expose extra roots beyond cwd. * fix(cli): keep lexical serve --add-dir roots for path matching Use symlink-resolved paths only for deduplication so Scope roots stay aligned with the un-evaluated WorkspaceRoot MCP serve already passes. |
||
|
euxaristia
|
6fc1220f6a |
fix(sandbox): scrub sensitive credentials from sandbox environment (#660)
* fix(windows): retry atomic file renames on Access is denied / Sharing violation * test(swarm): add unit tests for rename retry and non-retryable errors * test(swarm): skip TestMailboxRenameRetry on non-Windows platforms * fix(sandbox): scrub sensitive provider credentials from sandbox env * refactor(sandbox): unify environment creation and reduce lines of code * fix(sandbox): forward workspaceRoot through sandboxEnvironment, gate Windows env on backend * style(sandbox,swarm): remove stray blank lines flagged by gofmt * refactor(fsutil): extract shared rename retry helper, derive scrub list from provider catalog Address CodeRabbit review feedback: - Add internal/fsutil.RenameWithRetry with an injectable rename override and move the four duplicated renameWithRetry/isWindowsSharingViolation copies (cron, sessions, sandbox, swarm) onto it. Mailbox keeps its rename test seam by passing its field through. Add fsutil tests for the Windows retry and non-retryable paths. - Remove the orphaned metadata.json.tmp in cron writeJob when the rename permanently fails, matching the sibling writers. - Derive scrubSensitiveEnv keys from providercatalog AuthEnvVars plus a static list of non-catalog secrets, so new providers are scrubbed automatically. AWS_PROFILE and GOOGLE_APPLICATION_CREDENTIALS stay because they are pointers, not secrets. * fix(sandbox): deny sandboxed reads of cloud credential stores by default Address CodeRabbit finding that scrubbed env pointers still left the underlying credential files readable under the read-all workspace posture: - Add credentialDenyReadPaths: default profile-level deny-read entries for ~/.aws, ~/.config/gcloud, ~/.azure, and the GOOGLE_APPLICATION_CREDENTIALS target file, wired into both permission profile builders so the seatbelt and Linux helper backends enforce them. Entries are stat-filtered and a store nested under a user AllowRead entry is dropped, keeping an explicit opt-out. - Keep the defaults out of Policy.DenyRead, whose emptiness gates escalated execution, and skip Windows so the profile deny list does not flip the runner off the WRITE_RESTRICTED token path. - Scrub GOOGLE_APPLICATION_CREDENTIALS from the sandbox env; keep AWS_PROFILE since the deny-read rule on ~/.aws is the real protection and the SDKs fall back to the default profile regardless. - Add credential deny path tests, extend the scrub test, relax the profile test's exact deny count to containment. * test(cli): pin HOME in sandbox policy golden test The default credential-store deny-read entries depend on what exists in the real home directory (the macOS CI image ships ~/.aws), which leaked host paths into the policy JSON golden comparison. Point HOME at an empty temp directory so the golden stays hermetic. * fix(sandbox): protect GOOGLE_APPLICATION_CREDENTIALS without a home, retry lock violations Address CodeRabbit review feedback: - credentialDenyReadPaths no longer returns early when the home lookup fails; home-based candidates become conditional and the GOOGLE_APPLICATION_CREDENTIALS target is always considered. - fsutil.RenameWithRetry also retries ERROR_LOCK_VIOLATION (errno 33), matching the mcp lock handling; helper renamed to isWindowsSharingOrLockViolation. - Extend tests: homeless GOOGLE_APPLICATION_CREDENTIALS case and a lock violation step in the retry sequence. * fix(sandbox): tighten env-scrub test assertion and add missing secret keys TestScrubSensitiveEnv only checked an allowlist plus a length count, so a duplicate allowed key silently replacing a required one would still pass. Compare the exact expected key/value map instead. Also add ZERO_WEBSEARCH_API_KEY and ZERO_DAEMON_REMOTE_TOKEN to scrubSensitiveEnv's sensitiveKeys list: both are real bearer secrets read from the environment but were missing from the scrub list, leaving them readable by sandboxed commands when set in the parent shell. |
||
|
euxaristia
|
5c4815a66e |
fix(sandbox): unblock git fetch/commit/add under the write-restricted sandbox (#654)
* fix(sandbox): stop blocking git fetch/commit/add by unblocking .git writes The sandbox denied every write under .git for shell-executed commands, so any git operation that touches its own metadata (fetch, commit, add, pull, merge, stash) failed under the default sandbox. Narrow the block to .git/hooks (auto-executing scripts) and .git/config (remote URLs, credential.helper, core.hooksPath), the two subpaths that are actually dangerous to write; the rest of .git stays writable to git subprocesses since those writes go through git's own invariants. The app-level auto-allow gate for direct file-tool writes (write_file, edit_file, apply_patch) is unaffected and still treats all of .git as protected, since hand-editing git's internals bypasses those invariants. * test(cli): normalize Windows path separators in sandbox policy golden The sandbox policy golden is shared across platforms, but readOnlySubpaths carve-outs are built with filepath.Join and render as backslash paths on Windows. normalizePortableJSONRootString only converted a bare backslash, so the Windows smoke run diverged from the forward-slash golden. Replace all backslashes with slashes during normalization so the shared golden passes on every runner. |
||
|
euxaristia
|
635c93af51 |
fix(agent): raise default and deep-mode turn budgets (#650)
* fix(agent): raise default and deep-mode turn budgets The default per-run tool-turn budget of 50 was still too low for larger multi-step tasks that span several files: agents hit the ceiling and stopped with a "remaining work" summary instead of finishing. Raise the default from 50 to 80. The "deep" preset also carried MaxTurns: 50, identical to the default, so its "larger turn budget" promise no longer held on the turn-budget axis. Raise it to 160 so deep runs get a genuinely larger budget again. Users can still override per-session with /turns or --max-turns (bounded by MaxTurnsCeiling). * test(cli): expect deep-mode MaxTurns=160 after budget raise |
||
|
euxaristia
|
e4bd703cfb |
fix(cron): prevent cron job Mutate from clobbering concurrent updates (#630)
Inside fireJob, the Mutate transaction callback previously returned the stale in-memory job copy (read at tick start). This clobbered any concurrent updates made to the job (such asExpr, Prompt, or Cwd edits) while the job was running. Update the callback to apply schedule and status changes directly onto the fresh current job loaded from disk, and return current. |
||
|
euxaristia
|
5b4f48d2dc |
fix(cli): prevent consuming positional arguments as flag values (#619)
When parsing CLI flags, positional prompt text placed immediately after flags requiring values (e.g. --auto, --notify, --max-turns) would be greedily consumed as their value by nextFlagValue. Refine nextFlagValue to validate choices/types (autonomy levels, notify modes, integers) to detect invalid values early and fail validation rather than eating positional arguments. |
||
|
Gautam ManchandaniandGautam Manchandani
|
fdddb05ba8 |
fix: harden MCP credential boundaries (#597)
Co-authored-by: Gautam Manchandani <[email protected]> |
||
|
euxaristia
|
b6117af86d |
fix(exec): stop false INCOMPLETE downgrades on conversational final messages (#608)
* fix(agent): scope self-reported incompletion to sentences about the current objective
The detector matched inability stems anywhere in the final message, so a
conversational recap of a past exchange ("You asked if I could work
autonomously ... so I couldn't actually do it at the time") downgraded a
completed run to INCOMPLETE. Match per sentence instead, drop quoted spans,
and skip sentences that retell a past exchange. The real transcript is now a
regression test, and genuine admissions still fire, both present tense and
past tense about the current task.
* feat(exec): add --no-completion-gate for conversational callers
A chat frontend driving zero exec has an operator present, so a final
message that hands the turn back ("the sandbox blocks network egress,
approve it and I'll continue") is a complete conversational answer, not an
unfinished task. The headless completion gate still stamped such runs
INCOMPLETE (exit 4) on the honest inability phrasing, and no phrase
heuristic can fix that case because the admission is true. The new flag
maps to RequireCompletionSignal=false so conversational callers can opt
out. Default behavior is unchanged.
* fix(exec): address review findings on quote stripping and --use-spec combination
stripQuoted now removes only balanced quoted spans: an opening delimiter
that never closes is restored verbatim, so a stray quote cannot swallow
the rest of the message and hide a genuine admission from the detector.
Regression tests cover dangling straight, curly, and backtick delimiters.
--no-completion-gate combined with --use-spec is now rejected at parse
time, mirroring the --self-correct check: the spec-draft path never
consults the completion gate, so the flag would be silently ignored.
|
||
|
beardthelionandKevin Codex
|
a880ce80a6 |
feat: gate project-scoped hooks, plugins, and MCP servers behind workspace trust (#529)
* feat(workspacetrust): add exact-match trust store for project config gating * feat(hooks): add ExcludeProject option to LoadConfig * feat(plugins): add ExcludeProject option to Load * feat(cli): gate project hooks and plugins behind workspace trust Trust check lives inside newHookDispatcherWithExtra and activatePlugins (fail-closed by construction); trustRoot is the original launch dir so a --worktree run inherits the source repo's trust. Callers OR the skip reports and emit one combined stderr notice. * feat(cli): add zero trust command (trust/list/remove) * fix(review): key spec-draft trust on the original launch dir; cover trust CLI edge branches Spec-draft runs (zero exec --use-spec --worktree) passed the reassigned worktree path as trustRoot, dropping a trusted repo's project hooks (fail-closed, but breaks worktree-inheritance). Thread the captured pre-worktree trustRoot into execSpecDraftRun. Add tests for the trust 'remove' too-many-args and --help branches, and a resolveTrust comment noting the per-chokepoint check must not be hoisted to callers. * test(cli): end-to-end trust-gate coverage through agent.Run and exec --worktree Drives the real agent loop with a fake tool-calling provider so a project beforeTool hook actually fires (or is gated away) through production code: gate blocks a tool call via agent.Run, and --worktree / --use-spec --worktree key trust on the original launch dir. All three are load-bearing (RED when the gate or the trustRoot threading regresses). * style(cli): gofmt struct alignment in exec_spec.go * fix(workspacetrust): compare stored trust entries literally, no symlink re-resolution External adversarial review (codex) found a false positive: IsTrusted re-normalized stored entries on every check (Abs+EvalSymlinks), so a trusted path later replaced by a symlink to attacker content re-resolved to the new target and matched. Stored entries are already canonical (normalized once at Trust time); compare them as literals and only normalize the incoming query. Add a regression test for the symlink-retarget case. * feat(mcp): gate project-scoped MCP servers behind workspace trust Closes the P0 an external adversarial review (codex) found: project ./.zero/config.json stdio MCP servers spawned before the hook/plugin gate, an ungated arbitrary-execution path that could also self-trust the workspace. Add ExcludeProject to config.ResolveMCP (skips the project layer) and thread a fail-closed trust check into every MCP spawn site (exec, TUI setup+refresh, mcp tools list, mcp check); reporting sites stay ungated. Verified the sole server-command exec (mcp/client.go) is reachable only through the now-gated RegisterTools paths. * test(cli): cover default-mode beforeTool firing through the trust gate Assert a project beforeTool hook fires in the default auto and ask permission modes (not only under unsafe), and is blocked when the workspace is untrusted, so the gate is verified in the mode a normal run actually uses. * docs(cli): note MCP servers in zero trust help text and doc comment The gate covers project hooks, plugins, and MCP servers, but the help string and runTrust doc comment listed only hooks and plugins. * test(cli): make workspace-trust tests portable on macOS and Windows setTrustConfigRoot set HOME on darwin, but config.UserConfigDir is XDG-first there (=$HOME/.config), so tests wrote trust.json and user hooks.json where the code never read them; set XDG_CONFIG_HOME on every non-Windows platform instead. writeMarkerHook shells out to /bin/sh, absent on Windows, so its trusted-case marker never appeared; skip it there like writeMarkerHookScript already does. TestRunTrustAddThenList compared `trust list` output (canonicalized via EvalSymlinks) against the raw cwd, which diverges on the Windows runner's 8.3 short names (RUNNER~1 -> runneradmin); compare the resolved path, and add TestRunTrustListShowsCanonicalPath to exercise that via a symlink on any platform. * feat(cli): surface the project MCP skip in the workspace-trust notice Project MCP servers were dropped silently in an untrusted workspace: the trust notice covered hooks and plugins but not MCP, and app.go discarded the resolveTrust error. registerMCPToolsForWorkspace now returns a trustSkip, gated on a new projectMCPConfigExists check plus the previously-dropped store-read error, and emitTrustNotice is variadic so the exec, spec-draft, interactive, and `mcp tools list` paths all fold in the MCP skip. The skip drives the notice only; the gate stays on resolveTrust/excludeProject, so a trusted workspace is unaffected and an untrusted one still fails closed. * fix(cli): print zero trust --help to stdout with a success exit zero trust -h/--help/help routed usage to stderr with the usage exit code, unlike every other subcommand (mcp, sandbox, skills, cron, ...) which prints help to stdout and returns success. Align it; the unknown-subcommand path still writes to stderr with the usage exit code. * fix(cli): gate oauth MCP login and surface mcp-check trust notice Two project-scoped MCP activation sites were left outside the workspace trust gate. zero mcp oauth login loaded a project-defined OAuth server from an untrusted workspace and ran the full discovery/registration/token-exchange flow against its configured URL. Gate resolveOAuthServer with resolveTrust so an untrusted clone's ./.zero/config.json OAuth server is dropped before login, and emit the same one-line trust notice the other spawn sites use. zero mcp check already gated the project layer but discarded the trust-check result and never emitted the notice, so a gated project server read as a bare "not configured". Emit the notice on the not-found branch. The three remaining ungated resolveMCPConfig(cwd, false) sites (backends doctor/snapshot and the extensions listing) are enumeration-only and stay ungated. * test(cli): adapt MCP startup-skip tests to two-arg resolveMCPConfig --------- Co-authored-by: Kevin Codex <[email protected]> |
||
|
PierrunoYTandCopilot App
|
062328b632 |
fix: warn about untracked scratch files left behind after a run (#571)
* fix: warn about untracked scratch files left behind after a run Fixes #551. Headless exec runs could leave behind debug/scratch files (e.g. _fix_test.py, _debug.py) created by write_file during iteration, which would then be silently swept up by a later git add -A. - FileTracker now records brand-new files (RecordCreated/CreatedFiles), distinct from the existing version-baseline tracking used for conflict detection. - write_file records a path as created only on first write, not on overwrite of an existing file. - After a headless run completes, exec.go checks git status for any created file still present and untracked, and surfaces a warning via the existing writer.warning() (works across text/-o json/stream-json). No files are deleted automatically — a wrong guess at 'scratch' could destroy real work, so this only surfaces what git already knows. Co-authored-by: Copilot App <[email protected]> * fix: address scratch warning review comments * test: normalize created file tracker paths * fix: track apply_patch-created scratch files * fix: address scratch warning review feedback * fix: tighten scratch warning lifecycle --------- Co-authored-by: Copilot App <[email protected]> |
||
|
euxaristia
|
041167a272 |
Fix OpenGateway credential lookup, stream error code mapping, and model catalog (#580)
* Fix OpenGateway credential lookup, stream error code mapping, and model catalog
* fix(providers): map semantic OpenAI stream error codes, not just numeric
Addresses jatmn's review (following up on CodeRabbit's original request):
the streamed error classifier only recognized numeric codes ("429", 401,
403 as string/float64/int), so semantic string codes OpenAI-compatible
providers commonly send instead (rate_limit_exceeded, insufficient_quota,
invalid_api_key) fell back to a generic 500 and lost their rate-limit/auth
classification.
Unified the numeric and semantic mappings into one
openAIStreamErrorStatusByCode table so both forms of the same condition
classify identically. insufficient_quota maps to 429, matching OpenAI's
own API behavior for quota-exceeded errors.
Added TestStreamCompletionClassifiesStreamErrorCode covering both forms.
* test(providermodelcatalog): cover the new gitlawb-opengateway catalog IDs
Addresses jatmn's review (completing CodeRabbit's original request):
TestModelsAreProviderScoped only asserted mimo-v2.5-pro and tencent/hy3
for gitlawb-opengateway, missing the other newly curated IDs added in
this PR (xiaomi/mimo-v2.5-pro, minimax/minimax-m3, qwen/qwen3.7-max,
google/gemini-3.1-flash-lite, z-ai/glm-5.2,
nvidia/nemotron-3-ultra-550b-a55b:free). A typo or accidental removal
in any of them would not have failed a test.
* test: cover the last catalog ID and probeProviderHealth's stored-key wrap
Addresses jatmn's second follow-up review on PR #580:
- TestModelsAreProviderScoped still missed xiaomi/mimo-v2.5 (distinct
from xiaomi/mimo-v2.5-pro) among the newly curated gitlawb-opengateway
catalog entries.
- fillAppDeps wraps probeProviderHealth the same way it wraps
newProvider, applying the stored credential-store key before
delegating, but only newProvider's wrap had a regression test
(TestFillAppDepsWrapsNewProviderWithStoredKey). Added the parallel
TestFillAppDepsWrapsProbeProviderHealthWithStoredKey so the health-check
path used by `zero providers check --connectivity`, `zero doctor
--connectivity`, and the TUI doctor panel can't silently regress back
to keyless probes.
|