mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
fix/loopback-proxy-safe-dial
689
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Vasanthdev2004
|
221fb93041 |
fix(proxydial): bind the proxy exemption to the route the transport chose
Asking the Proxy function a question of our own invention answered for a request the transport never made. With NO_PROXY excluding a host, the real request to that host goes direct while a synthetic probe still comes back proxied, so a direct dial whose address happened to equal the configured proxy endpoint skipped the guard and reached whatever the name resolved to at connect time. A probe on the previous head dials rebind.example:80 with zero safety lookups; the guard now runs and refuses its private answer. Host-dependent proxy callbacks make the mismatch general, and no spelling of the probe host closes it, because the two calls are about different requests. The decision is now recorded where it is made. proxydial.Transport wraps the transport, asks its Proxy function about the actual request, and puts the answer in that request's context; the transport derives the dial context from the request context, so the dialer reads back the route this very connection is being opened for. Per-request, so concurrent requests cannot overwrite each other, and the context key is unexported so nothing outside the package can plant one. Address matching is gone with it. A transport opens one connection dial, and when a proxy is selected that dial goes to the proxy, so the recorded route already identifies the dial. Comparing the address on top could only disagree with the transport, which is what a Unicode proxy hostname did: Go dials xn--bro-hoa.example while the URL still spells it as configured, and the proxy was refused as an ordinary private address. Both spellings are now one endpoint with no comparison to get wrong. Documents where destination enforcement ends: with a proxy configured, the target hostname is sent to the proxy and the proxy resolves it, so the local pre-request checks no longer bind the address that is reached. A forward proxy already sees and can rewrite every request through it, so that trust comes with configuring one; the alternative is refusing to use proxies at all, which is the outage in #569. |
||
|
Vasanthdev2004
|
ac3958f860 |
fix(proxydial): have the dialer ask its transport for the proxy at dial time
Copying transport.Proxy into the dialer at construction left the wiring between the two unpinned: a dialer handed nil would pass every unit test while a proxied request still died as loopback. The dialer now consults the transport it is installed on, and each package drives a real request through a loopback listener acting as the proxy and checks that the CONNECT arrives. The proxy is substituted on the transport rather than through HTTPS_PROXY, since http.ProxyFromEnvironment reads the environment once per process and an env-based test would depend on test order. |
||
|
Vasanthdev2004andbaoyu0
|
63767f3e42 |
fix(providerhealth,tools): let the configured forward proxy through the loopback guard
With HTTPS_PROXY pointing at a local forward proxy, the transport dials the proxy first and tunnels the request through it. The SSRF-guarded dialer in providerhealth refused that dial as a loopback address, so `zero doctor --connectivity` failed with "proxyconnect tcp: provider connectivity URL is unsafe: loopback hosts are blocked" for a public provider that had already been validated, and web_fetch had no proxy support at all (#569). Each dialer is now handed its transport's own Proxy function and lets a dial through unpinned only when the address is exactly the proxy endpoint that function names for the request scheme: same host, same port, with a missing proxy port taken as the scheme default. A sibling loopback port is still refused. The shared check lives in internal/proxydial since providerhealth and tools otherwise share nothing. web_fetch honours http.ProxyFromEnvironment again. The guarded fetch had set Proxy to nil as part of its SSRF hardening; the target is still validated before the request, and what a forward proxy removes is only the dial-time rebind pin, which is inherent to using any forward proxy. The pre-permission URL validation stays in place. Started from #594 by baoyu0. This version keeps the pre-permission check, matches the proxy port exactly instead of any port when the proxy URL has none, and injects the transport's Proxy function instead of reading the environment a second time. Closes #569 Co-authored-by: baoyu0 <[email protected]> |
||
|
|
f30f550e60 |
fix(sandbox): protect worktree git pointers (#805)
* fix(sandbox): resolve git metadata carveouts for worktrees .git is a file (gitdir: ...) in worktrees/submodules, not a dir. The hooks/config write-deny carveout assumed a directory and pointed at <root>/.git/hooks, which has no mkdir-able parent under the .git file. bwrap then failed 'Can't mkdir parents ... Not a directory', blocking every sandboxed tool in worktree checkouts. Resolve the real (common) git dir via the gitdir pointer + commondir so worktrees deny the shared hooks/config like a plain clone. Plain checkouts (.git dir) and .git-absent roots keep the literal paths. * fix(sandbox): bound worktree git metadata carveouts to the write root Refuse gitdir/commondir pointers that escape the workspace instead of carving outside confinement. Invalid, stale, or non-directory pointers return no carveouts so bwrap does not mkdir under a .git file. * fix(sandbox): evaluate gitdir/commondir through symlinks Containment must use the real path so an in-root symlink cannot punch hooks/config carveouts outside the write root. * fix(sandbox): fail closed on gitdir eval and protect the pointer file Refuse carveouts when EvalSymlinks cannot produce a real in-root path, compare evaluated roots and targets, and write-deny the .git pointer so it cannot redirect hooks/config between commands. * fix(sandbox): deny worktree-local gitdir metadata and parse first gitdir line Keep hooks/config write-denied on both the common dir and the in-root worktree gitdir, emit those paths under the lexical write root, and accept a gitdir: pointer that has trailing comment lines. * fix(sandbox): skip special git metadata files and re-check carveouts at apply Read .git and commondir only when they are regular files so a FIFO cannot block profile construction. At sandbox apply, drop ReadOnlySubpaths that no longer sit inside the write root after a symlink swap. * fix(sandbox): follow directory .git symlinks and unix-tag FIFO tests A .git symlink to a directory is a normal checkout, so keep lexical hooks/config carveouts. FIFO coverage stays on unix so Windows still compiles. * fix(sandbox): protect worktree git pointer directly * test(sandbox): skip unavailable symlink fixture * fix(sandbox): preserve git symlink semantics * fix(sandbox): limit git pointer carveout to regular files --------- Co-authored-by: jatmn <[email protected]> Co-authored-by: jatmn <[email protected]> |
||
|
Vasanth T
|
aadb4a27e9 |
fix(specialist): keep the pinned model when a specialist is resumed (#1009)
* fix(specialist): keep the pinned model when a specialist is resumed Metadata.Model exists so a bounded, delegated task can run on a cheaper model than its parent, and BuildArgs appends it to the child argv through appendModelArgs. BuildResumeArgs never did. So a specialist pinned to a cheap model ran on that model exactly once. The moment the orchestrator resumed it, the child fell back to whatever the parent's configured model resolved to. Nothing surfaced it: the resumed child starts normally and does the work, so the only symptom is the bill. Cost-motivated delegation quietly stopped saving anything. Resuming does not restore the recorded model on its own. sessions.PrepareExec records the model a run used but never feeds it back into provider construction, so the flag has to be passed again rather than relied upon. BuildResumeArgsInput now carries ParentModel and ParentReasoningEffort, the same fallbacks the fresh path takes, and runResume passes what TaskRunOptions already held. The reasoning-effort rule travels with the model unchanged: the parent's effort is inherited only when the manifest pins no model of its own, because a manifest that chose a different model has not agreed to the parent's effort for it. Regressions drive both builders and compare them, so the two paths cannot drift again: a pinned model survives resume, an unpinned one still inherits the parent's, both halves of the effort rule hold, and the flag keeps its position relative to --auto in both. Refs #554 * test(specialist): cover the resume call site, not just its builder The builder tests all call BuildResumeArgs directly, so dropping the ParentModel field from the runResume call site still compiled and still passed every one of them. The defect this fixes lived at the call site, so it needs a test that goes through Run. Driven through the real dispatch with the RunChild seam capturing argv. * test(specialist): guard the fresh call site as well runFresh and runResume each construct their builder input by hand and carry a byte-identical ParentModel line. Deleting either compiles and, until now, deleting the fresh one was silent. The resume half is what this branch repairs. This covers the other half so the pair cannot drift again in the direction nobody was looking. |
||
|
KRATOS
|
a6ec0435e3 |
feat(acp): surface safe browser tool metadata (#1000)
* feat(acp): surface safe browser tool metadata * fix(acp): align browser permission titles * fix(acp): namespace browser metadata * fix(acp): reject unsafe browser title text |
||
|
Anandan
|
1b5db17656 |
fix(mcp): discover OAuth through protected resource metadata (#991)
* fix(mcp): discover OAuth through protected resource metadata * fix(mcp): refuse OAuth discovery redirects * fix(mcp): harden OAuth metadata discovery * fix(mcp): protect discovered OAuth endpoints |
||
|
Anandan
|
eeea3308fe |
fix(tui): move voice capture to Ctrl+Space (#968)
* fix(tui): keep voice mode from swallowing spaces * fix(tui): clarify voice capture controls * fix(tui): align active voice status with capture key |
||
|
PierrunoYTandClaude Opus 5
|
27b319ca88 |
test(execution): fail loudly when ps cannot answer instead of skipping (#961)
* test(execution): fail loudly when ps cannot answer instead of skipping Every test in process_unix_test.go proves something about zombie detection, and every one of those proofs is read out of ps. When ps could not answer, the tests skipped — which is indistinguishable from success in CI output — and one assertion passed vacuously for the same reason. A regression in ps parsing or availability would have shown up as green. requireProcessState probes BOTH ps pipelines against this test process, which is by definition alive and not a zombie, so a working ps must report a live state for it. When it cannot: - Under CI (or ZERO_REQUIRE_PS=1) this is a hard failure. ps is present on every image used today, so its absence means the pipeline regressed and that has to surface immediately. - Otherwise it stays a skip, so a contributor on an unsupported box is not blocked. Both pipelines are probed because they are deliberately separate: the tests assert what the production process-table parser concludes, so this file keeps its own per-PID ps oracle rather than borrowing that parser to establish its own preconditions. One regression must not be able to silence both sides at once. Once the probe passes, failing to observe an EXPECTED state is a defect rather than an environment problem, so the remaining escapes become failures: - The zombie waits (awaitZombie) fail instead of skipping. The caller never waits on the child, so its zombie row persists until reaped — reaching the deadline means the child never exited or ps stopped answering, and the two are reported distinctly. - A zombie group that is not signalable fails instead of skipping: a zombie still occupies the process table, so if kill(-pid, 0) fails the case under test is not present and the assertions after it would prove nothing. - processSignalTarget returning anything but the individual PID fails instead of skipping: the child inherited this process's group, so its PGID is this process's PID and can never equal its own PID. The vacuous assertion in TestTerminateProcessTreeStopsRunningGroup now requires ok. That is sound rather than merely stricter: the test never waits on the child, so the group's zombie row must still be present after termination, and an unknown state means ps stopped answering rather than that the group is clean. Verified on Linux (WSL, go1.26.6) rather than by cross-compilation alone: - all three tests pass unmodified - with a stub ps that exits 1: CI unset skips with the reason named; CI=true fails, naming which pipeline could not answer - the same broken-ps run against the ORIGINAL file produced two silent skips plus one unrelated failure after 12s, versus an immediate, explicit failure now Windows is unaffected (the file is !windows); go vet passes for linux, darwin and windows. Fixes #863 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * test(execution): treat an unsignalable zombie group as a kernel difference The macOS Smoke job failed: the zombie group 5931 is not signalable (operation not permitted) That was my error. I converted this skip along with the ps-availability ones, but it never had anything to do with ps: whether kill(2) may signal a group whose only member is a zombie is a KERNEL difference. Linux permits it, so signalTargetRunning goes on to consult ps — the branch this test was written for. Darwin returns EPERM, so signalTargetRunning short-circuits on the kill and never reaches ps. Rather than restore the blanket skip, the test now records which branch answers: - Signal permitted: assert processGroupHasRunningMember directly, so a ps-side regression cannot hide behind the kill result. This is new coverage; the old test only checked signalTargetRunning, which conflates the two paths. - Signal refused: log it and continue. The outcome assertion — a zombie-only group must not count as running — now runs on EVERY platform. Previously Darwin skipped before reaching it, so this is strictly more coverage there, not less. Any errno other than EPERM still fails: it means the group is not in the state the test needs, and nothing after it would be meaningful. TerminateProcessTree is asserted only where the signal is permitted, because terminateTarget's own kill(2) would hit the same EPERM and return it as a cleanup error. Worth a maintainer's eye: that may be a real gap in TerminateProcessTree on such a platform — a zombie-only tree is already exited and arguably should terminate cleanly rather than report failure, which is the same spurious-cleanup-failure class #774 set out to remove. Confirming that needs a Darwin host and is a production change, so it is flagged rather than guessed at here. Verified on Linux (WSL, go1.26.6): all three tests pass, including the new direct ps assertion. The Darwin branch was exercised by forcing the EPERM path on Linux — it logs both explanations and passes with the outcome assertion still enforced. go vet passes for linux, darwin and windows. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
Anandan
|
1fde941803 |
perf: reduce startup and turn overhead (#932)
* perf(tui): disable duplicate composer cursor blink * perf(tui): stop composer wakeups after idle * refactor(tools): publish immutable registry snapshots * perf(startup): defer optional tool discovery * perf(provider): reuse compatible response sessions * feat(provider): add native patch tool input * fix(provider): recognize saved chat sessions * perf(tui): render only on screen changes * fix(runtime): address performance review findings * fix(usage): expose cache efficiency details * perf(agent): reduce redundant bounded-task turns * fix: harden runtime review edge cases * fix: address runtime review findings * build: restore official Bubble Tea dependency * fix(cli): redact MCP registration errors * fix: harden turn session regressions * fix: keep run tools and patch roots consistent * fix: bind freeform patch writes to scoped roots * test: keep patch fixtures out of worktree * fix(tools): reject mixed-root freeform patches |
||
|
KRATOS
|
7acac7acee |
fix(sandbox): refcount temporary roots so one holder's cleanup cannot revoke another's (#911)
* fix(sandbox): a temporary root is refcounted, so one holder's cleanup cannot revoke another's Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for the self-contained pieces to arrive separately. A temporary grant was add-then-remove with no notion of who still needed the root. The second caller to ask for a root already present got a NO-OP undo, and the first caller's cleanup then removed the root out from under it — so a live grant lost its access while its holder still believed it had it. Two read-only tools in the same parallel batch, both blocked on the same directory, is exactly that shape, and read-only tools are precisely the ones the batch runs concurrently. tempReads/tempWrites now count live holders, and the root is only stripped when the last one releases. The release also performs both mutations under ONE hold of the lock. Dropping it between them left a window where the root was still in readRoots but no longer in tempReads: a concurrent AddTemporaryRead landing there reads it as a PERMANENT root, hands its caller a no-op undo, and this call then strips the root — so that caller believes it holds access it has already silently lost. Independent of the remaining #829 work; builds and tests against current main on its own. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a read covered by a TEMPORARY write root takes a reference too Found by a self-review of this branch: the refcount closed the defect within reads and within writes, but not across them. AddTemporaryRead returned a no-op undo whenever writeRootCoversLocked said a write root already covered the path, and the comment there claimed that cover was permanent. It is not: writeRootCoversLocked scans extraRoots, and AddTemporaryWrite appends TEMPORARY write roots into that same slice. So a read covered by another holder's temporary write took no reference, and that holder's cleanup silently revoked the reader's access — the very defect the refcount exists to close, reached across the read/write boundary instead of within one side of it. Demonstrated before fixing, and the regression fails without it: ReadRoots before release = [ws, …/outer] ReadRoots after release = [ws] the write holder's cleanup revoked a live read grant on …/outer/inner The read now takes a reference on the covering temporary write root, and the genuinely permanent case — the workspace root, or a session-scoped grant — still returns a no-op undo. The regression also asserts the other direction: once the reader releases too, the root is gone, so the reference does not leak the grant past its last holder. Origin-Session: local-0484ba | Claude Code | 6 prompts Origin-Snapshot: e6d90ff3ce46 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): make the concurrency test concurrent, and portable off macOS Both raised by CodeRabbit. The third finding on this PR — a read covered by a TEMPORARY write root taking no reference — was already fixed in 87a722cb with the regression it asked for; this covers the two that were still live, and both are cases of a test reporting more than it checked. THE CONCURRENCY TEST WAS NOT CONCURRENT. The gate spun until hasReadRoot saw somebody holding the root, which is satisfied by the FIRST holder, then opened the release. The spin wins that race against goroutines still being scheduled: instrumented over 200 runs, 194 peaked at a single simultaneous holder and not one ever reached eight. A test named for concurrent holders was exercising one holder at a time. Against a deliberately broken refcount, where the first release strips the root, it passed 33 times in 40 — it was a flaky test that mostly agreed with broken code. Counting all eight acquisitions makes the overlap real: 200 of 200 runs now peak at eight, and the same broken refcount is caught 40 times in 40. Holders signal the barrier on every path including failure, so a holder that returns early fails the test rather than hanging it. THE HELPER SKIPPED ON WINDOWS. scopeOutsideRoots reached for /Users/Shared and fell back to /var/empty, neither of which exists there, so every test built on it skipped and reported green having asserted nothing. It now uses the home directory, which no platform lists as a default write root, matching what scope_extra_read_test.go already does — and then PROVES the choice against defaultTempWriteRoots rather than assuming it, so a future default root turns these tests into an honest skip instead of a silent no-op. Origin-Session: local-abff1c | Claude Code | 2 prompts Origin-Snapshot: d2f269b81f33 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a permanent grant outlives the temporary one, and an undo is per holder Both P1s from @anandh8x, both reproduced before changing anything, and both are the same mistake in different places: treating "something covers this path" as though it answered "and will it still be there afterwards". A PERMANENT GRANT WAS REVOKED BY A TEMPORARY HOLDER. extraRoots holds temporary write roots alongside permanent ones, so Add asked only whether the path was already covered. A session-scoped grant made while a temporary holder happened to cover it recorded nothing at all, and disappeared the moment that holder released. Outliving the request that prompted it is the entire difference between Add and AddTemporaryWrite, and it was not happening. AddRead had it too, including across the boundary: a permanent read covered only by a temporary WRITE root died with that root. Permanent coverage is now looked for first and across every root, so a temporary cover cannot decide the answer when a permanent one is also present. The same root held temporarily is promoted in place — its holders' releases become no-ops, which is what permanence means here. A BROADER temporary root over a narrower permanent grant cannot be fixed by promotion, so the narrower root is recorded in its own right; that is the case he specifically called out, and it is the one a promotion-only fix would have missed. ONE HOLDER'S UNDO IS ONE HOLDER'S REFERENCE. The comment on releaseTemporaryRead claimed each undo is called exactly once and that flooring the count at zero covered the rest. Flooring stops the count going negative; it does nothing about a holder's second call consuming a DIFFERENT holder's reference. With two readers, calling the first's undo twice took the count 2 -> 1 -> 0 and removed a root the second was still using — the exact revocation this refcount exists to prevent, reached through a duplicate call rather than a sibling's cleanup, and a deferred cleanup in a retry path is how a caller does that by accident. Every undo handed out is now wrapped so it drops its own reference at most once, and the comment says what is actually true. Both mutation-checked: unwrapping the undo, and ignoring permanence in Add, each fail the test that covers them. Race detector clean. Origin-Session: local-abff1c | Claude Code | 3 prompts Origin-Snapshot: 07900397c62e Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * chore(sandbox): re-request an automated review of this head No code change. CodeRabbit's verdict on this PR is pinned to ff85dbcc, a commit that no longer exists on the branch, and a stale changes-requested keeps the PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on push rather than on request, and repeated @coderabbitai triggers over five days produced nothing, so this is the only lever available without maintainer rights. The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of 4a7063aa. The tree they approved is byte-identical to this one. Origin-Session: local-8cd239 | Claude Code | 6 prompts Origin-Snapshot: 149ab98453ad Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): a fixture failure fails, a report cannot deadlock, and AddRead's own branches are covered All raised by CodeRabbit on the full review. Every one verified before changing anything. ## The concurrent test could hang instead of failing failures was buffered at one slot per holder, but each goroutine can report at BOTH hasReadRoot checks — its own grant going missing while held, and again after the release gate. Nothing drains the channel until after wg.Wait(), so the ninth report blocks forever, wg.Done() is never reached and the wait hangs. The test would deadlock in exactly the case it exists to catch: a refcount bug severe enough to fail more than eight of eight holders. Buffered at 2*holders. ## A broken fixture reported itself as a pass TestANarrowerRequestHoldsTheCoveringGrant called t.Skip when os.MkdirAll failed. The directory is the fixture, not a precondition the environment may reasonably withhold, and the sibling test above it already uses t.Fatal for the same call. ## Two tests assumed something they never checked Both tests in scope_extra_read_test.go place their grant under $HOME precisely because NewScope seeds /tmp and $TMPDIR as write roots — a grant that collapsed into one of those would be writable for a reason unrelated to the property under test. $HOME is normally clear of them, but a sandboxed or CI environment that points HOME at a temp directory turns both tests into something that proves nothing while still reporting PASS. homeGrantOutsideTempRoots now builds the fixture for both and skips with the reason when the assumption does not hold. Verified by running with HOME set to a temp directory: both tests SKIP naming the covering root, where before they passed. The path is resolved before comparing, because the temp roots are resolved and /var is a symlink to /private/var on macOS. ## AddRead's branches were only covered from the write side Two subtests added: a permanent read under a temporary WRITE root, and a narrow permanent read under a BROAD temporary read root. Both mutation-checked by removing the corresponding temporary-coverage guard in AddRead — each fails. The first mutation I ran did not fail its test, and the finding looked wrong: the pattern being edited occurs in Add, AddRead and AddTemporaryWrite alike, and a first-match edit had changed Add. Re-run against the line inside AddRead, it fails as claimed. Recorded because a mutation that misses its target is indistinguishable from a test that holds. ## Also asserted: the refcount table is empty, not merely invisible hasReadRoot answers what a caller can see. An entry left at zero in tempReads is not visible that way and would still leak one map entry per acquire/release cycle, so the contention test now checks the terminal state directly. go test -race ./internal/sandbox/ -count=5: clean. Unrelated and pre-existing here: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-8cd239 | Claude Code | 12 prompts Origin-Snapshot: dd397730a138 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a reader outliving its writer keeps read authority, not write Reported by @jatmn, and it is a privilege escalation rather than a lifetime bug. AddTemporaryRead, given a path already covered by a temporary WRITE root, took a reference on that write root and returned an undo that released it. The lifetime was then correct — the reader no longer lost its access when the writer cleaned up — but the capability was wrong. extraRoots feeds validate(), which authorises WRITES, so once the write holder released first the path stayed in extraRoots for as long as the read holder lived. A caller that asked only to READ could write anywhere below the former write root after the write grant had ended. His diagnosis names the root cause exactly: one write-capability reference was doing two jobs, tracking a lifetime and standing in for a read-only dependent. The lifetime is now kept as what the caller actually asked for — a temporary READ root on its own path. AddTemporaryRead consults permanentWriteRootCoversLocked rather than writeRootCoversLocked, so a merely temporary write cover no longer short-circuits it into borrowing write authority. Mutation-checked: restoring writeRootCoversLocked in that branch compiles and fails the regression on three separate assertions — the reader keeps no read root of its own, ReadRoots() loses the path, and the writer's cleanup revokes a live read grant. Two things a verification pass turned up that are worth recording: The comment justifying this design said readRoots "feeds ReadRoots() and nothing else". That was false — it also feeds ExtraReadRoots(). The security argument survives, because neither confers write authority and validate() reads workspaceRoot plus extraRoots, which this path never enters. But that sentence IS the written justification for the whole choice, so it now lists both and says why neither grants write. The "the reader releases first" subtest passes on the unfixed tree: the escalation needs the WRITER to go first. It is a real no-regression control for the opposite ordering, not evidence the finding is closed, and it is renamed to say so rather than padding the count. Rebased onto |
||
|
KRATOS
|
1649008506 |
fix(tools): an edit keeps the reads it did not disturb (#908)
* fix(tools): an edit keeps the reads it did not disturb
Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for
the small self-contained pieces to arrive separately.
A successful edit re-baselined the file through RecordHash, which drops EVERY
recorded read range. A file read in three pieces therefore lost all three to a
single two-line edit, and the next six edits into regions that had already been
read were refused as unseen — the model had to re-read what it had just read in
order to keep working.
RecordEdit replaces that on the edit path. Because the edit is ours, the changed
span is known exactly, so ranges the edit did not touch are carried across,
ranges after it are shifted by the line delta, and a range the edit spans is
split around it.
An EXTERNAL change still drops everything, deliberately: once a formatter has
rewritten the file we no longer know which line holds what was read, and the
conservative drop is the only honest answer there.
"Seen whole" is now decided in ONE place. SeenWhole derives the answer from the
ranges because the raw flag is only ever set by a single covering read, but
RecordEdit branched on the flag — so a file read whole in two chunks reported
seen-whole true, then false after one edit, putting back the write_file refusal
the derived answer exists to prevent.
countLines now reports the count a READER would give. strings.Split leaves an
empty final element for content ending in a newline, so it counted 4 lines for a
3-line file; since SeenWhole asks whether the ranges cover 1..total, that
inflated total made full coverage unreachable for a file just read in full.
Nearly every text file ends in a newline, so this was the common case.
Origin-Session: local-de382f | Claude Code | 9 prompts
Origin-Snapshot: 92ae33c95cd1
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: ae653221fa15
* chore(tools): re-request an automated review of this head
No code change. CodeRabbit's verdict on this PR is pinned to 6f0cd6c4, a commit
that was force-pushed away five days ago, and a stale changes-requested keeps the
PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on
push rather than on request, and repeated @coderabbitai triggers over five days
produced nothing, so this is the only lever available without maintainer rights.
The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of
0b6fcf54, which is an unhappy trade for an empty commit — the tree they approved
is byte-identical to this one.
Origin-Session: local-8cd239 | Claude Code | 6 prompts
Origin-Snapshot: 149ab98453ad
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: ae653221fa15
* perf(tools): derive the changed-line span from bytes, not from per-line slices
Reported by @jatmn.
changedLineSpan split both file versions into []string, so it allocated one
string header per line for EACH version — work that scales with the size of the
file rather than the size of the edit. countLines then split a third time just to
take a length. All of it ran before RecordEdit had checked whether there was a
tracked observation to update at all.
He measured 32,036,640 bytes for two 2 MB versions of a million short lines, and
extrapolated roughly 1.6 GB of headers for a 100 MB file before counting the
content, the updated bytes and the hashes already live beside them.
The memory was not the sharp end. edit_file writes the updated bytes BEFORE
calling RecordEdit, so an out-of-memory kill lands after the user's file has
changed and before the tracker baseline catches up: the file and the record of it
disagree, and nothing says so. His instruction was to fix the allocation source
rather than add a size guard, and that a guard — if used at all — would have to
reject before os.WriteFile rather than during post-write re-baselining. Taken as
written: there is no new limit, and the same edits are accepted as before.
Two identical byte prefixes share every line that ends inside them, so counting
newlines in the common prefix counts the unchanged leading lines directly. Same
for the suffix, with one rule that the line-array version got for free: the line
a shared tail STARTS in is only shared when the tail begins on a line boundary in
both versions, or its opening bytes differ and it is a changed line that merely
ends the same way.
Measured on his own scenario, two 2 MB versions:
changedLineSpan 32,036,640 bytes -> 0 allocations
countLines per-line slice -> 0 allocations
## The equivalence is tested, not asserted
A rewrite of a diff primitive is only safe if it is behaviour-preserving, so the
replaced implementation is kept in the test file as an oracle and both are asked
the same questions. That caught 206 mismatches in my first attempt at the byte
scan — empty versions, and trailing lines that are only partially shared. Neither
appears in the hand-written cases, and I would have shipped it.
Two regressions: the oracle comparison, and an absolute allocation ceiling rather
than a ratio, so a future rewrite that reintroduces per-line slices fails rather
than merely getting slower. Mutation: restoring strings.Split in either function
allocates 3.2 MB and 3.6 MB against a 4 KB bound.
splitLinesForTracking lost its last caller and is removed — the same obsolete-
helper lint failure that broke Windows CI on #911, caught here by running
make lint-static before pushing rather than after. 0 issues.
Rebased onto
|
||
|
KRATOS
|
6fe0d1edcd |
fix(lockutil): use kernel-held advisory locks (#950)
* fix(lockutil): use kernel-held advisory locks * fix(lockutil): harden cross-platform lock files * test(lockutil): require one concurrent holder |
||
|
|
e59bf78cab |
fix(sandbox): Make WSL backend tests deterministic. (#948)
Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
Kevin CodexandClaude Fable 5
|
6645f9744b |
perf(agent): cheap compaction — dedicated summarizer model, input cap… (#532)
* perf(agent): cheap compaction — dedicated summarizer model, input caps, free reactive prune, window re-derive
Compaction summarization was the single most expensive recurring event
in a long run: the full elided middle — verbatim tool bodies included —
went to the session's main model at full price (a ~100k-token call near
a 200k window, recursively multiplied on overflow).
Four changes, each independently safe:
- Dedicated summarizer model. Compaction summaries route to a cheap
model resolved as ZERO_COMPACTION_MODEL > preferences.compactionModel
> a curated default on official endpoints (Haiku 4.5 / Gemini
Flash-Lite / gpt-4.1-mini; custom-endpoint catalogs are unknowable so
they get none). Built lazily on the first paid compaction; ANY
failure permanently falls back to the main provider for the run, so a
misconfigured summarizer can never break compaction. "main" at any
level forces the old behavior.
- Summarizer input caps. renderTranscript clamps tool-result bodies to
2,000 chars and tool-call arguments to 500 (head+tail, rune-safe) —
the summary needs what was done and decided, not raw payloads. Cuts
summarizer input 5-10x on tool-heavy sessions.
- Free reactive prune. recover() now runs the same prune-first stage as
the proactive path: a context-limit error is first answered by
pruning stale tool bodies at zero cost, without consuming the
one-shot reactive budget; the paid summarizer only runs when a later
error finds nothing left to prune.
- Window re-derivation on model switch. Options.ContextWindowFor
(wired to the model registry in exec and the TUI) lets the loop
re-derive the compaction threshold after escalate_model, closing the
documented KNOWN LIMITATION where a switch to a smaller-window model
could overflow and a larger one over-compacted.
## Rebase notes (2026-08-25, onto
|
||
|
Kevin CodexandClaude Fable 5
|
2749447cdc |
fix(tools): make apply_patch tolerant and re-expose edit_file (#956)
* fix(tools): make apply_patch tolerant and re-expose edit_file
Head-to-head benchmarking against Grok Build on grok-4.6 (3 tasks × 3 runs)
showed Zero solving every task but spending 2.7× the tokens and 2.3× the
time on the multi-file feature task. Every one of the 20 apply_patch calls
in those runs failed, after which the model degraded to whole-file
write_file rewrites and read→write verification loops.
Root causes and fixes:
- The sandbox and the tool both rejected an absolute path that lies inside
the workspace ("must stay inside the workspace"). Models routinely echo
the absolute path read_file showed them. Absolute paths now flow through
the normal workspace-scope validation (inside → allowed, outside → still
denied); unified-diff headers are rewritten to a/ b/ relative form before
git apply, and platform symlink prefixes (/var → /private/var) are
normalised the same way the other file tools do.
- The structured parser demanded a byte-exact "*** Begin Patch" /
"*** End Patch"; grok-4.6 writes "*** Begin Patch ***". Decorated marker
spellings are accepted, and a unified range after "@@" ("-12,4 +12,6",
optionally with a heading) no longer becomes a literal context search.
- Parse errors now carry a one-line format reminder so a failed call is
recoverable in one retry instead of a format-guessing loop.
- edit_file (exact search/replace with uniqueness guard, CRLF and fuzzy
fallbacks) is advertised to the model again alongside apply_patch, and
the apply_patch description shows the structured format.
- Writers (edit_file, write_file, apply_patch) recorded a file's line total
with lineCount, which is one higher than read_file's count for a file
ending in a newline. RecordSeenRange resets every observation when the
total changes, so a partial read_file after a successful edit silently
discarded whole-file knowledge and the next edit was refused with
"content that has not been read exactly in this session". All writers
now record the same total read_file reports (trackedLineTotal).
- read_file's padded " N | " line prefix was ~19% of every read (about
1.5k tokens per 800-line file) and was re-sent on every later call. It is
now the compact "N→" prefix models already know; nothing parsed the old
form.
- The editing-discipline prompt steers: edit_file for a targeted change,
apply_patch for multi-hunk/multi-file changes, write_file only for new
files or full rewrites; a successful edit result is confirmation, so the
file is not re-read to verify it.
Tests: regression tests for decorated markers, writer/reader line-total
agreement (edit → partial read → edit far away still succeeds), range hunk headers,
absolute in-workspace paths (structured + unified), outside-workspace
rejection, header-line-only relativisation, and tool visibility; the
prompt and tool-budget tests updated to the new contract.
Benchmark (grok-4.6, 3 tasks x 3 runs, harness in demo/bench-zero-vs-grok):
Zero 0.8.0 617 s / 1,527k tokens -> with this change 346 s / 493k tokens;
Grok Build 407 s / 713k. Every run of every configuration solved its task.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): keep CRLF line endings when relativizing patch headers
relativizeUnifiedPatchPaths split on a normalized "\n" and rejoined with
"\n", so any CRLF patch that needed a header rewrite lost its line endings
everywhere; git apply could then fail to match a CRLF file. Split on "\n"
only and carry each line's trailing "\r" through, including on rewritten
headers.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(sandbox): share the structured-patch classifier with apply_patch
Review follow-ups for #956:
- The tool accepted decorated structured markers ("***Begin Patch") that
the sandbox's applyPatchPaths did not recognise, so for that spelling the
boundary scanned the patch as a unified diff, extracted no targets and
validated nothing. The marker classifier now lives in the sandbox package
(StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it,
so both sides accept exactly the same set; tests cover inside, traversal
and outside targets for the no-space spelling at the boundary.
- Document the containment model at the apply site: structured patches are
applied through an opened os.Root (descriptor-relative, no-follow);
unified diffs keep the pre-existing git-apply pathname flow, which the
absolute-path rewrite does not widen (they become the same root-relative
form relative paths always used).
- OpenAI addendum: the one-file-per-call rule names read_file, edit_file and
write_file explicitly and allows a single multi-file apply_patch.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): compare slash-normalised patch paths so the check passes on Windows
structuredPatchHeaderPaths normalises separators to "/", so the absolute
outside-workspace path in the boundary test must be compared in its slash
form; on Windows the backslash original never matched and the Smoke job
failed.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): validate the parsed patch path at the scope boundary
For each accepted marker spelling, pass the exact path applyPatchRequestPaths
produced to scope.validate: an outside path must be denied and an inside
path accepted, so a separator-handling regression between the parser and the
scope cannot slip past the test.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): apply unified diffs in-process through the workspace root
Review follow-up for #956. Unified diffs were validated by pathname and then
handed to git apply, which reopened the targets by name — a check-to-use
window a symlink swap could exploit — and the absolute-path rewrite added in
this PR normalised `diff --git` headers with strings.Fields, which splits a
git C-quoted path containing whitespace.
Both go away by not shelling out at all: parseUnifiedPatch translates a
unified diff into the same operations a structured patch produces, and both
formats are applied by applyPatchOperations through os.OpenRoot — every
stat, read, create and write is descriptor-relative and refuses to follow a
link out of the root. git is no longer required by apply_patch.
Unified-diff support: modifications, creations (--- /dev/null), deletions
(+++ /dev/null), multiple hunks per file, git "rename from/to" (a move,
hunks optional) and "copy from/to" (a new copy operation whose destination
inherits only the tracker credit the source had), "\ No newline at end of
file" on either side, a/ b/ prefixes, C-quoted paths, CRLF patches, and
git's diff/index/mode lines. Hunks are placed at their "@@ -a,b" range when
the expected lines match there and located by context otherwise (so a
stale range still applies and duplicate blocks are disambiguated by the
range). Binary patches are rejected.
Removed: the git apply flow, relativizeUnifiedPatchPaths,
recheckPatchWriteTargets, missingPatchTargets, completeCreatedPatchTargets
and recordCreatedPatchTargets. validatePatchPaths/changedFilesFromPatch stay
for mutation-target listing.
Tests: the existing unified-diff suite now runs through the in-process
applier unchanged; new coverage for quoted whitespace paths (inside applies,
outside is refused untouched), a workspace path swapped for a symlink that
escapes the root (both formats refused, outside file unmodified), create +
delete + no-newline markers, range hint vs context fallback, rename with a
hunk, and binary/empty rejection.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): harden unified-diff parsing and cover the copy operation
Review follow-ups for #956 (in-process unified applier):
- A hunk that ends before its declared line counts is reported as
malformed instead of silently absorbing the next file's ---/+++ header
pair (which then failed later against the wrong file). Both shapes are
caught: leftover counts at a header or at end of input, and a --- line
directly followed by +++ inside a hunk. The element after a patch's final
newline is no longer counted as an empty context line, so a truncated
hunk cannot look complete.
- "\ No newline at end of file" may only follow a file's final hunk; a
marker before another hunk is rejected rather than leaking into that
hunk's result.
- Zero-context insertions (git diff -U0) are documented as
position-trusting by design, matching git apply without fuzz; hunks with
context are verified at the range before it is used.
Tests: copy from/to (source kept, destination created with the hunk
applied and tracked with the source's whole-file state; existing
destination refused untouched; copy onto itself refused), over-declared and
truncated hunk counts, and the no-newline marker before vs after the final
hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): require an @@ line before treating ---/+++ inside a hunk as a file boundary
A removed "-- x" line directly followed by an added "++ y" line is valid
hunk content that also matches the ---/+++ header shape; the boundary check
now additionally requires the following line to be a hunk header, and a
regression test keeps such a pair intact. End-of-input strictness for a hunk
cut short is documented as deliberate: applying a truncated hunk would make
a partial change (removals without their replacements).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* refactor(tools): share one line-count implementation between reader and writer
sourceLineCount and trackedLineTotal computed the same count; keeping two
copies invites exactly the reader/writer drift this PR fixed. The reader now
delegates to trackedLineTotal, which carries the tracker contract.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): verify a unified deletion's expected content before removing the file
A unified diff's "+++ /dev/null" hunks state the content being removed, but
the in-process applier dropped them and deleted unconditionally, so a stale
deletion removed a file whose current content the patch never described.
Unified deletions now keep their hunks, require at least one, and run them
through the same verifier as updates: the hunks must match the current file
and leave nothing behind, otherwise the deletion is refused and the file is
untouched. A structured "*** Delete File" keeps its unconditional meaning.
Hardening from the same review: every non-create change re-reads its source
through the opened root immediately before commit and refuses to proceed if
the bytes differ from what was planned.
Tests: matching deletion, stale deletion (file byte-for-byte unchanged),
partial deletion, header-only deletion, multi-hunk deletion with one stale
hunk (nothing removed) and all-good hunks, structured delete unchanged, and
a source changed between planning and commit for update and delete.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): a move with a missing source is refused before publishing
The pre-commit recheck now refuses a move whose source vanished after
planning instead of publishing the destination and then failing; update the
test to the safer contract (error names the recheck, nothing is created).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): byte-exact unified deletions and git's header-only empty-file forms
Review follow-ups for #956:
- A unified deletion is verified byte for byte: the removed lines (with the
removed side's "\ No newline at end of file" honoured) must equal the
current file exactly, with only the file's own line-ending style
normalised. None of the whitespace tolerance used to locate update hunks
applies, and a whitespace-only remainder no longer passes; a deletion is
not recoverable, so it fails closed.
- git describes an empty file's deletion or creation with header lines only
("diff --git", "deleted file mode"/"new file mode", "index") and no
hunks. Both forms are recognised: the deletion is applied only when the
current file is empty, the creation writes an empty file, and they compose
with hunk-carrying files in the same patch.
- The pre-commit recheck test also covers the copy operation and asserts no
destination is created on refusal.
Tests: whitespace/indentation/blank-line/space-only/missing-newline
mismatches all refused with the file unchanged, matching no-newline marker
and CRLF file accepted, header-only deletion of a non-empty file refused,
multi-file patch with header-only delete + create + a normal hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): remove the move source after planning via a deterministic hook
The refused-move test now creates source.txt, plans the move, and removes
the file through a test-only pre-commit hook (structuredPatchBeforeCommit)
so it exercises the post-planning disappearance path rather than a source
that was never there. Assertions unchanged: refused before publishing, no
destination created.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tui): read_file fixtures use the compact N→ prefix
Three TUI fixtures still built fake read_file output in the legacy
" N | text" shape; they pass either way but pinned an outdated contract.
They now mirror what appendReadFileRange emits, and the shape comment and
the expanded-card assertion are updated.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): name the committed files when a patch is partially applied
Review follow-up for #956. When a later change in a multi-file patch fails
(for example the pre-commit recheck refuses a file that changed after
planning), the error now lists exactly which files were already committed
and states that the rest are unchanged, instead of a generic "partially
applied" warning. The model can then re-read only what actually changed.
Test: a three-file structured patch whose second file is altered after
planning fails with "already committed: first.txt", first.txt holds the
patched content, and the other two files are untouched.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
---------
Co-authored-by: Claude Fable 5 <[email protected]>
|
||
|
github-actions[bot]
|
ad34dc8d81 |
chore(main): release 0.8.0 (#896)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>v0.8.0 |
||
|
beardthelion
|
6edf9a8b78 |
fix(sandbox): normalize launcher names before the command-prefix denylist (#934)
* fix(sandbox): retire recorded approvals through a policy bump Ahead of tightening which command prefixes may be approved, make the upgrade path safe. A grant already on disk that the narrower rules reject fails validation on read, and that path rejects the entire grants file rather than the one entry. Every caller treats a read error as "no grant" (engine.go LookupCommandPrefix, ApprovedCommandPrefixes, and the Lookup in Evaluate), so one stale prefix would silently disable the user's persisted deny grants too, with the error text never reaching them. Bump the approval policy version instead and let the store's existing migration handle it: it backs the file up, keeps deny grants, invalidates prior approvals, and reports the counts once through the startup notice that both frontends already print. That is what the mechanism was built for. The v1-migration test pinned the policy version as a literal, so it now derives the expected value from the constant and will not need editing on the next bump. * fix(sandbox): normalize launcher names before the command-prefix denylist unsafeCommandPrefixLauncher matched the program name as written, so python3.11, python2.7, python.exe, node.exe and git.exe all validated as ordinary commands and could be persisted as command-prefix grants. Lookup is a string-prefix match, so a single approval then auto-allowed every later invocation: granting [python3.11 -c] covers `python3.11 -c "import os; os.system(...)"` with no further prompt. internal/agent's commandName already normalizes case and Windows executable extensions for the allow side of the same decision; the deny side did not, and the two disagreeing is the bug. Reduce the program to the launcher it actually runs and match on that: executable extension, Windows' trailing dots and spaces (python. starts python.exe), CPython ABI flags, then a trailing version, testing the digit-stripped form as well so python3.11 and python3 both arrive as python. A name is only ever narrowed toward an existing entry, so the failure direction is an extra permission prompt, never a silent grant. Digits without a separator stay part of the name, which keeps base64, 7z and sha256sum grantable. Two Windows spellings cannot be resolved by name at all, so the shape is refused alongside a path: 8.3 short names truncate the stem, which makes POWERS~1.EXE unrecognizable, and python.exe::$DATA reaches the same executable through its default stream. Normalizing the first token before the banned-suggestion comparison keeps git.exe at parity with git. The list also gains launchers of the same class it already covers and that normalization cannot reach: cmd, wsl, the remaining POSIX shells, busybox, uv and uvx. Entries normalization now covers (/bin/bash, /bin/zsh, powershell.exe, python3, pypy3) are gone; the first two were already unreachable behind the path check above them. Covers the persisted store, the session-scoped grants the agent takes during a turn, the legacy-schema migration that prunes such a grant per entry, and the Windows spellings hermetically. * fix(sandbox): treat free-threaded CPython as a launcher too The ABI flags came from the pre-3.13 set, so "python3.13t" and "python3.13t.exe" normalized to themselves and stayed grantable as command prefixes while the GIL build "python3.13" was refused. A free-threaded interpreter runs arbitrary code just the same. Add "t" to the flags trimmed under the existing digit guard, which is what keeps ordinary names intact: "zstd" and "cat" have no digit beneath the trimmed letters, so they keep their own names and stay grantable. Reported by CodeRabbit on #934. * fix(sandbox): refuse the remaining spellings of listed launchers Same defect as the free-threaded build, found by sweeping the families already on the list for names it cannot reach: - nodejs is node on Debian and Ubuntu, and was grantable. - python3-dbg, python3.11-dbg and python3.13t-dbg are interpreters that run arbitrary code; so are bash-static, pwsh-preview and node-nightly. A closed set of build-channel suffixes now normalizes them. The set stays closed deliberately: a general "-word" strip would also swallow node-gyp, python3-config and ruby-lsp, which are ordinary tools a user should still be able to approve, and those are pinned as must-stay-grantable. - pnpm, yarn and bunx run package scripts exactly as npm and npx do, and npm and npx are already refused. - sudoedit escalates like sudo. The launcher-wrapper family (winpty, chroot, unshare, nsenter, parallel, script, unbuffer, taskset, flock) has the same inconsistency against the env/nohup/timeout/setsid entries already on the list, and is left for a separate decision rather than widened here. * test(sandbox): cover the -debug and -beta launcher suffixes Four of the six build-channel suffixes had regression cases; -debug and -beta did not, so removing either from the set left every test green. Both now fail RED when their entry is dropped. Reported by CodeRabbit on #934. |
||
|
Indra Gunawan
|
90dcfd127e |
fix(modelregistry): expose reasoning effort for DeepSeek V4 models (#931)
deepseek-v4-flash / deepseek-v4-pro expose thinking-mode effort control
through the OpenAI-compatible reasoning_effort parameter (low/high/max;
medium/xhigh map onto high). reasoningEffortsForModelName did not recognize
the deepseek family, so /effort only offered 'auto' for these models.
Advertise {low, high, max} so the /effort picker and Ctrl+T ring work, and
add fallback coverage for deepseek-v4-flash/pro/reasoner/chat.
|
||
|
Vasanth T
|
01c3498205 |
feat(update): recognise Homebrew installs and add mise, go install docs (#910)
* feat(update): recognise Homebrew installs and add mise, go install docs
* fix(update): match the keg shape, correct the docs, cover Apply and cross-target
* test(update): build the release fixture from the running platform
* fix(update): require a Homebrew receipt before refusing self-update
Review found the keg check was shape-only, and it was right: a directory
tree a user built themselves can have exactly a keg's shape. Classifying
one as Homebrew refuses a `zero upgrade` that would have worked
perfectly, on a machine Homebrew has never touched.
Two changes, cheap one first. The shape rule now requires the full
<prefix>/Cellar/<formula>/<version>/bin/<binary> depth rather than one
segment fewer, which already excludes the exact path the review raised.
Then, only for a path that still looks like a keg, one Stat for
INSTALL_RECEIPT.json — the file Homebrew writes into every keg and
nothing else does.
The receipt is what the old comment argued against, on the grounds that
it would put filesystem I/O on a path that runs at every version check.
That is no longer true of it: the Stat happens only after the shape test
has already matched, so an ordinary install still reaches no filesystem
here at all.
A directory named INSTALL_RECEIPT.json is not a receipt, which a bare
Stat would have accepted silently.
NOT taken: the same review asked for the Homebrew branch to be moved
ahead of the cross-target one in upgradeGuidance. That ordering is
deliberate and already pinned by
TestUpgradeGuidanceKeepsCrossTargetAnswerForHomebrew, whose comment
predicted exactly this reading — "it reads like an ordering bug until you
see which question is being answered". Homebrew is a property of the
binary on THIS machine; when the check was asked about a different
target the answer belongs to that other machine, and `brew upgrade zero`
would change this one instead. The reasoning is now recorded at the
branch as well as in the test, so the next reader finds it in both
places.
The install docs claimed detection was the Cellar path alone, which
undersold it, and stated the self-update rule absolutely in one paragraph
before excepting mise in the next. Both say the same thing now.
* test(update): give the Homebrew fixtures a receipt, and stop gating the receipt rule on the host
The previous commit made a receipt the thing that distinguishes a keg from any
directory sitting under a path containing "Cellar". Two fixtures still built a
keg without one, so detection correctly answered standalone and both failed:
TestDetectInstallMethodRecognisesAHomebrewKeg keg path: got "standalone", want "homebrew"
TestApplyRefusesToUpdateAHomebrewKeg refusal does not name the command that works:
download release archive: dial tcp: lookup example.test
The second is worth reading twice: with detection returning standalone, Apply
stopped refusing and went to the network, so the failure surfaced as a DNS error
rather than as a missing refusal. Both fixtures write a receipt now.
The reason this reached CI at all is the second half. Every test of the receipt
rule passed runtime.GOOS to isHomebrewPath, which already takes the target as a
parameter, so all of them skipped on Windows and the rule could only ever break
somewhere else. They pass an explicit target now and run everywhere. The two
that exercise DetectInstallMethod stay host-gated, because those are about the
wired entry point rather than the rule, but the rule underneath them is no
longer invisible here.
Verified the receipt path arithmetic on both non-Windows targets rather than
trusting the host-gated tests: isHomebrewPath is false before the receipt is
written and true after, with the receipt at the keg directory hasHomebrewReceipt
actually reads.
|
||
|
|
1ec7219aee |
feat: use Exa as default search provider (#926)
* feat: use Exa as default search provider Amp-Thread-ID: https://ampcode.com/threads/T-01a01522-a5f1-7139-8a55-5e417cba2751 Co-authored-by: Amp <[email protected]> * fix(config): carry a legacy default's disable across the Exa migration A user who ran `zero mcp disable firecrawl` chose not to open that outbound connection. Renaming the built-in default to exa re-opened it under the new name on upgrade, and because the replacement looked like an untouched default the startup warning stayed quiet too. Carry an explicit user-level disable of a retired default onto its successor, but only when the user never declared the successor themselves — an explicit exa entry wins either way. The carried-over disable is recorded as a user-level decision, so the lower-trust project layer cannot lift it while `zero mcp enable exa` still can. Regression coverage: the legacy-disabled upgrade path, the explicit-exa override, a configured-but-enabled legacy entry, the CLI re-enable, the project-layer re-enable attempt, and the resulting startup behavior (NormalizeConfig must not hand back an exa server to connect to). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(config): keep a retired default's transport for partial user entries A user who added an API key to the firecrawl default wrote only the header — the seeded default supplied the http type and URL. Once the default stops being seeded that entry resolves with no type, url, or command, and NormalizeConfig rejects it, taking down startup for every other server too rather than just that one. Record what Zero used to ship for a retired default and re-seed it under the user's own fields. Only for an entry that named no transport of its own: an entry with a self-hosted url or a stdio command is a complete definition the user owns, and grafting the retired http default onto a command produces a server that is rejected outright. Regression coverage at both ResolveMCP and NormalizeConfig for the header-only entry and its failure path, the self-hosted stdio entry that must be left alone, and a disabled retired entry keeping a usable transport so a later re-enable resolves. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Amp <[email protected]> Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
Eli Lap
|
1a81292d3f |
Fix LM studio (local) model discovery. (#918)
* Fix LM studio (local) model discovery.
* fix(openai): handle typed-nil properties in normalizeToolParameters
The `properties == nil` interface comparison misses typed-nil
`map[string]any` values, which JSON-serialize as `null` instead of
`{}`. Strict OpenAI-compatible servers (LM Studio) reject the request.
Type-assert first, then compare the underlying map, so a typed-nil
properties field is rewritten to an empty object. Adds a regression
test that exercises the typed-nil path.
* test(openai): cover non-map properties in normalizeToolParameters
CodeRabbit follow-up on the typed-nil fix: lock in the behavior
when `properties` is a non-map value (e.g., empty slice) so the
type-assert branch normalizes it to an empty object on the wire.
|
||
|
Vasanth T
|
a78c6c357e |
fix(acp): three defects a desktop ACP client hits (#915)
* fix(acp): four defects a desktop ACP client hits Found by driving `zero acp` from ZeroApp and comparing both sides of the wire. None of them fail loudly — they produce a blank transcript, a silent denial, a spurious crash, or two buttons nobody can tell apart. session/load restored the conversation into the agent's own memory and sent nothing. The turns were read from the store and used to build every later prompt, but not one notification went out, so a client that resumed opened an empty transcript against a model that remembered everything. Ask a follow-up and ZERO answers correctly, about a question no longer on screen. It now replays the loaded turns as user/agent message chunks before returning, so a client rendering on the reply already has them. Whole messages rather than per-token deltas: this is a transcript, not a stream. A permission option we offered could not be accepted. The fallback list for a request with no AvailableDecisions lived inside buildPermissionOptions, while requestPermission validated the client's answer against the raw field — so whenever ZERO did not enumerate, the client was sent Allow and Reject and its reply was checked against an empty slice. Every button it could possibly show failed closed to deny with "permission option was not offered". The user clicked Allow and ZERO recorded a denial, and nothing surfaced it, because a denial is a legitimate answer. One resolver now feeds both, so they agree by construction. The narrowing this validation exists for is unchanged: an unoffered broader grant is still refused. Cancelling a permission prompt came back as an internal error. Only context.Canceled was recognised, so errPermissionApprovalCanceled fell through to -32603 carrying the sentinel text, and clients render that as a failed turn — declining a tool looked like ZERO falling over. For apply_patch, dismissing the dialog is the only refusal a client is offered. It maps to StopCancelled now. The sentinel is exported so the surfaces can recognise it; a genuine failure is still an error. request_permissions labelled two different decisions "Allow". The label is the only thing an ACP client shows, and plain allow and strict-review allow are offered together, so the panel had two identical buttons where one silently enabled strict auto-review of what was granted. The distinction cannot live in the option id, which is opaque to the client, so it is in the name. Eleven tests, each confirmed to fail without its fix. * refactor(acp): drop the session/load replay, superseded by #914 gnanam's #914 lands the same fix and does more with it: the replayed messages carry stable ids derived from the store's event ids, session/load replays while session/resume deliberately does not, and both are capability-gated. Keeping a second, weaker replay here would have meant a conflict in handleSessionLoad and translate.go for no gain. translate.go is back to its state on main. What remains are the three defects #914 does not touch: the permission option that could not be accepted, the cancel that arrived as an internal error, and the two options labelled the same. * fix(acp): validate a permission reply against the options that were sent CodeRabbit caught a hole on the same seam this PR already fixes, in the other direction. buildPermissionOptions drops PermissionDecisionCancel, because ACP expresses cancellation through the outcome rather than an option. But ZERO enumerates cancel in AvailableDecisions for shell commands and apply_patch — the two commonest prompts — so validating against the decisions accepted {"outcome":"selected","optionId":"cancel"}: an identifier no client was ever shown, aborting the whole turn. Rather than reject cancel by name, decisionFromOutcome now takes the []PermissionOption that were actually sent, and requestPermission builds that list once and uses it for both halves. "What we offered" and "what we accept" become the same slice instead of two things kept in step, so anything optionKindFor declines to render is excluded structurally — the next such action is covered the day it is added. The regression test is written over every option-less decision rather than over cancel specifically, for the same reason. Cancelling through the outcome still works, which is asserted separately: closing the selected-id route must not close the real one. Also adds the cancellation coverage CodeRabbit asked for on the two sandbox-retry paths. Both wrap ErrPermissionApprovalCanceled and each is reached by a different denial, and the ACP mapping in this PR depends on them producing something recognisable. They also assert the escalated retry does not run: a cancelled prompt that still ran it would run the command the user had just declined. |
||
|
euxaristiaandeuxaristia
|
8dd33ec39e |
docs(agents): Record four recurring review blockers (#899)
* docs(agents): Record four recurring review blockers These classes come up across unrelated pull requests, are not caught by CI, and nothing in AGENTS.md currently prevents them, so each is rediscovered by a reviewer at the cost of a round trip. Host tool version floors is the clearest case. Zero shells out to git and the project declares no minimum version anywhere, so adding a modern flag raises that floor silently and an older host fails with a usage error instead of a clear message. Prove-the-test-fails addresses a recurring shape where the problem is not a missing test but one that passes either way, because an earlier guard rejects the input before the behavior under test runs. Hermetic tests covers isolation helpers applied to some tests but not others reaching the same storage. Platform error codes extends the existing platform-truth rule, which covers path canonicalization but not errno and NTSTATUS values. Advisory lint findings are deliberately not included: deadcode and lint-static both run with continue-on-error in ci.yml, and section 3 already requires fixing findings the change introduces. Refs #898 * docs(agents): Tighten version-floor, isolation, and errno guidance Three corrections from review, two of them to claims that were too strong. Git version floors are per subcommand, not per flag. --end-of-options reached the shared parser in 2.24 but git rev-parse only accepted it from 2.30, so recording the flag's general debut would still let a caller ship something that fails on 2.24 through 2.29. The hermetic-tests bullet named config, cache, and state but gave a procedure covering only config. It now covers each root, including %LocalAppData% for the cache root, which os.UserCacheDir resolves without consulting XDG. The errno bullet asserted ENOTDIR on Linux and Darwin. Only Linux was actually measured; Darwin was assumed, in a bullet whose whole point is not to assume. It now says the call may report either code and both should be accepted, which is what the guidance was trying to teach. Refs #898 * docs(agents): Clarify macOS and Windows roots for hermetic test isolation. os.UserConfigDir and os.UserCacheDir derive from HOME on macOS and AppData/LocalAppData on Windows rather than reading XDG variables. Refs #899 --------- Co-authored-by: euxaristia <[email protected]> |
||
|
Anandan
|
d7ac85cbf9 |
Polish Zero's TUI and harden patch, model, and background workflows (#916)
* Checkpoint UI polish * Fix structured apply patch handling * Restore tool previews when resuming sessions * Improve theme-aware code rendering * Replace persistent sidebar with run details * Remove inert transcript interactions * Cover resumed edit previews * Smooth active run status feedback * Preserve semantic colors behind overlays * Normalize ChatGPT model aliases * Render staged image attachments in terminal * Polish transcript and tool feedback * Resolve rebased diff renderer conflict * Address UI polish review findings * Fix smoke test regressions * Harden patch and transcript edge cases * Bind structured patches to workspace root * Use unique structured patch temp files * Retry structured patch temp allocation * Reuse rooted patch temp allocator * Fix Windows CI process checks * Fix structured patch safety boundaries Preserve read-before-write credit only when the model already knew the full source, reject ambiguous hunk matches, and retain CRLF line endings. Add regression coverage for unsafe overwrites, positive tracking cases, duplicate matches, and Windows-style files. * Resolve plan test seam after rebase Drop the obsolete sticky-plan height helper while retaining the PR status test seams brought forward by the UI changes. * Address final review correctness gaps Bound narrow waiting labels, reject malformed plan records, preserve missing final newlines, reject out-of-order EOF hunks, clear stale tracking after failed rollback, and keep deferred tool loading aligned with model visibility. * Harden patch transactions and review edges Publish new patch targets without overwriting raced destinations, journal only completed mutations, and create replacement temps beside their targets. Reject pre-plan diagnostics, cover tool-search transcript suppression, and leave sufficient cleanup time in the Windows process fixture. * Bind patch rollback to committed versions Capture file identity and content before publication, quarantine rollback targets atomically, and refuse destructive undo when a path was replaced or modified in place. Add deterministic coverage for publication and rollback races. * Align structured patch failure semantics Use prevalidated sequential application with explicit partial-success errors instead of unsafe multi-file rollback. Retain atomic per-file publication, no-overwrite creates, unsupported-hardlink fallback, and conservative tracker invalidation. * Preserve patch context bytes Track source offsets for structured-patch context lines and reuse the exact matched source text during replacement. This keeps tolerant matching from changing indentation or mixed line endings outside the requested edit. |
||
|
|
d66ad71532 |
chore: remove dead wrapper functions and unused test seams (#905)
* chore: remove dead wrapper functions and unused test seams These five functions are unreachable from any cmd/* main even when tests are included (deadcode -test ./...), so they are genuinely dead on every platform: - TraceEventKeys: one-line alias of ParseTraceEventKeys, no callers - ValidateToolCapabilities: one-line alias of ValidateCapabilities, no callers - GetLocalDiffStats and WatchPRState: test seams in internal/tui/export_test.go that no test calls - ScanSSEData: non-context SSE scanner in internal/providers/providerio, fully superseded by ScanSSEDataWithContext The five remaining -test findings are //go:build !linux stubs (landlock/seccomp/linux_helper) that are live on Linux, so they are left in place. Co-Authored-By: cairn-code <[email protected]> * Bump Go to 1.26.6 so govulncheck and Windows smoke pass. The 1.26.5 standard library still reports GO-2026-6218 and the Punycode label check that fail required CI; 1.26.6 matches main and clears them. --------- Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
|
d065467cd3 |
feat(zerogit): auto-create a conventional branch before push/pr on default branch (#671)
* feat(zerogit): auto-create a conventional branch before push/pr on default branch
zero changes push/pr refused to push straight to the default branch but
never offered an alternative, unlike other agent tooling that names and
checks out a feature branch automatically. Add CreateBranch, IsDefaultBranch,
CurrentGitUser, and branch-name slugify/build helpers to zerogit, and wire
push/pr to auto-create a "<user>/<slug>" branch (slug from an LLM when a
provider is configured, otherwise a deterministic fallback from the diff)
whenever the current branch is the default one and --yes/--dry-run weren't
passed.
* fix(zerogit): normalize LLM branch slugs, bound remote lookup, handle existing branches
Address CodeRabbit's review on the auto-branch-naming PR:
- generateAutoBranchSlug now takes the first non-empty, quote-trimmed
line of the model's response before slugifying, instead of folding
any preamble or wrapping quotes into the branch name.
- isDefaultBranch's ls-remote lookup is now bounded by a 5s timeout so
a slow or unreachable remote can't stall push/pr; falls back to the
local main/master heuristic as before.
- CreateBranch checks whether the target branch already exists locally
and checks it out instead of failing checkout -b on the collision.
Also add regression coverage: CurrentGitUser's OS-username fallback
tier, CreateBranch's existing-branch and generic checkout-failure
paths, a CLI-level test for changes pr (previously untested) mirroring
the existing changes push coverage, and a messy-LLM-response case for
the slug normalization fix.
* fix(zerogit,cli): make auto-branching collision-safe, remote-aware, fail-closed, and opt-in for LLM naming
- CreateBranch never checks out an existing branch on a name collision: it
picks a unique suffixed name off the current HEAD (name-2 .. name-9) and
fails visibly when the namespace is exhausted, so a stale branch with
unrelated history can no longer be published while the new commit stays
behind on the default branch.
- ensureFeatureBranch resolves the remote once (explicit --remote, then the
original branch's configured upstream, then origin) via IsDefaultBranch,
which now reports the resolved remote, and push/pr thread that remote into
Push, so a freshly created branch without tracking configuration no longer
falls back to origin in fork setups.
- The default-branch check fails closed: main and master count as default
with no network, an unreachable remote falls back to the local
refs/remotes/<remote>/HEAD record, and when neither answers the push is
refused with guidance instead of silently dropping the confirmation guard
for repositories whose default is trunk or develop.
- LLM branch naming is now opt-in via --auto on push/pr, matching commit
--auto: a configured provider alone no longer causes the change diff to be
uploaded during ordinary git-only pushes.
- After the ordinary commit-then-push sequence the tree is clean, so the
fallback name comes from the HEAD commit subject instead of the constant
user/changes.
* style(zerogit): align fake-runner result comments to gofmt output
* test(zerogit): cover Push's fail-closed path when the default branch is unknown
* fix(zerogit): keep a stale cached remote HEAD from clearing the push guard
When the live ls-remote default-branch lookup fails, isDefaultBranch fell
back to the local refs/remotes/<remote>/HEAD cache and returned
branch == cachedName. That cache is only a hint: if the server renamed its
default (main -> trunk) the record still names main, so checking whether
pushing trunk is safe returned false and the newly protected branch was
pushed without --yes.
Trust the cache only to block a push (a positive match proves the branch is
the recorded default); on a mismatch fall through to the existing
fail-closed unknown-default error instead of treating the branch as
unprotected.
* fix(cli): honor --diff-bytes when inspecting the diff for auto-branch naming
changes push/pr accept and document --diff-bytes, but ensureFeatureBranch
inspected the working tree with only Cwd set. With --auto the resulting
unbounded diff was embedded in the branch-name request to the provider, so a
user who capped the diff to limit how much proprietary source is uploaded
for naming still sent the complete diff.
Thread options.maxDiffBytes through ensureFeatureBranch into InspectOptions,
matching how the commit path already bounds the diff.
* fix(cli): refuse an auto-branch push when HEAD has nothing to publish
The auto-branch path decided to branch solely from working-tree status and
never checked that HEAD was ahead of the default branch. On a clean,
up-to-date default branch it would create and push user/<subject> at the
exact default tip; with only uncommitted edits it named a branch from those
edits but pushed the unchanged HEAD, leaving the edits local, and changes pr
then abandoned the empty branch before the host rejected the comparison.
Add zerogit.CommitsAhead and consult it before branching: when HEAD is
provably not ahead of <remote>/<default>, report "no changes to publish"
instead of creating the branch. An indeterminate count (for example a
remote-tracking ref that was never fetched) is treated as "cannot tell" so a
legitimate first push is not blocked.
* fix(cli): recover the branch slug from preamble or fenced LLM replies
The slug helper returned the first non-empty line verbatim, so a reply like
"Here is a suggested branch name:\nadd-login-page" produced
user/here-is-a-suggested-branch-name, and a reply wrapped in a code fence
slugified the ``` line to nothing and silently fell back to the local name.
Drop Markdown code-fence lines and prefer a line that already reads as a
kebab-case slug, falling back to the first quoted/plain line only when no
slug-shaped line exists. Cover preamble and fenced responses with tests.
* fix(zerogit): harden auto-branch remote handling
Three review findings:
- isDefaultBranch passed the remote to ls-remote before the HEAD
positional with no --, so a remote value shaped like an option
(--upload-pack=/bin/echo) was parsed as one. Terminate options with --
and cover a dash-prefixed remote.
- An unborn remote (freshly created, zero refs) answered ls-remote with
empty output, fell through to the fail-closed unknown-default error,
and made the very first feature-branch push a --yes dead end that
git remote set-head --auto cannot repair. ls-remote succeeding with no
refs now counts as proof there is no protected default; main/master
stay guarded by the name heuristic and every failure path stays
fail-closed.
- CreateBranch probed only local refs/heads for collisions, so a branch
existing only on the target remote (an old merged-PR branch) was
silently fast-forwarded by the later push -u. The target remote's
heads are now probed once (bounded, with --) and count as taken;
an unreachable remote fails visibly before anything is created.
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(zerogit): close branch-naming and push safety gaps from review
Require a confirmed unborn HEAD, not just an empty ls-remote --symref,
before granting the first-push exception, since a dangling remote
HEAD looked the same. Resolve branch naming and ahead-count checks
against the remote branch instead of a working-tree snapshot, which
could describe uncommitted edits a commit-only push would never
publish. Guard concurrent branch creation with a force-with-lease
push. Scope the ls-remote timeout to the default-branch preflight
only, so it no longer caps Push's own unrelated check.
* fix(cli): require a clean tree before auto-branch push/pr
Refuse ensureFeatureBranch when the working tree is dirty or the
ahead count against the remote default cannot be determined, so a
default-branch push cannot leave uncommitted edits behind or publish
an empty comparison under a guessed name.
* fix(zerogit): drop fixed five-second remote lookup timeout
IsDefaultBranch and CreateBranch applied a hard 5s bound on ls-remote,
so ordinary feature-branch pushes and the collision probe failed on slow
but reachable remotes (SSH/VPN) before Push could run. Honor the
caller's context instead; callers that need a bound pass a deadline.
* fix(cli): bypass the ahead-count and diff-base checks on a confirmed-unborn remote
jatmn flagged that the confirmed-unborn-remote exception this PR added to
IsDefaultBranch never actually helps the main/master case it was written
for: when the local branch is named main, isDefaultBranch returns true from
the name heuristic before it ever consults the remote, so
ensureFeatureBranch still unconditionally calls commitsAhead against
<remote>/<branch>. On a genuinely empty remote that ref cannot exist, so
`zero changes push`/`pr` from a local default branch against a brand-new
empty remote still dead-ended with "cannot determine whether HEAD is
ahead of origin/main" - the exact bug this PR set out to fix.
Tracing the flow further, the same problem also hit the diff-base Inspect
call right after: it uses the same nonexistent <remote>/<branch> ref, so
bypassing only commitsAhead would have left the dead end one line later.
ensureFeatureBranch now probes the new zerogit.IsUnbornRemote (ls-remote
--heads on the remote) whenever commitsAhead fails, and only bypasses both
checks when that probe positively confirms the remote has no refs at all.
An unreachable remote, or one that answers with refs, still fails closed
exactly as before - the fail-closed "unknown default branch" behavior is
unchanged and covered by TestEnsureFeatureBranchFailsWhenAheadCountUnknown
and the new TestEnsureFeatureBranchFailsWhenUnbornCheckErrors.
TestEnsureFeatureBranchCreatesBranchOnConfirmedUnbornRemote is the
regression test: confirmed failing (with the exact dead-end error) against
the pre-fix logic, passing after.
* fix(zerogit): resolve main/master against the live remote default first
isDefaultBranch short-circuited on the literal branch names main/master
before ever consulting the remote. A repository whose remote default is
genuinely something else (e.g. trunk) could still have a local main branch
tracking that remote, and the shortcut wrongly treated it as protected
instead of trusting the live symref result. The conventional-name check
now only applies as a last-resort fallback, after both the live ls-remote
symref lookup and the local remotes/<remote>/HEAD cache fail to answer.
* fix(cli): preserve push lease on retry and check the live default ref
Two related gaps in ensureFeatureBranch's auto-branch preflight:
- When an auto-branch push loses a force-with-lease race against a
concurrent creator, the generated branch is left checked out locally
with no successful push behind it. A retry took the "already off the
default branch" early return and dropped RequireNewRemoteBranch,
letting the next push silently fast-forward whatever the concurrent
creator published. The lease now stays required unless the branch
already has a configured upstream (proof a push already landed).
- The ahead-of-default check compared HEAD against the local
remote-tracking ref, which is only refreshed at clone or the last
fetch. A stale ref can under-report how far ahead HEAD is, or say
zero when the remote has actually moved past it, so ensureFeatureBranch
now refreshes that tracking ref before trusting it, and fails closed
if the refresh itself cannot be confirmed.
Adds regression coverage for both: a retry after a lease collision keeps
requiring the lease (and an already-published branch does not), and a
stale tracking ref is refreshed before the ahead check runs (with a
fail-closed case when that refresh cannot be confirmed).
* fix(cli): track generated branches, reject PR on unborn remote, classify main on unborn
* fix(zerogit,cli): preserve explicit nonexistence lease on force, propagate marker write errors, check unborn remote before branch switch
* fix(zerogit): preserve non-existence lease on target remote and roll back failed branch marking
Compare target remote against configured upstream remote to retain non-existence lease on target, roll back created branch if marking fails, and allow --yes to bypass PR default-branch preflight.
Refs #671
* fix(cli): address review findings for auto feature branch creation
* fix(cli): refine LLM preamble candidate slug extraction and enforce unborn-remote guard
* fix(cli): restore default branch after auto-branch and resolve --yes upstream
Do not leave auto-branch commits on the protected local default branch,
and resolve the real upstream for the unborn-remote guard under --yes
instead of always probing origin.
* test(cli): assert auto-branch passes capped diff into the provider
Thread --auto and a mock provider through DiffBytes coverage so the
cap is checked on the real LLM request path, not only Inspect.
* fix(cli): block auto-branch on unborn remotes and fix lease/restore
Refuse auto-creating a feature branch when the target remote has no refs yet.
Publishing only user/slug leaves remote HEAD dangling, so the next changes pr
fails closed and a later main push has no PR diff. Require push --yes first.
Keep the nonexistence lease unless branch@{upstream} is exactly the target
remote/generated-branch ref from a successful push -u. Inherited origin/main
from branch.autoSetupMerge=inherit must not drop the lease.
Restore the original default branch against its own upstream tip, not the
push destination, so --remote upstream does not rewrite a fork main that
tracks origin/main.
* fix(cli): recover push -u upstream and filter LLM slug preambles
Push -u can publish a remote branch then fail to write local upstream
config. Treat that as incomplete, recover with set-upstream-to, and probe
the remote before reasserting the nonexistence lease on retry. Also skip
one-word LLM acknowledgements before accepting a slug-shaped line.
Refs #671
* fix(cli): decide the branch-collision lease with one live remote check
Replace the local-state inference chain (upstream config, a
zeroAutoBranch marker, per-remote provenance tracking) that kept
producing new collision-safety edge cases across many review rounds
with a single live remoteHasBranch check against the destination
remote: missing there gets the nonexistence lease, already there gets
a plain push, and git's own fast-forward check is the safety net if
that existing branch isn't actually ours.
Removes MarkGeneratedBranch/IsGeneratedBranch, now dead, along with
the tests that pinned the specific local-state heuristics rather than
the collision-safety property itself.
* fix(cli,zerogit): address CodeRabbit findings on auto-branch naming
Recover trailing-punctuation LLM slugs, notice deterministic fallback,
report dual restore/rollback failures, and skip Push upstream repair on
dry-run. Align comments with fail-closed and DryRun behavior.
Refs #671
* fix(tools): set explicit stdin reader and use resolved binary for format-on-write
Use the binary path resolved by exec.LookPath and set formatter.Stdin
to an empty reader so format-on-write does not hang on inherited stdin
handles on Windows.
Refs #671
* fix(cli,zerogit,tools): address CodeRabbit review findings on PR 671
Remove redundant command validation, eliminate duplicate remote lookup in PR
workflow, harden ref options and branch switching, isolate test Git
environment, and add formatter lookup test.
Refs #671
* fix(cli,zerogit): address new CodeRabbit review findings on PR 671
Harden CreateBranch name validation, remove unreachable workflow error check,
complete doc comment, isolate test Git environment, and clean up unused
appDeps field.
Refs #671
* fix(cli,zerogit,tools): address third round of CodeRabbit review findings on PR 671
Use featureBranchOptions struct for ensureFeatureBranch, bound preflight
contexts, add upstream repair mismatch subtest, and centralize test Git
config isolation.
Refs #671
* fix(cli,zerogit): finish ensureFeatureBranch doc contract and close review gaps
The ensureFeatureBranch doc comment ended mid-sentence and sat on the featureBranchOptions type, so go doc attached function prose to the struct. Move it above the function and state which deps are required non-nil (inspectChanges, commitsAhead, headCommitSubject, currentGitUser, createBranch) versus optional and nil-guarded (refreshTrackingRef, isUnbornRemote, branchUpstreamRef, resetBranchRef, deleteBranch). Also extract the branch slug filename with path.Base because git paths are always slash-separated, gate two tests on the git version their fixtures need (autoSetupMerge=inherit requires 2.35, init --bare -b requires 2.28), and cover the literal "user" CurrentGitUser fallback via a test seam.
Refs #671
Co-Authored-By: cairn-code <[email protected]>
* Protect auto-branch publication and default ref restoration
Co-Authored-By: cairn-code <[email protected]>
* fix(cli): wire expected-old restore through the dependency seam
The P1 fix added expectedOld to resetBranchRef but left the appDeps field and the workflows.go import behind, so the CLI package did not compile. Update the field type to the 5-argument signature and restore the filepath import used by worktree release resolution.
Co-Authored-By: cairn-code <[email protected]>
* fix(cli,zerogit): wire CAS tip and close review test gaps
Addresses the CodeRabbit round on
|
||
|
Vasanth T
|
0eab63c908 |
docs(agents): record the normalize-before-match blocker (#900)
Three separate changes hit this in a fortnight, each written by someone being careful about exactly the thing that got them. A transform that removes bytes without leaving a gap is also a reassembler. Redaction that matches by shape, run before a sanitizer that strips control bytes, lets a credential split by a NUL or an ESC pass the patterns as two fragments and be rejoined on the way out: the MCP failure reason in #835, and the imported-transcript chokepoint in #878. The path form of the same mistake is comparing where a handle landed against a value produced by the same resolver the kernel just used, so a redirect agrees with itself: the ACL guard in #808, where junctions were caught only by an accident of Go's mode bits and directory symlinks were not caught at all. The unsplit value passing is what makes it survive review, so the note says the test needs a split case. |
||
|
|
64a783b08c |
fix(dictation): redact API keys from streaming transcriber errors (#852)
* fix(dictation): redact API keys from streaming transcriber errors Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <[email protected]> * fixup(dictation): address review: redact session/write errors, move redaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs #710 * fixup(dictation): sync Deepgram redaction test on CloseStream before close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs #710 * fix(dictation): preserve context.Canceled through streaming error redaction Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(dictation): close remaining cancellation and redaction gaps Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests. * test(dictation): cover dial and startup cancel identity Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check. * fix(dictation): prefer cancel over a racing OpenAI realtime error Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb/zero#710 * fix(dictation): prevent auto-submit on cancelled realtime race handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race. * test(dictation): exercise writeErrCh cancellation branch in realtime transcriber * fix(dictation): add custom key header redaction and single-pass redaction tests * fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test * fix(dictation): initialize non-zero session ID and filter stale session 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs #710 * fix(dictation): redact API keys from streaming transcriber errors Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <[email protected]> * fixup(dictation): address review: redact session/write errors, move redaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs #710 * fixup(dictation): sync Deepgram redaction test on CloseStream before close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs #710 * fix(dictation): preserve context.Canceled through streaming error redaction Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(dictation): close remaining cancellation and redaction gaps Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests. * test(dictation): cover dial and startup cancel identity Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check. * fix(dictation): prefer cancel over a racing OpenAI realtime error Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb/zero#710 * fix(dictation): prevent auto-submit on cancelled realtime race handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race. * test(dictation): exercise writeErrCh cancellation branch in realtime transcriber * fix(dictation): add custom key header redaction and single-pass redaction tests * fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test * fix(dictation): initialize non-zero session ID and filter stale session 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs #710 * fix(dictation): drop providerio widen; pin session and redaction tests Revert the Token/X-Api-Key providerio.Redact expansion (belongs in its own PR with coverage). Make Deepgram single-pass, cancel-race, and live-session partial tests pin the branches they name, and restore the ST1005 nolint on the user-facing Deepgram error string. * fix(dictation): ignore stale batch startup by session ID Gate dictationStartedMsg on session ID so a late Start result after cancel cannot reset a newer recording. Rename and comment cleanups from review; race suite recorded on the PR body. * fix(dictation): exit writers on cancel and pin dial/write redaction paths Select on ctx.Done while waiting for audio chunks so cancelled streaming sessions do not leave writer goroutines blocked on an open idle channel. Add non-cancellation redaction tests for Deepgram dial failure and OpenAI Realtime dial, session-configuration, and async writer failures. Refs #852 * fix(dictation): close the realtime socket on writer failure so a silent server cannot strand the read loop CodeRabbit review 4885680006 flagged that the OpenAI Realtime writer goroutine pushed write errors to writeErrCh and returned without closing the connection, so conn.Read stays parked forever when the peer goes silent after session setup and the error is never drained. Close the socket on both the append and commit write failure paths (CloseNow, since the close handshake itself would block on a silent peer), and add a silent-server regression test. Refs #852 Co-Authored-By: cairn-code <[email protected]> * test(dictation): cancel the silent-server writer failure test during teardown CodeRabbit review 4909044011 identified that the silent-server regression test used context.Background(), allowing its goroutine to remain blocked in conn.Read after a timeout. Give the test a cancellable context and defer cancellation so teardown always releases the blocked read. Refs #852 Co-Authored-By: cairn-code <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
|
9c292092a2 |
fix(daemon): clean up child after startup timeout (#774)
* fix(daemon): clean up child after startup timeout * fix(daemon): terminate startup process tree * test(background): accept reaped-later zombie children * fix(background): retain configured process group target * fix(background): address termination review feedback * fix(daemon): use launch-time group identity, fix zombie/reap classification Addresses jatmn's three #774 review findings: - TerminateCommand rediscovered the process group via execution.TerminateProcessTree's Getpgid(pid) lookup. On Darwin, that lookup can return ESRCH once an unreaped group leader has already exited, even though live descendants remain in the group it configured at launch — silently falling back to signalling only the (already dead) leader PID and leaving descendants running (the required macOS smoke job's failure). Add execution.TerminateProcessGroup, which signals the negative PID directly using the launch-time Setpgid invariant instead of rediscovering it, and route TerminateCommand through it. - signalTargetRunning's zombie check matched the individual-PID fallback target (a process that is not its own group leader) against the process table by PGID. That target's actual PGID differs from its own PID by definition, so no row ever matched, unknown resulted every time, and a zombie individual-PID target was always conservatively reported as still running. Branch on the sign of the target: group members by PGID, individual PIDs by PID. - TerminateCommand returned the termination error even when the subsequent reap succeeded — a successful cmd.Wait is authoritative proof the leader is gone and collected, regardless of how the kill attempt itself went (e.g. taskkill /T racing an already-exited PID on Windows). Classify a successful reap as success. All three 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(daemon): harden command termination inputs Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-abb8-73de-9489-51e9e6514c6b Co-authored-by: Pierre Bruno <[email protected]> * fix(background): preserve process tree failures Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * docs(daemon): clarify boundary-probe and Setsid-guard comments per PR #774 review Co-authored-by: Cursor <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]> Co-authored-by: Cursor <[email protected]> |
||
|
KRATOSandClaude Opus 4.8
|
060d38ce71 |
feat(security): path-jail primitive, credential-store locking, worktree git hardening (#891)
Split 1/3 of #829, carved out as the independently-reversible hardening layer with no dependency on the orchestration work that stacks on top of it. - internal/pathjail: os.Root handle-relative path confinement — ancestor traversal is confined, the final component is checked for reparse points (including a name written with a trailing separator), unprivileged Windows junctions are covered, temp-name fragments are rejected before they can escape the directory, whitespace-bearing paths are confined as spelled, and temp files stay unpredictable and exclusive. - internal/credstore: file locking around the credential read-modify-write — exclusive for writers, shared for readers, so a reader cannot race a publish. - internal/worktrees: git invocation hardening. pathjail has no importers in this PR by design; internal/memory and internal/specialist adopt it in the stacked PRs (2/3 and 3/3). Origin-Session: local-de382f | Claude Code | 4 prompts Origin-Snapshot: 52db9b74dad5 Co-authored-by: Claude Opus 4.8 <[email protected]> |
||
|
Anandan
|
04fd3c01a7 |
Improve diff viewer rendering (#902)
* Improve diff viewer rendering * Address diff viewer review findings * Fix diff viewer interaction regressions * Preserve header-like diff content * Fix diff hunk parsing and image vulnerability * Strengthen diff blank-context coverage * Fix diff rendering edge cases |
||
|
Vasanth T
|
dc15e82298 | fix: bump Go to 1.26.6 for stdlib vulnerability fixes (#903) | ||
|
Anandan
|
2d2450e9a7 |
fix(providers): discover ChatGPT capabilities (#890)
* fix(providers): discover ChatGPT capabilities * fix(providers): preserve discovered capabilities * test(agent): cover service tier switching * fix(tui): restrict fast mode to ChatGPT |
||
|
|
cabfeefc4e |
security(update): stage binary replacement at an unpredictable, exclusive path (#751)
* security(update): stage binary replacement at an unpredictable, exclusive path Fixes #742. installBinary staged downloaded release bytes at the fixed <target>.new path and opened it with O_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged process, that process could pre-create <target>.new as a hard link or reparse point to another file the (possibly elevated) updater can write; the truncating open then overwrote that unintended file with verified Zero executable bytes. stagingFilePath now generates a cryptographically random suffix so the path can't be targeted in advance, and platform-specific createStagingFile opens it exclusively without following a pre-existing link: O_CREATE|O_EXCL on POSIX (which POSIX guarantees fails on a pre-existing path, symlink or not, without resolving it), and CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus a post-open GetFileInformationByHandle check on Windows (not a reparse point, not a directory, single hard link). replace_windows.go's rename-swap sequence is unchanged: the dangerous operation was always the truncating open of the staged path, not the .old rename/removal that follows. Co-Authored-By: Claude Sonnet 5 <[email protected]> * security(update): bind the binary swap to the staged object, not its pathname Addresses the two open findings on #751. [P1] The staged file is no longer promoted by pathname. Randomizing the name and creating it exclusively stops PRE-creation, but under #742's model — a lower-privileged principal that can write in the installation directory — the name can be observed after the fact and the entry replaced between the write and the swap, so the updater would install the substituted file. installBinary now keeps the creating handle open and promotes through it: - Windows renames through the handle itself (SetFileInformationByHandle/FileRenameInfo, hence the added DELETE access), so there is no second pathname lookup to win. replaceBinary is gone; promote owns the aside-rename of the running binary and its restore-on-failure retry. - POSIX has no rename-by-descriptor, so staging moves into a private directory created next to the target by os.MkdirTemp (mode 0700, random name, created exclusively). An attacker who can write in the installation directory cannot create, replace, or list entries inside it. promote additionally sets the executable bit through the handle (os.Chmod would re-resolve the path) and fails closed if the entry ever stops naming the object it wrote. [P2] Staging cleanup now covers every failure path: the removal defer is registered before the copy, and stageBinary discards the object if the copy fails, so a mid-write error (ENOSPC, unreadable source) no longer leaks a release-sized file that the next attempt will not reuse. CleanupStaleBinary also sweeps leftovers from a hard crash — staging files on Windows, staging directories on POSIX, where it used to be a no-op — skipping anything younger than an hour so a concurrent update is never disturbed. Tests: substitution-race regression per platform (Windows asserts the verified bytes are installed despite a replaced staging entry; POSIX asserts the private 0700 directory and that promote refuses a substituted entry), success controls, cleanup-on-failure, and the crash-leftover sweep. The forced-staging-failure test for helper refresh now uses a stageBinary seam, because the staging location can no longer be occupied from outside — which is the point of the fix. * security(update): close remaining updater staging races Address jatmn's review findings on PR #751: - Windows promote() now verifies targetPath is actually reachable after a reported-successful handle rename before trusting it. Some Windows versions have been observed accepting SetFileInformationByHandle against a handle whose directory entry was substituted out from under it without the object actually moving, which let promote report success while targetPath was left missing entirely. renameFileByHandle is now a package var so this is covered by a deterministic regression test rather than relying on reproducing the exact trigger condition. - When a promotion failure's restore-to-.old also fails (a writable-parent attacker occupying targetPath with a lock MOVEFILE_REPLACE_EXISTING can't get past), that combination is now wrapped in ErrTargetPossiblyTampered instead of reading like an ordinary failed update, with a best-effort MOVEFILE_DELAY_UNTIL_REBOOT fallback so an admin-context updater can still recover the original at next boot. - POSIX promote() now binds its final rename to a directory descriptor opened when the staging directory is created (unix.Renameat), not a pathname. The 0700 staging directory protects its contents from a writable-parent principal, but not its own directory entry — that principal could rename the staging directory aside and recreate a look-alike with an attacker file at the same basename in the gap between the identity check and the rename, which a plain os.Rename would silently follow. - Stale-cleanup on both platforms now matches the exact generated artifact shape (POSIX: prefix + os.MkdirTemp's all-digit suffix; Windows: prefix + 32 lowercase hex chars + ".new") instead of a loose prefix/suffix, so a user's own similarly-named file or directory is never swept up. Both also re-check identity immediately before deleting to shrink the window a writable-parent principal could swap the checked path in. - copyFrom now copies in chunks and refreshes the POSIX staging directory's mtime between them, since writing into an already-created file never touches the parent directory's own mtime — a large or slow copy could previously look abandoned to a concurrent update's cleanup sweep while still in progress. Verified on real Windows and Linux (via WSL), plus cross-compiled build/vet checks for darwin/linux-arm64. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(update): address staging review findings * fix(update): verify promoted object identity safely Bind POSIX staging creation and both rename endpoints to directory descriptors, clean only identity-matched empty directories, and verify Windows promotion by file identity rather than path spelling. Preserve unverifiable crash leftovers and add adversarial cleanup, hard-link, and reparse-ancestor coverage. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-27da-72ec-8e6d-5d43f127b6a1 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): preserve promotion recovery signals Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): keep the recovery copy a failed restore promised Two error-path follow-ups from the #742 staging review. When a promotion fails and restoring the original also fails, the error tells the operator their original is preserved at <binary>.old — and then the next Apply deleted it, because a present target read as proof the copy was stale. What occupies the target in exactly that case is the bytes the updater could not verify, so cleanup was erasing the one it could. A failed restore now marks the copy, cleanup honors the mark, and a successful promotion clears it. The POSIX staging flow could also leave an empty .zero-stage-* directory behind when the stat immediately after mkdirat failed. Nothing knows that name yet, so it would have stayed for good; it is now removed best-effort with rmdir semantics, which cannot touch anything but the empty directory just created. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): stop the recovery copy from being destroyed by the next attempt The marker added in d9523ba kept cleanup from deleting <target>.old, but four paths still undermined the promise it was meant to make. Promotion destroyed the copy it was preserving. Skipping the pre-rename cleanup was not enough: os.Rename uses MOVEFILE_REPLACE_EXISTING on Windows, so renaming the running binary aside overwrote the last verified copy with the unverified bytes the earlier failure left at the target — and a promotion that then failed could only move those unverified bytes back. Promotion now refuses while the tamper state is unresolved. Only the operator can say whether the file at the target is theirs, so the error names both moves that end it: restore the copy, or delete the marker to accept what is installed. The marker itself was a predictable link-following truncate write. Its path is fixed, so a writer in the install directory could pre-create it as a hard link or reparse point and have the elevated updater write through it. It is now created with the same CREATE_NEW + FILE_FLAG_OPEN_REPARSE_POINT + fresh-regular- file check as a staging file, and never truncates; an existing name is treated as already-marked rather than an object to open. A marker that could not be written left the promise unqualified while the next run's cleanup deleted exactly the file it named. That failure now rides in the error, telling the operator to copy it somewhere safe now. Helper refreshes downgraded tampering to a warning. An ordinary helper failure still warns — a stale helper is better than a failed update — but a helper whose path may hold unverified content fails the apply, because helpers are resolved from the install directory and executed by the sandbox runner, and reporting Applied: true would hand the operator a success while a sibling executable is suspect. ErrTargetPossiblyTampered moved to apply.go so callers on every platform can test for it without build tags. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): keep the recovery copy when its marker cannot be established Surfacing a failed marker write told the operator to hurry; it did not stop the next run from deleting the file they were being told to save. When no marker can be established, the copy is now moved to an unpredictable sibling name that routine cleanup never touches — CleanupStaleBinary only ever removes the exact "<target>.old" — and the error names where it went. Telling the operator to act now remains the fallback for when even that move fails. The marker check is conservative in the same direction: only a definite "not there" allows the copy to be deleted. An Lstat that fails for any other reason leaves the question open, and deleting is irreversible while keeping costs one stale file. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): preserve Windows recovery state across retries Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> * fix(update): watch every recovery path and never orphan a partial marker A failed second-or-later update marks the randomized aside it used, but promotion only consulted the canonical <target>.old marker, so a retry proceeded over an unverified target while the verified copy sat marked on a path the refusal logic did not watch. Promotion now enumerates every recovery candidate — canonical and randomized asides — and refuses while any of them is marked, naming each recovery path and its marker. The missing-target refusal had the same single-path blind spot: with a stale canonical .old from an earlier successful update plus the aside from an interrupted attempt, it named only the canonical path, which can be the wrong binary. It now names the single actual candidate or refuses the ambiguous layout outright until the operator resolves it. A failed marker write could also leave a partial .keep behind while the recovery copy was relocated elsewhere; later refusals then named a location that no longer held the verified bytes. markOldBinaryPreserved now removes the entry it created before reporting a write/close failure, and conservatively reports success when that removal (or the state check) fails so oldPath stays authoritative. keepUnmarkedRecoveryCopy is handle-bound end to end: it pins the recovery copy with a no-delete-sharing, no-reparse open, verifies it is a single-link regular file, renames through the handle with ReplaceIfExists false, and confirms identity at the destination, failing closed with ErrTargetPossiblyTampered on any race. Finally, the error for an unestablishable marker still promised that a later update would otherwise remove the copy, but CleanupStaleBinary is now a no-op; the operator is told that a manual copy is required. * fix(update): tolerate the fail-closed outcome of a fully-unlinked substitution TestPromoteInstallsTheStagedObjectNotTheStagedPath deletes the staging file's only directory entry and recreates it as an "attacker" file, then required promote to succeed with the verified bytes installed. CI's windows-latest runner permits that delete (this workstation's exclusive share mode blocks it, so the path went unexercised here); on that runner, renaming the now fully-unlinked staging handle back into existence is not something every Windows build honors, so verifyPromotedTarget's post- rename identity check finds nothing at targetPath, promote fails, and restoreOriginalBinary moves the pre-update binary back — a safe, fail- closed outcome, not a security regression. The test now accepts either outcome after a real substitution: promote succeeding with the verified bytes, or promote failing as long as the attacker's substituted bytes were never installed. Any promote failure that occurs without substitution having actually happened still fails the test outright. * fix(update): surface unverified recovery path, match .old case-insensitively keepUnmarkedRecoveryCopy discarded the destination path when the rename to it succeeded but the post-rename identity verification failed, leaving restoreOriginalBinary's error telling the operator to save oldPath after it had already been vacated. Return the attempted kept path alongside the verification error so the caller can still name it. existingRecoveryPaths compared "<target>.old" filenames case-sensitively, but NTFS is case-insensitive/case-preserving, so a recovery or marker file spelled e.g. "zero.exe.OLD" could be silently skipped by every caller's fail-closed check. Fold both sides before matching. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(update): bound Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbdec-f8dc-71f9-abdd-ea044a902b9a Co-authored-by: Pierre Bruno <[email protected]> * fix(update): secure Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): terminate Windows rename paths Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * test(update): allow namespaced recovery copies Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): bind Windows cleanup to handles, drop dead cleanup API Failure-path staging cleanup on Windows re-resolved the staging pathname after the exclusive handle was closed, so a principal who can write in the installation directory could substitute that entry in the gap and have the updater delete a file of their choosing. Removal is now requested through the handle (FileDispositionInfo) before it is released, and nothing is removed by name afterwards; the same change covers the staging-file verification failure path and the partial recovery marker. CleanupStaleBinary has had no production caller since bounded, identity-bound cleanup moved into promote, so the exported no-op and the tests that could only pass vacuously against it are removed. The POSIX link-regression tests now drive createStagingFileAt, the primitive createStagedBinary actually uses, instead of a path-taking helper kept alive only for them. Also documents the fail-closed Windows recovery states in docs/UPDATE.md, including the refusal a planted .old/.keep pair can cause. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): bind Windows recovery to handles and bound its state Addresses the 2026-08-02 review round on #751. Keep the recovery restore bound to the object promote moved aside instead of its pathname: promote now opens the target with delete access and no delete sharing before renaming it aside, and restoreOriginalBinary renames that same handle back (retrying through the handle, never re-resolving the source name), so a directory writer cannot swap the aside entry during the promotion gap and have the substitute installed. Run the recovery-state checks as a standalone-install precondition, under the same lock promotion uses, so "already up to date" can no longer report success while a .keep/.recovery state is unresolved. The in-promotion check stays to close the race. Make the cleanup record a queue: entries are only retired once their object is actually gone, so a copy held open by a scanner is retried by a later update rather than leaking, while records for paths that vanished or that this updater could never have created are retired instead of accumulating. Also from review of the above: - delete the pathname restore chain that lost its production callers (restoreOriginalBinary's pathname form, openIdentityFile, recordRecoveryCleanup) and point their tests at the live handle path - name the relocation path in the error when the post-move verification fails, since oldPath is already vacated by then - scope the recovery docs to Windows and describe POSIX replace semantics Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
AnandanandVasanthdev2004
|
0c1de25a04 |
Add interactive terminal companions (#887)
* Add terminal companions * Polish interactive terminal companions * Fix terminal companion edge cases * Strengthen terminal companion regression tests * Preserve config layout when selecting pets * Handle sparse pet preference layouts * fix(terminalpet): reset SGR before erasing a sixel companion On Windows Terminal the companion draws a filled black rectangle over the interface, and dragging it mangles text under and around it. It looks like a platform quirk. It is an untested code path that only Windows reaches. Sixel is the only protocol here that erases by PAINTING CHARACTERS. Kitty hands the terminal an image id and the placement goes away. A sixel is already in the cell grid, so clearRenderedImage writes literal spaces over the cells it occupied. Those spaces are ordinary characters and carry whatever SGR the TUI last set, and nothing normalised it, so every erase stamped a filled block in the ambient background. A drag erases and repaints on every pixel, so the block was continuously refreshed over the UI. It stayed invisible everywhere else because ghostty, kitty, WezTerm and iTerm all resolve to the Kitty protocol and delete by id, never reaching this code. Windows Terminal is the only mainstream terminal that lands on the sixel branch, which is why testing on Linux and macOS could not have caught it. The erase now resets SGR before painting, and uses DECSC/DECRC rather than CSI s/u because only DECSC saves the attributes this clobbers. Restoring them matters: the erase is emitted mid-frame, between styled writes the TUI has already begun. Measured before and after rather than reasoned about. Before: \x1b[s\x1b[3;41H \x1b[4;41H \x1b[5;41H \x1b[u no reset anywhere. After, the reset leads and DECRC closes. Tests assert the bytes, since bytes are what the terminal sees: SGR is reset BEFORE the first row is painted rather than merely present somewhere, the erase still covers every claimed cell so the fix cannot be satisfied by erasing nothing, and the Kitty path still deletes by id rather than acquiring the character-painting erase. Verified by mutation: restoring the old sequence fails the first of those with the exact reported symptom. Deliberately not fixed here, since both are design calls rather than defects: the erase rectangle is computed from the app's own cell maths rather than the terminal's real footprint, and the sixel path has no in-place move, so every drag pixel is a full erase and repaint where Kitty gets a cheap reposition. * fix(tui): ask sixel terminals for their cell size and erase what was painted The companion blanks a row and a column of live interface on Windows Terminal. The cause is one predicate. petPixelProtocolSupported gates two unrelated questions that only looked like one: whether pixel-precise dragging is available, and whether to ask the terminal for its cell size with CSI 16 t. It answers true for Kitty only, so on a sixel terminal the request was never sent, the reply never arrived, and petCellPixelWidth/Height stayed zero for the whole session. Everything downstream then took its fallback. petImageHeightPixels returned its blind preferredHeight of 75 pixels instead of the cell-aware branch below it, and the erase in clearRenderedImage used petImageColumns and petImageRows. Those constants describe the RESERVED area, not what was painted. Windows Terminal reports a 20x10 cell, so a 75-pixel sprite covers 4 rows and about 8 columns while the erase blanked 5 by 9, taking a row and a column of interface with it on every move. A drag repaints per pixel, so it did that continuously. Windows Terminal answers CSI 16 t, confirmed by hand: it replies ESC [6;20;10t. This was never a question of terminal support. Nothing asked. So the request now goes out for every image protocol, via a predicate separate from the dragging one, and the sixel footprint is computed from the rendered pixel height and the reported cell size rather than from the constants. It is still clamped to the reserved area, because the layout keeps that many cells clear and the drag clamps to it, so a larger computed footprint would mean erasing into live interface. Deliberately NOT widening petPixelProtocolSupported itself: that would enable pixel-precise dragging on sixel as a side effect, which is a separate feature decision rather than part of this defect. Kitty is untouched and a test pins that. There Columns and Rows are the placement REQUEST handed to the terminal, which owns the region and scales into it, so recomputing them would resize the image rather than fix an erase. Tests use the real 20x10 measurement: the sixel footprint is 4 rows not 5, Kitty keeps its requested placement, the fallback without metrics stays the reserved area rather than collapsing to zero and erasing nothing, and every image protocol asks for the measurement. * Keep Sixel companions in the reserved dock --------- Co-authored-by: Vasanthdev2004 <[email protected]> |
||
|
github-actions[bot]
|
2458e0c67b |
chore(main): release 0.7.0 (#837)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>v0.7.0 |
||
|
Vasanth T
|
027a67080b |
fix(mcp): name the non-text blocks a tool result drops (#874)
* fix(mcp): name the non-text blocks a tool result drops TextContent keeps only blocks whose type is "text", so a server that returns an image, audio, or an embedded resource produced "(empty MCP tool result)". A screenshot or browser server would complete a call successfully, hand back a perfectly good image, and the model would be told nothing came back. It then usually retries, which spends another call to reach the same empty answer, and the user never learns an image existed at all. Carrying the payload is the larger half of #823 and is not this change. Dropping silently is the part that causes the retry loop, and it can be fixed on its own. Decode mimeType on Content, and add DroppedContentSummary, which describes what TextContent discarded ("1 image/png block", "2 resource blocks, 1 audio/wav block"), grouping by mime type and ordering by first appearance so the same result always reads the same way. Servers that omit mimeType fall back to the block type rather than printing an empty label. registryTool.Run appends that as a note, and only when something was actually dropped, so a text-only result is byte-for-byte unchanged. The note also says retrying will return the same thing, since telling the model what happened without telling it not to retry only fixes half the loop. Refs #823. * fix(mcp): do not promise a retry returns the same content The dropped-block note said "Retrying will return the same thing". registryTool.Run makes a fresh remote call on every retry, so the server may well answer differently; that is a promise this code is in no position to make. What cannot change is that Zero still has nowhere to put a non-text block, so the note now says retrying cannot RECOVER the payload. Same guidance, a claim that is actually true. The test pins both directions: the new wording must be present, and the absolute claim must not come back. Raised by CodeRabbit on #874. |
||
|
euxaristiaandeuxaristia
|
fb4f293c20 |
feat(providers): live model lists for OpenRouter and OpenGateway (#860)
* feat(providers): fetch live model lists for OpenRouter and OpenGateway Prefer each aggregator public /v1/models catalog over the dead OpenGateway models.json path and models.dev-only OpenRouter mapping so the picker shows current routes with context windows and free labels. Refs #859 * fix(providers): parse OpenRouter string pricing and simplify sort assertions Parse OpenRouter prompt and completion string pricing fields into InputCost and OutputCost, and directly assert alphabetical model sort order in discovery tests. Refs #859 * fix(providers): scale live pricing and restore OpenRouter fallback Convert OpenRouter/OpenGateway per-token pricing to per-million for Model costs, and fall back to models.dev when the live OpenRouter fetch fails so the picker still degrades. * fix(providers): fall back on OpenRouter parse errors; cover live-only merge Route malformed OpenRouter HTTP 200 bodies through the models.dev fallback, and exercise catalog vs live payload split so live-only models stay merged. * fix(providers): address remaining CodeRabbit findings on live catalogs Reject non-finite OpenRouter pricing, keep capability-marked live-only models, and harden related regression tests. --------- Co-authored-by: euxaristia <[email protected]> |
||
|
Vasanth T
|
ccc42b94e7 |
test(agent): raise the eager tool schema ceiling to 3650 (#877)
main is red. TestEagerToolSchemaTokenBudget fails at 3578 tokens against a 3550 ceiling, and every open PR inherits it. Bisected: |
||
|
Vasanth T
|
2ec5c3ae6b |
fix(agent): tell the user a prefix approval leaves the sandbox (#885)
Approving a command prefix does not only stop the prompt from reappearing. shellExecutionArgsForApproval rewrites the call to require_escalated, which resolves to a nil sandbox engine, so the command runs unsandboxed on the host. The engine's escalation gate is then satisfied by the PermissionGranted flag from the approval the user just gave for the sandboxed form. The escalation itself is deliberate and guarded: proposedCommandPrefix refuses to offer a prefix when any other segment of the command is not known-safe, precisely because approval covers the whole command. The defect is that the prompt never said so. The user chose "allow command prefix for session" and got "and also run it outside the sandbox". So this discloses rather than removes. PermissionRequest gains PrefixApprovalEscalates, computed by shellPrefixApprovalEscalates from the same guards the rewrite consults, and the two prefix options append "(runs outside the sandbox)" when it is set. Both the TUI and the ACP path carry it, since an editor user decides on the same terms as a terminal user. The main test asserts the disclosure and the rewrite agree on every input rather than checking each against a fixed expectation, so the two cannot drift apart later. Verified by mutation: reverting the helper to always-false fails it, as does making the label unconditional. |
||
|
|
ff62f73b7f |
fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees (#855)
* fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees 1. Prevent trailing redaction leaks in github_token, aws_access_key_id, and google_api_key by adding trailing word boundaries and allowing variable lengths. Refine the openai_key pattern to cleanly distinguish legacy keys and modern prefixed keys (sk-proj-, sk-svcacct-) from ordinary kebab-case phrases. 2. Implement auto-pruning of zero-owned git worktrees older than 24 hours at the start of worktrees.Prepare to prevent indefinite disk space leaks. * fix(secrets,worktrees): close tail-leak edge case and worktree data-loss risk Drop the trailing \b anchor on the four secret patterns whose body class allows "-" (slack_token, google_api_key, the modern openai_key branch, jwt). \b requires a word/non-word transition, so a secret ending in "-" right before a delimiter has none, and the engine backtracked the greedy quantifier to drop that last character instead of failing the match, leaking it. The body character class already provides the real stopping boundary, so the anchor was unnecessary. Fix two issues in worktree Clean flagged in review: - Staleness was decided by the worktree directory's own mtime, which only changes when an entry is added/removed/renamed directly inside it, not when a long-running task edits existing files deeper in the tree. Clean now walks the tree and treats any recently modified entry as live, and also skips any worktree a caller has explicitly locked via git worktree lock. - baseDir ownership used a raw strings.HasPrefix, so a sibling like "<baseDir>-other" would false-match. Replaced with a filepath.Rel path-boundary check. * fix(worktrees): check exit codes on removal, fail closed on inspection errors defaultRunGit deliberately returns a nil error alongside a nonzero CommandResult.ExitCode for a failed git invocation, so the worktree remove call must check ExitCode itself instead of trusting a nil error to mean success. Route it through gitOutput, which already does that. worktreeIsStale treated an inspection failure (an unreadable file, a WalkDir error) the same as "keep walking," which can let an incompletely-inspected worktree be judged stale. Any inspection error now makes it ineligible for removal instead. * fix(secrets,worktrees): catch appended-suffix keys and scope pruning to owned worktrees The scanner's trailing \b anchors made a credential vanish entirely when followed by a word character outside its body class (an appended suffix like AKIA...EXTRA, or ghp_..._suffix): the fixed or unbounded-greedy quantifier had no valid word boundary to land on and the whole match failed, so the real secret reached the redaction output unredacted. Dropping the trailing anchors lets the body class itself stop the match, so the credential prefix still gets redacted even when noise follows it. Also recognize sk-admin- alongside sk-proj-/sk-svcacct- so OpenAI admin keys aren't skipped by the narrowed modern-key branch. Clean pruned any worktree under the caller-supplied BaseDir, but Prepare only ever creates worktrees under a per-repository zero-worktree-<repoKey> subtree of it. Scope pruning to that subtree so a worktree a user manages by hand elsewhere under a shared BaseDir is never force-removed. Also refuse to force-remove a worktree whose mtime looks stale but that still has uncommitted or untracked changes: a task can hold live work while waiting on a model, network, or user for longer than the staleness window without writing to the tree again. * fix(worktrees): make nested-activity test exercise the deep walk it claims to activePath/internal was created by the same MkdirAll as the nested pkg dir but never backdated, so it kept a fresh mtime and worktreeIsStale's walk reported "not stale" as soon as it hit that directory, before ever reaching the freshly-written file two levels deeper. The test passed without actually exercising recursion past the first directory. * fix(worktrees): lock zero-created worktrees and treat ignored files as dirty Prepare never called git worktree lock, so the entry.locked skip in Clean only ever protected worktrees a human locked by hand, never zero's own; a worktree that finished committing and sat idle (e.g. waiting on a slow model or network retry) for more than 24h looked clean-and-stale and got force-removed by the mtime+dirty heuristic alone. Lock every worktree Prepare creates so it gets the same protection. worktreeIsDirty also used git status --porcelain with no --ignored, so a worktree holding only .gitignore-matched task data (credentials, generated drafts, artifacts) reported as clean and got force-removed with --force, silently discarding it. Add --ignored so those files count as dirty too. * fix(worktrees): release Zero's Prepare lock so Clean can reclaim finished worktrees Prepare locks every worktree it creates so Clean's mtime+dirty staleness heuristic never force-removes one Zero is still using, but nothing ever unlocked it, making the automatic disk-space cleanup permanently inert. Add Release (git worktree unlock) and wire it in two ways: zero exec --worktree defers a release once its own run finishes, since that flow's use of the worktree is bound to its own process. zero worktrees prepare hands the path to a longer-lived external caller with no defined end-of-life, so a new zero worktrees release <path> subcommand lets that caller release it explicitly when done. * fix(worktrees): normalize release path, aggregate Clean errors, unlock deleted worktrees Address CodeRabbit's review on the lock-release fix: - zero worktrees release now resolves its path argument to absolute before calling Release, since git worktree unlock matches against the path git recorded at creation, not whatever directory the caller happens to be running from. - Clean now aggregates removal failures with errors.Join instead of overwriting lastErr, so multiple stale worktrees failing removal in the same pass are all reported, not just the last one. - Release falls back to options.Cwd as the git working directory when the worktree path itself no longer exists (e.g. a caller deleted a locked worktree by hand instead of releasing it first), so the orphaned lock can still be cleared. Added regression coverage for all three. * style(worktrees): align aggregation test comments to gofmt output * fix(worktrees,cli): restore reuse lease and scope unlock to owned locks - Prepare re-locks a reused worktree so Clean's staleness heuristic cannot force-remove it while the new caller is still using it; a lock already held by a live external caller is kept in place and reported through the new Result.LockAcquired field. - exec --worktree only releases the lock its own Prepare call acquired and surfaces a failed release on stderr with the affected path instead of discarding the error. - worktrees release wires the resolved workspace root into Options.Cwd so the deleted-path recovery works outside the worktree directory. - The relative-path release test derives its expected value via filepath.Abs, matching the resolution the CLI uses, so macOS /var vs /private/var spellings no longer break it. * fix(worktrees,cli): reject in-use leases, validate before cleanup, add release -C - Prepare rejects a worktree whose lock another run still holds, on both the reuse and the create-race paths, instead of handing a second live caller an unprotected shared checkout whose sole Git lock the first caller's exit would release. - The automatic stale-worktree pruning runs only after the request itself validates, so a rejected command (an invalid --name) has no destructive cleanup side effect; covered end to end with real git in both directions. - worktrees release accepts -C/--cwd naming the source repository, which the deleted-path recovery needs when launched outside the repo (the deleted worktree path is a one-way hash with no way back to its source). * test(worktrees): canonicalize test roots to physical spelling git records worktree paths in physical form, so the CI runners' symlinked (/var -> /private/var) and 8.3-short (RUNNER~1) temp spellings made Clean's containment check skip the test's stale entry and the pruning assertion fail on macOS and Windows. * fix(worktrees): preserve orphaned commits, verify release ownership, canonicalize base dir Four fixes from the latest review round: - Clean now creates a durable ref (refs/zero/orphaned-worktree/<sha>) for a detached worktree's HEAD before force-removing it, when that commit isn't already reachable from any other ref. Prepare always creates worktrees with `worktree add --detach`, so a commit made there had no ref pointing at it once the worktree was deleted, making it immediately eligible for git gc despite never having been merged/pushed elsewhere. - Clean resolves its configured base directory through EvalSymlinks before comparing it against git's reported worktree paths: `git worktree list --porcelain` reports each worktree's PHYSICAL location (resolving symlink components), so a symlinked --worktree-dir made every worktree created under it permanently unprunable. - Release now verifies path has a zero-worktree-<repoKey> ancestor directory component before running `git worktree unlock`, so it can't be used to clear the lock on a worktree a user or another tool manages by hand. The check doesn't need to know which --dir a given Prepare call used (nothing records that against a specific worktree, and the CLI never threads BaseDir through to Release) — the repoKey component is Prepare's actual ownership signature regardless of which directory it was created under. - Route the release command's printed path and exec's release-failure diagnostic through the existing CLI redaction helper, and split the worktrees help text into prepare-specific and release-specific flag sections (release only ever supported -C/--cwd, not the --name/--dir/ --json the shared block advertised). All four have regression tests confirmed to fail without their fix (two using real git worktrees, not just fakeRunner sequences). Build, vet, and gofmt clean on linux/windows/darwin. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(worktrees): recoverable PID leases and reclaimable released worktrees Two review findings on the cleanup lifecycle: - A lock left by an abnormal exit (SIGKILL, crash, power loss) was skipped by Clean forever, recreating the permanent disk leak this PR set out to fix. exec --worktree now records its PID in the lease reason; Clean expires a lease whose recorded owner is provably dead, unlocking only after the staleness, dirty, and HEAD-preservation guards all pass. Human locks and PID-less leases (external `worktrees prepare` owners) remain permanent until explicit release, and any ambiguity in the liveness probe counts as alive. - An explicitly released worktree holding only gitignored residue (node_modules, build output) was skipped at every age. Release is the owner's completion signal, so unlocked entries now block removal only on tracked/untracked changes; expired crashed leases keep the conservative --ignored probe since they never signaled completion. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(worktrees): address review feedback on lease detection and release safety Split dead-lease PID checking into posix/windows implementations so Windows can reliably tell a dead process from a live one. Fix a path canonicalization mismatch in two release tests. Derive release ownership from git worktree list instead of the git-dir parent, which was wrong for repos with a separate git-dir. Refuse to clear a lock that was not taken by Zero in the first place. * fix(worktrees): canonicalize paths for clean/release ownership checks Compare Clean containment and Release ownership against physical path spellings so macOS /var vs /private/var and symlink TMPDIR layouts match git worktree list. Require a registered porcelain entry and a Zero lease reason before unlock; treat an already-unlocked Zero worktree as a no-op. * fix(worktrees,secrets): address jatmn review findings on #632 - Prepare and Release now agree on repoKey regardless of which worktree (main or linked) Prepare runs from, by keying off git worktree list's first entry (always the main worktree) instead of --show-toplevel. A worktree prepared from a linked checkout previously failed its own ownership check on release and its lease could never be cleared. - osProcessAlive on Windows no longer treats every OpenProcess failure as "process is dead": only ERROR_ACCESS_DENIED (a live process this caller lacks rights to query) is now distinguished from a genuinely missing PID, so Clean can no longer force-remove an active worktree whose owning process it simply couldn't query. - The openai_key redaction pattern now recognizes sk-or-v1- (OpenRouter) alongside the existing sk-proj-/sk-svcacct-/sk-admin- prefixes, so hyphenated OpenAI-compatible provider keys are redacted again without reopening the sk-<kebab-case-phrase> false-positive this pattern was narrowed to avoid. - Release/exec --worktree error text is redacted before reaching stderr, matching the already-redacted success path; ownership errors interpolate the caller-supplied path, which could carry a key-shaped segment. - canonicalizePath resolves symlinks through the nearest existing ancestor when the target itself no longer exists, so the documented `release -C` recovery path works again for a worktree deleted by hand under a symlinked --worktree-dir. - Prepare rolls back the worktree `git worktree add` just created if the subsequent lock call fails for a reason other than a concurrent racer, instead of leaking an unleased checkout until Clean's 24h staleness window reclaims it. Not addressed here: the P2 finding that worktree ownership is provable only by a directory-name convention plus a lock-reason prefix, both of which a user can reproduce by hand. A durable per-worktree ownership marker would close that gap, but internal/worktrees has been on main since #70, so a marker requirement could reject worktrees an already-installed zero created before this change existed. Needs a decision on migration before implementing. * fix(worktrees): prove Prepare ownership with a git-admin marker Address review findings that path convention plus lease-reason prefix are forgeable by hand. Prepare now writes a zero-owner marker into the worktree admin dir; Release and Clean require it before force-touching a path. Clean also keys its owned subtree off the main worktree root so linked-checkout calls still prune the same bucket Prepare uses. Tests plant the marker and list the main worktree first so fixtures match production. * fix(cli): complete worktrees release in shell completions * test(worktrees,cli): add coverage for linked-worktree Clean, forged lease rejection, and completions - TestCleanFromLinkedWorktreePrunesStaleWorktree: pins Clean deriving its owned-subtree key from the main worktree root (not the invoking linked checkout's --show-toplevel) so Prepare/Clean run from a linked worktree actually reclaim the worktrees Prepare created there. - TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker: pins the ownership-marker requirement against the exact forgery jatmn described - a worktree under the predictable zero-worktree-<repoKey> path, manually locked with a reason that merely starts with the zero lease prefix. - Fix TestPrepareCreatesDetachedGitWorktree: its fake git runner ran out of canned results at Prepare's post-lock ownership-marker write, so writeOwnershipMarker resolved gitDir to "" and os.WriteFile wrote "zero-owner" as a relative path into the test process's real working directory instead of failing loudly. Give it autoAbsoluteGitDir like the other Prepare-exercising tests use and assert on the marker-write call. - completions_test.go: assert `worktrees`/`worktree` completions include `release` alongside `prepare`. * fix(redaction,worktrees): sort extra secret values by length descending and canonicalize worktree paths * fix(secrets,worktrees): restore Anthropic key redaction and handle legacy worktree cleanup * fix(secrets): add bash output redaction regression test for Anthropic API keys * fix(worktrees): touch worktree mtime on reuse to prevent stale pruning When Prepare reuses an existing worktree, it now touches the directory to refresh its mtime. This prevents Clean from force-removing a long-running but idle worktree (e.g. waiting on a model) that has no recent file changes but is still actively in use. Addresses the data-loss risk flagged in the PR review where mtime-only staleness + --force removal could discard live worktrees. Co-authored-by: cairn-code Co-Authored-By: cairn-code <[email protected]> * fix(worktrees): properly handle os.Chtimes error on reused worktree path Co-Authored-By: cairn-code <[email protected]> * fix(worktrees,secrets): unlock using porcelain entry path, set RepoRoot to primaryRoot, and add anthropic_key pattern Unlock using matched porcelain entry path, set Result.RepoRoot to primaryRoot in Prepare, add dedicated anthropic_key secret pattern, and canonicalize fake-runner test paths. Refs #632 * fix(tools): correct redaction placeholder assertion in Anthropic key test TestFormatBashOutputRedactsAnthropicKey checked for the openai_key placeholder instead of anthropic_key, a copy-paste leftover. The redaction itself was already correct; only the assertion was wrong. * test(worktrees): physicalize test paths for macOS tempdir symlink resolution * fix(secrets,worktrees): address CodeRabbit review findings Align RedactString token boundaries with secrets.Scan, strengthen the Anthropic bash redaction assertion, fix Clean fixtures so they reach the guards under test, and fail closed on ambiguous processAlive probes. * fix(secrets,worktrees): restore broad key redaction and legacy Clean safety Address human review on #855: keep sk- bodies with a digit filter instead of enumerated vendor prefixes, add a looser JWT form, restore the sk-test fixture, probe legacy ownership before dirty, treat non-INVALID Windows OpenProcess errors as alive, redact Abs/cwd release errors, and reclaim dead-owner leases on Prepare reuse. * fix(secrets,redaction): always redact known OpenAI key prefixes Alphabet-only sk-proj-/sk-svcacct-/sk-admin- tokens are still credentials; keep the digit filter only for unknown sk- vendor forms so kebab phrases like sk-learn-… stay un-redacted. * Preserve digit-free legacy keys during redaction. Refs #855 * fix(secrets,worktrees): address CodeRabbit findings on PR #855 Redact full compact JWE tokens, pin git locale for lock parsing, write the ownership marker atomically, surface missing-dir unlock failures, and fail closed when HEAD probes are indeterminate. Align Clean fixtures and digit-free known-prefix redaction tests with the shipped behavior. Refs #855 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: cairn-code <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
|
0176ee49ff |
feat(agent): add PermissionModePlan for interactive read-only planning (#853)
* feat(agent): add PermissionModePlan for interactive read-only planning * fix(agent): drop redundant modeName branch, add plan mode regression tests string(permissionMode) already yields "plan" / "spec-draft", so the if/else recomputing modeName in the denial message was dead branching on the same values. Also add plan-mode coverage mirroring three of the four existing spec-draft regression tests: advertised tool set, and denied write_file/bash calls. The fourth (submit-and-stop review control) has no plan-mode analog, since plan mode has no submit tool. * fix(agent): deny request_permissions in plan/spec-draft even when the registry omits it request_permissions is dispatched by name in executeToolCall before the registry-based ToolAdvertised gate runs, so that gate only helps when the tool happens to be present in the caller's registry. A plan- or spec-draft-mode registry that simply omits the tool (rather than registering it as denied) let the call fall through to a real turn/session-scoped permission grant, defeating the read-only boundary. Deny it unconditionally at the top of executeRequestPermissions for both read-only modes instead. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(agent): suppress executable hooks while plan/spec-draft mode is active Plan mode promises a read-only turn, but sessionStart/sessionEnd fire on every run and beforeTool/afterTool fire around allowed read calls, and all four execute configured host commands outside the advertised-tool and sandbox gates — so a project hook could mutate the workspace or spawn a process from a session that advertises it cannot. Gate all four dispatch points on the run's permission mode, with a regression test asserting no hook command launches during a plan-mode run. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(agent): close plan-mode tool advertisement bypass toolAdvertisedInPlan whitelisted ask_user and update_plan by name alone, so a caller could register a mutating tool under either name and have it advertised and executed in plan mode. Validate every tool against its Safety() instead. Also exclude lsp_navigate, which is marked SideEffectRead but lazily spawns a real language-server process, contradicting plan mode's read-only guarantee. Add regression tests for both. * fix(agent): keep trust-gated hooks in spec-draft mode Plan mode still suppresses executable hooks so a read-only planning turn cannot spawn host processes via session or tool hooks. Spec-draft keeps the existing trust model: project hooks fire when the workspace (or its worktree trust root) is trusted. Unconditionally suppressing hooks in spec-draft broke TestExecSpecWorktreeInheritsTrustEndToEnd for trusted worktrees under --use-spec --worktree. * fix(agent): cover plan-mode spoof and lsp_navigate denials Expand the name-only spoof regression to both update_plan and ask_user, and add an execution-path denial for lsp_navigate so plan mode cannot spawn language servers even when a call still arrives. * fix(agent): filter tool_search deferred candidates by plan/spec-draft visibility tool_search resolved and ranked deferred tools by EnabledTools/DisabledTools only, never by the run's permission-mode visibility. tool_search itself is already denied at dispatch in plan/spec-draft (its Safety carries no side effect, so it fails the same SideEffect==Read advertisement gate direct calls use), so this was not reachable through the normal Run() path today. But it is a real landmine: if that outer gate is ever loosened independently (e.g. tool_search's no-side-effect Safety is judged advertisable), the loader had no gate of its own and would hand a deferred write/mutator tool's name, description, and full schema straight to a plan/spec-draft model. Mirror agent.ToolAdvertised's plan/spec-draft branches inside the tools package (toolAdvertisedForPermissionMode, next to the existing toolAllowedByFilters mirror that avoids the same import cycle) and apply it alongside the operator filters in visibleDeferredTools and visibleEagerToolNames. Added unit tests in tool_search_test.go for both modes, and an end-to-end agent test that force-calls tool_search in plan mode and asserts no schema leaks. Verified by reverting tool_search.go and confirming the new tests fail (one shows load_tools resolving to the mutator's name); restored and confirmed they pass. Also confirmed via a temporary probe that if plan mode's outer advertisement gate is loosened, this filter is what actually stops the leak. * fix(agent): require Safety for spec-draft ask_user/submit_spec Do not advertise or load re-registered control tools by name alone in spec-draft mode. ask_user must be SideEffectRead+Allow and submit_spec must be SideEffectWrite+Allow, matching the real tools. Apply the same filter in tool_search and add spoof regression tests. Refs Gitlawb/zero#642 * fix(agent): wire plan mode into TUI, CLI, and ACP entry points Address the P1 finding that PermissionModePlan was documented but never selected by /plan, zero exec, or ACP mode selectors. /plan on|off now toggles the session permission mode (restoring the prior mode on off), zero exec --plan selects plan for a run, and ACP advertises plan as a client-selectable mode. Integration coverage for each entry path. * fix(cli): reject --plan combined with --worktree Worktree preparation runs in runExec before the plan permission mode is assigned, so `zero exec --plan --worktree` could still trigger workspace mutation ahead of the read-only gate. Reject the combination during option validation, alongside the existing --use-spec/--skip-permissions- unsafe conflict checks, so no worktree prep can occur. Addresses a coderabbitai finding on PR #642. * test(tools): assert spoofed tool schema doesn't leak via tool_search The spoofed-control-tool regression only asserted on the description string; Parameters() exposed no distinctive schema marker, so a regression that leaked the schema without the description would still have passed. Add a spoofed_secret property to the test tool's schema and assert it's absent from result.Output alongside the description. Addresses a coderabbitai finding on PR #642. * fix(tui): gate local mutating commands behind plan mode /plan on only flips the agent permission mode, which gates agent tool calls. Local TUI commands that run entirely inside the TUI process bypass that gate: /rewind restores workspace files from a checkpoint, /export writes a transcript to disk, and /sandbox-setup spawns a native host process. Add a shared plan-mode guard at the start of dispatchCommand (mirroring the existing BTW-unavailable guard) that rejects these three commands while permissionMode is agent.PermissionModePlan, with regression coverage proving each is blocked with no mutation/process spawn in plan mode and unaffected outside it. Addresses a coderabbitai finding on PR #642. * fix(agent,specialist): layer plan mode system prompt and enforce read-only subagent mode * Fix jatmn review findings for PR 642 * fix(agent,cli,tui): resolve CodeRabbit review comments on active turn model field and --plan permission mode conflict * fix(agent): propagate permission mode to exec options and serialize ACP mode changes Parse permissionMode in resolveExecPermissionMode, acquire turnMu.Lock in ACP handleSetMode to serialize mode changes with active turns, and block MCP subcommands in TUI plan mode. Refs #642 * fix(agent,tui): update tests off removed tools.CoreTools/NewWriteFileTool/NewLSPNavigateTool wrappers Those were thin unscoped wrappers around the Scoped variants, deleted upstream in #706 since nothing else called them directly. Only these tests still did; switch to the Scoped calls main's own tests already use. * fix(agent): fail-closed beforeTool vetoes and permission-mode plan guards Keep beforeTool deny gates active under plan mode so hooksSuppressed no longer fails open, and stop propagating --permission-mode for auto/ask/member children so swarm members keep write tools. Apply --plan combination rejects to --permission-mode plan as well. Refs #853 * fix(agent): address CodeRabbit findings for plan-mode advertisement and entry paths Unify plan/spec-draft tool advertisement in tools.ToolAdvertisedForPermissionMode (with PermissionDeny short-circuit), cover ACP config and --permission-mode plan list-tools paths, and allow bare /mcp while blocking mutating MCP subcommands. Refs #853 --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: euxaristia <[email protected]> |
||
|
Anandan
|
160e3bed1a |
Unify tool output views (#880)
* Unify tool output views * Preserve finalized tool outcomes * Make outcome tests portable |
||
|
Vasanth T
|
7fbd864e91 |
fix(tui): fall back to CellMotion where AllMotion is unreliable (#872)
* fix(tui): fall back to CellMotion where AllMotion is unreliable AllMotion (1003) buys exactly one thing over CellMotion (1002): hover highlighting. Wheel, click and drag all arrive under either. But a terminal that does not implement 1003 does not degrade to 1002, it sends nothing at all, and because the TUI holds the alternate screen the terminal's own scrollback and wheel are gone too. The user is left unable to scroll by any means, and a long answer then reads as a hang rather than as a missing highlight. That is how #870 arrived: a completed turn with "59 more" lines below the fold, reported as a freeze. The existing code already conceded 1003 is fragile by carving out PRoot. The legacy Windows console is the same case and was not covered, so a bare zero.exe outside Windows Terminal asked for a mode its terminal does not answer. Move the decision into mouseModeFor, which trusts terminals that identify themselves (WT_SESSION for Windows Terminal, TERM_PROGRAM for hosts like VS Code and mintty) and assumes an unidentified Windows terminal is the legacy console. Guessing wrong that way costs a hover highlight; guessing wrong the other way costs every mouse event. ZERO_MOUSE_MODE=all|cell|off overrides it, so a user on a terminal we guessed wrong about can fix it without waiting for a release. An unrecognised value is ignored rather than treated as "off", so a typo cannot silently kill the mouse. Refs #870. * test(tui): pin the inherited-environment limit of the mouse-mode guess WT_SESSION and TERM_PROGRAM are inherited, so they answer for whichever process set them rather than for the console host attached now. A shell started from Windows Terminal or Git Bash that later runs zero.exe against the legacy console still carries them, and mouseModeFor then asks for AllMotion on a host that may drop every mouse event: the #870 failure class through a narrower door. Not fixed, and deliberately so. Telling an inherited variable from a live one needs the console host rather than the environment, and erring the other way costs every Windows Terminal user their hover highlighting for a case that takes an unusual launch path to reach. So the limit is written down in the code and pinned by a test that also asserts ZERO_MOUSE_MODE=cell still overrides it, since that override is the only recourse an affected user has. If real host detection lands later the test fails by design, and its message says to delete it. Raised by @jatmn on #872. |
||
|
euxaristia
|
020c28eaf9 |
fix(tui): fill alt-screen frame with theme panel background (#850)
* fix(tui): fill alt-screen frame with theme panel background Blank and cleared cells fell back to the terminal's own default background instead of the theme's panel color, flashing white on light themes during resize or when ClearScreen erases the frame. Wrap the rendered content in zeroTheme.panel sized to the full frame so every cell is explicitly painted. * fix(tui): apply theme panel content to tea.View after render tea.NewView stores the original string; reassigning the local content variable left the view unchanged. Call SetContent after the panel render so the alt-screen frame uses the themed output. |
||
|
euxaristiaandgoogle-labs-jules[bot]
|
21e1e8d072 |
style(cli): flatten trust workspace resolve errors with %s (#849)
* fix: prevent leaking sensitive info in wrapped error responses Modified `fmt.Errorf` calls in `internal/cli/trust.go` to use `%s` instead of `%w` when wrapping errors that are subsequently redacted via `redaction.ErrorMessage()`. This ensures the underlying error is flattened to a plain string, preventing sensitive information from being exposed later via `errors.Unwrap()`. Co-authored-by: euxaristia <[email protected]> * chore(cli): drop agent sentinel from trust error-format tidy Remove .jules/sentinel.md (local agent artifact, not in main). Keep the trust.go %s flatten as a readability tidy only; redaction.ErrorMessage already stringifies inline, so this is not a security fix. * style(cli): note intentional %s flatten on trust resolve errors Keep a short comment at both sites so the %s form is not "fixed" back to %w; ErrorMessage stringifies inline. --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> |
||
|
|
936aa4e77c |
fix(lsp): dispatch notifications off read loop (#759)
* fix(lsp): dispatch notifications off read loop * fix(lsp): keep notification reads non-blocking * fix(lsp): make the notification queue lossless Addresses the P1 on #759. The previous commit traded the read-loop deadlock for silent protocol-event loss: at 64 queued items enqueueNotification discarded the OLDEST message. A textDocument/publishDiagnostics is the server's only report for its URI, so a dropped one makes session.waitForDiagnostics time out and Manager.Check return no diagnostics even though the server published them — and the overflow test codified that loss as intended behavior. The queue is now unbounded: the read loop still never blocks (so a handler may make a re-entrant Client.Call), and nothing is discarded. Both alternatives are worse — blocking on a full buffer is the original deadlock, and dropping loses state the protocol never repeats. Growth is bounded in practice by what the server emits while a handler runs; the backing array is released as soon as the queue drains, so a burst does not retain capacity afterwards. Tests: TestClientNotificationQueueIsLossless replaces the drop-oldest test, and the end-to-end burst test now also asserts that all 512 notifications sent while a handler was blocked in a re-entrant call arrive, in order. Both were checked against the old drop-oldest policy and fail there (64 of 512 delivered, FIFO broken), so they have teeth. * fix(lsp): stop retaining notifications after Close Addresses the shutdown defect CodeRabbit found in the lossless queue. Client.Close signals the worker loop to exit, but readLoop keeps consuming frames until the transport ends — and Server.Shutdown closes the client BEFORE closing stdin, so a language server emitting notifications while it handles shutdown/exit fed an unbounded queue that nothing would ever drain. failPending's closeOnce now marks the queue closed and releases whatever is in it, and enqueueNotification drops instead of appending once that flag is set. Both run under notifyMu, so an enqueue racing with shutdown either sees the flag and drops its item or appends just before it and has the item cleared. Queued work is dropped rather than drained: the worker is gone by definition of close, and callers that need diagnostics collect them before shutting the client down. TestClientDropsNotificationsAfterClose keeps the transport writable after Close, as Server.Shutdown does, and requires 64 subsequent notifications to neither reach the handler nor accumulate. Verified against the unguarded enqueue, where it reports 64 retained notifications. * fix(lsp): baseline diagnostics against receipt, bound the notification backlog Addresses jatmn's two #759 review findings: - Manager.Check captured its diagnostics baseline from publishCount, which only advances once session.handleNotification actually runs for a URI. Dispatch happens off a queue now, so a publish for a version Check is about to supersede could already be sitting received-but-undispatched at baseline-capture time, then run afterward and look newer than the baseline purely because handling lagged receipt. Since many servers omit the version field, the existing staleness check in handleNotification (positive-version only) couldn't catch it either. Stamp every notification with a receipt sequence at enqueue time (Client.NotificationSeq), carry it through the queue to the handler, and baseline against receipt instead of against how many publishes have been processed. - The lossless notification queue had no failure policy: a language server sustaining a higher notification rate than the single handler can drain (no stuck handler required) would retain every full json.RawMessage payload without bound. Add notifyQueueLimit; exceeding it fails and closes the client observably, letting the manager's existing dead-session eviction take over on next use, instead of growing the queue until the heap gives out. Both regression tests are verified to fail against the pre-fix code: the receipt-vs-baseline race was reproduced directly against the pre-fix API (confirmed real, not hypothetical) before porting the test to the fixed API, and the overload test was confirmed to fail with the limit check removed. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(lsp): bound notification backlog bytes Track retained notification payload bytes in addition to queue length and fail the client before the backlog exceeds 16 MiB. Wake diagnostic waiters when that failure closes the client, preserving already-received fresh diagnostics during debounce. Add race-enabled regression coverage for byte overload cleanup and client-failure wakeup. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b80c-717b-b041-5d5135a61d96 Co-authored-by: Pierre Bruno <[email protected]> * fix(lsp): preserve diagnostics across client closure When a fresh publish and client closure wake a diagnostic waiter together, reconcile against the recorded receipt sequence instead of allowing select order to discard the publish. Add a regression that repeatedly makes both wakeups ready together and verifies diagnostics handled before closure are preserved. Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-5dd3-7118-a7a6-fba8f9d5af29 Co-authored-by: Pierre Bruno <[email protected]> * fix(lsp): drain accepted notifications on close Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(lsp): pin the receipt boundary at read time and finish the drain Stamping a notification's receipt seq at enqueue left a window async dispatch had opened: readMessage consumes the frame, then json.Unmarshal and the enqueue run later on the same goroutine while another can capture a publishBaseline. A publishDiagnostics for superseded text could land above a baseline taken after it was already off the wire and — most servers omit the version field the staleness check needs — reach the caller as the answer for the new text. The read loop now stamps every frame the moment it leaves the wire and hands that seq to the enqueue, so decoding cost sits outside the boundary. ReceiptSeq replaces NotificationSeq: the clock counts frames, and gaps in it are fine. Notification drain handling is now uniform. Every exit from waitForDiagnostics catches the worker up on notifications it already accepted before reading what was recorded, instead of only the seq<=baseline closure arm doing so. That covers cancellation racing an accepted-but-queued publish (which used to make Check return (nil, nil) for text the server had answered), and the debounce closure arm returning while a newer publish was still queued (which handed the caller the older one). After closure the drain is bounded — admission is shut and a handler's re-entrant Call fails immediately — so it is no longer cut short by ctx; on a live client the catch-up is bounded by a short grace instead. Also cap Content-Length before allocating the body, so the notification byte budget is authoritative rather than measured after a huge frame is materialized. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(lsp): follow publishes accepted while catch-up is waiting Catch-up snapshotted the accepted sequence once, but the reader keeps running while it waits. The worker finishing the item that was last at the start satisfied the check, so catch-up returned with a newer publish for the same URI still queued — and the caller read diagnosticsFor and got the older one. That is the failure the closed-client drain exists to prevent, reproduced on the live path by the async dispatch this PR introduces. The target is now re-read every pass. This cannot spin forever: the live-client grace already caps the wait no matter how fast the server publishes, and a closed client still drains unconditionally because its backlog cannot grow. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * test(lsp): wait for follow-up notification enqueue Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> * test(lsp): pin receipt stamping to read time, not enqueue Regression test for the gap flagged in review pullrequestreview-4834747507 on #759: TestPublishBaselineRejectsFrameReadBeforeBaseline calls stampReceipt directly and never drives readLoop, so it doesn't pin where the stamp is taken. A response frame (read but never enqueued as a notification) distinguishes the two call sites directly. Co-authored-by: Cursor <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]> Co-authored-by: Cursor <[email protected]> |
||
|
|
30f6d8f591 |
fix(specialist): make overwrites atomic (#757)
* fix(specialist): make overwrites atomic * fix(specialist): harden atomic replacement * fix(specialist): replace through a Windows-atomic, DACL-preserving primitive Addresses both P1 findings on #757. os.Rename — which fsutil.RenameWithRetry wraps — is documented non-atomic outside Unix, and the retry loop only handles sharing violations. Worse, the replacement is a freshly created temporary file, so renaming it over the destination also publishes the directory's inherited DACL in place of the destination's own: overwriting a specialist that had been explicitly restricted inside a group-readable directory would expose its system prompt to that group. temp.Chmod(0o600) cannot prevent that on Windows, where Go maps only the owner-write bit. New fsutil.ReplaceWithRetry publishes through a platform primitive: - Windows uses ReplaceFileW with REPLACEFILE_WRITE_THROUGH, one operation that carries the replaced file's security descriptor, attributes, and streams over to the replacement. REPLACEFILE_IGNORE_ACL_ERRORS is deliberately not passed — silently losing the descriptor is the failure being fixed — so a merge failure surfaces and leaves the destination untouched. A missing destination falls back to a rename (nothing to replace or preserve), as do volumes that cannot perform a replace at all (FAT/exFAT, some network redirectors), which carry no ACL either. The transient-lock retry is unchanged. - Unix keeps rename(2), which is already atomic within one filesystem. Tests: Windows coverage asserts the destination's creation time survives (a rename would not, proving the call goes through ReplaceFileW) and that a protected owner-only DACL survives both at the fsutil layer and through the real `Create --force` path; a plain rename was verified to replace that DACL with the directory's inherited ACEs, so the assertion has teeth. POSIX covers both destination shapes. * fix(fsutil): stop ignoring ACL merge errors, recover partial ReplaceFileW Addresses jatmn's two #757 review findings: - The call passed REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) while the comment claimed ACL failures were fail-closed. Microsoft documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically: with either set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved". Passing 0x2 therefore let a --force overwrite silently publish the temp file's inherited directory DACL over a restricted specialist and expose its system prompt. Pass no flags at all. - With no backup file, ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) and ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leave the destination deleted (1176) or renamed to an unreachable name (1177) while the replacement survives only under its temporary name. Returning those unrepaired let the caller's deferred temp-file cleanup remove the sole surviving copy, so one failed 'specialist create --force' destroyed both the old manifest and the new one. recoverPartialReplace now moves the replacement into place and still reports the error, since the destination's descriptor died with the destination. Also drops the doc claim that REPLACEFILE_WRITE_THROUGH (0x1) flushes before returning: Microsoft documents that value as "not supported". The two fsutil regression tests are verified to fail against the pre-fix code. The specialist-side test guards the caller half of the recovery contract and does not fail pre-fix; its comment says so. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(fsutil): preserve Windows replacement backups Use a managed ReplaceFileW backup so partial failures can restore the original file and DACL. Fail closed when atomic replacement is unsupported, retry rollback and backup cleanup, and retain a known backup if rollback cannot complete.\n\nCover unsupported volumes, 1176/1177 semantics, cleanup, rollback retries, and symlink rejection. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-be8e-75eb-b274-b7e22af373a0 Co-authored-by: Pierre Bruno <[email protected]> * fix(specialist): report committed cleanup warnings Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * docs(specialist): describe what a failed overwrite actually leaves behind The Windows partial-replace recovery errors told an operator the replacement "remains at" the caller's temporary path, but the only caller removes that file in a deferred cleanup before the error ever surfaces — it named a file that was already gone. Recovery from a failed overwrite is always about the original, so the errors now describe only what the replace itself left on disk: the state of the destination and of the managed backup. The replacement path is no longer a parameter, since nothing reports it. The one terminal state that needs a human — 1177 whose rollback exhausted its retries — now says so outright. The original then exists only under an opaque .zero-replace-*.backup name, and the loader reads *.md, so the specialist stays invisible until somebody moves it back. The error names both paths and the action; docs/SPECIALISTS.md walks through it; and loadDirectory records why the non-.md siblings an interrupted overwrite can leave are skipped rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(specialist): serialize managed loads during replacement Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
euxaristiaandgoogle-labs-jules[bot]
|
c52db055b0 |
fix: suppress gosec G204 warning for trusted local config commands (#848)
Addresses a Gosec G204 (Command Injection) warning where provider command strings were passed directly into `sh -c` and `cmd /C`. Since these commands originate from the user's trusted local config and explicitly require shell features (like pipes and variable expansions) to function, removing the shell invocation introduces a breaking change. This adds a documented `#nosec` annotation to suppress the false positive instead of degrading functionality. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> |