mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 18:00:54 -10:00
feat/structural-code-query
548
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 anandh8x
|
85b1689a62 | Trim eager read tool schemas | ||
|
anandh8x
|
6a258e1876 | Merge main into feat/structural-code-query | ||
|
KRATOS
|
cae0269e77 |
fix(tui): a copied NUL byte no longer panics the TUI on Windows (#876)
syscall.StringToUTF16 PANICS rather than returning an error when the string contains a NUL — documented standard-library behaviour — and github.com/atotto/clipboard's Windows path calls it directly. So one NUL anywhere in a copied transcript selection killed the whole program: "zero: tui error: program was killed: program experienced a panic", reported from a real Windows machine. The NUL reaches the clipboard because ansi.Strip removes escape SEQUENCES and leaves C0 bytes untouched, and the transcript carries them from two independent directions: tool output (a binary read, git's -z NUL-separated listings — which this repo itself parses in files_git_sweep.go) and Zero's own card protocol, whose row prefixes are literally "\x00command-card\x00" and "\x00plan-card\x00". Sanitised at the WRITE rather than at each source, because the sources are unbounded: any tool result a user can select from is one, and a fix per source is a fix the next tool re-breaks. The same pass protects the OSC52 fallback, where a stray control byte terminates the escape sequence early and silently corrupts the copy instead of crashing — which is likely why this went unnoticed off Windows, where the identical NUL produces a bad copy rather than a dead process. \n and \t survive: a multi-line selection is the normal case and both are legal clipboard content. Fixes #875 |
||
|
Vasanth TandClaude Opus 5
|
cd0eb194e4 |
feat(tools): give tool results an image channel and add view_image (#843)
* feat(tools): give tool results an image channel and add view_image Closes #824. tools.Result carried only text, so no tool could hand the model a picture. The sharpest edge was our own capture tooling: browser_screenshot, desktop_screenshot and browser_pdf wrote a PNG, set Meta["artifact_path"] and returned "Artifact captured: <path>". Nothing read that path afterwards, so the model asked for a screenshot, was told one existed, and could not look at it. Zero was never image-blind on the way in — zeroruntime.ImageBlock, Message.Images and internal/imageinput are mature. What was missing is the model-initiated leg. Result.Images is delivered as a following USER message, not on the tool result itself. Every provider drops images on a tool-role message: Anthropic maps a tool result to a tool_result block whose content is a string, Gemini to a functionResponse, and OpenAI guards its image content-parts to the user role with an explicit comment saying so. Attaching them to the tool result would have plumbed the whole path and delivered nothing. A separate message also keeps one tool result per tool call, which the providers validate. Blocks with no data, or a media type outside the provider allow-list, are dropped rather than sent — a rejected image would fail the whole turn, and the tool's text still stands on its own. The message names the tool, since the images arrive detached from their result and nothing else says which call produced them when several ran in one turn. view_image resolves through resolveScopedReadPath before imageinput.LoadFile. That scoping is load-bearing: LoadFile joins a relative path to the workspace root and otherwise takes the path as given, so without it the tool would read anywhere on disk. It is registered with the other scoped readers and declared read-only and thread-safe, matching read_file. The capture tools now attach what they wrote. A PDF or an unreadable file yields no image and the text stands alone, so a capture is never failed over its attachment. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(agent): keep tool results contiguous when a tool returns an image The image messages are user-role, and the first version appended each one immediately after its own tool result. With two tool calls in a turn that produces tool_result, user, tool_result — and a user message between two tool results breaks strict provider replay. Anthropic makes it concrete. appendUserBlocks coalesces consecutive user content into one block list, and tool_result blocks must come first in that message, so the interleaved shape maps to: [tool_result, text, image, tool_result] which the API rejects. loop.go already documents this exact rule three lines below the change, for the self-correction feedback it defers for the same reason. Images are now collected during the tool loop and appended once every tool_result is recorded — including the aborted placeholders on the abort, stop-reason and repeated-failure paths, so an early exit cannot strand them mid-sequence either. The original test used a single tool call, which cannot exhibit the bug. The new one runs two calls where the first returns an image and asserts the image message does not fall between the first and last tool result. Also from review: view_image joins the "execute" tool category, which grants read_file but not view_image, so a specialist declared with tools: [execute] could not look at a screenshot it had just captured. Covered by a test that holds for every category granting read_file rather than naming one. And the core read-only set is now asserted by tool name, not only by count — a count still passes when one reader is swapped for another. Co-Authored-By: Claude Opus 5 <[email protected]> * test(agent): pin the tool-result/image ordering contract The assertions only tracked the first and last tool-result index, so four distinct broken orderings passed: a message wedged between the two tool results, a duplicated image flush, a duplicated tool result, and both results answering the same tool_call_id. Mutation-testing each of those against the real loop confirmed the old assertions were green on all four. Now it collects every tool-result index and id and requires exactly two, in c1-then-c2 order, at adjacent indexes, with the image following the last one and delivered exactly once. Adjacency is the assertion that carries the contract: Anthropic needs every tool_result block ahead of any other block in the user message they coalesce into, so anything between the two results is the 400 this change exists to avoid. The image is deliberately NOT pinned to exactly last+1. appendUserBlocks merges consecutive user-role content, so [tool_result, tool_result, text, image] maps to tool_result blocks first and replays fine; pinning would fail that valid shape while killing nothing the adjacency and count assertions do not already catch. messageShape reports the actual role ordering on failure, and the loop uses two ifs rather than a switch so an image left on a tool-role message is reported as the misplacement it is instead of "the image never reached the model". --------- Co-authored-by: Claude Opus 5 <[email protected]> |
||
|
Vasanth T
|
4ca4fa75d3 |
fix(agent): end the additional_permissions retry loop (#864)
* fix(agent): end the additional_permissions retry loop
Asking the agent to run an ordinary read-only command on Windows could leave it
oscillating between two rejections instead of executing:
additional_permissions requires sandbox_permissions set to "with_additional_permissions"
additional_permissions must include at least one of network or file_system, ...
Both come from inlineAdditionalPermissionsProfile, and each names only the fix
that ADDS something. The first pushes the model toward setting the flag; the
second pushes it back toward requesting a permission the command does not need.
Neither ever says to omit the field, which is the correct action for a read-only
command, so the two messages form a two-state oscillator. Every rejection now
carries that remedy.
The schema half is why the object is attached at all. Neither field description
said to omit them for ordinary commands, so a model that legitimately used
with_additional_permissions once in a session had nothing telling it to stop
attaching a permission object to the next command. Both descriptions now lead
with "omit", and they are shared constants rather than duplicated verbatim in
bash.go and exec_command.go, where they were free to drift.
Rejection is still rejection. Nothing that was refused is now accepted on the
strength of an empty or mismatched payload, and sandbox_permissions is never
upgraded on the model's behalf; only the guidance changed.
The one behavioural change is that an explicit additional_permissions: null is
now treated as the omitted field, since a null grants nothing and carries no
flag. That flips three shapes from reject to execute: null with use_default,
with require_escalated, and with no mode at all. The escalated one still routes
through the normal escalation prompt and is not a new capability — the identical
request without the null key already behaved that way. A FLAGGED null still
errors, so with_additional_permissions plus null is not silently downgraded.
Fixes #845. Fixes #847.
* fix(tools): condition the omit guidance on needed access, not read-only
The description said to omit sandbox_permissions "for ordinary commands,
including every read-only one". That overstates it: curl and gh are read-only
and still need sandboxed network, so a model following it literally would omit
the override on a command that genuinely needs one and hit the opposite failure.
It now says to omit unless the command needs access the sandbox does not already
give it. That is the claim that is actually true, and it is one a model can act
on without first deciding what counts as read-only.
The schema test asserted the wrong half too — a bare substring check for
"read-only" pinned the overstatement rather than the contract. It now pins the
conditional, with the reasoning recorded so the weaker assertion does not come
back.
Reported by CodeRabbit on #864.
|
||
|
Anandan
|
edf660ab85 |
perf: compact common test output (#846)
* perf: compact common test output * test: strengthen command reducer guarantees * fix: tighten command output summary detection |
||
|
anandh8x
|
e3337d06d5 | Prevent overflow in ranged minified reads | ||
|
anandh8x
|
3aa190bd7f | Fix bounded source read edge cases | ||
|
anandh8x
|
00ad4d4b8d | Preserve lexical context in ranged reads | ||
|
anandh8x
|
a394fd60ee | Improve source read efficiency and compatibility | ||
|
Vasanth T
|
91b413c514 |
fix(sandbox): stop the Windows write jail honouring Everyone-granted paths (#865)
* fix(sandbox): stop the Windows write jail honouring Everyone-granted paths The restricted token put the World SID (S-1-1-0, Everyone) in its restricted-SID list unconditionally, from the original sandbox baseline and with no comment saying why. A WRITE_RESTRICTED token runs two checks for a write and needs both: the normal one against its enabled SIDs, and a second against the restricted list. The write jail rests entirely on that second check only succeeding where Zero has explicitly ACL'd a capability SID. Everyone is a SID every principal carries, so on any path whose DACL grants Everyone write, the restricted half passed for free and confinement fell back to the ordinary user's own permissions -- the exact boundary this token exists to be stricter than. Reachable with no privilege, no symlink and no race: an Everyone-writable directory is enough, and share roots opened with Everyone:F and loose third-party installer ACLs supply them. Ruled out as the common case: C:\Users\Public\Documents grants BATCH, not Everyone. The runner already stated the rule this broke -- it declines to add the user SID, Administrators or SYSTEM because "each has write access nearly everywhere" -- while listing Everyone among the restricted SIDs. Removed only from the WRITE_RESTRICTED token, because without that flag it is load-bearing rather than gratuitous. The restricted-SID check then applies to reads too, and default Windows DACLs grant BUILTIN\Users rather than anything in this list, so a token with no Everyone cannot open cmd.exe and dies at launch with STATUS_ACCESS_DENIED (0xC0000022). Dropping it there was tried first and takes TestWindowsUnelevatedRealSandboxSmoke with it. That path is taken only when a profile configures DenyRead, which is already the posture trading capability for read-deny enforcement (#612), so the bypass survives there deliberately and narrowly; closing it needs a read-side grant that is not a universal group. Verified against real tokens rather than reasoned about. The new smoke test drives the real runner and asserts three outcomes that only agree when the jail is intact: a granted root is writable, an ordinary outside path is denied, and an Everyone-writable outside path is denied too. Before the change its third case wrote the file with exit 0; the first two were already correct, which is what made the gap invisible. Restoring the SID unconditionally turns it red again. * test(sandbox): make a denied write distinguishable from a runner failure The denial assertions expected exit 1, which the runner also returns for its own errors -- a failed marker validation, a bad argument, a token it could not build. Written that way the test passes both when the sandbox denies the write and when nothing ever ran, and the second case proves nothing while being indistinguishable from success. That is a poor property for any test and a bad one for this test, whose entire job is to notice a confinement regression. The leading "granted root is writable" assertion covered part of it, but said nothing about whether the two later commands launched. The denial commands now report a distinctive exit code the runner cannot produce for itself, so the code proves cmd.exe actually ran and its redirect was refused. Re-checked in both directions rather than assumed: the test passes against the fix, and restoring the World SID unconditionally fails it with "exit code = 0, want 77" -- still catching the bug it was written for. Raised by CodeRabbit on #865. |
||
|
Vasanth T
|
021281ebba |
fix(cli): report MCP servers that failed to start (#822) (#827)
* fix(cli): report MCP servers that failed to start (#822) `zero mcp check` inspected only the error returned by registration and then printed "is reachable". RegisterTools is best-effort by design, so a server that fails to connect is recorded on the runtime and registration returns nil. That is right for startup, where one bad server must not stop Zero launching, and wrong here: this is the command a user runs when a server is misbehaving, and it told them the broken server was fine. The failure data already existed as SkippedServer{Name, Err}; nothing consumed it in this path. The check now fails with the recorded error, in both text and JSON output. Covered by a test that spawns a genuinely missing command rather than stubbing the runtime, because the defect was precisely a mismatch between what registration returns and what it records. Reverting the fix makes it print "MCP server broken is reachable. 0 tool(s) available." * test(cli): cover the unreachable --json contract The text path was covered and the JSON path was not, though the change adds its own return with a different payload: status "unreachable" plus a redacted error, rather than status "ok" with a tool count. A caller parsing that shape was relying on something nothing pinned. Reverting the fix now fails both cases. The JSON one is the more useful of the two, because the wrong output is still valid JSON and would decode cleanly into a caller expecting success. |
||
|
|
8e266797a8 |
perf: improve harness context efficiency (#838)
* Improve harness context efficiency
* Address harness efficiency review findings
* Make recaps idle and cancellable
Delay recap generation until the session has been inactive, orient it around the active goal and next task, cancel stale work on activity, and render the result ephemerally above the composer.
Also strengthen the formatter safety regression to exercise formatter-modified edit output.
* Track every replace-all edit span
Translate each replacement into its post-edit line range, including cumulative shifts from earlier replacements, so follow-up edits remain authorized for every model-supplied change.
* Make review regressions cross-platform
Verify replace-all tracking through the canonical edit tool path instead of a platform-sensitive temp path, and cover cancellation when recaps are disabled during generation.
* fix(tools): derive SeenWhole from the recorded ranges
SeenWhole was a flag set only by a SINGLE read covering the file, while
SeenRange judged coverage from the accumulated ranges. The two then
disagreed about identical reads: after reading lines 1-50 and 51-100 of a
100-line file, SeenRange(1,100) was true and SeenWhole was false.
That matters because write_file gates overwrites on SeenWhole. A file past
the read byte budget cannot be read whole in one call — the read is
truncated and RecordSeenRange is correctly skipped, since the model did not
see those lines — so no sequence of reads could ever set the flag, and the
overwrite was refused permanently with:
the intended change depends on content that has not been read exactly
in this session. Read the affected lines with read_file, then retry.
Advice that could not change the answer. edit_file was unaffected because it
gates on SeenRange, so the escape was to edit rather than rewrite, but a
model following the error text would loop.
SeenWhole is now computed from the ranges against the file's recorded line
count, so scoped reads that together cover a file make it writable and the
error text becomes actionable. The observation carries that line count, and
a read reporting a different one clears the accumulated ranges: they
describe a shape the file no longer has.
Coverage is judged by merging sorted ranges rather than walking every line
against every range, which the previous SeenRange did — same answer, and it
no longer scans line-by-line over a large file.
Co-Authored-By: Claude Opus 5 <[email protected]>
* test(tools): key the large-file test on the resolved path
read_file resolves the workspace root through EvalSymlinks before recording
anything, so the tracker is keyed on the resolved path. The test asserted
against the raw t.TempDir() path, which is the same string on Windows and a
different one on macOS, where the temp dir sits under /var — a symlink to
/private/var. The SeenWhole assertions would have looked up a key that never
existed: passing here, failing on macOS CI.
Also switches the line generator to fmt.Fprintf (staticcheck QF1012).
Both from CodeRabbit on #842.
Co-Authored-By: Claude Opus 5 <[email protected]>
* test(tools): actually switch the line generator to fmt.Fprintf
The previous commit's message claimed this was done. It was not: the edit
silently did not apply and I reported it fixed without re-reading the file,
so builder.WriteString(fmt.Sprintf(...)) survived at the head — caught by
@anandh8x, not by me.
Verified this time by running the linter rather than by reading the diff:
golangci-lint --enable-only unused,ineffassign,staticcheck ./internal/tools/
reports 0 issues.
* fix: keep exact-read workflows recoverable
* test: cover permission normalization wiring
* fix: commit file observations after output budgeting
* fix: defer file observations across retries
* test: normalize patch line endings
---------
Co-authored-by: Vasanthdev2004 <[email protected]>
Co-authored-by: Claude Opus 5 <[email protected]>
|
||
|
Anandan
|
d37de9214b |
fix: use OAuth credentials in provider health checks (#828)
* fix: use OAuth credentials in provider health checks * fix(providerhealth): verify OAuth login for all probes * fix(providerhealth): retain API key redaction * fix(providerhealth): redact bare OAuth tokens * test(cli): isolate OAuth doctor config * test(cli): isolate doctor config path |
||
|
Anandan
|
588635487f |
feat(tui): replace /retitle with local /rename (#826)
* Improve session naming and resume picker * Address session rename review feedback |
||
|
Anandan
|
845df6c7d2 |
fix(tui): hide unavailable sidebar shortcut (#821)
* fix(tui): hide unavailable sidebar shortcut * test(tui): cover medium sidebar footer hint |
||
|
Anandan
|
15eb16f736 |
fix(tui): exclude permission waits from turn time (#820)
* fix(tui): exclude permission waits from turn time * fix(tui): exclude permission waits from TTFT |
||
|
|
15b7a5475c |
fix(acp): interrupt idle reads on cancellation (#782)
* fix(acp): interrupt idle reads on cancellation * fix(acp): make idle-read interruption per-Read, not per-Serve-call Addresses the P2 on #782. The previous design raced a single, whole-connection Once (endRead) between the read loop's own "I have a terminal result" claim and a watcher goroutine's "ctx is done, treat this as interrupted" claim. This has a real, deterministic failure mode that has nothing to do with goroutine-scheduling luck: bufio.Reader can obtain data AND a terminal error from ONE underlying Read call (io.Reader explicitly permits returning (n>0, err) together). If a line delimiter is found within that data, ReadBytes returns the line with a nil error for that call, but bufio caches the error internally and returns it — without ever touching the underlying reader again — on the very next ReadBytes call. If cancellation happens in the gap between that first, successful read and the next one (e.g. while a synchronously-dispatched handler is still writing its response), the watcher's ctx.Done() case fires and claims the Once BEFORE the read loop ever gets back to a ReadBytes call — so when the cached error then surfaces (with zero contact with the underlying reader, let alone anything closing it), it is misattributed as a cancellation-driven interruption and silently swallowed as a clean shutdown, hiding a genuine broken transport. The fix moves interruption-detection down to the Read call itself instead of bracketing the whole Serve loop. interruptibleReader wraps the connection's reader: each Read runs the underlying call on its own goroutine and races it against ctx.Done(). If the underlying call finishes first, its result is returned untouched — cancellation a moment later has no bearing on a call that already had its answer. If ctx.Done() fires first, the reader is closed (when closable) to nudge a genuinely blocked read, a generation counter is bumped, and the call still waits for and returns the underlying call's own result rather than a synthesized one. Serve then brackets each ReadBytes call (which may invoke the wrapped Read zero or more times — zero when bufio answers from its own buffer, including a previously-cached error) with a snapshot of that counter before and after. A read whose bracket shows no change was never raced against cancellation — either answered from the buffer or genuinely completed before ctx.Done() could matter — so its outcome is reported truthfully regardless of the ctx state by the time it surfaces. Only a bracket that changed proves this specific call actually took the cancellation branch. Verified the fix has teeth: reverted to the original implementation and confirmed the new regression test (which forces the exact bufio-caching sequence deterministically, with a panic guard proving the underlying reader is never called a second time) fails 100% of the time against it, then restored the fix and confirmed it passes. Also applied the two open review threads on the existing tests: readStarted and writeStarted are now buffered channels with non-blocking sends rather than close(), so a future change causing an extra read or write can't panic on a double-close. Validated with go build/vet/test on Windows, plus go vet/build and `go test -race ./internal/acp/... -count=50` (and count=100 focused on the cancellation/error tests) under WSL/Linux — this session's Windows host has no C toolchain for -race directly. * fix(acp): preserve decided read results and require explicit reader ownership Addresses jatmn's two #782 review findings: - interruptibleReader.Read's select between resultCh and ctx.Done() chooses pseudo-randomly when both are ready, so a real terminal read error decided just before cancellation could be misattributed to shutdown and silently swallowed. Re-check resultCh non-blockingly before claiming a call for cancellation. - NewConn inferred reader ownership from an io.Closer assertion and closed any reader that happened to implement it, including one a caller merely borrowed. Split into NewConn (never closes r) and NewOwnedConn (explicit ownership, closes r on cancellation), and switch the sole production call site (process stdin) to the latter. Also applies CodeRabbit's atomic.Int64 and leading-ctx-parameter nits. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(acp): synchronize read completion with cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-9e4e-7506-85ab-e151888bfd71 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): preserve read completion across cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): bound buffered error synchronization Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): report shutdown transport errors Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): inject cancellation context Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
|
bdbda89f3e |
fix(swarm): close lifecycle admission on shutdown (#776)
* fix(swarm): close lifecycle admission on shutdown * fix(swarm): admit lifecycle work without holding the lock across Launch Addresses the P1 on #776 plus the two documentation P3s. [P1] beginLifecycleAdmission returned with lifecycleMu.RLock() held for the caller's entire duration — including the call into the injected MemberLauncher.Launch. Close needs the write lock before it can call s.cancel(), so a launcher that blocks in Launch waiting on its context (the ordinary shape of a launcher that waits for a slot, a daemon connection, or a sandbox handshake) deadlocked every Close caller: the launch held the read lock while Close waited for the write lock to reach the very cancellation the launch needed to unblock. beginLifecycleAdmission is now a counted ticket: it holds lifecycleMu only across the closed check and a lifecycleWork.Add(1), then returns a release func (lifecycleWork.Done) the caller defers. Close flips closed and releases the lock BEFORE calling cancel, then waits out lifecycleWork (admitted-but-unfinished work) followed by watchers (goroutines admitted work may still be starting) — so it remains a true completion barrier without holding any lock across external launcher code. Every call site (Spawn, Handoff, AdoptOrphans, the retry path in watch, and afterExit) moved to the ticket pattern. [P3] beginLifecycleAdmission's contract and the Scheduler/Close handoff are now both documented at the point that matters: the func-returning signature makes the release contract explicit in the type itself, and Scheduler's comment spells out why the read/write lock ordering guarantees exactly one of Scheduler or Close closes whatever scheduler ends up installed. Tests: TestCloseCancelsLauncherBlockedInLaunch reproduces the exact deadlock (spawn parked in Launch, Close in a second goroutine, both must complete once cancellation reaches the launcher) — verified it hangs under the old held-lock pattern by temporarily restoring it. TestCloseWaitsForAdmittedSpawn is the converse: Close must not return while an admitted spawn is still running. Validated with `go test -race ./internal/swarm/... -count=1` under WSL/Linux (this session's Windows host has no C toolchain for -race directly), plus `go vet ./...` under WSL and go build/vet/test on Windows. * fix(swarm): fail late queue admissions and dequeues after shutdown Addresses jatmn's two #776 review findings: - Close swept (clearQueue) every team's queue before waiting for admitted-but-in-flight lifecycle operations to finish. A Spawn, Handoff, or AdoptOrphans call that got its ticket right before closed flipped could still append to a team's queue after the sweep ran, stranding that spec forever (never launched, since shutdown began; never failed, since the sweep won't run again). Reorder Close to wait for lifecycleWork before sweeping queues, so every queue append from already-admitted work is visible to the sweep. - afterExitAdmitted dequeued a queued spec (t.onExit) and launched it unconditionally once its own ticket was granted, even though Close can flip closed in the gap between that dequeue (which makes the spec invisible to the clearQueue sweep) and the launch. Re-check closed after dequeuing so a spec pulled off the queue after shutdown began fails with ErrSwarmClosed instead of starting a new worker. Both regression tests are verified to fail against the pre-fix code (confirmed by temporarily reverting each fix and rerunning). Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(swarm): snapshot teams after lifecycle work stops Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-a4d5-7078-a80b-802bab23607e Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): reject late admitted launches during shutdown Re-check the closed state at the final launcher boundary so lifecycle work admitted before Close cannot start or retry a member after shutdown has already won the race. Add deterministic late-dispatch and scheduler-handoff coverage, stress concurrent Spawn and Close under the race detector, and keep the late-team snapshot regression isolated from launcher behavior. Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-4f7f-766e-a312-a7db232b9987 Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): reject post-close launch commits Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): gate retry relaunch on the post-close commit The post-Launch shutdown gate added for spawns and queue drains was missing from the bounded-retry path, which called the launcher and swapped in the new handle unguarded. Since FuncLauncher returns a live handle immediately without consulting its context, a Close landing between the pre-check and Launch's return left the watcher supervising a member relaunched after shutdown began — and recording that member's outcome on the task. Extract the commit gate as commitLaunch and route the relaunch through it via relaunchAdmitted, so an initial launch and a retry pass the same three gates: admission, the pre-Launch re-check, and the post-Launch commit. When shutdown wins, the handle is reaped and the watcher records the terminal failure instead of adopting it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
Vasanth T
|
8acd42a3bd |
fix(providers): retry provably pre-send transport failures (#447) (#750)
* fix(providers): retry provably pre-send transport failures (#447) A flaky link that failed the streaming completion POST at connect time stopped the turn and forced a manual continue, because the shared retry layer never retries transport errors: a completion POST is non-idempotent, so a replay could duplicate billable work. That rationale does not apply to a PROVABLY pre-send failure, where no request bytes ever left this host: a replay cannot duplicate anything, exactly like a 429/503 where the server told us it did not accept the request. SendWithRetry now retries, with the existing backoff, only that narrow class: DNS resolution failure, a refused or unreachable dial, and a TLS handshake timeout. Every ambiguous or post-send failure stays unretried and is checked first so a mixed message can never be misread: connection reset, broken pipe, EOF, a bare i/o timeout, context deadline. All three providers route their completion POST through this layer, so one change covers them uniformly. This deliberately narrows the issue's ask (which listed connection reset) to the provably-safe subset, and keeps the reset-stays-unretried test green. Tests cover the retry-on-pre-send, no-retry-on-ambiguous, and predicate-classification paths. NOTE FOR REVIEW: this refines a deliberately conservative, documented no-retry stance. Opening it for the owner's sign-off on the stance, not as a settled call. * fix(providers): classify pre-send dials by errno so Windows retries too CodeRabbit flagged that the pre-send predicate matched only POSIX dial wording, so a refused or unreachable dial on Windows (whose text differs entirely: 'No connection could be made because the target machine actively refused it') would not be retried. Match the syscall errno via errors.Is instead: Go maps ECONNREFUSED / ENETUNREACH / EHOSTUNREACH / ECONNRESET to the host's real values, including the WSA codes on Windows, so classification is identical on every platform. Errno matching is also authoritative where the string markers were only heuristic, closing the theoretical substring gap. The string markers stay as a fallback for errors already flattened to text. ECONNRESET is added to the post-send exclusions by identity so a Windows reset is excluded too. Tests build the production error shape (net.OpError -> os.SyscallError -> errno) so the Windows path is exercised portably on every platform, plus an end-to-end SendWithRetry case for an errno-wrapped refused dial. Cross -compiled and vetted for linux and darwin. * fix(providers): scope pre-send retry to the dial phase and fail fast on permanent errors Addresses gnanam's two [Major] review findings on this PR. [Major] Pre-send errno classification was not scoped to the dial, so a post-send read/write carrying ECONNREFUSED/ENETUNREACH/EHOSTUNREACH (a route dropping mid-generation) was misclassified "provably pre-send" and could replay a non-idempotent completion POST up to 6 times. errors.Is matches an errno anywhere in the chain regardless of which operation raised it, so isPreSendTransportError now requires an Op=="dial" *net.OpError before admitting those errnos or their POSIX wording. That makes "no request bytes were sent" structural: a post-send failure can no longer replay a POST. The dial errno match still covers Windows (Go maps the WSA* codes), so the platform coverage this PR added is preserved. [Major] Permanent failures hung the agent ~60s on the 429-tuned schedule. NXDOMAIN (net.DNSError with IsNotFound) is now treated as permanent and not retried, and the pre-send path gets its own short bound and sub-second schedule (preSendMaxAttempts=3, 500ms/1s) instead of reusing the 2/4/8/16/30s rate-limit backoff, so a mistyped host or a dead local daemon fails in about 1.5s. [Minor] Fixed the stale SendWithRetry doc that still claimed transport errors are never replayed. Tests: post-send read/write errnos and NXDOMAIN now assert exactly 1 call (no replay); dial errnos plus the string fallback assert the pre-send cap; added the pre-send backoff schedule. Race detector and all provider packages green. * fix(providers): match Windows WSA dial errnos so refused dials retry on Windows too CodeRabbit caught a real Windows bug, which I verified with a real dial on Windows: errors.Is(err, syscall.ECONNREFUSED) returns FALSE for a refused dial there, because a real Windows dial carries the raw WSAECONNREFUSED (10061) while Go's syscall.ECONNREFUSED is a distinct value the net package never produces. The POSIX string fallback ("connection refused") does not match the Windows wording ("No connection could be made because the target machine actively refused it") either. So the pre-send dial classification silently never fired on Windows, the exact platform this PR set out to fix. Move the dial errnos into a platform-specific list (dialPreSendErrnos): POSIX everywhere, plus the WSA* codes on Windows. The existing wrapDialErrno fixtures wrap the Go syscall constants, which is why they passed on both platforms and missed this. Adds TestIsPreSendTransportErrorRealRefusedDial, which does a REAL refused dial and asserts pre-send classification, so the platform's true error shape is exercised on every OS. * fix(providers): exclude Plan 9 from the POSIX dial-errno build tag * fix(providers): keep the pre-send errno classification off Plan 9 retry.go referenced syscall.ECONNRESET and dialPreSendErrnos directly, but the errno definitions live in build-tagged files that exclude Plan 9 (which has no BSD-style errno constants), so the package stopped compiling for GOOS=plan9 on this branch even though it builds on the merge base. Move the two syscall touch-points behind per-platform helpers: retry.go now calls isConnResetErrno and ranges dialPreSendErrnos, both provided by retry_dialerrno.go, retry_dialerrno_windows.go, and a new retry_dialerrno_plan9.go. The Plan 9 variant supplies an empty errno list and a false isConnResetErrno, so retry.go falls back to its existing string-marker heuristics there. Behavior on linux/darwin/windows is unchanged (isConnResetErrno matches the same syscall.ECONNRESET as before). retry_test.go is tagged !plan9 because it constructs real syscall.Errno dial shapes. Verified: GOOS=plan9 build and vet now pass; windows and linux build, vet, gofmt, and the full providerio test suite stay green. * fix(providers): stop pre-send retry from replaying a redirected POST Addresses jatmn's review on #750. [P1] client.Do could follow a 307/308 after already transmitting the original POST, and a dial failure to the redirect target then satisfied isPreSendTransportError and replayed the request, re-billing a completion the first host already received. SendWithRetry now disables redirect following (CheckRedirect returns http.ErrUseLastResponse on a cloned client), so any dial/DNS/TLS error from Do can only come from the single initial request and the pre-send classification is sound. A 3xx surfaces as the response for the caller's existing non-2xx handling. Regression test asserts the POST is sent once and the 307 is not followed. [P2] The pre-send retry used the loop-wide attempt counter shared with 429/503 retries, so a couple of status retries exhausted the pre-send budget and the first dial/DNS/TLS blip after them got no retry. Track pre-send failures and their backoff ordinal in a separate counter. Regression test covers a status-then-transport-failure sequence. [P3] Correct the Plan 9 and wrapDialErrno comments: the Plan 9 string-marker fallback keeps the pre-send inclusions and the separate reset exclusion, and the errno fixtures do not reproduce a real Windows dial (that carries distinct WSA errnos), which the real-dial test covers. * fix(providers): decouple the status retry budget from pre-send retries Addresses jatmn's follow-up on #750. The previous change gave pre-send failures their own counter, but every retry still advanced the loop-wide attempt counter that the 429/503 branch used. So the coupling survived in the opposite direction: two safely replayed refused dials shortened the later status retry window and started its backoff at a later rung. Give the status path its own statusAttempts counter and backoff ordinal. With both budgets tracked separately the shared loop counter has no remaining reader, so the loop no longer carries one. Behaviour is unchanged when only one kind of failure occurs: the first retryable status still waits on rung 1 and still gets maxAttempts requests. Adds TestSendWithRetryStatusBudgetSurvivesPreSendRetries, the inverse of the existing mixed-sequence test: two refused dials followed by rate limiting must still leave the status path its full window. * fix(providers): keep following redirects, but never replay a redirected POST Addresses jatmn's follow-up on #750. Blocking redirects outright was too blunt. SendWithRetry is the shared completion path, so replacing every caller's CheckRedirect with ErrUseLastResponse turned a legitimate 307/308 from a redirecting OpenAI-compatible gateway into a terminal non-2xx that providers report as an upstream error, and it discarded an injected client's redirect policy. Observe the redirect instead of preventing it: a per-attempt flag is set from CheckRedirect, which then delegates to the caller's policy when one is set and otherwise mirrors net/http's default ten-hop cap. The pre-send retry is gated on that flag, so an attempt that entered a redirect is never replayed while redirecting endpoints keep working. The safety property is unchanged, since a dial failure on a redirect hop follows a POST the origin already received. Adds three regressions: a redirect is followed through to the final response, a dial failure after a redirect is not replayed, and a caller-supplied CheckRedirect still runs and still decides. * test(providerio): pin the redirect asymmetry for retryable statuses A reviewer read the redirect guard as incomplete, because it stops a redirected POST being replayed after a transport error but not after a 429/503/529. The asymmetry is intentional, and nothing recorded that, so the next reader would reasonably reach the same conclusion. The two cases differ in what is known about the request. A transport failure after a redirect leaves it unknown whether the redirect target received and began processing the POST, so replaying could duplicate billable work. A retryable status means every hop answered: the 307/308 is the origin declining and delegating, and the 429 is the target declining as well, so nothing accepted the request and a replay duplicates nothing. The exposure is also the same as a retryable status with no redirect involved, which this package has always retried, so guarding here would not close a gap. It would make a rate-limited completion behind a redirecting gateway fail outright rather than back off, which is the regression class already raised against disabling redirects wholesale. Adds the case as a test asserting the intended behaviour, plus a comment at the branch explaining why it is not guarded. * fix(providers): retry connect timeouts and proxy CONNECT failures Three classification gaps against #447, all raised in review. A connect-phase timeout was never retried. It is provably pre-send for the same structural reason a refused dial is, since the connection was never established and no request bytes left this host, but it arrives carrying "i/o timeout" and the blanket wording exclusion rejected it before the connect gate could admit it. Intermittent connect timeouts are exactly the transient failure the issue is about, so the most common case was the one not covered. It is now admitted ahead of the wording exclusions, gated on the connect phase so a post-send read timeout, which is also Timeout(), still cannot replay a POST. A failed CONNECT through an HTTP proxy was never retried either. net tags it Op "proxyconnect" rather than "dial", so it missed the gate entirely. Same guarantee applies: the tunnel to the model host was never built. The gate now accepts both, which matters most in the environment where transient connect failures are most common. ETIMEDOUT and WSAETIMEDOUT join the errno lists so a connect timeout still classifies if it reaches us with the errno but without the timeout flag. The pre-send budget also now resets after a hop that reaches the server. It is meant to bound consecutive connect failures, not the lifetime of the call, and without the reset an early run of blips permanently spent it, so a later blip surfaced as a transport error while the status budget still had room. That is the coupling the two counters were split apart to remove, in the one ordering the earlier tests missed. Total work stays bounded, since every reset costs a status attempt and those are capped. Also corrects the SendWithAuthRetry comment, which still claimed the inner path retried only 429/503/529. Tests cover dial and proxyconnect timeouts as retryable and read/write timeouts as not, a refused proxy CONNECT, and the interleaved pre-send/status/pre-send sequence. Each fix fails its test when reverted. |
||
|
Vasanth T
|
1c38e8cba5 |
fix(config): make the provider-command timeout an upper bound (#811) (#813)
providerCommandTimeout bounded only the wait, so the call could take arbitrarily longer than the 5s its error text promised. Two phases sat outside it. Starting the process ran before the clock began. On Windows that is CREATE_SUSPENDED, job-object creation and assignment, and a system-wide thread snapshot, none of it fast on a cold or contended machine. The deadline is now armed first and the start races it, so process creation is inside the budget. Draining after termination was unbounded too: the timer path called Terminate and then received on the wait with no limit. Terminate normally makes that immediate, which is exactly why it survived unnoticed, but a tree that resists termination held the call open long past the deadline that had just expired. That receive is now bounded, and a call that gives up leaves cmd.Wait running, so the output buffers are synchronised: the I/O pump keeps writing into buffers the caller already holds, which a plain bytes.Buffer cannot survive. The timeout error also carried nothing actionable. It was a bare string with the underlying error and the command's stderr both dropped, so a user whose provider command was slow got no clue which phase ran out or what the command had said. It now names the phase, wraps the cause, and includes the redacted output the failure path already had. Both bounds are asserted on extracted helpers rather than end to end, because end to end they are untestable: a terminated command stops writing and its wait returns at once, so an unbounded drain and an unsynchronised buffer both pass a whole-call test. The first version of these tests did exactly that and proved nothing. Removing either bound now fails, as does arming the deadline after the start, which reports the wrong phase. |
||
|
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
|
3e7692d8f0 |
fix(sandbox): scrub the daemon token file pointer (#677) (#818)
scrubSensitiveEnv removed ZERO_DAEMON_REMOTE_TOKEN but not its file counterpart. TokenFromEnv accepts either form, so a sandboxed command still inherited the path to the bridge token, and the default posture is read-all, so it could then read the token itself. Scrubbing the pointer is the whole fix rather than half of one: the path comes from this variable alone and TokenFromEnv has no default location to fall back on, so a command that never sees the variable has nothing to guess at. This mirrors GOOGLE_APPLICATION_CREDENTIALS, which is already scrubbed for the same reason. Extends the existing scrub table, which compares the surviving environment exactly, so the case fails if the entry is dropped. Verified: without the fix the assertion reports ZERO_DAEMON_REMOTE_TOKEN_FILE surviving into the scrubbed environment. |
||
|
Vasanth T
|
33c94ec7a3 |
fix(sandbox): deny reads of git's credential stores (#815) (#816)
* fix(sandbox): deny reads of git's credential stores (#815) git's credential-store backend keeps host passwords and personal access tokens in cleartext, at ~/.git-credentials or ~/.config/git/credentials depending on whether the user is on the XDG layout. Neither was on the deny list, so any command the agent runs could read them. Scoped to those two files deliberately. #815 also covers SSH private keys and the GPG keyring, and those are a harder trade: denying ~/.ssh stops a sandboxed git push over SSH from working, and excluding that directory is not even sufficient, since an IdentityFile directive can point key material anywhere. These two cost nothing by comparison, because git reads them to authenticate rather than to know who the user is, so a sandboxed git keeps working and simply cannot act as the user. The XDG one denies the file rather than the directory around it, which is the opposite of the choice made for ~/.config/zero and is deliberate: ~/.config/git also holds the global git config, which userGitConfigReadPaths grants on purpose so a sandboxed git can read user.name and aliases instead of failing outright. Denying the directory would take that away. Tests assert both stores are denied, that the git config beside the XDG one stays readable, and that an explicit read grant still re-includes them like every other entry. Dropping either entry fails the first; widening the XDG entry to the directory fails the config assertion. * test(sandbox): assert the credential deny exists before the grant lifts it The allow-read case only checked that the credential store was absent from the deny list once an explicit grant covered it. A deny rule that was never added satisfies that just as well, so the test passed with the production change reverted and proved nothing about the interaction it was named for. Check both runs: denied with no grant, absent with one. The first leg is what establishes there is something for the grant to override. |
||
|
Vasanth T
|
c9f9dc02ef |
test(tools): stop internal/tools failing on stock Windows (#814)
* test(tools): stop internal/tools failing on stock Windows This package fails on every run on a Windows box without PowerShell 7, which is what a stock install is: PowerShell 7 is a separate download. CI never sees any of it, because the runners have 7. Three distinct causes, all surfaced by the move to PowerShell in #804. The stop-intent test waited a fixed second for a session to die and asserted on whatever was true by then. Under cmd.exe that was enough; under PowerShell the tree is slower to stand up and tear down, and the FIRST stop in a fresh process pays the interpreter's cold start, so it routinely returned with termination under way but unfinished. The assertion read that as a failed stop. It now re-sends the stop until the session reports it has exited. Repeating is safe, since Terminate is a no-op against an already-dead tree, and it keeps the interrupt flag that a poll with empty chars would drop. Separately, and pre-existing, every test that starts a session hit a teardown race: a terminated process does not release its handles the instant the session reports exited, and Windows refuses to remove a directory with an open handle where POSIX does not. t.TempDir removes once and fails the test on the sharing violation, so ordinary teardown timing was reported as a broken test. Those roots now clean up with a bounded retry, and give up quietly rather than failing: the process is already terminated, so the worst case is a stale directory under the OS temp root. Last, the MSYS guard test asserted a windows_shell_syntax block using a command containing `&&`. On PowerShell 5.1 the windows_powershell_version preflight added by #804 fires first and shadows it. The `&&` was incidental to what that case is about, so it is gone. This one is worth noting as a shape rather than a typo: a new preflight silently changed which block an older assertion got, and only on the shell CI does not run. Verified by running the package repeatedly rather than once: previously failing on every run, now passing 3 of 3, and the stop-intent test passing 8 of 8 alone and 6 of 6 under CPU contention. Disabling the terminate path still fails it, so the assertion continues to bind. * test(tools): assert the stop intent killed the session The retry loop added to survive a slow PowerShell teardown also made the test unfalsifiable. The helper sleeps 5s, well inside the 30s deadline, so a terminate that did nothing at all would let the sleep end on its own and every existing assertion would still hold: the session reports exited, it reports an exit code, and the interrupted flag echoes the request rather than the outcome. Neutering KillProcessTree confirmed it, the test passed. Assert instead that the helper's completion line never appears, which distinguishes a killed session from a finished one without depending on how long the loop took. Output is accumulated across every poll rather than read off the last result, because each continuation returns only what the session produced since the previous collection. |
||
|
Vasanth T
|
ec3b822505 |
fix(tui): paste clipboard images with Ctrl+V (#534) (#807)
* fix(tui): paste clipboard images with Ctrl+V Ctrl+V did nothing when the clipboard held a screenshot, while right-click paste attached it. The image plumbing was already there: routePaste probes the OS clipboard for an image whenever a paste arrives with empty text. The gap is that a clipboard holding only an image produces no bracketed paste at all, since the terminal has no text to send, so routePaste never runs and its probe never fires. Right-click reached the probe only because that path always delivers a clipboardReadMsg, empty or not. Bubble's own Ctrl+V binding stays disabled, so nothing else was listening for the key. Handle Ctrl+V by issuing the image probe directly. It reads image content only, never text, so bracketed paste remains the single text path and a Ctrl+V with text on the clipboard is unaffected. The probe yields no message when there is no image, keeping it a silent no-op, and it is gated on noBlockingModal so a permission prompt or picker still swallows the key. readClipboardImage becomes a var so the tests stub it instead of depending on the machine's real clipboard. Fixes #534 * fix(tui): also probe the clipboard on Cmd+V, and test the real key route Two things raised in review, both fair. The handler matched keyCtrl only, which is ModCtrl. macOS reports Command as ModSuper, as the rest of the TUI already models it, so Cmd+V never reached the image probe even though the change claimed to cover it. It now matches both, via a keySuper helper alongside the existing modifier helpers. Worth being precise about the limit: this only helps where the terminal delivers the key to the application. A terminal that handles Cmd+V itself pastes the clipboard TEXT and sends no key event at all, so an image-only clipboard still produces nothing for this to react to. That is a separate problem from the one being fixed here. The tests also did not exercise the route they were named for. One asserted that Update returned some non-nil command and then ran a freshly built probe; the other never called Update at all. Both now stub the reader BEFORE Update and run the command Update actually returned. That matters more than it sounds. Making the branch return a different non-nil command leaves the old assertion passing and fails the new one, which is exactly the hole: the old test proved a command was issued, not that it was the image probe. Removing the route entirely, or reverting to ModCtrl only, also fails now. * fix(imageinput): report an unreadable clipboard image instead of silence On macOS the clipboard reader shells to pngpaste or a PyObjC one-liner, and neither ships with the OS: pngpaste is Homebrew-only and Apple dropped the bundled PyObjC. A stock Mac therefore fails every time, and the failure was returned as (nil, nil), which the caller reads as "no image on the clipboard" and treats as a silent no-op. Pressing Ctrl+V produced nothing at all: no image, no error, no hint that anything was missing. The darwin path already knows the difference. It probes with `clipboard info` first and only reaches the extraction step once the clipboard is confirmed to hold an image, so a failure there is not absence, it is "there is an image and nothing here can read it". That now returns ErrClipboardImageUnreadable, which the existing error path turns into a transcript notice naming what to install. The defect predates this branch, but this branch is what makes the path reachable from Ctrl+V, so shipping without it would close #534 with a feature that is inert on one of the three platforms. Raised by gnanam, who measured it on a real Mac rather than inferring it. Deliberately narrow. The Linux arm cannot tell "no tool" from "no image" without probing separately, and Windows uses PowerShell, which is always present, so neither is touched. The test drives the whole path: stubbed reader returns the error, Ctrl+V issues the probe, and the assertion is that the failure reaches the transcript as an error row rather than stopping earlier. Swallowing the error again makes it fail. * test(imageinput): cover the darwin unreadable-clipboard decision itself The test added with the fix asserted around the change rather than on it. It substituted ReadClipboardImage with a stub returning the error, which proves the TUI surfaces an error handed to it, but never reaches readClipboardImageDarwin, where the decision lives. Reverting both production return sites to nil,nil left every package green, which is the definition of a test that is not testing the change. Caught by gnanam. readClipboardImageDarwin shells out twice and had no seam, so the two steps are now package-level vars, the same shape as the readClipboardImage var this change already relied on. That makes the case that matters reachable: the info probe succeeds and reports an image, and the extractor then fails. It cannot be arranged from outside the function, because from there both steps are one opaque call. Five cases, and the negative ones carry as much weight as the positive. An image with no working extractor, and an extractor that exits 0 with no bytes, must both be actionable. A clipboard offering no image, and a failing info probe, must both stay silent, because turning those into a notice would make every stray Ctrl+V nag the user. The extractor must not run at all in either silent case. Repeating gnanam's experiment now fails: reverting both return sites to nil,nil fails the two actionable cases and leaves the silent ones passing, which is exactly the discrimination the fix is supposed to add. |
||
|
Anandan
|
81a5e6cb71 |
fix(sandbox): preserve user config and retry denied writes (#801)
* fix(sandbox): preserve user config and retry denied writes Keep user identity and configuration paths discoverable while retaining managed cache and temporary storage. Classify native sandbox failures as typed recoverable denials so approved commands can retry outside the sandbox. Add regression coverage for global Git ignores, typed denial handling, and permission retry behavior. * fix(sandbox): tighten denial retry safeguards * test(sandbox): assert denial contract * fix(sandbox): trust typed denials and restore Cargo state Stop promoting command output keywords into recoverable unrestricted denials, so only structured adapter facts can enter the approved unsandboxed retry path. Restore the managed CARGO_HOME directory and environment override so Cargo remains writable while the caller's real home stays read-only. * fix(sandbox): mask preserved credential configs Preserving caller configuration made credential-bearing tool files discoverable under Linux's read-all sandbox profile. Add default and environment-overridden npm, GitHub CLI, netrc, Docker, Kubernetes, and Zero config paths to the credential deny set, with regression coverage. Clarify the typed-denial retry and platform read-model comments without changing the approval behavior. * fix(sandbox): protect custom gcloud config CLOUDSDK_CONFIG can relocate gcloud credentials outside the XDG-derived default. Include the configured directory in the credential deny profile and cover the override with a regression test. * fix(sandbox): deny zero credential directory * test(sandbox): keep zero credential directory deny path expectations |
||
|
Anandan
|
5d1869e7bc |
feat: add persistent session goals (#803)
* feat: add persistent session goals * Fix goal run isolation and guards * Strengthen loop goal isolation test * Cover goal edit state transitions * fix(goal): bound automatic continuations Persist a provider-independent continuation limit, require approval for model-created goals, and keep resume, BTW, and loop runs from starting or charging unrelated goal work. Also reject trailing lifecycle arguments and bound persisted objective length. * fix(goal): resume continuations after btw |
||
|
Anandan
|
518d2e53ba |
fix(windows): use PowerShell for command execution (#804)
* fix(windows): prefer PowerShell for command execution Detect PowerShell 7 or Windows PowerShell before falling back to cmd.exe, and share the selected host shell across agent tools and the interactive shell escape. Configure UTF-8 output, preserve restricted-sandbox MSYS safeguards, and add Windows coverage for pipelines, workspace writes, and interactive stdin sessions. * test(windows): align shell coverage with PowerShell Make Windows command fixtures and guidance assertions follow the selected host shell, preserve cmd fallback coverage, and use a genuine MSYS executable when PowerShell aliases are available. Remove the unsupported non-TTY PowerShell stdin assertion; Windows interactive stdin requires a separate PTY transport. * fix(sandbox): classify Windows executable suffixes Normalize .exe, .cmd, .bat, and .com command names before safety and network lookup so PowerShell invocations such as curl.cmd receive the same network permission handling as curl. Add analyzer and engine-level regressions for the Windows PowerShell command shape observed in CI. * fix(sandbox): normalize absolute Windows command paths Extract drive-letter and UNC basenames before removing shell escape characters so fully qualified Windows executables continue matching canonical safety and network classifications. Cover both drive and UNC curl paths with analyzer regressions. * fix(sandbox): normalize relative Windows paths Recognize drive-relative, explicit relative, and suffixed backslash executable paths before generic shell escape removal so Windows commands continue to match canonical sandbox risk entries. * fix(windows): harden PowerShell execution * fix(sandbox): classify PowerShell removal aliases |
||
|
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. |
||
|
|
88fb7b6192 |
fix(sandbox): add platform-specific write lock around grant state file (#752) (#755)
* fix(sandbox): add platform-specific write lock around grant state file (#752) Add grants_lock_unix.go and grants_lock_windows.go with a lockStateFile method that serializes access to the persisted grant state across processes. Wire the lock into Grant, GrantCommandPrefix, Revoke, RevokePath, and Clear. Also add .gitattributes to force LF line endings for Go files so gofmt checks pass on Windows. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(sandbox): do not lock the grants file when there is no notice to consume Addresses jatmn's #755 finding. ConsumeMigrationNotice took the interprocess lock unconditionally, and lockStateFile MkdirAlls the grants directory and creates <grants>.lockfile. Every startup and every 'zero exec' calls this, so it turned reading the grant state into requiring a WRITABLE grants directory: a user with no grants file whose ZERO_SANDBOX_GRANTS_PATH (or its parent config directory) sits on a read-only mount failed outright with 'failed to migrate sandbox grants', where the pre-lock code read an empty state and wrote nothing. Peek at the state without the lock first and return early when no migration write is required. Only when one is — a schema/policy migration, or clearing NoticePending — take the lock and re-read under it, since the unlocked peek is a snapshot another process may have already consumed or migrated, and is meaningless by contract when needsMigration is set. Both regression tests are verified to fail against the pre-fix code. The read-only-directory test skips on Windows (mode bits do not deny writes) and as root (permissions are bypassed), so a portable companion asserts the same requirement by requiring that nothing is created at all. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
Vasanth T
|
f5fa4ffc64 |
fix(config): stop billing provider-command fixtures to the 5s timeout (#810)
Both background-child tests fail intermittently on the Windows runner, and have since #690. #800 and #802 each relaxed an assertion, which is why neither held: the deadline itself was the problem. The tests drove LoadProviderCommand, so the fixture's own startup was billed to the production 5s budget. On a loaded runner the outer timer won the select in runProviderCommand, and both symptoms follow from that. The nonzero-exit test saw a timeout instead of the exit status it asserts, because the timer preempted the shell's exit. The other test lost its PID file, because the timer's Terminate killed the background child before it could record a PID, putting the file permanently out of reach however long the assertion polled. Neither test is a claim about the 5s budget. They assert which error each cmd.Wait path yields and that the leftover descendant is killed, so they now call runProviderCommand directly with a budget of their own and the production deadline stops competing with process startup. Making the child outlive that budget lets both drop their elapsed-versus-lifetime bounds as well, so no wall-clock assertion is left in either test. That is only worth doing if the two timeout paths can be told apart, and they could not: the WaitDelay path and the outer-timer path both returned the same bare sentinel, so asserting on it would have been satisfied by the very timeout the tests exist to rule out. The WaitDelay path now wraps rather than replaces. Callers asking errors.Is for the sentinel are unaffected, so no user-visible message changes. The Windows fixture also waited for its ready marker by launching a second PowerShell. That cold start is pure overhead added after the PID is already written, so the wait now runs in cmd itself and is bounded. Verified by looping both tests under CPU contention: 7 failures in 16 iterations before, none in 32 after, with per-iteration time going from a spread of 2.3s to 66s down to a steady 2.1s. Removing the wrap makes the WaitDelay assertion fail, so it is load-bearing rather than decorative. |
||
|
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]> |
||
|
Anandan
|
ac50a5a840 |
test(config): wait for background PID file (#802)
* test(config): wait for background PID file * test(config): make delayed PID check deterministic |
||
|
Anandan
|
a4a805a6c8 |
fix(tui): reset quiet clock for new runs (#799)
New turns inherited lastStreamActivity from the prior turn, so a fresh request could immediately look stalled. Initialize stream activity from the same timestamp as turnStartedAt and cover the stale-clock case with a deterministic regression test. |
||
|
Anandan
|
9fe02ae52e | fix(sandbox): clarify scoped permission prompts (#798) | ||
|
Anandan
|
9d5c82d441 |
test(config): remove Windows startup timing assumptions (#800)
Cold PowerShell startup under concurrent Windows CI can consume most of the provider timeout, so an unsynchronized PID file may never be written and a fixed four-second bound can fail despite correct process-tree cleanup. Treat an absent PID as a helper that never reached startup, retain strict liveness checks for synchronized children, and bound completion by the child's natural lifetime instead. |
||
|
euxaristiaandClaude Sonnet 5
|
97ef90d27a |
chore: remove code unreachable on every supported platform (#706)
* chore: remove code unreachable on every supported platform The Security and code health job's advisory deadcode step reports 152 unreachable functions on main. Running the same check under GOOS=linux, windows, and darwin shows 13 of those are false positives whose callers live in windows build-tagged files, so only functions unreachable on all three platforms are touched. Functions with no references anywhere are deleted, including the internal/reasoning package, which nothing imports. Functions that only tests reference move into per-package export_test.go files so tests keep their seams while the production binary sheds the code. Test call sites of deleted convenience wrappers are ported to the live APIs (scoped tool constructors, tools.DeferredSource, FinishReason checks). Kept despite the report: cron.Store.Runs (only reader of runs.jsonl, which production writes), and agenteval.TraceEventKeys and MissingTraceEvents (used across packages by the trace contract test). * docs(config): correct ValidateFile's errors.As claim Message is a flattened string (.Error() already called on the parse error), not a wrapped error, so callers can't recover the concrete json error type via errors.As. Fix the comment to match. Co-Authored-By: Claude Sonnet 5 <[email protected]> * chore(trace,skills): drop obsolete Sink adapters and fix stale doc references The dead-code pass removed the Emit methods but left Sink, NDJSONSink, and TextSink exported with a contract nothing implements; remove the types with their contract. Also remove the stray blank line at EOF of trace.go flagged by gofmt, and update skills package docs and HOW_ZERO_WORKS.md to stop directing callers to DiscoveryRoots/Duplicates, which are now test seams. Co-Authored-By: Claude Fable 5 <[email protected]> * chore: clean up remaining review nits on dead code removal Replace a pointless fmt.Errorf(...%w...).Error() with fmt.Sprintf using %v, since the wrapped error was discarded immediately anyway. Remove a comment block that was duplicated verbatim. * docs(skills): drop remaining references to test-only List/Get Load and MergedSkills comments still named List/Get as production entry points after they moved to export_test.go. Point callers at Load and ListFromRoots instead. * chore: remove now-unreferenced export_test.go seams execSessionManager.len() and likelySandboxDenied/sandboxDenialKind had no remaining callers after the sandbox unify-command-execution rebase; the production code they wrapped for tests no longer exists. --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
5c58256abc |
fix(config): kill provider-command process tree via job object on Windows (#690)
* fix(config): kill provider-command process tree via job object on Windows taskkill /T walks the process tree by parent PID in user space and can miss descendants, leaving the sleeping child holding the inherited stdout/stderr pipes so cmd.Wait() blocked until natural completion (~10s instead of the 5s timeout). Assign the started command to a job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and terminate the job on timeout so the kernel kills the whole tree atomically; taskkill remains as a fallback. Also set cmd.WaitDelay so Wait can never hang on pipes held by an escaped orphan, and add a regression assertion that the sleeping child is actually gone after the timeout returns. Fixes #683 Co-Authored-By: Claude Fable 5 <[email protected]> * fix(config): harden provider-command tree termination Address code review on PR #690: terminate the process tree when cmd.Wait() returns exec.ErrWaitDelay (a background descendant kept inherited pipes open, previously left running); start the Windows process suspended and assign it to the job object before resuming its main thread, closing the window where a fast child could escape the job; drop JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so a successful command's detached helpers survive Close() instead of being reaped; escape single quotes in generated PID-file paths; and correct a misnamed Windows exit-code constant in a comment. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(config): avoid gofmt smart-quote rewrite in doc comment gofmt's doc-comment formatter rewrites a trailing '' into a curly closing quote, which CI's gofmt -l check flags as unformatted. Reword to avoid the pattern instead of embedding a raw quote pair. * fix(config): terminate leaked descendants on nonzero exit, avoid PID reuse Terminate the process tree on any error returned from cmd.Wait, not only exec.ErrWaitDelay. Go only returns ErrWaitDelay when the shell itself exited successfully; a nonzero exit (e.g. sleep 600 & exit 7) yields the bare *ExitError instead, so a detached background child that held the inherited pipes open was never terminated on that path. Also stop falling through to the taskkill/PID fallback once a job handle successfully contained the tree: by the time Terminate runs on the ErrWaitDelay path, cmd.Wait has already reaped the root process, so its PID may have been reused by an unrelated process, and taskkill /T /PID against a reused PID would force-kill the wrong tree. Addresses PR #690 review feedback. * fix(config): surface suspended-thread resume failures instead of hanging resumeMainThread now reports whether it actually resumed at least one thread. If Toolhelp/OpenThread/ResumeThread all fail, the launched process would otherwise sit suspended forever, never exit on its own, and get misreported as a plain 5s provider-command timeout instead of the real resume failure. attachCommandProcess now terminates the process immediately in that case. Also documents the known, non-regressive limitation that the taskkill fallback (used only when job creation/assignment itself fails) can still miss a descendant that has reparented away from the tree it walks by PID. Addresses remaining PR #690 review feedback; PR description corrected separately to drop the outdated JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE claim. * fix(config): guard taskkill with retained process identity * fix(config): retain POSIX process group identity * test(tui): match provider wizard shortcut * test(tui): pin credential-step env for deterministic footer assertion The credential step's footer only shows the /→ shortcut once a key is present, whether typed or inherited from the provider's AuthEnvVars. The previous commit changed the assertion to "Enter/→ continue" without accounting for that, so the test only passed by coincidence when ANTHROPIC_API_KEY happened to be set in the environment — failing deterministically on a clean CI runner (as smoke reported on all three OSes). Pin the env var empty and assert the no-key footer text so the test is correct regardless of ambient environment. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(config): preserve provider process attributes * test(config): keep SysProcAttr coverage portable --------- Co-authored-by: Claude Fable 5 <[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. |
||
|
euxaristiaandClaude Fable 5
|
5bb1bc11c3 |
fix(tui): fix rendering corruption over multipass + Windows Terminal (#709)
* fix(tui): disable bubbletea hard scroll optimization unconditionally Vendors a patched charm.land/bubbletea/v2 (via a local go.mod replace) with hard scroll optimization (using terminal scroll-region sequences to shift existing lines instead of repainting) disabled unconditionally, instead of only on GOOS=="windows". That GOOS check only catches a Windows terminal driving the program directly. It misses a Linux process running inside a multipass VM whose output is ultimately displayed by Windows Terminal over a remote shell, which corrupts rendering badly (garbled/overlapping text, worse on resize) the same way the check exists to prevent. There's no reliable way to detect that remote hop from inside the VM, so scroll optimization is now off in all cases. * fix(tui): force redraw on streamed newlines to clear stale caret The streaming caret (appendStreamingCursor) is appended to whichever visual line is currently last. Over a remote terminal chain (multipass VM output displayed by Windows Terminal), the renderer's incremental cell diff sometimes fails to clear the caret's old cell when a newline moves it to a new line, leaving ghost carets behind. Fire tea.ClearScreen when a streamed delta contains a newline: that's exactly the moment the caret moves and the risk exists, so it forces a full redraw right then instead of leaving it to the incremental diff, without doing so on every animation tick. * chore(tui): trim upstream repo metadata from vendored bubbletea fork The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs #709 * fix(tui): address review round on the rendering workaround and vendored fork Fork correctness fixes (all flagged by review): - key.go: KeyMediaRecord repeated the previous const expression and decoded record presses as previous-track; map it to uv.KeyMediaRecord. - mouse.go: MouseButton11 likewise collapsed onto MouseButton10. - termios_unix.go: BSDLY/BS0 are output-delay bits; read them from Oflag instead of Lflag so useBackspace is set correctly. - cursed_renderer.go: count wrapped rows with (lineWidth-1)/w so an exact terminal-width multiple no longer inserts a blank line; compare colors by normalized RGBA via a nil-safe helper instead of interface equality (which panics on non-comparable implementations); and make clearScreen invalidate lastView so the queued erase flushes with the next frame instead of deferring until an unrelated view change — the reordering that could leave the ghost caret in place. - exec_test.go: implement reset() on the spy renderer and inject it before Run; assert exec preserves renderer state (releaseTerminal with reset=false). - logging_test.go: restore the default logger's output/prefix/flags and fail fast on setup errors. - tea_test.go: tie the Quit-pumping goroutine to a channel closed when Run returns instead of leaking it until process exit. TUI scoping (review P2): the forced redraw on streamed newlines is now rate-limited to ~10/s so heavy streaming output no longer turns every newline into a full-screen repaint, and ZERO_NO_STREAM_CLEAR=1 opts a correctly-rendering terminal back into the fast incremental path. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(tui): guarantee deferred stream-clear redraw and other review fixes A throttled stream-clear could drop the redraw entirely when a newline arrived in the last delta of a turn, leaving a ghost caret on screen. Add a one-shot deferred flush timer plus a flush on stream end. Fix a stale comment and a test that leaked global logger state in the vendored bubbletea fork. * fix(tui): use m.now for stream-clear throttle and harden tests Drive stream-clear rate limiting through the mockable clock and a shared streamClearThrottle constant so both the agentTextMsg and flush paths stay in sync. Add an explicit two-newline-within-window regression and detect ClearScreen by function pointer so tests do not evaluate Tick side effects. * fix(tui): relocate bubbletea fork out of third_party Move the maintained bubbletea/v2 hard-scroll patch from third_party/bubbletea-v2-patched to patches/bubbletea-v2 and update the root go.mod replace. third_party/ is immutable vendored content per AGENTS.md; a maintained fork belongs outside that tree. * fix(ci,docs): run vendored bubbletea fork tests and document unsupported go install path patches/bubbletea-v2 is its own Go module so `go test ./...` from the repo root never touched its 7 test files. Add a CI step (and a make test-patches target for local use) that runs the fork's own suite across the smoke matrix. The root go.mod's local `replace charm.land/bubbletea/v2 => ./patches/bubbletea-v2` also breaks `go install github.com/Gitlawb/zero/cmd/zero@<version>`, since a versioned install can't resolve a replace pointing at a filesystem path. Document that path as unsupported in README.md and docs/INSTALL.md and point at git clone + go build instead, per jatmn's review findings on PR #709. * ci: run vendored bubbletea fork tests with -race CodeRabbit review on this PR. The fork exists specifically to patch a concurrency bug (charmbracelet/bubbletea#1690), so its regression test needs the race detector to actually catch a reintroduced race instead of just passing on a benign-looking run. * fix(tui): drop vendored bubbletea fork; keep stream-clear workaround Maintainers asked not to ship an in-tree bubbletea replace: it breaks versioned go install and is a large maintenance surface for a narrow multipass/Windows Terminal scroll bug. Remove patches/bubbletea-v2 and the replace directive; keep the model.go full-redraw-on-streamed-newline workaround (ZERO_NO_STREAM_CLEAR to opt out). Restore normal install docs. Refs Gitlawb/zero#709 --------- Co-authored-by: Claude Fable 5 <[email protected]> |
||
|
PierrunoYTandClaude Sonnet 5
|
d9b882eb0a |
fix(cron): reserve job IDs atomically (#686)
* fix(cron): reserve job IDs atomically * fix(cron): hold the per-ID lock from reservation through persistence Address code review on PR #686: reserveID made the timestamp directory visible via os.Mkdir before metadata.json existed, but Remove (which takes the per-ID lock before deleting) never coordinated with that window. A concurrent `zero cron rm <id>` could delete the bare reservation and report success right after Add reserved it; writeJob's MkdirAll would then silently resurrect the directory and persist the job anyway. A second Add could also re-reserve the same id while the first writer was still running, putting two writers on the same metadata path. reserveID now acquires the same per-ID lock (used by Get/Update/ Mutate/Remove/AppendRun) for each candidate id before attempting os.Mkdir, and returns it still held; Add releases it only after writeReservedJob commits (or tears down) the reservation. A concurrent Remove for that id now blocks until the reservation is resolved one way or the other, and a concurrent Add attempting the same candidate id serializes through the lock rather than only racing the Mkdir. Adds two regression tests: one proving Remove waits for and removes the committed job rather than the bare reservation, and one proving a second reserveID call for the same candidate id blocks on the shared lock and moves on to the next candidate. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> |
||
|
nb213andbinyangzhu000-sudo
|
c00a4734b5 |
Add Atlas Cloud provider preset (#784)
Co-authored-by: binyangzhu000-sudo <[email protected]> |
||
|
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 |
||
|
PierrunoYTandAmp
|
baa4be13ac |
fix: make extension installs transactional (#762)
Co-authored-by: Amp <[email protected]> |
||
|
|
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. |