A profile that sets denyRead drops WRITE_RESTRICTED, and the token that
replaces it carried the World SID. Every principal carries Everyone, so the
restricted-SID check passed for free on any path whose DACL grants Everyone
write, and the workspace write jail fell back to the caller's own permissions,
which is the boundary the token exists to be stricter than. No privilege, no
symlink and no race: an Everyone-writable directory was enough, and share roots
opened Everyone:F and loose installer trees supply them. #865 closed this for
the WRITE_RESTRICTED token and left this half open.
It could not simply be dropped. Without WRITE_RESTRICTED the restricted-SID
check covers reads as well, and default Windows DACLs grant BUILTIN\Users
rather than anything in the list, so a token without Everyone cannot open
cmd.exe and dies at launch with a bare access denial. Confirmed by
impersonating both token shapes against C:\Windows\System32\cmd.exe.
So the read capability takes its place. BuildWindowsACLPlan already grants that
SID on every read root and denies it on every denyRead path under exactly the
same condition, so the read allowance and the restriction become one decision,
and it names only what setup granted rather than every principal on the
machine. The principal path already worked this way.
Regressions drive a real restricted token through a real impersonated write:
an Everyone-only DACL is refused, a directory the capability grants is written,
and the token's own restricted-SID list is read back from the token. A separate
test pins the plan and the token to the same answer, because the two halves are
decided in different files off the same field.
Closes#869
cleanupSandboxRuntimeRoots reclaims inactive sibling workspace roots on an age
and count policy, treating them as disposable cache state. Setup and its marker
treat their DACL as durable provisioned state. When the reclaimed workspace runs
again, command-side preparation recreates the same deterministic pathname as the
ordinary caller, and the new directory inherits from its parent without the
capability SID ACE elevated setup applied to the object that used to be there.
ValidateWindowsSandboxSetupMarker still passed, because it fingerprints pathnames
and actions rather than the identity of the ACL-bearing object. So the restricted
child launched and then failed its cache, temp and package-cache writes with a
bare access-denied and nothing pointing at setup.
The command now asks the object, not the pathname: before the token is minted it
confirms the runtime root carries an allow ACE for its capability SID, and
refuses with the setup remedy when it does not. That reconciles the two owners
without making cleanup preserve trees it is meant to reclaim.
The regression applies a real plan, reclaims the directory the way cleanup does,
recreates the pathname the way an ordinary run does, and asserts the refusal
names the root and the remedy; accepting a missing grant fails it on that. Not
covered, and said plainly: a write performed by a real capability token, which
needs a provisioned sandbox an unelevated box cannot create. The ACE presence is
the fact the command consumes, and that is what is pinned.
ensureWindowsSandboxRuntimeCandidates ran as Administrator and used os.MkdirAll,
which follows links. Both candidate ancestries are prepared by the invoking user,
so a junction planted at a not-yet-created component below the cache directory,
or at the fallback anchor, made elevated setup build the runtime tree beneath a
redirected, Administrator-writable target. The ACL applier's no-follow check runs
afterwards and does spot the redirection, but the privileged create has already
happened by then and sits outside its rollback boundary. A leaf check after
MkdirAll would leave the same gap.
Candidates are now created by peermsg.EnsurePrivateDir, which descends
handle-relative with no-follow, refuses any component that is a link, and refuses
a leaf this user does not own. It is handed a physically resolved base first,
because it walks from the volume root and a redirected cache or TEMP above the
owned tail is the operator's business: the same pairing the fallback anchor
already uses, and the same mistake that refused every fallback on macOS when the
parent was left unresolved. physicalTempDir is generalised to physicalDir for
that, keeping GetFinalPathNameByHandle on Windows because EvalSymlinks does not
traverse a junction.
The regression plants a junction at the first owned component and asserts the
redirected target stays empty; letting the creator follow links again fails it on
that assertion.
The .git rename guard belonged to one planner rather than to the grant. The
allow-write mask both backends share includes DELETE, and it inherits from each
write root onto .git; the carveouts that actually protect git are attached to
.git/config and .git/hooks as objects, so renaming .git aside and recreating it
discards them and hands back credential.helper and core.hooksPath. The principal
planner denied DELETE there and the capability planner did not, and the
capability backend is the default. Both now derive that object from one place, so
a change to the shared mask cannot update one consumer and miss the other. The
regression reads the ACE back off the applied object rather than asserting plan
shape, because the plan is what was wrong.
The unelevated tier now states its boundary before it mutates anything. A profile
carrying denyRead runs on a strict token, which applies the restricted-SID check
to reads, so the read capability must be granted at the volume root that
permissionProfileReadRoots seeds. Elevated setup can write that DACL; an ordinary
user cannot, and the common opener asks for WRITE_DAC on every entry, so this
tier failed on that one root on every command. Dropping the root ACE instead is
not available, because the strict token would then fail its own read check for
the executable. So it is refused up front, naming the root, the denyRead cause
and elevated setup as the remedy. That last part matters: the previous
after-the-fact diagnostic told the reader that running setup elevated would NOT
fix it, which is the opposite of the truth for this case.
Separately, `sandbox exec` no longer reports a signaled child as exit 255. On
Unix ExitCode() answers -1 for signal termination and os.Exit truncates that, so
a child killed by SIGTERM was indistinguishable from one that chose to exit 255.
It now returns the conventional 128+signal, read from the ProcessState, with
Windows keeping the exit code it already reports.
peermsg.EnsurePrivateDir walks every component from the root and refuses
any link. That is the right rule for the directory Zero owns and the wrong
rule for the operator's temp location above it: on macOS os.TempDir sits
under /var, a symlink to /private/var, so 8af05997 refused every fallback
on every Mac with "refusing non-directory or symlink runtime path component
var".
Anchor the fallback under the physical temp dir instead, so only the anchor
itself is a new component. EvalSymlinks off Windows; GetFinalPathNameByHandle
on Windows, where EvalSymlinks does not traverse a junction and a redirected
TEMP would trip the same way. The fixture compares against the resolved
temp dir rather than its spelling, since t.TempDir can come back as an 8.3
short name.
The fallback runtime root moved from a fresh os.MkdirTemp parent to the
predictable <temp>/zero/runtime/v1/<hash> so that elevated setup and the later
command process agree on it. That agreement is required and stays. But a
predictable path directly under the shared system temp is not a private
allocation. On an ordinary shared /tmp another local user can create /tmp/zero
with mode 0700 first, and every affected user then fails before the sandbox
command starts; a pre-existing redirected component makes host-side
preparation lease, create, chmod and clean somewhere other than the derived
tree. Determinism was being used as if it were ownership.
The tree now lives beneath <temp>/zero-runtime-<user>, and that anchor is
created and validated by peermsg.EnsurePrivateDir before anything is built
under it: handle-relative descent, no-follow, refused if any component is a
link or the leaf is not owned by the current user, mode 0700. That primitive
already existed with exactly the semantics jatmn described, so it is exported
and reused rather than written a fourth time. On Unix the uid is in the name
because /tmp is shared; on Windows os.TempDir is already per-user and the
private descriptor plus owner check do the rest.
Validation runs on both paths that can hand back a fallback root:
sandboxRuntimeRootFor returns one itself when the cache lands inside the
workspace, and prepareSandboxRuntime asks for one when the cache root cannot be
leased. Both are covered, each with a junction planted at the anchor, and both
refuse with the junction target untouched; a control shows a clean fallback
still prepares beneath the anchor.
Two things went wrong on the way and are worth recording. The first draft
called pathWithinRoot with its arguments reversed, which asked whether the
anchor was beneath the root, answered false every time, and skipped the
validation entirely while every dependent test reported success through the
junction. And the first lease-failure trigger planted its file at the derived
leaf, so preparation failed on that mkdir before ever reaching the fallback
branch. Both fixed; with validation disabled the two refusal tests now die on
"reported success" while the control stays green.
TestPrincipalLaunchPreflightExplainsAMissingPrivilege ran the real preflight and
then asserted SeAssignPrimaryTokenPrivilege appeared in the message. Which
privilege is missing is a property of the account running the test, not of the
code: an unelevated user often holds neither, but this box holds
SeAssignPrimaryTokenPrivilege and not SeIncreaseQuotaPrivilege, so only the
other name appeared and the test failed for a reason that was never a defect.
Its own comment said it asserted "the shape of the answer rather than a fixed
verdict, because the verdict legitimately differs by machine", and then it
hardcoded one of the two names. The comment was right and the assertion did not
match it.
principalLaunchPrivilegeError splits the rendering out from the token work, so
each combination is driven directly: neither held, only assign-primary-token,
only increase-quota, and nothing missing. Each asserts the message names what IS
missing and does NOT name the one that is held, since sending an operator after
a privilege they already have is its own failure. Making the message always name
both fails the two single-privilege cases.
The real preflight keeps a test, reduced to what does not vary: if it refuses,
the refusal names at least one of the two rather than something the operator
cannot act on. Stability across calls is unchanged.
runtimeRootTestConfig put the workspace and sandbox home under t.TempDir, but
windowsSandboxRuntimeCandidates still derived a candidate through the
production sandboxUserCacheDir seam, and the test creates every candidate and
registers os.RemoveAll cleanup for each.
So the test reached into the real user cache: it fails outright on a read-only
home, and on an ordinary developer or CI account it creates and then deletes a
path outside its own storage. The workspace hash makes a collision unlikely; it
does not make somebody else's directory test-owned.
Redirected before any candidate is derived, and restored with t.Cleanup.
Production derivation is unchanged, and the assertion that setup materializes
every granted write root still stands.
A before/after count of the real cache cannot observe this, since the test
creates and removes in the same run. The mechanism is the evidence:
prepareSandboxRuntime reads sandboxUserCacheDir, and the test RemoveAlls every
candidate that derivation produces.
The gate asked whether the CALLING process held SeAssignPrimaryTokenPrivilege
and SeIncreaseQuotaPrivilege. That is the wrong lifetime, and it got the answer
backwards where it mattered.
The process that needs those privileges is the later, ordinary command process.
Setup does not run it, and nothing carries launch authority across the UAC
boundary. So the caller-token check refused unelevated setup, which was never
the dangerous case, and PASSED elevated setup, which is precisely the case that
lands a local account, its password, its logon-right assignments, workspace
ACEs, the recovery ledger and network filter state for a backend no later
command can use. The check existed to prevent durable machine state serving
something that cannot run, and in the one configuration that creates that state
it allowed it.
Requiring every sandboxed command to run elevated would line the privileges up
and is deliberately not done: it is a worse boundary than the restricted token
it would replace, and it is not the lifecycle this is for. Until a reviewed
bootstrap or broker exists, provisioning is closed rather than half-working.
windowsPrincipalLaunchAvailable replaces the preflight var and takes no
argument, because the answer is about which mechanisms exist rather than about
the current process. It is the one place that learns to say yes when a broker
lands, and everything below it -- the plans, the ledger, the ACL and filter
work -- still builds and is still tested against it.
The runner keeps its own privilege check at launch time, which is the correct
lifetime for one; only the setup wiring is removed.
The regression test asserts the property the old shape could not express: the
refusal is stable regardless of what the calling process holds, and it does not
tell the operator to re-run from an elevated terminal, which is the advice that
produced the unusable state. Restoring the caller-passes behaviour fails both
new tests.
Refs #662, which this is foundation for and does not close.
TestTeardownPathDerivationCreatesNothing compared the number of entries in the
SHARED temp directory before and after deriving the teardown paths. The
assertion it wants is that deriving a path creates nothing, and a count cannot
tell creation from removal: that root is also used by every other test binary
running at the same time and by the OS, so a concurrent cleanup made the count
fall and the failure read "temp directory gained -1 entries".
It compares the entry names now and reports anything that APPEARED, which is the
question actually being asked and is indifferent to whatever else disappears.
The failure also names the entry rather than a delta, so the next person sees
what was created instead of a number.
Measured on an ordinary unelevated session: the token holds neither
SeAssignPrimaryTokenPrivilege nor SeIncreaseQuotaPrivilege, so the principal
launch path can never engage for the process the runner is designed to be
called from. Elevated setup does not change that, because the command runs
later from the caller rather than from setup.
Until now setup succeeded completely in that situation. It created a local
account, its password, its logon-right assignments, the workspace ACEs, the
recovery ledger and the network filter state, and then every principal-mode
command refused before opening its executable. The operator was left with
durable machine state serving a backend that cannot run, and nothing said so at
the point they could still act on it.
The check runs in the caller's own process, which is the one whose privileges
decide the answer, and before anything crosses the UAC boundary. It is wired to
the same function the launch path uses, so a change to what a launch requires
cannot leave setup provisioning for a capability that no longer exists.
Three existing tests asserted argument plumbing with the opt-in on and passed
only because nothing checked; they stub the preflight now, so they no longer
depend on the privileges of whoever runs the suite.
This does not give the principal backend a working launch path. That needs a
different architecture and is not in this change.
Three follow-ups from going back over the review findings. None was raised
directly; two are the same shape as things that were.
windowsPrincipalPlanFingerprint was the one of three buildWindowsPrincipalACLPlan
call sites that did not pass DenyWrite. Apply and teardown both did, so the hash
the marker carries described a different plan than the one that actually gets
applied, and a change to the policy's deny-write paths moved the applied plan
while leaving the marker where it was. It is not a live hole, because the
capability ACLPlanHash covers the same paths and moves the marker anyway. That
is also exactly what would have kept it invisible until somebody changed the
capability plan's shape.
The role list was spelled out in three places and two of them are meant to
differ, which is why writing them out by hand kept going wrong.
windowsSandboxPrincipalIsInstalled asked only the offline and online roles while
teardown retires the legacy account too, so a machine still holding the untagged
pre-split account was reported clean and the opted-out marker claimed a teardown
that had not happened. Provisioning has the opposite constraint: legacy must
never appear there or setup would recreate that account on every run of an
already-upgraded machine. Both are named now, windowsSandboxLiveRoles and
windowsSandboxRetirableRoles, with a test pinning the legacy role into exactly
one of them.
And the opt-out error said "retire the principal" when a workspace has two plus
the legacy one, all of which that re-run retires.
jatmn's P1 asked for creation AND cleanup to be handle-bound rather than
resolved from a pathname. Creation and the materialization unwind both are.
The DACL restore was not, and its comment read as though it were.
It re-opened the target with a no-follow open, which rules out a reparse point
swapped in since apply and nothing else. The other substitution passes it
untouched: rename the target aside and put an ordinary directory of the same
name in its place. Nothing there is a link, so the open succeeds, the pre-apply
DACL lands on the decoy, and the real object keeps the ACEs from the setup that
just aborted. The snapshot now records the volume serial and file index of the
object it read the DACL from, the restore proves it is writing back to that same
object, and a mismatch is refused rather than forced through. Leaving the real
object with the aborted setup's ACEs is the safe direction, since the caller is
failing anyway.
Three things nothing was pinning, all of which could be deleted with a green
suite. This repository has already had a fix silently reverted by a later
change, so these are worth more than their size.
rollbackWindowsACLSnapshots documented its reverse iteration as pinned by
TestRollbackUnwindsDescendantsBeforeAncestors. That test did not exist anywhere
in the repo; the only match for the name was the sentence claiming it. The
ordering is load-bearing twice over, because a materialized directory must be
empty before its own removal and SetSecurityInfo propagates inheritable ACEs
downward, so the ancestor has to go last. It exists now.
The principal ACL rollback restores the ledger alongside the DACLs, and the
neighbouring test asserted only the order of the two ACL reverts and never read
the ledger. Deleting the restore left the suite green while the paths it put
back were unnamed, so cleanup could not find them.
windowsSandboxUserIsManaged promises an account carrying the legacy bare comment
gets the workspace key stamped on. The probe for the legacy comment was seamed
and the upgrade itself was not, so nothing could observe the call. It is seamed
now, with the negative case covered too so the assertion cannot be satisfied by
an unconditional rewrite.
* feat(sandbox): give each workspace an offline and an online principal
The principal backend stood down whenever the network was denied, which is the
default, so opting into it left the restricted-token path doing all the work in
normal use. The reason was that network denial is enforced by block filters
keyed to the offline-marker SID, and a principal token cannot carry it:
LogonUser builds a token from an account's real group memberships, and the
marker is a synthetic capability SID.
A real local group closes that gap. ZeroSandboxOffline is created by setup, the
block filters name its SID alongside the marker, and a principal is denied the
network by being a member. Each workspace therefore gets two accounts that
differ only in that membership, and the command's network mode selects between
them. A group rather than each principal's own SID because principals are per
workspace: one filter set covers every offline principal on the machine instead
of needing a filter per workspace.
Both principals are provisioned together even though a given setup run sees one
profile, because setup needs elevation and commands do not. Provisioning lazily
would mean an unelevated command discovering it needs an account it cannot
create. They also get identical filesystem access, so an approved network
command sees the same filesystem as an ordinary one.
Two orderings are load bearing. The network plan is now built AFTER
provisioning, because the group it keys to is created there; planning first
installed filters naming only the marker and left every offline principal with
an open network while looking correctly set up. And the role tag sits before
the workspace hash in the account name, so truncation at the 20 character limit
eats hash characters rather than the tag, which would otherwise collide the two
roles onto one account on exactly the workspaces most likely to truncate.
Anything that is not an explicit allow maps to the offline principal, so an
unrecognised mode loses the network rather than keeping it.
The filter identity set is resolved rather than assumed, and stays absent until
the group exists, so a machine that never provisions principals computes the
same plan as before. That matters because the plan is hashed into the setup
marker and re-derived on every command; an identity set that differed between
setup and the command path would fail every command as out of date.
Cost worth stating: this doubles the sandbox accounts on a machine, to two per
workspace.
* fix(sandbox): prove ownership before deleting a sandbox account
removeWindowsSandboxIdentity is called with a DERIVED name, so it could be
pointed at a name that happens to belong to somebody else's local account.
Deleting a user is not a recoverable mistake, and the only thing standing
between the two cases was the name matching a pattern we generate ourselves.
Raised by CodeRabbit against the test fixtures, but the production teardown path
had the same hazard, so the guard belongs there rather than in the tests.
The ownership check itself now lives on the base branch, which grew the same
helper to stop provisioning ADOPTING a squatted account. This applies it to the
other end: an account that is not ours is left alone rather than deleted. The
gated tests get the protection for free, since their pre-clean goes through the
same helper.
Also appends the trimmed offline-group SID rather than the raw one. Worth noting
the reported consequence does not hold: newWindowsWFPUserCondition canonicalises
before converting, so a padded value would have been trimmed before reaching
StringToSid. The resolver returns SID.String(), which never carries whitespace,
so this is defensive tidying rather than a fix.
* fix(sandbox): assert the filters cover principals, and report a retained account
Two follow-ups from review, both on the same theme: a control that quietly does
nothing looks identical to one that works.
The network plan must be built AFTER provisioning, because provisioning creates
the group the block filters name. Built first, the filters name only the
offline marker and every offline principal has an open network while setup
reports success. That ordering is invisible at the call site, so setup now
checks the plan actually names the offline group before installing anything and
refuses if it does not. A later refactor that moves the plan build back fails
loudly instead of producing a security control that enforces nothing.
The predicate is separate so it can be asserted directly: a plan carrying only
the marker must read as uncovered, one carrying the group as covered, case
differences must not read as missing, and a host with no group provisioned has
no principal to miss and must not be refused. Making coverage always report
true fails that test.
Removal also reported plain success when it declined to delete an account Zero
did not create. Leaving it alone is right, but telling an operator cleanup
completed when a name they may care about was deliberately retained is not.
That case is now a distinguishable sentinel, and teardown treats it as success,
since "no principal of ours under this name" is the goal state either way.
* fix(sandbox): spare adopted principals when dual-role setup rolls back
Provisioning already declined to delete an account it had adopted, and the
outer setup rollback then appended an unconditional removal for every role
that got that far. With two roles that is the common case rather than an
unlucky one: the offline role usually succeeds, so a failure in the online
role or in ACL application destroyed a principal that was working before
the run started. The removal closure is now only appended for a principal
this run created.
Threads the workspace key into the delete path as well. Ownership was
proven from the account comment alone, which on a name collision belongs
to a DIFFERENT workspace, so deleting it would have been the same
unrecoverable mistake the check exists to prevent.
Policy DenyWrite now reaches the principal ACL plan here too, matching the
single-principal path.
Fixes the mode-independence test, which required exactly one identity SID
and so failed on any Windows host that already had ZeroSandboxOffline,
where the plan legitimately carries two. CI never saw it because the Linux
and macOS jobs leave the hook nil and a fresh Windows runner has no group.
The hook is now pinned, and the test additionally asserts the property it
is named for in the group-present case, including that the infra hash
changes when the group appears, which is the cross-workspace coupling
raised for a maintainer decision.
* test(sandbox): stub the password reset and pin the resolved group SIDs
Two review points on the tests added in the previous commit.
The provisioning stub left resetWindowsSandboxUserPassword as a real
call. Nothing under test reaches it any more, because rotation moved to
the caller, but a test that resets a real managed account's password if
the code ever moves back is not a risk worth carrying. It is now stubbed
to fail the test instead, which also states the contract.
The group-present assertion checked only that two identity SIDs were
present. A duplicated offline marker or an unrelated SID would satisfy
that while meaning something quite different, so it now pins both
positions.
Also drops a duplicated stub assignment left by the rebase.
* fix(sandbox): refuse an offline group zero does not own
ensureWindowsLocalGroup accepted NERR_GroupExists and ERROR_ALIAS_EXISTS as
success without inspecting the group it was about to reuse. Setup then resolved
that group's SID and installed it on the persistent WFP deny filters, and made
the sandbox principal a member of it.
If anything else on the machine already owns a group named ZeroSandboxOffline —
another tool, a policy, a prior unrelated convention — that is not a no-op. Every
existing member abruptly loses outbound access, because the filters now name
their group. In the other direction the sandbox principal inherits whatever
permissions that group carries, which is the opposite of what an offline
principal is for.
The add now reports its raw status and the already-exists branch verifies the
group carries this setup's managed marker before adopting it, failing with an
actionable message otherwise. A lookup error fails closed rather than adopting.
NetLocalGroupAdd and the ownership lookup sit behind seams so the branch is
reachable in tests without an elevated machine; the marker compared is the
group's own, so the principals group and the offline group cannot be confused.
Reported by jatmn on #812.
* fix(sandbox): recheck offline group membership before minting a token
Network denial does not follow from picking the offline account. The WFP block
filters match the offline GROUP'S SID, and LogonUser builds a token from the
account's real memberships — so membership is the whole enforcement, and the
command path never revalidated it.
An account that drifts out of ZeroSandboxOffline through local policy, an
administrator, or a re-setup that could not re-add it still resolves, still has
its stored secret, and still logs on. Its token no longer satisfies the filter
condition, so a NetworkDeny command gets full egress under a profile that asked
for none. The stale setup marker keeps the whole path looking healthy.
The offline role now confirms the membership its mode depends on before the
secret is read, and falls back to the restricted token when it is absent. That
direction is deliberate: the restricted token carries the offline marker the
same filters match, so egress stays blocked, and only read confinement is lost.
A failed lookup surfaces rather than downgrading silently. The online role is
not checked, since it is not in that group by design.
Reported by jatmn on #812.
* fix(sandbox): derive setup's runtime root deterministically or not at all
windowsSandboxRuntimeRootPath resolved through sandboxRuntimeRootFor, which
falls back to os.MkdirTemp when the user cache lives inside the workspace and
memoizes that only in-process.
Elevated setup is its own process. It granted the principals an ACE on temp root
A; the next command, being a new process, derived temp root B, where the
principal has no ACE, and failed ordinary cache writes with a bare ACCESS_DENIED
and nothing pointing at the sandbox. Teardown, a third process, cleaned a third
directory. The three callers that have to agree exactly could not agree at all.
Setup now uses the same side-effect-free derivation teardown already used, and
reports no runtime root when that derivation is unusable rather than inventing
one. A root only the granting process can name is worse than no root: the
principal loses the runtime tree, which is a degraded sandbox, instead of the
sandbox appearing provisioned while every command fails.
TestTeardownPathDerivationCreatesNothing asserted the opposite — that setup
"should still fall back to a usable tree" — so it is inverted here, with the
reasoning recorded in the test. That assertion encoded the assumption this
finding overturns: a per-process temp tree is not usable. Restoring the fallback
fails it with the invented path in the message, and a new
TestSetupAndTeardownDeriveTheSameRuntimeRoot pins the ordinary case, so
"report none" cannot quietly become the answer everywhere.
Reported by jatmn on #812.
* style(sandbox): separate the two doc paragraphs the rebase ran together
Adapting the ACL-record test to dual roles left #808's fail-open rationale
and #812's per-role rationale as one unbroken block. Both are worth
keeping; they are two points, not one.
* fix(sandbox): fail closed when offline-group coverage cannot be verified
The post-provisioning assertion ran inside `if groupErr == nil`, so a
failed lookup skipped it and setup carried on to install filters and
write a success marker. The comment directly above it says what that
costs: a machine reporting a successful setup while every offline
principal has an open network.
An empty SID was the same hole by a different route. Resolving to
("", nil) means the group does not exist, which is the ordinary state
before provisioning and an impossible one after it, and
WindowsNetworkPlanCoversPrincipals answers true for an empty SID
(correctly, for the pre-provisioning callers that ask it). So the
assertion passed vacuously in exactly the case where the group setup
had just created was missing.
Move the check into assertWindowsNetworkPlanCoversOfflineGroup, which
takes the resolver as a parameter and fails closed on every answer that
is not a definite yes: lookup error (wrapped, so the Win32 reason still
reaches the operator), empty SID, plan omitting the group, and a nil
resolver. Setup rolls back and exits 1 on each.
Taking the resolver as a parameter is what makes the error paths
testable, which is the regression the review asked for.
Reported by @anandh8x on #812.
* fix(sandbox): scope the offline-group assert to provisioned runs
c404ebd2 made the coverage assert reject an empty group SID, closing the
vacuous pass where a missing group counted as covered. It ran the assert
unconditionally, and the offline group is only created inside
provisionWindowsSandboxIdentity, which runs only under the
ZERO_WINDOWS_SANDBOX_IDENTITY opt-in.
So on a default machine with principals opted out, the resolver reports
("", nil) exactly as it should, and setup died with "the sandbox offline
group does not exist after provisioning" on a path that worked before
c404ebd2. The empty-SID rejection is correct after provisioning and wrong
before it.
Pass provisioned to the assert and return early when it is false, gated
at the call site on the same windowsSandboxIdentityEnabled check that
decides whether principals are provisioned at all. The fail-closed
behaviour anandh8x asked for is unchanged whenever provisioning ran.
Reported by @jatmn on #812.
* fix(sandbox): keep opted-out setup markers valid, and refuse a foreign offline group
Two of jatmn's findings on this PR.
Existing markers stay compatible (maintainer decision). The offline group is
machine-global, and the plan included its SID whenever the group existed. So the
first workspace to opt in changed the computed NetworkInfraHash for every OTHER
sandbox home on the machine, and those homes rejected their own stored markers
until each was re-run from an elevated terminal, having opted into nothing.
The inclusion is now gated on THIS home's opt-in rather than on the group
existing, so an opted-out home computes exactly the plan it computed before any
of this existed. Setup and the command path read the flag from the same
environment, so they agree. Opting in after setup does invalidate that home's
marker, which is correct: it has no principals yet.
Do not install filters for an unowned offline group (P1). The ownership check
only ran through principal provisioning, so an opt-out setup reached the resolver
and adopted any local alias carrying the name. applyWindowsNetworkPlan turns
every SID in the plan into an allowed-to-match WFP descriptor, so a foreign group
meant global deny filters against every one of ITS members: anyone with a local
group by that name loses the network for those accounts because we ran setup.
The resolver now requires the managed comment, and refuses rather than skipping,
because a plan whose filters cover no principal while setup reports success is
the failure this backend exists to prevent.
Three existing tests exercised the group path without the opt-in and now set it.
The new test asserts the other direction, that an opted-out home's hash is
unchanged when another workspace creates the group, since that is the property
the decision turns on. Verified both ways: disabling the gate fails the existing
tests, making it unconditional fails the new one.
* fix(doctor): report the principal that dual-role setup actually uses
jatmn's P2. This branch made the offline principal work under NetworkDeny, but
the doctor helper still described the old restricted-token standdown, so
`zero doctor` reported active:false and told operators reads were unconfined for
a correctly provisioned offline principal, recommending they enable network or
drop the opt-in to fix something that was not broken.
WindowsSandboxPrincipalInactiveReason is removed rather than reworded. Its only
condition was the deny-mode standdown, so after this branch it could never return
anything, and a check that cannot fire is worse than no check.
What replaced it matters more than what it said. That helper existed to be the
SINGLE rule doctor and the runtime both read, precisely so they could not drift,
and drift is what happened anyway when dual-role changed the behaviour under one
of them. WindowsSandboxPrincipalRoleForNetwork is now that shared rule:
windowsSandboxRoleForNetwork delegates to it and doctor calls it, so the reported
account and the used account cannot disagree. Doctor now names which principal a
command runs as instead of asserting a standdown.
One thing the existing tests caught. Routing the shared rule through
NormalizeNetworkMode case-folds, so "ALLOW" selected the ONLINE principal where
the runtime required an exact match and failed closed to offline. Sharing a rule
is only an improvement if it shares the stricter one, so the comparison is exact
and a test pins the casing.
* fix(sandbox): converge the dual-role branch with the rebased identity work
Rebasing #812 onto the new #808 needed real resolution rather than taking a
side, and this records what each conflict actually decided.
The account key. #808 made the principal key caller-scoped so elevated setup
provisions the account the caller will later look for. #812 derived usernames
from the workspace key alone. Every username derivation now uses the caller
scoped key, including the two inline call sites a blanket substitution missed:
the identity lookup in the unrecorded-retire path and the ledger read in
windowsPrincipalRevocationPaths. That second one is why teardown could not find
a recorded root the current policy no longer named. The setup LOCK stays keyed
to the workspace on purpose, because two users setting up one shared workspace
still write DACLs on the same paths and must serialize against each other.
The network plan stays where #812 put it, after provisioning, because the block
filters are keyed to the offline group that provisioning creates. Building it
earlier, as #808 does, would install filters naming only the marker and leave
every offline principal with an open network while looking correctly set up.
Group ownership converged on #812's implementation, not mine. #808 grew a check
hardcoded to the users group; #812 already had the general
ensureWindowsLocalGroup plus windowsLocalGroupOwnedByZero, which covers both
managed groups. The narrower version was removed and its test rewritten against
the general seams, so the users group keeps the coverage anandh8x asked for
while the offline group keeps its own.
Two functions the resolution dropped and the compiler caught:
windowsSandboxPrincipalKey and windowsCurrentUserSID. Worth naming because the
previous attempt at this convergence lost the same first function silently.
Also closes jatmn's remaining findings on this branch. The opt-out installed
check now asks about BOTH role accounts rather than one, since retiring one
while the other survives is exactly the half-done teardown an opted-out marker
must not report as success. And the post-provisioning filter-coverage assert now
resolves the offline group through the existing hook rather than the concrete
function, so it is stubbable like everything else around it.
The SensitiveEnvKeys omission jatmn reported on sandbox_exec.go arrives with the
rebase; it was fixed on #808.
* fix(sandbox): keep offline coverage and grant the fallback runtime root
Two ways the dual-role split left a workspace worse off than it looked.
The block filters are machine-global and every setup installs them by
deleting and recreating one fixed set, but the plan a home builds names
the offline group only when THAT home opted in. So an ordinary opted-out
setup for a second workspace replaced the filters without the group SID,
and the first workspace's offline principal, still in the group, still
passing the runtime membership check and still holding a valid marker,
was no longer matched by any filter. A NetworkDeny command there gained
egress silently, because the second setup did exactly what it was asked.
The gate itself is left alone, because it is load bearing for a different
reason: the plan is hashed into each home's marker, and keying it on the
group's existence made the first workspace to opt in invalidate every
other home's marker on the machine. What a home RECORDS is about its own
configuration; what setup INSTALLS is about the machine. Answering both
from one plan is what forced a choice between stale markers and a silent
hole, so WindowsNetworkPlanForApply answers the second question only, at
the apply call site, leaving the fingerprinted plan untouched.
Setup also granted the principals an ACE on the cache-derived runtime
root alone, and none at all when the cache sat inside the workspace. That
was correct when the other branch minted a random per-process directory
through MkdirTemp, but fallbackSandboxRuntimeRoot now derives its path by
hashing the workspace and creates nothing, so every process agrees on it.
Commands in that layout therefore DO select it and redirect TMP, GOCACHE
and the package caches into it, against a tree neither principal could
write. Setup now grants the same candidate set the capability plan
already covers, and creates each one, since applyWindowsACLPlan fails on
a target that does not exist.
Reverting either fix fails its regression: the opted-out plan installs
filters naming only the marker SID, and setup grants nothing while
commands write to Temp\zero\runtime\v1\<hash>.
Two existing assertions had to be inverted rather than adapted, and both
were asserting the old bug. One required setup to report NO runtime root
in the cache-inside-workspace layout; the other compared setup's single
root for equality against the command's choice. Setup covers the whole
candidate set now precisely because that choice is made per process, so
the contract is membership.
* fix(sandbox): treat an unreadable offline group as a failure, and retire the pre-split principal
Three findings from review.
A resolution failure in WindowsNetworkPlanForApply returned a marker-only plan,
and the machine's WFP filters are replaced wholesale from that plan. So an
opted-out setup whose group lookup failed transiently removed the offline group
SID another workspace's principals depend on, and that workspace's NetworkDeny
commands silently regained egress while its marker and its direct membership
check both still passed. The old reasoning was that refusing to install would
trade a partial denial for no denial; that is wrong, because the alternative to
installing is leaving the existing filters alone. It is fatal now, which is only
reachable from an opted-out home since assertWindowsNetworkPlanCoversOfflineGroup
is gated on provisioned.
Splitting the single principal into offline and online roles changed both
account names without changing the marker schema, so an installation made by the
previous version kept a valid marker, setup was never re-run, and the runner
found neither zero-sbx-d<key> nor zero-sbx-n<key> and fell back to the
restricted-token backend with no read confinement. The schema version is bumped
so that installation reports as out of date, and a legacy role derives the old
untagged name so the ordered retirement can remove the account, its secret, its
logon rights, its ACEs and its ledger. It is retired, never provisioned, and
there is a test for that because the two lists are one line apart.
Doctor reported that commands run as the selected principal whenever the marker
validated, but marker validation compares serialized plans and hashes and names
no account: deleting the account or its secret, or dropping the offline account
out of its group, leaves the marker valid while the runtime falls back or fails.
Verifying liveness needs Windows-only queries internal/doctor cannot make, so
the claim is narrowed to what the marker actually proves. The role is still
reported, since that part is derived rather than assumed.
TestACLComparablePathDoesNotResolveAReparsePoint failed on Windows CI
while passing locally. The test was wrong, not the code.
It asserted that windowsACLComparablePath returns a string equal to the
path passed in. GetLongPathName legitimately rewrites that string: a CI
runner's temp directory is an 8.3 short name, so expanding RUNNER~1 to
runneradmin produces a different string naming exactly the same object,
and the assertion failed on the one property the function is supposed to
have.
Both sides are now normalized before comparison, which states the real
property: a path THROUGH the reparse point must not normalize to the
target's normalization, or the guard would compare a redirected handle
against a redirected expectation and match itself. A second assertion
keeps the link component present so a resolution to something else
entirely still fails.
The test's documented limit is unchanged and still honest: a junction
cannot separate the old basis from the new one, because EvalSymlinks does
not resolve junctions either. It remains an invariant test.
verifyWindowsACLTargetNotRedirected asks GetFinalPathNameByHandle where
the handle landed, then compared that against
canonicalSandboxWorkspaceRoot(path), which runs filepath.EvalSymlinks.
That is the same resolution the kernel had just performed, so for a
directory symlink the two sides agreed precisely BECAUSE the redirect
happened: elevated setup went on to rewrite the DACL of an object outside
the workspace while the check reported success.
Junctions were rejected, but by accident rather than by design. Go reports
a junction as ModeIrregular rather than ModeSymlink, so EvalSymlinks
refuses it, the canonicalization falls back to the lexical path, and the
mismatch surfaces. That is a property of the standard library's mode bits,
not of this guard, and a Go release that resolved mount points would
silently disarm the one case it was known to catch.
The expected side is now normalized without resolving anything.
GetLongPathName expands an 8.3 short name by reading directory entries and
does not follow a link to its target, and EqualFold still covers casing,
so the two spellings that legitimately name the same object still compare
equal. Every failure degrades to the lexically cleaned path, which can
only produce a spurious refusal, never a spurious match.
A target deliberately spelled through a symlink is now refused. That is
the intended direction: the question here is whether the object the handle
landed on is the object that was named.
On the tests, plainly: the directory-symlink regression is the one that
separates the old basis from the new, and it needs Developer Mode or
SeCreateSymbolicLinkPrivilege, so it skips on a machine without either and
skipped on mine. The junction test beside it passes against both bases and
says so in its own comment; it is an invariant test guarding the Go
behaviour the old code accidentally depended on, not a regression for this
change.
Both unelevated ACL failures told the reader to re-run with `--sandbox
forbid`. There is no such option: SandboxPreferenceForbid is an internal
engine state with no flag behind it, so acting on it produced an unknown
option and left them stuck on the failure they had just been told how to
clear. Advice that does not work costs more than none, because finding
that out takes the reader's time.
Name the real way out instead, the user config key, which is honored
from global config only so a cloned repo cannot set it. The
elevated-setup remedy beside it was already correct and stays.
Reported by jatmn against the same string on #640. It predates this
branch, having arrived with the unelevated fallback tier in #427, and
the copy on #886 is fixed separately in 1b304e13.
Also covers the secret write with the junction regression it was owed:
the caller owns the sandbox home, so they can put a reparse point where
the secret directory is expected, and the pathname version followed it
in an elevated process. The test asserts the refusal names the reparse
point and that nothing survives on the far side, since refusing while
still creating the file would leave the caller holding it.
The child environment starts as the invoking user's, and the deliberate
sandbox redirects only replace HOME, the temp variables and the per-tool
cache dirs. Everything identifying the account survived, so a command
running as the principal read USERPROFILE, APPDATA, LOCALAPPDATA,
HOMEDRIVE, HOMEPATH, USERNAME and USERDOMAIN describing the CALLER,
whose profile the principal deliberately cannot open. Native tools
resolve per-user state through exactly those, so they fail during
startup or quietly look somewhere they have no business reading.
Point them into the sandbox runtime tree, which is already granted to
the principal and already holds its caches, so the paths are writable by
construction. Naming the real Windows profile would need LoadUserProfile
to have run, and a variable pointing at a directory that does not exist
yet is a worse answer than one pointing somewhere usable.
Layered under the deliberate redirects rather than over them:
sandboxRuntimeEnvironment stays the single owner of HOME, TMPDIR, TMP
and TEMP, and a regression pins that so the two cannot drift into
setting the same variables from two places.
This is the environment half of the finding. Loading the principal's
profile and known folders is left until the launch mechanism is settled,
since LOGON_WITH_PROFILE would do it as a side effect.
CreateProcessAsUser exempts only a restricted version of the caller's
own primary token from SE_ASSIGNPRIMARYTOKEN_NAME, which is precisely
why the ordinary restricted-token path works while holding nothing
special. A principal token comes from LogonUser against a separate local
account, so the exemption does not apply and both that privilege and
SE_INCREASE_QUOTA_NAME are required. Nothing enabled or checked either,
and a token measured on an ordinary unelevated process holds neither, so
the failure arrived as a bare "Access is denied" from inside process
creation, before the command's executable was ever opened, and read as
the command being rejected.
Enable them where they are held, since present-but-disabled still fails
the access check and that is where an elevated administrator lands, and
refuse with the specific names and a way out where they are not.
Detected by ENUMERATING the token. AdjustTokenPrivileges reports an
unheld privilege by returning success with ERROR_NOT_ALL_ASSIGNED, which
this binding does not surface: it returns nil for SeTcbPrivilege on an
ordinary process. A check built on its error passed everywhere, which is
worse than no check, since it would call the sandbox ready in exactly
the case it cannot run.
This does not make the principal launchable. It makes the reason legible
while the launch mechanism itself is settled.
The elevated secret write resolved its path four separate times:
MkdirAll, an O_TRUNC open, SetNamedSecurityInfo by name, then WriteFile
by name. The sandbox home belongs to the invoking user, who is the party
this sandbox contains, so each resolution was a place to swap a
component. A symlink leaf lets an Administrator truncate a file of the
caller's choosing and then rewrite its DACL; a junction alone is enough
to plant the deterministic secret somewhere the caller controls.
Create the leaf relative to a pinned no-follow parent handle, refuse it
if it is a reparse point, and apply both the DACL and the bytes to that
handle. The name is never resolved again after the create.
The payload is now sealed before the file exists, so a failure there
leaves nothing on disk rather than an empty file for someone to race.
Cleanup on failure stays by name, which is safe in the direction that
matters: at worst it misses and leaves a locked-down file, never deletes
something it did not create.
A linked worktree or submodule has .git as a FILE holding a `gitdir:`
pointer, not a directory. The principal plan names .git/config and
.git/hooks and materializes both, and the Windows materializer gets
there by descending through .git as a directory. A regular file cannot
have children, so opted-in elevated setup aborted and the sandbox could
not be used in a worktree at all. Zero's own development worktrees are
this shape, which is how it went unnoticed.
Deny the pointer file itself there instead. That is the stronger
protection rather than a fallback: a principal able to rewrite `gitdir:`
repoints the repository at a control directory of its choosing, which
subsumes editing config or planting a hook. The real control directory
sits outside the write root, so nothing is inherited there and no
carveout is needed.
Decided by an Lstat rather than a lexical guess, since the layout is a
property of the checkout. An absent .git keeps the directory-shaped
carveouts so they are still created before git first runs.
A bulk edit dropped the separator, leaving the drive-relative
`C:workspace` where `C:\workspace` was meant. Windows resolves the
first well enough that the two halves still agreed; on Linux a backslash
is an ordinary character, so the command half derived no runtime
candidates at all and the marker carried two the command never saw.
The setup half no longer pre-augments either: BuildWindowsSandboxSetupArgs
folds the runtime roots in itself, and passing them in hid that from the
one test that covers it.
A profile with DenyRead drops WRITE_RESTRICTED, so Windows runs the
restricted-SID check over reads as well as writes. Read roots were
granted only to the principal's account SID, and the write jail removes
that SID from the restricting set on purpose, so reads passed the normal
check and matched nothing on the restricted one. The result was not a
narrower sandbox but an unusable one: no read root readable, including
the executable the command was trying to start.
Mint a read capability SID, grant it on every read root in the
capability plan, and carry it in the strict token's restricting set, so
the read allow-list and the restriction come from one value instead of
two that can drift.
Deny it on every DenyRead path too. The read roots begin at the
filesystem root, so without that the carveouts stay readable through the
new grant and the deny list stops meaning anything, which is the whole
reason the strict token is chosen. The no-write-roots case keeps its
ReadOnly deny as well: both SIDs the token can carry must be denied, not
only the newest.
Gated on DenyRead, which is what selects the strict token. Elsewhere
reads never reach the restricted check and the grant would be ACEs on
the filesystem root that buy nothing. Both halves decide from the
profile alone, so they cannot disagree about whether the entries exist.
Every sandboxed command failed marker validation with two plans of the
same size and different hashes:
marker ...\windows-setup.json has 12 entries, hash 76fda2032e66;
this command expects 12 entries, hash 4be1dbf8b642
The temp-derived runtime candidate reads os.TempDir(), and the sandbox
points TMPDIR, TMP and TEMP at its own runtime temp for everything it
launches. The command runner inherits that env, so when it derived the
candidate set it produced a root under the runtime tree while setup,
whose TEMP is untouched, produced one under the real temp. Same count,
different path, and no command could run.
A fingerprint both halves compare cannot be a function of the caller's
environment. Derive it only where TEMP is still the operator's: the
setup args builder, the Windows command plan, and doctor. The runner now
takes the profile it is handed, and commandConfig no longer re-derives
on behalf of whoever happens to call it.
The regression settles the profile first and redirects TEMP afterwards,
in that order, so it reproduces the mismatch against the old behaviour.
"permission roots or deny lists changed" told an operator that every
sandboxed command would now refuse to run, and nothing else. Not which
side is stale, not by how much, not even whether the marker belongs to
this workspace. Debugging it meant reading the source and guessing,
which is what happened.
Report the marker path, both entry counts and both hashes. The counts
separate the two shapes this takes: equal counts mean the same roots
spelled differently, unequal counts mean one side has roots the other
has never heard of.
Elevated setup stopped working entirely:
zero-windows-sandbox-setup.exe: windows ACL target does not exist:
...\AppData\Local\Temp\zero\runtime\v1\67b1b01412f588b9
Putting both runtime candidates into the capability plan added a write
root that nothing creates. The capability plan deliberately refuses to
materialize a write root, since an absent path is a typo or a stale
config and inventing the tree would grant write on a directory nobody
asked for, so the whole run fails on a path that is merely missing. Only
the selected root was ever created, and only on the principal path.
Create every candidate on the setup side, before the plan that grants
them is built. The regression walks the plan and fails on any granted
write root that does not exist, so the two halves cannot drift apart
again: today one function chooses the candidates and another creates
them, and nothing else couples them.
Found by the elevated end-to-end run, which is the only thing that
executes this path. No unit test reached it and CI does not run it.
The aliasing test appended a sentinel to the returned jail and then
ranged over the caller's slice looking for it. That can never fail:
append returns a new header, so the caller's length never grows and the
range never reaches the slot the write landed in. It passed against an
implementation that returns the caller's slice verbatim, which is the
exact thing it claims to rule out. Lint noticed the symptom as an
ineffectual assignment.
Assert through the backing array instead: element storage must not be
shared, and an append must not write into the caller's spare capacity.
Both fail against an aliasing implementation.
Also drop windowsSandboxDeterministicRuntimeRootPath, which lost its
last caller to the shared runtime-candidate helper and duplicated its
derivation.
A fresh setup marker rejected the very command it had just been written
for. Engine.run augments the profile with the selected runtime root
before the Windows runner sees it, but setup fingerprinted the bare
profile, so the extra write root changed the ACL plan hash and every
command on a restricted filesystem failed with "permission roots or
deny lists changed".
The runtime root also reached the principal ACL plan only. A principal
command runs on a WRITE_RESTRICTED token, where a write needs the
normal check and the restricting-SID check to both pass, so a root
carrying just the account ACE was still unwritable once the marker
agreed.
Derive the runtime candidates once and present them on both sides of
the setup protocol. Setup covers the cache-derived root and the
temp-derived fallback rather than whichever one it happened to select,
since selection is per process and a command that fell back would
otherwise land on an unprovisioned tree. Both are pure functions of the
workspace root, so setup can cover the set and a later process can
select from it.
Regressions fail without this: the marker one reproduces the exact
rejection, the capability-plan one shows the missing restricting-SID
grant.
probe-inside.txt is a leftover from the elevated end-to-end probe and was never
meant to be committed. A `git add -A` in the write-jail commit swept it in.
It is what the automated review's diff-hygiene check was failing on: the file
carries trailing whitespace, so `git diff --check` reported a blocker while
tests, build and smoke all passed.
Reported by jatmn. sandboxRuntimeRootFor falls back to a private tree when the
cache-derived runtime root would land inside the workspace, and that fallback
called os.MkdirTemp and cached the answer in a process-global map.
Separate processes therefore got separate answers. Elevated setup granted the
sandbox principal write access to the directory it created, and every later
__windows-command-runner process created a different one and pointed TMP,
GOCACHE, npm and the rest at it. Those directories are made by the calling user
and carry no ACE for the principal, so ordinary cache and temp writes failed
with a bare ACCESS_DENIED and nothing naming the sandbox. sandboxRuntimeRootFor
already documented that both callers must agree exactly; the fallback was the
branch where that could not hold.
It now hashes the workspace under os.TempDir, the same shape as the
cache-derived root, so every process reaches the same path with no shared state.
When even that lands inside the workspace it returns an error rather than
picking somewhere arbitrary: a runtime tree governed by the workspace's own
policy makes the sandbox's cache writes indistinguishable from the work it is
confining.
Two consequences worth naming.
It creates nothing now, so the split between deterministicSandboxRuntimeRoot and
the resolver is no longer about avoiding a side effect. The comments that
justified that split on those grounds were rewritten rather than left describing
behaviour the code no longer has.
Teardown can name the fallback tree for the first time. windowsPrincipalTeardown
Paths used the deterministic resolver precisely because the fallback was random
and unnameable, which meant an opted-out machine kept principal ACEs on whatever
tree the fallback had produced. It now goes through the shared resolver and
revokes what commands actually used.
Tests pin the property the defect turned on: a repeated call, which is what a
second process looks like with no shared state, must return the same root. The
old implementation could not have passed that, since only the in-process map
made repeat calls agree. Also pinned: per-workspace separation, that naming the
tree creates nothing so teardown leaves no directory behind, and that the root
stays outside the workspace.
Reported by jatmn. The principal path appended the account's own SID to the
restricting-SID list before building a WRITE_RESTRICTED token.
That token allows a write only when BOTH the normal token and the restricting
list allow it. The account SID is already enabled in the normal token, so
listing it as a restricting SID makes the second check a formality for anything
granted to that account: every path carrying a direct principal ACE passes both
halves and is writable wherever it sits. The principal's own profile directory,
which Windows creates on first logon with exactly such an ACE, is outside every
configured write root and was writable for that reason.
It is the same defect as the World SID in the restricted list (#865), with the
account SID in place of Everyone. A SID already carried by the normal token
cannot also restrict it.
The previous reasoning, recorded in the comment this replaces, was that the ACL
plan grants the workspace to the principal SID so removing it would jail the
principal out of its own tree. That is not so, because setup applies
BuildWindowsACLPlan on every path, not only the restricted-token one, so each
configured write root already carries a capability ACE as well as the principal
ACE. Confining to the capability SIDs therefore restores the intersection the
jail is supposed to be: a write root satisfies the normal token through its
principal ACE and the restriction through its capability ACE, while a path
holding only a principal ACE now fails the restricted check.
The account SID is passed to windowsPrincipalJailSIDs and excluded there rather
than simply not passed. Naming it makes the exclusion the function's contract
instead of an omission a later edit could undo silently, and it also strips the
SID should it ever arrive through the capability list, which is the route the
World SID took.
On the test jatmn asked for: the existing jail test grants WinBuiltinGuestsSid,
a GROUP, so it exercised a configuration the product never ships and could not
have caught this. The new test plants the account SID INSIDE the capability list
and asserts it is gone, which is deliberately falsifiable: a list that never
contained it would pass against any implementation, including one that appends
the SID straight back. Verified by mutation, disabling the filter fails it with
the reported symptom. A second test pins that the capability SIDs survive, so
the fix cannot degenerate into jailing the principal out of its workspace.
Still outstanding from the same review and not addressed here: the runtime-root
fallback that hands setup and each command runner a different directory, and the
elevated end-to-end evidence.
Reported by anandh8x as P2 #3. ensureWindowsSandboxGroup treated NERR_GroupExists
and ERROR_ALIAS_EXISTS as plain success, so any local group that happened to
carry our name was adopted. Its members, and every grant already keyed to it,
silently became part of the sandbox's identity.
A name is not proof of provenance, which is the same reasoning
windowsSandboxUserIsManaged already applies to an ACCOUNT of our name. The group
half was missing. An unprivileged user cannot create a local group, but an
administrator, an installer or an earlier build can, and the principal would
then inherit whatever it grants.
An existing group is now adopted only when it carries the managed comment, read
back with NetLocalGroupGetInfo. Anything else is refused by name rather than
adopted, renamed around or deleted: removing somebody else's group would be
destructive, and provisioning into it is the hole being closed.
The decision is split from the syscall into resolveWindowsSandboxGroupAdd so it
can be tested without Administrator and without leaving a real local group on
the machine running the suite. Tests cover both "already exists" statuses,
refusal for a foreign group, adoption of our own so re-running setup still
converges, no ownership probe when we just created it ourselves, an unreadable
probe surfacing rather than being guessed either way, and a real API failure
still failing.
Also rebased onto main, 17 commits behind, under the fresh-base rule. All 51
commits replayed with no conflicts.
Three corrections, the first to my own previous commit.
Making the opt-out retirement failure fatal was too broad.
removeWindowsSandboxPrincipalForSetup deliberately continues past a secret file
owned by another administrator's setup, reporting it at the end through
errors.Join, so that one unlinkable file cannot strand the account, its logon
rights and its ACEs. The account is already gone when that error is returned.
Treating it as fatal made `zero sandbox setup` fail permanently on such a
machine and rolled back its ACL work, taking the DEFAULT restricted-token
sandbox down over inert residue.
The rule is now the narrower one that was always meant: fatal when the ACCOUNT
survived, because that is exactly what makes an opted-out marker a lie. Asked of
the account rather than inferred from the error, since the error is a join whose
parts are not separable at the call site, and the lookup fails closed so an
undeterminable state counts as still installed. A retirement that left something
inert behind still completes, and still names what it left.
Second, assertWindowsSetupRunsAsCaller cannot fire on any path Zero takes, and
said otherwise. Zero never elevates the helper: runSandboxSetupHelper is a plain
exec.Command and runWindowsSandboxSetup refuses unless already elevated, so the
operator supplies elevation by opening an elevated terminal and BOTH halves run
there. The caller SID is resolved from the process that becomes the helper, so
the two are equal by construction. The guard is kept, because it is a
correctness assertion that becomes load-bearing the moment a ShellExecute
"runas" path is added and already covers a helper invoked directly, but its doc
now says plainly what it does not do. The residual hazard has a different shape
and is loud: elevating as another administrator provisions into that account's
sandbox home, and the operator's own session finds no marker and is told to run
setup.
Third, parseSandboxExecArgs claimed to scan for the separator up front. Every
branch of its loop body returned, so it only ever examined index 0 and the scan
did not exist. Rewritten as the straight-line decision it actually was, which is
also the rule we want: only the first token is the wrapper's.
sandbox exec also built its engine without SensitiveEnvKeys, so a key named by
apiKeyEnv in the user's config was scrubbed for every sandboxed tool call and
handed to the one command whose purpose is to reproduce that environment.
Tests: the opt-out branch now covers both sides of the rule, and
parseSandboxExecArgs gets the coverage it never had, including the reported
`-- cmd --help` case and an invariant that the returned command is always a
trailing slice of the input.
Two failures on the elevated setup path, both reported by anandh8x.
The first is caller identity. windowsSandboxPrincipalKey sampled the CURRENT
process token, and elevated setup is not the caller's process: under
over-the-shoulder UAC or `runas` it belongs to whichever administrator typed the
credentials. Setup therefore provisioned an account, a secret, an ACL ledger and
a set of grants named after that administrator, while every later command
derived the ordinary user's key, found nothing, and fell back to the restricted
token. The admin-keyed leftovers were unreachable and nothing would ever reclaim
them.
The caller's SID now crosses the UAC boundary in the setup args, exactly as the
principal opt-in already does and for exactly the same reason, and the key is
derived from it. That fixes everything merely NAMED after the invoking user.
It cannot fix the secret. CryptProtectData derives its key from the user that
stores the blob, so a password sealed by another administrator could not be
unsealed by the caller no matter how the account were named. Provisioning is
refused up front in that case, before an account or a grant exists, with a
message that says how to clear it. The refusal is scoped to the principal
opt-in: the default restricted-token sandbox stores no secret and works fine
across that boundary.
The second is the opt-out path. Retiring an existing principal could fail, and
setup printed the error and carried on to write an opted-out marker and exit 0.
Teardown being idempotent was the argument for continuing, but it is the reason
this must fail instead: a machine with nothing to retire passes straight
through, so an error here means a real account, secret, logon right, ACE set and
ledger are still installed, and the marker now claims none of them exist. Setup
returns non-zero and rolls back its ACL work.
runWindowsSandboxSetup had no coverage at all, since every step wants an
elevated token, a machine-global mutex, real DACLs, the WFP engine or a local
account. The external effects are now seams in the idiom the package already
used for two of them, and the tests drive the step order and each failure path,
including the marker never being written.
Four of the five findings were real. Taking them in order of what they cost.
Cross-user principal collision (P1). The account name was keyed on the workspace
alone while its DPAPI secret and ACL ledger both live under the invoking user's
sandbox home, so two Windows users sharing one workspace derived the same account
with private bookkeeping. The second user found no ledger of their own and either
retired the first user's working principal or adopted it and rotated its password
while storing only their own secret; either way the first user's marker still
validated and their next command could not log on.
windowsSandboxPrincipalKey now folds the invoking user's SID into the key. The
setup LOCK deliberately keeps the workspace-only key: two users setting up one
shared workspace still write DACLs on the same paths, so they must keep
serializing even though they now provision separate accounts. Splitting the two
keys is the point, and there is a test asserting they differ.
Ledger not restored on rollback (P1). The narrowed ledger is written only after
everything succeeds, but the rollback closure restored the old ACEs and left it
narrowed. A later setup or teardown reads the ledger to decide what to revoke, so
those restored paths would never be revisited: access outside the current policy
held by an account nothing knows to clean up. The closure now writes back the
union recorded before any DACL changed. Over-recording is the safe direction.
sandbox exec skipped the production planning path (P2). It built a SandboxManager
directly, so it never reached prepareSandboxRuntime and ran without the runtime
write root that a real sandboxed command gets. It now goes through
Engine.BuildCommandPlan. This matters beyond tidiness: the command exists to
prove enforcement, and it was proving a path no tool call takes.
Plan resources leaked (P2). No plan.Cleanup(), so every invocation left a
/tmp/zero-sandbox-report-* behind on Linux. Deferred immediately after the build.
The `--` separator (P2). The reported case, `exec -- cmd --help`, already worked:
the loop hits the separator first and returns. The real break was the tolerated
separator-less form, where `exec mycmd --help` printed Zero's help and never ran
mycmd. The separator is now located before any help flag is interpreted, and help
is only read from the wrapper's own arguments.
Two existing ledger tests seeded state under a username derived from the
workspace key and had to move to the principal key. Worth noting for anyone
running this branch: an account provisioned before this commit will not be found
by the new key, and setup will retire it as unrecorded. That is correct behaviour
for an unmerged branch with no released accounts, but it is not a no-op locally.
anandh8x's finding #3, the last of the twelve.
Setup is a transaction, not a write. It rotates the principal's password,
stores the secret, read-modify-writes the ACL ledger, installs WFP filters and
writes the marker. Two setups for the same workspace at once interleave those
steps: the account ends up with one process's password while the stored secret
is the other's, so every later command fails to log on, and the ledger's
read-modify-write silently loses a path set so ACEs that should have been
revoked stay on disk with nothing recording them.
Atomic individual writes do not address this. Each step being all-or-nothing is
orthogonal to two processes interleaving BETWEEN steps, which is the actual
failure, so the lock spans from the elevation check through the marker write.
Nothing to port here: the reference implementation has the identical bug. Its
password rotation and secret write hold no lock, its named mutex guards only an
unrelated read-ACL-only mode and is machine-singleton, and the singleflight
that looks like a fix is process-local.
Decisions worth the reviewer's attention.
Global rather than Local namespace, because the protected state is not
session-scoped: the local account, its LSA logon rights and the WFP filters are
machine state, so setups in different logon sessions must still exclude each
other. Per-workspace rather than machine-wide, because every artifact the lock
protects is per-workspace and a global lock would serialize unrelated setups.
The DACL is Administrators and SYSTEM only, protected. A sandbox principal able
to open this object could hold it and stall every future setup, or create it
first and squat the name so real setup believes it holds a lock it does not.
The OS thread is pinned. Win32 mutex ownership is thread-affine, so a goroutine
that migrated between the wait and the release would call ReleaseMutex from a
thread that does not own the object, leaving the mutex held until process exit,
which is a deadlock for every later setup on the machine.
WAIT_ABANDONED is treated as acquired and reported. The previous holder died
inside the transaction, so the machine may be half set up; setup is idempotent
and re-running it is the repair, whereas refusing would leave no way forward.
A bounded wait rather than skip-and-succeed, because skipping here means not
setting the sandbox up at all.
On the tests: the production DACL means an unelevated process cannot open an
existing lock, so the contention test could not reach the mutex wait anywhere
that matters, including CI. The SDDL is therefore a var the wait tests relax
for themselves, and a separate test pins the production value so loosening it
takes an explicit change. Writing the test first is what surfaced this at all.
anandh8x's findings #6 and #8. They are one commit because #6 calls straight
into the path #8 breaks, so fixing #6 alone would have shipped an opt-out that
aborts on exactly the machines #8 describes.
#8 first. readWindowsSandboxSecret already treats permission-denied as
unavailability rather than breakage, and documents why: an operator who
elevated with a separate administrative account ends up with a secret their
ordinary account cannot open. Removal did not agree. os.Remove returning a
permission error aborted teardown at its very first step, so the account, its
logon rights, its ACEs and its ledger all stayed installed because one file
could not be unlinked. The operator asked for removal, got an error, and kept a
working principal.
Removal now classifies that case distinctly and teardown carries on, reporting
the leftover secret at the end rather than swallowing it. An ordinary removal
failure is still fatal, and there is a test for the difference: misclassifying
one as the other would turn a real failure into a shrug.
#6. Opting out has to actually retire the principal, because that is what we
tell people it does. ValidateWindowsSandboxSetupMarker sends an operator here
in as many words. There was no opt-out branch at all, so the marker flipped to
opted-out while everything stayed exactly where it was: the instruction was a
lie, and the leftovers were invisible, because nothing afterwards looks for a
principal it believes was never provisioned.
Setup now runs teardown on the opt-out path. Not fatal if it does not complete:
teardown is idempotent and a machine that never had a principal passes straight
through, so a failure means genuine residue rather than a missing account. Say
so and finish a setup whose sandbox is otherwise fine.
anandh8x's findings #7 and #10.
#7. windowsSandboxPrincipalEligible stands the principal down under
network-deny, which is the DEFAULT policy, so an operator who set the opt-in to
confine reads silently gets the same-user restricted token and no read
confinement at all. Nothing told them. The runner cannot: it is re-exec'd per
command, so the notice would land on the stderr of essentially every tool call,
and it is not per-command actionable anyway. The setup marker validates happily
because setup really did provision the account. The runtime comment already
said this belonged on `zero doctor`, and doctor never carried it.
Doctor now warns, and the rule has one definition rather than two. The
eligibility check asks the same exported predicate doctor does, so the two
cannot drift into disagreeing about whether a principal is in play. Drift here
would mean doctor telling someone reads are confined while commands run on the
restricted token, which is worse than saying nothing.
A warning rather than a failure: commands work and the network is still
enforced. What is wrong is the operator's picture of what they have.
#10. windowsSandboxUserIsManaged has always claimed a legacy account is
"adopted and its comment rewritten on the way through", and it never was:
provisioning only ever set the password (USER_INFO_1003 is documented in this
file as the password-only form). So a pre-key account stayed permanently
unattributable, and every later run had to keep accepting the bare comment to
avoid orphaning it.
Adoption now stamps the workspace key via USER_INFO_1007, which ends that and
lets the bare form eventually be retired rather than accepted forever. Best
effort: the account is adopted and working either way, and failing provisioning
over a cosmetic stamp would strand a usable sandbox for bookkeeping. Reported
rather than swallowed, so a run that keeps re-adopting the same legacy account
is visible.
The legacy-comment probe is a separate function rather than folded into
windowsSandboxUserIsManaged, so that keeps its (bool, error) shape and the test
seam around it does not have to change.
anandh8x's finding #4. The marker hashed BuildWindowsACLPlan, the
capability-SID plan, while principal grants are built separately by
buildWindowsPrincipalACLPlan from the same profile. So the marker was blind to
them: narrowing or removing a principal read root left setup looking current
and the old AllowRead ACE sitting on disk. Narrowing a policy has to be able
to take access away, and this was the one path where it could not.
The fingerprint hashes the plan with a fixed placeholder trustee rather than
the real principal SID. The account is recreated with a fresh SID whenever it
is reprovisioned, so hashing the real one would move the fingerprint on every
rebuild even when the granted paths were identical, and every command would
then rerun setup. What must invalidate the marker is the set of paths and
actions, which is what this captures.
Empty when the principal backend is not opted into, so the default install's
marker does not churn. Schema version goes 5 to 6 so existing markers are
treated as stale rather than read as having no principal grants.
Validation refuses a mismatch with the action to take, since the remedy is
re-running elevated setup so the stale grants are actually revoked rather than
merely re-planned.
Until now the sandbox could only be exercised through a full agent turn with a
model in the loop. `zero sandbox policy` reports the posture it would take and
`zero sandbox check` evaluates a hypothetical decision, but nothing ran a
command and let anyone look at what happened on disk afterwards.
That is the root of the verification gap on this branch. Enforcement is covered
almost entirely by tests asserting the shape of an ACL plan, and almost not at
all by tests asserting that a write was refused. The two are not the same
thing, and this branch has already produced the proof: the .git rename guard
was emitted correctly by the planner and silently dropped by the applier, and
four tests covering the plan passed while the ACE was absent from the
directory.
It goes through SandboxManager.BuildCommandPlan, the same path a shell tool
takes, so what it demonstrates is what users get rather than what a test double
does. Backend, enforcement level and workspace are written to stderr before the
command runs, and a downgrade is printed explicitly, so a harness can assert
the sandbox was actually engaged instead of passing because it quietly stood
down. Exit status is the command's own, which a harness asserting a refusal
needs.
Everything after `--` is the command, so its flags are never parsed as ours.
Verified by hand on Windows: a write inside the workspace succeeds and the file
exists; the same write to C:\Windows\Temp fails with UnauthorizedAccessException
and the file is not created. Both halves matter, since a command can fail for
an unrelated reason while the write still lands.
This is the prerequisite for a real smoke harness, which is the actual goal.
Found while testing the default sandbox path, not the principal path this PR
is about. One write root that the current user cannot re-DACL fails the entire
unelevated ACL plan, and the success marker is only recorded on success, so
the identical failure repeats on every later command until that root leaves
the plan. The workspace is effectively unusable in the meantime.
Refusing to run is correct and is left alone: without those ACEs there is no
write jail, so continuing would run a command that believes it is sandboxed
and is not. The defect was the diagnosis. Every failure got the same guess,
"the workspace may be on a filesystem the current user does not own", and
recommended elevated setup, which does nothing when the real problem is a
system directory sitting in the root set.
An access denial now names the exact path, says the sandbox cannot enforce a
boundary there, points at TEMP and TMP as the usual way such a path gets in,
and states plainly that elevated setup will NOT help. Other failures keep the
old message.
The extractor reads the path back out of an error string produced two
functions away, which the compiler cannot check, so a test drives the real
producer rather than hand-writing the message: if openWindowsACLTarget rewords
its error the test fails instead of the diagnostic quietly going blank. That
test earned itself immediately by catching the first version splitting on the
first colon and returning "C", since on Windows the path opens with a drive
colon. It splits on colon-space now, which a drive colon can never be.
The helper sits behind the Windows build tag beside its only caller, for the
same reason windowsACLPlanPaths was moved earlier in this branch.
Cross-checking against another Windows sandbox implementation showed it joins
its account to the built-in Users group explicitly, and then verifies at apply
time whether Users already hold read before granting. Ours gets that membership
implicitly from NetUserAdd with USER_PRIV_USER and asserts the consequence in a
comment. Same conclusion, weaker footing, so the comment now says where the
membership comes from and that a hardened image which strips Users read would
need an explicit bounded read set instead.
Two of anandh8x's findings on this PR.
The read grant. permissionProfileReadRoots seeds its list with the bare
separator, because the workspace-write posture is read-all with a write jail.
That costs nothing for the capability backend, whose child runs as the caller
and could read those paths anyway. For a principal it became a real,
persistent, inheritable allow-read ACE for a separate local account at the root
of the current drive, reaching every directory that does not block inheritance.
Verified against the shipped profile rather than argued: with the filter
removed the plan emits AllowRead on the drive root.
Dropping it does not remove the reads a principal needs. It is a member of
Users, and the machine's own ACLs already grant Users read on the system and
program directories. What the grant added was read access to the places Users
are deliberately kept out of, which is the opposite of what a sandbox is for.
The volume root is detected structurally, since filepath.Dir of a root is that
same root, so drive-qualified paths, a bare separator and UNC roots are all
covered on either build host.
The teardown. removeWindowsSandboxPrincipalForSetup discarded the result of
revokeWindowsPrincipalACEs, deleted the account, deleted the ledger and
returned success. A failed revocation therefore left ACEs naming that SID on
the user's tree while the only record of which paths they sat on was removed
moments later: residue nothing could find again.
The existing reasoning for not failing hard was right and is kept, since
refusing to remove the account would strand the principal and its logon rights
permanently. So teardown still completes, but the error is now remembered, the
ledger is kept when revocation failed, and the returned error says the ledger
was kept. A record that outlives its principal is a smaller problem than
unfindable residue.
The new read-root test is built from the production profile, and skips loudly
rather than passing silently if that profile ever stops carrying a volume root.
It also asserts the workspace itself stays reachable, so the fix cannot trade a
real grant for a broken sandbox.
Found by gnanam1990 and anandh8x independently, and confirmed on disk before
fixing: the deny-delete ACE that stops a principal replacing .git was silently
absent on exactly the workspace shape that matters most.
.git deliberately carries no Materialize, because creating an empty one breaks
`git init`. Groups are applied in ascending path order, so the .git group ran
while .git did not yet exist, was skipped as a non-materializing missing
target, and nothing revisited it when the .git\config carveout created .git as
its parent chain moments later. windowsACLGroupRequiresExistingTarget did not
catch it either, since it only treats AllowWrite as requiring an existing
target. Net effect: the principal could rename .git aside and recreate it
without the config and hooks carveouts, which is the escape the guard exists
to stop.
Groups skipped for a missing target are now retried once after the pass, since
a later group can create what an earlier one needed. Groups that are genuinely
absent simply skip again. Ordering is otherwise untouched, so the reverse-order
unwind that rollback depends on still holds.
Fixing the gate to demand an existing target for deny-delete would have been
wrong: it turns a fresh clone into a setup failure. Materializing .git is
likewise ruled out by git init.
The four existing guard tests assert the mask and what the planner emits, and
all four pass with the guard absent from disk, which is why a green suite said
nothing. The new tests apply the plan and read the actual DACL back through
SDDL. Verified the fresh-workspace case fails without this change and passes
with it, while the already-has-.git case passes either way, so the new coverage
is pinned to the defect rather than to the code.
Also moves windowsACLPlanPaths behind the Windows build tag. Every caller is
Windows-tagged, so defining it in the portable file made it dead code on Linux
and macOS and failed make lint-static, as anandh8x reported.
ProtectedMetadataNames is documented as names, but it is joined onto the write
root to place a deny ACE and to materialize the directory that ACE protects. A
value containing ".." or a separator puts both outside the workspace: the deny
lands on a directory the sandbox does not own, and elevated setup creates it
there. Raised by CodeRabbit on this PR.
Not reachable today, since the only caller passes a package constant. It is
guarded so that stays true if a future caller sources these from config, which
is the kind of change that would not obviously be a security decision.
The component check moves to the portable file so both users share it. The
handle-relative primitives need it because NtCreateFile resolves a relative
name containing separators the ordinary way, following any junction inside it;
the plan builder needs it for the escape above. Separators are matched
explicitly rather than through filepath.Base, because these are Windows paths
whatever the build host is, and on Linux filepath.Base leaves a
backslash-joined name untouched and would wave it through.
Rejection tests are in the portable test file, so they run on all three
platforms alongside the rest of the plan-builder coverage.
Closes the two path-swap findings on this PR. Both had the same cause: an
object was addressed by a pathname that the kernel re-resolves at the moment
the call runs, so a workspace owner could change what that name meant in the
gap between checking it and using it. Junctions need no privilege, so this was
reachable by exactly the unprivileged user the sandbox exists to contain.
Materialization verified a component, closed the handle, then handed the same
string to os.Mkdir. Swapping the verified ancestor for a junction inside that
gap got the component created outside the approved tree, as Administrator, and
verifying again afterwards cannot un-create it. It now opens the deepest
existing ancestor once and creates each missing component relative to the
handle of the level above, so the tree it descends is pinned to objects rather
than named by strings.
Rollback called os.RemoveAll on the target pathname, so an ancestor swapped to
a junction after creation sent a recursive elevated delete into an unrelated
tree. It now records what it created as single components under an anchor and
unwinds them handle-relative, deepest first. It also removes the whole chain
rather than only the final component, which the old comment claimed was fixed
and was not, and it removes only what the kernel confirmed this run created,
so a racing creator no longer gets its directory deleted on teardown.
Three things this turned up that were not in the original findings:
The delete primitive used FILE_DELETE_ON_CLOSE, which defers the removal to
cleanup and reports a non-empty directory to nobody: open succeeded, close
succeeded, directory still there. Wired into rollback that would have been
worse than the os.RemoveAll it replaced, since rollback would report success
while leaving state on disk. It now sets the disposition explicitly, so
STATUS_DIRECTORY_NOT_EMPTY comes back to the caller. Residue is preferable to
recursing through a path someone else controls; lying about it is not.
The child create never inspected the handle it returned, so an existing
junction was handed back as the next parent in the walk and every deeper
create landed on the far side of it. Component names are now validated as
single components too: NtCreateFile resolves a relative name containing
separators the ordinary way, which would have walked straight through an
intermediate junction.
Rollback re-opens the anchor by pathname and checks its volume and file index
against what materialization saw, because replacing a directory with another
real directory of the same name needs no reparse point at all and passes every
no-follow check there is. Holding the handle instead would be stronger, but
three call sites deliberately discard the rollback closure and would leak.
Testing. makeWindowsACLDirChainNoFollow carries a seam that fires between
verifying the anchor and creating anything, because a race nobody can trigger
on demand is not a regression test. The existing junction test plants its
junction before the walk starts and so never reached this. The new test has a
control arm that performs the identical swap and creates by pathname, and
asserts the object does escape, so the fixed arm proves the hole was open
rather than proving some code ran. Also covers the file target, rollback
through the closure callers actually hold, and the non-empty case that the
old empty-directory-only delete test hid.
Validation now runs before the filesystem is touched, so a malformed entry no
longer creates a chain and then fails.
The two rename-guard tests built their expectations from a hardcoded
C:\work\repo, but buildWindowsPrincipalACLPlan normalizes every write
root before it names an ACE. On Windows that root is already absolute,
so normalizing is a no-op and both tests passed locally. On Linux and
macOS it is not absolute, so the plan named a different path and both
tests failed. The plan builder is portable code with no build tag, so
these tests run on every platform.
Use an OS-neutral root and normalize the expected path the same way the
plan does, which is what the other untagged test in this package
already does. The Windows-only assertions about the ACE mask and its
inheritance stay where they are, in the file that is tagged for it.
Groundwork for the two remaining materialization P1s. No call sites yet;
the walk and the rollback move onto these next.
Every pathname-based call re-resolves the whole path inside the kernel
when it runs, so verifying a component and then creating through it are
two separate resolutions of the same string. A workspace owner can swap
an ancestor for a junction in that gap. Demonstrated, not theorised: with
the same swap performed between verify and create, os.Mkdir put the new
directory OUTSIDE the approved tree ("landed inside approved tree: false,
ESCAPED outside approved tree: true"), and the post-create check cannot
un-create it.
A handle pins the object rather than the name, so a create resolved
against it cannot be redirected however the path is later rearranged.
os.Mkdir, os.OpenFile and os.RemoveAll are pathname-based by construction
with no relative form on Windows, hence NtCreateFile with
OBJECT_ATTRIBUTES.RootDirectory. x/sys/windows already exposes every
piece, so this adds no hand-rolled syscall bindings.
createWindowsACLChildDirectory reports whether it created or opened,
because rollback must delete only what it made; removing a directory that
already existed would destroy a user's data over an unrelated failure.
deleteWindowsACLChildDirectory is the counterpart that rollback needs:
os.RemoveAll on a pathname whose ancestor has since become a junction
recurses outside the workspace.
The fix also makes the race testable. Against the old code the swap had
to be threaded into the middle of a function; here it is three ordinary
lines between an open and a create, because the handle is held across
them. Both tests perform the real swap and assert the operation stayed
inside the verified directory, and the delete test leaves a bystander
under the decoy whose survival proves the delete never resolved by path.
Refs #808.
The write-denied carveouts guarding .git are attached to .git/config and
.git/hooks as OBJECTS. The workspace allow grant is inheritable and
carries DELETE, and nothing denied DELETE on .git itself, so a principal
could rename .git aside, recreate it, and create fresh config and hooks
that inherit the allow with no deny of their own. That restores
credential.helper and core.hooksPath, and with them arbitrary code
execution on the next git command.
.git could not simply join sandboxFullyProtectedMetadataNames next to
.zero and .agents: that list emits DenyWrite, whose mask includes
FILE_GENERIC_WRITE, and git has to write index, objects and refs. Its
absence from that list was correct and was also the hole.
Add WindowsACLDenyDelete: DELETE, WRITE_DAC and WRITE_OWNER only.
Renaming a directory needs DELETE on that directory, so denying it is
what closes the replacement. WRITE_DAC and WRITE_OWNER come along because
a guard the principal can rewrite is not a guard. FILE_GENERIC_WRITE and
FILE_DELETE_CHILD stay out so git keeps working.
The ACE is applied uninherited, which is why the narrow mask is safe:
inherited onto .git's children it would deny DELETE on every file inside
and git could not remove a lock file or a ref. windowsExplicitAccessEntries
hardcoded SUB_CONTAINERS_AND_OBJECTS_INHERIT for every directory entry,
so inheritance is now decided per action.
The entry is not materialized. git creates .git, and an empty one made by
setup breaks git init.
Reported by @jatmn on #808.
The two new record tests failed on the Windows runner and passed
everywhere else. The plan builder runs every root through
normalizeProfilePath, whose EvalSymlinks expands the 8.3 short name
GitHub's runners hand out for TEMP, so the record held
C:\Users\runneradmin\... while t.TempDir() had returned
C:\Users\RUNNER~1\... and an EqualFold on the raw spellings called two
spellings of one directory different paths.
Production was never affected: the recorded paths and the newly planned
paths both go through that same normalization, so setup and teardown agree
with each other. This was only the test being naive about what "same path"
means on Windows, and a developer whose volume has 8.3 name generation
disabled cannot reproduce it — which is how it shipped.
The comparison now normalizes both sides through normalizeProfilePath and
keys them with windowsCapabilityPathKey, which is what the ACL plans
themselves use. Re-checked against the mutation that matters: reverting the
revocation to the new plan's paths alone still fails
TestReSetupRevokesARootTheNarrowedPolicyDropped, so the looser-looking
comparison has not defanged the assertion.
applyWindowsPrincipalACLs revoked the trustee only from the paths of the
plan it was about to apply. A root that LEFT the policy is by definition
absent from that plan, so its ACE survived the re-setup marker validation
forces and the principal kept write access the current policy no longer
grants -- the sandbox widened as a result of being tightened. Teardown
repeated the same current-plan-only calculation, so retiring the principal
cleaned every path except that one, and then deleted the account, leaving
the ACE naming a SID nothing could resolve.
Nothing on Windows can answer "which paths hold an ACE for this SID"
without walking every volume, so the grants are now written down as they
are made: a per-principal record beside the secret, keyed the same way
because one sandbox home serves every workspace on the machine. Setup
revokes over the union of the recorded paths and the new plan's; teardown
revokes over the same union.
The record is written as that union BEFORE any DACL changes and narrowed
to the granted set after, so a crash in between leaves a superset rather
than a record missing paths the run granted. A superset is the safe
direction: revoking a path that holds no ACE for the trustee is a no-op.
The interesting case is a principal from an earlier setup with no record.
Proceeding with an empty prior set would be the fail-open, on the one path
where the prior set is not empty but unenumerable. Setup retires that
account before provisioning instead: Windows never reuses a deleted local
account's RID, so whatever ACEs cannot be found end up naming a principal
that no longer exists, and the SID minted next is one no DACL on the
machine can already carry. It needs no new operator action, which matters
-- there is no `zero sandbox teardown` to send anyone to.
Every guard is mutation-checked. Reverting the union to the new plan's
paths fails the end-to-end test against real DACLs with "the principal
kept its grant on a root the narrowed policy removed"; moving the record
after the grant, dropping the retirement, reverting teardown, and trusting
an unknown schema each turn a different test red.
Two existing tests move to the new applyWindowsPrincipalACLs signature.
Elevated setup and the commands that later use a principal each read
ZERO_WINDOWS_SANDBOX_IDENTITY from their own process environment, and setup runs
in a separate, UAC-elevated process whose environment is not the caller's.
commandConfig() returned a nil Env, so the gate in windows_setup_windows.go fell
through to os.Getenv in the elevated helper. The two halves could disagree and
nothing detected it.
Direction B is the dangerous one: command opted in, setup did not. Marker
validation passed (the marker recorded nothing about the opt-in), the principal
lookup then declined with a nil error, and the command ran on the same-user
restricted token — which by its own comment does not confine reads — while the
operator believed an account boundary was isolating it. No warning fired.
Direction A left an orphaned account holding batch-logon rights and workspace
ACEs that teardown never retires, because teardown sits inside the same opt-in
branch.
The opt-in is now resolved in the shell the user typed `zero sandbox setup` into
and serialized across the UAC boundary as --sandbox-principal 0|1. The marker
records it (schema 4 -> 5) and validation refuses on mismatch, before the ACL and
network checks. Both directions refuse rather than fall back silently: an
unreadable value is rejected outright, since guessing "off" would provision a
weaker sandbox than asked for and report success.
PrincipalOptIn is a *bool, not a bool. Unset means "consult the environment"
rather than "opted out", so the existing smoke-test callers that do not set it
keep working instead of serializing --sandbox-principal 0 while the command half
still consults os.Getenv.
Dropped the deny-mode fallback warning added alongside this. Announcing it looked
right, since deny is the default mode and an opted-in operator therefore never
gets a principal for ordinary commands. But this runner is re-exec'd per command,
so its sync.Once is once per COMMAND: the notice would print on nearly every tool
call, and noise that repeats gets filtered rather than acted on. It is also not
actionable per command. `zero doctor` carries the opt-in now and is the right
surface for a standing configuration fact. The deny-mode behaviour stays pinned
by TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied.
Reported by jatmn on #808.
The principal branch handed the raw LogonUser token to CreateProcessAsUser. That
token is a full token for the account, so the sandboxed child kept every write
its ambient memberships grant. The ACL plan can add grants and denies at named
paths, but it cannot revoke what BUILTIN\Users, Authenticated Users or
NT AUTHORITY\BATCH already allow elsewhere — so an opted-in command whose
profile permitted writes only to the workspace and runtime roots could still
write C:\Users\Public\Documents, which grants BATCH modify and which a batch
logon therefore satisfies.
The principal now gets its own identity AND the restricted token, not one or the
other: reads stay confined by its ACEs, writes by the restricted-SID check.
The principal's own SID joins the capability SIDs deliberately.
applyWindowsPrincipalACLs grants the workspace to identity.SID rather than to a
capability SID, so omitting it would leave the workspace grant matching nothing
in the restricted list — a jail that locks out the inmate and no one else. The
SID is read back from the token itself rather than threaded through the call, so
it cannot drift from the identity actually running.
Not fixed here, and separate from this finding: worldSID is unconditionally in
the restricted-SID list, so any path whose DACL grants Everyone still satisfies
the restricted check on both this path and the pre-existing fallback. That
predates the principal work and is raised with the maintainers separately.
Reported by jatmn on #808.
FILE_FLAG_OPEN_REPARSE_POINT only stops the FINAL path component being followed.
materializeWindowsACLTarget built its target with os.MkdirAll and os.OpenFile on
the pathname, and both resolve ancestors — so the ancestor reparse check on the
subsequent no-follow re-open ran only after the objects already existed.
An ordinary workspace owner needs no privilege to create a junction. Turning
.git into one before elevated setup runs, with config absent, had setup create
the target at a location of the attacker's choosing as Administrator. Rejecting
it afterwards does not undo that, and the failure path removes only the final
component, so every intermediate directory MkdirAll created outside the
workspace survived permanently.
Creation now goes through makeWindowsACLDirChainNoFollow, which walks up to the
deepest existing ancestor and verifies it no-follow first. One check suffices for
the whole chain above it because GetFinalPathNameByHandle answers for the entire
resolved path. Missing components are then created one at a time, each
re-verified immediately after creation, so a component swapped for a junction
mid-walk is caught before anything lands underneath it.
Taken over the relative-handle NtCreateFile route because x/sys/windows offers no
ergonomic relative-create primitive, and this leaves a window of one component
with an immediate post-create check rather than create-everything-then-verify.
Reported by jatmn on #808.
The provisioning rollback removes the stored secret when this run created the
account or rotated an adopted one's password, because in both cases what is on
disk cannot authenticate and absent beats stale: the command path treats a
missing secret as "not provisioned" and falls back, while a stale one fails the
logon and reports a broken sandbox.
That removal ignored its own error. When it failed, the invariant it exists to
keep was not restored — a credential for a password that no longer works stayed
on disk — and setup said nothing. The operator met a provisioned-but-unusable
principal on the next command instead of hearing it from the run that broke it.
undo now returns that one error and the five failure paths join it onto the
error they were already returning, so the original cause and the cleanup failure
both surface. The message names the file and what to do about it. The other undo
steps still swallow: they leave residue, while this one leaves a credential.
Reported by jatmn on #808.
windowsPrincipalTeardownPaths said the runtime root was "resolved without
creating it, since teardown has no business making directories on its way
out". That was my comment and it was false: it went through
sandboxRuntimeRootFor, whose fallback calls os.MkdirTemp when the
cache-derived root would land inside the workspace. So cleanup created a
fresh temp directory, and a useless one — the fallback root is random per
process and could never match the tree the commands actually used.
sandboxRuntimeRootFor is split: deterministicSandboxRuntimeRoot computes the
cache-derived path and says whether it is usable, creating nothing, and the
existing resolver keeps the fallback on top of it. Teardown takes the pure
one and simply has no runtime tree to revoke when it reports unusable, which
is correct — there is no way to name the random root from here anyway.
The first version of the test called the pure resolver directly. It passed,
and reverting the call site to the creating one left it passing. It now
drives windowsPrincipalTeardownPaths and counts temp-directory entries
across the call, and that mutation fails it.
Co-Authored-By: Claude Opus 5 <[email protected]>
Three findings from jatmn's review, all reachable only under the opt-in
principal backend but all real.
FILE_DELETE_CHILD is no longer granted. On a parent it authorises deleting a
child whatever the child's own DACL says, so granting it on a write root
handed back the carve-outs underneath: delete .git/config, recreate it, and
the replacement inherits the grant with no deny of its own — restoring the
credential.helper and core.hooksPath control the carve-out exists to
prevent. It was granted to keep the mask symmetric with the deny mask, which
is the wrong instinct: denying a capability is not a reason to grant it. The
comment two lines up already made that argument for WRITE_DAC and
WRITE_OWNER. DELETE alone still covers removing and renaming files inside
the roots, which is what the grant is actually for — verified before
removing it.
Note this does NOT close the second route jatmn described: .git itself
carries no ACE, so renaming the whole directory aside needs only DELETE.
That needs a guard on .git and is not in this commit.
ACL targets are now rejected when a PARENT is a reparse point. CreateFile
resolves ancestors even with FILE_FLAG_OPEN_REPARSE_POINT, so the
final-component check passed while elevated setup rewrote the DACL of an
object outside the workspace. Junctions need no privilege to create, unlike
symlinks, so this was reachable by exactly the unprivileged user the sandbox
contains. GetFinalPathNameByHandle answers where the handle really landed,
covering every component in one call instead of walking the path and racing
between checks. The comparison is against the path's own resolved form, so a
differently-cased or 8.3 spelling is still accepted.
The revocation's rollback is returned instead of discarded. Discarding it
was justified on the grounds that the only failure path removes the
principal outright — true for a principal this run CREATED, false for one it
ADOPTED, which #812 keeps alive on failure rather than destroying someone
else's working account. The account survived with its previous ACEs stripped
and the new ones rolled back: logged on, and unable to reach its own
workspace. Teardown still discards it deliberately, since putting ACEs back
on an account about to be deleted is the opposite of the point.
Co-Authored-By: Claude Opus 5 <[email protected]>
TestCanonicalWorkspaceRootFallsBackWhenResolutionFails asserted that a path
with missing segments came back as the plain cleaned path. That was the
behaviour before the ancestor walk, and Windows CI failed it correctly:
canonical("C:\Users\RUNNER~1\...\001\never-created\deeper")
= "C:\Users\runneradmin\...\001\never-created\deeper", want the cleaned path
The existing ancestor resolved and the missing remainder was re-appended,
which is precisely what the walk exists to do. The assertion now says that:
the result equals the canonical parent joined with the segments that do not
exist, and those segments survive rather than collapsing to the ancestor.
Co-Authored-By: Claude Opus 5 <[email protected]>
The previous fix normalized both the workspace root and the cache root, and
macOS CI still failed the same way. EvalSymlinks fails outright when the
LEAF does not exist, and a cache root has not been created at the point it
is first normalized — so the workspace resolved (/var to /private/var)
while the cache root did not, and the containment check that decides
whether the runtime tree must move out of the workspace compared the two
anyway.
canonicalSandboxWorkspaceRoot now resolves the longest existing ancestor
and re-appends the remainder, so a path normalizes the same way whether or
not its final segments exist:
/var/.../001/.cache leaf missing, walk up
/var/.../001 resolves
/private/var/.../001/.cache
Terminates at the filesystem root, where it falls back to the cleaned
absolute path, and a path with no symlink anywhere along it is unchanged.
The regression test needs a symlink to produce a spelling only resolution
reconciles, so it skips on Windows — where creating one needs privilege —
and runs on the platforms that caught this. I could not reproduce either CI
spelling locally: this box's temp paths are already canonical and 8.3 alias
creation is disabled on the volume.
Co-Authored-By: Claude Opus 5 <[email protected]>
sandboxRuntimeRootFor compares the workspace root against the runtime root
it derives from the cache root, and falls back to a private temp tree when
the derived root would land inside the workspace. The previous commit
canonicalized only the workspace root, so that comparison ran on two
different spellings of the same path and the containment check missed:
macOS: /var/folders/... vs /private/var/folders/...
Windows: C:\Users\RUNNER~1\... vs C:\Users\runneradmin\...
The fallback never fired and the runtime tree was placed inside the
workspace it exists to stay out of. Both CI runners caught it; my box did
not, because its temp paths are already canonical and 8.3 alias creation is
disabled on the volume, so I could not reproduce either spelling locally.
Both inputs now go through canonicalSandboxWorkspaceRoot, on the
cross-platform path and the Windows setup path.
The regression test uses a symlink, which is the portable way to produce a
spelling only resolution reconciles — Clean cannot see through one. It
skips on Windows, where creating one needs privilege, and runs on the
platforms that caught the bug.
Two things about that test are deliberate. My first version used a
redundant-segment path, which Clean already normalizes, so reverting the
fix left it passing. My second resolved nothing before asserting, and
through the link the runtime root shares no textual prefix with the
workspace — it would have called a root sitting physically inside the
workspace "outside" and passed against the exact bug it exists for. It now
resolves before comparing.
Co-Authored-By: Claude Opus 5 <[email protected]>
Windows CI caught two faults in the previous commit. Both came from
canonicalizing one side of a pair.
setupWindowsSandboxRuntimeRoot resolved the workspace root while
prepareSandboxRuntime still only cleaned it, so the two disagreed exactly
where they had to agree. It passed locally because my temp paths were
already canonical; a Windows runner's TEMP is an 8.3 short path that
resolution expands:
setup granted ...\runtime\v1\5e2d212300ccdfba
commands use ...\runtime\v1\92c31f8cf536dfde
The canonicalization moves to canonicalSandboxWorkspaceRoot in
runtime_state.go and both sides call it, which is what the original fix
should have done.
The carveout shape was rebuilt from the RESOLVED write root and compared
against subpaths that cannot resolve, since .git/config does not exist at
setup and normalizeProfilePath falls back to Clean when EvalSymlinks fails.
Two spellings of the same path therefore missed the lookup and .git/config
went back to being created as a directory — the original bug, reintroduced
quietly by its own fix. gitMetadataCarveoutIsFile now matches on the
trailing segments, derived from the spec list so it cannot drift from it,
and no reconstructed absolute path is compared at all.
Both failures now have regression tests that reproduce the non-canonical
root by lowercasing, which needs no short name and no privilege. Reverting
either fix fails them.
Co-Authored-By: Claude Opus 5 <[email protected]>
Two ways setup and the command path disagreed.
Teardown removed the secret, the LSA rights and the account, but never the
ACEs. Once the account is gone its SID stops resolving and every ACE naming
it becomes an orphaned raw-SID entry on the user's own tree — precisely the
residue the capability-SID model left behind and this one exists to avoid.
Revocation now runs while the SID still resolves, by trustee so it also
clears grants written by older versions. A revoke failure is deliberately
not fatal: a path the user has since deleted cannot be cleaned, and
refusing to remove the account over it would strand the principal and its
logon rights permanently, which is worse than a leftover ACE.
The runtime root was derived from filepath.Clean(WorkspaceRoots[0]) at
setup while Engine.resolveCommandDir cleans, absolutizes and then
EvalSymlinks it. That needs no symlink to diverge — Windows opens a path in
any casing and EvalSymlinks canonicalizes it:
setup sees c:\users\me\myworkspace
command sees C:\Users\me\MyWorkspace
so setup granted the principal one runtime tree and every command used
another. The grant that exists to make npm/go/pip caches writable landed
where nothing reads, surfacing as a bare ACCESS_DENIED on a cache write.
Both now go through canonicalWindowsSandboxWorkspaceRoot. An unresolvable
root falls back to the cleaned absolute path, matching the command path
rather than failing.
setupWindowsSandboxRuntimeRoot is split into derivation and creation so
teardown can name the tree without making directories on its way out.
The first version of the divergence test called the canonicalization helper
directly. It passed, and reverting setup to filepath.Clean — the actual bug
— left it passing. It now drives windowsSandboxRuntimeRootPath, and that
mutation fails it.
Co-Authored-By: Claude Opus 5 <[email protected]>
windowsPrincipalRevokePlan was implemented and tested but had no production
caller, so nothing ever used it. applyWindowsACLPlan merges into the
existing DACL, which means a re-run after narrowing a write root or
shortening a deny list left the previous, wider ACEs sitting beside the new
ones: the principal kept access the current policy no longer granted, and
the sandbox silently widened as a result of being tightened.
Setup does get the chance to notice — marker validation already refuses
commands with "permission roots or deny lists changed" until setup runs
again — so the re-apply is exactly where this belongs.
The ACL step is extracted into applyWindowsPrincipalACLs: build the plan,
revoke every ACE naming this trustee on the paths it touches, then apply.
Revocation is by trustee rather than by remembered path, so it also clears
grants written by an older version of Zero. Its rollback is discarded on
purpose — the only failure path from here removes the principal outright,
and restoring stale ACEs for an account about to be deleted is the residue
this exists to prevent.
Extracting it also makes the ordering testable without new provisioning
seams, which #812 already adds with a different signature; adding them here
would have collided on its rebase.
Three tests: revocation actually drops a grant on a root that left the
policy while keeping the one that stayed (asserted against the real DACL,
counting deny ACEs as well as allow, since trustee revocation drops both);
revoking a path that was never created is a no-op rather than an error; and
the production path revokes BEFORE it applies. That last one is the one
that matters — the first two pass just as happily with the call site
deleted, and deleting it kills only the third.
Co-Authored-By: Claude Opus 5 <[email protected]>
Provisioning refuses an account that is already in Administrators, Power
Users or Backup Operators, but group membership is not frozen at setup. An
account provisioned clean can be added afterwards — by an operator, or by
an attacker who already has that access and would like the sandbox to hand
it back. Every command after that minted a token for a privileged account.
The re-check goes on the path that mints the token, not inside
lookupWindowsSandboxIdentity. Teardown resolves the same identity to revoke
its logon rights before deleting the account, so refusing there would leave
the very account this guards against permanently undeletable by Zero. The
command path already propagates anything that is not the not-provisioned
sentinel, so this surfaces to the operator instead of silently dropping
back to the restricted token.
Also make the gating test hermetic. Its "absent" case passed an empty map,
which falls through to os.Getenv, so a developer with the opt-in exported
saw a different result from CI:
ZERO_WINDOWS_SANDBOX_IDENTITY=1 go test ./internal/sandbox/
--- FAIL: TestWindowsSandboxIdentityGating/absent
enabled = true, want false for ""
Every case there supplies an explicit map entry, so the process variable is
now pinned to prove none of them consult it. The os.Getenv fallback is what
elevated setup actually runs on — it passes no Env — so it gets its own
table rather than riding on a case that also has a map entry.
Co-Authored-By: Claude Opus 5 <[email protected]>
Materializing the git control-plane carveouts creates a missing target so
the deny-write ACE is in place before git first runs. It did that with
os.MkdirAll on the full path, which is right for .git/hooks and wrong for
.git/config: git wants a file there.
The consequence is worse than a mis-ACL'd path. On a fresh workspace
neither carveout exists — which is exactly the case materialization was
added for, so this is the common path rather than a corner — and elevated
setup would leave a directory where git's config file belongs:
warning: unable to access '<ws>/.git/config': Permission denied
fatal: unknown error occurred while reading the configuration files
git init then fails outright and the workspace is unusable.
Materialization now takes the shape from the carveout definition:
gitMetadataWriteCarveoutSpecs is the single source of truth and
gitMetadataWriteCarveouts derives its list from it, so a carveout cannot be
added in one place and have its shape forgotten in the other. A file target
gets its parent chain created and then an empty file; a directory target is
unchanged. A racing creator winning the O_EXCL is treated as success, since
the target existing is all materialization needed.
The regression test runs a real `git init` over the applied plan. It names
Guests as the principal rather than Everyone — with Everyone the deny ACE
also denies the test process and git fails for an unrelated reason, which
would have made the test pass for the wrong reason once the shape was fixed.
Co-Authored-By: Claude Opus 5 <[email protected]>
Adoption takes over an account whose name and ownership comment match,
resets its password and hands it to the sandbox. An account that is also
in Administrators, Power Users or Backup Operators would give the sandbox
the rights it exists to withhold: rewriting the ACLs confining it, reading
the secret locked to the invoking user, and stopping Zero. The name is
derived rather than discovered, so an account can match without anyone
intending it to.
Membership is resolved by well-known SID rather than by group name, so a
localised install where the group is Administratoren or Administrateurs is
still recognised.
Raised as a non-blocking follow-up in review; it is cheap enough to do now
rather than track.
Four review findings on the elevated setup path.
The principal had no access to the sandbox runtime root.
permissionProfileWithRuntime appends that root to WriteRoots on every
command and redirects HOME, GOCACHE, npm_config_cache and similar into it,
but it lives under the user cache rather than the workspace, so the
profile setup builds its ACL plan from never contains it. On the
restricted-token path that costs nothing, since the child still runs as
the caller. A principal is a separate local account with none of those
rights, so every npm install, go build or pip install would have failed on
a cache write with a bare ACCESS_DENIED and nothing naming the sandbox as
the cause. Setup now resolves the same root and grants it.
The derivation is extracted so both callers share it. If setup and
prepareSandboxRuntime ever disagreed, the ACE would land on one directory
while commands used another, which is the same failure with a harder
diagnosis, so a test asserts the two agree.
The git control-plane carveouts are materialized. .git/config and
.git/hooks arrive as ReadOnlySubpaths, and applyWindowsACLPlan skips an
absent target, so on a workspace where git had not run yet the deny ACEs
were never written and the principal kept inherited write access once git
created them.
Command-time lookup verifies workspace ownership. The account name carries
only 11 characters of the workspace digest; the comment carries all of it.
Provisioning already refused a foreign account, but the command path
resolved the name straight to a SID, so the workspace that lost a
collision would have run as the other one's principal. SID resolution
still runs first, so an absent account stays the unavailable sentinel
rather than becoming a collision error.
The gated round-trip test asserted a logon with the password from a second
provisioning call. Rotation moved to the setup path, so that value is a
fresh string the account never held. It now exercises the guarantee the
setup path actually makes: the stored secret logs the principal on.
Second round of review findings, both on the elevated setup path.
The rollback revoked logon rights whenever they had been attempted,
without regard to whether this run created the account.
revokeWindowsSandboxLogonRights passes AllRights, which drops every right
the account holds and deletes its LSA object outright. On an adopted
principal that is not a rollback but destruction: a transient grant,
secret-path or secret-write failure during a re-run stripped the
SeBatchLogonRight and deny-logon rights an earlier setup had established,
leaving exactly the broken-but-present principal this path exists to
avoid. Revocation is now scoped to accounts this run created. The rights
granted to an adopted account are the ones it is supposed to hold, so
leaving them is the safe direction.
A secret the current user cannot read now falls back instead of failing
the command. The secret's DACL names whoever ran setup, so an operator who
elevated with a separate administrative account, through runas or an
over-the-shoulder UAC prompt, leaves a secret their ordinary account
cannot open. That is the documented fail-soft case, and treating it as a
hard error made every sandboxed command fail on a machine that was merely
set up by a different admin. Permission errors from the removal path are
deliberately still reported, since incomplete teardown is worth knowing
about.
Both are covered by injected-failure tests and fail if the guard is
removed. The secret read is seamed to inject the permission error, because
producing a real ERROR_ACCESS_DENIED needs DACL surgery and would test the
platform rather than the mapping.
Six findings from review, all on the elevated setup path.
Teardown was not scoped to what the run created. provisionWindowsSandbox
PrincipalForSetup was careful never to delete an account it had adopted,
and then setupWindowsSandboxPrincipal called removePrincipal on any ACL
failure with no such guard. Re-running elevated setup on a working machine
and hitting one transient ACL error therefore deleted the local account,
its secret and its logon rights. It now returns whether it created the
principal and the outer teardown honours it; ACEs are still reverted,
since this run applied them.
Password rotation moved to immediately before the secret is committed.
Resetting an adopted account's password at the top of provisioning meant
every later step ran against an account whose password had been replaced
with no copy stored. Any failure there left a live account authenticated
by a password nothing on disk knew, and since the account pre-existed the
rollback correctly declined to delete it, so the command path read the
absent secret as "not provisioned" and fell back to the weaker backend for
good. The two operations are now adjacent. The rollback also stops
removing the secret when this run neither created the account nor rotated
it, because that secret still works.
Policy DenyWrite now reaches the principal ACL plan. The capability plan
has always emitted these; the principal plan denied write only on
protected metadata and read-only subpaths, so once the runner used a
principal token a policy deny elsewhere was not enforced at all.
Principal deny-read entries are materialized, matching the capability
plan, so a path created after setup still gets a deny ACE.
Logon-right revocation is keyed to the attempt rather than to success.
Rights are added one at a time and the grant returns on first failure, so
a partial grant left LSA entries behind pointing at a SID that deleting
the account then made unresolvable.
The ownership comment now carries the full workspace key. The account name
holds only 11 characters of the digest, so two workspaces could derive one
name and silently share an account, a secret and an ACL identity; a
mismatch is now refused. Accounts provisioned before the key was recorded
are still adopted.
Also warns once on stderr when the opt-in is set and a provisioned
principal cannot be used, rather than downgrading in silence.
Two findings from review, both consequences of the principal being a
separate account rather than the calling user.
WindowsACLAllowWrite granted FILE_GENERIC_WRITE, which covers creating and
modifying but not removing or renaming, and a rename needs delete on the
source. Under the old same-user token this was invisible because the caller
already held inherited rights on its own tree. A principal inherits nothing,
so it could write files it could never delete, which fails ordinary editing
and most git operations rather than an edge case. DELETE and
FILE_DELETE_CHILD are now part of the grant, matching WindowsACLDenyWrite,
which already treats delete as part of write. WRITE_DAC and WRITE_OWNER stay
out: they are denied so the principal cannot rewrite its own restrictions.
provisionWindowsSandboxIdentity returned a zero identity alongside
created=true when group attachment or SID resolution failed after NetUserAdd
had already created the account. The caller's rollback deletes by
identity.Username, so it was asked to delete the empty string and left the
account behind. Group attachment is the case that matters, being both the
enforcement boundary and something local policy can refuse. The name now
comes back with the error.
The four provisioning calls are indirected so the failure paths are
reachable in a test. Seaming only the post-creation pair would not have been
enough: every step needs an elevated caller, so the test would have stopped
at the group check and passed without reaching what it names.
Also seeds the empty-secret test with a genuinely empty file. The previous
whitespace seed was several bytes, so it never reached the length check and
failed later in DPAPI instead, which another test already covers.
GetAce returns a generic ACE_HEADER and the helper reinterprets it as an
ACCESS_ALLOWED_ACE. That holds for the fixed-layout types, but an object
ACE carries Flags and two GUIDs ahead of the trustee, so SidStart would
land mid-structure and Copy would read whatever bytes follow. The caller
asserts that no unexpected trustee appears in the DACL, and on such an ACE
it would print a nonsense SID rather than name the entry that does not
belong.
Nothing under test builds anything but allowed ACEs today, so this changes
no current outcome. It keeps the failure legible if that ever changes.
The cleanup added for partial provisioning left the window it existed for
uncovered. It only removed the on-disk secret when this run had written one,
and it derived the secret path after the logon-rights grant, so a failure
before that point had nothing to remove.
That is exactly the case that matters. Provisioning ALWAYS sets the account's
password, including resetting a pre-existing account's, so from the moment it
returns the stored secret is already stale. A failure in the rights grant then
left that stale secret on disk against a password that had just changed, and
the next command failed the logon and reported a broken sandbox instead of
falling back.
The path is now resolved from the account name before anything can fail, and
removal is unconditional rather than gated on having written one. Absent beats
stale: the command path treats a missing secret as "not provisioned" and falls
back to the restricted token, which is the outcome a failed setup should leave
behind.
Raised by CodeRabbit, twice from different angles, on the commit that added the
cleanup.
Two problems in the provisioning path, both raised in review.
The account name is derived from a workspace hash rather than discovered, so it
can be occupied by a local account that has nothing to do with Zero, whether by
coincidence or because somebody put it there. Provisioning treated "NetUserAdd
says it exists" as "this is ours", reset the account's password, added it to the
managed group and adopted it. That is a stranger's account taken over during an
elevated setup, on the strength of a name matching a pattern we generate
ourselves.
Ownership is now proven from the comment provisioning stamps before anything is
touched, and a name held by an account Zero did not create fails with a typed
collision error instead of being adopted.
Second, a failure anywhere after the account existed left it behind. The
rollback the setup path installs is only built once provisioning has returned
successfully, so nothing could undo a failure between creating the account and
storing its secret; the account, and possibly its granted logon rights, simply
stayed. Provisioning now unwinds what the run actually did, in reverse, on every
failure path.
Scoped to what THIS run created, deliberately. An account that already existed
and belongs to Zero is a working principal from an earlier setup, so deleting it
because a later run failed would turn a partial failure into a total one. For
that case the repair is dropping the stored secret instead: this run reset the
password, so the secret no longer matches, and absent beats stale because the
command path treats a missing secret as "not provisioned" and falls back rather
than failing.
The ownership gate is asserted against real accounts every Windows install
carries, which needs no privilege because it only has to establish that they are
not ours. Classifying everything as managed makes it fail.
Retiring a principal deleted the account but left its LSA account rights
behind, keyed to a SID that no longer resolves. That is the orphaned residue
this model is supposed to avoid, and the reason ACE revocation is keyed to the
trustee rather than to a record of what was granted; the logon-rights half was
simply missing. CodeRabbit spotted it as a test-cleanup gap, but the production
teardown path had the same hole.
revokeWindowsSandboxLogonRights drops every right held by the principal and
removes its LSA entry, and setup teardown now calls it BEFORE deleting the
account, while the SID still resolves. Removing all rights rather than naming
them is deliberate: the principal is being retired, so rights granted by an
older setup that this one no longer knows about should go too.
An account that holds no rights is not an error, since that is the state
teardown wants. That tolerance depends on STATUS_OBJECT_NAME_NOT_FOUND
surviving LsaNtStatusToWinError as something errors.Is can still match, which
is the sort of Windows errno assumption that is often wrong, so there is now an
unprivileged test asserting it, including that the tolerance does not also
swallow access-denied.
Both gated tests now clean up rights and account, in that order. The
provisioning round trip had no cleanup at all and, since it started granting a
batch logon right, was leaving both behind on whatever machine ran it.
NetUserAdd leaves a pre-existing account completely untouched, password
included, and ensureWindowsSandboxUser treated that status as success. So a
second setup run generated a fresh random password, stored it as the secret,
and left the account still authenticating with the old one. Every later command
then failed to log on with a principal that looked correctly provisioned. Two
comments claimed the caller reset the password in that case; nothing did.
Caught by CodeRabbit.
ensureWindowsSandboxUser now reports whether the account already existed, and
provisioning resets the password through NetUserSetInfo when it did, so the
value it returns is always the account's real password. The comments now
describe what the code does.
The gated provisioning test provisions twice and then logs on with the password
from the SECOND run, which is the only honest assertion here: a stale password
is indistinguishable from a correct one until something tries to authenticate
with it.
Also makes the syscall keep-alives explicit. The LSA and LogonUser call sites
borrow Go memory that was either not kept alive at all (the policy attributes,
the rights descriptor, the three logon strings) or kept alive only after the
error check, so the failure path returned with it already collectable. The two
netapi32 sites that used a deferred no-op closure now use runtime.KeepAlive as
well, so one idiom is used throughout.
lookupWindowsSandboxIdentity collapsed every SID-resolution failure into the
"no principal is provisioned" sentinel, which threw away the check
resolveWindowsSandboxSID deliberately makes: a name that resolves to a group or
alias rather than a user account. The command path treats that sentinel as
permission to fall back quietly, so an account name squatted by something that
is not a user reached the operator as silence and a downgrade to the restricted
token. Caught by gnanam in review.
Only ERROR_NONE_MAPPED now means setup has not run. Anything else is a
principal that exists but cannot be used, and the runtime path propagates it
rather than swallowing it, which is where the description already said the line
should sit.
The decision lives in its own function because the lookup derives its account
name from a workspace key, so a test cannot hand it a name that resolves to a
group. The test drives the classifier with a real error from a well-known local
group, needs no privilege, and fails if the old collapse-everything behaviour
is restored.
Also corrects a comment pointing at sandboxRuntimeKey, which does not exist.
The function is windowsSandboxWorkspaceKey.
The file ACL stays the primary control and is what keeps the sandbox
principal from reading its own credential. It only binds while the filesystem
is the one being asked, though, so a backup or a mounted image hands over the
password in the clear. CryptProtectData ties the ciphertext to the invoking
user's logon secret, which covers exactly that gap.
The principal name is passed as entropy, so a blob copied onto another
principal's path fails to decrypt rather than authenticating the wrong
account. A secret written by an older build reads as unavailable and falls
back to the restricted token; the next elevated setup rewrites it.
The round-trip test needs no privilege, so it runs everywhere rather than
joining the gated set, and it asserts the password does not appear verbatim
in the stored bytes.
Network denial is enforced by WFP filters keyed to the offline-marker SID.
The restricted token carries that SID; a token from LogonUser cannot, because
it names the account rather than a synthetic capability SID. Routing a
denied-network command through a sandbox principal therefore left the block
filters matching nothing and dropped egress enforcement altogether, and deny
is the default mode.
The principal now stands down whenever the network is denied and the
restricted-token backend runs instead, so read confinement is never traded
for a silent loss of network denial. Keying the filters to the principal's
own SID is the follow-up that lifts the restriction.
The decision sits in its own predicate rather than inline: on a machine with
nothing provisioned the lookup declines for its own reasons, so a test that
called through it would have passed with the guard removed.
Also names the opt-out variable when a provisioned principal cannot be used,
since the backend is opt-in and the operator needs a way back.
Completes the chain. Until now the principal entry points had no non-test
callers, so `zero sandbox setup` created no account and the runner seam always
fell back: the feature was inert end to end.
Setup now provisions this workspace's principal, grants it the batch logon
right, stores its password locked to the invoking user, and applies the ACL plan
that gives it read+write on the workspace and read on the declared read roots. A
principal is a separate account with no inherent access to the caller's tree, so
those grants are what make the sandbox able to run at all, and their absence
elsewhere is what puts credential stores out of reach.
Provisioning is folded into the existing rollback rather than each later failure
path having to remember it, and the rollback revokes ACEs before deleting the
account: removing the account first would leave ACEs naming a SID that no longer
resolves, which is the orphaned residue this model exists to avoid.
Gated on the same opt-in as the runner. Account creation is visible in
`net user` and is exactly what endpoint protection and enterprise policy object
to, so it happens only when asked for; without the opt-in the capability-SID
backend remains the whole of setup, unchanged.
Provisioning is now validated on real Windows, but LsaAddAccountRights and
LogonUser had still never executed, so the principal was proven to exist without
being proven usable.
The batch logon doubles as the assertion that the rights grant worked: a
LOGON32_LOGON_BATCH logon fails with ERROR_LOGON_TYPE_NOT_GRANTED unless
SeBatchLogonRight is actually held, so a token coming back is evidence the grant
landed rather than merely that the call returned success. Granting twice is
exercised too, since setup re-runs must not fail on rights already held.
The token's user SID is compared against the principal's. If a token came back
belonging to the caller the identity boundary would be an illusion and reads
would still run as the user, which is the whole thing this model exists to stop.
Removes any leftover account first and cleans up after itself, because an
interrupted earlier run would leave an account whose password no longer matches
a freshly generated one.
The skip fired on an unset environment variable but read as though elevation
was missing. `set VAR=1` is cmd syntax and sets a shell variable rather than an
environment variable in PowerShell, so the test skipped silently after the
operator believed they had enabled it. Spell out all three shells.
Wires the principal model into the runner and settles where the account's
password lives.
The secret is stored under the sandbox home with an explicit,
inheritance-protected DACL naming only the invoking user and SYSTEM. The sandbox
principal is deliberately absent from it: a principal that could read the file
could mint its own token and the identity boundary would be decorative. The ACL
is applied to an empty file before the password is written, so the bytes never
exist under the config directory's inherited permissions, and PROTECTED drops any
inherited ACE outright.
At command time the runner asks for a principal token first and uses it in place
of the restricted token, because a separate account has reads denied by the
filesystem rather than left open the way a same-user restricted token must leave
them. The lookup is fail-soft: opt-out, no provisioned account, or no stored
secret all report "not available" and the existing restricted-token path runs
unchanged. Only a provisioned-but-unusable identity surfaces an error, since
that means setup ran and the sandbox is broken rather than absent.
The backend stays behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 while the privileged
paths are unvalidated, so no existing install changes behaviour.
Groundwork for closing the Windows half of #662 and #675, where
credentialDenyReadPaths is a no-op today.
Every Windows backend currently derives its token from the calling user via
CreateRestrictedToken, so the sandbox can constrain writes but not reads: a deny
ACE that would stop the sandboxed child reading a credential store names the
same account Zero runs as, and would lock Zero out too. That is why deny-read is
skipped on Windows rather than merely unimplemented.
This adds a separate local account per workspace, held in one managed group, so
the sandbox has an identity of its own:
- provisioning: managed group, stable per-workspace account name inside the
20-character limit, crypto/rand password meeting complexity policy, SID
resolution, idempotent so setup re-runs converge
- logon rights: grants only SeBatchLogonRight and explicitly denies interactive,
network, remote-interactive and service logon, then mints a token with
LogonUser pinned to the local machine
- ACLs keyed to the principal: denies emitted before allows so carve-outs
survive, workspace granted read+write, read roots granted read, protected
metadata denied write and materialized
- removal: revocation by trustee, so retiring a principal drops every ACE naming
it without needing a record of what was granted
The inversion is the point. A separate account has no access to the caller's
profile at all, so credential stores are unreachable by construction rather than
by enumerating deny rules, and the same SID is what a write grant or a firewall
rule can be keyed to.
Nothing is wired into command execution yet: these paths are additive and no
existing behavior changes. See the pull request for the open question about
where the principal's password lives.
* 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]>
* 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 ad34dc8d as requested. go test -race ./internal/sandbox/ -count=5:
clean. Pre-existing here and on main, unrelated:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: d2b8f44a9abc
* fix(sandbox): a nested write holder keeps only the subtree it asked for
Reported by CodeRabbit, and it is the write-side twin of the read-side
escalation @jatmn found — I had disclosed it on this PR as still open, so this
closes it rather than deferring it again.
AddTemporaryWrite, given a path already covered by a broader TEMPORARY write
root, took a reference on that covering root. The lifetime was right and the
capability was wrong, exactly as on the read side: extraRoots is what validate()
authorises writes from, so keeping the covering root alive handed the nested
holder everything its neighbour had asked for.
Reproduced:
both held tempWrites={outer:2} extraRoots=[... outer]
after outer release tempWrites={outer:1} extraRoots=[... outer]
-> the inner-only holder can WRITE outer/sibling/not-mine.txt
A narrower request now records its own root. Each holder gets exactly the
authority it asked for and a lifetime of its own:
both held tempWrites={outer:1, inner:1} extraRoots=[... outer inner]
after outer release tempWrites={inner:1} extraRoots=[... inner]
-> sibling denied, inner retained
Only an EXACT match shares a refcount entry; a narrower one never does, which is
the distinction the escalation turned on. The nesting costs a redundant
extraRoots entry while both holders are live, which is not wrong — the broader
root already permits everything the narrower one does.
Three regressions: the escalation itself (broad holder releases first), the
opposite order as a control, and two holders of the same root sharing one entry.
Mutation: restoring the covering-root borrow compiles and fails the escalation
test on both the sibling and the covering root itself.
Honest note on a second mutation. Removing the permanent-cover early return
compiles and fails NOTHING. That is correct rather than a coverage gap: a
permanently covered request would then record its own temporary root, which is
redundant but harmless, since the permanent root outlives it and already permits
the writes. The early return is a cleanliness guard, not a correctness one, and
I would rather say so than write a test that pretends otherwise.
go test -race ./internal/sandbox/ -count=5: clean. Pre-existing here and on main:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
Origin-Session: local-c962d7 | Claude Code | 5 prompts
Origin-Snapshot: 3bb420a0a97b
* test(sandbox): the outside-roots fixture fails loudly instead of skipping
Raised by CodeRabbit, and the helper had already written the argument against
itself: its own comment says "A skip is not a pass" while three of its four exits
were skips.
Every caller of scopeOutsideRoots asserts an AUTHORIZATION boundary — who may
write where, and whose release revokes it. A skipped fixture means that assertion
never ran and the build reported green anyway, which is precisely the failure the
helper was written to close. It closed it for one case (a home directory that
resolves nowhere on Windows) and left the others open.
Three exits become fatal:
os.UserHomeDir failing — resolves on every platform Zero supports
os.MkdirTemp(home) failing — a home that will not take a temp directory is a
broken machine, not a limited one
os.MkdirAll under base — base was just created successfully, so a
subdirectory failing under it is the same
If any fires, the right outcome is a red build somebody fixes, not silent
non-coverage of the sandbox's write rules.
ONE SKIP STAYS, and it is a different kind. When the grant path is already inside
a default write root, the fixture built correctly and the machine is fine — the
grant simply cannot demonstrate anything, because that path is writable before
any temporary grant exists. Running on would assert a permission that was never
in question. The comment now says which kind it is, so the distinction survives
the next reader.
Verified the fatal path fires: forcing the MkdirTemp error compiles and fails
TestANestedWriteHolderKeepsOnlyItsOwnSubtree at the fixture with the reason,
where it previously reported a pass.
go test -race ./internal/sandbox/ -count=3: clean. Pre-existing here and on main:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
Origin-Session: local-c962d7 | Claude Code | 13 prompts
Origin-Snapshot: a599377c09e0
* fix(sandbox): least authority for nested readers, and a hermetic grant fixture
@jatmn's three findings, taken as one pass over the grant construct rather than
as three local patches — his point that the same invariant kept not being
reapplied to the symmetric branch was fair, and this is the symmetric branch.
## The nested reader kept its neighbour's tree
AddTemporaryRead had exactly the shape AddTemporaryWrite was fixed for one
commit earlier: reader B asking for outer/inner took a reference on reader A's
covering root. Lifetime correct, capability wrong. readRoots feeds validateRead
and the native sandbox profile, so once A released, B could read outer/sibling.
Reproduced: tempReads={outer:2}; after A releases, the inner-only reader reads a
sibling it never asked for.
The rule is now the same on both sides: permanent coverage decides first, an
EXACT temporary root shares one refcount, and a narrower request records its own
root. Mutation: restoring the borrow fires both escalation assertions.
## Slice order no longer decides permanent coverage
permanentReadRootCoversLocked asks the whole question over both lists before any
insertion order can answer it, and AddRead now uses the same predicate instead of
its own scan, so the two cannot drift.
Honest note: with the borrow gone, this mutation is no longer catchable. Making
the scan return at the first covering entry compiles and fails nothing, because
AddTemporaryRead no longer borrows anything — it either no-ops or records its own
root, and neither escalates. The guard now prevents a redundant refcount entry
rather than an escalation. Verified by measurement, not assumed.
## A test that asserted the defect
TestANarrowerRequestHoldsTheCoveringGrant proved a lifetime property by
asserting the BROAD root was still present after the broad holder released —
which is the escalation written down as a requirement. It is replaced by tests
that assert lifetime and authority separately: the requested path stays readable,
the sibling and the covering root do not, plus the reverse release order,
exact-root sharing, and a permanent narrow root not borrowing a broad temporary
one.
## The obsolete predicate that broke CI
writeRootCoversLocked lost its last caller when the three coverage decisions
moved to permanentWriteRootCoversLocked and exact-root logic. make lint-static
reproduced the Windows `unused` failure locally. Removed rather than suppressed.
This is on me: I did not run lint-static before the previous push, which is a
required step, and CI caught what I should have.
## Fixtures out of the real home
scopeOutsideRoots and homeGrantOutsideTempRoots both built fixtures under
os.UserHomeDir(). Fixed at the cause jatmn identified rather than by relaxing the
setup checks: NewScope seeds the system temp directory as a permanent write root,
which is what made a t.TempDir() grant vacuous and sent the fixture to HOME in the
first place. Both helpers now build the Scope DIRECTLY — the pattern
scopeOutsideDefaults and temporaryWriteFixture already used — so no defaults are
seeded, t.TempDir() is outside by construction, and nothing touches HOME.
Each helper now asserts the NEGATIVE against its own scope before returning: the
target must be unwritable, and unreadable where relevant, before any grant
exists. That is stronger than the old check against defaultTempWriteRoots(),
which asked about a scope these tests no longer build.
Verified with HOME readable but not writable (caches redirected): the sandbox
suite passes. The one remaining failure there is
TestSeatbeltEnforcesExtraWriteRoots in seatbelt_integration_darwin_test.go, which
is NOT in this PR's diff — pre-existing, same shape, and worth its own issue
rather than riding along here.
gofmt, go vet, lint-static (0 issues), go test -race ./internal/sandbox/ -count=5:
all clean. Pre-existing here and on main:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
Origin-Session: local-c962d7 | Claude Code | 14 prompts
Origin-Snapshot: a599377c09e0
* refactor(sandbox): drop the child-grant accessors this PR does not consume
Reported by @jatmn.
ExtraRoots and ExtraReadRoots have no production caller anywhere in the tree —
only their own definitions and this PR's tests. They arrived with the initial
split because the extraction was made at the file level: the temporary-root
implementation came across while the CLI and runner wiring that consumes them
stayed behind in #829's child read-grant propagation area. That leaves unrelated
production surface in a branch scoped to refcounting.
Both accessors are removed, along with the getter-only propagation assertion that
existed to exercise them. The refcount regressions that used ExtraRoots only to
inspect beyond-workspace write state now use a test-only helper over Roots(),
because that inspection is a test concern rather than an API this branch needs.
Nothing about the refcount, permanence, least-authority, atomic-release or
hermetic-fixture work changes, and no child propagation is wired here — that
would widen the scope rather than fix the extraction boundary.
One comment needed correcting rather than deleting. The justification for keeping
a reader's lifetime as a READ root named the lists readRoots feeds, and that
sentence has now been wrong in both directions: an earlier draft said "ReadRoots
and nothing else" while ExtraReadRoots also read it, and the accessor it was
corrected to mention is the one being removed here. It now describes what ships
and says why the list has to stay exhaustive as accessors change.
Rebased onto 6fe0d1ed, 0 behind. lint-static: 0 issues.
go test -race ./internal/sandbox/ -count=3: clean. Pre-existing here and on main:
TestRunDoctorFormatsRedactedProviderDiagnostics and
TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
* fix(tools): an edit keeps the reads it did not disturb
Split out of #829 as an independent fix, per @Vasanthdev2004's review asking for
the small self-contained pieces to arrive separately.
A successful edit re-baselined the file through RecordHash, which drops EVERY
recorded read range. A file read in three pieces therefore lost all three to a
single two-line edit, and the next six edits into regions that had already been
read were refused as unseen — the model had to re-read what it had just read in
order to keep working.
RecordEdit replaces that on the edit path. Because the edit is ours, the changed
span is known exactly, so ranges the edit did not touch are carried across,
ranges after it are shifted by the line delta, and a range the edit spans is
split around it.
An EXTERNAL change still drops everything, deliberately: once a formatter has
rewritten the file we no longer know which line holds what was read, and the
conservative drop is the only honest answer there.
"Seen whole" is now decided in ONE place. SeenWhole derives the answer from the
ranges because the raw flag is only ever set by a single covering read, but
RecordEdit branched on the flag — so a file read whole in two chunks reported
seen-whole true, then false after one edit, putting back the write_file refusal
the derived answer exists to prevent.
countLines now reports the count a READER would give. strings.Split leaves an
empty final element for content ending in a newline, so it counted 4 lines for a
3-line file; since SeenWhole asks whether the ranges cover 1..total, that
inflated total made full coverage unreachable for a file just read in full.
Nearly every text file ends in a newline, so this was the common case.
Origin-Session: local-de382f | Claude Code | 9 prompts
Origin-Snapshot: 92ae33c95cd1
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: ae653221fa15
* chore(tools): re-request an automated review of this head
No code change. CodeRabbit's verdict on this PR is pinned to 6f0cd6c4, a commit
that was force-pushed away five days ago, and a stale changes-requested keeps the
PR at mergeStateStatus=BLOCKED exactly as a human one would. It re-reviews on
push rather than on request, and repeated @coderabbitai triggers over five days
produced nothing, so this is the only lever available without maintainer rights.
The cost is that this dismisses @Vasanthdev2004's and @anandh8x's approvals of
0b6fcf54, which is an unhappy trade for an empty commit — the tree they approved
is byte-identical to this one.
Origin-Session: local-8cd239 | Claude Code | 6 prompts
Origin-Snapshot: 149ab98453ad
Origin-Session: local-c962d7 | Claude Code | 3 prompts
Origin-Snapshot: ae653221fa15
* perf(tools): derive the changed-line span from bytes, not from per-line slices
Reported by @jatmn.
changedLineSpan split both file versions into []string, so it allocated one
string header per line for EACH version — work that scales with the size of the
file rather than the size of the edit. countLines then split a third time just to
take a length. All of it ran before RecordEdit had checked whether there was a
tracked observation to update at all.
He measured 32,036,640 bytes for two 2 MB versions of a million short lines, and
extrapolated roughly 1.6 GB of headers for a 100 MB file before counting the
content, the updated bytes and the hashes already live beside them.
The memory was not the sharp end. edit_file writes the updated bytes BEFORE
calling RecordEdit, so an out-of-memory kill lands after the user's file has
changed and before the tracker baseline catches up: the file and the record of it
disagree, and nothing says so. His instruction was to fix the allocation source
rather than add a size guard, and that a guard — if used at all — would have to
reject before os.WriteFile rather than during post-write re-baselining. Taken as
written: there is no new limit, and the same edits are accepted as before.
Two identical byte prefixes share every line that ends inside them, so counting
newlines in the common prefix counts the unchanged leading lines directly. Same
for the suffix, with one rule that the line-array version got for free: the line
a shared tail STARTS in is only shared when the tail begins on a line boundary in
both versions, or its opening bytes differ and it is a changed line that merely
ends the same way.
Measured on his own scenario, two 2 MB versions:
changedLineSpan 32,036,640 bytes -> 0 allocations
countLines per-line slice -> 0 allocations
## The equivalence is tested, not asserted
A rewrite of a diff primitive is only safe if it is behaviour-preserving, so the
replaced implementation is kept in the test file as an oracle and both are asked
the same questions. That caught 206 mismatches in my first attempt at the byte
scan — empty versions, and trailing lines that are only partially shared. Neither
appears in the hand-written cases, and I would have shipped it.
Two regressions: the oracle comparison, and an absolute allocation ceiling rather
than a ratio, so a future rewrite that reintroduces per-line slices fails rather
than merely getting slower. Mutation: restoring strings.Split in either function
allocates 3.2 MB and 3.6 MB against a 4 KB bound.
splitLinesForTracking lost its last caller and is removed — the same obsolete-
helper lint failure that broke Windows CI on #911, caught here by running
make lint-static before pushing rather than after. 0 issues.
Rebased onto ad34dc8d, 0 behind. go test -race ./internal/tools/: clean.
Pre-existing here and on main: TestRunDoctorFormatsRedactedProviderDiagnostics
and TestRunDoctorConnectivityProbesProvider exit 3 in this environment.
Origin-Session: local-c962d7 | Claude Code | 17 prompts
Origin-Snapshot: a599377c09e0
* perf(agent): cheap compaction — dedicated summarizer model, input caps, free reactive prune, window re-derive
Compaction summarization was the single most expensive recurring event
in a long run: the full elided middle — verbatim tool bodies included —
went to the session's main model at full price (a ~100k-token call near
a 200k window, recursively multiplied on overflow).
Four changes, each independently safe:
- Dedicated summarizer model. Compaction summaries route to a cheap
model resolved as ZERO_COMPACTION_MODEL > preferences.compactionModel
> a curated default on official endpoints (Haiku 4.5 / Gemini
Flash-Lite / gpt-4.1-mini; custom-endpoint catalogs are unknowable so
they get none). Built lazily on the first paid compaction; ANY
failure permanently falls back to the main provider for the run, so a
misconfigured summarizer can never break compaction. "main" at any
level forces the old behavior.
- Summarizer input caps. renderTranscript clamps tool-result bodies to
2,000 chars and tool-call arguments to 500 (head+tail, rune-safe) —
the summary needs what was done and decided, not raw payloads. Cuts
summarizer input 5-10x on tool-heavy sessions.
- Free reactive prune. recover() now runs the same prune-first stage as
the proactive path: a context-limit error is first answered by
pruning stale tool bodies at zero cost, without consuming the
one-shot reactive budget; the paid summarizer only runs when a later
error finds nothing left to prune.
- Window re-derivation on model switch. Options.ContextWindowFor
(wired to the model registry in exec and the TUI) lets the loop
re-derive the compaction threshold after escalate_model, closing the
documented KNOWN LIMITATION where a switch to a smaller-window model
could overflow and a larger one over-compacted.
## Rebase notes (2026-08-25, onto 2749447c)
Rebased across 214 commits. main now summarizes through the context planner
(#879) and switches models via turn sessions, so the agent side was reworked
rather than merged textually:
- Summarizer selection lives on compactionState and is resolved lazily per
main model through summarizeWithPlanner (planner + trace preserved); a
failing dedicated summarizer is marked broken and the same slice is
retried once on the main provider.
- Options.Summarizer now receives the main model in force:
`func(ctx, mainModelID string) (Provider, error)`, and may return
(nil, nil) when no dedicated model applies. After a successful
escalate_model switch the loop calls compactor.switchModel, which
re-derives the window and re-resolves the summarizer — a run that started
on the cheap model and escalated regains cheap summaries (review P2).
- recover()'s free prune stage runs before the one-shot reactive guard
(pruning is free and cannot loop) and mirrors maybeCompact's two prune
passes; the free retry is taken only when the pruned history fits the
window the provider enforces (with a 5% estimate margin), otherwise the
same call falls through to the paid summarizer. Gating on the 70%
proactive threshold would have sent histories in the 70-95% band to the
summarizer unnecessarily (review P1 and the follow-up question).
- ContextWindowFor is wired through the turn-session switch path; the
planner's window is updated alongside the compactor's.
Tests: the PR's suite is ported to the new seams, plus insufficient-prune
fall-through, free prune after the budget is spent, window-vs-threshold
gating, and summarizer re-resolution across a model switch.
* fix(agent): branch on a dedicated-summarizer flag instead of comparing providers
Review follow-ups for #532:
- summarizeProvider now reports whether the dedicated summarizer was
selected and summarizeClosure branches on that flag; the previous
`chosen == provider` compared Provider interface values, which panics
when the dynamic type is not comparable. Regression test uses a provider
with a slice field for both the summarizer and the main provider.
- CompactionModelID's curated-default test covers the official OpenAI
profile alongside Anthropic and Google.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
---------
Co-authored-by: Claude Fable 5 <[email protected]>
* fix(tools): make apply_patch tolerant and re-expose edit_file
Head-to-head benchmarking against Grok Build on grok-4.6 (3 tasks × 3 runs)
showed Zero solving every task but spending 2.7× the tokens and 2.3× the
time on the multi-file feature task. Every one of the 20 apply_patch calls
in those runs failed, after which the model degraded to whole-file
write_file rewrites and read→write verification loops.
Root causes and fixes:
- The sandbox and the tool both rejected an absolute path that lies inside
the workspace ("must stay inside the workspace"). Models routinely echo
the absolute path read_file showed them. Absolute paths now flow through
the normal workspace-scope validation (inside → allowed, outside → still
denied); unified-diff headers are rewritten to a/ b/ relative form before
git apply, and platform symlink prefixes (/var → /private/var) are
normalised the same way the other file tools do.
- The structured parser demanded a byte-exact "*** Begin Patch" /
"*** End Patch"; grok-4.6 writes "*** Begin Patch ***". Decorated marker
spellings are accepted, and a unified range after "@@" ("-12,4 +12,6",
optionally with a heading) no longer becomes a literal context search.
- Parse errors now carry a one-line format reminder so a failed call is
recoverable in one retry instead of a format-guessing loop.
- edit_file (exact search/replace with uniqueness guard, CRLF and fuzzy
fallbacks) is advertised to the model again alongside apply_patch, and
the apply_patch description shows the structured format.
- Writers (edit_file, write_file, apply_patch) recorded a file's line total
with lineCount, which is one higher than read_file's count for a file
ending in a newline. RecordSeenRange resets every observation when the
total changes, so a partial read_file after a successful edit silently
discarded whole-file knowledge and the next edit was refused with
"content that has not been read exactly in this session". All writers
now record the same total read_file reports (trackedLineTotal).
- read_file's padded " N | " line prefix was ~19% of every read (about
1.5k tokens per 800-line file) and was re-sent on every later call. It is
now the compact "N→" prefix models already know; nothing parsed the old
form.
- The editing-discipline prompt steers: edit_file for a targeted change,
apply_patch for multi-hunk/multi-file changes, write_file only for new
files or full rewrites; a successful edit result is confirmation, so the
file is not re-read to verify it.
Tests: regression tests for decorated markers, writer/reader line-total
agreement (edit → partial read → edit far away still succeeds), range hunk headers,
absolute in-workspace paths (structured + unified), outside-workspace
rejection, header-line-only relativisation, and tool visibility; the
prompt and tool-budget tests updated to the new contract.
Benchmark (grok-4.6, 3 tasks x 3 runs, harness in demo/bench-zero-vs-grok):
Zero 0.8.0 617 s / 1,527k tokens -> with this change 346 s / 493k tokens;
Grok Build 407 s / 713k. Every run of every configuration solved its task.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): keep CRLF line endings when relativizing patch headers
relativizeUnifiedPatchPaths split on a normalized "\n" and rejoined with
"\n", so any CRLF patch that needed a header rewrite lost its line endings
everywhere; git apply could then fail to match a CRLF file. Split on "\n"
only and carry each line's trailing "\r" through, including on rewritten
headers.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(sandbox): share the structured-patch classifier with apply_patch
Review follow-ups for #956:
- The tool accepted decorated structured markers ("***Begin Patch") that
the sandbox's applyPatchPaths did not recognise, so for that spelling the
boundary scanned the patch as a unified diff, extracted no targets and
validated nothing. The marker classifier now lives in the sandbox package
(StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it,
so both sides accept exactly the same set; tests cover inside, traversal
and outside targets for the no-space spelling at the boundary.
- Document the containment model at the apply site: structured patches are
applied through an opened os.Root (descriptor-relative, no-follow);
unified diffs keep the pre-existing git-apply pathname flow, which the
absolute-path rewrite does not widen (they become the same root-relative
form relative paths always used).
- OpenAI addendum: the one-file-per-call rule names read_file, edit_file and
write_file explicitly and allows a single multi-file apply_patch.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): compare slash-normalised patch paths so the check passes on Windows
structuredPatchHeaderPaths normalises separators to "/", so the absolute
outside-workspace path in the boundary test must be compared in its slash
form; on Windows the backslash original never matched and the Smoke job
failed.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(sandbox): validate the parsed patch path at the scope boundary
For each accepted marker spelling, pass the exact path applyPatchRequestPaths
produced to scope.validate: an outside path must be denied and an inside
path accepted, so a separator-handling regression between the parser and the
scope cannot slip past the test.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): apply unified diffs in-process through the workspace root
Review follow-up for #956. Unified diffs were validated by pathname and then
handed to git apply, which reopened the targets by name — a check-to-use
window a symlink swap could exploit — and the absolute-path rewrite added in
this PR normalised `diff --git` headers with strings.Fields, which splits a
git C-quoted path containing whitespace.
Both go away by not shelling out at all: parseUnifiedPatch translates a
unified diff into the same operations a structured patch produces, and both
formats are applied by applyPatchOperations through os.OpenRoot — every
stat, read, create and write is descriptor-relative and refuses to follow a
link out of the root. git is no longer required by apply_patch.
Unified-diff support: modifications, creations (--- /dev/null), deletions
(+++ /dev/null), multiple hunks per file, git "rename from/to" (a move,
hunks optional) and "copy from/to" (a new copy operation whose destination
inherits only the tracker credit the source had), "\ No newline at end of
file" on either side, a/ b/ prefixes, C-quoted paths, CRLF patches, and
git's diff/index/mode lines. Hunks are placed at their "@@ -a,b" range when
the expected lines match there and located by context otherwise (so a
stale range still applies and duplicate blocks are disambiguated by the
range). Binary patches are rejected.
Removed: the git apply flow, relativizeUnifiedPatchPaths,
recheckPatchWriteTargets, missingPatchTargets, completeCreatedPatchTargets
and recordCreatedPatchTargets. validatePatchPaths/changedFilesFromPatch stay
for mutation-target listing.
Tests: the existing unified-diff suite now runs through the in-process
applier unchanged; new coverage for quoted whitespace paths (inside applies,
outside is refused untouched), a workspace path swapped for a symlink that
escapes the root (both formats refused, outside file unmodified), create +
delete + no-newline markers, range hint vs context fallback, rename with a
hunk, and binary/empty rejection.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): harden unified-diff parsing and cover the copy operation
Review follow-ups for #956 (in-process unified applier):
- A hunk that ends before its declared line counts is reported as
malformed instead of silently absorbing the next file's ---/+++ header
pair (which then failed later against the wrong file). Both shapes are
caught: leftover counts at a header or at end of input, and a --- line
directly followed by +++ inside a hunk. The element after a patch's final
newline is no longer counted as an empty context line, so a truncated
hunk cannot look complete.
- "\ No newline at end of file" may only follow a file's final hunk; a
marker before another hunk is rejected rather than leaking into that
hunk's result.
- Zero-context insertions (git diff -U0) are documented as
position-trusting by design, matching git apply without fuzz; hunks with
context are verified at the range before it is used.
Tests: copy from/to (source kept, destination created with the hunk
applied and tracked with the source's whole-file state; existing
destination refused untouched; copy onto itself refused), over-declared and
truncated hunk counts, and the no-newline marker before vs after the final
hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): require an @@ line before treating ---/+++ inside a hunk as a file boundary
A removed "-- x" line directly followed by an added "++ y" line is valid
hunk content that also matches the ---/+++ header shape; the boundary check
now additionally requires the following line to be a hunk header, and a
regression test keeps such a pair intact. End-of-input strictness for a hunk
cut short is documented as deliberate: applying a truncated hunk would make
a partial change (removals without their replacements).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* refactor(tools): share one line-count implementation between reader and writer
sourceLineCount and trackedLineTotal computed the same count; keeping two
copies invites exactly the reader/writer drift this PR fixed. The reader now
delegates to trackedLineTotal, which carries the tracker contract.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): verify a unified deletion's expected content before removing the file
A unified diff's "+++ /dev/null" hunks state the content being removed, but
the in-process applier dropped them and deleted unconditionally, so a stale
deletion removed a file whose current content the patch never described.
Unified deletions now keep their hunks, require at least one, and run them
through the same verifier as updates: the hunks must match the current file
and leave nothing behind, otherwise the deletion is refused and the file is
untouched. A structured "*** Delete File" keeps its unconditional meaning.
Hardening from the same review: every non-create change re-reads its source
through the opened root immediately before commit and refuses to proceed if
the bytes differ from what was planned.
Tests: matching deletion, stale deletion (file byte-for-byte unchanged),
partial deletion, header-only deletion, multi-hunk deletion with one stale
hunk (nothing removed) and all-good hunks, structured delete unchanged, and
a source changed between planning and commit for update and delete.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): a move with a missing source is refused before publishing
The pre-commit recheck now refuses a move whose source vanished after
planning instead of publishing the destination and then failing; update the
test to the safer contract (error names the recheck, nothing is created).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): byte-exact unified deletions and git's header-only empty-file forms
Review follow-ups for #956:
- A unified deletion is verified byte for byte: the removed lines (with the
removed side's "\ No newline at end of file" honoured) must equal the
current file exactly, with only the file's own line-ending style
normalised. None of the whitespace tolerance used to locate update hunks
applies, and a whitespace-only remainder no longer passes; a deletion is
not recoverable, so it fails closed.
- git describes an empty file's deletion or creation with header lines only
("diff --git", "deleted file mode"/"new file mode", "index") and no
hunks. Both forms are recognised: the deletion is applied only when the
current file is empty, the creation writes an empty file, and they compose
with hunk-carrying files in the same patch.
- The pre-commit recheck test also covers the copy operation and asserts no
destination is created on refusal.
Tests: whitespace/indentation/blank-line/space-only/missing-newline
mismatches all refused with the file unchanged, matching no-newline marker
and CRLF file accepted, header-only deletion of a non-empty file refused,
multi-file patch with header-only delete + create + a normal hunk.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tools): remove the move source after planning via a deterministic hook
The refused-move test now creates source.txt, plans the move, and removes
the file through a test-only pre-commit hook (structuredPatchBeforeCommit)
so it exercises the post-planning disappearance path rather than a source
that was never there. Assertions unchanged: refused before publishing, no
destination created.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* test(tui): read_file fixtures use the compact N→ prefix
Three TUI fixtures still built fake read_file output in the legacy
" N | text" shape; they pass either way but pinned an outdated contract.
They now mirror what appendReadFileRange emits, and the shape comment and
the expanded-card assertion are updated.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
* fix(tools): name the committed files when a patch is partially applied
Review follow-up for #956. When a later change in a multi-file patch fails
(for example the pre-commit recheck refuses a file that changed after
planning), the error now lists exactly which files were already committed
and states that the rest are unchanged, instead of a generic "partially
applied" warning. The model can then re-read only what actually changed.
Test: a three-file structured patch whose second file is altered after
planning fails with "already committed: first.txt", first.txt holds the
patched content, and the other two files are untouched.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
---------
Co-authored-by: Claude Fable 5 <[email protected]>
* fix(sandbox): retire recorded approvals through a policy bump
Ahead of tightening which command prefixes may be approved, make the
upgrade path safe. A grant already on disk that the narrower rules
reject fails validation on read, and that path rejects the entire grants
file rather than the one entry. Every caller treats a read error as "no
grant" (engine.go LookupCommandPrefix, ApprovedCommandPrefixes, and the
Lookup in Evaluate), so one stale prefix would silently disable the
user's persisted deny grants too, with the error text never reaching
them.
Bump the approval policy version instead and let the store's existing
migration handle it: it backs the file up, keeps deny grants,
invalidates prior approvals, and reports the counts once through the
startup notice that both frontends already print. That is what the
mechanism was built for.
The v1-migration test pinned the policy version as a literal, so it now
derives the expected value from the constant and will not need editing
on the next bump.
* fix(sandbox): normalize launcher names before the command-prefix denylist
unsafeCommandPrefixLauncher matched the program name as written, so
python3.11, python2.7, python.exe, node.exe and git.exe all validated as
ordinary commands and could be persisted as command-prefix grants.
Lookup is a string-prefix match, so a single approval then auto-allowed
every later invocation: granting [python3.11 -c] covers
`python3.11 -c "import os; os.system(...)"` with no further prompt.
internal/agent's commandName already normalizes case and Windows
executable extensions for the allow side of the same decision; the deny
side did not, and the two disagreeing is the bug. Reduce the program to
the launcher it actually runs and match on that: executable extension,
Windows' trailing dots and spaces (python. starts python.exe), CPython
ABI flags, then a trailing version, testing the digit-stripped form as
well so python3.11 and python3 both arrive as python. A name is only
ever narrowed toward an existing entry, so the failure direction is an
extra permission prompt, never a silent grant. Digits without a
separator stay part of the name, which keeps base64, 7z and sha256sum
grantable.
Two Windows spellings cannot be resolved by name at all, so the shape is
refused alongside a path: 8.3 short names truncate the stem, which makes
POWERS~1.EXE unrecognizable, and python.exe::$DATA reaches the same
executable through its default stream. Normalizing the first token
before the banned-suggestion comparison keeps git.exe at parity with
git.
The list also gains launchers of the same class it already covers and
that normalization cannot reach: cmd, wsl, the remaining POSIX shells,
busybox, uv and uvx. Entries normalization now covers (/bin/bash,
/bin/zsh, powershell.exe, python3, pypy3) are gone; the first two were
already unreachable behind the path check above them.
Covers the persisted store, the session-scoped grants the agent takes
during a turn, the legacy-schema migration that prunes such a grant
per entry, and the Windows spellings hermetically.
* fix(sandbox): treat free-threaded CPython as a launcher too
The ABI flags came from the pre-3.13 set, so "python3.13t" and
"python3.13t.exe" normalized to themselves and stayed grantable as
command prefixes while the GIL build "python3.13" was refused. A
free-threaded interpreter runs arbitrary code just the same.
Add "t" to the flags trimmed under the existing digit guard, which is
what keeps ordinary names intact: "zstd" and "cat" have no digit beneath
the trimmed letters, so they keep their own names and stay grantable.
Reported by CodeRabbit on #934.
* fix(sandbox): refuse the remaining spellings of listed launchers
Same defect as the free-threaded build, found by sweeping the families
already on the list for names it cannot reach:
- nodejs is node on Debian and Ubuntu, and was grantable.
- python3-dbg, python3.11-dbg and python3.13t-dbg are interpreters that
run arbitrary code; so are bash-static, pwsh-preview and
node-nightly. A closed set of build-channel suffixes now normalizes
them. The set stays closed deliberately: a general "-word" strip would
also swallow node-gyp, python3-config and ruby-lsp, which are ordinary
tools a user should still be able to approve, and those are pinned as
must-stay-grantable.
- pnpm, yarn and bunx run package scripts exactly as npm and npx do, and
npm and npx are already refused.
- sudoedit escalates like sudo.
The launcher-wrapper family (winpty, chroot, unshare, nsenter, parallel,
script, unbuffer, taskset, flock) has the same inconsistency against the
env/nohup/timeout/setsid entries already on the list, and is left for a
separate decision rather than widened here.
* test(sandbox): cover the -debug and -beta launcher suffixes
Four of the six build-channel suffixes had regression cases; -debug and
-beta did not, so removing either from the set left every test green.
Both now fail RED when their entry is dropped.
Reported by CodeRabbit on #934.
deepseek-v4-flash / deepseek-v4-pro expose thinking-mode effort control
through the OpenAI-compatible reasoning_effort parameter (low/high/max;
medium/xhigh map onto high). reasoningEffortsForModelName did not recognize
the deepseek family, so /effort only offered 'auto' for these models.
Advertise {low, high, max} so the /effort picker and Ctrl+T ring work, and
add fallback coverage for deepseek-v4-flash/pro/reasoner/chat.
* feat(update): recognise Homebrew installs and add mise, go install docs
* fix(update): match the keg shape, correct the docs, cover Apply and cross-target
* test(update): build the release fixture from the running platform
* fix(update): require a Homebrew receipt before refusing self-update
Review found the keg check was shape-only, and it was right: a directory
tree a user built themselves can have exactly a keg's shape. Classifying
one as Homebrew refuses a `zero upgrade` that would have worked
perfectly, on a machine Homebrew has never touched.
Two changes, cheap one first. The shape rule now requires the full
<prefix>/Cellar/<formula>/<version>/bin/<binary> depth rather than one
segment fewer, which already excludes the exact path the review raised.
Then, only for a path that still looks like a keg, one Stat for
INSTALL_RECEIPT.json — the file Homebrew writes into every keg and
nothing else does.
The receipt is what the old comment argued against, on the grounds that
it would put filesystem I/O on a path that runs at every version check.
That is no longer true of it: the Stat happens only after the shape test
has already matched, so an ordinary install still reaches no filesystem
here at all.
A directory named INSTALL_RECEIPT.json is not a receipt, which a bare
Stat would have accepted silently.
NOT taken: the same review asked for the Homebrew branch to be moved
ahead of the cross-target one in upgradeGuidance. That ordering is
deliberate and already pinned by
TestUpgradeGuidanceKeepsCrossTargetAnswerForHomebrew, whose comment
predicted exactly this reading — "it reads like an ordering bug until you
see which question is being answered". Homebrew is a property of the
binary on THIS machine; when the check was asked about a different
target the answer belongs to that other machine, and `brew upgrade zero`
would change this one instead. The reasoning is now recorded at the
branch as well as in the test, so the next reader finds it in both
places.
The install docs claimed detection was the Cellar path alone, which
undersold it, and stated the self-update rule absolutely in one paragraph
before excepting mise in the next. Both say the same thing now.
* test(update): give the Homebrew fixtures a receipt, and stop gating the receipt rule on the host
The previous commit made a receipt the thing that distinguishes a keg from any
directory sitting under a path containing "Cellar". Two fixtures still built a
keg without one, so detection correctly answered standalone and both failed:
TestDetectInstallMethodRecognisesAHomebrewKeg keg path: got "standalone", want "homebrew"
TestApplyRefusesToUpdateAHomebrewKeg refusal does not name the command that works:
download release archive: dial tcp: lookup example.test
The second is worth reading twice: with detection returning standalone, Apply
stopped refusing and went to the network, so the failure surfaced as a DNS error
rather than as a missing refusal. Both fixtures write a receipt now.
The reason this reached CI at all is the second half. Every test of the receipt
rule passed runtime.GOOS to isHomebrewPath, which already takes the target as a
parameter, so all of them skipped on Windows and the rule could only ever break
somewhere else. They pass an explicit target now and run everywhere. The two
that exercise DetectInstallMethod stay host-gated, because those are about the
wired entry point rather than the rule, but the rule underneath them is no
longer invisible here.
Verified the receipt path arithmetic on both non-Windows targets rather than
trusting the host-gated tests: isHomebrewPath is false before the receipt is
written and true after, with the receipt at the keg directory hasHomebrewReceipt
actually reads.
* feat: use Exa as default search provider
Amp-Thread-ID: https://ampcode.com/threads/T-01a01522-a5f1-7139-8a55-5e417cba2751
Co-authored-by: Amp <[email protected]>
* fix(config): carry a legacy default's disable across the Exa migration
A user who ran `zero mcp disable firecrawl` chose not to open that outbound
connection. Renaming the built-in default to exa re-opened it under the new
name on upgrade, and because the replacement looked like an untouched default
the startup warning stayed quiet too.
Carry an explicit user-level disable of a retired default onto its successor,
but only when the user never declared the successor themselves — an explicit
exa entry wins either way. The carried-over disable is recorded as a user-level
decision, so the lower-trust project layer cannot lift it while
`zero mcp enable exa` still can.
Regression coverage: the legacy-disabled upgrade path, the explicit-exa
override, a configured-but-enabled legacy entry, the CLI re-enable, the
project-layer re-enable attempt, and the resulting startup behavior
(NormalizeConfig must not hand back an exa server to connect to).
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
* fix(config): keep a retired default's transport for partial user entries
A user who added an API key to the firecrawl default wrote only the header —
the seeded default supplied the http type and URL. Once the default stops being
seeded that entry resolves with no type, url, or command, and NormalizeConfig
rejects it, taking down startup for every other server too rather than just
that one.
Record what Zero used to ship for a retired default and re-seed it under the
user's own fields. Only for an entry that named no transport of its own: an
entry with a self-hosted url or a stdio command is a complete definition the
user owns, and grafting the retired http default onto a command produces a
server that is rejected outright.
Regression coverage at both ResolveMCP and NormalizeConfig for the header-only
entry and its failure path, the self-hosted stdio entry that must be left
alone, and a disabled retired entry keeping a usable transport so a later
re-enable resolves.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
---------
Co-authored-by: Amp <[email protected]>
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
* Fix LM studio (local) model discovery.
* fix(openai): handle typed-nil properties in normalizeToolParameters
The `properties == nil` interface comparison misses typed-nil
`map[string]any` values, which JSON-serialize as `null` instead of
`{}`. Strict OpenAI-compatible servers (LM Studio) reject the request.
Type-assert first, then compare the underlying map, so a typed-nil
properties field is rewritten to an empty object. Adds a regression
test that exercises the typed-nil path.
* test(openai): cover non-map properties in normalizeToolParameters
CodeRabbit follow-up on the typed-nil fix: lock in the behavior
when `properties` is a non-map value (e.g., empty slice) so the
type-assert branch normalizes it to an empty object on the wire.
* fix(acp): four defects a desktop ACP client hits
Found by driving `zero acp` from ZeroApp and comparing both sides of the
wire. None of them fail loudly — they produce a blank transcript, a
silent denial, a spurious crash, or two buttons nobody can tell apart.
session/load restored the conversation into the agent's own memory and
sent nothing. The turns were read from the store and used to build every
later prompt, but not one notification went out, so a client that resumed
opened an empty transcript against a model that remembered everything.
Ask a follow-up and ZERO answers correctly, about a question no longer on
screen. It now replays the loaded turns as user/agent message chunks
before returning, so a client rendering on the reply already has them.
Whole messages rather than per-token deltas: this is a transcript, not a
stream.
A permission option we offered could not be accepted. The fallback list
for a request with no AvailableDecisions lived inside
buildPermissionOptions, while requestPermission validated the client's
answer against the raw field — so whenever ZERO did not enumerate, the
client was sent Allow and Reject and its reply was checked against an
empty slice. Every button it could possibly show failed closed to deny
with "permission option was not offered". The user clicked Allow and ZERO
recorded a denial, and nothing surfaced it, because a denial is a
legitimate answer. One resolver now feeds both, so they agree by
construction. The narrowing this validation exists for is unchanged: an
unoffered broader grant is still refused.
Cancelling a permission prompt came back as an internal error. Only
context.Canceled was recognised, so errPermissionApprovalCanceled fell
through to -32603 carrying the sentinel text, and clients render that as
a failed turn — declining a tool looked like ZERO falling over. For
apply_patch, dismissing the dialog is the only refusal a client is
offered. It maps to StopCancelled now. The sentinel is exported so the
surfaces can recognise it; a genuine failure is still an error.
request_permissions labelled two different decisions "Allow". The label
is the only thing an ACP client shows, and plain allow and strict-review
allow are offered together, so the panel had two identical buttons where
one silently enabled strict auto-review of what was granted. The
distinction cannot live in the option id, which is opaque to the client,
so it is in the name.
Eleven tests, each confirmed to fail without its fix.
* refactor(acp): drop the session/load replay, superseded by #914
gnanam's #914 lands the same fix and does more with it: the replayed
messages carry stable ids derived from the store's event ids, session/load
replays while session/resume deliberately does not, and both are
capability-gated. Keeping a second, weaker replay here would have meant a
conflict in handleSessionLoad and translate.go for no gain.
translate.go is back to its state on main. What remains are the three
defects #914 does not touch: the permission option that could not be
accepted, the cancel that arrived as an internal error, and the two
options labelled the same.
* fix(acp): validate a permission reply against the options that were sent
CodeRabbit caught a hole on the same seam this PR already fixes, in the
other direction.
buildPermissionOptions drops PermissionDecisionCancel, because ACP
expresses cancellation through the outcome rather than an option. But
ZERO enumerates cancel in AvailableDecisions for shell commands and
apply_patch — the two commonest prompts — so validating against the
decisions accepted {"outcome":"selected","optionId":"cancel"}: an
identifier no client was ever shown, aborting the whole turn.
Rather than reject cancel by name, decisionFromOutcome now takes the
[]PermissionOption that were actually sent, and requestPermission builds
that list once and uses it for both halves. "What we offered" and "what we
accept" become the same slice instead of two things kept in step, so
anything optionKindFor declines to render is excluded structurally — the
next such action is covered the day it is added.
The regression test is written over every option-less decision rather
than over cancel specifically, for the same reason. Cancelling through
the outcome still works, which is asserted separately: closing the
selected-id route must not close the real one.
Also adds the cancellation coverage CodeRabbit asked for on the two
sandbox-retry paths. Both wrap ErrPermissionApprovalCanceled and each is
reached by a different denial, and the ACP mapping in this PR depends on
them producing something recognisable. They also assert the escalated
retry does not run: a cancelled prompt that still ran it would run the
command the user had just declined.
* docs(agents): Record four recurring review blockers
These classes come up across unrelated pull requests, are not caught by CI,
and nothing in AGENTS.md currently prevents them, so each is rediscovered by a
reviewer at the cost of a round trip.
Host tool version floors is the clearest case. Zero shells out to git and the
project declares no minimum version anywhere, so adding a modern flag raises
that floor silently and an older host fails with a usage error instead of a
clear message. Prove-the-test-fails addresses a recurring shape where the
problem is not a missing test but one that passes either way, because an
earlier guard rejects the input before the behavior under test runs. Hermetic
tests covers isolation helpers applied to some tests but not others reaching
the same storage. Platform error codes extends the existing platform-truth
rule, which covers path canonicalization but not errno and NTSTATUS values.
Advisory lint findings are deliberately not included: deadcode and lint-static
both run with continue-on-error in ci.yml, and section 3 already requires
fixing findings the change introduces.
Refs #898
* docs(agents): Tighten version-floor, isolation, and errno guidance
Three corrections from review, two of them to claims that were too strong.
Git version floors are per subcommand, not per flag. --end-of-options reached
the shared parser in 2.24 but git rev-parse only accepted it from 2.30, so
recording the flag's general debut would still let a caller ship something
that fails on 2.24 through 2.29.
The hermetic-tests bullet named config, cache, and state but gave a procedure
covering only config. It now covers each root, including %LocalAppData% for
the cache root, which os.UserCacheDir resolves without consulting XDG.
The errno bullet asserted ENOTDIR on Linux and Darwin. Only Linux was actually
measured; Darwin was assumed, in a bullet whose whole point is not to assume.
It now says the call may report either code and both should be accepted, which
is what the guidance was trying to teach.
Refs #898
* docs(agents): Clarify macOS and Windows roots for hermetic test isolation.
os.UserConfigDir and os.UserCacheDir derive from HOME on macOS and AppData/LocalAppData on Windows rather than reading XDG variables.
Refs #899
---------
Co-authored-by: euxaristia <[email protected]>
* Checkpoint UI polish
* Fix structured apply patch handling
* Restore tool previews when resuming sessions
* Improve theme-aware code rendering
* Replace persistent sidebar with run details
* Remove inert transcript interactions
* Cover resumed edit previews
* Smooth active run status feedback
* Preserve semantic colors behind overlays
* Normalize ChatGPT model aliases
* Render staged image attachments in terminal
* Polish transcript and tool feedback
* Resolve rebased diff renderer conflict
* Address UI polish review findings
* Fix smoke test regressions
* Harden patch and transcript edge cases
* Bind structured patches to workspace root
* Use unique structured patch temp files
* Retry structured patch temp allocation
* Reuse rooted patch temp allocator
* Fix Windows CI process checks
* Fix structured patch safety boundaries
Preserve read-before-write credit only when the model already knew the full source, reject ambiguous hunk matches, and retain CRLF line endings. Add regression coverage for unsafe overwrites, positive tracking cases, duplicate matches, and Windows-style files.
* Resolve plan test seam after rebase
Drop the obsolete sticky-plan height helper while retaining the PR status test seams brought forward by the UI changes.
* Address final review correctness gaps
Bound narrow waiting labels, reject malformed plan records, preserve missing final newlines, reject out-of-order EOF hunks, clear stale tracking after failed rollback, and keep deferred tool loading aligned with model visibility.
* Harden patch transactions and review edges
Publish new patch targets without overwriting raced destinations, journal only completed mutations, and create replacement temps beside their targets. Reject pre-plan diagnostics, cover tool-search transcript suppression, and leave sufficient cleanup time in the Windows process fixture.
* Bind patch rollback to committed versions
Capture file identity and content before publication, quarantine rollback targets atomically, and refuse destructive undo when a path was replaced or modified in place. Add deterministic coverage for publication and rollback races.
* Align structured patch failure semantics
Use prevalidated sequential application with explicit partial-success errors instead of unsafe multi-file rollback. Retain atomic per-file publication, no-overwrite creates, unsupported-hardlink fallback, and conservative tracker invalidation.
* Preserve patch context bytes
Track source offsets for structured-patch context lines and reuse the exact matched source text during replacement. This keeps tolerant matching from changing indentation or mixed line endings outside the requested edit.
* chore: remove dead wrapper functions and unused test seams
These five functions are unreachable from any cmd/* main even when tests are included (deadcode -test ./...), so they are genuinely dead on every platform:
- TraceEventKeys: one-line alias of ParseTraceEventKeys, no callers
- ValidateToolCapabilities: one-line alias of ValidateCapabilities, no callers
- GetLocalDiffStats and WatchPRState: test seams in internal/tui/export_test.go that no test calls
- ScanSSEData: non-context SSE scanner in internal/providers/providerio, fully superseded by ScanSSEDataWithContext
The five remaining -test findings are //go:build !linux stubs (landlock/seccomp/linux_helper) that are live on Linux, so they are left in place.
Co-Authored-By: cairn-code <[email protected]>
* Bump Go to 1.26.6 so govulncheck and Windows smoke pass.
The 1.26.5 standard library still reports GO-2026-6218 and the Punycode
label check that fail required CI; 1.26.6 matches main and clears them.
---------
Co-authored-by: euxaristia <[email protected]>
Co-authored-by: cairn-code <[email protected]>
* feat(zerogit): auto-create a conventional branch before push/pr on default branch
zero changes push/pr refused to push straight to the default branch but
never offered an alternative, unlike other agent tooling that names and
checks out a feature branch automatically. Add CreateBranch, IsDefaultBranch,
CurrentGitUser, and branch-name slugify/build helpers to zerogit, and wire
push/pr to auto-create a "<user>/<slug>" branch (slug from an LLM when a
provider is configured, otherwise a deterministic fallback from the diff)
whenever the current branch is the default one and --yes/--dry-run weren't
passed.
* fix(zerogit): normalize LLM branch slugs, bound remote lookup, handle existing branches
Address CodeRabbit's review on the auto-branch-naming PR:
- generateAutoBranchSlug now takes the first non-empty, quote-trimmed
line of the model's response before slugifying, instead of folding
any preamble or wrapping quotes into the branch name.
- isDefaultBranch's ls-remote lookup is now bounded by a 5s timeout so
a slow or unreachable remote can't stall push/pr; falls back to the
local main/master heuristic as before.
- CreateBranch checks whether the target branch already exists locally
and checks it out instead of failing checkout -b on the collision.
Also add regression coverage: CurrentGitUser's OS-username fallback
tier, CreateBranch's existing-branch and generic checkout-failure
paths, a CLI-level test for changes pr (previously untested) mirroring
the existing changes push coverage, and a messy-LLM-response case for
the slug normalization fix.
* fix(zerogit,cli): make auto-branching collision-safe, remote-aware, fail-closed, and opt-in for LLM naming
- CreateBranch never checks out an existing branch on a name collision: it
picks a unique suffixed name off the current HEAD (name-2 .. name-9) and
fails visibly when the namespace is exhausted, so a stale branch with
unrelated history can no longer be published while the new commit stays
behind on the default branch.
- ensureFeatureBranch resolves the remote once (explicit --remote, then the
original branch's configured upstream, then origin) via IsDefaultBranch,
which now reports the resolved remote, and push/pr thread that remote into
Push, so a freshly created branch without tracking configuration no longer
falls back to origin in fork setups.
- The default-branch check fails closed: main and master count as default
with no network, an unreachable remote falls back to the local
refs/remotes/<remote>/HEAD record, and when neither answers the push is
refused with guidance instead of silently dropping the confirmation guard
for repositories whose default is trunk or develop.
- LLM branch naming is now opt-in via --auto on push/pr, matching commit
--auto: a configured provider alone no longer causes the change diff to be
uploaded during ordinary git-only pushes.
- After the ordinary commit-then-push sequence the tree is clean, so the
fallback name comes from the HEAD commit subject instead of the constant
user/changes.
* style(zerogit): align fake-runner result comments to gofmt output
* test(zerogit): cover Push's fail-closed path when the default branch is unknown
* fix(zerogit): keep a stale cached remote HEAD from clearing the push guard
When the live ls-remote default-branch lookup fails, isDefaultBranch fell
back to the local refs/remotes/<remote>/HEAD cache and returned
branch == cachedName. That cache is only a hint: if the server renamed its
default (main -> trunk) the record still names main, so checking whether
pushing trunk is safe returned false and the newly protected branch was
pushed without --yes.
Trust the cache only to block a push (a positive match proves the branch is
the recorded default); on a mismatch fall through to the existing
fail-closed unknown-default error instead of treating the branch as
unprotected.
* fix(cli): honor --diff-bytes when inspecting the diff for auto-branch naming
changes push/pr accept and document --diff-bytes, but ensureFeatureBranch
inspected the working tree with only Cwd set. With --auto the resulting
unbounded diff was embedded in the branch-name request to the provider, so a
user who capped the diff to limit how much proprietary source is uploaded
for naming still sent the complete diff.
Thread options.maxDiffBytes through ensureFeatureBranch into InspectOptions,
matching how the commit path already bounds the diff.
* fix(cli): refuse an auto-branch push when HEAD has nothing to publish
The auto-branch path decided to branch solely from working-tree status and
never checked that HEAD was ahead of the default branch. On a clean,
up-to-date default branch it would create and push user/<subject> at the
exact default tip; with only uncommitted edits it named a branch from those
edits but pushed the unchanged HEAD, leaving the edits local, and changes pr
then abandoned the empty branch before the host rejected the comparison.
Add zerogit.CommitsAhead and consult it before branching: when HEAD is
provably not ahead of <remote>/<default>, report "no changes to publish"
instead of creating the branch. An indeterminate count (for example a
remote-tracking ref that was never fetched) is treated as "cannot tell" so a
legitimate first push is not blocked.
* fix(cli): recover the branch slug from preamble or fenced LLM replies
The slug helper returned the first non-empty line verbatim, so a reply like
"Here is a suggested branch name:\nadd-login-page" produced
user/here-is-a-suggested-branch-name, and a reply wrapped in a code fence
slugified the ``` line to nothing and silently fell back to the local name.
Drop Markdown code-fence lines and prefer a line that already reads as a
kebab-case slug, falling back to the first quoted/plain line only when no
slug-shaped line exists. Cover preamble and fenced responses with tests.
* fix(zerogit): harden auto-branch remote handling
Three review findings:
- isDefaultBranch passed the remote to ls-remote before the HEAD
positional with no --, so a remote value shaped like an option
(--upload-pack=/bin/echo) was parsed as one. Terminate options with --
and cover a dash-prefixed remote.
- An unborn remote (freshly created, zero refs) answered ls-remote with
empty output, fell through to the fail-closed unknown-default error,
and made the very first feature-branch push a --yes dead end that
git remote set-head --auto cannot repair. ls-remote succeeding with no
refs now counts as proof there is no protected default; main/master
stay guarded by the name heuristic and every failure path stays
fail-closed.
- CreateBranch probed only local refs/heads for collisions, so a branch
existing only on the target remote (an old merged-PR branch) was
silently fast-forwarded by the later push -u. The target remote's
heads are now probed once (bounded, with --) and count as taken;
an unreachable remote fails visibly before anything is created.
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(zerogit): close branch-naming and push safety gaps from review
Require a confirmed unborn HEAD, not just an empty ls-remote --symref,
before granting the first-push exception, since a dangling remote
HEAD looked the same. Resolve branch naming and ahead-count checks
against the remote branch instead of a working-tree snapshot, which
could describe uncommitted edits a commit-only push would never
publish. Guard concurrent branch creation with a force-with-lease
push. Scope the ls-remote timeout to the default-branch preflight
only, so it no longer caps Push's own unrelated check.
* fix(cli): require a clean tree before auto-branch push/pr
Refuse ensureFeatureBranch when the working tree is dirty or the
ahead count against the remote default cannot be determined, so a
default-branch push cannot leave uncommitted edits behind or publish
an empty comparison under a guessed name.
* fix(zerogit): drop fixed five-second remote lookup timeout
IsDefaultBranch and CreateBranch applied a hard 5s bound on ls-remote,
so ordinary feature-branch pushes and the collision probe failed on slow
but reachable remotes (SSH/VPN) before Push could run. Honor the
caller's context instead; callers that need a bound pass a deadline.
* fix(cli): bypass the ahead-count and diff-base checks on a confirmed-unborn remote
jatmn flagged that the confirmed-unborn-remote exception this PR added to
IsDefaultBranch never actually helps the main/master case it was written
for: when the local branch is named main, isDefaultBranch returns true from
the name heuristic before it ever consults the remote, so
ensureFeatureBranch still unconditionally calls commitsAhead against
<remote>/<branch>. On a genuinely empty remote that ref cannot exist, so
`zero changes push`/`pr` from a local default branch against a brand-new
empty remote still dead-ended with "cannot determine whether HEAD is
ahead of origin/main" - the exact bug this PR set out to fix.
Tracing the flow further, the same problem also hit the diff-base Inspect
call right after: it uses the same nonexistent <remote>/<branch> ref, so
bypassing only commitsAhead would have left the dead end one line later.
ensureFeatureBranch now probes the new zerogit.IsUnbornRemote (ls-remote
--heads on the remote) whenever commitsAhead fails, and only bypasses both
checks when that probe positively confirms the remote has no refs at all.
An unreachable remote, or one that answers with refs, still fails closed
exactly as before - the fail-closed "unknown default branch" behavior is
unchanged and covered by TestEnsureFeatureBranchFailsWhenAheadCountUnknown
and the new TestEnsureFeatureBranchFailsWhenUnbornCheckErrors.
TestEnsureFeatureBranchCreatesBranchOnConfirmedUnbornRemote is the
regression test: confirmed failing (with the exact dead-end error) against
the pre-fix logic, passing after.
* fix(zerogit): resolve main/master against the live remote default first
isDefaultBranch short-circuited on the literal branch names main/master
before ever consulting the remote. A repository whose remote default is
genuinely something else (e.g. trunk) could still have a local main branch
tracking that remote, and the shortcut wrongly treated it as protected
instead of trusting the live symref result. The conventional-name check
now only applies as a last-resort fallback, after both the live ls-remote
symref lookup and the local remotes/<remote>/HEAD cache fail to answer.
* fix(cli): preserve push lease on retry and check the live default ref
Two related gaps in ensureFeatureBranch's auto-branch preflight:
- When an auto-branch push loses a force-with-lease race against a
concurrent creator, the generated branch is left checked out locally
with no successful push behind it. A retry took the "already off the
default branch" early return and dropped RequireNewRemoteBranch,
letting the next push silently fast-forward whatever the concurrent
creator published. The lease now stays required unless the branch
already has a configured upstream (proof a push already landed).
- The ahead-of-default check compared HEAD against the local
remote-tracking ref, which is only refreshed at clone or the last
fetch. A stale ref can under-report how far ahead HEAD is, or say
zero when the remote has actually moved past it, so ensureFeatureBranch
now refreshes that tracking ref before trusting it, and fails closed
if the refresh itself cannot be confirmed.
Adds regression coverage for both: a retry after a lease collision keeps
requiring the lease (and an already-published branch does not), and a
stale tracking ref is refreshed before the ahead check runs (with a
fail-closed case when that refresh cannot be confirmed).
* fix(cli): track generated branches, reject PR on unborn remote, classify main on unborn
* fix(zerogit,cli): preserve explicit nonexistence lease on force, propagate marker write errors, check unborn remote before branch switch
* fix(zerogit): preserve non-existence lease on target remote and roll back failed branch marking
Compare target remote against configured upstream remote to retain non-existence lease on target, roll back created branch if marking fails, and allow --yes to bypass PR default-branch preflight.
Refs #671
* fix(cli): address review findings for auto feature branch creation
* fix(cli): refine LLM preamble candidate slug extraction and enforce unborn-remote guard
* fix(cli): restore default branch after auto-branch and resolve --yes upstream
Do not leave auto-branch commits on the protected local default branch,
and resolve the real upstream for the unborn-remote guard under --yes
instead of always probing origin.
* test(cli): assert auto-branch passes capped diff into the provider
Thread --auto and a mock provider through DiffBytes coverage so the
cap is checked on the real LLM request path, not only Inspect.
* fix(cli): block auto-branch on unborn remotes and fix lease/restore
Refuse auto-creating a feature branch when the target remote has no refs yet.
Publishing only user/slug leaves remote HEAD dangling, so the next changes pr
fails closed and a later main push has no PR diff. Require push --yes first.
Keep the nonexistence lease unless branch@{upstream} is exactly the target
remote/generated-branch ref from a successful push -u. Inherited origin/main
from branch.autoSetupMerge=inherit must not drop the lease.
Restore the original default branch against its own upstream tip, not the
push destination, so --remote upstream does not rewrite a fork main that
tracks origin/main.
* fix(cli): recover push -u upstream and filter LLM slug preambles
Push -u can publish a remote branch then fail to write local upstream
config. Treat that as incomplete, recover with set-upstream-to, and probe
the remote before reasserting the nonexistence lease on retry. Also skip
one-word LLM acknowledgements before accepting a slug-shaped line.
Refs #671
* fix(cli): decide the branch-collision lease with one live remote check
Replace the local-state inference chain (upstream config, a
zeroAutoBranch marker, per-remote provenance tracking) that kept
producing new collision-safety edge cases across many review rounds
with a single live remoteHasBranch check against the destination
remote: missing there gets the nonexistence lease, already there gets
a plain push, and git's own fast-forward check is the safety net if
that existing branch isn't actually ours.
Removes MarkGeneratedBranch/IsGeneratedBranch, now dead, along with
the tests that pinned the specific local-state heuristics rather than
the collision-safety property itself.
* fix(cli,zerogit): address CodeRabbit findings on auto-branch naming
Recover trailing-punctuation LLM slugs, notice deterministic fallback,
report dual restore/rollback failures, and skip Push upstream repair on
dry-run. Align comments with fail-closed and DryRun behavior.
Refs #671
* fix(tools): set explicit stdin reader and use resolved binary for format-on-write
Use the binary path resolved by exec.LookPath and set formatter.Stdin
to an empty reader so format-on-write does not hang on inherited stdin
handles on Windows.
Refs #671
* fix(cli,zerogit,tools): address CodeRabbit review findings on PR 671
Remove redundant command validation, eliminate duplicate remote lookup in PR
workflow, harden ref options and branch switching, isolate test Git
environment, and add formatter lookup test.
Refs #671
* fix(cli,zerogit): address new CodeRabbit review findings on PR 671
Harden CreateBranch name validation, remove unreachable workflow error check,
complete doc comment, isolate test Git environment, and clean up unused
appDeps field.
Refs #671
* fix(cli,zerogit,tools): address third round of CodeRabbit review findings on PR 671
Use featureBranchOptions struct for ensureFeatureBranch, bound preflight
contexts, add upstream repair mismatch subtest, and centralize test Git
config isolation.
Refs #671
* fix(cli,zerogit): finish ensureFeatureBranch doc contract and close review gaps
The ensureFeatureBranch doc comment ended mid-sentence and sat on the featureBranchOptions type, so go doc attached function prose to the struct. Move it above the function and state which deps are required non-nil (inspectChanges, commitsAhead, headCommitSubject, currentGitUser, createBranch) versus optional and nil-guarded (refreshTrackingRef, isUnbornRemote, branchUpstreamRef, resetBranchRef, deleteBranch). Also extract the branch slug filename with path.Base because git paths are always slash-separated, gate two tests on the git version their fixtures need (autoSetupMerge=inherit requires 2.35, init --bare -b requires 2.28), and cover the literal "user" CurrentGitUser fallback via a test seam.
Refs #671
Co-Authored-By: cairn-code <[email protected]>
* Protect auto-branch publication and default ref restoration
Co-Authored-By: cairn-code <[email protected]>
* fix(cli): wire expected-old restore through the dependency seam
The P1 fix added expectedOld to resetBranchRef but left the appDeps field and the workflows.go import behind, so the CLI package did not compile. Update the field type to the 5-argument signature and restore the filepath import used by worktree release resolution.
Co-Authored-By: cairn-code <[email protected]>
* fix(cli,zerogit): wire CAS tip and close review test gaps
Addresses the CodeRabbit round on 93e3448e: fillAppDeps now defaults currentBranchTip so the compare-and-swap old value actually reaches ResetBranchRef in production; TestEnsureFeatureBranchRestoresDefaultBranchAfterCreate asserts the CAS value; the real-git auto-branch test asserts local main is restored to its upstream tip; docs explain the deliberate context.Background push, CurrentBranchTip semantics, the RemoteHasBranch tip-equality ownership check, and the Git 2.24 requirement for CommitsAhead; CreateBranch gains name-validation subtests.
Co-Authored-By: cairn-code <[email protected]>
* fix(zerogit): make RemoteHasBranch report existence only, not tip equality
RemoteHasBranch's tip-equality check made ensureFeatureBranch reassert the
nonexistence lease (--force-with-lease=<branch>:) on every push after the
first: a second local commit puts HEAD ahead of the just-published remote
tip, so the check reported false for a branch that plainly exists, and Git
rejected the follow-up push with a stale-info error instead of a normal
fast-forward.
The concurrent-creator race the tip check was meant to guard was already
handled one layer up: ensureFeatureBranch drops to a plain push when the
branch exists and relies on Git's own fast-forward check as the safety net
(see TestRunChangesPushDropsLeaseWhenBranchExistsOnRemote), so a race
survivor with unrelated history is rejected there regardless of tip
equality here.
Add TestRemoteHasBranchReportsTrueForDivergedTip, which fails under the old
tip-equality check and passes now.
Refs #671
* fix(cli,zerogit): preserve the generated branch on a compare-and-swap conflict
After auto-branching, ensureFeatureBranch restores the default branch to its
own upstream tip using the compare-and-swap form of update-ref. When that
refused because the default branch had moved concurrently, the failure was
indistinguishable from an ordinary update-ref write error, so the rollback
path ran and deleted the freshly created feature branch. That branch
exclusively owns the user's commits at that point, so a lost race destroyed
work in order to "recover" from it.
Add ErrCompareAndSwapConflict, returned by ResetBranchRef only when an
expected-old value was supplied (the sole way git refuses is a tip mismatch).
ensureFeatureBranch now recognizes it and returns without rolling back,
reporting that the branch was preserved so the user can reconcile the default
branch by hand.
Extend TestResetBranchRefRefusesConcurrentDefaultAdvance, which already drove
a real concurrent advance, to require the sentinel. Add
TestEnsureFeatureBranchPreservesBranchOnCompareAndSwapConflict, which fails
with "must not delete the generated branch" when the new branch is removed.
Refs #671
* Address review feedback for auto-branch workflow.
Refresh the exact configured upstream ref before branching and restoring, guard direct URL and path remote destinations from tracking-ref assertion, and remove undeclared --end-of-options from CommitsAhead.
Refs #671
* fix(zerogit,cli): resolve reviewer feedback on branch restoration and remote handling
Update CommitsAhead and ResetBranchRef to safely terminate option parsing with --, inspect all linked worktrees before ref updates, resolve advertised branches on direct path and URL remotes, and query git configuration directly for tracking remote and merge branch without splitting on slashes.
Refs #671
* fix(zerogit): invoke rev-list without positional delimiter to treat revs correctly
Remove premature option terminator from rev-list command invocation while validating base ref against option-like prefixes.
Refs #671
* fix(cli,zerogit): resolve restore tip commit and enforce push remote unborn policy
Resolve restore tip commits via ResolveRemoteBranchTip for named and direct URL/path upstreams to eliminate bare branch fallback, enforce unborn destination policy on push remotes before branch creation, and extract resolveBranchPushPlan to isolate decision logic from repository mutation.
Refs #671
* fix(cli,zerogit): address review feedback on destination remote and upstream matching
Forward resolved pushRemote to branchPushPlan, update HasUpstream to support multi-segment remote upstream references, clarify IsNamedRemote comment, and extend CLI argument parsing tests.
Refs #671
* docs(cli): complete ensureFeatureBranch documentation and wire branchUpstreamRemoteAndMerge
Refs #671
---------
Co-authored-by: Claude Fable 5 <[email protected]>
Co-authored-by: euxaristia <[email protected]>
Co-authored-by: cairn-code <[email protected]>
Three separate changes hit this in a fortnight, each written by someone
being careful about exactly the thing that got them.
A transform that removes bytes without leaving a gap is also a
reassembler. Redaction that matches by shape, run before a sanitizer that
strips control bytes, lets a credential split by a NUL or an ESC pass the
patterns as two fragments and be rejoined on the way out: the MCP failure
reason in #835, and the imported-transcript chokepoint in #878. The path
form of the same mistake is comparing where a handle landed against a
value produced by the same resolver the kernel just used, so a redirect
agrees with itself: the ACL guard in #808, where junctions were caught
only by an accident of Go's mode bits and directory symlinks were not
caught at all.
The unsplit value passing is what makes it survive review, so the note
says the test needs a split case.
* fix(dictation): redact API keys from streaming transcriber errors
Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into
websocket URLs/headers and echoed unredacted transport errors and
server-side error payloads. Wrap those error endpoints with
providerio.Redact and switch the connect-error format specifier from %w
to %s so the raw error cannot leak via errors.Unwrap().
Adds regression tests asserting the API key is not present in the
returned stream error for both transcribers.
Co-authored-by: euxaristia <[email protected]>
* fixup(dictation): address review: redact session/write errors, move redaction tests
- Redact API key in OpenAI Realtime session configuration error.
- Return redacted OpenAI Realtime write error from writeErrCh instead of discarding.
- Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and
TestDeepgramStreamTranscribeErrorRedaction next to their source files.
Refs #710
* fixup(dictation): sync Deepgram redaction test on CloseStream before close
Address CodeRabbit nitpick: the server now drains the client's audio frames and
waits for CloseStream before rejecting the connection with an API-key-bearing
policy-violation close reason. This ensures StreamTranscribe observes the close
reason (and redacts it) instead of failing earlier on a write error.
Refs #710
* fix(dictation): preserve context.Canceled through streaming error redaction
Formatting the read error with %s dropped the unwrap chain, so an Esc
abort of a Deepgram or OpenAI Realtime dictation no longer matched
errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious
error toast on top of the cancellation notice. Return the cancellation
sentinel itself before redacting — it carries no key — and keep the
redacted flat string for genuine failures. Adds regression tests that
cancel a live stream and assert the sentinel survives.
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(dictation): close remaining cancellation and redaction gaps
Key cancellation detection on ctx.Err() instead of
errors.Is(err, context.Canceled), which was racing against
writer-goroutine transport errors that could overwrite the error
value right before the check. Applied consistently across both
transcriber implementations. Fix goroutine leaks and a possible hang
in the cancellation regression tests.
* test(dictation): cover dial and startup cancel identity
Add regression tests for Esc-abort during WebSocket dial and right after
accept (session update / first write) so those redaction sites keep
returning context.Canceled for the TUI errors.Is check.
* fix(dictation): prefer cancel over a racing OpenAI realtime error
Esc can cancel after conn.Read already has an error frame. Check ctx.Err()
after partial callbacks and on the realtimeError path so the stream still
returns context.Canceled instead of a redacted failure notice.
Refs Gitlawb/zero#710
* fix(dictation): prevent auto-submit on cancelled realtime race
handleDictationTranscribed suppressed context.Canceled but then fell
through to the msg.submit branch. A delta -> Esc -> already-buffered
realtime error race returns a nonempty transcript alongside
context.Canceled, so with stt.autoSubmit on, Esc could submit the
composer's restored pre-existing text instead of doing nothing.
Treat a canceled result as terminal before the auto-submit path is
reached, and add a TUI-level test covering the race.
* test(dictation): exercise writeErrCh cancellation branch in realtime transcriber
* fix(dictation): add custom key header redaction and single-pass redaction tests
* fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test
* fix(dictation): initialize non-zero session ID and filter stale session 0 messages
Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio.
Refs #710
* fix(dictation): redact API keys from streaming transcriber errors
Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into
websocket URLs/headers and echoed unredacted transport errors and
server-side error payloads. Wrap those error endpoints with
providerio.Redact and switch the connect-error format specifier from %w
to %s so the raw error cannot leak via errors.Unwrap().
Adds regression tests asserting the API key is not present in the
returned stream error for both transcribers.
Co-authored-by: euxaristia <[email protected]>
* fixup(dictation): address review: redact session/write errors, move redaction tests
- Redact API key in OpenAI Realtime session configuration error.
- Return redacted OpenAI Realtime write error from writeErrCh instead of discarding.
- Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and
TestDeepgramStreamTranscribeErrorRedaction next to their source files.
Refs #710
* fixup(dictation): sync Deepgram redaction test on CloseStream before close
Address CodeRabbit nitpick: the server now drains the client's audio frames and
waits for CloseStream before rejecting the connection with an API-key-bearing
policy-violation close reason. This ensures StreamTranscribe observes the close
reason (and redacts it) instead of failing earlier on a write error.
Refs #710
* fix(dictation): preserve context.Canceled through streaming error redaction
Formatting the read error with %s dropped the unwrap chain, so an Esc
abort of a Deepgram or OpenAI Realtime dictation no longer matched
errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious
error toast on top of the cancellation notice. Return the cancellation
sentinel itself before redacting — it carries no key — and keep the
redacted flat string for genuine failures. Adds regression tests that
cancel a live stream and assert the sentinel survives.
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(dictation): close remaining cancellation and redaction gaps
Key cancellation detection on ctx.Err() instead of
errors.Is(err, context.Canceled), which was racing against
writer-goroutine transport errors that could overwrite the error
value right before the check. Applied consistently across both
transcriber implementations. Fix goroutine leaks and a possible hang
in the cancellation regression tests.
* test(dictation): cover dial and startup cancel identity
Add regression tests for Esc-abort during WebSocket dial and right after
accept (session update / first write) so those redaction sites keep
returning context.Canceled for the TUI errors.Is check.
* fix(dictation): prefer cancel over a racing OpenAI realtime error
Esc can cancel after conn.Read already has an error frame. Check ctx.Err()
after partial callbacks and on the realtimeError path so the stream still
returns context.Canceled instead of a redacted failure notice.
Refs Gitlawb/zero#710
* fix(dictation): prevent auto-submit on cancelled realtime race
handleDictationTranscribed suppressed context.Canceled but then fell
through to the msg.submit branch. A delta -> Esc -> already-buffered
realtime error race returns a nonempty transcript alongside
context.Canceled, so with stt.autoSubmit on, Esc could submit the
composer's restored pre-existing text instead of doing nothing.
Treat a canceled result as terminal before the auto-submit path is
reached, and add a TUI-level test covering the race.
* test(dictation): exercise writeErrCh cancellation branch in realtime transcriber
* fix(dictation): add custom key header redaction and single-pass redaction tests
* fix(dictation): track dictation sessionID to drop stale completions after Esc and synchronize write cancellation test
* fix(dictation): initialize non-zero session ID and filter stale session 0 messages
Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio.
Refs #710
* fix(dictation): drop providerio widen; pin session and redaction tests
Revert the Token/X-Api-Key providerio.Redact expansion (belongs in its own PR with coverage). Make Deepgram single-pass, cancel-race, and live-session partial tests pin the branches they name, and restore the ST1005 nolint on the user-facing Deepgram error string.
* fix(dictation): ignore stale batch startup by session ID
Gate dictationStartedMsg on session ID so a late Start result after cancel cannot reset a newer recording. Rename and comment cleanups from review; race suite recorded on the PR body.
* fix(dictation): exit writers on cancel and pin dial/write redaction paths
Select on ctx.Done while waiting for audio chunks so cancelled streaming
sessions do not leave writer goroutines blocked on an open idle channel.
Add non-cancellation redaction tests for Deepgram dial failure and OpenAI
Realtime dial, session-configuration, and async writer failures.
Refs #852
* fix(dictation): close the realtime socket on writer failure so a silent server cannot strand the read loop
CodeRabbit review 4885680006 flagged that the OpenAI Realtime writer goroutine pushed write errors to writeErrCh and returned without closing the connection, so conn.Read stays parked forever when the peer goes silent after session setup and the error is never drained. Close the socket on both the append and commit write failure paths (CloseNow, since the close handshake itself would block on a silent peer), and add a silent-server regression test.
Refs #852
Co-Authored-By: cairn-code <[email protected]>
* test(dictation): cancel the silent-server writer failure test during teardown
CodeRabbit review 4909044011 identified that the silent-server regression test used context.Background(), allowing its goroutine to remain blocked in conn.Read after a timeout. Give the test a cancellable context and defer cancellation so teardown always releases the blocked read.
Refs #852
Co-Authored-By: cairn-code <[email protected]>
---------
Co-authored-by: Claude Fable 5 <[email protected]>
Co-authored-by: euxaristia <[email protected]>
Co-authored-by: cairn-code <[email protected]>
* fix(daemon): clean up child after startup timeout
* fix(daemon): terminate startup process tree
* test(background): accept reaped-later zombie children
* fix(background): retain configured process group target
* fix(background): address termination review feedback
* fix(daemon): use launch-time group identity, fix zombie/reap classification
Addresses jatmn's three #774 review findings:
- TerminateCommand rediscovered the process group via
execution.TerminateProcessTree's Getpgid(pid) lookup. On Darwin,
that lookup can return ESRCH once an unreaped group leader has
already exited, even though live descendants remain in the group
it configured at launch — silently falling back to signalling only
the (already dead) leader PID and leaving descendants running (the
required macOS smoke job's failure). Add
execution.TerminateProcessGroup, which signals the negative PID
directly using the launch-time Setpgid invariant instead of
rediscovering it, and route TerminateCommand through it.
- signalTargetRunning's zombie check matched the individual-PID
fallback target (a process that is not its own group leader)
against the process table by PGID. That target's actual PGID
differs from its own PID by definition, so no row ever matched,
unknown resulted every time, and a zombie individual-PID target
was always conservatively reported as still running. Branch on the
sign of the target: group members by PGID, individual PIDs by PID.
- TerminateCommand returned the termination error even when the
subsequent reap succeeded — a successful cmd.Wait is authoritative
proof the leader is gone and collected, regardless of how the kill
attempt itself went (e.g. taskkill /T racing an already-exited PID
on Windows). Classify a successful reap as success.
All three regression tests are verified to fail against the pre-fix
code (confirmed by temporarily reverting each fix and rerunning).
Co-Authored-By: Claude Sonnet 5 <[email protected]>
* fix(daemon): harden command termination inputs
Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-abb8-73de-9489-51e9e6514c6b
Co-authored-by: Pierre Bruno <[email protected]>
* fix(background): preserve process tree failures
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8
Co-authored-by: Pierre Bruno <[email protected]>
* docs(daemon): clarify boundary-probe and Setsid-guard comments per PR #774 review
Co-authored-by: Cursor <[email protected]>
---------
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Amp <[email protected]>
Co-authored-by: Cursor <[email protected]>
Split 1/3 of #829, carved out as the independently-reversible hardening layer
with no dependency on the orchestration work that stacks on top of it.
- internal/pathjail: os.Root handle-relative path confinement — ancestor
traversal is confined, the final component is checked for reparse points
(including a name written with a trailing separator), unprivileged Windows
junctions are covered, temp-name fragments are rejected before they can
escape the directory, whitespace-bearing paths are confined as spelled, and
temp files stay unpredictable and exclusive.
- internal/credstore: file locking around the credential read-modify-write —
exclusive for writers, shared for readers, so a reader cannot race a publish.
- internal/worktrees: git invocation hardening.
pathjail has no importers in this PR by design; internal/memory and
internal/specialist adopt it in the stacked PRs (2/3 and 3/3).
Origin-Session: local-de382f | Claude Code | 4 prompts
Origin-Snapshot: 52db9b74dad5
Co-authored-by: Claude Opus 4.8 <[email protected]>
* **update:** recognise Homebrew installs and add mise, go install docs ([#910](https://github.com/Gitlawb/zero/issues/910)) ([01c3498](https://github.com/Gitlawb/zero/commit/01c3498205e69d26e5f929225f9455b276444f39))
* use Exa as default search provider ([#926](https://github.com/Gitlawb/zero/issues/926)) ([1ec7219](https://github.com/Gitlawb/zero/commit/1ec7219aee7b2840481ab0905f131644e27d2a4e))
* **zerogit:** auto-create a conventional branch before push/pr on default branch ([#671](https://github.com/Gitlawb/zero/issues/671)) ([d065467](https://github.com/Gitlawb/zero/commit/d065467cd3ec3ed91c5dc60554c36fc4b4d6de4c))
### Bug Fixes
* **acp:** three defects a desktop ACP client hits ([#915](https://github.com/Gitlawb/zero/issues/915)) ([a78c6c3](https://github.com/Gitlawb/zero/commit/a78c6c357e1d21146d7cd2e8030a754e06f7f1f5))
* bump Go to 1.26.6 for stdlib vulnerability fixes ([#903](https://github.com/Gitlawb/zero/issues/903)) ([dc15e82](https://github.com/Gitlawb/zero/commit/dc15e822987d8d7de467c62c375518f435b74d8c))
* **daemon:** clean up child after startup timeout ([#774](https://github.com/Gitlawb/zero/issues/774)) ([9c29209](https://github.com/Gitlawb/zero/commit/9c292092a26c9092fd873d072b0af1b0d9d39ef0))
* **dictation:** redact API keys from streaming transcriber errors ([#852](https://github.com/Gitlawb/zero/issues/852)) ([64a783b](https://github.com/Gitlawb/zero/commit/64a783b08caa2c7b21f188033b9ed84a14e00e84))
for_,want:=range[]string{"Which database should remain?","Postgres only; do not touch MySQL","internal/db.go","https://example.com/spec","Inspect the Windows implementation"}{
for_,want:=range[]string{"Which database should remain?","Postgres only; do not touch MySQL","internal/db.go","internal/api.go","internal/config.go","https://example.com/spec","Inspect the Windows implementation"}{
t.Fatalf("eager tool schemas are %d tokens, over the %d ceiling — trim a schema, drop a core tool, or raise the ceiling deliberately (deferring will NOT help: this test disables deferral)",got,maxEagerToolSchemaTokens)
t.Fatalf("expected the OpenAI addendum in the gpt-5 prompt")
}
ifgot:=strings.Join(strings.Fields(strings.ToLower(buildSystemPrompt(Options{Model:"gpt-5"})))," ");!strings.Contains(got,"never run apply_patch through exec_command"){
t.Fatalf("OpenAI prompt must route edits through the native patch tool:\n%s",got)
}
// Claude is aligned with the core prompt and gets no family addendum now that
// comment discipline is universal; it must not pick up another family's block.
returnbranchPushPlan{},fmt.Errorf("remote %s has no branches yet; push the initial default branch first with --yes (`zero changes push --yes`), then use auto-branching for subsequent work",pushRemote)
}
}
// Branching off the default branch only makes sense when HEAD carries a
// commit that is not already on the remote default branch. A clean,
// up-to-date default branch would otherwise publish a feature branch at the
// exact default tip. If the ahead count cannot be determined (for example
// the remote-tracking ref was never fetched), fail rather than guess. When
// the remote is confirmed unborn, refuse auto-branching entirely: the
// initial default branch must be established first (changes push --yes)
// so the remote has a real HEAD before any feature branch is published.
returnbranchPushPlan{},fmt.Errorf("remote %s has no branches yet; push the initial default branch first with --yes (`zero changes push --yes`), then use auto-branching for subsequent work",trackingRemote)
}
cause:=aheadErr
ifcause==nil{
cause=fetchErr
}
returnbranchPushPlan{},fmt.Errorf("cannot determine whether HEAD is ahead of %s/%s: %w; fetch the remote tracking branch first",trackingRemote,trackingBranch,cause)
}
ifahead==0{
returnbranchPushPlan{},fmt.Errorf("no changes to publish: HEAD is not ahead of %s/%s; commit your work before pushing",trackingRemote,trackingBranch)
}
// Resolve the restore tip to a concrete commit before creating any branch.
// It must never be a bare local branch name: resolving "main" yields the
// very commit the feature branch is about to own, so restore becomes a
// silent no-op and the publishable commits stay on the default branch. A
// named remote resolves through its just-refreshed remote-tracking ref; a
// direct URL or path asks the remote itself. Fail closed rather than
return"","",false,fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w; generated branch %s was preserved because the default branch changed concurrently",plan.CurrentBranch,plan.RestoreTip,err,result.Branch)
return"","",false,fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w; rollback of %s also failed: %v",plan.CurrentBranch,plan.RestoreTip,err,result.Branch,rollbackErr)
}
return"","",false,fmt.Errorf("failed to restore default branch %s to %s after auto-branching: %w",plan.CurrentBranch,plan.RestoreTip,err)
}
}
if!opts.JSONMode{
fmt.Fprintf(stdout,"Created branch %s (was on %s)\n",result.Branch,plan.CurrentBranch)
}
returnresult.Branch,plan.PushRemote,true,nil
}
// fallbackBranchSlug derives a deterministic branch-name slug from a change
// summary without calling an LLM, so ensureFeatureBranch still works when no
promptBuilder.WriteString("Analyze the following git diff and generate a short git branch name slug for it.\n")
promptBuilder.WriteString("The slug must be 2 to 5 lowercase words separated by hyphens (kebab-case), using only letters, digits, and hyphens, with no prefix like \"feature/\" or \"fix/\" and no surrounding quotes.\n")
promptBuilder.WriteString("Output ONLY the raw slug text, nothing else.\n\n")
result:=check("sandbox.backend","Sandbox backend",StatusWarn,fmt.Sprintf("Native sandbox backend %s is installed, but Windows sandbox setup is missing or out of date: %v.",backend.Name,err),map[string]any{
fmt.Sprintf("Sandbox principal setup is current; a command in this workspace selects the %s principal. Setup state does not confirm the account is usable.",role),map[string]any{
"backend":string(backend.Name),
"platform":goos,
"optIn":true,
// Deliberately not "active": nothing here checked the account, the
t.Fatalf("zombie group %d is unsignalable with an unexpected error (%v); the zombie case is not present, so nothing below would be meaningful",pid,err)
}
psPathReachable=false
t.Logf("kernel refuses to signal zombie-only group %d (EPERM); signalTargetRunning answers from kill(2) rather than ps here, so the ps-based zombie check is asserted directly on platforms that allow the signal",pid)
}
// Where the signal IS permitted, pin the ps-based check itself. This is the
// regression #774 was about, and asserting only signalTargetRunning would
// let a ps-side regression hide behind the kill result.
ifpsPathReachable{
running,ok:=processGroupHasRunningMember(pid)
if!ok{
t.Fatalf("process group %d has no rows while its zombie is unreaped; ps stopped reporting the group rather than the group being gone",pid)
}
ifrunning{
t.Fatal("the ps-based group check must not report a zombie-only group as running")
}
}
// Valid on every platform: whichever branch answers, a zombie-only group is
// not running. On a kernel that refuses the signal this is the whole of the
// coverage, and it is more than the previous skip provided.
ifsignalTargetRunning(-pid){
t.Fatal("a group holding only a zombie must not count as running")
}
if!psPathReachable{
// terminateTarget's own kill(2) would hit the same EPERM and be returned
// as a cleanup error, so this assertion cannot be made where the kernel
// refuses the signal. That may be a real gap in TerminateProcessTree on
// such a platform rather than a test limitation — a zombie-only tree is
// already exited and should terminate cleanly — but confirming and
// changing that is a production fix, not this test's business.
t.Log("skipping the TerminateProcessTree assertion: its kill(2) would hit the same EPERM as the probe above")
// ok reports whether ANY row for this group was found. This test never waits
// on the child, so its zombie row must still be there — an unknown state
// means ps stopped answering, not that the group is gone. Treating that as a
// pass is what let this assertion succeed without checking anything.
running,ok:=processGroupHasRunningMember(pid)
if!ok{
t.Fatalf("process group %d has no rows after termination; the unreaped child's zombie row must persist, so the group's state is undeterminable rather than clean",pid)
}
ifrunning{
t.Fatal("the group still has a running member after termination")
}
}
// TestSignalTargetRunningTreatsZombieIndividualPIDAsExited is the regression
// test for jatmn's second #774 finding: signalTargetRunning's zombie check
// matched the individual-PID fallback target (a process that is NOT its own
// group leader — processSignalTarget's positive-PID case) against process-table
// rows by PGID. That target's actual PGID differs from its own PID by
// definition (that's exactly why processSignalTarget fell back to the
// individual PID instead of a negative group target), so no row ever matched,
// "unknown" resulted every time, and signalTargetRunning conservatively (and
// incorrectly) treated a zombie individual-PID target as still running.
// credential lookup uses it to avoid reusing legacy user tokens by name.
ProjectConfiguredbool
// UnconfiguredDefault is true when this server is one of Zero's built-in
// defaults (e.g. keyless Firecrawl) that the user never touched in their
// defaults (e.g. keyless Exa) that the user never touched in their
// config — no credentials, no overrides. Callers use it to avoid warning
// loudly when a server nobody configured fails to connect.
UnconfiguredDefaultbool
Loaded 100 of 385 files, more files were not shown because too many files have changed in this diff.
Show more
Reference in new issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Vasanthdev2004