feat(security): path-jail primitive, credential-store locking, worktree git hardening

[gnanam1990]

Split 1 of 3 of #829 — the independently-reversible hardening layer

Per @anandh8x's review of #829, splitting the change into focused PRs. This is the base: the security/hardening layer, which has no dependency on the orchestration work and carries its own independent rollback domain. The orchestration and durable-memory work stack on top of it.

What this contains

• internal/pathjail (new) — os.Root handle-relative path confinement: ancestor traversal is confined, the final component is checked for reparse points, unprivileged Windows junctions are covered, and temp files are unpredictable and exclusive. This is the path-containment primitive @anandh8x verified as correct on feat(specialist): zeromaxing posture and orchestrate plan execution #829.
• internal/credstore — cross-process file locking around the credential store.
• internal/worktrees — git invocation hardening.

pathjail has no importers in this PR by design; internal/memory (split 2) and internal/specialist (split 3) adopt it in the stacked PRs.

On the split shape

The full seven-way split @anandh8x proposed isn't mechanically achievable as independently-building PRs: the plan subsystem is a single package whose 29 plan_*.go files cross-reference each other, and the zeromaxing posture reaches into the shared TUI model — so "plan core", "saved plans", "model routing" and "posture" cannot compile as separate PRs without the module-seam refactor requested separately. The feasible stack is three: (1) this hardening layer → (2) durable memory → (3) the orchestration remainder, which together reconstruct #829 byte-for-byte.

Validation

• go build ./... clean.
• go test ./internal/pathjail/ ./internal/credstore/ ./internal/worktrees/ — all green.
• Rebased on current main (cabfeefc).

Part of #829.

Summary by CodeRabbit

• Bug Fixes

  • Prevented concurrent credential-store updates from overwriting, resurrecting, or losing credentials.
  • Improved cancellation of Git operations so stalled child processes no longer delay command completion.

• Security

  • Strengthened filesystem protections against directory escapes and unsafe links.
  • Added safer temporary-file creation within protected storage locations.

• Reliability

  • Improved Git command behavior across Unix and Windows with consistent environments and bounded shutdown waits.
  • Added cross-platform safeguards for credential-store file access and replacement.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: de87616b-4bdf-4193-bbe2-51b9fc00126a

📥 Commits

Reviewing files that changed from the base of the PR and between f94d32e and 7193cac.

📒 Files selected for processing (6)

• internal/credstore/concurrency_test.go
• internal/credstore/credstore.go
• internal/credstore/filelock_unix.go
• internal/credstore/filelock_windows.go
• internal/pathjail/pathjail.go
• internal/pathjail/pathjail_test.go

🚧 Files skipped from review as they are similar to previous changes (6)

• internal/credstore/filelock_unix.go
• internal/pathjail/pathjail_test.go
• internal/credstore/filelock_windows.go
• internal/credstore/concurrency_test.go
• internal/credstore/credstore.go
• internal/pathjail/pathjail.go

---

Walkthrough

The PR adds file-backed credential locking, a root-confined filesystem package, and hardened Git subprocess handling. It adds regression tests for concurrency, path confinement, and process cancellation.

Changes

Credential-store locking

Layer / File(s)	Summary
Persistent credential-store locking internal/credstore/credstore.go, internal/credstore/filelock_*	File-backed operations use a persistent separate lock file. Unix and Windows implementations support shared and exclusive locking.
Concurrency and lock-file regression coverage internal/credstore/concurrency_test.go	Tests cover concurrent updates, cross-process writes, lock-file persistence, and lock-acquisition failures.

Path-jail filesystem confinement

Layer / File(s)	Summary
Root-confined filesystem operations internal/pathjail/pathjail.go	The package validates root-relative paths, rejects reparse points, and creates exclusive temporary files beneath an os.Root.
Cross-platform path-jail validation internal/pathjail/pathjail_test.go	Tests cover links, root escapes, absent paths, path preservation, unsafe fragments, and collision retries.

Git subprocess hardening

Layer / File(s)	Summary
Centralized hardened Git execution internal/worktrees/worktrees.go, internal/worktrees/run_git_*	Git commands use shared working-directory, locale, wait-delay, and cancellation configuration. Unix commands terminate their process groups.
Git hardening and cancellation tests internal/worktrees/run_git_test.go, internal/worktrees/run_git_unix_test.go	Tests validate constructor settings, subprocess construction, Git execution, and Unix process-group termination.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CredentialStore
  participant LockFile
  participant DataFile
  CredentialStore->>LockFile: acquire lock
  CredentialStore->>DataFile: read or update credentials
  CredentialStore->>LockFile: release lock

Loading

sequenceDiagram
  participant Caller
  participant pathjail
  participant osRoot
  Caller->>pathjail: open confined path
  pathjail->>osRoot: validate and access root-relative path
  osRoot-->>Caller: return confined file handle

Loading

sequenceDiagram
  participant WorktreeCaller
  participant newHardenedCommand
  participant GitProcessGroup
  WorktreeCaller->>newHardenedCommand: create Git command
  newHardenedCommand->>GitProcessGroup: configure cancellation
  WorktreeCaller->>GitProcessGroup: cancel command
  GitProcessGroup->>GitProcessGroup: terminate process group
  GitProcessGroup-->>WorktreeCaller: return from Wait

Loading

Possibly related PRs

• Gitlawb/zero#774: Both changes address Git subprocess process-tree termination.
• Gitlawb/zero#855: Both changes modify Git execution in internal/worktrees.
• Gitlawb/zero#857: Both changes address path containment, atomic state writes, and regression coverage.

Suggested reviewers: jatmn, kevincodex1, vasanthdev2004

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the three primary security and hardening changes in the pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 86.36% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/credstore/concurrency_test.go`:
- Around line 13-20: Add a cross-platform regression test in the concurrency
tests that replaces the configured credential directory with a regular file,
then verifies both Store.Set and Store.Delete return the acquireFileLock setup
error. Reuse fileStore and existing test fixtures/helpers where applicable, and
assert the failure path for each method without changing successful contention
coverage.

In `@internal/credstore/filelock_unix.go`:
- Around line 35-38: Change acquireFileLock in
internal/credstore/filelock_unix.go (lines 35-38) and
internal/credstore/filelock_windows.go (lines 35-38) to return release functions
that propagate unlock and file.Close errors. In internal/credstore/credstore.go,
update Set (lines 142-146) and Delete (lines 183-187) to merge release errors
into the returned error whenever the read-modify-write operation otherwise
succeeds, ensuring cleanup failures are never reported as success.

In `@internal/pathjail/pathjail.go`:
- Around line 53-54: Remove the strings.TrimSpace calls in the path-opening flow
so Open preserves the original root and dir values when passing them to
filepath.Rel, os.MkdirAll, and os.OpenRoot. Add a regression test covering
leading and trailing spaces in both paths, verifying the intended
space-containing confinement boundary is used.
- Around line 124-125: Restrict generated temporary paths in the pathjail
creation flow around handle.OpenFile so prefix and suffix cannot escape dir;
reject path separators or validate that the joined relative path remains beneath
dir before opening it. Add regression cases in
internal/pathjail/pathjail_test.go lines 130-153 covering ../ in both prefix and
suffix, requiring errors and confirming no file is created outside dir.

In `@internal/worktrees/run_git_unix_test.go`:
- Around line 55-80: The shell script in the run_git_unix_test setup must not
interpolate pidFile directly: pass it as a positional argument to sh and
reference that parameter when writing the PID. In the grandchild-discovery
failure path, cancel the context and wait for command completion before failing
the test, rather than skipping, so spawned processes are cleaned up and the
security regression remains enforced.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 29a8c0d6-40be-4ad6-9c70-bbfba0cd72a9

📥 Commits

Reviewing files that changed from the base of the PR and between cabfeef and fcd48b8.

📒 Files selected for processing (11)

• internal/credstore/concurrency_test.go
• internal/credstore/credstore.go
• internal/credstore/filelock_unix.go
• internal/credstore/filelock_windows.go
• internal/pathjail/pathjail.go
• internal/pathjail/pathjail_test.go
• internal/worktrees/run_git_test.go
• internal/worktrees/run_git_unix.go
• internal/worktrees/run_git_unix_test.go
• internal/worktrees/run_git_windows.go
• internal/worktrees/worktrees.go

[gnanam1990]

@anandh8x this is the first of the 3-PR split you asked for on #829 — the independently-reversible hardening layer, with the durable-memory and orchestration PRs stacked behind it (opened in sequence as each lands, since a fork branch can't anchor an upstream base). Two questions before I open the next two:

1. Shape — is this the cut you had in mind (hardening lifted out first, then memory, then the orchestration remainder as one)? As noted on feat(specialist): zeromaxing posture and orchestrate plan execution #829, the finer seven-way split needs the module-seam extraction first, so I kept this to the three layers that have genuinely independent build and rollback domains.

2. Parent issue — neither this PR nor feat(specialist): zeromaxing posture and orchestrate plan execution #829 links an issue-approved parent. How would you like that handled — is there an existing approved issue this work should reference, or should one be filed (Discussions → Ideas) before I open the memory and orchestration PRs? I'd rather sort the gate now than have the stack stall on it.

Happy to adjust the boundaries if you'd cut them differently.

[Vasanthdev2004]

Good change, and the package docs are the best part of it. The pathjail comment explaining why Lstat-then-act cannot work, and why a junction needs ModeIrregular rather than ModeSymlink, is real reasoning rather than decoration.

I checked the core claims instead of taking them:

• os.Root does refuse junction traversal (path escapes from parent).
• RefuseReparse does catch a junction as the final component.
• a junction does report irregular=true, symlink=false, so testing both bits is necessary rather than belt-and-braces.

I also went hunting for a filepath.Rel case-sensitivity bug on Windows and there isn't one: Rel uses EqualFold there, so an upper-cased root resolves fine. Scope is clean too. Against origin/main (cabfeef) this is exactly the 11 files and +900/-2 you describe. And every s.write() in credstore sits inside a lock-held function, so writer coverage is complete.

Three things worth fixing before it lands, all small.

1. The credstore lock is writers-only, and on Windows that makes locked writes fail

Get and Providers call read() with no lock. write() publishes with os.Rename, and Go's os.Open on Windows does not request FILE_SHARE_DELETE, so MoveFileEx cannot replace a file another opener still holds.

Measured cross-process at a 2ms cadence: 11 of 400 locked Set calls failed with credstore: publish: ... Access is denied, and roughly 7.5% of the reader's Get calls hit a sharing violation.

On unix this is fine, since POSIX rename does not care about open descriptors. So the two lock primitives are equivalent but the publish step around them is not: writers-only is sufficient on unix and insufficient on Windows.

Cheapest fix is to take the lock in Get and Providers too, shared if you want reader concurrency (LOCK_SH, or LockFileEx without LOCKFILE_EXCLUSIVE_LOCK).

2. RefuseReparse returns nil for a junction with a trailing separator

os.Root.Lstat on Windows resolves the terminal component when the path ends in a separator (doInRootAlwaysResolveTerminalSlash in root_windows.go). So Lstat on a junction name with a trailing separator stats the target, an ordinary directory, and the guard reports "not a link" for a name that is one.

if trimmed := strings.TrimRight(relative, `/\`); trimmed != "" {
    relative = trimmed
}

before the Lstat, plus a case beside the existing junction test.

3. CreateTemp pastes prefix and suffix into the path unsanitized

Separators and .. in either are path syntax rather than name text. A prefix of ..\sibling\note puts the file outside the dir the doc promises. Worse, a suffix containing .. erases the random component: .tmp\..\..\evil produced a file named literally evil at the jail root, so the unpredictability O_EXCL is there to protect is gone.

filepath.Base each of them, or reject a separator or .. outright.

Advisory, not blocking

• Open resolves root itself by pathname (MkdirAll then OpenRoot), so a junction planted at root or at an ancestor relocates the whole jail. Your doc calls this out as a deliberate trade-off, and the stores this replaces already create the same roots by pathname, so it is not a regression. Worth settling before splits 2 and 4 adopt it though, because "the boundary the caller chose and trusts" is a weaker claim when pathjail is the thing creating that boundary.
• ModeIrregular is set for every reparse tag except symlink, AF_UNIX and dedup. OneDrive placeholders and Store AppExecLink stubs fall in that set, so a store under a OneDrive-backed Documents folder would be refused outright. The stdlib carves out dedup for exactly this reason. Testing the reparse tag rather than the mode would avoid it.
• On Windows only the direct git.exe is killed on cancel, while POSIX kills the whole group. LFS filters, credential helpers and fsmonitor survive, and on Windows a survivor holding handles inside the worktree is what blocks removal. A job object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE is the analogue if you want it here, otherwise worth a line saying it is deferred.
• The cancellation hardening does not currently run at all: exec.go:205, exec.go:232, workflows.go:87 and workflows.go:177 all pass context.Background(), so the Cancel hook and the cancel branch of WaitDelay are unreachable from production.
• GIT_DIR and friends still pass through and take precedence over command.Dir. Not introduced here, since the previous code set no Env at all and this is strictly an improvement, but "consistent environments" invites the question and an explicit filter is a few lines.
• Tests: all four concurrency tests pin Storage: "file", while encrypted-file is the default on every platform except macOS with a keyring, and it takes a different path through securefile.Crypter with its own nested lock. Also the junction test skips silently off Windows, where linkDir uses os.Symlink, so a predicate narrowed back to ModeSymlink alone would still pass there.
• The "byte-for-byte reconstruction" framing does not hold for worktrees.go. The LC_ALL/LANG line was re-resolved against main rather than replayed, so that file deserves fresh eyes rather than split-of-already-reviewed treatment.

One to dismiss

CodeRabbit's filelock_unix.go:38 "return lock-release errors" is not real. The payload is fsync'd and renamed before release() runs, and file.Close() drops the flock even if the explicit LOCK_UN errors, so a dropped release error cannot lose data. Returning it would only force Set and Delete to choose between the write error and a close diagnostic.

Happy to re-review quickly once 1 to 3 are in. The shape of this is right and I would like it landed.

[anandh8x]

I found issues that should be addressed before this is ready.

Findings

• [P1] Preserve legitimate whitespace in confined paths
  internal/pathjail/pathjail.go:52-54

  Open trims both root and dir, so a legitimate path whose name begins or ends with a space is silently changed to a different filesystem object before filepath.Rel, MkdirAll, and OpenRoot. I reproduced this with a root named "root ": Open succeeded but created/opened "root" instead. Treat only an actually empty root as missing and preserve the caller's original path spelling.

• [P1] Keep generated temporary files inside the requested directory
  internal/pathjail/pathjail.go:118-125

  CreateTemp joins caller-controlled prefix and suffix without rejecting path components. A prefix such as ../escaped causes the file to be created outside dir (though still under the broader os.Root). I reproduced this: a request under intended returned an escaped.<random>.tmp path in its parent. Reject separators/traversal in both prefix and suffix, or verify the resulting relative path remains beneath dir.

• [P2] Report lock-release failures instead of returning success
  internal/credstore/filelock_unix.go:35-38
internal/credstore/filelock_windows.go:35-38
internal/credstore/credstore.go:142-152,183-199

  Both release closures discard unlock and Close failures, and Set / Delete use a plain deferred release. A completed write can therefore be reported as fully successful even when the lock cleanup failed. Return an error from the release closure and join it into the operation result, at least when the read-modify-write otherwise succeeded.

• [P2] Make the Unix cancellation regression clean up on its failure path
  internal/worktrees/run_git_unix_test.go:48-80

  The shell program interpolates pidFile directly and the grandchild == 0 branch calls t.Skip without cancelling and waiting for the already-started command. That can leak the shell/grandchild and also turns a failed security assertion into a skipped test. Pass the path as a positional shell argument; if PID discovery fails, cancel, wait for completion, then fail.

Scope and validation

The split is otherwise focused and the architecture is sensible: the handle-relative path boundary is reusable, the credential lock covers the complete read-modify-write transaction, and git hardening is wired through one constructor.

All GitHub checks are green. Locally, the focused package tests and vet passed, and the three affected packages cross-compiled as Windows test binaries. I could not run -race in this environment because no C compiler is installed.

The requested test for replacing the credential directory with a regular file is worthwhile regression coverage, but current Set and Delete already propagate the MkdirAll lock-setup error, so I do not consider it a separate implementation blocker.

Non-blocking: the PR description says “split 1 of 3,” while the commit message says “1/4” and references “2/4 and 4/4.” Please make the split count consistent.

[gnanam1990]

Thanks both — pushed 3079b06c addressing the blocking findings. Each fix has a regression test; the strengthened ones are mutation-checked (revert the fix → the test fails).

Fixed

• pathjail.CreateTemp fragment injection (@anandh8x P1, @Vasanthdev2004 Explore OpenTUI Solid TUI migration #3) — prefix/suffix are now rejected up front if they carry a separator or ./... The regression test uses Open(root, subdir) so it exercises the real case os.Root does not backstop: a ../escaped fragment landing in root but outside the promised dir. Without the fix that returns success and leaks the file into the parent; the test asserts nothing escapes.
• pathjail.Open whitespace trimming (@anandh8x P1) — only an empty/whitespace-only value is treated as missing; the path is otherwise used verbatim, so "root " is confined as spelled. Test asserts the trimmed "root" is never created.
• pathjail.RefuseReparse trailing separator (@Vasanthdev2004 fix plan mode and add test #2) — strings.TrimRight(relative, "/\\") before Lstat, so a junction named with a trailing slash is inspected as itself rather than resolved to its target. Test added beside the junction case.
• credstore readers need the lock (@Vasanthdev2004 Add contributing guidelines #1) — acquireFileLock now takes exclusive bool; writers take it exclusive, Get/Providers take it shared (LOCK_SH / LockFileEx without LOCKFILE_EXCLUSIVE_LOCK), so a reader can't race a publish — which is what bites on Windows, where an unsynchronized reader holding the file open blocks the rename.
• Worktrees cancellation test (@anandh8x P2) — pidFile is passed as a positional $1 instead of interpolated, and a missing pid now cancels, waits for the command, and t.Fatals instead of leaking the shell and silently skipping the assertion.
• Split numbering — commit now says 1/3 (2/3, 3/3), matching the description.

One I'd push back on

• Returning lock-release errors (@anandh8x P2 / CodeRabbit): I'm with @Vasanthdev2004's "one to dismiss" here. The payload is fsync'd and renamed before release() runs, and file.Close() drops the flock even if the explicit LOCK_UN/UnlockFileEx errors — so a release hiccup can't lose data or leave the store inconsistent, and folding it into the result would report an otherwise-successful write as failed. Happy to surface it as a non-fatal diagnostic if you want visibility, but I don't think it should change the returned error.

Advisory — noted, not in this PR

• ModeIrregular refusing OneDrive/AppExecLink placeholders (@Vasanthdev2004): real, and the right fix inspects the reparse tag (carving out dedup as the stdlib does) rather than the mode — Windows-syscall work worth its own focused change.
• GIT_DIR and friends passthrough: not introduced here (the prior code set no Env at all), and an explicit filter is a behavior change I'd rather not slip into a hardening PR speculatively.
• Windows job-object kill-on-close as the process-group analogue: deferred; the Unix group-kill is what's wired here.
• Cancellation hook unreachable (context.Background() at exec.go/workflows.go): those files live in the orchestration split (3/3), not this PR — I'll wire the cancellable context there.
• Encrypted-file concurrency coverage: the concurrency tests pin Storage:"file"; broadening to encrypted-file (the securefile.Crypter path) is worthwhile and I'll add it as a follow-up.

I also gave worktrees.go's LC_ALL=C/LANG=C line fresh eyes per your note — it's correct: forced C locale appended to the full environment for stable git-output parsing, strictly more than the prior code which set no Env.

Verified: gofmt, go vet (unix + windows), go build ./..., go test -race on all three packages (a C compiler is present here, so the race check ran), and Windows test binaries compile.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/pathjail/pathjail_test.go`:
- Around line 130-152: Extend TestCreateTempIsExclusiveAndUnpredictable with a
controllable random-byte source used by the internal CreateTemp implementation.
Pre-create the first generated temporary name, then call CreateTemp and verify
the existing file remains unchanged while creation retries and returns a
different name; retain the existing suffix and uniqueness assertions.
- Around line 157-170: The test TestOpenPreservesLeadingAndTrailingSpaceInPaths
should use a portable leading-space path component instead of a trailing-space
component. Create a child entry through handle, then inspect it via handle at
the expected leading-space path to verify the filesystem preserves the requested
spelling; do not rely on handle.Name() or host-specific trailing-space behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 206438fd-4950-476c-a97c-d7e17d0a5ba3

📥 Commits

Reviewing files that changed from the base of the PR and between fcd48b8 and 3079b06.

📒 Files selected for processing (6)

• internal/credstore/credstore.go
• internal/credstore/filelock_unix.go
• internal/credstore/filelock_windows.go
• internal/pathjail/pathjail.go
• internal/pathjail/pathjail_test.go
• internal/worktrees/run_git_unix_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/credstore/filelock_windows.go
• internal/worktrees/run_git_unix_test.go
• internal/pathjail/pathjail.go

[Vasanthdev2004]

Re-reviewed at 3079b06c. All three of my blocking findings are genuinely fixed, and I checked each rather than taking the summary:

1. credstore readers. acquireFileLock(exclusive bool), writers true, Get and Providers false, LOCK_SH on unix and LockFileEx without LOCKFILE_EXCLUSIVE_LOCK on Windows, both held under defer release() across the read. That closes it properly: the writer now waits for readers to drop the lock instead of racing their open handle into its rename.
2. RefuseReparse trailing separator. TrimRight before the Lstat, with the root case guarded so "." survives, and a test beside the junction one.
3. CreateTemp fragments. Rejected up front, and I like that the test uses Open(root, subdir) so it exercises the case os.Root does not backstop, where the fragment lands inside the root but outside the promised dir. That is the version that actually proves something.

Agreed on the pushback about lock-release errors, for the reason you give.

The deferrals look right too. The ModeIrregular tag work, the GIT_DIR filter and the job object are each their own change, and wiring the cancellable context in split 3 is the correct home for it since that is where the call sites live.

One new thing, and it is the only reason this is not an approve.

TestOpenPreservesLeadingAndTrailingSpaceInPaths cannot hold on Windows

Smoke (windows-latest) is red on this head; macOS and ubuntu are green. The assertion is:

if _, err := os.Stat(filepath.Join(base, "root")); err == nil {
    t.Error(`a trimmed "root" was created instead of the requested "root "`)
}

Win32 strips trailing spaces and dots from path components, so "root " and "root" are not two objects there, they are one. I ran it to be sure:

os.MkdirAll(<tmp>/"root ")  ->  created on disk: "root"
stat("root ") err=<nil>
stat("root")  err=<nil>

So the directory that appears is root, the trimmed stat succeeds, and the test reports a trim that never happened. The premise behind the finding is true on unix and simply not expressible on Windows.

Your fix is fine and I would keep it. It is the test that needs the gate: either if runtime.GOOS == "windows" { t.Skip(...) } with a line saying why, or keep the handle.Name() half everywhere and run only the trimmed-stat half off Windows. Worth a sentence about Win32 component normalization so nobody re-adds it later.

Worth naming the shape as well, since this stack has two more splits coming: this is a case only a Windows machine can see, and you are on macOS. The same will apply to anything touching path semantics in 2/3 and 3/3, so it may be worth me running those on Windows before they go up rather than after CI says so.

Everything else is green here: go build ./..., and internal/credstore and internal/worktrees both pass on Windows.

[anandh8x]

Re-reviewed the latest head (3079b06). The earlier traversal, credential-locking, and Unix cancellation blockers are substantially fixed, but this is not ready to approve yet.

Findings

• [P1] Make the whitespace regression portable on Windows — internal/pathjail/pathjail_test.go:157-170

  Windows smoke is red because the test assumes "root " and "root" are distinct filesystem objects. Win32 normalizes the trailing-space component, so the assertion fails even though Open no longer trims the supplied path. Keep the production fix, but test a portable leading-space component by creating a child through the returned handle and verifying it at the expected path.

• [P2] Preserve whitespace-only paths too — internal/pathjail/pathjail.go:53-61

  Open still uses strings.TrimSpace(root) == "" and the same check for dir. That means a legitimate whitespace-only path is rejected as a missing root, or silently replaced with root for dir. This is the remaining part of the original retargeting bug. Check root == "" and dir == "" while otherwise preserving the exact caller input.

• [P2] Exercise the exclusive-create collision path — internal/pathjail/pathjail_test.go:130-152

  The implementation correctly uses O_EXCL, but the current uniqueness loop never forces a collision. A truncating implementation with deterministic distinct names could still pass. Add a controllable random source for the internal helper, pre-create the first generated name, then verify the existing file is unchanged and creation retries with a different name. This is a regression-test gap rather than evidence that the current production path is broken.

Validation

Locally, the three focused packages passed three repeated test runs and go vet; all three also cross-compiled as Windows test binaries. Native Windows smoke currently fails at TestOpenPreservesLeadingAndTrailingSpaceInPaths. The branch is also one commit behind current main, although that commit does not overlap these files.

[gnanam1990]

Pushed f94d32e8. All three findings fixed, rebased onto current main, and the Windows smoke failure is mine — thanks for the precise diagnosis.

Fixed

• [P1] Windows-portable whitespace test. You were both right about the cause: Win32 normalizes a component's trailing spaces away, so "root " and "root" are one object there and the assertion could never hold. Replaced with a leading-space component, which survives normalization on every platform, so the test runs everywhere rather than being skipped on the one OS where path semantics are most interesting. It also now writes a file through the handle and stats it at the expected spelling, so it proves the bytes land where the caller asked rather than just checking handle.Name().
• [P2] Whitespace-only paths preserved. Open now tests root == "" / dir == "" exactly. Worth flagging what this caught: my first attempt at the regression test used filepath.Join(base, " "), and it passed against the old TrimSpace guard — an absolute path ending in a space still has a non-empty TrimSpace, so it never exercised the bug. The path has to be whitespace-only in full, i.e. relative, so the test now uses t.Chdir + Open(" ", " "). That version fails with the old guard (no root to confine " " to) and passes with the fix.
• [P2] O_EXCL collision path exercised. Added a randomBytes seam (a package var over rand.Read) so a test can force the collision a real random source will not produce on demand. The test pre-creates the first generated name, then asserts CreateTemp does not return it, the existing file's contents are untouched, and the source was drawn more than once so the retry genuinely ran. Dropping O_EXCL from the production call makes it fail.

Each fix is mutation-verified — I reverted each guard in turn and confirmed the corresponding test fails, which is how I found the vacuous whitespace test above rather than shipping it.

Also

• Rebased onto current main (2d2450e9), so the branch is no longer behind.
• Verified: gofmt, go vet (unix + windows), go build ./..., go test -race -count=3 on all three packages, and Windows test binaries compile.

@Vasanthdev2004 — on your offer to run 2/3 and 3/3 on Windows before they go up: yes please, and I think it's the right call. This failure is exactly the shape you named — a case only a Windows machine can see, from a macOS author — and both remaining splits touch path semantics (the memory store and the plan store are the two pathjail adopters). I'd rather find those on your machine than in CI. I'll ping you when 2/3 is ready rather than pushing it blind.

[Vasanthdev2004]

Approving at f94d32e8. Ran it on Windows, which is the machine that mattered here.

All three packages pass: internal/pathjail, internal/credstore, internal/worktrees, plus go build ./... and gofmt clean. Merge base is exactly origin/main (2d2450e9), so the rebase is real.

The leading-space substitution is the right call and it does hold on Windows. A leading space survives component normalization there, unlike a trailing one, so the test runs on all three platforms instead of skipping on the one where path semantics are most interesting. Writing through the handle and stating the file at the expected spelling is a stronger assertion than handle.Name() too.

I also mutation-checked two of your guards rather than taking the summary:

• Dropping O_EXCL from the production create fails TestCreateTempRetriesWithoutClobberingAnExistingName. The randomBytes seam does its job.
• Restoring the old TrimSpace guard does not fail TestOpenPreservesSpaceInPaths.

That second one is worth passing back, because it is the same vacuity you caught in your own first attempt, surviving in the sibling test. TrimSpace only touches whitespace at the ends of the whole string, and filepath.Join(base, " root") produces an absolute path beginning with a drive letter, so the guard is a no-op against it on every platform. I confirmed the mutation actually landed before concluding that, since a revert that silently fails to apply looks identical to a passing test:

root = strings.TrimSpace(root)
dir = strings.TrimSpace(dir)
if root == "" {
...
--- PASS: TestOpenPreservesSpaceInPaths

So the sensitive test for that fix is TestOpenAcceptsAWhitespaceOnlyComponent, and it is skipped on Windows, which leaves the whitespace guard with no Windows coverage at all. The fix itself is correct and I am not blocking on this. If you want the coverage, a relative " root" under t.Chdir would bite on both platforms, since the whitespace is then at the string boundary where TrimSpace can reach it.

Not blocking, just noting it so the next reader does not trust the comment more than the assertion.

Good round. Three iterations, each finding fixed with a regression rather than a patch, and you found one of your own vacuous tests on the way. Taking you up on running 2/3 and 3/3 here before they go up, both adopters are path-semantics code and this is the machine that sees it.

[anandh8x]

Re-reviewed latest head f94d32e8. The previous three blockers were addressed correctly, and all GitHub checks are green, including Windows. I found four remaining issues before approval.

Findings

• [P1] Preserve valid trailing backslashes on Unix — internal/pathjail/pathjail.go:101-108

  RefuseReparse unconditionally applies strings.TrimRight(relative, /\). On Unix, backslash is a valid filename character rather than a separator, so this inspects a different final component. I reproduced it with a symlink named linked\\: RefuseReparse inspected linked and returned nil instead of ErrReparse. Strip only separators recognized by the active platform, with the Windows-specific dual-separator behavior kept behind a small platform check, and add the Unix regression.

• [P2] Propagate lock-release failures — internal/credstore/filelock_unix.go:41-44, internal/credstore/filelock_windows.go:42-45, internal/credstore/credstore.go:142-229

  Both release closures still discard unlock and Close errors, and all four file-backed operations defer them without observing the result. This can report success after incomplete cleanup and conflicts with the repository rule that cleanup or unlock failure must not be reported as success. Return an error from the release function and join it into the result from Set, Get, Delete, and Providers.

• [P2] Cover lock-acquisition failure paths — internal/credstore/concurrency_test.go

  The tests cover successful contention but not lock setup failure. Add a cross-platform regression that replaces the configured credential directory with a regular file and verifies both Set and Delete return the acquisition error. This is required failure-path coverage for the new security boundary.

• [P2] Make the portable whitespace test mutation-sensitive — internal/pathjail/pathjail_test.go:163-188

  TestOpenPreservesSpaceInPaths uses filepath.Join(base, " root"), so the full absolute string begins with the temp-directory prefix. Restoring the old TrimSpace behavior therefore leaves the path unchanged and the test still passes. After t.Chdir, use a relative " root" so the leading whitespace is at the string boundary on every platform; then verify the handle-created child lands under that exact spelling.

Validation

Focused -race tests for internal/pathjail, internal/credstore, and internal/worktrees passed, as did focused vet, formatting, repository-wide vet, release build, and release smoke. All GitHub checks are green. The full local suite reached unrelated environment-dependent sandbox-policy failures in internal/agent and internal/cli; the affected PR packages passed.

[gnanam1990]

Pushed 7193cac7 — all four addressed.

Fixed

• [P1] Unix trailing backslashes. Good catch, and the failure mode is exactly as you describe: a symlink named linked\ was inspected as linked and came back nil. Replaced the literal TrimRight(relative, `/\`) with a helper over os.IsPathSeparator, which is platform-correct by definition — both separators on Windows, / only on unix. Kept as one cross-platform function with the small conditional inside rather than a build-tagged pair, per the repo's preference for unified behavior. Regression added (unix-gated, since the name isn't expressible on Windows); reverting to the unconditional trim fails it.

• [P2] Vacuous whitespace test. @Vasanthdev2004 caught the same thing independently, and you're both right — it's the identical vacuity I found in my own first attempt, surviving in the sibling test. filepath.Join(base, " root") puts the space mid-string where TrimSpace can't reach it, so the guard was a no-op against it on every platform. Now t.Chdir(base) + a relative " root", so the whitespace sits at the string boundary. It now fails against the restored TrimSpace guard on both counts (the handle-created child isn't under the requested spelling, and a trimmed "root" appears), which also gives the whitespace fix its first Windows coverage — previously the only sensitive test was the unix-skipped one.

• [P2] Lock-release failures propagated. You and @Vasanthdev2004 landed on opposite sides of this, so I took the reading that satisfies both concerns rather than picking a side: the release closure now returns an error, and each of Set, Get, Delete, Providers folds it in with errors.Join via a named return. That meets your rule — a cleanup that didn't complete is no longer reported as success — while avoiding the objection that motivated my earlier pushback, since joining annotates the result instead of forcing a choice between the write error and a close diagnostic. If the write fails and the release also fails, the caller sees both.

• [P2] Lock-acquisition failure coverage. Added TestOperationsFailWhenTheLockCannotBeAcquired: it replaces the credential directory with a regular file (which fails the lock's MkdirAll on every platform) and asserts all four operations return an error rather than proceeding unserialized — plus that the blocking file is untouched, so nothing was written through the failed path.

Every guard here is mutation-verified; that's how the vacuous test above got caught rather than shipped a second time.

Validation

gofmt, go vet (unix + windows), go build ./..., go test -race -count=3 on all three packages, Windows test binaries compile, zero-release build + smoke, git diff --check. Branch remains on current main.

@Vasanthdev2004 — thanks for running it on Windows and for mutation-checking rather than trusting my summary; that's what turned up the dead assertion. The offer to run 2/3 and 3/3 on Windows before they go up still stands as far as I'm concerned, and I'll take you up on it.

[Vasanthdev2004]

Approving again at 7193cac7. Ran it on Windows.

go build ./..., go vet, gofmt clean, and internal/pathjail, internal/credstore and internal/worktrees all pass. Merge base is still exactly origin/main (2d2450e9).

The vacuous whitespace test is genuinely fixed, and I checked it the same way as before. Restoring the old TrimSpace guard now FAILS TestOpenPreservesSpaceInPaths, where on the previous head it passed. I confirmed the mutation landed before drawing that conclusion. Moving the path under t.Chdir so the space sits at the start of the whole string is the right move: that is the only position TrimSpace can reach, which is precisely why the absolute-path version could never bite. The comment you left on it says exactly that, which is what stops someone reinstating the weaker form later.

On the backslash finding, credit where it is due: @anandh8x caught a bug I introduced. The TrimRight(relative, `/\`) in my previous review is what put it there. Backslash is a separator only on Windows, so on unix that trims a legal filename character and inspects linked when asked about a symlink genuinely named linked\, reporting "not a link" for one that is. Routing through os.IsPathSeparator is correct by construction rather than by enumeration, and keeping it as one function with the conditional inside is the right shape.

Being explicit about what I did and did not verify: I confirmed the helper's shape, that Windows behaviour is unchanged, and that the new regression is present and correctly gated. I did not execute the unix path, since this machine cannot run it, so that half rests on @anandh8x's review and on CI rather than on me.

The lock-release change reads well too. Reporting the unlock error while keeping Close unconditional means a cleanup that did not complete is no longer indistinguishable from one that did, and joining it at the callers annotates a successful write rather than masking it. Better than either of the two positions we were arguing between.

Four rounds, every finding closed with a regression that fails without the fix, and two vacuous tests caught along the way (one by you, one by me, the same shape both times). Good to land once @anandh8x clears his outstanding review; his findings look addressed to me, but the approval is his to give.

[anandh8x]

Re-reviewed latest head 7193cac. The previously requested fixes are correct: Unix backslash names are preserved, lock release errors are propagated, lock-acquisition failures are covered, and the whitespace regression is now mutation-sensitive. Focused race tests, vet, Windows cross-compilation, formatting, build, smoke, and all GitHub checks pass across Linux, macOS, and Windows. The deterministic release-failure regression remains worthwhile follow-up coverage, but it is not evidence of a production defect and is non-blocking.

[Vasanthdev2004]

@kevincodex1 this one is ready from our side and worth merging ahead of the queue, but GitHub reports it as blocked and I do not have the rights to see why.

State at 7193cac7: approved by @anandh8x, @coderabbitai and me, base is exactly current main (2d2450e9), and every check that reported is green, including Smoke on all three platforms.

What is holding it: mergeStateStatus is BLOCKED. The only check not green is CodeRabbit, which has neither a status nor a conclusion, so it looks like it was created and never reported rather than failed. Branch protection reads 404 for me (maintain, not admin), so I cannot confirm whether that check is required. If it is, it probably needs a re-run or an admin merge rather than anything from the author.

Why it is worth doing first: it is 1 of 3 in a stack. #897 sits directly on top of it and #829's orchestration remainder behind that, so merging this unblocks a chain rather than a single PR. It also carries the internal/pathjail primitive that the next two adopt, so the longer it sits the more rebasing the stack accumulates.

Four rounds of review on it. Every finding was closed with a regression that fails without its fix, and I ran the last two heads on Windows myself since the author is on macOS and the path semantics only misbehave here.

No rush from me if you would rather wait on the CodeRabbit check, just flagging that it does not look like it is going to report on its own.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="Gitlawb/zero"
pr="891"

echo "== Pull request state =="
gh pr view "$pr" --repo "$repo" --json number,headRefOid,baseRefOid,mergeStateStatus,mergeable,reviewDecision,statusCheckRollup \
  --jq '{number, headRefOid, baseRefOid, mergeStateStatus, mergeable, reviewDecision, statusCheckRollup: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl, workflowName}]}'

echo
echo "== Check runs for PR head =="
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
  --jq '.check_runs[] | {name, status, conclusion, started_at, completed_at, details_url, app: .app.slug}'

echo
echo "== Combined commit status =="
gh api "repos/$repo/commits/$head_sha/status" \
  --jq '{state, statuses: [.statuses[] | {context, state, target_url, creator: .creator.login}]}'

echo
echo "== Branch protection visibility =="
gh api "repos/$repo/branches/main/protection" --jq '{required_status_checks, required_pull_request_reviews}' \
  || echo "Branch-protection details are not available to the current GitHub token."

Length of output: 4797

---

@Vasanthdev2004 CodeRabbit is currently successful on 7193cac7.

GitHub reports the following state for #891:

• mergeable: MERGEABLE
• reviewDecision: APPROVED
• All six GitHub Actions checks: SUCCESS
• Combined commit status CodeRabbit: success
• mergeStateStatus: BLOCKED

The null CodeRabbit-like entry in statusCheckRollup does not represent a pending check. GitHub also exposes a legacy combined status for CodeRabbit, and that status is success.

The remaining block is a repository rule that the available token cannot read. The branch-protection endpoint returned HTTP 403. A repository administrator should inspect the main branch ruleset or protection configuration for required status contexts, required deployments, merge-queue requirements, or linked-issue requirements. The missing issue-approved parent is a plausible non-check gate.

---

🧠 Learnings used

Learnt from: CR
Repo: Gitlawb/zero PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T16:02:16.338Z
Learning: Applies to **/* : Formatting, vet, tests, build, smoke, diff hygiene, and vulnerability checks are hard requirements; report exact environment or unrelated external failures and obtain maintainer direction rather than silently ignoring them.

_{You are interacting with an AI system.}

[Vasanthdev2004]

@kevincodex1 correcting myself: the CodeRabbit check is not what is blocking this, and my message above was wrong about the cause.

I checked it properly instead of inferring. That check is pending on all eight open PRs, and it is also pending on #890 and #887, both of which merged. So it is not a required check and it does not block anything.

What I can still say, from outside branch protection:

• #891 is APPROVED by @anandh8x, @coderabbitai and me, base is exactly current main (2d2450e9), and every check that reports is green.
• It is the only one of the eight where BLOCKED is not explained by its reviews. The other seven are either CHANGES_REQUESTED or awaiting a first approval, and #812 reads CLEAN because its base is a feature branch rather than main.

So something in main's protection is unsatisfied that I cannot see. Reading it 404s for me (maintain, not admin), so this needs someone with admin to say what. The candidates I would check first are a required review from a specific person or team, or a required status context that no workflow produces at all, which would not show up in the checks list.

Sorry for the noise. The ask is unchanged and the reason to prioritise it stands: 1 of 3, with #897 approved and stacked directly on it, and #829's remainder behind that.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Vasanthdev2004, thanks for correcting the record.

The pending CodeRabbit check does not explain the BLOCKED state because merged PRs #890 and #887 have the same pending check.

An administrator must inspect the main branch ruleset or branch protection configuration. The administrator should check required reviewers, required status contexts, required deployments, merge-queue rules, and linked-issue requirements.

The priority rationale for #891 remains valid. It is the first PR in the three-PR stack and unblocks #897 and the remaining work from #829.

_{You are interacting with an AI system.}