mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 13:31:26 -10:00
fix/opencode-session-header
728
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 anandh8x
|
daabe11164 | fix(providers): send stable session headers to OpenCode | ||
|
Fayupable
|
dcf229fecd |
refactor(tests): replace duplicated wait-for helpers with testutil.WaitFor (#1039)
Fixes #1023 |
||
|
github-actions[bot]
|
3fae0de2ea |
chore(main): release 0.9.0 (#1059)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
github-actions[bot]
|
4ba112a0eb |
chore(main): release 0.9.0 (#958)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>v0.9.0 |
||
|
euxaristiaandeuxaristia
|
b76c764b77 |
fix(providerio): close idle connections periodically to prevent HTTP/2 PROTOCOL_ERROR (#1058)
* fix(providerio): close idle connections periodically to prevent HTTP/2 PROTOCOL_ERROR OpenRouter and other HTTP/2 backends can silently reset or drop connections that sit idle in Go's client connection pool across user turns. When a subsequent turn reuses the stale connection, it fails on stream connect with PROTOCOL_ERROR. Launch a background idle connection closer on non-darwin platforms to evict idle pooled connections on a 30-second interval, forcing fresh handshakes after idle pauses while preserving connection reuse across fast agent turns. Refs #1057 * test(providerio): poll for idle connection eviction with bounded deadline Replace the fixed 60ms sleep in TestStartIdleConnCloserClosesIdleConnections with a 2-second bounded polling loop to avoid test flakes under heavy CI load or under the race detector. Refs #1057 --------- Co-authored-by: euxaristia <[email protected]> |
||
|
Anandan
|
d73ec81e2d |
feat(tui): interactive terminal attach for tty exec sessions (#1055)
* feat(execution): add a stdin-only write for interactive processes * feat(tui): attach overlay for interactive terminal sessions * feat(tui): add /attach for interactive terminal sessions * feat(tools): point the model at tty sessions for interactive prompts * feat(tui): open the terminal automatically for tty exec sessions * feat(tools): tell the model to keep polling while the user types * feat(tui): give attached terminal sessions the full viewport * feat(tools): route sudo through an escalated tty session * fix(tui): route the auto-attach tick to its run * fix(tui): defer auto-attach while another view owns the screen * fix(tui): keep bracketed-paste framing for attached sessions * fix(tools): let tty sessions run interactive commands * fix(tui): capture the auto-attach baseline before the exec starts |
||
|
Vladimir Babin
|
2479d946c1 |
fix(mcp): reject a server-initiated request as a streamable-HTTP response (#1054)
* fix(mcp): reject a server-initiated request as a streamable-HTTP response networkClient.request matched the decoded POST body to the in-flight call on id alone, so a server that answered tools/call with a method-bearing frame produced an empty, error-free result. Apply the same isRequestOrNotification guard the SSE paths already use and return a protocol error naming the method. Fixes #1050 Signed-off-by: Vladimir Babin <[email protected]> * fix(mcp): describe the unexpected frame by method, not as a request A method-bearing frame without an id is a notification, so the error should not call it a request. Signed-off-by: Vladimir Babin <[email protected]> --------- Signed-off-by: Vladimir Babin <[email protected]> |
||
|
Pujitha PaladuguandPujitha Paladugu
|
d76f7f6931 |
ci: run the action-summary test suite in zero-action-smoke (#1053)
Motivation: scripts/action-summary.mjs is invoked by the composite action in action.yml on every action run, and ships with a 12-test suite in scripts/action-summary.test.mjs. Neither the action-smoke workflow nor the main Go-focused ci.yml executed that suite, and the action-smoke workflow's paths filters did not even include those two script files, so a change to the helper or its tests triggered no CI job at all. Approach: add scripts/action-summary.mjs and scripts/action-summary.test.mjs to both the pull_request and push paths filters in .github/workflows/zero-action-smoke.yml, and add a Setup Node (actions/setup-node, node-version 24, matching the pin already used in release-artifacts.yml) step followed by a `node --test scripts/action-summary.test.mjs` step in the validate job, ahead of the existing action.yml validation steps. Validation: - `node --test scripts/action-summary.test.mjs` passes locally: 12/12 tests pass (Node v22.23.1; repo engines field requires >=18, CI pins 24). - Broke an assertion in a scratch copy of the test file outside the repo and reran `node --test`: 1 test failed and the process exited non-zero, confirming the new CI step actually gates on failures as the issue requires. - `python3 -c "import yaml; yaml.safe_load(...)"` confirms the edited workflow YAML still parses. - Confirmed .github/workflows/ci.yml has no Node step (Go-only pipeline), so zero-action-smoke.yml is the correct integration point. - Base branch CI (upstream/main) is currently green. Report: https://github.com/Gitlawb/zero/issues/1030 Assisted-by: claude-sonnet-5 (via Claude Code) Signed-off-by: Pujitha Paladugu <[email protected]> Co-authored-by: Pujitha Paladugu <[email protected]> |
||
|
Pujitha PaladuguandPujitha Paladugu
|
37d69c96ce |
ci: disable persisted checkout credentials on release workflows (#1052)
Motivation: release-artifacts.yml's three actions/checkout steps (ordinary checkout, workflow-dispatch requested-ref checkout, and the npm-publish job checkout) and pr-auto-review.yml's single checkout step all leave persist-credentials at its default, so the workflow token stays in the job's local git config for every later step, including build/test/package steps that never need it. ci.yml and zero-action-smoke.yml already disable this. Neither modified file runs git push/commit/tag: the GitHub Release publish step supplies its own explicit GH_TOKEN, and npm publishing uses OIDC trusted publishing, so removing the persisted credential changes no behavior. Approach: Add `persist-credentials: false` to each of the four checkout steps, matching the exact convention already used in ci.yml/zero-action-smoke.yml. Validation: - go build ./... and go vet ./... pass. - gofmt -l . reports nothing. - go test ./internal/installtest/... passes. - actionlint on both changed files reports only two pre-existing shellcheck warnings in release-artifacts.yml (SC2016, SC2034) that are unrelated to this diff and were confirmed present before the change as well (verified by stashing and re-running actionlint against the original files). - A live GitHub Actions run of the release/publish workflows was not reproducible locally; this change only disables a default the workflows never rely on, so no functional change is expected. Report: https://github.com/Gitlawb/zero/issues/1029 Assisted-by: claude-sonnet-5 (via Claude Code) Signed-off-by: Pujitha Paladugu <[email protected]> Co-authored-by: Pujitha Paladugu <[email protected]> |
||
|
Pujitha PaladuguandPujitha Paladugu
|
176f5a242b |
fix(oauth): force-close loopback callback connections past the shutdown deadline (#1051)
* fix(oauth): force-close loopback callback connections past the shutdown deadline Motivation: LoopbackListener.Close shuts the callback server down with a 1-second context and discards the result. http.Server.Shutdown only closes idle connections, so a client that sends a partial request header and never finishes it counts as active; Shutdown blocks out the full deadline, times out, and Close returns while that connection and its Serve goroutine remain open indefinitely. This is a loopback-only (127.0.0.1), short-lived, single-use server, so the impact is a resource leak past the end of the flow, not a remote vulnerability or crash. Approach: - add a ReadHeaderTimeout to the server so a stalled request header is bounded on its own; - follow a timed-out or otherwise failed Shutdown with server.Close() to forcibly tear down any connection still open past the deadline; - join the Serve goroutine (via a done channel) before Close returns, so its completion is observable instead of left dangling. internal/mcp/oauth.go and internal/provideroauth/openrouter.go build their own loopback servers with the same pattern; they are left untouched here to keep this change scoped to the file the issue's reproduction demonstrates. Validation: - go build ./internal/oauth/... and go vet ./internal/oauth/...: clean. - gofmt -l internal/oauth/: clean (make fmt-check clean repo-wide). - go test ./internal/oauth/... -race -v -count=1: all pass, including three new tests (TestLoopbackCloseForceClosesSlowHeaderConnection, TestLoopbackCloseIsIdempotent, TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback). - Confirmed TestLoopbackCloseForceClosesSlowHeaderConnection fails against the pre-fix loopback.go (with the new test kept) and passes with the fix applied. - Full repo go build ./..., go vet ./..., and go test ./... pass. - Could not run make lint-static (golangci-lint) locally: it needs a very large dependency download and the local environment ran out of disk space fetching it. That check is continue-on-error (advisory) in .github/workflows/ci.yml, not a required gate. Report: https://github.com/Gitlawb/zero/issues/1028 Signed-off-by: Pujitha Paladugu <[email protected]> Assisted-by: claude-sonnet-5 (via Claude Code) * test(oauth): synchronize loopback regression tests instead of sleeping TestLoopbackCloseForceClosesSlowHeaderConnection relied on a fixed 100ms sleep to let Serve accept a connection before sending a stalled header; under load this could fail or silently skip the force-close path it's meant to exercise. Use an http.Server.ConnState hook (via a new unexported newLoopbackListener, set before Serve starts to avoid a race) to wait for the real StateNew signal instead. TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback measured Close's elapsed time without joining its http.Get goroutine or draining/closing the response body; join it first so the timing starts from a connection that's actually back to idle. Also register t.Cleanup(l.Close) in both tests so a Fatalf between construction and the existing explicit Close doesn't leak the listener's Serve goroutine. Signed-off-by: Pujitha Paladugu <[email protected]> --------- Signed-off-by: Pujitha Paladugu <[email protected]> Co-authored-by: Pujitha Paladugu <[email protected]> |
||
|
Anandan
|
9e609c5242 |
feat(exec): default headless runs to the turn-budget ceiling (#1049)
* feat(config): report whether the tool-turn budget was explicitly set ResolvedConfig.MaxTurns is ambiguous today: a configured 80 and the built-in default 80 are indistinguishable, so a caller that wants a different default cannot tell "user chose this" from "nobody set it". FileConfig now records at each merge gate (config files, provider command, ZERO_MAX_TURNS, CLI overrides) when a source supplies a positive maxTurns, and Resolve surfaces it as MaxTurnsSet. * feat(exec): default headless runs to the turn-budget ceiling The shared 80-turn default is sized for the TUI, where the user can say "continue" and the run resumes. Headless exec has nobody to do that, so benchmark evals truncated at 80 turns mid-implementation. An unset budget now resolves to MaxTurnsCeiling for exec only; any explicit source (flag, mode preset, ZERO_MAX_TURNS, config file) and exec-profile budgets keep their precedence. The TUI default is unchanged. * test(cli,config): cover the headless turn-budget default * refactor(cli): extract the headless turn-budget decision |
||
|
Anandan
|
2f531178f6 |
feat(tui): richer /resume session browser (#1047)
* feat(tui): support an optional detail line on picker rows
Add a faint second line under picker items so richer surfaces can show
secondary metadata (project, model, size) without crowding the label.
The line renders on the selection band, joins the search haystack, and
only appears at medium width and above; detail rows halve the visible
window so the overlay keeps its height, and mouse hit-testing maps a
click on either line to the same item.
* feat(tui): show project, model, and size under /resume rows
Each session row now carries a detail line with the project directory
(~-contracted), the model that ran it, and its event count, plus a
status chip for forks and tagged sessions. The line also joins the
filter haystack, so searching by project path or model name works.
Narrow terminals keep the existing single-line list.
* feat(tui): show relative last-activity age in /resume rows
Replace the absolute timestamp column with relative age ("2h ago",
"12d ago") so recency reads at a glance; distant history still shows
the calendar date. Also adds coverage for the detail line: rendering
at wide vs narrow tiers, status chips, and filtering by model.
* refactor(tui): drop the event count from /resume detail rows
Event count is an internal metric — the detail line now carries just
project, model, and the status chip, which are the fields that
actually discriminate sessions.
* fix(tui): sanitize session metadata before it reaches picker rows
Persisted titles (--session-title, /rename) and other metadata can
legally contain newlines; a stray line would break the picker's fixed
row geometry and misalign mouse hit-testing. Reuse sanitizeCardField
for the label and detail line, and cover the detail-line click path.
* fix(tui): strip carriage returns from rendered session metadata
A \r in a persisted title or model id rewinds the cursor to column 0
and overwrites the row. Fold it into sanitizeCardField alongside the
existing newline handling, and cover it in the picker test.
* fix(tui): pad picker detail lines on the selection band
The detail line only painted its text, so the selection highlight
stopped at the end of the text instead of spanning the row like the
label line above it. Pad the remainder with the row surface.
* test(tui): cover carriage returns in sanitizeCardField
* fix(tui): drop all control runes from rendered session metadata
The picker detail line newly renders cwd, model id, and tag, and any
of them can legally carry ESC/CSI/OSC/BEL/BS/VT/NEL bytes — a project
directory name containing \x1b[2J would repaint the terminal when the
picker opens. sanitizeCardField now maps line separators and tabs to
spaces and drops every remaining control rune, matching the package's
other sanitizers; this also closes the same hole for titles, which
predates the detail line.
* fix(tui): include exactly-30-day sessions in the relative-age branch
|
||
|
Vasanth T
|
3b43c43eca |
fix(sandbox): keep a derived ACL target inside the write root it came from (#1040)
* fix(sandbox): keep a derived ACL target inside the write root it came from FILE_FLAG_OPEN_REPARSE_POINT refuses a reparse point at the final path component and resolves every component above it, which is right for a path the operator named and not enough for one this package derived. The write-root carveouts are derived: <root>/.git/hooks and <root>/.git/config are constructed from a root, and <root>/.git is a name an unprivileged workspace writer can create before setup runs. mklink /J needs no privilege, so a junction there had the apply open <junction-target>/hooks, an ordinary directory with nothing wrong about its final component, and zero sandbox setup wrote a deny ACE on it as Administrator, outside the workspace. A derived entry now carries the root it came from, and the apply requires the object it finally holds to still live under it. The check is on the handle rather than the name: GetFinalPathNameByHandle answers where the open object actually is, and both sides go through it so the spelling normalizes the same way. Materialization is checked before it creates, since os.MkdirAll follows the same reparse points. Deliberately not a refusal of every reparse point on the path. Above the write root the path is the operator's, who may keep a workspace under a junction or a mapped directory; only the tail the sandbox appended is held strict. Closes #1024 * fix(sandbox): reject the reparse ancestor itself before materializing The pre-create check asked where the deepest existing ancestor lives, and a junction answers with its own path: the open does not follow a final-component reparse point, so <root>/.git came back as <root>/.git and matched. os.MkdirAll does follow it. What disqualifies that ancestor is that it IS a reparse point, not where it reports living. Found by reverting the check and watching the test still pass, which said the guard was doing nothing rather than that the test was weak. Pinned against the function now, because through the whole apply the create is made and then removed on the failure path, so the filesystem afterwards looks identical either way. The plan wiring gets its own test for the same reason: every apply-level case here hands the group an anchor directly, so none of them would notice the builder never setting one. * fix(sandbox): anchor only the carveouts that are under the root, and put the DACL back Two from review, both right. ReadOnlySubpaths is a profile field an operator can set to any path, and one placed outside the write root is a configuration that works today; anchoring it unconditionally turned that into a containment refusal. Only paths lexically under the root are anchored now, which is where the derived carveouts are anyway, and anything else keeps the final-component guard it always had. The successful-apply tests left their deny ACE in place, so t.TempDir could fail to remove the tree. Restoring the snapshot exposed the sharper half: the ACE denied the group the test runs as, which revoked its own WRITE_DAC and left the rollback unable to reopen the target. The tests deny a group this process is not a member of instead, which is what a capability SID is in production, and roll back afterwards. * test(sandbox): check the plan anchoring on every platform, out-of-root case included The plan builder is cross-platform and its anchor tests were in a Windows-only file, so nothing checked the wiring on Linux or macOS. Moved beside the other BuildWindowsACLPlan tests, with the case review raised: an out-of-root ReadOnlySubpath stays unanchored, and an in-root one is still held to its write root. * test(sandbox): give the cross-platform anchor tests native paths Moving these beside the other BuildWindowsACLPlan tests made them run on Linux and macOS, where their Windows path literals stop meaning what they say: pathWithinRoot is filepath.Rel underneath, a backslash is an ordinary character off Windows, and C:\workspace\.git\hooks is then one component that is not under C:\workspace, so every anchor came back empty. The neighbouring tests get away with such literals because they only compare strings they built the same way and never ask whether one contains another. Paths are built with filepath.Join from temp roots now, so the containment question is asked in the separator the running platform actually uses. |
||
|
KRATOS
|
faec8ee201 |
feat(acp): emit structured file changes in tool updates (#999)
* feat(tools): retain bounded structured file diffs * feat(acp): emit structured file changes in tool updates * fix(acp): harden structured file diff transport * fix(tools): reject unsafe rich diff previews * fix(acp): preserve safe rich diff semantics * fix(tools): bind structured diffs to committed changes * fix(acp): preserve structured diff identity * fix(tools): close rich diff race gaps * fix(tools): report partial patch paths from workspace * fix(tools): finalize structured diff evidence * fix(tools): separate unverified patch writes * test(tools): cover failed formatter recovery * fix(acp): serialize file deletions as nullable diffs * fix(tools): recreate files after formatter failure * test(acp): require nullable diff fields on wire |
||
|
KRATOS
|
c1937dfac7 |
feat(acp): add standard session list and resume (#914)
* feat(acp): add standard session list and resume
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): preserve notification wire order
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): bind sessions to persisted workspaces
Origin-Session: local-abff1c | Claude Code | 7 prompts
Origin-Snapshot: b7d0806d49f9
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): compare workspaces by filesystem identity, not by spelling
@Vasanthdev2004's three findings, all reproduced before changing anything.
TWO SPELLINGS OF ONE DIRECTORY WERE TWO WORKSPACES. Both comparisons were string
equality on ResolveWorkspaceRoot output, and that resolver is abs plus
filepath.Clean plus a stat — it does not fold case and does not resolve
junctions. A session persisted from the TUI was unresumable from an editor
holding a different spelling of the same project folder, and session/list
filtered by the other spelling returned nothing, which makes it an invisible
failure rather than a reported one. It lands on exactly the case this feature
exists for, and on the two processes most likely to disagree about spelling.
os.SameFile asks the filesystem which directories these are, which is the
question. filepath.EvalSymlinks is NOT the fix on Windows — it normalises a
drive letter and returns a junction path unchanged, so the alias survives it,
and junctions need no privilege. String equality stays as the fast path, and a
stat failure falls back to it rather than widening the match: this gate refuses
access to another workspace's files and configuration, so an unanswerable
comparison denies.
THE TEST COULD NOT SEE ANY OF IT. testDeps resolves with the identity function,
so the guard degenerated to "are these two strings different" fed two unrelated
temp directories — it could only ever answer yes. The rejection direction was
pinned and the acceptance direction was asserted nowhere. The new test uses a
resolver reproducing the production normalisation and drives the ACCEPTANCE
direction through an alias, skipping if the filesystem folds the alias away so
it never passes vacuously. Reverting to string equality fails it three ways:
load, resume, and a list that returns zero.
THE WIRE KEYS ARE AN EXTERNAL CONTRACT. Nothing pinned sessionId, cwd, title,
updatedAt, _meta, modelId, createdAt, sessions, nextCursor, cursor, loadSession,
promptCapabilities, sessionCapabilities, list or resume, so renaming a Go field
would break every client and leave the suite green. Renaming modelId to model_id
now fails.
Origin-Session: local-79d7a0 | Claude Code | 5 prompts
Origin-Snapshot: c175cabb9d50
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): test the alias guard where the bug lives, and stop listing unusable sessions
THE TEST COULD NOT RUN ON THE PLATFORM THE BUG IS FROM. @Vasanthdev2004's point,
and he is right that it is not a nit. The alias test built its second name with
os.Symlink, which needs a privilege an ordinary Windows session does not hold, so
it SKIPPED there — and Windows is where junctions exist and where this defect came
from. The identity guard was verified by CI on the two platforms that never had
the problem. It now builds the alias with mklink /J on Windows, which needs no
privilege and is how he found the defect in the first place, and keeps the symlink
arm elsewhere.
This is the second time in this series a correct fix shipped with a test that
could not exercise it: the same helper shape was added to internal/memory for the
same reason a day earlier.
A SESSION WITH NO PERSISTED WORKSPACE IS NOT RESUMABLE, SO IT IS NOT LISTED.
CodeRabbit's finding. activatePersistedSession refuses an empty Cwd, but the
listing advertised it anyway — a menu entry that only fails when taken. The test
asserts both halves, because the listing is only correct relative to what resume
will accept: the omitted session is checked to really fail on resume, and a usable
session is checked to survive the filter.
Mutation-checked: removing the filter advertises the unusable session again.
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): resolve every listed workspace, not only when a filter was supplied
@anandh8x's P1, both halves reproduced before changing anything.
The loop resolved item.Cwd only when a cwd filter was present, and skipped only a
blank one. Two shapes stayed on the menu that session/resume then refuses:
- a session whose persisted workspace has since been deleted, advertised as
resumable
- a legacy entry holding a relative path, reported as cwd "." although ACP
requires SessionInfo.cwd to be absolute
Every entry is now resolved unconditionally, anything that cannot resolve is
omitted, and the RESOLVED root is what SessionInfo carries — absolute as the
contract requires, and the same value the client hands back on resume. The
optional identity filter then applies to the resolved roots, which is also where
it belonged.
Listing is a menu: activatePersistedSession resolves and refuses what it cannot
reach, so anything this loop cannot resolve is something a client would be
offered and then denied.
Mutation-checked: restoring the resolve-only-when-filtered shape re-advertises
the deleted workspace and re-emits the relative cwd.
Origin-Session: local-76c8d7 | Claude Code | 6 prompts
Origin-Snapshot: 259b715cf0fd
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* test(acp): assert the relative entry is retained, not merely absolute
CodeRabbit's catch, and the test was genuinely weaker than it looked. It checked
that the deleted workspace was gone, the live one kept, and every listed cwd
absolute — all of which a "fix" that simply DISCARDED any non-absolute entry
would satisfy, while losing a resumable session.
Presence is now asserted separately from spelling: the relative entry must still
be listed, and listed with an absolute path.
Mutation-checked: skipping non-absolute entries instead of resolving them now
fails with "a session with a resolvable relative workspace was dropped rather
than normalised". The first attempt at that mutation did not compile, so it
proved nothing until it was rewritten — worth saying, because a mutation that
fails to build looks exactly like a test that passes.
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): a relative persisted workspace is omitted, not rebased
Reported by @anandh8x. Resolving a stored relative cwd does not recover the
session's workspace, it invents one: ResolveWorkspaceRoot joins it against
whatever directory the ACP server happens to be running in, and that invented
absolute path was then advertised as the session's workspace and accepted as
its home on resume.
Reproduced on f2c6fc9a, a session persisted with cwd ".":
LISTED legacy-rel as cwd="/Users/kratos/dev/f914/internal/acp"
resume with an UNRELATED workspace -> err=... cwd does not match its persisted workspace
resume with NO cwd (falls back to ".") -> err=<nil>
The mismatch check does its job when the client names a workspace, so the only
opening was the fallback path, where the rebased value was compared against
itself and always agreed. A conversation created for one project could be
resumed against another project's files, configuration and tools.
Both doors now take the same guard: handleSessionList omits an entry whose
persisted cwd is not absolute, and activatePersistedSession refuses one rather
than resolving it. The original base is not knowable from the metadata, so
guessing at it is not an option a fix can take.
This reverses an earlier assertion in TestSessionListResolvesEveryWorkspace,
which expected the relative entry to be normalised and retained. That was
requested in review on the grounds that dropping it loses a resumable session.
It does, but the entry was never resumable into its own workspace, only into
this process's. The test now asserts it is dropped, and a new
TestResumeRefusesARelativePersistedWorkspace covers the fallback path that the
listing filter alone leaves open.
Both guards mutation-checked: removing either one fails its test.
Pre-existing on this branch and on its merge-base, unrelated to this change:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider both exit 3 in this environment.
Origin-Session: local-8cd239 | Claude Code | 11 prompts
Origin-Snapshot: 365efe3045f2
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* test(acp): the resume guard is asserted through both activating methods
Raised by CodeRabbit: the test is named for resume and called session/load.
session/load and session/resume are separate entry points that today share
activatePersistedSession, so an assertion through either one passes while the
guard holds — but the name promised a surface it was not touching. Both are now
named explicitly, which keeps that true: if resume is ever given its own path,
this fails rather than quietly covering half of what it claims to.
With the guard removed, both methods accept a relative persisted workspace on
the no-cwd fallback. The named-workspace case was already refused by the
existing mismatch check; the fallback was the only door open, and it is open on
both.
Not taken in this PR, from the same review: threading a rooted directory handle
through ResolveWorkspaceRoot and workspace construction so a root rename or link
swap cannot redirect later file operations. That is a real question and a
pre-existing one — this change adds a refusal and no path handling — but it is a
capability refactor across workspace and tool access with its own race test, not
something to fold into a session-list fix. Worth its own issue.
Origin-Session: local-8cd239 | Claude Code | 11 prompts
Origin-Snapshot: 365efe3045f2
Origin-Session: local-13d543 | Claude Code | 5 prompts
Origin-Snapshot: 6b5eb8ba4e5b
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(acp): activation requires an absolute cwd in the REQUEST, not just the record
Reported by @jatmn.
ResumeSessionParams is a type alias for LoadSessionParams, so JSON decoding turns
an OMITTED resume cwd into an empty string. That blank reached the shared
activation path, whose blank-cwd fallback substitutes meta.Cwd — so
{"sessionId":"known"} activated a persisted session, even though ACP v1 requires
session/resume to carry an absolute working directory.
This is a different hole from the persisted-cwd one fixed earlier on this branch.
That guard asks whether the STORED workspace is identifiable; this asks whether
the CALLER named one at all. The earlier fix does not cover it, because a stored
absolute cwd passes that check and the blank request then silently inherits it.
requestedWorkspace validates the request's own cwd before anything reaches the
fallback: absent, empty and whitespace-only are all invalid params, and so is a
relative path. Applied to both activating methods rather than to resume alone —
load's omitted-cwd fallback was inheriting the same way, and leaving one door
open is how this class survived the last fix.
Mutation-checked at the wire, which is where the defect lives: making the blank
case return no error compiles and fails the regression on four separate requests
— session/load and session/resume, each with cwd omitted and with cwd blank. A
Go-level test would not have caught it, since the defect is in decoding an absent
field.
Two notes from a verification pass, neither a defect:
Error precedence changed: a blank cwd with an UNKNOWN session id now reports the
cwd problem instead of "session not found". Kept deliberately and pinned — it
stops the server confirming whether a session exists to a request that named no
workspace.
AdditionalDirectories is declared on two params structs and consumed nowhere in
the repo. Not a hole today, but it is the same shape — client-supplied paths with
no absoluteness rule — so the field now carries a note saying it must go through
requestedWorkspace when wired up.
Rebased onto
|
||
|
KRATOS
|
90014210d2 |
fix(daemon): publish status files atomically (#949)
* fix(daemon): publish status files atomically * fix(daemon): bind status publication to directory handle * fix(daemon): validate status directory access * fix(daemon): accept current token directory owner * fix(daemon): migrate owned runtime directories safely * fix(daemon): open Windows security handle relatively * fix(daemon): use current NT directory object * fix(observability): harden existing crash directories * fix(security): bind crash report creation to private root * fix(observability): atomically publish crash reports * fix(observability): preserve committed crash reports * fix(observability): revalidate committed crash path * fix(daemon): secure runtime lifecycle boundaries * fix(daemon): bind status ownership through cleanup * fix(daemon): preserve Windows status security * fix(daemon): retain trusted runtime root for lifecycle * fix(daemon): roll back redirected socket binds * fix(daemon): address status publication review |
||
|
Vasanth T
|
6937a309cf |
fix(tools): say when format-on-write ran out of time (#1044)
* fix(tools): say when format-on-write ran out of time The 10s bound exists so a wedged formatter cannot hang a tool call, which is right, and on expiry the unformatted write stood with nothing said. Every other fallback here is a standing fact about the environment (toggle off, no formatter for the extension, binary not installed) and is silent because nothing is wrong. A deadline firing is not that: formatting was configured, available and expected, and the file was written unformatted because the machine was slow. Left silent, the caller believes it wrote canonical style and finds out from a CI format check it cannot see, which is the failure this feature exists to prevent. The timeout is now reported on the write and edit summaries, naming the file and the formatter. The caller cancelling and the formatter exiting non-zero are deliberately not reported: the first is already surfaced as a cancelled run, and the second usually means content the formatter could not parse, which a write does not promise to fix. The bound stays 10s rather than being raised. Raising it makes a wedged formatter block longer, which is the thing the bound is for; what was missing was knowing when it fired. Closes #1022 * fix(tools): put the written bytes back when the formatter fails These formatters edit in place, so one killed by the deadline, killed by the caller, or exiting partway through its own rewrite can leave the target truncated: neither the input nor the output. Returning the written bytes on top of that left the tracker baseline and the diff preview describing a file that is not on disk, so the next edit would compare against content the file does not have. That is a worse failure than the missing formatting the previous commit set out to disclose. The failure paths write the bytes back, and when even that fails the user hears about it, since it is their file. Written back unconditionally on that path rather than only when the bytes differ: comparing first means reading the file to find out, and a read that fails leaves the same ambiguity this exists to remove. The fixture is proven to damage the file on a throwaway copy, because a successful restore erases the evidence from the real one. My first attempt asserted that on the target itself, where it could only pass vacuously or report the opposite of what it saw. |
||
|
Vasanth T
|
da244bcbfc |
fix(sessions): render resume context fields in key order (#1043)
* fix(sessions): render resume context fields in key order
extractText walked a map[string]any directly and Go randomizes map
iteration, so the same session put the same fields in a different order in
every process: one run rendered a tool call as c1 read_file {"path":...},
the next as {"path":...} c1 read_file. That block is part of the user
prompt, so the prompt itself changed run to run, which leaves a provider
nothing stable to prefix-cache for it and makes two resumes of one session
impossible to diff while debugging.
Only the ordering is decided here. The summary short-circuit is untouched,
a list keeps the order it was written in, and which fields survive at all
stays the caller's projection.
Pinned as exact rendered strings rather than as two renders agreeing: a
same-process repeat passes whenever the random order happens to repeat,
which for a small map is often. The raw-JSON case proved it while being
written, matching on the first five attempts before diverging on the sixth.
Closes #1020
* test(sessions): name the expected context block instead of the first render
Comparing later renders against the first only asks whether the renderer is
stable, and a renderer that is stably wrong passes it: any fixed field order
satisfies it, including the unsorted one this change replaces.
It also hid something. The events were tool calls, which
promptContextEvents filters out of the resume context on this branch, so
the block being compared was a single message line and nothing said so. The
test now names the block it expects, on events that survive the filter, and
both failures are visible.
|
||
|
Vasanth T
|
e34c8d2252 |
test(cli): skip the serve-scope symlink test when the platform refuses the link (#1042)
Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which an ordinary developer account does not hold, so this reported a failure with nothing wrong in the code: "A required privilege is not held by the client". CI runners are elevated, so it only ever appeared locally, where it cost a round of triage on #1003 before being ruled environmental and would do the same to any contributor on a stock Windows machine. The two other symlink tests in this package, and most of the rest of the repo, already answer this way. Swept the remaining os.Symlink-then-Fatal sites rather than assuming: internal/release passes, the one in internal/agent is never reached, and the unix-only files cannot hit it. This was the outlier. Closes #1021 |
||
|
Vasanth T
|
91d921f4c5 |
feat(tui): offer mid-run model escalation behind --allow-escalation (#1019)
* feat(tui): offer mid-run model escalation behind --allow-escalation Mid-run escalation existed but only exec offered it. The TUI already handled every consequence of a switch, re-deriving the compaction threshold "after a mid-run escalate_model switch" and resolving the summarizer against the active profile, while nothing on that surface could cause one: escalate_model was registered by exec alone. This closes that, opt-in, the same conservative way exec answers the question. Opt-in rather than default because escalation moves a run onto a different model, which changes what it costs and which provider sees the conversation. That is the operator's call, and the exec flag already answers it. The switchers are built per turn from m.providerProfile rather than once at launch. A TUI session can change models with /model, so a closure capturing the startup profile would escalate from whatever the session began with instead of what is in force now, and would carry that stale profile's base URL and credential with it. exec has no such problem because its profile cannot change mid-run, which is why the wiring lives in different places on the two surfaces. The nil-contract logic is now shared rather than copied. providers. EscalationSwitchers is the single implementation both surfaces call: the loop swaps only on a non-nil provider, so a (nil, nil) return has to leave everything untouched including the caller's usage attribution, and the session switcher is installed only when the run STARTED optimized so escalation cannot change the transport underneath a session. A second copy of that would only ever have been exercised on one surface. exec's behaviour is unchanged: its six existing escalation tests pass against the shared builder. The tool and the switchers ride on one flag and a test asserts they cannot be separated. Registering escalate_model without wiring a switcher ships a tool the loop silently ignores, so the model reports an escalation that never happened; that is the specific failure worth a guard rather than a comment. Note this does not answer all of #554. Assigning models per phase, plan with one and execute with another, is still not built. This makes the mechanism reachable from the interactive surface, which is the part that was one flag away. Refs #554 * fix(tui): attribute usage after an escalation to the model in force exec reassigns currentModel when the escalation switcher fires; the TUI captured usageModelID once per run and handed the switcher no callback, so every usage event after a mid-run escalation was billed to the model the run started on. The switcher now updates usageModelID, and the final response carries the model per usage event so the batch fallback does not swing the other way and bill the events before the switch to the escalated model. Covered by a run through a scripted escalation that checks both the live and the batch attribution. * fix(cli): accept the root flags in any order Each leading-flag splitter stops at the first token it does not own, so running them once in a fixed sequence made the order load-bearing: `zero --allow-escalation --theme auto` stranded the theme as an unknown command and exited with an argument error instead of launching, and the same happened after --skip-permissions-unsafe. The three splitters now run until they make no progress, at the root and after the unsafe flag, so every ordering reaches the TUI with every flag applied. A --theme written before --skip-permissions-unsafe was dropped on that path; it is kept now, with a later one winning as the last occurrence does. * fix(cli): forward a root --allow-escalation into exec, reject it elsewhere The root flag was stripped before dispatch, so `zero --allow-escalation exec ...` and `zero --allow-escalation -p ...` ran without the opt-in the help text promises, and `zero --allow-escalation version` accepted a flag it could only discard. It now follows --add-dir: re-synthesised into the exec argument list for the exec and -p shapes, so exec parses it as its own flag, and rejected loudly for every other command. The ordering test now generates every permutation of the three root flags, with --skip-permissions-unsafe absent or at any position. * fix(tui): record the model on an escalated run persisted usage events The live usage record carried the model in force; the persisted session payload did not. The usage report rebuilds cost from that payload and falls back to the session-wide model when an event does not name one, so an escalated interactive run was priced end to end at the model it started on. Written only under escalation, matching what exec records under the same flag, so an ordinary run persists the same compact payload as before. * test(cli): isolate the new launch helpers from real user state A temporary working directory isolates project files and nothing else. runWithDeps fills the dependencies a test leaves out with production ones, and the interactive launch path reads user config, opens stores, refreshes the models.dev cache and migrates any inline plaintext API key into the credential store before it reaches an injected runTUI callback. Running these tests on a developer machine therefore rewrote that developer's config.json and wrote a credential file beside it, and the results depended on whatever providers, MCP servers and plugins the machine had. Both new helpers now point every per-user base directory at a throwaway root, pin the models.dev cache path, disable its background fetch, and force the file credential backend so nothing reaches the host keyring. A new test seeds a config with an inline key outside those fixture roots and fails if either helper rewrites it or leaves a file beside it; without the isolation it reports the rewritten config and the credentials.enc files. * test(cli): check each launch helper isolation on its own t.Setenv restores at the end of the test, not when the helper returns, so one test exercising both helpers let the first isolation cover for the second. Each helper now seeds its own emulated user config in a subtest, and removing the isolation from either one fails that subtest. * test(cli): pin the isolation itself, not only what this platform writes The exec helper writes its models.dev cache from a background goroutine, so a footprint check could not prove its isolation deterministically. Each subtest now also asserts the helper moved the per-user paths off the seeded root and disabled the fetch and the keyring backend, which fails for either helper when the isolation call is removed. |
||
|
Vasanth T
|
6ae8396f02 |
fix(sessions): carry an interrupted turn's work into the resume prompt (#1016)
* fix(sessions): carry an interrupted turn's work into the resume prompt A turn that read six files and then died on a provider error left this behind for the next turn: - #1 message: add retries to the http client - #6 error: provider error: upstream timeout Nothing named the files, so the next turn re-read them from scratch (#913). The work was never lost from the session: tool calls and results are recorded as they happen, and the error path carries them out with everything else. promptContextEvents then dropped both types on the way into the prompt, so the record existed and nothing read it. Filtering them was deliberate (#460) and is right for work an answer already describes: forty read_file results add length and no information next to the assistant message explaining them. It is wrong only for work with no answer after it, and that is exactly what an interruption leaves. A turn that ends normally was survivable for that reason, which is why this went unnoticed: the answer was doing the remembering. So the events after the last assistant message come along and the rest stays filtered. The tail gets its own allowance rather than sharing the conversation budget, so a tool-heavy interrupted turn still cannot push an earlier message out of the context. That is the property #460 added the filter for and it is asserted directly. This changes a tested contract. TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow asserted that tool results are omitted, and its fixture is an interrupted turn: 43 results after an assistant answer with nothing speaking for them. That is the #913 shape, so no rule satisfies both and the contract had to move. The test keeps the guarantee it was written for, that conversation survives a noisy turn, and a counterpart now pins the other half: work an answer already covers is still filtered. Not fixed here, and worth saying plainly: this carries what was DONE, not what was READ. Zero has no conversation-history parameter at all, agent.Run seeds from the system prompt and the user prompt every time, so file contents cannot survive a turn boundary today. The next turn learns it already read a path and can decide, rather than starting blind. * fix(sessions): carry the tool outcome, not the tool output The tail was carrying whole tool-result events, so up to 500 bytes of raw tool output rode into a later turn prompt. Nothing redacts on the way in, and the prompt renderer truncates rather than filters, so the only thing standing between a file read and a later prompt was a length limit. It bought almost nothing. The tail exists so a resumed turn knows what the interrupted one did, and the CALL already says that: the tool name and its arguments, which is the path for a read. The result body is the one part of the pair that can carry file contents. The status is kept rather than dropping the result entirely. A bare call reads as work that succeeded, so a failed read would come back as a file the next turn believes it already has. Raised by CodeRabbit on the PR, and right. * test(sessions): scope the outcome assertion to the tool_result lines The status check searched the whole prompt for "error", which the provider error message in the same fixture also contains. Blanking the status out left the test passing, so it was asserting the presence of an unrelated line. Scoped to the tool_result lines, it fails on that mutation naming the outcome it lost. * test(sessions): pin the whole of every tool line, not a search for one string Two assertions here were weaker than what they claimed. Searching the prompt for the fixture's whole secret only rejects that exact string, so a change that carried a truncated or partly redacted prefix of the output into the prompt would have passed. And asking whether "ok" and "error" each appear somewhere among the result lines passes just as well with the two statuses swapped, or both hung off the wrong call. Both tool calls and both results are now pinned outright, which says what the trim actually promises: nothing but the tool name and the outcome survives a result, and the line order pairs each outcome with its own call. Compared as a set of fields per line rather than as a string, because the renderer walks the payload map and Go randomizes that order, so the same events come out in a different field order on every run. Reported by CodeRabbit on this PR. * fix(sessions): keep a tool call's identity and drop its payload in resume context toolResultOutcome strips a result down to name and status, and calls were left verbatim, so an interrupted write_file put its content into the next turn's prompt while the matching result body did not. Same secret, one door left open. Admitting calls is the #913 fix and has to stay: a resumed turn knowing which file was read is the whole point. What had to change is the symmetry. toolCallIdentity is the call-side half of toolResultOutcome. It decodes the arguments, keeps the fields that say what a call was about, and drops the ones that carry what it was about to write, run or search for. Paths, directories, urls, names, patterns and a read window survive; write_file's content, edit_file's old and new strings, apply_patch's hunks and a shell command line do not. An allow-list rather than a deny-list, so an argument this file has never seen is dropped rather than replayed. A new tool with a new body field then shows up as a missing path in a resume prompt, which is visible, instead of a new leak, which is not. Keyed by argument name rather than tool name, so an MCP tool whose argument is a url keeps it without being enumerated. Arguments that do not decode as an object are removed outright: text that could not be read is text that cannot be checked. The tail budget gets a comment rather than a change. Tail slots exist only when the conversation uses fewer than the 80-event cap, so a session already at the cap carries no interrupted tool work, which is what every session did before tool events were admitted and what the #460 cap is there to hold. Reserving a minimum tail by trimming conversation further is a product decision, and it is not this one. Reported by jatmn. * fix(sessions): scrub credentials out of retained tool-call values An allow-listed key is not a safe value. web_fetch accepts a credential in the query string and redacts the URL it reports back, so an interrupted fetch had its token dropped from the result and kept verbatim in the call, and this projection then carried it into the next resume or fork prompt. Retained string values now go through the same redaction web_fetch uses on its own returned URL: host, path and ordinary query survive, the token does not, and non-string values are untouched. * fix(sessions): project a tool call's top-level fields instead of patching one toolResultOutcome builds its output from the fields it keeps, so a field it has never heard of cannot reach a prompt through it. The call side edited arguments in place and returned every sibling key, and returned the payload untouched when there were no arguments at all, so a producer recording one more top-level field would put it into the next turn. Both halves now name what survives and drop the rest. * fix(sessions): name the value shapes a projected argument may keep, and scope the ambiguous aliases Two halves of the same gap: the projection decided which KEYS survive and said nothing about the values under them or about which tool gave them meaning. Values. Strings were scrubbed and everything else passed through, which reads as "a number is nothing to scrub" and is true of a number and not of an object. MCP schemas allow object and array properties, so {"query":{"api_key":"...","content":"private body"}} arrived under a permitted key and carried a whole payload into the next prompt. Redacting a serialized object would not have helped, since ordinary private text has no credential shape to match. The retained shapes are named now: a scrubbed string, and the scalars a read window is made of. A container is dropped and the call keeps its name and id, so a resumed turn still knows which tool ran. Meaning. grep accepts "search" for its pattern and edit_file accepts the same word for the text being replaced, so admitting it globally would have replayed file contents. It is scoped to grep, and the rule is kept as a test: an argument name any mutating tool accepts for body content belongs in the per-tool table or nowhere. The unambiguous supported forms are added to the shared table: glob's match, and read_file's start_line, end_line, max_lines, byte_offset and byte_limit. Driven through the store on both resume and fork, including a call the tool would have rejected, since the agent records OnToolCall before execution. The persisted events are asserted unchanged: this is a view for the prompt, not an edit to the record. * test(sessions): assert the window key with its value, not the number alone strings.Contains(out, "40") finds it inside "4096", so the start_line assertion passed whether or not start_line survived the projection, which is the opposite of what it claimed. The rendered arguments carry the pairs, so the assertions name them. |
||
|
beardthelion
|
2837f5efb3 |
fix(installtxn): put back an install a killed commit left behind (#997)
* fix(installtxn): put back an install a killed commit left behind
CommitDir publishes by two renames: the live target moves into a workspace
backup, then the staged copy is renamed into place. Neither is journaled, so a
process killed between them left the target absent with the install's only copy
retained in a workspace nothing ever read. plugins.Load and skills.Load
enumerate directories, so the extension simply disappeared, while the lockfile
went on listing it. Recovering needed one fact the transaction never wrote
down: which install a backup belonged to. CommitDir now records that before it
moves anything, and Recover puts the backup back when the target is absent.
Recovery is a second data-loss surface, so it is bounded on every side. It
refuses a name that is not a single element inside the install root, never
replaces a live target, identifies a workspace by the name StageDir gives one
rather than by contents alone, and leaves intact anything it cannot attribute.
Every path that takes the install lock recovers, not just the two that install.
Recovering on the install path alone is worse than not recovering: a removal
takes the not-present branch, drops the lockfile entry and reports success
while the backup it never looked at stays on disk, and the next install
republishes it, reinstating an extension the user deleted. Removal and the
terminalpet install hold the same lock and now do the same thing first.
Recovery stays an explicit call rather than a side effect of Lock, matching how
the other staged-swap transactions here invoke their repair pass and keeping a
filesystem mutation visible at the sites that cause it.
* fix(installtxn): retire a backup the publish rename already superseded
Recover skipped any workspace whose target was occupied, which left the backup
of a commit that was killed after its publish rename but before its cleanup.
Removing that install then deleted the live target and its lockfile entry but
not the skipped backup, and the next recovery read the absent target as an
interrupted swap and published the stale tree again. plugins.Load and
skills.Load enumerate directories, so the removed extension was loadable again
with nothing in the lockfile naming it.
The target already records how far the commit got, so recovery reads it as the
phase rather than carrying a phase file. Absent means the swap never finished
and the backup is put back as before. Present means the publish rename
committed, so the backup beside it is superseded and its workspace is retired.
The live install is never replaced or removed either way, and the guards that
skip a workspace with no backup, an unreadable or missing marker, or a recorded
name that is not a single element inside the install root are unchanged. The
retire path removes the workspace directly rather than through
cleanupWorkspace, which refuses one holding a previous precisely because it
cannot tell a superseded backup from one still owed a restore.
Reading the target that way is only safe once nothing can leave a partial tree
there. rollback deleted the failed install in place before restoring, so a
process killed partway through that delete left a husk at the target while the
backup was still the only complete copy, and recovery would have taken the husk
for a committed publish and deleted the last good tree. The renames are now
ordered so the target is never partial: the failed install moves aside into the
workspace, the backup moves back to the target, and only then is the set aside
tree removed. A first install has no backup to protect and recorded no target,
so it still deletes in place.
The move aside can fail too, and then the failed install stays live at the
target with the backup still the only copy of what it replaced. Rollback drops
the workspace marker on that path, which leaves a workspace nothing can
attribute, and recovery already leaves those alone.
* fix(installtxn): reconcile both halves of an interrupted commit
Recovery inferred three facts from incidental filesystem shape: target
presence stood in for the transaction phase, a public dot prefix plus two
ordinary filenames stood in for ownership, and a void best effort call stood
in for both "nothing to recover" and "recovery failed".
CommitDir swaps the trees and only then publishes the lockfile, so a kill
between the two leaves exactly what a kill after both leaves. A phase bit
written after the publish only inverts which side of the write the ambiguity
falls on, so Recover now asks the caller which of the two trees its published
metadata records. plugins and skills answer from the recorded hash; the
terminalpet install publishes no metadata and passes nil, where a live target
is the committed one. A backup the metadata still records goes back over the
target through the same move aside ordering rollback uses.
Recover returns an error, and every caller that takes the install lock aborts
on it before reading the lockfile, inspecting the target, installing over it,
or reporting a successful removal. Malformed, legacy, and unattributable
workspaces are still skipped without error; a workspace we own and could not
resolve is reported, and every workspace is processed before returning.
Ownership is now proven rather than inferred. The workspace marker carries a
magic and version first line, so a user authored skill directory that happens
to be named with the workspace prefix and to hold a previous directory is left
alone. A recorded name carrying the workspace prefix is refused too, since it
names another transaction rather than an install. Retirement clears the backup
before the workspace around it: os.RemoveAll unlinks the marker first, so a
failure inside the backup used to cost the workspace the attribution that gets
it reported at all.
* fix(installtxn): do not read a target we could not probe as absent
recoverWorkspace read any os.Lstat failure on the target as nothing being
there, which enters the restore branch: the one branch that moves a tree,
taken on the strength of a question that never got an answer. Only a
not-exist error means the swap did not finish; every other probe failure
leaves the phase unknown, so it is reported and the workspace is left as it
was.
The same distinction the reconcilers already draw between a missing file and
a file that could not be read, applied to the probe that decides which branch
recovery takes at all.
* fix(installtxn): ask the reconciler about the name that resolved the path
recoverableTarget trims the recorded name before resolving the path, but the
raw line went to the reconciler, so the two halves of one decision ran on two
different values. A marker recording "demo " resolves dir/demo and then asks
the lockfile about "demo ". The lookup misses, a miss proves the publish never
ran, and the committed target is replaced with the tree it superseded. Found
by CodeRabbit on the round-2 push; the test observes that replacement before
the fix.
Also fixes the two CI failures on the new tests, neither of which was a
product defect:
macOS and Windows: the skills recovery matrix compared the lock source against
the path the test handed the installer, while Install records
canonicalSource(source). t.TempDir returns /var on macOS where the recorded
value is /private/var, and an 8.3 short name on Windows where it is the long
one. The expectation now comes from the same resolver the installer uses.
Windows: the two plugins failure-injection tests make a directory unwritable to
force a rename or removal to fail. Windows does not block either that way and
root ignores the bits, so the injected failure never fired and the tests
asserted against a success they never meant to produce. They now skip there, as
the installtxn and skills equivalents already did.
* fix(installtxn): stop recovery acting on trees the lockfile never described
An adversarial pass over the recovery protocol, driven end to end through the
built binary rather than package tests, found that the protocol still acted on
evidence it did not have.
The one that loses data: a missing lock entry was read as proof the publish
never ran, so recovery replaced whatever was at the target with the retained
backup. A missing entry proves no such thing. The entry can be lost after a
publish that did run, since a truncated or deleted lockfile reads as an empty
one, and a directory with no entry is a state these packages already support
(Load returns it, Remove handles it). Worse, the tree at the target may have
nothing to do with the transaction: `zero tools make foo` scaffolds into the
plugins root, and a hand-written skill directory is an ordinary thing to find
in the skills root. Both were destroyed and replaced by a stale backup, and the
install reported success. Reproduced through the CLI both ways before the fix.
A missing entry is now PhaseUnknown, which stops every caller and touches
nothing.
The rest are the same class as the target probe fixed in
|
||
|
HazyLab
|
08fbea7a32 |
fix(redaction): split camelCase keys in normalizeKey (#994)
* fix(redaction): split camelCase keys in normalizeKey A lower-to-upper boundary now inserts an underscore, so accessToken and refreshToken match the registry instead of leaking into logs. Token-count fields such as promptTokens stay unredacted. Fixes #922 * fix(config): replace deprecated reflect.Ptr with reflect.Pointer * fix(doctor): report auth presence under a non-sensitive key camelCase normalization treats credentialConfigured as a credential segment, so check() redacted the set/not-set indicator. authConfigured does not match the registry. |
||
|
cairn-internandcairn-intern
|
82ae3a8809 |
docs: name the five supported targets and drop Plan 9 as a review gate (#986)
* docs: name the five supported targets and drop Plan 9 as a review gate Plan 9 has never compiled and is not a release or CI target. Reviews still asked for plan9 shims. Document the five release platforms as the supported set so unsupported GOOS is not a review criterion. Fixes Gitlawb/zero#817 * docs: exclude unsupported GOOS/GOARCH from review criteria CONTRIBUTING.md names five exact release combinations. Align AGENTS.md and .coderabbit.yaml so other GOOS/GOARCH pairs are not a review criterion. Unsupported targets such as plan9 remain allowed as fixtures. * docs: retain Android support in target policy --------- Co-authored-by: cairn-intern <[email protected]> |
||
|
|
488def68dc |
fix(config): preserve unknown top-level fields (#984)
* fix(config): preserve unknown top-level fields Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp <[email protected]> * fix(config): match JSON Unicode case folding Amp-Thread-ID: https://ampcode.com/threads/T-01a044e2-92ad-774b-9a86-094e2e5293bf Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Amp <[email protected]> |
||
|
|
b04fe96e9e |
fix(config): lock config read-modify-write across processes (#960)
* fix(config): lock config read-modify-write across processes Every config mutator loads the whole document, edits independent fields, and publishes a complete replacement by rename. The rename is atomic, so a reader never sees partial JSON — but two processes that loaded the same revision each write a full document, and the second rename silently discards the first one's acknowledged update. The result is valid JSON with one update missing and no error anywhere. lockConfigFile takes a cross-process advisory lock through lockutil, using the retry-with-deadline idiom cron, hooks and oauth already share (10s timeout, 20ms retry). Callers acquire BEFORE their first read, so the lock spans load, mutation, validation and publication and the read inside it is authoritative — holding it only around the write would still let both processes start from the same stale revision. The lock file is a sibling (config.json.lock), never the config itself: an advisory lock is held against an inode, and publishing by rename installs a new one, so locking the config directly would leave each process holding a different inode. Covered: all 14 mutators in writer.go, plus ClearProviderKeyStored and MigratePlaintextProviderKeys in credentials.go. The migration matters most — it rewrites the config on every startup, so it is the likeliest writer to collide with an interactive mutation in another Zero. The SetProviderDescription test seam locks too, so it cannot stand in as the one unsynchronized writer. Two shapes needed care: - The lock is not reentrant. EnsureCatalogProvider scans for an existing profile and then upserts, so UpsertProvider is split into a locking wrapper and upsertProviderLocked; one lock now spans the scan and the upsert, which also closes the window where two processes could both create the same catalog profile. - SetPet edits raw bytes to preserve unknown members and formatting rather than round-tripping the struct. It takes the same lock, so it neither clobbers nor is clobbered by the struct writers. Regression tests, each verified to FAIL with the lock disabled: - TestConcurrentMutationsDoNotLoseUpdates — theme, pet, recaps, favorites and a provider mutated at once; all five are independent fields, so a lost update shows up as a zero value in exactly one of them. - TestConcurrentProviderUpsertsAllSurvive — 16 distinct providers added concurrently, all must be present. - TestConcurrentSameFieldMutationsSerialize — 24 writers contending on one field; every call succeeds and the document stays readable. - TestCrossProcessMutationExcludesAndPreserves — the coordinated two-process case. Goroutines share this process's descriptors, so only a second OS process shows the lock is kernel-held. The child announces itself, then the parent asserts its write cannot land while the lock is held elsewhere, does its own mutation, releases, and requires both updates to survive. Fixes #832 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(cli): take the config lock for MCP config edits The previous commit locked internal/config's mutators, but that is not every writer of the user config document. internal/cli's MCP editor reads the SAME file, edits it, and republishes it with the identical temp-file+rename shape, at three sites (add/update, remove, disable/enable). Locking only one package left `zero mcp add` free to clobber a concurrent provider or preference write — and to be clobbered by one — with the file still valid JSON afterwards, which is the same silent lost update issue #832 reports. config.LockFile exports the existing helper so the lock is one authority across packages rather than a private detail of internal/config. A second, adjacent implementation would drift from the first exactly the way this writer already drifted from the mutators. TestRunMCPAddParticipatesInConfigLock is the regression. Racing the two writers and waiting to observe a lost update is NOT reliable — the losing interleaving is narrow, and a straightforward concurrent version of this test passed five consecutive runs against the unlocked code, so it would have shipped as reassurance that proved nothing. It instead asserts the deterministic property: while the config lock is held elsewhere, the MCP writer's update cannot land, and it completes once the lock is released. That fails immediately against the unlocked code and passes with it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(config): report config unlock failures instead of discarding them AGENTS.md requires that a mutator never report success when unlock failed, and the release error was being dropped on the floor. A failed Release can leave the lock held for the rest of the process, so returning (cfg, nil) after one claims a state the next mutation cannot reproduce — it will block for the full lock timeout and then fail. lockConfigFile now returns lock.Release directly, and every caller joins it into its own result with the idiom credstore already uses: defer func() { err = errors.Join(err, unlock()) }() Joined, not chosen between: a release failure annotates the result rather than masking the mutation error that explains what actually went wrong. That covers all 16 mutators in writer.go, both in credentials.go, and the test seam. The three MCP sites return an exit code rather than an error, so they convert a release failure into a crash exit and a stderr message, without overwriting a failure the command had already reported. A release failure cannot be provoked through the public API — lockutil.Release is idempotent and reports nil once released — so the mutators call through a lockConfigFileFn seam that a test can substitute. My first attempt at this test could only ever t.Skip, which asserts nothing; the seam is what turns it into a real assertion. - TestMutationReportsUnlockFailure: SetTheme surfaces the release error AND the mutation is still published, since a release failure annotates the result rather than undoing the write. - TestMutationErrorSurvivesUnlockFailure: an unknown-provider error and the release failure are both present in the joined error. Both verified to fail when the defer discards the release error. Note for review: cron, hooks, oauth and swarm all still discard their lockutil.Release error, so this makes internal/config stricter than its siblings. Worth deciding whether the guideline should be applied to them too — out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * fix(cli): complete MCP config lock handling Amp-Thread-ID: https://ampcode.com/threads/T-01a043da-1703-70c5-9d8d-904cd8fd964b Co-authored-by: Pierre Bruno <[email protected]> * test(cli): synchronize MCP config lock assertion Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp <[email protected]> * fix(tui): persist provider model selection atomically Amp-Thread-ID: https://ampcode.com/threads/T-01a04c92-2d1d-7508-91bc-416341b7e8b0 Co-authored-by: Amp <[email protected]> * fix(tui): surface unsaved model selections on every switch path choosePicker replaces handleModelCommand's status text with its own applied-notice, so the "· not saved (…)" qualifier that text carried was dropped and the selection reported an unqualified success. The user only found out at the next launch, when the old persisted model came back. dispatchCommand drops the same text on a committed switch, so typed /model had the identical hole — the cross-provider path already returned the outcome as a value precisely because display copy is not a contract. handleModelCommand now returns that outcome too, and one modelAppliedNoticeFor decides how a committed switch is announced for both entry points, so neither can regain the gap independently. Recent-model history is written under the same lock, so a selection that already lost the lock would pay a SECOND full timeout for one keystroke and add a redundant save error. It now falls back to the in-session updateRecentModels for exactly that case, keyed on a new config.ErrLockUnavailable rather than on any persistence failure: a missing config file also fails the lookup, and that write still has its own reason to be attempted. Regressions cover both entry points. The picker test holds the config lock — the case the review asked for — and bounds elapsed time, which is what proves one keystroke costs one wait; it fails at 20s against the previous code. The typed test uses an unwritable config path so it proves the same plumbing without a second ten-second timeout. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01REzorhNj3F1DGPXyn5Uq7j --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
|
79fa1422ef |
fix(providers): register Atomic Chat as local OpenAI-compatible runtime (#550)
* fix(providers): register Atomic Chat as local OpenAI-compatible runtime Point atomic-chat at the default local API (http://127.0.0.1:1337/v1), mark it Local with no API key, and auto-detect it alongside Ollama and LM Studio. Keep opencode-go-anthropic-compatible in the provider catalog and curated model list after rebasing onto latest main. Co-authored-by: Cursor <[email protected]> * docs(zh): sync Atomic Chat local runtime into README_ZH The English README lists Atomic Chat in the provider list and tells local-model users to run its desktop app on http://127.0.0.1:1337/v1, but the Chinese README still mentioned only Ollama and LM Studio. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(providers): give the local Atomic Chat runtime its own catalog ID Repurposing the existing atomic-chat ID from the hosted, key-gated preset to a keyless local one silently downgraded credential checks for profiles already saved against the remote endpoint: MissingCredentialEnv resolves RequiresAuth from the catalog descriptor, so those profiles were reported as ready and only failed later with an unauthenticated call to api.atomic.chat. Keep the hosted atomic-chat descriptor as it was and register the local runtime under a separate atomic-chat-local ID, mirroring the existing ollama-cloud/ollama split. Stored profiles keep their identity and no migration is needed. Co-Authored-By: Claude Opus 4.8 <[email protected]> * test(onboarding): assert the full Atomic Chat local endpoint and default model Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(models): drop the placeholder curated list for atomic-chat-local Atomic Chat's model catalog is a Hugging Face scrape refreshed every 12h plus arbitrary owner/repo lookups, so the served model ID is whichever model the user loaded and no hardcoded list stays accurate. The single local-model entry short-circuited Models() without adding anything: the descriptor is openai-compatible, so modelMatchesProvider never matches a registry entry and the fallback already yields the descriptor's DefaultModel. Output is unchanged; the difference is that local-model is no longer presented as a curated contract. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(onboarding): pin the probed model in the local-runtime adopt command probeLocalRuntime already reads the runtime's /v1/models, but SetupAction dropped the result and emitted 'zero providers add <id> --set-active' with no --model. The add path then persisted the catalog DefaultModel, which for a local runtime is the placeholder 'local-model', and the first completion failed with an unknown-model response. SetupAction now pins the probed id via SetupCommandWithModel. The catalog default wins when the server actually serves it, so an existing Ollama setup does not switch models; otherwise the first advertised id is used. An empty probe result leaves the command on the catalog default rather than inventing an id. Covers a server whose returned id is not 'local-model'. Co-Authored-By: Claude Opus 4.8 <[email protected]> * fix(onboarding): shell-safe adopt command + reject atomic-chat-local placeholder model commandArg now POSIX single-quotes rendered args instead of strconv.Quote, so a model id from an untrusted /v1/models response cannot expand when the adopt command is pasted into a shell. providerProfileForAdd now errors when atomic-chat-local is added without --model instead of persisting the local-model placeholder that fails the first completion. Adds regression tests for command substitution, backticks, and the require-model guard. * fix(onboarding): finish Atomic Chat setup across supported shells Rebase on current main and keep the hosted Atomic Chat preset intact. Only emit setup arguments that are portable across POSIX, cmd.exe and PowerShell; show interactive guidance for other model IDs. Reject the Atomic Chat placeholder before both CLI and TUI persistence, and stop advertising adoption when discovery finds no usable model. Validation: full Go test suite, focused race suite, vet, release build, smoke, formatting, advisory lint and vulncheck pass with isolated config and inherited Anthropic overrides removed. Onboarding, CLI and TUI test packages cross-compile for Windows and Linux. Regression tests fail on the rebased pre-fix code and pass with these changes. Refs Gitlawb/zero#550 and Gitlawb/zero#592. * fix(onboarding): preserve option-looking model IDs in adopt commands Use the existing --model=value grammar when a discovered model begins with a hyphen. Validate the model separately from the fixed separator so shell safety remains unchanged and '=' is not added to the allowlist. Exercise generated detection actions through shell tokenization, the actual add parser, and profile construction for Atomic Chat, LM Studio and Ollama. Cover leading hyphens, flag-looking IDs, ordinary IDs, spaces, and unsafe inline values. Before the fix, the new regression failed with: generated adoption command rejected by add parser: --model requires a value Validation: full Go suite, focused race tests, formatting, vet, release build, smoke, lint-static and vulncheck passed. Affected test packages cross-compiled for Windows and Linux. Refs Gitlawb/zero#550. * fix(onboarding): select eligible chat models and preserve Ollama defaults P2: apply the existing known-noncoding exclusion before default preference or first-model fallback. When a nonempty list has no eligible model, offer commandless guidance instead of persisting the rejected/default model. P3: recognize an Ollama untagged default's :latest spelling, preserving the advertised ID. Prefer exact matches and leave other tags/providers alone. Add selector tests plus a mock-server integration test through discovery, the generated action, the real add parser, profile persistence, a fresh process, and the chat completion request. Before this fix, that test failed: fresh process sent model "text-embedding-local", want eligible model "-loaded chat model" The Ollama alias case also failed before the fix: AdoptModel() = "qwen3:8b", want "llama3.1:latest" Explicit model choices, shell checks, hosted/local provider identities and Atomic placeholder guards retain their existing behavior. Validation: full Go suite, focused race tests, formatting, vet, release build, smoke, lint-static and vulncheck passed. Affected test packages cross-compiled for Windows and Linux. Integration evidence uses a mock server; native Windows shell and actual runtime behavior are not claimed. Refs Gitlawb/zero#550 and Gitlawb/zero#592. --------- Co-authored-by: Cursor <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: alex <[email protected]> |
||
|
cairn-internandcairn-intern
|
1aacf38b72 |
fix(background): treat POSIX zombie leader as already-exited (#981)
* fix(background): treat POSIX zombie leader as already-exited terminateOwnedProcess always returned leaderAlreadyExited=false, so TerminateCommand surfaced "did not exit after SIGKILL" when ps was unavailable and the leader was already a waitable zombie. Probe the leader the way Windows uses GetExitCodeProcess, then still signal the group so live descendants are not skipped. * fix(background): preserve live-group termination failures --------- Co-authored-by: cairn-intern <[email protected]> |
||
|
HazyLabandcl-ment
|
3063593129 |
ci: harden GitHub Actions permissions, add job timeouts, concurrency and coverage reporting (#951)
* ci: harden workflow permissions, add timeouts, concurrency and coverage report * ci: reuse make fmt-check and refine coverage table formatting * ci: scan Windows build graph via vulncheck-windows and collapse coverage summary to single-pass awk * ci: invoke the pinned Windows govulncheck via a local GOBIN go install writes the binary to GOBIN, which is not necessarily on PATH. Install into .cache/gobin and run that executable. GOOS=windows applies only to the scan so the scanner stays a host binary. * ci: declare contents:read on the action smoke workflow Pull-request workflows inherit the default GITHUB_TOKEN unless they set an explicit permission block. The smoke workflow is now covered by the same least-privilege policy as ci.yml. * ci: parse workflow permissions structurally and quote the Windows scanner path The pull-request token check now ignores commented lookalikes and accepts an inline on: [pull_request] trigger. vulncheck-windows keeps the cache path as one shell word. * ci: drop -race from this PR so issue 939 stays with PR 940 Coverage profile remains. The dedicated race-detector job is owned by the open race PR. * ci: parse workflow permissions as YAML and scope GOWORK=off * ci: label the non-race smoke test explicitly --------- Co-authored-by: cl-ment <[email protected]> |
||
|
HazyLabandcl-ment
|
d6b25fc303 |
perf(providers): parallelize isolated provider unit tests and shrink retry backoffs (#955)
* perf(providers): parallelize isolated provider unit tests and shrink retry backoffs Enable t.Parallel() on independent provider test cases and shrink default retry backoffs from seconds to 1ms during test runs, reducing provider test suite duration from ~300s to ~2.4s without flakiness. * test(providerio): add unit coverage for ShrinkBackoffForTest and restoration * test(openai): keep Codex idle-timeout acceptance test serial StreamIdleTimeout arms a wall-clock watchdog. Parallel scheduling can lose the 50ms race against response.completed even though the test only checks that the option is accepted. * fix(config): replace deprecated reflect.Ptr with reflect.Pointer --------- Co-authored-by: cl-ment <[email protected]> |
||
|
|
7c50a9fb98 |
security(mcp): prevent server requests from matching client pending responses (#942)
* security(mcp): prevent server requests from matching client pending responses * fix(mcp): send courtesy -32601 reply asynchronously off read loop * fix(mcp): restore jsonRPCIDEchoable and validate request IDs before responding * fix(mcp): queue and drop courtesy replies without blocking client callers * fix(mcp): drop canceled queued requests * fix(mcp): preserve valid numeric request IDs * fix(mcp): treat JSON method member presence as the request discriminator An explicit empty method string is a request, not a response. Courtesy -32601 is still dropped when the write queue is full: failAll on that path stopped the reader and blocked inbound pipe writes (TestStdioClientUndrainedServerDoesNotBlockCallerWithDeadline). * fix(mcp): count dropped courtesy replies when the write queue is full The inbound reader must not stop. An empty select default is now an atomic counter rather than a silent discard. * fix(mcp): retain method presence, writer lifecycle, and original JSON-RPC IDs * fix(mcp): bound courtesy overflow, pin writer queue, match exponent IDs * fix(mcp): skip SSE frames that carry a method member Treat method presence the same on stdio and SSE so empty or null method never completes a pending client request. --------- Co-authored-by: cl-ment <[email protected]> Co-authored-by: KRATOS <[email protected]> |
||
|
euxaristiaandeuxaristia
|
275e248669 |
fix(oauth): Split an oversized keyring token blob across entries (#1007)
* fix(oauth): split an oversized keyring token blob across entries Store every provider and MCP token in one keyring entry and the store stops working once the logins outgrow it. On macOS the secret rides inside a `security -i` command line capped at 4095 bytes, which leaves 3027 bytes of JSON for all logins combined, so a second OIDC login fails to save and every write after it fails too. Split a blob that does not fit across numbered entries and put a manifest in the anchor account. Chunks live in two alternating generations: a write fills the one that is not live, then replaces the manifest, so that single write is the commit point and a crash partway through still reads the previous generation. `zc1:` cannot prefix base64, so an entry written by an existing build is still recognised and read without a migration step. Reserve the range a write will occupy before occupying it. Without that, a write interrupted while filling a longer generation leaves chunks above the count the manifest records, and no later cleanup knows to delete them: a fragment of a token blob would stay in the keychain for good. Expose the per-entry budget from internal/keyring rather than hardcoding the macOS figure in the oauth store, sharing one line builder with Set so the budget and the boundary it describes cannot drift. Backends with no limit report so and keep the single-entry layout, so Linux is untouched. Refs #937 * fix(oauth): lock keyring reads, validate manifest digest, and harden chunk tests * test(oauth): assert Status blocks during keyring lock regression test * fix(oauth): sweep both chunk generations when a store regrows A shrink writes the blob back under the anchor, which replaces the manifest and takes the per-generation chunk counts with it. From then on nothing can name the chunks a failed cleanup left behind, so the growth branch's sweep of the target generation is the only one that will ever reach them — and it only ever targets family A. A keychain that refused one delete during the shrink therefore kept a superseded generation of access, ID and refresh tokens indefinitely, with no way for the user to know. Sweep the other generation alongside the target, and document that the reclaim waits for the next growth: a store that shrinks once and never grows again keeps the residue, which sweeping on every whole write would close at a cost of 128 `security` invocations per save on macOS. Also record why Load and Status take the cross-process lock, and give the missing-chunk error the same "log in again" advice the digest failure carries. Refs #937 * fix(oauth): serialize keyring operations under user-level lock Derive the keyring backend lock file path from the user's home directory rather than file store configuration, ensuring processes with distinct store paths share the same lock domain for the OS keychain. Refs #938 * Address review feedback on chunked keyring token store and recovery Refs #938 * Refine auth reset message and chunk format notation Refs #938 * Harden OAuth store reset lifecycle, keyring migration recovery, and completions. Refs #938 * Validate keyring cleanup markers, propagate lock resolution errors, and test reader concurrency. Refs #938 * Preserve live OAuth credentials when stale cleanup markers survive migration. Refs #937 * Verify JSON output from OAuth reset to cover its recovery response. Refs #937 * Verify that JSON OAuth reset actually removes stored logins. Refs #937 * Require confirmation before resetting shared OAuth credentials. Refs #937 --------- Co-authored-by: euxaristia <[email protected]> |
||
|
|
0207f2b269 |
fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead (#1006)
* fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead * fix(sandbox): address CodeRabbit review feedback on PR 640 * refactor(sandbox): Address CodeRabbit nitpicks on PR 640. Co-Authored-By: cairn-code <[email protected]> * fix(sandbox): skip inherited ACEs and reject DenyRead upfront during setup Skip inherited ACEs when preserving read deny entries, correct syntax in the command runner error message, and evaluate unsupported DenyRead profiles before the elevation check in Windows sandbox setup. Refs #640 * Reject unsupported DenyRead profiles upfront during sandbox setup Refs #640 * Migrate legacy Windows DenyWrite ACEs, randomize smoke probes, and clean dead descendant machinery. Refs #640 * Use /d /c for denied-write probe command and verify ownership on cleanup. Refs #640 * Honor NoInherit in legacy ACL migration and verify probe ownership. Refs #639 * Preserve DenyRead bits when migrating combined legacy deny ACEs. Refs #639 --------- Co-authored-by: euxaristia <[email protected]> Co-authored-by: cairn-code <[email protected]> |
||
|
PierrunoYTandAmp
|
8b3d1b1597 |
fix(tools): reject fuzzy edits with drifted content (#983)
Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp <[email protected]> |
||
|
cairn-internandcairn-intern
|
a3852bac59 |
fix(update): keep Windows failed-restore state in trusted recovery records (#979)
* fix(update): keep Windows failed-restore state in trusted recovery records A failed restore currently records the last verified binary only as a .deletable .keep marker in the install directory. Write an unresolved identity-bound record to the existing per-user update-recovery store on that failure path too, and consult it during preflight so deleting the marker cannot silence the tamper refusal. Fixes #868 * fix(update): keep a fail-closed signal if unresolved recovery records cannot be written Ignoring a failed appendUnresolvedRecoveryRecord left only the sibling .keep marker. An install-directory writer can delete that marker, after which preflight finds no unresolved state and a retry can overwrite the last verified recovery copy. Relocate that copy to a .recovery name when the per-user record cannot be persisted so promotion still refuses. Fixes #868 * fix(update): do not relocate the live binary when failed-restore compensation cannot record persistFailedRestoreSignal relocated through the handle when the per-user unresolved record could not be written, without checking that the object was still the aside copy. A restore that had already put the verified bytes back at targetPath then had that binary moved out of the executable path. Verify identity with verifyPromotedTarget before relocating. Unreadable recovery-state JSON now fails closed instead of looking like an empty queue. The per-user vs per-machine split is documented: a second account still relies on the install-directory .keep marker. Fixes #868 * fix(update): durably commit recovery state on Windows --------- Co-authored-by: cairn-intern <[email protected]> |
||
|
euxaristiaandeuxaristia
|
a3e7446681 |
chore(tui): remove unused test exports GetLocalDiffStats and WatchPRState (#975)
Refs #904 Co-authored-by: euxaristia <[email protected]> |
||
|
KRATOS
|
0021475d4c |
fix(swarm): stop original member before handoff (#957)
* fix(swarm): stop source before handoff * fix(swarm): make handoff recovery fail closed * test(swarm): remove handoff timing assumptions |
||
|
HazyLabandcl-ment
|
deba3adfca |
security(update): prevent tar extraction from traversing escaping symlinks (#943)
* security(update): prevent tar extraction from traversing escaping symlinks (fixes #920) When extracting tar archives, lexical path cleaning alone does not prevent subsequent file entries from being written through a previously extracted directory symlink that resolves outside destDir (Zip/Tar Slip). This introduces verifyNoSymlinkEscape in safeExtractPath, which recursively inspects existing path components between destDir and target using os.Lstat and filepath.EvalSymlinks to reject entries attempting to write through escaping symlinks. * test(update): exercise chained symlink escape during file extract The previous case was a single escaping symlink already rejected by the existing Linkname check. Seed a mid -> outside chain under destDir and extract a regular file through it so verifyNoSymlinkEscape is what fails, with no write outside destDir. * fix(update): resolve destDir symlinks and permit valid dangling symlinks in archive * docs(update): document POSIX-only scope for verifyNoSymlinkEscape * internal/update: test extracting files through directory symlinks * fix(update): reject dangling symlinks instead of lexical fallback EvalSymlinks failure used filepath.Clean on the link text, which does not preserve already-traversed links. A dangling member is now an error. * fix(update): resolve dangling symlink chains from the physical parent * fix(update): authorize tar paths through OpenRoot and reject outbound symlink entries * fix(update): resolve intermediate symlink parents and cap Root link depth Walk each path component from the physical parent, reject a ninth link, and look up release binaries through OpenRoot as regular files only. --------- Co-authored-by: cl-ment <[email protected]> |
||
|
euxaristiaandeuxaristia
|
501f32160f |
ci: Run the race detector on pull requests (#940)
* ci: run the race detector on pull requests The Makefile declares `go test ./... -race -count=1` as the project's test command, but no workflow invokes that target. ci.yml and pr-auto-review.yml both run plain `go test ./...`, and `-race` appears nowhere under .github/workflows, so the detector has never run on a pull request. The concurrent paths where a race is easiest to introduce and hardest to see in review, the turn loop, streaming provider I/O, the session store, cron, swarm mailboxes, and the cross-process lock paths in the credential and OAuth stores, are all unchecked. Add a dedicated job rather than putting -race on the existing matrix step. A race run is typically several times a plain one, and the matrix Test step is followed by build and smoke, so folding it in would serialize behind those and make that job the new long pole. On its own it runs alongside the Windows job, which is the current long pole at 9m29s, and a failure reads as a race rather than as a smoke failure. Run `make test` instead of repeating its flags, so the project's declared test command and what CI executes cannot drift apart again. Refs #939 * ci: add permissions to race detector job --------- Co-authored-by: euxaristia <[email protected]> |
||
|
Vasanth Tandbaoyu0
|
1e4db7c664 |
fix: let the configured forward proxy through the loopback guard (#1025)
* fix(providerhealth,tools): let the configured forward proxy through the loopback guard With HTTPS_PROXY pointing at a local forward proxy, the transport dials the proxy first and tunnels the request through it. The SSRF-guarded dialer in providerhealth refused that dial as a loopback address, so `zero doctor --connectivity` failed with "proxyconnect tcp: provider connectivity URL is unsafe: loopback hosts are blocked" for a public provider that had already been validated, and web_fetch had no proxy support at all (#569). Each dialer is now handed its transport's own Proxy function and lets a dial through unpinned only when the address is exactly the proxy endpoint that function names for the request scheme: same host, same port, with a missing proxy port taken as the scheme default. A sibling loopback port is still refused. The shared check lives in internal/proxydial since providerhealth and tools otherwise share nothing. web_fetch honours http.ProxyFromEnvironment again. The guarded fetch had set Proxy to nil as part of its SSRF hardening; the target is still validated before the request, and what a forward proxy removes is only the dial-time rebind pin, which is inherent to using any forward proxy. The pre-permission URL validation stays in place. Started from #594 by baoyu0. This version keeps the pre-permission check, matches the proxy port exactly instead of any port when the proxy URL has none, and injects the transport's Proxy function instead of reading the environment a second time. Closes #569 Co-authored-by: baoyu0 <[email protected]> * fix(proxydial): have the dialer ask its transport for the proxy at dial time Copying transport.Proxy into the dialer at construction left the wiring between the two unpinned: a dialer handed nil would pass every unit test while a proxied request still died as loopback. The dialer now consults the transport it is installed on, and each package drives a real request through a loopback listener acting as the proxy and checks that the CONNECT arrives. The proxy is substituted on the transport rather than through HTTPS_PROXY, since http.ProxyFromEnvironment reads the environment once per process and an env-based test would depend on test order. * fix(proxydial): bind the proxy exemption to the route the transport chose Asking the Proxy function a question of our own invention answered for a request the transport never made. With NO_PROXY excluding a host, the real request to that host goes direct while a synthetic probe still comes back proxied, so a direct dial whose address happened to equal the configured proxy endpoint skipped the guard and reached whatever the name resolved to at connect time. A probe on the previous head dials rebind.example:80 with zero safety lookups; the guard now runs and refuses its private answer. Host-dependent proxy callbacks make the mismatch general, and no spelling of the probe host closes it, because the two calls are about different requests. The decision is now recorded where it is made. proxydial.Transport wraps the transport, asks its Proxy function about the actual request, and puts the answer in that request's context; the transport derives the dial context from the request context, so the dialer reads back the route this very connection is being opened for. Per-request, so concurrent requests cannot overwrite each other, and the context key is unexported so nothing outside the package can plant one. Address matching is gone with it. A transport opens one connection dial, and when a proxy is selected that dial goes to the proxy, so the recorded route already identifies the dial. Comparing the address on top could only disagree with the transport, which is what a Unicode proxy hostname did: Go dials xn--bro-hoa.example while the URL still spells it as configured, and the proxy was refused as an ordinary private address. Both spellings are now one endpoint with no comparison to get wrong. Documents where destination enforcement ends: with a proxy configured, the target hostname is sent to the proxy and the proxy resolves it, so the local pre-request checks no longer bind the address that is reached. A forward proxy already sees and can rewrite every request through it, so that trust comes with configuring one; the alternative is refusing to use proxies at all, which is the outage in #569. --------- Co-authored-by: baoyu0 <[email protected]> |
||
|
Vasanth T
|
f9092668a8 |
fix(agent): stop the tool-schema ratchet meaning a different thing per host (#1017)
* fix(agent): stop the tool-schema ratchet meaning a different thing per host TestEagerToolSchemaTokenBudget counted exec_command shell guidance along with the schemas, and that guidance is host-specific. The same tree measured 3498 on Linux, about 3600 on a Windows host with pwsh 7, and 3653 on Windows PowerShell 5.1, because shellGuidanceForRuntime appends a longer block for 5.1 (it has to explain that && and || do not work there). 5.1 is stock Windows, and that case has been over the 3650 ceiling since view_image landed. So `go test ./...` failed for anyone on a stock Windows box while CI stayed green: the runners have pwsh 7 and take the cheaper branch. The workflow does run the tests on windows-latest, so this was never a coverage gap, just a ceiling sized against whichever shell the runner happened to have. The two halves are now charged separately. The schema ceiling measures the schemas alone and reads 3498 on Linux and on Windows PowerShell 5.1, so it ratchets the same thing everywhere. The host guidance gets its own ceiling at 200, above the worst case rather than above this machine: 5.1 is 155 tokens, pwsh 7 is 110, cmd.exe is 87, POSIX is 0. No guidance text was trimmed. It is what stops the model writing bash on Windows, and the 5.1 note is the reason a chained command does not silently do half its work. Paying 155 tokens for it is the right trade; measuring it as if it were a schema was not. HostExecCommandShellGuidance is exported for the budget, which is production surface added for a test. It is one accessor over existing logic, and the alternative is a ratchet that cannot tell the two costs apart. A guard in internal/tools pins that the guidance really is the tail of the description, so the subtraction cannot silently start measuring the wrong thing. * test(tools,agent): make the guidance contract checkable off Windows The ratchet separates the fixed tool schemas from this host's exec_command shell guidance, and both halves of that separation were only being checked on Windows. The description test returned early off Windows once the accessor reported no guidance, so it never looked at the description. Dropping the GOOS condition in the constructor would have a POSIX host append its own guidance (about 150 characters on darwin) with the accessor still reporting none and the ratchet charging every byte of it to the schemas, and nothing would fail. Both the description and the accessor now take goos, so the contract is checked for all three platforms from any one of them. The budget also went from subtracting a separately rounded guidance estimate to removing the guidance from the description before estimating. ApproxTextTokens is an integer division, so floor((schema+guidance)/4) - floor(guidance/4) is one token high whenever the two remainders add past four, and which side of that a host lands on depends on the length of its guidance. That is the same host-dependence the ratchet was being fixed for, one token smaller. No behaviour change: exec_command's description is byte for byte what it was on every platform. |
||
|
KRATOS
|
7f5e5af7bb |
fix(acp): make configured MCP tools callable (#1003)
* fix(acp): register configured MCP tools per turn * fix(acp): address MCP review findings |
||
|
|
f30f550e60 |
fix(sandbox): protect worktree git pointers (#805)
* fix(sandbox): resolve git metadata carveouts for worktrees .git is a file (gitdir: ...) in worktrees/submodules, not a dir. The hooks/config write-deny carveout assumed a directory and pointed at <root>/.git/hooks, which has no mkdir-able parent under the .git file. bwrap then failed 'Can't mkdir parents ... Not a directory', blocking every sandboxed tool in worktree checkouts. Resolve the real (common) git dir via the gitdir pointer + commondir so worktrees deny the shared hooks/config like a plain clone. Plain checkouts (.git dir) and .git-absent roots keep the literal paths. * fix(sandbox): bound worktree git metadata carveouts to the write root Refuse gitdir/commondir pointers that escape the workspace instead of carving outside confinement. Invalid, stale, or non-directory pointers return no carveouts so bwrap does not mkdir under a .git file. * fix(sandbox): evaluate gitdir/commondir through symlinks Containment must use the real path so an in-root symlink cannot punch hooks/config carveouts outside the write root. * fix(sandbox): fail closed on gitdir eval and protect the pointer file Refuse carveouts when EvalSymlinks cannot produce a real in-root path, compare evaluated roots and targets, and write-deny the .git pointer so it cannot redirect hooks/config between commands. * fix(sandbox): deny worktree-local gitdir metadata and parse first gitdir line Keep hooks/config write-denied on both the common dir and the in-root worktree gitdir, emit those paths under the lexical write root, and accept a gitdir: pointer that has trailing comment lines. * fix(sandbox): skip special git metadata files and re-check carveouts at apply Read .git and commondir only when they are regular files so a FIFO cannot block profile construction. At sandbox apply, drop ReadOnlySubpaths that no longer sit inside the write root after a symlink swap. * fix(sandbox): follow directory .git symlinks and unix-tag FIFO tests A .git symlink to a directory is a normal checkout, so keep lexical hooks/config carveouts. FIFO coverage stays on unix so Windows still compiles. * fix(sandbox): protect worktree git pointer directly * test(sandbox): skip unavailable symlink fixture * fix(sandbox): preserve git symlink semantics * fix(sandbox): limit git pointer carveout to regular files --------- Co-authored-by: jatmn <[email protected]> Co-authored-by: jatmn <[email protected]> |
||
|
Vasanth T
|
aadb4a27e9 |
fix(specialist): keep the pinned model when a specialist is resumed (#1009)
* fix(specialist): keep the pinned model when a specialist is resumed Metadata.Model exists so a bounded, delegated task can run on a cheaper model than its parent, and BuildArgs appends it to the child argv through appendModelArgs. BuildResumeArgs never did. So a specialist pinned to a cheap model ran on that model exactly once. The moment the orchestrator resumed it, the child fell back to whatever the parent's configured model resolved to. Nothing surfaced it: the resumed child starts normally and does the work, so the only symptom is the bill. Cost-motivated delegation quietly stopped saving anything. Resuming does not restore the recorded model on its own. sessions.PrepareExec records the model a run used but never feeds it back into provider construction, so the flag has to be passed again rather than relied upon. BuildResumeArgsInput now carries ParentModel and ParentReasoningEffort, the same fallbacks the fresh path takes, and runResume passes what TaskRunOptions already held. The reasoning-effort rule travels with the model unchanged: the parent's effort is inherited only when the manifest pins no model of its own, because a manifest that chose a different model has not agreed to the parent's effort for it. Regressions drive both builders and compare them, so the two paths cannot drift again: a pinned model survives resume, an unpinned one still inherits the parent's, both halves of the effort rule hold, and the flag keeps its position relative to --auto in both. Refs #554 * test(specialist): cover the resume call site, not just its builder The builder tests all call BuildResumeArgs directly, so dropping the ParentModel field from the runResume call site still compiled and still passed every one of them. The defect this fixes lived at the call site, so it needs a test that goes through Run. Driven through the real dispatch with the RunChild seam capturing argv. * test(specialist): guard the fresh call site as well runFresh and runResume each construct their builder input by hand and carry a byte-identical ParentModel line. Deleting either compiles and, until now, deleting the fresh one was silent. The resume half is what this branch repairs. This covers the other half so the pair cannot drift again in the direction nobody was looking. |
||
|
KRATOS
|
a6ec0435e3 |
feat(acp): surface safe browser tool metadata (#1000)
* feat(acp): surface safe browser tool metadata * fix(acp): align browser permission titles * fix(acp): namespace browser metadata * fix(acp): reject unsafe browser title text |
||
|
Anandan
|
1b5db17656 |
fix(mcp): discover OAuth through protected resource metadata (#991)
* fix(mcp): discover OAuth through protected resource metadata * fix(mcp): refuse OAuth discovery redirects * fix(mcp): harden OAuth metadata discovery * fix(mcp): protect discovered OAuth endpoints |
||
|
Anandan
|
eeea3308fe |
fix(tui): move voice capture to Ctrl+Space (#968)
* fix(tui): keep voice mode from swallowing spaces * fix(tui): clarify voice capture controls * fix(tui): align active voice status with capture key |
||
|
PierrunoYTandClaude Opus 5
|
27b319ca88 |
test(execution): fail loudly when ps cannot answer instead of skipping (#961)
* test(execution): fail loudly when ps cannot answer instead of skipping Every test in process_unix_test.go proves something about zombie detection, and every one of those proofs is read out of ps. When ps could not answer, the tests skipped — which is indistinguishable from success in CI output — and one assertion passed vacuously for the same reason. A regression in ps parsing or availability would have shown up as green. requireProcessState probes BOTH ps pipelines against this test process, which is by definition alive and not a zombie, so a working ps must report a live state for it. When it cannot: - Under CI (or ZERO_REQUIRE_PS=1) this is a hard failure. ps is present on every image used today, so its absence means the pipeline regressed and that has to surface immediately. - Otherwise it stays a skip, so a contributor on an unsupported box is not blocked. Both pipelines are probed because they are deliberately separate: the tests assert what the production process-table parser concludes, so this file keeps its own per-PID ps oracle rather than borrowing that parser to establish its own preconditions. One regression must not be able to silence both sides at once. Once the probe passes, failing to observe an EXPECTED state is a defect rather than an environment problem, so the remaining escapes become failures: - The zombie waits (awaitZombie) fail instead of skipping. The caller never waits on the child, so its zombie row persists until reaped — reaching the deadline means the child never exited or ps stopped answering, and the two are reported distinctly. - A zombie group that is not signalable fails instead of skipping: a zombie still occupies the process table, so if kill(-pid, 0) fails the case under test is not present and the assertions after it would prove nothing. - processSignalTarget returning anything but the individual PID fails instead of skipping: the child inherited this process's group, so its PGID is this process's PID and can never equal its own PID. The vacuous assertion in TestTerminateProcessTreeStopsRunningGroup now requires ok. That is sound rather than merely stricter: the test never waits on the child, so the group's zombie row must still be present after termination, and an unknown state means ps stopped answering rather than that the group is clean. Verified on Linux (WSL, go1.26.6) rather than by cross-compilation alone: - all three tests pass unmodified - with a stub ps that exits 1: CI unset skips with the reason named; CI=true fails, naming which pipeline could not answer - the same broken-ps run against the ORIGINAL file produced two silent skips plus one unrelated failure after 12s, versus an immediate, explicit failure now Windows is unaffected (the file is !windows); go vet passes for linux, darwin and windows. Fixes #863 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq * test(execution): treat an unsignalable zombie group as a kernel difference The macOS Smoke job failed: the zombie group 5931 is not signalable (operation not permitted) That was my error. I converted this skip along with the ps-availability ones, but it never had anything to do with ps: whether kill(2) may signal a group whose only member is a zombie is a KERNEL difference. Linux permits it, so signalTargetRunning goes on to consult ps — the branch this test was written for. Darwin returns EPERM, so signalTargetRunning short-circuits on the kill and never reaches ps. Rather than restore the blanket skip, the test now records which branch answers: - Signal permitted: assert processGroupHasRunningMember directly, so a ps-side regression cannot hide behind the kill result. This is new coverage; the old test only checked signalTargetRunning, which conflates the two paths. - Signal refused: log it and continue. The outcome assertion — a zombie-only group must not count as running — now runs on EVERY platform. Previously Darwin skipped before reaching it, so this is strictly more coverage there, not less. Any errno other than EPERM still fails: it means the group is not in the state the test needs, and nothing after it would be meaningful. TerminateProcessTree is asserted only where the signal is permitted, because terminateTarget's own kill(2) would hit the same EPERM and return it as a cleanup error. Worth a maintainer's eye: that may be a real gap in TerminateProcessTree on such a platform — a zombie-only tree is already exited and arguably should terminate cleanly rather than report failure, which is the same spurious-cleanup-failure class #774 set out to remove. Confirming that needs a Darwin host and is a production change, so it is flagged rather than guessed at here. Verified on Linux (WSL, go1.26.6): all three tests pass, including the new direct ps assertion. The Darwin branch was exercised by forcing the EPERM path on Linux — it logs both explanations and passes with the outcome assertion still enforced. go vet passes for linux, darwin and windows. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01U389qmQUhoZB3YtXkFmiTq --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> |
||
|
Anandan
|
1fde941803 |
perf: reduce startup and turn overhead (#932)
* perf(tui): disable duplicate composer cursor blink * perf(tui): stop composer wakeups after idle * refactor(tools): publish immutable registry snapshots * perf(startup): defer optional tool discovery * perf(provider): reuse compatible response sessions * feat(provider): add native patch tool input * fix(provider): recognize saved chat sessions * perf(tui): render only on screen changes * fix(runtime): address performance review findings * fix(usage): expose cache efficiency details * perf(agent): reduce redundant bounded-task turns * fix: harden runtime review edge cases * fix: address runtime review findings * build: restore official Bubble Tea dependency * fix(cli): redact MCP registration errors * fix: harden turn session regressions * fix: keep run tools and patch roots consistent * fix: bind freeform patch writes to scoped roots * test: keep patch fixtures out of worktree * fix(tools): reject mixed-root freeform patches |
||
|
KRATOS
|
7acac7acee |
fix(sandbox): refcount temporary roots so one holder's cleanup cannot revoke another's (#911)
* fix(sandbox): a temporary root is refcounted, so one holder's cleanup cannot revoke another's Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for the self-contained pieces to arrive separately. A temporary grant was add-then-remove with no notion of who still needed the root. The second caller to ask for a root already present got a NO-OP undo, and the first caller's cleanup then removed the root out from under it — so a live grant lost its access while its holder still believed it had it. Two read-only tools in the same parallel batch, both blocked on the same directory, is exactly that shape, and read-only tools are precisely the ones the batch runs concurrently. tempReads/tempWrites now count live holders, and the root is only stripped when the last one releases. The release also performs both mutations under ONE hold of the lock. Dropping it between them left a window where the root was still in readRoots but no longer in tempReads: a concurrent AddTemporaryRead landing there reads it as a PERMANENT root, hands its caller a no-op undo, and this call then strips the root — so that caller believes it holds access it has already silently lost. Independent of the remaining #829 work; builds and tests against current main on its own. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a read covered by a TEMPORARY write root takes a reference too Found by a self-review of this branch: the refcount closed the defect within reads and within writes, but not across them. AddTemporaryRead returned a no-op undo whenever writeRootCoversLocked said a write root already covered the path, and the comment there claimed that cover was permanent. It is not: writeRootCoversLocked scans extraRoots, and AddTemporaryWrite appends TEMPORARY write roots into that same slice. So a read covered by another holder's temporary write took no reference, and that holder's cleanup silently revoked the reader's access — the very defect the refcount exists to close, reached across the read/write boundary instead of within one side of it. Demonstrated before fixing, and the regression fails without it: ReadRoots before release = [ws, …/outer] ReadRoots after release = [ws] the write holder's cleanup revoked a live read grant on …/outer/inner The read now takes a reference on the covering temporary write root, and the genuinely permanent case — the workspace root, or a session-scoped grant — still returns a no-op undo. The regression also asserts the other direction: once the reader releases too, the root is gone, so the reference does not leak the grant past its last holder. Origin-Session: local-0484ba | Claude Code | 6 prompts Origin-Snapshot: e6d90ff3ce46 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): make the concurrency test concurrent, and portable off macOS Both raised by CodeRabbit. The third finding on this PR — a read covered by a TEMPORARY write root taking no reference — was already fixed in 87a722cb with the regression it asked for; this covers the two that were still live, and both are cases of a test reporting more than it checked. THE CONCURRENCY TEST WAS NOT CONCURRENT. The gate spun until hasReadRoot saw somebody holding the root, which is satisfied by the FIRST holder, then opened the release. The spin wins that race against goroutines still being scheduled: instrumented over 200 runs, 194 peaked at a single simultaneous holder and not one ever reached eight. A test named for concurrent holders was exercising one holder at a time. Against a deliberately broken refcount, where the first release strips the root, it passed 33 times in 40 — it was a flaky test that mostly agreed with broken code. Counting all eight acquisitions makes the overlap real: 200 of 200 runs now peak at eight, and the same broken refcount is caught 40 times in 40. Holders signal the barrier on every path including failure, so a holder that returns early fails the test rather than hanging it. THE HELPER SKIPPED ON WINDOWS. scopeOutsideRoots reached for /Users/Shared and fell back to /var/empty, neither of which exists there, so every test built on it skipped and reported green having asserted nothing. It now uses the home directory, which no platform lists as a default write root, matching what scope_extra_read_test.go already does — and then PROVES the choice against defaultTempWriteRoots rather than assuming it, so a future default root turns these tests into an honest skip instead of a silent no-op. Origin-Session: local-abff1c | Claude Code | 2 prompts Origin-Snapshot: d2f269b81f33 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a permanent grant outlives the temporary one, and an undo is per holder Both P1s from @anandh8x, both reproduced before changing anything, and both are the same mistake in different places: treating "something covers this path" as though it answered "and will it still be there afterwards". A PERMANENT GRANT WAS REVOKED BY A TEMPORARY HOLDER. extraRoots holds temporary write roots alongside permanent ones, so Add asked only whether the path was already covered. A session-scoped grant made while a temporary holder happened to cover it recorded nothing at all, and disappeared the moment that holder released. Outliving the request that prompted it is the entire difference between Add and AddTemporaryWrite, and it was not happening. AddRead had it too, including across the boundary: a permanent read covered only by a temporary WRITE root died with that root. Permanent coverage is now looked for first and across every root, so a temporary cover cannot decide the answer when a permanent one is also present. The same root held temporarily is promoted in place — its holders' releases become no-ops, which is what permanence means here. A BROADER temporary root over a narrower permanent grant cannot be fixed by promotion, so the narrower root is recorded in its own right; that is the case he specifically called out, and it is the one a promotion-only fix would have missed. ONE HOLDER'S UNDO IS ONE HOLDER'S REFERENCE. The comment on releaseTemporaryRead claimed each undo is called exactly once and that flooring the count at zero covered the rest. Flooring stops the count going negative; it does nothing about a holder's second call consuming a DIFFERENT holder's reference. With two readers, calling the first's undo twice took the count 2 -> 1 -> 0 and removed a root the second was still using — the exact revocation this refcount exists to prevent, reached through a duplicate call rather than a sibling's cleanup, and a deferred cleanup in a retry path is how a caller does that by accident. Every undo handed out is now wrapped so it drops its own reference at most once, and the comment says what is actually true. Both mutation-checked: unwrapping the undo, and ignoring permanence in Add, each fail the test that covers them. Race detector clean. Origin-Session: local-abff1c | Claude Code | 3 prompts Origin-Snapshot: 07900397c62e Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * chore(sandbox): re-request an automated review of this head No code change. CodeRabbit's verdict on this PR is pinned to ff85dbcc, a commit that no longer exists on the branch, and a stale changes-requested keeps the PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on push rather than on request, and repeated @coderabbitai triggers over five days produced nothing, so this is the only lever available without maintainer rights. The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of 4a7063aa. The tree they approved is byte-identical to this one. Origin-Session: local-8cd239 | Claude Code | 6 prompts Origin-Snapshot: 149ab98453ad Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * test(sandbox): a fixture failure fails, a report cannot deadlock, and AddRead's own branches are covered All raised by CodeRabbit on the full review. Every one verified before changing anything. ## The concurrent test could hang instead of failing failures was buffered at one slot per holder, but each goroutine can report at BOTH hasReadRoot checks — its own grant going missing while held, and again after the release gate. Nothing drains the channel until after wg.Wait(), so the ninth report blocks forever, wg.Done() is never reached and the wait hangs. The test would deadlock in exactly the case it exists to catch: a refcount bug severe enough to fail more than eight of eight holders. Buffered at 2*holders. ## A broken fixture reported itself as a pass TestANarrowerRequestHoldsTheCoveringGrant called t.Skip when os.MkdirAll failed. The directory is the fixture, not a precondition the environment may reasonably withhold, and the sibling test above it already uses t.Fatal for the same call. ## Two tests assumed something they never checked Both tests in scope_extra_read_test.go place their grant under $HOME precisely because NewScope seeds /tmp and $TMPDIR as write roots — a grant that collapsed into one of those would be writable for a reason unrelated to the property under test. $HOME is normally clear of them, but a sandboxed or CI environment that points HOME at a temp directory turns both tests into something that proves nothing while still reporting PASS. homeGrantOutsideTempRoots now builds the fixture for both and skips with the reason when the assumption does not hold. Verified by running with HOME set to a temp directory: both tests SKIP naming the covering root, where before they passed. The path is resolved before comparing, because the temp roots are resolved and /var is a symlink to /private/var on macOS. ## AddRead's branches were only covered from the write side Two subtests added: a permanent read under a temporary WRITE root, and a narrow permanent read under a BROAD temporary read root. Both mutation-checked by removing the corresponding temporary-coverage guard in AddRead — each fails. The first mutation I ran did not fail its test, and the finding looked wrong: the pattern being edited occurs in Add, AddRead and AddTemporaryWrite alike, and a first-match edit had changed Add. Re-run against the line inside AddRead, it fails as claimed. Recorded because a mutation that misses its target is indistinguishable from a test that holds. ## Also asserted: the refcount table is empty, not merely invisible hasReadRoot answers what a caller can see. An entry left at zero in tempReads is not visible that way and would still leak one map entry per acquire/release cycle, so the contention test now checks the terminal state directly. go test -race ./internal/sandbox/ -count=5: clean. Unrelated and pre-existing here: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-8cd239 | Claude Code | 12 prompts Origin-Snapshot: dd397730a138 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 3 prompts Origin-Snapshot: d2b8f44a9abc * fix(sandbox): a reader outliving its writer keeps read authority, not write Reported by @jatmn, and it is a privilege escalation rather than a lifetime bug. AddTemporaryRead, given a path already covered by a temporary WRITE root, took a reference on that write root and returned an undo that released it. The lifetime was then correct — the reader no longer lost its access when the writer cleaned up — but the capability was wrong. extraRoots feeds validate(), which authorises WRITES, so once the write holder released first the path stayed in extraRoots for as long as the read holder lived. A caller that asked only to READ could write anywhere below the former write root after the write grant had ended. His diagnosis names the root cause exactly: one write-capability reference was doing two jobs, tracking a lifetime and standing in for a read-only dependent. The lifetime is now kept as what the caller actually asked for — a temporary READ root on its own path. AddTemporaryRead consults permanentWriteRootCoversLocked rather than writeRootCoversLocked, so a merely temporary write cover no longer short-circuits it into borrowing write authority. Mutation-checked: restoring writeRootCoversLocked in that branch compiles and fails the regression on three separate assertions — the reader keeps no read root of its own, ReadRoots() loses the path, and the writer's cleanup revokes a live read grant. Two things a verification pass turned up that are worth recording: The comment justifying this design said readRoots "feeds ReadRoots() and nothing else". That was false — it also feeds ExtraReadRoots(). The security argument survives, because neither confers write authority and validate() reads workspaceRoot plus extraRoots, which this path never enters. But that sentence IS the written justification for the whole choice, so it now lists both and says why neither grants write. The "the reader releases first" subtest passes on the unfixed tree: the escalation needs the WRITER to go first. It is a real no-regression control for the opposite ordering, not evidence the finding is closed, and it is renamed to say so rather than padding the count. Rebased onto |