564 Commits

Include renames

Author	SHA1	Message	Date
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	2026-08-12 22:41:05 +05:30
3 people	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]>	2026-08-11 20:03:18 +08:00
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]>	2026-08-11 09:24:06 +08:00
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.	2026-08-10 10:59:03 +08:00
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]>	2026-08-10 10:58:38 +08:00
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: edf660ab and 4ca4fa75 pass, cd0eb194 (#843, which added view_image to the core set) fails at 3588. #867 later trimmed it to 3578. #843's own CI was green, because the ceiling is only crossed once the tool set is combined, so no per-PR check could have caught it. Raised rather than paid down. The overrun is 28 tokens, and view_image costs 82, the second cheapest tool in the set against exec_command's 909 and request_permissions' 397. It is simply the one that crossed the line. The alternative was trimming a description to reclaim 28 tokens, degrading a tool's usability to satisfy a line drawn against a 2026-07 measurement that predates two tools. The ratchet worked exactly as intended: it caught the creep and forced a decision instead of a drift. Also corrects the failure message. It advised "defer a tool", but this test pins DeferThreshold at 0 so deferral is inactive and marking a tool deferred leaves it exposed here, changing nothing. Deferring is still worth doing for real sessions; it just cannot move this number. The levers that can are a smaller schema, one fewer core tool, or a deliberate raise, and the message now says so.	2026-08-10 10:57:42 +08:00
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.	2026-08-10 10:56:49 +08:00
4 people	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]>	2026-08-10 10:51:11 +08:00
3 people	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]>	2026-08-10 10:15:29 +08:00
Anandan	160e3bed1a	Unify tool output views (#880) ... * Unify tool output views * Preserve finalized tool outcomes * Make outcome tests portable	2026-08-10 09:58:05 +08:00
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.	2026-08-10 09:57:44 +08:00
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.	2026-08-10 09:57:25 +08:00
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>	2026-08-10 09:50:44 +08:00
4 people	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]>	2026-08-10 09:50:03 +08:00
3 people	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]>	2026-08-10 09:49:32 +08:00
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>	2026-08-10 09:12:58 +08:00
6 people	55fa05f0ab	fix(sandbox): deny reads of Zero credential stores (#681) ... * fix(sandbox): deny reads of zero credential stores * fix(sandbox): address PR review comments - Deny reads of mcp-oauth-tokens.json.secret in the default Zero config candidates: file-backed oauth stores keep their encryption secret in a sibling .secret file, so the default MCP token store needs the same protection as the override paths (CodeRabbit). - Clarify that explicit credential-file overrides are still filtered by on-disk existence like every other candidate (Copilot). - Align the zeroUserConfigDir doc comment with config.UserConfigDir: macOS honors XDG_CONFIG_HOME before falling back to ~/.config (Copilot). Co-Authored-By: Claude Fable 5 <[email protected]> * fix(sandbox): deny the whole Zero config dir, not itemized filenames Address code review on PR #681: - Deny credentialDenyReadPaths' Zero-config candidate as the containing directory instead of an itemized filename list. The token/credential/ config stores each publish through a randomly-named .tmp-<pid>-<nanos> sibling before their atomic rename, and the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated backup after importing it — none of those names were covered by the old itemized list, so a sandboxed command could read them under the read-all posture. - Stop dropping default candidates that don't exist yet at profile-build time. A store created later in a long-lived sandboxed session (e.g. a concurrent ) previously got no deny rule at all; every backend already treats a deny rule over a not-yet-existing path as a harmless no-op that still takes effect once the path appears. - Resolve ZERO_OAUTH_TOKENS_PATH / ZERO_MCP_OAUTH_TOKENS_PATH overrides the same way the token stores resolve them (relative-to-cwd, no ~ expansion) instead of through normalizeProfilePath, which tilde-expands and so could derive a deny path different from where the store actually writes. Adds regression coverage for the directory-wide deny (including the migrated backup and synthetic temp-file siblings), for building the profile before the store directory exists, and for the tilde-override resolution mismatch. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(sandbox): address macOS deny-path test and config parity Recompute the expected Zero config deny path after MkdirAll and resolve the temp base with EvalSymlinks so macOS /var -> /private/var does not flake TestPermissionProfileDeniesZeroCredentialFiles. Add a parity test that sandbox.zeroUserConfigDir stays aligned with config.UserConfigDir. Co-authored-by: PierrunoYT <[email protected]> * fix(sandbox): harden credential deny paths * test(cli): normalize credential deny baseline * fix(sandbox): close credential path gaps * test(sandbox): normalize command credential path * fix(sandbox): handle absent credential paths safely * fix(sandbox): protect MCP OAuth siblings * fix(sandbox): protect OAuth lock siblings * docs(sandbox): clarify Linux future-path limits * test(sandbox): probe credential store reads in the Linux smoke test The smoke test wired HOME/XDG_CONFIG_HOME at credential directories but never attempted a read through the sandbox, so it could not catch a real-bwrap regression for the paths this change protects. Write a secret into ~/.aws and $XDG_CONFIG_HOME/zero and add both to the deny-read table. * fix(sandbox): keep the credential deny usable, current, and unguessable Four problems with the credential deny baseline, all in the same mechanism. The whole-directory deny over $XDG_CONFIG_HOME/zero also hid the supported user plugin, specialist, and command roots, whose commands and scripts are executed through this sandbox, so an installed user plugin could no longer start. Narrowing the deny to itemized files was not an option: the stores publish through temporary names and a concurrent login can add a store that did not exist when the profile was built, and only a directory rule covers both. The directory deny therefore stays, and the non-secret subtrees are carved back out through a new DenyReadCarveouts field: seatbelt re-allows them after the deny (SBPL is last-match-wins) plus ancestor metadata so path resolution can reach them, and bubblewrap masks the directory with --perms 111 instead of 000 -- unlistable but traversable -- and re-binds the carveout read-only before the tmpfs is remounted read-only. A relative ZERO_OAUTH_TOKENS_PATH, ZERO_MCP_OAUTH_TOKENS_PATH, or XDG_CONFIG_HOME was resolved against the command directory only, while oauth.ResolveStorePath and mcp.ResolveTokenStorePath call filepath.Abs, i.e. resolve against the Zero process working directory. A command run from a subdirectory therefore had the wrong path denied and the real store stayed readable under the read-all posture. Relative candidates now resolve against both directories, since the process that resolves the value is not necessarily the one that writes the store. The regression test runs the command from a different directory than the process, which the previous tests did not. bubblewrap cannot mount over a path that is absent when the namespace is assembled, so a store created mid-session stayed readable through the live read-only host-root bind for the rest of a long-lived sandbox session. The directories Zero owns -- its config directory and the per-store publication directories -- are now reported as EnsureDenyReadDirs and created (0700) before the namespace is assembled, so their mask always exists. Third-party stores such as ~/.aws are still skipped when absent: Zero must not create them, and that limitation stays documented on the field. The fixed <store>.tmp publication sibling made the plaintext token, and via the same path its encryption key, observable to a same-user sandboxed process with write access to an override parent: it could wait for the known name and rename the opened inode into an allowed path. Both stores now publish through a per-store <store>.publish directory with a randomly-named file inside. The directory name is derived from the store path, so the profile can deny it up front, while the random name inside is what cannot be waited for or renamed away. A test keeps the duplicated suffix in step with oauth.PublicationDirSuffix. Also adds the seatbelt render coverage that was missing: dropping DenyReadIfExists from denyReadRules made every credential store readable again on macOS without failing a single test. * fix(sandbox): close credential deny review gaps Preserve process credential roots alongside command overrides while limiting host precreation and read carveouts to trusted process-derived paths. Canonicalize future credential roots through existing path aliases, reject symlink carveouts, and collapse automatic masks already covered by user or automatic directory denies. Normalize the policy golden and add regression coverage across profile, Seatbelt, and bubblewrap rendering. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): normalize credential carveout paths Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): canonicalize backend credential masks Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * test(sandbox): normalize backend mask expectations Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): fail closed for replaceable token stores Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): allow keyring token path overrides Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfa-8469-731e-87c0-b26b7641cf59 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): harden command credential roots Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-c723-74d9-a323-026040b94436 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): close credential deny review gaps Reapply credential denies after Seatbelt extension carveouts and model the legacy MCP token path as a migration input rather than an atomic writer. Retain Bubblewrap fail-closed markers for OAuth outputs exposed through readable carveouts, and add regressions for both platform policies. Amp-Thread-ID: https://ampcode.com/threads/T-019fbf55-f6b0-71fc-b384-652ba32b9645 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> Co-authored-by: Cursor Agent <[email protected]> Co-authored-by: PierrunoYT <[email protected]> Co-authored-by: Amp <[email protected]>	2026-08-10 07:33:29 +08:00
6 people	1057d74512	docs(readme,cli): document /new & zero upgrade, add Discord link, and sync help text (#489) ... * docs(readme): document /new, zero upgrade, and add Discord link Add the /new slash command and zero upgrade command to the README's command references, expand docs/UPDATE.md to cover the --apply install flow, and link the project Discord server. * docs(update): escape pipe in --endpoint table cell CodeRabbit flagged the unescaped | in the --endpoint placeholder breaking the Markdown table layout. * docs(readme-zh): bring /new and zero upgrade in line with README.md README_ZH.md had picked up the Discord link but was still missing the /new slash command and zero upgrade entries added to README.md, letting the translation drift out of sync. * cli: add upgrade to root help output * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * docs: make required update flag explicit in common commands zero update errors without --check or --apply, so the bare form in the common-commands examples was misleading. Addresses the Copilot finding from the PR #489 review. * docs: use a permanent (non-expiring) Discord invite in the READMEs * docs(cli): match upgrade help summary to download/verify/install wording Copilot flagged the root zero --help summary for upgrade as less specific than the download/verify/install phrasing already used in README.md and docs/UPDATE.md for the same command. Co-Authored-By: Claude Sonnet 5 <[email protected]> * docs: use discord.gg short link for the community invite discord.com/invite/<code> and discord.gg/<code> resolve to the same invite; discord.gg is the canonical short form to publish. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(update,readme-zh): keep npm --json stdout clean, add ZH Discussions route `zero upgrade --json` (and `zero update --apply --json`) on an npm-managed install wired npm's own stdout directly to the process stdout, so its progress output landed ahead of the JSON result and made it unparseable. Route npm's stdout into a buffer when JSON output is requested, only surfacing it in the error message if the install fails. README_ZH.md's Community section only pointed to Discord, unlike the English README, which reserves Discord for real-time chat and routes Q&A/ideas/sharing to GitHub Discussions per SUPPORT.md/CONTRIBUTING.md policy. Bring the Chinese section in line with the same split. Addresses the outstanding findings from jatmn's review on PR #489. Co-Authored-By: Claude Sonnet 5 <[email protected]> * docs(update): qualify npm-managed JSON output Document the npm subprocess output caveat instead of promising a single parseable JSON document for apply operations. Drop the JSON-specific updater plumbing and tests so the PR remains focused on documentation and help discovery. Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-8bcd-71bc-9bde-9b4c28b78b38 Co-authored-by: Pierre Bruno <[email protected]> * docs(update): align upgrade guidance Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): keep the upgrade recommendation about this machine `zero update --check --target windows-x64` answers a question about a different machine, but the new call to action told the user to run `zero upgrade`, which only ever installs onto this one — replacing the target-specific download guidance the flag used to print. The recommendation is now conditional: a current-platform check keeps it, and a cross-target check gets the manual instruction plus a note naming the machine `zero upgrade` would have touched instead. Also align two flag contracts with what the code does: `zero update --help` described --repo/--endpoint as check-only although apply honors them, and UPDATE.md did not mention that both are ignored on an npm-managed apply, which takes its release from the npm registry rather than GitHub. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): do not send a custom-source check to the default source `zero upgrade` is a fresh invocation: it does not inherit the --repo/--endpoint of the check that suggested it, so recommending it bare after a custom-source check told the user to install from a repository they never asked about. The check now records the flag it was given and the guidance repeats it, naming where a bare upgrade would install from instead. ZERO_UPDATE_RELEASE_URL is deliberately not recorded: every Check reads it from the environment, including the one inside Apply, so a bare upgrade already follows it and telling the user to repeat it would be wrong. Two smaller edges from the same review: a cross-target check on a host with no published release target no longer ends by describing what `zero upgrade` does there, since it does not work there at all; and the flag table and help text now carry the npm caveat the prose bullet already had, so docs and help agree. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): keep source guidance accurate Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]> Co-authored-by: Vasanthdev2004 <[email protected]> Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>	2026-08-10 07:32:53 +08:00
Anandan	f922cb378e	Add cross-session messaging (#884) ... * Add cross-session messaging * Harden cross-session messaging Authenticate sender metadata against live registry records, bound peer lifecycle state, preserve records across transient discovery failures, and make receipt delivery direct and bounded. Also serialize TUI peer approvals, harden platform runtime paths and transport deadlines, widen session references, and add regression coverage for identity spoofing, queue limits, path boundaries, and tool failures. * Support canonical macOS socket paths Resolve Unix transport directories to their physical path before applying strict no-symlink traversal, allowing the standard /var to /private/var macOS alias without weakening runtime-directory validation. * Fix macOS socket path regression test Create the canonicalization fixture beneath a guaranteed short Unix path so it exercises the primary endpoint on macOS, and report temporary-directory cleanup failures. * Harden peer messaging lifecycle * Fix Windows peer directory traversal * Handle Windows token-owned peer directories * Accept equivalent Windows ACL masks * Build Windows peer ACL explicitly * Pin Windows ACL trustee SIDs * Validate Windows peer ACL semantics * Propagate Windows peer handle cleanup errors * Use null handle for Windows close failure test * Test held-message eviction callback	2026-08-09 18:12:09 +05:30
Anandan	7f39a63067	Centralize agent context planning (#879) ... * Centralize agent context planning * Fix context plan request accounting * Freeze planned tool schemas	2026-08-08 16:40:17 +05:30
Anandan	ff608c7834	Reduce compaction context while preserving task state (#873) ... * Reduce compaction context while preserving task state * Fix durable compaction evidence identity * Preserve structured tool errors during compaction * Unify manual and automatic compaction * Report compaction truncation structurally * Preserve automatic compaction metadata	2026-08-08 11:26:27 +05:30
Anandan	57858a31d1	Improve source read efficiency and model compatibility (#867) ... * Improve source read efficiency and compatibility * Preserve lexical context in ranged reads * Fix bounded source read edge cases * Prevent overflow in ranged minified reads * Trim eager read tool schemas	2026-08-07 21:29:45 +05:30
KRATOS	cae0269e77	fix(tui): a copied NUL byte no longer panics the TUI on Windows (#876) ... syscall.StringToUTF16 PANICS rather than returning an error when the string contains a NUL — documented standard-library behaviour — and github.com/atotto/clipboard's Windows path calls it directly. So one NUL anywhere in a copied transcript selection killed the whole program: "zero: tui error: program was killed: program experienced a panic", reported from a real Windows machine. The NUL reaches the clipboard because ansi.Strip removes escape SEQUENCES and leaves C0 bytes untouched, and the transcript carries them from two independent directions: tool output (a binary read, git's -z NUL-separated listings — which this repo itself parses in files_git_sweep.go) and Zero's own card protocol, whose row prefixes are literally "\x00command-card\x00" and "\x00plan-card\x00". Sanitised at the WRITE rather than at each source, because the sources are unbounded: any tool result a user can select from is one, and a fix per source is a fix the next tool re-breaks. The same pass protects the OSC52 fallback, where a stray control byte terminates the escape sequence early and silently corrupts the copy instead of crashing — which is likely why this went unnoticed off Windows, where the identical NUL produces a bad copy rather than a dead process. \n and \t survive: a multi-line selection is the normal case and both are legal clipboard content. Fixes #875	2026-08-07 20:17:19 +08:00
Vasanth TandClaude Opus 5	cd0eb194e4	feat(tools): give tool results an image channel and add view_image (#843) ... * feat(tools): give tool results an image channel and add view_image Closes #824. tools.Result carried only text, so no tool could hand the model a picture. The sharpest edge was our own capture tooling: browser_screenshot, desktop_screenshot and browser_pdf wrote a PNG, set Meta["artifact_path"] and returned "Artifact captured: <path>". Nothing read that path afterwards, so the model asked for a screenshot, was told one existed, and could not look at it. Zero was never image-blind on the way in — zeroruntime.ImageBlock, Message.Images and internal/imageinput are mature. What was missing is the model-initiated leg. Result.Images is delivered as a following USER message, not on the tool result itself. Every provider drops images on a tool-role message: Anthropic maps a tool result to a tool_result block whose content is a string, Gemini to a functionResponse, and OpenAI guards its image content-parts to the user role with an explicit comment saying so. Attaching them to the tool result would have plumbed the whole path and delivered nothing. A separate message also keeps one tool result per tool call, which the providers validate. Blocks with no data, or a media type outside the provider allow-list, are dropped rather than sent — a rejected image would fail the whole turn, and the tool's text still stands on its own. The message names the tool, since the images arrive detached from their result and nothing else says which call produced them when several ran in one turn. view_image resolves through resolveScopedReadPath before imageinput.LoadFile. That scoping is load-bearing: LoadFile joins a relative path to the workspace root and otherwise takes the path as given, so without it the tool would read anywhere on disk. It is registered with the other scoped readers and declared read-only and thread-safe, matching read_file. The capture tools now attach what they wrote. A PDF or an unreadable file yields no image and the text stands alone, so a capture is never failed over its attachment. Co-Authored-By: Claude Opus 5 <[email protected]> * fix(agent): keep tool results contiguous when a tool returns an image The image messages are user-role, and the first version appended each one immediately after its own tool result. With two tool calls in a turn that produces tool_result, user, tool_result — and a user message between two tool results breaks strict provider replay. Anthropic makes it concrete. appendUserBlocks coalesces consecutive user content into one block list, and tool_result blocks must come first in that message, so the interleaved shape maps to: [tool_result, text, image, tool_result] which the API rejects. loop.go already documents this exact rule three lines below the change, for the self-correction feedback it defers for the same reason. Images are now collected during the tool loop and appended once every tool_result is recorded — including the aborted placeholders on the abort, stop-reason and repeated-failure paths, so an early exit cannot strand them mid-sequence either. The original test used a single tool call, which cannot exhibit the bug. The new one runs two calls where the first returns an image and asserts the image message does not fall between the first and last tool result. Also from review: view_image joins the "execute" tool category, which grants read_file but not view_image, so a specialist declared with tools: [execute] could not look at a screenshot it had just captured. Covered by a test that holds for every category granting read_file rather than naming one. And the core read-only set is now asserted by tool name, not only by count — a count still passes when one reader is swapped for another. Co-Authored-By: Claude Opus 5 <[email protected]> * test(agent): pin the tool-result/image ordering contract The assertions only tracked the first and last tool-result index, so four distinct broken orderings passed: a message wedged between the two tool results, a duplicated image flush, a duplicated tool result, and both results answering the same tool_call_id. Mutation-testing each of those against the real loop confirmed the old assertions were green on all four. Now it collects every tool-result index and id and requires exactly two, in c1-then-c2 order, at adjacent indexes, with the image following the last one and delivered exactly once. Adjacency is the assertion that carries the contract: Anthropic needs every tool_result block ahead of any other block in the user message they coalesce into, so anything between the two results is the 400 this change exists to avoid. The image is deliberately NOT pinned to exactly last+1. appendUserBlocks merges consecutive user-role content, so [tool_result, tool_result, text, image] maps to tool_result blocks first and replays fine; pinning would fail that valid shape while killing nothing the adjacency and count assertions do not already catch. messageShape reports the actual role ordering on failure, and the loop uses two ifs rather than a switch so an image left on a tool-role message is reported as the misplacement it is instead of "the image never reached the model". --------- Co-authored-by: Claude Opus 5 <[email protected]>	2026-08-06 21:35:41 +05:30
Vasanth T	4ca4fa75d3	fix(agent): end the additional_permissions retry loop (#864) ... * fix(agent): end the additional_permissions retry loop Asking the agent to run an ordinary read-only command on Windows could leave it oscillating between two rejections instead of executing: additional_permissions requires sandbox_permissions set to "with_additional_permissions" additional_permissions must include at least one of network or file_system, ... Both come from inlineAdditionalPermissionsProfile, and each names only the fix that ADDS something. The first pushes the model toward setting the flag; the second pushes it back toward requesting a permission the command does not need. Neither ever says to omit the field, which is the correct action for a read-only command, so the two messages form a two-state oscillator. Every rejection now carries that remedy. The schema half is why the object is attached at all. Neither field description said to omit them for ordinary commands, so a model that legitimately used with_additional_permissions once in a session had nothing telling it to stop attaching a permission object to the next command. Both descriptions now lead with "omit", and they are shared constants rather than duplicated verbatim in bash.go and exec_command.go, where they were free to drift. Rejection is still rejection. Nothing that was refused is now accepted on the strength of an empty or mismatched payload, and sandbox_permissions is never upgraded on the model's behalf; only the guidance changed. The one behavioural change is that an explicit additional_permissions: null is now treated as the omitted field, since a null grants nothing and carries no flag. That flips three shapes from reject to execute: null with use_default, with require_escalated, and with no mode at all. The escalated one still routes through the normal escalation prompt and is not a new capability — the identical request without the null key already behaved that way. A FLAGGED null still errors, so with_additional_permissions plus null is not silently downgraded. Fixes #845. Fixes #847. * fix(tools): condition the omit guidance on needed access, not read-only The description said to omit sandbox_permissions "for ordinary commands, including every read-only one". That overstates it: curl and gh are read-only and still need sandboxed network, so a model following it literally would omit the override on a command that genuinely needs one and hit the opposite failure. It now says to omit unless the command needs access the sandbox does not already give it. That is the claim that is actually true, and it is one a model can act on without first deciding what counts as read-only. The schema test asserted the wrong half too — a bare substring check for "read-only" pinned the overstatement rather than the contract. It now pins the conditional, with the reasoning recorded so the weaker assertion does not come back. Reported by CodeRabbit on #864.	2026-08-06 21:34:54 +05:30
Anandan	edf660ab85	perf: compact common test output (#846) ... * perf: compact common test output * test: strengthen command reducer guarantees * fix: tighten command output summary detection	2026-08-06 20:35:13 +05:30
Vasanth T	91b413c514	fix(sandbox): stop the Windows write jail honouring Everyone-granted paths (#865) ... * fix(sandbox): stop the Windows write jail honouring Everyone-granted paths The restricted token put the World SID (S-1-1-0, Everyone) in its restricted-SID list unconditionally, from the original sandbox baseline and with no comment saying why. A WRITE_RESTRICTED token runs two checks for a write and needs both: the normal one against its enabled SIDs, and a second against the restricted list. The write jail rests entirely on that second check only succeeding where Zero has explicitly ACL'd a capability SID. Everyone is a SID every principal carries, so on any path whose DACL grants Everyone write, the restricted half passed for free and confinement fell back to the ordinary user's own permissions -- the exact boundary this token exists to be stricter than. Reachable with no privilege, no symlink and no race: an Everyone-writable directory is enough, and share roots opened with Everyone:F and loose third-party installer ACLs supply them. Ruled out as the common case: C:\Users\Public\Documents grants BATCH, not Everyone. The runner already stated the rule this broke -- it declines to add the user SID, Administrators or SYSTEM because "each has write access nearly everywhere" -- while listing Everyone among the restricted SIDs. Removed only from the WRITE_RESTRICTED token, because without that flag it is load-bearing rather than gratuitous. The restricted-SID check then applies to reads too, and default Windows DACLs grant BUILTIN\Users rather than anything in this list, so a token with no Everyone cannot open cmd.exe and dies at launch with STATUS_ACCESS_DENIED (0xC0000022). Dropping it there was tried first and takes TestWindowsUnelevatedRealSandboxSmoke with it. That path is taken only when a profile configures DenyRead, which is already the posture trading capability for read-deny enforcement (#612), so the bypass survives there deliberately and narrowly; closing it needs a read-side grant that is not a universal group. Verified against real tokens rather than reasoned about. The new smoke test drives the real runner and asserts three outcomes that only agree when the jail is intact: a granted root is writable, an ordinary outside path is denied, and an Everyone-writable outside path is denied too. Before the change its third case wrote the file with exit 0; the first two were already correct, which is what made the gap invisible. Restoring the SID unconditionally turns it red again. * test(sandbox): make a denied write distinguishable from a runner failure The denial assertions expected exit 1, which the runner also returns for its own errors -- a failed marker validation, a bad argument, a token it could not build. Written that way the test passes both when the sandbox denies the write and when nothing ever ran, and the second case proves nothing while being indistinguishable from success. That is a poor property for any test and a bad one for this test, whose entire job is to notice a confinement regression. The leading "granted root is writable" assertion covered part of it, but said nothing about whether the two later commands launched. The denial commands now report a distinctive exit code the runner cannot produce for itself, so the code proves cmd.exe actually ran and its redirect was refused. Re-checked in both directions rather than assumed: the test passes against the fix, and restoring the World SID unconditionally fails it with "exit code = 0, want 77" -- still catching the bug it was written for. Raised by CodeRabbit on #865.	2026-08-04 18:51:35 +05:30
Vasanth T	021281ebba	fix(cli): report MCP servers that failed to start (#822) (#827) ... * fix(cli): report MCP servers that failed to start (#822) `zero mcp check` inspected only the error returned by registration and then printed "is reachable". RegisterTools is best-effort by design, so a server that fails to connect is recorded on the runtime and registration returns nil. That is right for startup, where one bad server must not stop Zero launching, and wrong here: this is the command a user runs when a server is misbehaving, and it told them the broken server was fine. The failure data already existed as SkippedServer{Name, Err}; nothing consumed it in this path. The check now fails with the recorded error, in both text and JSON output. Covered by a test that spawns a genuinely missing command rather than stubbing the runtime, because the defect was precisely a mismatch between what registration returns and what it records. Reverting the fix makes it print "MCP server broken is reachable. 0 tool(s) available." * test(cli): cover the unreachable --json contract The text path was covered and the JSON path was not, though the change adds its own return with a different payload: status "unreachable" plus a redacted error, rather than status "ok" with a tool count. A caller parsing that shape was relying on something nothing pinned. Reverting the fix now fails both cases. The JSON one is the more useful of the two, because the wrong output is still valid JSON and would decode cleanly into a caller expecting success.	2026-08-03 22:50:15 +05:30
3 people	8e266797a8	perf: improve harness context efficiency (#838) ... * Improve harness context efficiency * Address harness efficiency review findings * Make recaps idle and cancellable Delay recap generation until the session has been inactive, orient it around the active goal and next task, cancel stale work on activity, and render the result ephemerally above the composer. Also strengthen the formatter safety regression to exercise formatter-modified edit output. * Track every replace-all edit span Translate each replacement into its post-edit line range, including cumulative shifts from earlier replacements, so follow-up edits remain authorized for every model-supplied change. * Make review regressions cross-platform Verify replace-all tracking through the canonical edit tool path instead of a platform-sensitive temp path, and cover cancellation when recaps are disabled during generation. * fix(tools): derive SeenWhole from the recorded ranges SeenWhole was a flag set only by a SINGLE read covering the file, while SeenRange judged coverage from the accumulated ranges. The two then disagreed about identical reads: after reading lines 1-50 and 51-100 of a 100-line file, SeenRange(1,100) was true and SeenWhole was false. That matters because write_file gates overwrites on SeenWhole. A file past the read byte budget cannot be read whole in one call — the read is truncated and RecordSeenRange is correctly skipped, since the model did not see those lines — so no sequence of reads could ever set the flag, and the overwrite was refused permanently with: the intended change depends on content that has not been read exactly in this session. Read the affected lines with read_file, then retry. Advice that could not change the answer. edit_file was unaffected because it gates on SeenRange, so the escape was to edit rather than rewrite, but a model following the error text would loop. SeenWhole is now computed from the ranges against the file's recorded line count, so scoped reads that together cover a file make it writable and the error text becomes actionable. The observation carries that line count, and a read reporting a different one clears the accumulated ranges: they describe a shape the file no longer has. Coverage is judged by merging sorted ranges rather than walking every line against every range, which the previous SeenRange did — same answer, and it no longer scans line-by-line over a large file. Co-Authored-By: Claude Opus 5 <[email protected]> * test(tools): key the large-file test on the resolved path read_file resolves the workspace root through EvalSymlinks before recording anything, so the tracker is keyed on the resolved path. The test asserted against the raw t.TempDir() path, which is the same string on Windows and a different one on macOS, where the temp dir sits under /var — a symlink to /private/var. The SeenWhole assertions would have looked up a key that never existed: passing here, failing on macOS CI. Also switches the line generator to fmt.Fprintf (staticcheck QF1012). Both from CodeRabbit on #842. Co-Authored-By: Claude Opus 5 <[email protected]> * test(tools): actually switch the line generator to fmt.Fprintf The previous commit's message claimed this was done. It was not: the edit silently did not apply and I reported it fixed without re-reading the file, so builder.WriteString(fmt.Sprintf(...)) survived at the head — caught by @anandh8x, not by me. Verified this time by running the linter rather than by reading the diff: golangci-lint --enable-only unused,ineffassign,staticcheck ./internal/tools/ reports 0 issues. * fix: keep exact-read workflows recoverable * test: cover permission normalization wiring * fix: commit file observations after output budgeting * fix: defer file observations across retries * test: normalize patch line endings --------- Co-authored-by: Vasanthdev2004 <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>	2026-07-31 23:30:03 +05:30
Anandan	d37de9214b	fix: use OAuth credentials in provider health checks (#828) ... * fix: use OAuth credentials in provider health checks * fix(providerhealth): verify OAuth login for all probes * fix(providerhealth): retain API key redaction * fix(providerhealth): redact bare OAuth tokens * test(cli): isolate OAuth doctor config * test(cli): isolate doctor config path	2026-07-30 22:32:58 +05:30
Anandan	588635487f	feat(tui): replace /retitle with local /rename (#826) ... * Improve session naming and resume picker * Address session rename review feedback	2026-07-30 19:58:02 +05:30
Anandan	845df6c7d2	fix(tui): hide unavailable sidebar shortcut (#821) ... * fix(tui): hide unavailable sidebar shortcut * test(tui): cover medium sidebar footer hint	2026-07-30 17:33:08 +05:30
Anandan	15eb16f736	fix(tui): exclude permission waits from turn time (#820) ... * fix(tui): exclude permission waits from turn time * fix(tui): exclude permission waits from TTFT	2026-07-30 17:03:27 +05:30
4 people	15b7a5475c	fix(acp): interrupt idle reads on cancellation (#782) ... * fix(acp): interrupt idle reads on cancellation * fix(acp): make idle-read interruption per-Read, not per-Serve-call Addresses the P2 on #782. The previous design raced a single, whole-connection Once (endRead) between the read loop's own "I have a terminal result" claim and a watcher goroutine's "ctx is done, treat this as interrupted" claim. This has a real, deterministic failure mode that has nothing to do with goroutine-scheduling luck: bufio.Reader can obtain data AND a terminal error from ONE underlying Read call (io.Reader explicitly permits returning (n>0, err) together). If a line delimiter is found within that data, ReadBytes returns the line with a nil error for that call, but bufio caches the error internally and returns it — without ever touching the underlying reader again — on the very next ReadBytes call. If cancellation happens in the gap between that first, successful read and the next one (e.g. while a synchronously-dispatched handler is still writing its response), the watcher's ctx.Done() case fires and claims the Once BEFORE the read loop ever gets back to a ReadBytes call — so when the cached error then surfaces (with zero contact with the underlying reader, let alone anything closing it), it is misattributed as a cancellation-driven interruption and silently swallowed as a clean shutdown, hiding a genuine broken transport. The fix moves interruption-detection down to the Read call itself instead of bracketing the whole Serve loop. interruptibleReader wraps the connection's reader: each Read runs the underlying call on its own goroutine and races it against ctx.Done(). If the underlying call finishes first, its result is returned untouched — cancellation a moment later has no bearing on a call that already had its answer. If ctx.Done() fires first, the reader is closed (when closable) to nudge a genuinely blocked read, a generation counter is bumped, and the call still waits for and returns the underlying call's own result rather than a synthesized one. Serve then brackets each ReadBytes call (which may invoke the wrapped Read zero or more times — zero when bufio answers from its own buffer, including a previously-cached error) with a snapshot of that counter before and after. A read whose bracket shows no change was never raced against cancellation — either answered from the buffer or genuinely completed before ctx.Done() could matter — so its outcome is reported truthfully regardless of the ctx state by the time it surfaces. Only a bracket that changed proves this specific call actually took the cancellation branch. Verified the fix has teeth: reverted to the original implementation and confirmed the new regression test (which forces the exact bufio-caching sequence deterministically, with a panic guard proving the underlying reader is never called a second time) fails 100% of the time against it, then restored the fix and confirmed it passes. Also applied the two open review threads on the existing tests: readStarted and writeStarted are now buffered channels with non-blocking sends rather than close(), so a future change causing an extra read or write can't panic on a double-close. Validated with go build/vet/test on Windows, plus go vet/build and `go test -race ./internal/acp/... -count=50` (and count=100 focused on the cancellation/error tests) under WSL/Linux — this session's Windows host has no C toolchain for -race directly. * fix(acp): preserve decided read results and require explicit reader ownership Addresses jatmn's two #782 review findings: - interruptibleReader.Read's select between resultCh and ctx.Done() chooses pseudo-randomly when both are ready, so a real terminal read error decided just before cancellation could be misattributed to shutdown and silently swallowed. Re-check resultCh non-blockingly before claiming a call for cancellation. - NewConn inferred reader ownership from an io.Closer assertion and closed any reader that happened to implement it, including one a caller merely borrowed. Split into NewConn (never closes r) and NewOwnedConn (explicit ownership, closes r on cancellation), and switch the sole production call site (process stdin) to the latter. Also applies CodeRabbit's atomic.Int64 and leading-ctx-parameter nits. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(acp): synchronize read completion with cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-9e4e-7506-85ab-e151888bfd71 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): preserve read completion across cancellation Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): bound buffered error synchronization Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-483a-7119-92aa-4953a72f2a76 Co-authored-by: Pierre Bruno <[email protected]> * fix(acp): report shutdown transport errors Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * test(acp): inject cancellation context Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>	2026-07-29 09:46:55 +08:00
3 people	bdbda89f3e	fix(swarm): close lifecycle admission on shutdown (#776) ... * fix(swarm): close lifecycle admission on shutdown * fix(swarm): admit lifecycle work without holding the lock across Launch Addresses the P1 on #776 plus the two documentation P3s. [P1] beginLifecycleAdmission returned with lifecycleMu.RLock() held for the caller's entire duration — including the call into the injected MemberLauncher.Launch. Close needs the write lock before it can call s.cancel(), so a launcher that blocks in Launch waiting on its context (the ordinary shape of a launcher that waits for a slot, a daemon connection, or a sandbox handshake) deadlocked every Close caller: the launch held the read lock while Close waited for the write lock to reach the very cancellation the launch needed to unblock. beginLifecycleAdmission is now a counted ticket: it holds lifecycleMu only across the closed check and a lifecycleWork.Add(1), then returns a release func (lifecycleWork.Done) the caller defers. Close flips closed and releases the lock BEFORE calling cancel, then waits out lifecycleWork (admitted-but-unfinished work) followed by watchers (goroutines admitted work may still be starting) — so it remains a true completion barrier without holding any lock across external launcher code. Every call site (Spawn, Handoff, AdoptOrphans, the retry path in watch, and afterExit) moved to the ticket pattern. [P3] beginLifecycleAdmission's contract and the Scheduler/Close handoff are now both documented at the point that matters: the func-returning signature makes the release contract explicit in the type itself, and Scheduler's comment spells out why the read/write lock ordering guarantees exactly one of Scheduler or Close closes whatever scheduler ends up installed. Tests: TestCloseCancelsLauncherBlockedInLaunch reproduces the exact deadlock (spawn parked in Launch, Close in a second goroutine, both must complete once cancellation reaches the launcher) — verified it hangs under the old held-lock pattern by temporarily restoring it. TestCloseWaitsForAdmittedSpawn is the converse: Close must not return while an admitted spawn is still running. Validated with `go test -race ./internal/swarm/... -count=1` under WSL/Linux (this session's Windows host has no C toolchain for -race directly), plus `go vet ./...` under WSL and go build/vet/test on Windows. * fix(swarm): fail late queue admissions and dequeues after shutdown Addresses jatmn's two #776 review findings: - Close swept (clearQueue) every team's queue before waiting for admitted-but-in-flight lifecycle operations to finish. A Spawn, Handoff, or AdoptOrphans call that got its ticket right before closed flipped could still append to a team's queue after the sweep ran, stranding that spec forever (never launched, since shutdown began; never failed, since the sweep won't run again). Reorder Close to wait for lifecycleWork before sweeping queues, so every queue append from already-admitted work is visible to the sweep. - afterExitAdmitted dequeued a queued spec (t.onExit) and launched it unconditionally once its own ticket was granted, even though Close can flip closed in the gap between that dequeue (which makes the spec invisible to the clearQueue sweep) and the launch. Re-check closed after dequeuing so a spec pulled off the queue after shutdown began fails with ErrSwarmClosed instead of starting a new worker. Both regression tests are verified to fail against the pre-fix code (confirmed by temporarily reverting each fix and rerunning). Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(swarm): snapshot teams after lifecycle work stops Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-a4d5-7078-a80b-802bab23607e Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): reject late admitted launches during shutdown Re-check the closed state at the final launcher boundary so lifecycle work admitted before Close cannot start or retry a member after shutdown has already won the race. Add deterministic late-dispatch and scheduler-handoff coverage, stress concurrent Spawn and Close under the race detector, and keep the late-team snapshot regression isolated from launcher behavior. Amp-Thread-ID: https://ampcode.com/threads/T-019fa55a-4f7f-766e-a312-a7db232b9987 Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): reject post-close launch commits Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(swarm): gate retry relaunch on the post-close commit The post-Launch shutdown gate added for spawns and queue drains was missing from the bounded-retry path, which called the launcher and swapped in the new handle unguarded. Since FuncLauncher returns a live handle immediately without consulting its context, a Close landing between the pre-check and Launch's return left the watcher supervising a member relaunched after shutdown began — and recording that member's outcome on the task. Extract the commit gate as commitLaunch and route the relaunch through it via relaunchAdmitted, so an initial launch and a retry pass the same three gates: admission, the pre-Launch re-check, and the post-Launch commit. When shutdown wins, the handle is reaped and the watcher records the terminal failure instead of adopting it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>	2026-07-29 09:46:25 +08:00
Vasanth T	8acd42a3bd	fix(providers): retry provably pre-send transport failures (#447) (#750) ... * fix(providers): retry provably pre-send transport failures (#447) A flaky link that failed the streaming completion POST at connect time stopped the turn and forced a manual continue, because the shared retry layer never retries transport errors: a completion POST is non-idempotent, so a replay could duplicate billable work. That rationale does not apply to a PROVABLY pre-send failure, where no request bytes ever left this host: a replay cannot duplicate anything, exactly like a 429/503 where the server told us it did not accept the request. SendWithRetry now retries, with the existing backoff, only that narrow class: DNS resolution failure, a refused or unreachable dial, and a TLS handshake timeout. Every ambiguous or post-send failure stays unretried and is checked first so a mixed message can never be misread: connection reset, broken pipe, EOF, a bare i/o timeout, context deadline. All three providers route their completion POST through this layer, so one change covers them uniformly. This deliberately narrows the issue's ask (which listed connection reset) to the provably-safe subset, and keeps the reset-stays-unretried test green. Tests cover the retry-on-pre-send, no-retry-on-ambiguous, and predicate-classification paths. NOTE FOR REVIEW: this refines a deliberately conservative, documented no-retry stance. Opening it for the owner's sign-off on the stance, not as a settled call. * fix(providers): classify pre-send dials by errno so Windows retries too CodeRabbit flagged that the pre-send predicate matched only POSIX dial wording, so a refused or unreachable dial on Windows (whose text differs entirely: 'No connection could be made because the target machine actively refused it') would not be retried. Match the syscall errno via errors.Is instead: Go maps ECONNREFUSED / ENETUNREACH / EHOSTUNREACH / ECONNRESET to the host's real values, including the WSA codes on Windows, so classification is identical on every platform. Errno matching is also authoritative where the string markers were only heuristic, closing the theoretical substring gap. The string markers stay as a fallback for errors already flattened to text. ECONNRESET is added to the post-send exclusions by identity so a Windows reset is excluded too. Tests build the production error shape (net.OpError -> os.SyscallError -> errno) so the Windows path is exercised portably on every platform, plus an end-to-end SendWithRetry case for an errno-wrapped refused dial. Cross -compiled and vetted for linux and darwin. * fix(providers): scope pre-send retry to the dial phase and fail fast on permanent errors Addresses gnanam's two [Major] review findings on this PR. [Major] Pre-send errno classification was not scoped to the dial, so a post-send read/write carrying ECONNREFUSED/ENETUNREACH/EHOSTUNREACH (a route dropping mid-generation) was misclassified "provably pre-send" and could replay a non-idempotent completion POST up to 6 times. errors.Is matches an errno anywhere in the chain regardless of which operation raised it, so isPreSendTransportError now requires an Op=="dial" *net.OpError before admitting those errnos or their POSIX wording. That makes "no request bytes were sent" structural: a post-send failure can no longer replay a POST. The dial errno match still covers Windows (Go maps the WSA* codes), so the platform coverage this PR added is preserved. [Major] Permanent failures hung the agent ~60s on the 429-tuned schedule. NXDOMAIN (net.DNSError with IsNotFound) is now treated as permanent and not retried, and the pre-send path gets its own short bound and sub-second schedule (preSendMaxAttempts=3, 500ms/1s) instead of reusing the 2/4/8/16/30s rate-limit backoff, so a mistyped host or a dead local daemon fails in about 1.5s. [Minor] Fixed the stale SendWithRetry doc that still claimed transport errors are never replayed. Tests: post-send read/write errnos and NXDOMAIN now assert exactly 1 call (no replay); dial errnos plus the string fallback assert the pre-send cap; added the pre-send backoff schedule. Race detector and all provider packages green. * fix(providers): match Windows WSA dial errnos so refused dials retry on Windows too CodeRabbit caught a real Windows bug, which I verified with a real dial on Windows: errors.Is(err, syscall.ECONNREFUSED) returns FALSE for a refused dial there, because a real Windows dial carries the raw WSAECONNREFUSED (10061) while Go's syscall.ECONNREFUSED is a distinct value the net package never produces. The POSIX string fallback ("connection refused") does not match the Windows wording ("No connection could be made because the target machine actively refused it") either. So the pre-send dial classification silently never fired on Windows, the exact platform this PR set out to fix. Move the dial errnos into a platform-specific list (dialPreSendErrnos): POSIX everywhere, plus the WSA* codes on Windows. The existing wrapDialErrno fixtures wrap the Go syscall constants, which is why they passed on both platforms and missed this. Adds TestIsPreSendTransportErrorRealRefusedDial, which does a REAL refused dial and asserts pre-send classification, so the platform's true error shape is exercised on every OS. * fix(providers): exclude Plan 9 from the POSIX dial-errno build tag * fix(providers): keep the pre-send errno classification off Plan 9 retry.go referenced syscall.ECONNRESET and dialPreSendErrnos directly, but the errno definitions live in build-tagged files that exclude Plan 9 (which has no BSD-style errno constants), so the package stopped compiling for GOOS=plan9 on this branch even though it builds on the merge base. Move the two syscall touch-points behind per-platform helpers: retry.go now calls isConnResetErrno and ranges dialPreSendErrnos, both provided by retry_dialerrno.go, retry_dialerrno_windows.go, and a new retry_dialerrno_plan9.go. The Plan 9 variant supplies an empty errno list and a false isConnResetErrno, so retry.go falls back to its existing string-marker heuristics there. Behavior on linux/darwin/windows is unchanged (isConnResetErrno matches the same syscall.ECONNRESET as before). retry_test.go is tagged !plan9 because it constructs real syscall.Errno dial shapes. Verified: GOOS=plan9 build and vet now pass; windows and linux build, vet, gofmt, and the full providerio test suite stay green. * fix(providers): stop pre-send retry from replaying a redirected POST Addresses jatmn's review on #750. [P1] client.Do could follow a 307/308 after already transmitting the original POST, and a dial failure to the redirect target then satisfied isPreSendTransportError and replayed the request, re-billing a completion the first host already received. SendWithRetry now disables redirect following (CheckRedirect returns http.ErrUseLastResponse on a cloned client), so any dial/DNS/TLS error from Do can only come from the single initial request and the pre-send classification is sound. A 3xx surfaces as the response for the caller's existing non-2xx handling. Regression test asserts the POST is sent once and the 307 is not followed. [P2] The pre-send retry used the loop-wide attempt counter shared with 429/503 retries, so a couple of status retries exhausted the pre-send budget and the first dial/DNS/TLS blip after them got no retry. Track pre-send failures and their backoff ordinal in a separate counter. Regression test covers a status-then-transport-failure sequence. [P3] Correct the Plan 9 and wrapDialErrno comments: the Plan 9 string-marker fallback keeps the pre-send inclusions and the separate reset exclusion, and the errno fixtures do not reproduce a real Windows dial (that carries distinct WSA errnos), which the real-dial test covers. * fix(providers): decouple the status retry budget from pre-send retries Addresses jatmn's follow-up on #750. The previous change gave pre-send failures their own counter, but every retry still advanced the loop-wide attempt counter that the 429/503 branch used. So the coupling survived in the opposite direction: two safely replayed refused dials shortened the later status retry window and started its backoff at a later rung. Give the status path its own statusAttempts counter and backoff ordinal. With both budgets tracked separately the shared loop counter has no remaining reader, so the loop no longer carries one. Behaviour is unchanged when only one kind of failure occurs: the first retryable status still waits on rung 1 and still gets maxAttempts requests. Adds TestSendWithRetryStatusBudgetSurvivesPreSendRetries, the inverse of the existing mixed-sequence test: two refused dials followed by rate limiting must still leave the status path its full window. * fix(providers): keep following redirects, but never replay a redirected POST Addresses jatmn's follow-up on #750. Blocking redirects outright was too blunt. SendWithRetry is the shared completion path, so replacing every caller's CheckRedirect with ErrUseLastResponse turned a legitimate 307/308 from a redirecting OpenAI-compatible gateway into a terminal non-2xx that providers report as an upstream error, and it discarded an injected client's redirect policy. Observe the redirect instead of preventing it: a per-attempt flag is set from CheckRedirect, which then delegates to the caller's policy when one is set and otherwise mirrors net/http's default ten-hop cap. The pre-send retry is gated on that flag, so an attempt that entered a redirect is never replayed while redirecting endpoints keep working. The safety property is unchanged, since a dial failure on a redirect hop follows a POST the origin already received. Adds three regressions: a redirect is followed through to the final response, a dial failure after a redirect is not replayed, and a caller-supplied CheckRedirect still runs and still decides. * test(providerio): pin the redirect asymmetry for retryable statuses A reviewer read the redirect guard as incomplete, because it stops a redirected POST being replayed after a transport error but not after a 429/503/529. The asymmetry is intentional, and nothing recorded that, so the next reader would reasonably reach the same conclusion. The two cases differ in what is known about the request. A transport failure after a redirect leaves it unknown whether the redirect target received and began processing the POST, so replaying could duplicate billable work. A retryable status means every hop answered: the 307/308 is the origin declining and delegating, and the 429 is the target declining as well, so nothing accepted the request and a replay duplicates nothing. The exposure is also the same as a retryable status with no redirect involved, which this package has always retried, so guarding here would not close a gap. It would make a rate-limited completion behind a redirecting gateway fail outright rather than back off, which is the regression class already raised against disabling redirects wholesale. Adds the case as a test asserting the intended behaviour, plus a comment at the branch explaining why it is not guarded. * fix(providers): retry connect timeouts and proxy CONNECT failures Three classification gaps against #447, all raised in review. A connect-phase timeout was never retried. It is provably pre-send for the same structural reason a refused dial is, since the connection was never established and no request bytes left this host, but it arrives carrying "i/o timeout" and the blanket wording exclusion rejected it before the connect gate could admit it. Intermittent connect timeouts are exactly the transient failure the issue is about, so the most common case was the one not covered. It is now admitted ahead of the wording exclusions, gated on the connect phase so a post-send read timeout, which is also Timeout(), still cannot replay a POST. A failed CONNECT through an HTTP proxy was never retried either. net tags it Op "proxyconnect" rather than "dial", so it missed the gate entirely. Same guarantee applies: the tunnel to the model host was never built. The gate now accepts both, which matters most in the environment where transient connect failures are most common. ETIMEDOUT and WSAETIMEDOUT join the errno lists so a connect timeout still classifies if it reaches us with the errno but without the timeout flag. The pre-send budget also now resets after a hop that reaches the server. It is meant to bound consecutive connect failures, not the lifetime of the call, and without the reset an early run of blips permanently spent it, so a later blip surfaced as a transport error while the status budget still had room. That is the coupling the two counters were split apart to remove, in the one ordering the earlier tests missed. Total work stays bounded, since every reset costs a status attempt and those are capped. Also corrects the SendWithAuthRetry comment, which still claimed the inner path retried only 429/503/529. Tests cover dial and proxyconnect timeouts as retryable and read/write timeouts as not, a refused proxy CONNECT, and the interleaved pre-send/status/pre-send sequence. Each fix fails its test when reverted.	2026-07-28 22:42:01 +08:00
Vasanth T	1c38e8cba5	fix(config): make the provider-command timeout an upper bound (#811) (#813) ... providerCommandTimeout bounded only the wait, so the call could take arbitrarily longer than the 5s its error text promised. Two phases sat outside it. Starting the process ran before the clock began. On Windows that is CREATE_SUSPENDED, job-object creation and assignment, and a system-wide thread snapshot, none of it fast on a cold or contended machine. The deadline is now armed first and the start races it, so process creation is inside the budget. Draining after termination was unbounded too: the timer path called Terminate and then received on the wait with no limit. Terminate normally makes that immediate, which is exactly why it survived unnoticed, but a tree that resists termination held the call open long past the deadline that had just expired. That receive is now bounded, and a call that gives up leaves cmd.Wait running, so the output buffers are synchronised: the I/O pump keeps writing into buffers the caller already holds, which a plain bytes.Buffer cannot survive. The timeout error also carried nothing actionable. It was a bare string with the underlying error and the command's stderr both dropped, so a user whose provider command was slow got no clue which phase ran out or what the command had said. It now names the phase, wraps the cause, and includes the redacted output the failure path already had. Both bounds are asserted on extracted helpers rather than end to end, because end to end they are untestable: a terminated command stops writing and its wait returns at once, so an unbounded drain and an unsynchronised buffer both pass a whole-call test. The first version of these tests did exactly that and proved nothing. Removing either bound now fails, as does arming the deadline after the start, which reports the wrong phase.	2026-07-28 22:41:09 +08:00
Felix-Ayush	58daea7a48	feat(providers): add Fireworks AI provider preset (#772) ... * feat(providers): add Fireworks AI provider preset Add an OpenAI-compatible Fireworks catalog entry with curated coding models, models.dev alias mapping, onboarding tests, and README setup. * fix(provider): use correct Fireworks DeepSeek V3 model ID Update default and catalog model IDs to deepseek-v3-0324 per Fireworks API.	2026-07-28 22:40:41 +08:00
Vasanth T	3e7692d8f0	fix(sandbox): scrub the daemon token file pointer (#677) (#818) ... scrubSensitiveEnv removed ZERO_DAEMON_REMOTE_TOKEN but not its file counterpart. TokenFromEnv accepts either form, so a sandboxed command still inherited the path to the bridge token, and the default posture is read-all, so it could then read the token itself. Scrubbing the pointer is the whole fix rather than half of one: the path comes from this variable alone and TokenFromEnv has no default location to fall back on, so a command that never sees the variable has nothing to guess at. This mirrors GOOGLE_APPLICATION_CREDENTIALS, which is already scrubbed for the same reason. Extends the existing scrub table, which compares the surviving environment exactly, so the case fails if the entry is dropped. Verified: without the fix the assertion reports ZERO_DAEMON_REMOTE_TOKEN_FILE surviving into the scrubbed environment.	2026-07-28 21:56:28 +08:00
Vasanth T	33c94ec7a3	fix(sandbox): deny reads of git's credential stores (#815) (#816) ... * fix(sandbox): deny reads of git's credential stores (#815) git's credential-store backend keeps host passwords and personal access tokens in cleartext, at ~/.git-credentials or ~/.config/git/credentials depending on whether the user is on the XDG layout. Neither was on the deny list, so any command the agent runs could read them. Scoped to those two files deliberately. #815 also covers SSH private keys and the GPG keyring, and those are a harder trade: denying ~/.ssh stops a sandboxed git push over SSH from working, and excluding that directory is not even sufficient, since an IdentityFile directive can point key material anywhere. These two cost nothing by comparison, because git reads them to authenticate rather than to know who the user is, so a sandboxed git keeps working and simply cannot act as the user. The XDG one denies the file rather than the directory around it, which is the opposite of the choice made for ~/.config/zero and is deliberate: ~/.config/git also holds the global git config, which userGitConfigReadPaths grants on purpose so a sandboxed git can read user.name and aliases instead of failing outright. Denying the directory would take that away. Tests assert both stores are denied, that the git config beside the XDG one stays readable, and that an explicit read grant still re-includes them like every other entry. Dropping either entry fails the first; widening the XDG entry to the directory fails the config assertion. * test(sandbox): assert the credential deny exists before the grant lifts it The allow-read case only checked that the credential store was absent from the deny list once an explicit grant covered it. A deny rule that was never added satisfies that just as well, so the test passed with the production change reverted and proved nothing about the interaction it was named for. Check both runs: denied with no grant, absent with one. The first leg is what establishes there is something for the grant to override.	2026-07-28 18:31:33 +08:00
Vasanth T	c9f9dc02ef	test(tools): stop internal/tools failing on stock Windows (#814) ... * test(tools): stop internal/tools failing on stock Windows This package fails on every run on a Windows box without PowerShell 7, which is what a stock install is: PowerShell 7 is a separate download. CI never sees any of it, because the runners have 7. Three distinct causes, all surfaced by the move to PowerShell in #804. The stop-intent test waited a fixed second for a session to die and asserted on whatever was true by then. Under cmd.exe that was enough; under PowerShell the tree is slower to stand up and tear down, and the FIRST stop in a fresh process pays the interpreter's cold start, so it routinely returned with termination under way but unfinished. The assertion read that as a failed stop. It now re-sends the stop until the session reports it has exited. Repeating is safe, since Terminate is a no-op against an already-dead tree, and it keeps the interrupt flag that a poll with empty chars would drop. Separately, and pre-existing, every test that starts a session hit a teardown race: a terminated process does not release its handles the instant the session reports exited, and Windows refuses to remove a directory with an open handle where POSIX does not. t.TempDir removes once and fails the test on the sharing violation, so ordinary teardown timing was reported as a broken test. Those roots now clean up with a bounded retry, and give up quietly rather than failing: the process is already terminated, so the worst case is a stale directory under the OS temp root. Last, the MSYS guard test asserted a windows_shell_syntax block using a command containing `&&`. On PowerShell 5.1 the windows_powershell_version preflight added by #804 fires first and shadows it. The `&&` was incidental to what that case is about, so it is gone. This one is worth noting as a shape rather than a typo: a new preflight silently changed which block an older assertion got, and only on the shell CI does not run. Verified by running the package repeatedly rather than once: previously failing on every run, now passing 3 of 3, and the stop-intent test passing 8 of 8 alone and 6 of 6 under CPU contention. Disabling the terminate path still fails it, so the assertion continues to bind. * test(tools): assert the stop intent killed the session The retry loop added to survive a slow PowerShell teardown also made the test unfalsifiable. The helper sleeps 5s, well inside the 30s deadline, so a terminate that did nothing at all would let the sleep end on its own and every existing assertion would still hold: the session reports exited, it reports an exit code, and the interrupted flag echoes the request rather than the outcome. Neutering KillProcessTree confirmed it, the test passed. Assert instead that the helper's completion line never appears, which distinguishes a killed session from a finished one without depending on how long the loop took. Output is accumulated across every poll rather than read off the last result, because each continuation returns only what the session produced since the previous collection.	2026-07-28 18:00:41 +08:00
Vasanth T	ec3b822505	fix(tui): paste clipboard images with Ctrl+V (#534) (#807) ... * fix(tui): paste clipboard images with Ctrl+V Ctrl+V did nothing when the clipboard held a screenshot, while right-click paste attached it. The image plumbing was already there: routePaste probes the OS clipboard for an image whenever a paste arrives with empty text. The gap is that a clipboard holding only an image produces no bracketed paste at all, since the terminal has no text to send, so routePaste never runs and its probe never fires. Right-click reached the probe only because that path always delivers a clipboardReadMsg, empty or not. Bubble's own Ctrl+V binding stays disabled, so nothing else was listening for the key. Handle Ctrl+V by issuing the image probe directly. It reads image content only, never text, so bracketed paste remains the single text path and a Ctrl+V with text on the clipboard is unaffected. The probe yields no message when there is no image, keeping it a silent no-op, and it is gated on noBlockingModal so a permission prompt or picker still swallows the key. readClipboardImage becomes a var so the tests stub it instead of depending on the machine's real clipboard. Fixes #534 * fix(tui): also probe the clipboard on Cmd+V, and test the real key route Two things raised in review, both fair. The handler matched keyCtrl only, which is ModCtrl. macOS reports Command as ModSuper, as the rest of the TUI already models it, so Cmd+V never reached the image probe even though the change claimed to cover it. It now matches both, via a keySuper helper alongside the existing modifier helpers. Worth being precise about the limit: this only helps where the terminal delivers the key to the application. A terminal that handles Cmd+V itself pastes the clipboard TEXT and sends no key event at all, so an image-only clipboard still produces nothing for this to react to. That is a separate problem from the one being fixed here. The tests also did not exercise the route they were named for. One asserted that Update returned some non-nil command and then ran a freshly built probe; the other never called Update at all. Both now stub the reader BEFORE Update and run the command Update actually returned. That matters more than it sounds. Making the branch return a different non-nil command leaves the old assertion passing and fails the new one, which is exactly the hole: the old test proved a command was issued, not that it was the image probe. Removing the route entirely, or reverting to ModCtrl only, also fails now. * fix(imageinput): report an unreadable clipboard image instead of silence On macOS the clipboard reader shells to pngpaste or a PyObjC one-liner, and neither ships with the OS: pngpaste is Homebrew-only and Apple dropped the bundled PyObjC. A stock Mac therefore fails every time, and the failure was returned as (nil, nil), which the caller reads as "no image on the clipboard" and treats as a silent no-op. Pressing Ctrl+V produced nothing at all: no image, no error, no hint that anything was missing. The darwin path already knows the difference. It probes with `clipboard info` first and only reaches the extraction step once the clipboard is confirmed to hold an image, so a failure there is not absence, it is "there is an image and nothing here can read it". That now returns ErrClipboardImageUnreadable, which the existing error path turns into a transcript notice naming what to install. The defect predates this branch, but this branch is what makes the path reachable from Ctrl+V, so shipping without it would close #534 with a feature that is inert on one of the three platforms. Raised by gnanam, who measured it on a real Mac rather than inferring it. Deliberately narrow. The Linux arm cannot tell "no tool" from "no image" without probing separately, and Windows uses PowerShell, which is always present, so neither is touched. The test drives the whole path: stubbed reader returns the error, Ctrl+V issues the probe, and the assertion is that the failure reaches the transcript as an error row rather than stopping earlier. Swallowing the error again makes it fail. * test(imageinput): cover the darwin unreadable-clipboard decision itself The test added with the fix asserted around the change rather than on it. It substituted ReadClipboardImage with a stub returning the error, which proves the TUI surfaces an error handed to it, but never reaches readClipboardImageDarwin, where the decision lives. Reverting both production return sites to nil,nil left every package green, which is the definition of a test that is not testing the change. Caught by gnanam. readClipboardImageDarwin shells out twice and had no seam, so the two steps are now package-level vars, the same shape as the readClipboardImage var this change already relied on. That makes the case that matters reachable: the info probe succeeds and reports an image, and the extractor then fails. It cannot be arranged from outside the function, because from there both steps are one opaque call. Five cases, and the negative ones carry as much weight as the positive. An image with no working extractor, and an extractor that exits 0 with no bytes, must both be actionable. A clipboard offering no image, and a failing info probe, must both stay silent, because turning those into a notice would make every stray Ctrl+V nag the user. The extractor must not run at all in either silent case. Repeating gnanam's experiment now fails: reverting both return sites to nil,nil fails the two actionable cases and leaves the silent ones passing, which is exactly the discrimination the fix is supposed to add.	2026-07-28 17:51:28 +08:00
Anandan	81a5e6cb71	fix(sandbox): preserve user config and retry denied writes (#801) ... * fix(sandbox): preserve user config and retry denied writes Keep user identity and configuration paths discoverable while retaining managed cache and temporary storage. Classify native sandbox failures as typed recoverable denials so approved commands can retry outside the sandbox. Add regression coverage for global Git ignores, typed denial handling, and permission retry behavior. * fix(sandbox): tighten denial retry safeguards * test(sandbox): assert denial contract * fix(sandbox): trust typed denials and restore Cargo state Stop promoting command output keywords into recoverable unrestricted denials, so only structured adapter facts can enter the approved unsandboxed retry path. Restore the managed CARGO_HOME directory and environment override so Cargo remains writable while the caller's real home stays read-only. * fix(sandbox): mask preserved credential configs Preserving caller configuration made credential-bearing tool files discoverable under Linux's read-all sandbox profile. Add default and environment-overridden npm, GitHub CLI, netrc, Docker, Kubernetes, and Zero config paths to the credential deny set, with regression coverage. Clarify the typed-denial retry and platform read-model comments without changing the approval behavior. * fix(sandbox): protect custom gcloud config CLOUDSDK_CONFIG can relocate gcloud credentials outside the XDG-derived default. Include the configured directory in the credential deny profile and cover the override with a regression test. * fix(sandbox): deny zero credential directory * test(sandbox): keep zero credential directory deny path expectations	2026-07-27 15:35:04 +05:30
Anandan	5d1869e7bc	feat: add persistent session goals (#803) ... * feat: add persistent session goals * Fix goal run isolation and guards * Strengthen loop goal isolation test * Cover goal edit state transitions * fix(goal): bound automatic continuations Persist a provider-independent continuation limit, require approval for model-created goals, and keep resume, BTW, and loop runs from starting or charging unrelated goal work. Also reject trailing lifecycle arguments and bound persisted objective length. * fix(goal): resume continuations after btw	2026-07-27 13:22:09 +05:30
Anandan	518d2e53ba	fix(windows): use PowerShell for command execution (#804) ... * fix(windows): prefer PowerShell for command execution Detect PowerShell 7 or Windows PowerShell before falling back to cmd.exe, and share the selected host shell across agent tools and the interactive shell escape. Configure UTF-8 output, preserve restricted-sandbox MSYS safeguards, and add Windows coverage for pipelines, workspace writes, and interactive stdin sessions. * test(windows): align shell coverage with PowerShell Make Windows command fixtures and guidance assertions follow the selected host shell, preserve cmd fallback coverage, and use a genuine MSYS executable when PowerShell aliases are available. Remove the unsupported non-TTY PowerShell stdin assertion; Windows interactive stdin requires a separate PTY transport. * fix(sandbox): classify Windows executable suffixes Normalize .exe, .cmd, .bat, and .com command names before safety and network lookup so PowerShell invocations such as curl.cmd receive the same network permission handling as curl. Add analyzer and engine-level regressions for the Windows PowerShell command shape observed in CI. * fix(sandbox): normalize absolute Windows command paths Extract drive-letter and UNC basenames before removing shell escape characters so fully qualified Windows executables continue matching canonical safety and network classifications. Cover both drive and UNC curl paths with analyzer regressions. * fix(sandbox): normalize relative Windows paths Recognize drive-relative, explicit relative, and suffixed backslash executable paths before generic shell escape removal so Windows commands continue to match canonical sandbox risk entries. * fix(windows): harden PowerShell execution * fix(sandbox): classify PowerShell removal aliases	2026-07-27 13:21:47 +05:30
Vasanth T	d82b94defe	test(cli): order the prewarm probe against the turn (#809) ... * test(cli): order the prewarm probe against the turn TestRunExecOptimizedSessionUnderGate intermittently sees zero prewarm HEAD probes on the Windows runner. It polled for up to 5s after the run returned, reasoning that the deadline exceeded the 3s prewarm timeout so a slow probe could not flake it. That reasoning does not hold. The probe is issued on the run's context and runExec cancels that context as it returns, because the stop function signalContext hands back is a CancelFunc. Once the run is over the probe is cancelled rather than slow, so polling cannot observe one that has not already landed, at any deadline. Against a mock server the turn finishes in microseconds, which leaves the probe racing teardown. The turn now blocks until the probe arrives, giving it a window while the run is still alive. The wait is bounded, so a probe that never fires still fails the assertion with a useful message. This widens the window rather than closing it: prewarmTimeout is armed inside the probe goroutine, so holding the turn cannot extend it. The comment says so, and names prewarmTimeout as the thing to look at if this ever recurs. * test(cli): fail when the prewarm hold gives up The bounded wait let the turn proceed after its guard expired, so a probe that arrived late still left the count at 1 and the test passed. That proves the probe was eventually received, not that it preceded the turn, which is the only thing this test claims. Raised by CodeRabbit. The expiry is now recorded and checked before the count, since the count cannot tell the two apart. Verified by suppressing the probe, which makes the guard expire: the assertion fires with its own message rather than the count assertion, and the run takes the full guard duration.	2026-07-27 12:12:34 +08:00
3 people	88fb7b6192	fix(sandbox): add platform-specific write lock around grant state file (#752) (#755) ... * fix(sandbox): add platform-specific write lock around grant state file (#752) Add grants_lock_unix.go and grants_lock_windows.go with a lockStateFile method that serializes access to the persisted grant state across processes. Wire the lock into Grant, GrantCommandPrefix, Revoke, RevokePath, and Clear. Also add .gitattributes to force LF line endings for Go files so gofmt checks pass on Windows. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(sandbox): do not lock the grants file when there is no notice to consume Addresses jatmn's #755 finding. ConsumeMigrationNotice took the interprocess lock unconditionally, and lockStateFile MkdirAlls the grants directory and creates <grants>.lockfile. Every startup and every 'zero exec' calls this, so it turned reading the grant state into requiring a WRITABLE grants directory: a user with no grants file whose ZERO_SANDBOX_GRANTS_PATH (or its parent config directory) sits on a read-only mount failed outright with 'failed to migrate sandbox grants', where the pre-lock code read an empty state and wrote nothing. Peek at the state without the lock first and return early when no migration write is required. Only when one is — a schema/policy migration, or clearing NoticePending — take the lock and re-read under it, since the unlocked peek is a snapshot another process may have already consumed or migrated, and is meaningless by contract when needsMigration is set. Both regression tests are verified to fail against the pre-fix code. The read-only-directory test skips on Windows (mode bits do not deny writes) and as root (permissions are bypassed), so a portable companion asserts the same requirement by requiring that nothing is created at all. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <[email protected]>	2026-07-27 12:11:35 +08:00
Vasanth T	f5fa4ffc64	fix(config): stop billing provider-command fixtures to the 5s timeout (#810) ... Both background-child tests fail intermittently on the Windows runner, and have since #690. #800 and #802 each relaxed an assertion, which is why neither held: the deadline itself was the problem. The tests drove LoadProviderCommand, so the fixture's own startup was billed to the production 5s budget. On a loaded runner the outer timer won the select in runProviderCommand, and both symptoms follow from that. The nonzero-exit test saw a timeout instead of the exit status it asserts, because the timer preempted the shell's exit. The other test lost its PID file, because the timer's Terminate killed the background child before it could record a PID, putting the file permanently out of reach however long the assertion polled. Neither test is a claim about the 5s budget. They assert which error each cmd.Wait path yields and that the leftover descendant is killed, so they now call runProviderCommand directly with a budget of their own and the production deadline stops competing with process startup. Making the child outlive that budget lets both drop their elapsed-versus-lifetime bounds as well, so no wall-clock assertion is left in either test. That is only worth doing if the two timeout paths can be told apart, and they could not: the WaitDelay path and the outer-timer path both returned the same bare sentinel, so asserting on it would have been satisfied by the very timeout the tests exist to rule out. The WaitDelay path now wraps rather than replaces. Callers asking errors.Is for the sentinel are unaffected, so no user-visible message changes. The Windows fixture also waited for its ready marker by launching a second PowerShell. That cold start is pure overhead added after the PID is already written, so the wait now runs in cmd itself and is bounded. Verified by looping both tests under CPU contention: 7 failures in 16 iterations before, none in 32 after, with per-iteration time going from a spread of 2.3s to 66s down to a steady 2.1s. Removing the wrap makes the WaitDelay assertion fail, so it is load-bearing rather than decorative.	2026-07-27 10:00:32 +08:00
Felix-Ayush	9fbdc34155	feat(skills): report hash drift in skill info (#795) ... Mirror plugin info: compare on-disk SKILL.md to the skills.lock hash so edited installs are visible. Also drop the nonexistent skills lock command from EXTENDING.md.	2026-07-27 10:00:07 +08:00
3 people	583e6530bd	fix(lint): resolve static analysis baseline (#769) ... * fix(lint): resolve static analysis baseline * fix(lint): clear linux staticcheck findings Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-b2ee-7442-8a31-80b2cf9dd270 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Amp <[email protected]>	2026-07-27 09:59:33 +08:00