fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees - #632
fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees#632euxaristia wants to merge 26 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSecret matching and overlapping-secret redaction are tightened with expanded boundary tests. Worktree preparation now cleans stale owned worktrees, manages locks, supports explicit release, and integrates lifecycle handling into CLI commands. ChangesSecret scanner and redaction
Worktree lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Exec
participant Prepare
participant Clean
participant Git
participant Release
Exec->>Prepare: prepare worktree with LeasePID
Prepare->>Clean: Clean(ctx, options, 24h)
Clean->>Git: list, inspect, unlock, remove, and prune worktrees
Prepare->>Git: create or reuse and lock worktree
Exec->>Release: release acquired lock on exit
Release->>Git: git worktree unlock
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3679cf6 to
fd1e893
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed #632 from a security-redaction and worktree-hygiene angle. Build, go vet ./..., gofmt, and go test ./internal/secrets/... ./internal/worktrees/... -count=1 are all clean; the new tests for both areas pass.
Secret redaction (internal/secrets/scanner.go)
The leak is real and this closes it for the main vectors. The original fixed-length patterns ({36}, {35}, {16}) matched only a prefix of a longer/appended key, leaving the tail un-redacted; switching to variable-length {N,} makes the greedy match consume the whole run. I traced the un-redacted tail path through Redact -> strings.ReplaceAll and confirmed the longer-key test cases (ghp_... 50 chars, AIza... 46 chars) now fully redact. The openai_key refactor is a nice precision win: the old \bsk-[A-Za-z0-9_-]{20,} matched sk-learn-machine-learning-model as a secret (I confirmed the new kebab-case test fails on the old pattern); the explicit sk-proj-/sk-svcacct- alternation plus an alnum-only legacy branch fixes that without losing real legacy or modern keys. Existing high-confidence tests still pass, so no over-redaction regression on the canonical shapes.
One minor edge case worth a look (non-blocking): the trailing \b is added to every pattern, but several body classes include -, which is a non-word character. For google_api_key, slack_token, the openai modern branch, and jwt, a secret that ends in - followed by a space backtracks and leaves the trailing - un-redacted (I verified AIzaSyA...v- -> [REDACTED]-). It is only a low-entropy char and real secrets rarely end in -, so it does not block, but it is the same tail-leak class this PR is fixing. The variable-length {N,} alone already closes the long-key leak, so the trailing \b could be dropped on the --allowing patterns without reopening it.
Worktree prune (internal/worktrees/worktrees.go)
Clean is scoped correctly in the happy path: it only removes worktrees whose path is under zero's baseDir (user/external worktrees are skipped), it uses a conservative 24h threshold, and it is auto-invoked only in production (RunGit == nil), so tests with a fake runner are unaffected. The new TestCleanPrunesStaleWorktrees verifies the call sequence.
My main concern is that "stale" is decided solely by the worktree directory's mtime, and removal uses --force. Zero does not git worktree lock its worktrees (Prepare only runs git worktree add --detach), and there is no active-task registry consulted before removal. The directory mtime is set at creation and is not refreshed when a task merely edits existing files deeper in the tree, so a long-running task (e.g. a swarm/background agent running >24h) can look stale. If another task then starts and calls Prepare, Clean will run git worktree remove --force on the active worktree, discarding any uncommitted work and leaving the running task pointing at a deleted directory. For the typical short-lived interactive worktree this is fine, and I agree the orphan-accumulation problem is real and worth solving, but the 24h-mtime + --force combination is a data-loss risk for long runs. I'd suggest at least skipping --force on the first pass (so dirty worktrees are preserved) or consulting an active-task marker before removing.
Two smaller notes:
- The baseDir ownership check is a raw
strings.HasPrefix(path, baseDir); a sibling directory like<baseDir>-otherwould satisfy it. Zero never creates worktrees there so it is practically safe, but a path-boundary check (baseDir + separator) would be more robust and avoids a Windows case-sensitivity quirk in the compare. - The PR bundles two unrelated fixes (secret scanner regex + worktree cleanup) in different packages with no code dependency. Both are small and legitimate, so I would not block on splitting, but if you prefer focused PRs these could have been two.
Verdict
Comment. The redaction fix is correct and worth merging on its own; I'd like the worktree prune's --force + mtime-only + no-active-check interaction addressed (or at least acknowledged) before I'm comfortable relying on it for long-running --worktree runs. I'm not blocking the security fix — flagging this so it isn't lost.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Thanks for these. The scanner fix is solid and ready on its own — variable-length + {N,} correctly closes the long-key tail leak, and the openai_key split is a clean precision win. I want to merge that part.
The worktree Clean is a real fix for a real leak, but the current shape is a data-loss footgun: mtime-only is not a safe stale signal, and --force will happily remove an active worktree when a long-running task (>24h) is still in it. Two concrete things I'd want addressed before I rely on this:
-
Bound the data-loss risk on long runs. The cleanest option is for
Preparetogit worktree lockeach worktree it creates, and haveCleanskip locked worktrees. A lighter alternative is totouchthe worktree dir on each subsequent in-tree operation (so liveness tracks actual use, not creation). Either is fine; please don't ship the mtime-only +--forcecombination. -
baseDirownership check is a rawstrings.HasPrefix(path, baseDir). Use a path-boundary check (baseDir + separator) so<baseDir>-otherdoesn't match, and the comparison is filesystem-correct on Windows.
Two non-blockers I'd also accept in this PR or as follow-ups:
- The trailing
\bon the-allowing patterns (google_api_key, slack_token, modern openai_key, jwt) leaves a trailing-un-redacted — same tail-leak class the PR is fixing, just with a low-entropy char. Drop\b` on those patterns only. - The PR bundles two unrelated fixes. If the worktree fix grows, splitting is fine; otherwise one PR is OK.
CI is green; once the worktree liveness signal is in and the path-boundary check is in, this is a fast approve.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Two legitimate fixes bundled in one PR. The secrets scanner changes look ready. The worktree Clean path has real data-loss risk that @anandh8x already flagged in a formal CHANGES_REQUESTED review. Merges cleanly on main after rebase (6081cf0). Gate: @anandh8x requested changes; @Vasanthdev2004 commented (not approved).
What looks good
- Secret redaction fix is correct. Variable-length
{N,}plus trailing\bon fixed-alphabet patterns closes the long-key tail leak forgithub_token,aws_access_key_id, etc. New tests (TestScanRedactsLongerKeysWithoutTailLeak) cover the failure mode. openai_keyprecision win. Splittingsk-proj-/sk-svcacct-from legacysk-[A-Za-z0-9]{20,}stops false positives likesk-learn-machine-learning-modelwithout losing real keys.- Worktree leak is real. Orphaned worktrees under
~/.local/state/zero/worktreesaccumulating is a valid hygiene problem;Cleanis scoped to zero-owned paths underbaseDirand only auto-runs in production (RunGit == nil). - Verification:
make lintclean;go test -race -count=1 ./internal/secrets/... ./internal/worktrees/...passes.
Nits
- Bundled unrelated fixes (scanner + worktrees) — fine at this size, but splittable if the worktree side grows.
- Trailing
\bon hyphen-allowing patterns (google_api_key,slack_token, modernopenai_key,jwt) can leave a trailing-un-redacted when the secret ends in-before a space. Low-entropy tail; same class as the bug being fixed. Drop\bon those patterns only (per @anandh8x / @Vasanthdev2004). _ = Clean(...)inPreparesilently swallows cleanup failures — acceptable for best-effort prune, but worth a debug log if cleanup starts mattering.
Issues
1. Clean can delete active long-running worktrees — blocking
Stale detection uses only directory mtime, and removal uses git worktree remove --force. Prepare does not git worktree lock, and there is no liveness marker. A task running >24h that doesn't touch the worktree root looks stale; the next Prepare can force-remove it mid-run, discarding uncommitted work.
Fix options (either is fine per @anandh8x):
git worktree lockon create;Cleanskips locked worktrees, or- Touch worktree dir on reuse / active use so mtime tracks liveness.
Do not ship mtime-only + --force without one of these.
2. baseDir ownership check is a raw prefix match — blocking
A sibling like <baseDir>-other/... would match. Use a path-boundary check (baseDir + string(os.PathSeparator) or filepath.Rel) so the comparison is filesystem-correct on Windows too.
Recommendation: Merge the scanner half as-is (or split the PR). Address worktree liveness + path boundary before relying on auto-prune in production.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/worktrees/worktrees.go`:
- Around line 357-361: The worktree cleanup flow around os.Stat and runGit
currently treats nonzero Git exit codes as success because defaultRunGit can
return nil errors with a failed CommandResult. Route prune and removal
operations through gitOutput or explicitly validate CommandResult.ExitCode
before reporting success, and add a test covering nonzero Git exits.
- Around line 431-448: Update worktreeIsStale to fail closed when
filepath.WalkDir or DirEntry.Info encounters an error: track inspection failure
and return false rather than treating the worktree as stale. Preserve the
existing stale=false and filepath.SkipAll behavior when a modification time is
newer than cutoff, while ensuring any incomplete inspection cannot authorize
forced removal.
🪄 Autofix (Beta)
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: 509b95aa-0ff5-4f58-8334-6f1ddeb083f4
📒 Files selected for processing (4)
internal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.go
|
Addressed both reviews. Dropped the trailing \b on the four secret patterns whose body class allows a hyphen (slack_token, google_api_key, the modern openai_key branch, jwt), which could leave a trailing hyphen un-redacted right before a delimiter. For the worktree Clean data-loss risk: staleness is no longer decided by the top-level directory's own mtime (which does not update when a task edits existing files deeper in the tree), Clean now walks the tree and treats any recently modified entry as live, and also respects an explicit git worktree lock. Replaced the baseDir ownership check's raw string prefix match with a filepath.Rel path-boundary check so a sibling directory can't false-match. Also fixed two issues CodeRabbit found in that same fix: the removal call now checks the git exit code instead of trusting a nil error, and an inspection failure during the staleness walk now fails closed instead of open. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep detecting fixed-format keys with appended word characters
internal/secrets/scanner.go:42
The new trailing word boundaries make a credential disappear from the scanner when a character outside its body class is nevertheless a word character. For example,AKIAIOSFODNN7EXAMPLEEXTRAhas no boundary after the 16th AWS body character, and aghp_key followed by_suffixlikewise has no boundary after its alphanumeric body;Scanreturns no finding and the full credential reaches the redaction/model-context path. This is the appended-key case the PR is meant to harden. Preserve redaction of the credential prefix (or otherwise consume the appended run safely) and add these delimiter cases to the regression tests. -
[P1] Do not drop OpenAI admin keys from redaction
internal/secrets/scanner.go:49
The narrowed modern-key alternative accepts onlysk-proj-andsk-svcacct-; the legacy alternative then requires the character immediately aftersk-to be alphanumeric. As a result, ansk-admin-...key matches neither branch and is emitted unchanged byRedact, whereas the previous hyphen-tolerant matcher redacted it. Keep the false-positive protection for ordinary kebab-case text while recognizing the other valid OpenAI key families, and cover them with a test. -
[P1] Restrict automatic pruning to worktrees Zero actually created
internal/worktrees/worktrees.go:350
Preparecreates worktrees below<BaseDir>/zero-worktree-<repoKey>/, butCleanauthorizes deletion of every worktree merely located anywhere below the caller-providedBaseDir. A user can point--worktree-dirat a shared directory that already has a manually managed same-repository worktree; after 24 hours of old mtimes, the next Zero prepare runsgit worktree remove --forceon that unrelated checkout and discards its uncommitted changes. Require the per-repository Zero-owned subtree (or a durable ownership marker) before pruning. -
[P1] Do not force-remove worktrees based only on filesystem mtimes
internal/worktrees/worktrees.go:368
The new recursive scan still does not establish that an unlocked worktree is abandoned. A Zero task can hold uncommitted work while waiting on a model, user, network, or other process for more than 24 hours without writing a file; anotherPreparethen classifies it stale and force-removes it. The same race exists if activity begins afterWalkDirfinishes.Preparenever locks or leases the worktrees it creates, so the locked-entry exemption does not protect the normal path. Add a durable active-worktree mechanism and/or refuse forced removal of dirty worktrees; cover active-but-idle and dirty-old cases.
|
Pushed d5146e2 for all four P1s. Appended-suffix keys: the fixed/unbounded-greedy quantifiers on aws_access_key_id, github_token, and github_pat required a trailing word boundary right after the credential body. When a word character outside the body class follows immediately (AKIA...EXAMPLEEXTRA, ghp_..._suffix), there is no word/non-word transition at that point, so the whole match failed and the credential passed through unredacted. Removed the trailing \b anchors on all three; the character class itself already stops the match where it should, and the leading \b still keeps them from firing mid-word. This matches how slack_token, google_api_key, and the modern openai_key branch already worked. Added a regression test covering both patterns with appended suffixes. OpenAI admin keys: added sk-admin- alongside sk-proj-/sk-svcacct- to the prefixed alternative so it isn't caught by the narrowed legacy branch (which requires an alphanumeric character immediately after sk-, and admin- has a hyphen there). Added it to the existing modern-prefixed-key test. Worktree pruning scope: Clean now computes the same zero-worktree- subtree Prepare creates worktrees under, and only prunes entries inside that subtree rather than anywhere under the caller-supplied BaseDir. A worktree a user manages by hand elsewhere under a shared BaseDir is never touched. Added a test with a hand-managed worktree directly under BaseDir to confirm it's skipped, and adjusted the existing sibling-prefix test to exercise the boundary at the repoDir level. Dirty worktrees: added a worktreeIsDirty check (git status --porcelain in the worktree) before force-removing anything the mtime walk flagged stale. If it has uncommitted or untracked changes, Clean skips it instead of removing it, since a task can hold live work in a worktree while waiting on a model, network, or user for longer than the staleness window without touching the tree again. The check fails closed (treats an inspection error as dirty) so a broken git call can't authorize a removal. This covers the concrete data-loss scenario in the finding; a durable lease/lock mechanism for the harder "active but never wrote anything" case would be a larger follow-up if you want it. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/worktrees/worktrees_test.go (1)
387-434: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
TestCleanSkipsWorktreeWithRecentNestedActivitydoesn't actually test deeply nested file detection.The intermediate directory
activePath/internalis created byMkdirAllbut never backdated, so its mtime ≈ now.filepath.WalkDirvisitsactivePath/internalbefore reachinghandler.go, finds it recent, and returnsnot-staleimmediately — never exercising the nested file's mtime. This means the test would still pass even ifworktreeIsStaleonly checked direct children instead of recursively walking the tree.Backdate all intermediate directories and explicitly set only the file's mtime to now so the walk must reach
handler.goto determine the worktree is not stale.🛠️ Proposed fix for test fidelity
if err := os.Chtimes(nestedDir, twoDaysAgo, twoDaysAgo); err != nil { t.Fatal(err) } // The file itself is freshly written (default mtime is now), simulating a // task actively editing code deep in the worktree. nestedFile := filepath.Join(nestedDir, "handler.go") if err := os.WriteFile(nestedFile, []byte("package pkg"), 0o644); err != nil { t.Fatal(err) } + + // Backdate all directories so that only the nested file has a recent + // mtime. Without this, activePath/internal retains its creation-time + // mtime and WalkDir finds it recent before reaching the file. + if err := os.Chtimes(filepath.Join(activePath, "internal"), twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(nestedDir, twoDaysAgo, twoDaysAgo); err != nil { + t.Fatal(err) + } + now := time.Now() + if err := os.Chtimes(nestedFile, now, now); err != nil { + t.Fatal(err) + }🤖 Prompt for 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. In `@internal/worktrees/worktrees_test.go` around lines 387 - 434, Update TestCleanSkipsWorktreeWithRecentNestedActivity to backdate every intermediate directory created under activePath, including activePath/internal, before writing the file. Ensure only nestedFile retains its current mtime, so the recursive worktreeIsStale traversal must reach handler.go to identify recent activity.
🤖 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.
Outside diff comments:
In `@internal/worktrees/worktrees_test.go`:
- Around line 387-434: Update TestCleanSkipsWorktreeWithRecentNestedActivity to
backdate every intermediate directory created under activePath, including
activePath/internal, before writing the file. Ensure only nestedFile retains its
current mtime, so the recursive worktreeIsStale traversal must reach handler.go
to identify recent activity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 094782d0-0556-42c0-b22f-07f7995446e5
📒 Files selected for processing (4)
internal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/worktrees/worktrees.go
|
Pushed 310f4a6 for the CodeRabbit test-fidelity note on TestCleanSkipsWorktreeWithRecentNestedActivity. activePath/internal was created by the same MkdirAll as the nested pkg directory but was never backdated, so it kept a fresh mtime and the staleness walk reported not-stale as soon as it reached that directory, never actually exercising the recursive check past the first level. Backdated it too so the walk has to pass through internal and pkg (both old) and reach handler.go (fresh) to reach the right answer. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not force-delete a clean worktree without an active-owner lease
internal/worktrees/worktrees.go:47-51,374-390
Preparenow runs cleanup before every production worktree creation, while the worktrees it creates are detached and never locked or leased. A live task can be clean and untouched for more than 24 hours while it waits on a user, model, network, or external process (or after it has committed intermediate work). A laterPrepareclassifies that worktree as stale and runsgit worktree remove --force, deleting the active checkout and potentially stranding its unpushed detached commit. The dirty and recent-mtime checks do not establish liveness. Keep a durable active ownership marker/lock for the lifetime of a task, or do not automatically force-remove a worktree unless there is a safe completion signal. -
[P1] Treat ignored task data as live before invoking forced removal
internal/worktrees/worktrees.go:374-390
The sole content check isgit status --porcelain, which intentionally omits ignored files, and the subsequentgit worktree remove --forceremoves those files. I reproduced this with a tracked.gitignore: anignored-datafile yields empty normal porcelain output, but--ignoredreports!! ignored-data; the forced removal then deletes the worktree. That can discard ignored local credentials, generated drafts, or task artifacts even though the code says untracked work is protected. Include ignored content in the liveness guard, or avoid forced automatic removal of worktrees containing user/task data.
|
Pushed 4b2f232 for the two remaining worktree-cleanup P1s:
Added 3 new tests plus updated 2 existing ones for the changed status command and lock call. One deliberate deviation: didn't add a |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Release Zero-owned worktree locks when the task is finished
internal/worktrees/worktrees.go:137
This adds a permanentgit worktree lockto every newly created worktree, butCleanskips every locked entry and there is nogit worktree unlockor other release path anywhere in the production lifecycle. Consequently, once a normalzero exec --worktreeorzero worktrees preparetask finishes, that checkout remains ineligible for the new 24-hour cleanup forever. The lock prevents the data-loss case from the earlier review, but it also makes the advertised disk-space cleanup inert for every worktree Zero creates. Please add a lifecycle-bound lease/unlock mechanism (or use a distinct active-task marker) so completed worktrees can become safe cleanup candidates.
|
Pushed a fix for the lock-never-released issue. Prepare locks every worktree it creates so Clean's mtime+dirty heuristic never force-removes one Zero is still using, but nothing unlocked it anywhere, so Clean was permanently inert for every Zero-created worktree. Added
Also confirmed the two scanner findings from the earlier review (appended-suffix key leaks, dropped
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/worktrees/worktrees.go (2)
425-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAggregate removal errors.
If multiple stale worktrees fail to be removed (e.g., due to file-locking or permissions),
lastErris continuously overwritten and only the final error is returned. Consider usingerrors.Jointo aggregate and surface all removal failures.🛠️ Proposed refactor
- if _, err := gitOutput(ctx, runGit, repoRoot, "worktree", "remove", "--force", entry.path); err != nil { - lastErr = fmt.Errorf("remove worktree %s: %w", entry.path, err) - } + if _, err := gitOutput(ctx, runGit, repoRoot, "worktree", "remove", "--force", entry.path); err != nil { + lastErr = errors.Join(lastErr, fmt.Errorf("remove worktree %s: %w", entry.path, err)) + }🤖 Prompt for 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. In `@internal/worktrees/worktrees.go` around lines 425 - 428, Update the stale worktree removal error handling around gitOutput so failures are accumulated with errors.Join instead of overwriting lastErr. Preserve any existing error and include each new “remove worktree” error, ensuring the final result reports all removal failures.
159-168: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSupport unlocking worktrees with missing directories.
If a user manually deletes a locked worktree directory (
rm -rf <path>),Releasewill fail to unlock it because the execution layer cannotchdirinto the non-existentpathto rungit.
Consider falling back tooptions.Cwdifpathdoes not exist. This allows users to still unlock and prune the leaked worktree by runningzero worktrees release <path>from the main repository.🛠️ Proposed fallback
func Release(ctx context.Context, options Options, path string) error { runGit := options.RunGit if runGit == nil { runGit = defaultRunGit } + + dir := path + if _, err := os.Stat(path); os.IsNotExist(err) { + if cwd, err := resolveCwd(options.Cwd); err == nil { + dir = cwd + } + } - if _, err := gitOutput(ctx, runGit, path, "worktree", "unlock", path); err != nil { + if _, err := gitOutput(ctx, runGit, dir, "worktree", "unlock", path); err != nil { return fmt.Errorf("unlock git worktree: %w", err) } return nil }🤖 Prompt for 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. In `@internal/worktrees/worktrees.go` around lines 159 - 168, Update Release to choose a valid working directory before calling gitOutput: use path when it exists, but fall back to options.Cwd when the locked worktree directory is missing. Preserve the existing worktree unlock arguments and error handling so release can unlock deleted worktrees from the main repository.
🤖 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/cli/workflows.go`:
- Around line 136-138: Normalize path to an absolute path before invoking
deps.releaseWorktree in the release workflow. Update the path variable used by
the call so both the git working directory and unlock target receive the
normalized value, while preserving the existing error handling.
---
Nitpick comments:
In `@internal/worktrees/worktrees.go`:
- Around line 425-428: Update the stale worktree removal error handling around
gitOutput so failures are accumulated with errors.Join instead of overwriting
lastErr. Preserve any existing error and include each new “remove worktree”
error, ensuring the final result reports all removal failures.
- Around line 159-168: Update Release to choose a valid working directory before
calling gitOutput: use path when it exists, but fall back to options.Cwd when
the locked worktree directory is missing. Preserve the existing worktree unlock
arguments and error handling so release can unlock deleted worktrees from the
main repository.
🪄 Autofix (Beta)
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: 662cbef2-989e-4762-b009-c0b6abd056e2
📒 Files selected for processing (6)
internal/cli/app.gointernal/cli/exec.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.go
|
Addressed CodeRabbit's review on the lock-release fix.
Added regression coverage for all three. go build, go vet, and go test -race -count=1 ./internal/worktrees/... ./internal/cli/... are all clean. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore gofmt compliance for the changed worktree test
internal/worktrees/worktrees_test.go:342-347
The Ubuntu required smoke job stops atgofmt -l .and reports this file as unformatted, so none of its vet, test, build, or smoke stages run. Run gofmt on the changed Go sources and commit the result. -
[P1] Make the relative-release test work on macOS temporary paths
internal/cli/workflow_test.go:190-217
On macOS,t.TempDir()may be spelled through/var/...whileos.Getwd/filepath.Absreturns the physical/private/var/...path. The test compares those spellings byte-for-byte and fails in the required macOS smoke job, preventing the PR from passing CI. Compare canonical/equivalent paths (or derive the expected value with the same path-resolution behavior) instead of asserting the lexical temporary-directory spelling. -
[P1] Lock a reused worktree before handing it to a new task
internal/worktrees/worktrees.go:107-116
The reuse branch returns before the newgit worktree lockcall. A normal sequence iszero worktrees prepare task-a, release it after a prior run, then preparetask-aagain for a long-lived external caller. The second caller receives an unlocked, clean, old path; another productionPreparerunsClean, which can classify it as stale and force-remove it while that caller is waiting. Re-establish the active lease for a verified reused target and cover reuse followed by a cleanup pass. -
[P1] Do not unlock a worktree lock this exec invocation did not acquire
internal/cli/exec.go:173-181
exec --worktreedefersReleasefor both newly created and reused results, althoughPreparepermits reuse of an already locked worktree. Thus runningzero exec --worktree task-aagainst a worktree an externalzero worktrees prepare task-acaller is still using clears that caller's lock on exit; a later cleanup can force-delete the clean, idle workspace. Track whether this invocation acquired the lock (or otherwise preserve the pre-existing lock) and only release that ownership. -
[P2] Wire a repository working directory into the deleted-worktree release path
internal/cli/workflows.go:146
Releaseintentionally falls back toOptions.Cwdwhen the target directory no longer exists, but this CLI call passes an emptyOptions. Invokingzero worktrees release <deleted-path>outside the source repository consequently runsgit worktree unlockwith a non-repository working directory and leaves the orphaned lock behind forever;Cleanskips locked entries. Resolve and pass the source workspace root (and add a CLI-level deleted-path test) so the advertised recovery path works independently of the caller's current directory. -
[P2] Do not report a successful exec when releasing its worktree lock failed
internal/cli/exec.go:179-181
The deferred release error is discarded. Ifgit worktree unlockfails,zero exec --worktreecan report success while leaving a lock thatCleanwill permanently skip, recreating the disk leak this change is intended to solve with no diagnostic or remediation path. Surface the failure with the affected path, or return a failure status once the run's primary result has been emitted.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review (post 7 commits, head 323b3d2)
Thanks for the thorough follow-up — nearly everything from my Jul 11 review is addressed. The secrets scanner half looks ready. The worktree lifecycle is much safer now (deep-walk staleness, isUnderDir scoping, dirty/--ignored guards, lock-on-create, and the release path).
Prior blockers — status
- ✅ Secret redaction tail-leak / appended-suffix /
sk-admin-/ kebab-case false positives - ✅
HasPrefix→ path-boundary check; prune scoped tozero-worktree-<repoKey>/ - ✅ Exit-code checking, fail-closed inspection, ignored-file dirty detection
- ✅ Lock + release lifecycle (
worktrees release,exec --worktreedefer) ⚠️ Reuse path still skips re-locking;exec --worktreeunconditionally unlocks reused worktrees
CI regression (commit 323b3d2)
acc7361 was green; latest commit broke required Smoke:
- Ubuntu —
gofmt -l .reportsinternal/worktrees/worktrees_test.go(misaligned comments ~L453) - macOS —
TestRunWorktreesReleaseNormalizesRelativePathfails on/varvs/private/varpath spelling
Remaining correctness issues
Preparereuse skips lock (worktrees.go:107-116) — re-establish the active lease when returning a reused target.exec --worktreereleases locks it didn't acquire (exec.go:179-181) — only unlock when this invocation created/locked the worktree.
Once gofmt + macOS test are fixed and the two lifecycle edge cases are addressed, this is a fast approve from my side.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review (post 7 commits, head 323b3d2)
Thanks for the thorough follow-up — nearly everything from my Jul 11 review is addressed. The secrets scanner half looks ready. The worktree lifecycle is much safer now (deep-walk staleness, isUnderDir scoping, dirty/--ignored guards, lock-on-create, and the release path).
Prior blockers — status
- ✅ Secret redaction tail-leak / appended-suffix /
sk-admin-/ kebab-case false positives - ✅
HasPrefix→ path-boundary check; prune scoped tozero-worktree-<repoKey>/ - ✅ Exit-code checking, fail-closed inspection, ignored-file dirty detection
- ✅ Lock + release lifecycle (
worktrees release,exec --worktreedefer) ⚠️ Reuse path still skips re-locking;exec --worktreeunconditionally unlocks reused worktrees
CI regression (commit 323b3d2)
acc7361 was green; latest commit broke required Smoke:
- Ubuntu —
gofmt -l .reportsinternal/worktrees/worktrees_test.go(misaligned comments ~L453) - macOS —
TestRunWorktreesReleaseNormalizesRelativePathfails on/varvs/private/varpath spelling
Remaining correctness issues
Preparereuse skips lock (worktrees.go:107-116) — re-establish the active lease when returning a reused target.exec --worktreereleases locks it didn't acquire (exec.go:179-181) — only unlock when this invocation created/locked the worktree.
Once gofmt + macOS test are fixed and the two lifecycle edge cases are addressed, this is a fast approve from my side.
|
Pushed a33cc67 and 0c8ef19 for the latest rounds from jatmn and gnanam1990.
go build, go vet, and go test ./internal/worktrees/... ./internal/cli/... pass locally. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not hand a locked worktree to another active run
internal/worktrees/worktrees.go:119-131
A secondzero exec --worktree task-atreats Git's "already locked" response as an acceptable reused result and runs in the first invocation's worktree withLockAcquired=false. When the first invocation exits it releases the sole Git lock, leaving the second still-active run unprotected; a laterPreparecan then classify the clean, old tree as stale and force-remove it. The two executions also concurrently edit the same supposedly isolated checkout. Reject an in-use lease (or implement a shared lease with correct ownership) rather than returning the path to a second live caller; preserve the actual acquisition result on the create race as well. -
[P2] Validate the request before automatic cleanup
internal/worktrees/worktrees.go:52-75
ProductionPrepareinvokes destructiveCleanbefore it resolves and validatesoptions.Name. For example,zero worktrees prepare --name ../orzero exec --worktree ../can prune stale, clean Zero-owned worktrees and only then return the invalid-name error. Move cleanup after request validation so a rejected command has no cleanup side effect. -
[P2] Make deleted-worktree release usable outside the source repository
internal/cli/workflows.go:153-157
When the worktree directory has already been deleted,Releaseneeds a working directory in the source repository. This code supplies the launch directory instead; from a non-repository directory,git worktree unlock <path>fails and leaves the orphaned lock thatCleanwill skip forever. The command has no repository argument to recover from that situation. Accept/require the source repository for this recovery path (for example with--cwd) and cover the real deleted-path case.
|
Pushed 5858220 for the July 15 round.
go build, go vet, and the worktrees and cli suites pass locally. |
|
Pushed a2e2591 for the macOS and Windows smoke failures in the new validation-order test: git records worktree paths in physical spelling, so the runners' symlinked and 8.3-short temp spellings made Clean's containment check skip the test's stale entry. The test now canonicalizes its repo and base directories up front, matching what git reports. |
c2495bf to
88ca1fe
Compare
|
Pushed 88ca1fe (rebased onto current main, including the atomic cron job-ID fix from #686) addressing the remaining worktree-ownership findings. Ownership marker
Clean repoDir keying
Package tests updated and green. |
|
Pushed cd5991e addressing the P3 shell-completion gap: |
…ease rejection, and completions - TestCleanFromLinkedWorktreePrunesStaleWorktree: pins Clean deriving its owned-subtree key from the main worktree root (not the invoking linked checkout's --show-toplevel) so Prepare/Clean run from a linked worktree actually reclaim the worktrees Prepare created there. - TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker: pins the ownership-marker requirement against the exact forgery jatmn described - a worktree under the predictable zero-worktree-<repoKey> path, manually locked with a reason that merely starts with the zero lease prefix. - Fix TestPrepareCreatesDetachedGitWorktree: its fake git runner ran out of canned results at Prepare's post-lock ownership-marker write, so writeOwnershipMarker resolved gitDir to "" and os.WriteFile wrote "zero-owner" as a relative path into the test process's real working directory instead of failing loudly. Give it autoAbsoluteGitDir like the other Prepare-exercising tests use and assert on the marker-write call. - completions_test.go: assert `worktrees`/`worktree` completions include `release` alongside `prepare`.
|
Verified the state after the last push (88ca1fe / cd5991e) against jatmn's 2026-07-22T14:36:14Z review and gnanam1990's re-review, then closed the remaining gaps. jatmn P1 (rebase) - already resolved by the time I picked this up: jatmn P2 ( jatmn P2 ( jatmn P3 ( gnanam1990's two flagged edge cases - both already fixed and covered by tests: gnanam1990's CI findings - both already fixed: While re-verifying I found and fixed one incidental bug in Verified clean: Nothing here reads as a product/policy call - everything was either already landed or a straightforward test-completeness gap. Pushed as 21bfdbc. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore Anthropic-key redaction in command output
internal/secrets/scanner.go:51
This replaces the previous generalsk-[A-Za-z0-9_-]{20,}match with a closed list of prefixes, but omits the supported Anthropic formsk-ant-apiNN-.... The repository's own general text-redactor explicitly recognizes that form (internal/redaction/redaction.go:72), whileformatBashOutputsends command stdout/stderr through this scanner directly. Consequently, a command that prints an Anthropic key is now forwarded to the model unredacted. Add the Anthropic prefix (and a regression covering bash output) before narrowing this pattern. -
[P2] Preserve an automatic cleanup path for pre-upgrade worktrees
internal/worktrees/worktrees.go:853
Every worktree created by the base version lackszero-owner, but the new automaticCleansilently skips all such stale entries. Those existing worktrees are precisely the accumulated disk leak described in the PR, so upgrading leaves them permanently unreclaimable unless a user happens to reuse each named worktree first. Provide a migration/compatibility path for legacy Zero-created entries (with an appropriate safety guard) instead of requiring a marker that did not exist when they were made. -
[P2] Roll back a lease when writing its ownership marker fails
internal/worktrees/worktrees.go:210
Aftergit worktree locksucceeds, a failure fromwriteOwnershipMarkerreturns directly. For example, an ENOSPC or permission failure in the Git admin directory leaves a newly-created or reused worktree locked but markerless.Releaserejects its missing marker andCleanskips it, sozero worktrees preparehas created a worktree that no Zero recovery command can release or prune. Undo the lock and remove a newly-created target (or otherwise make this partial state recoverable); mirror the existing lock-failure rollback and cover this failure path. -
[P2] Unlock an expired lease before pruning a missing worktree
internal/worktrees/worktrees.go:818
The code recognizes a dead PID lease, but if that worktree directory has already disappeared it only invokesgit worktree prune. Git preserves locked worktree registrations during pruning, and this branch never unlocks the expired lease, so a crashedexec --worktreewhose directory is removed remains registered forever. Recover the verified expired lease before pruning, or route this case through the same orphan-release flow.
…ng and canonicalize worktree paths
…gacy worktree cleanup
|
Addressed the review findings from @jatmn's review:
All tests pass (go test ./internal/secrets/... ./internal/worktrees/...). Ready for re-review! |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase onto the current
mainand restore the required-check baseline
91e859d0910ecea520d3f22ddcb5e5ea64fff6a2
This head is based ond9b882e, while currentmainisac50a5a, leaving it eight commits behind. The missing commits account for the Windows provider-command and terminal-rendering changes visible in a raw base-to-head comparison; those are stale-base drift, not independent defects in this PR. The submitted head nevertheless fails both macOS and Windows Smoke jobs ininternal/worktrees, and its base-to-head review surface is 163 files rather than the branch's 16-file own delta. Please rebase, rerun the platform checks, and have the resolved base-to-head diff reviewed before merge. -
[P1] Do not migrate arbitrary unmarked worktrees into Zero-owned worktrees
internal/worktrees/worktrees.go:383
isLegacyZeroWorktreeaccepts every unlocked registered worktree below the predictablezero-worktree-<repoKey>directory: after the containment check, it only requires thatrev-parse --absolute-git-dirsucceeds.Cleanthen writeszero-ownerand force-removes a stale clean match. A user can create a same-repository worktree manually in that directory, leave it clean for 24 hours, and have the nextzero worktrees preparesilently delete it. This also contradicts the existing safety test's statement that a hand-created worktree at that location must not be pruned. The legacy path needs a verifiable ownership signal or must skip markerless unlocked worktrees.
|
Carrying a finding over here so it does not get lost. #785 raised that the worktree ownership check is forgeable, and I closed it as a duplicate of this PR because the code it describes only exists on this branch, not on The point, for whenever this gets its next pass: Not adding it to the existing change requests, just making sure it is on the record here rather than in a closed issue. Credit to @euxaristia for spotting it. |
When Prepare reuses an existing worktree, it now touches the directory to refresh its mtime. This prevents Clean from force-removing a long-running but idle worktree (e.g. waiting on a model) that has no recent file changes but is still actively in use. Addresses the data-loss risk flagged in the PR review where mtime-only staleness + --force removal could discard live worktrees. Co-authored-by: cairn-code Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/completions_test.go (1)
65-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend native shell syntax validation to fish and Elvish.
Only
bashandzshcurrently run under a real interpreter;fish,powershell, andelvishare only checked for balanced blocks/braces. Usefish --no-executefor fish andelvish -compileonlyfor Elvish, while keepingexec.LookPath+ skip logic for missing interpreters. PowerShell has no standalone no-execution syntax flag, so use a parser-based check there if this coverage is required.🤖 Prompt for 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. In `@internal/cli/completions_test.go` around lines 65 - 104, Extend the native syntax validation in TestCompletionsGeneratesEverySupportedShell by setting syntaxShell for fish and elvish, then update assertNativeShellSyntax to invoke fish with --no-execute and elvish with -compileonly. Preserve exec.LookPath-based skipping when either interpreter is unavailable; leave PowerShell on its existing structural checks unless a parser-based validator is already available.
🤖 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.
Nitpick comments:
In `@internal/cli/completions_test.go`:
- Around line 65-104: Extend the native syntax validation in
TestCompletionsGeneratesEverySupportedShell by setting syntaxShell for fish and
elvish, then update assertNativeShellSyntax to invoke fish with --no-execute and
elvish with -compileonly. Preserve exec.LookPath-based skipping when either
interpreter is unavailable; leave PowerShell on its existing structural checks
unless a parser-based validator is already available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a4c9257-daa7-43bf-bb9d-e36e1c6cfea5
📒 Files selected for processing (8)
internal/cli/app.gointernal/cli/completions.gointernal/cli/completions_test.gointernal/cli/exec.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/redaction/audit_fixes_test.gointernal/redaction/redaction.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/secrets/scanner.go (1)
48-52: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOptional: the bare
ant-branch is loose enough to hit ordinary kebab-case text.
\bsk-ant-[A-Za-z0-9_-]{20,}fires on any hyphenated phrase starting withsk-ant-and 20+ word/hyphen chars (e.g.sk-ant-eater-migration-patterns-x). Real Anthropic keys are ~90+ chars, so a larger minimum on that catch-all branch would cut false positives without losing coverage;sk-ant-api\d{2}-can stay as-is.♻️ Suggested tightening
- {"openai_key", regexp.MustCompile(`\bsk-(?:proj-|svcacct-|admin-|or-v1-|ant-api\d{2}-|ant-)[A-Za-z0-9_-]{20,}|\bsk-[A-Za-z0-9]{20,}`)}, + {"openai_key", regexp.MustCompile(`\bsk-(?:proj-|svcacct-|admin-|or-v1-|ant-api\d{2}-)[A-Za-z0-9_-]{20,}|\bsk-ant-[A-Za-z0-9_-]{40,}|\bsk-[A-Za-z0-9]{20,}`)},Note the existing
TestScanDetectsAnthropicKeyscasesk-ant-1234...-12345would need lengthening if you take this.🤖 Prompt for 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. In `@internal/secrets/scanner.go` around lines 48 - 52, Increase the minimum length constraint only for the bare `sk-ant-` branch in the `openai_key` regular expression, leaving the `sk-ant-apiNN-` branch unchanged. Update `TestScanDetectsAnthropicKeys` so its bare Anthropic key fixture meets the new threshold while preserving detection coverage.internal/worktrees/worktrees.go (1)
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInjected clock is ignored for staleness timestamps.
Options.Nowis honored for the default worktree name but both the reuse touch (os.Chtimes(target, time.Now(), time.Now())) andClean'scutoffread the wall clock directly, so age-related behavior can't be driven deterministically in tests. Threadingnow()through would make the staleness window testable withoutChtimesfixture gymnastics.Also applies to: 806-806
🤖 Prompt for 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. In `@internal/worktrees/worktrees.go` around lines 161 - 168, Use the injected clock from Options.Now throughout worktree staleness handling: replace direct wall-clock reads in the reuse touch around os.Chtimes and Clean’s cutoff calculation with the shared now() value. Preserve the existing staleness window and ensure both paths use the same deterministic timestamp source.internal/worktrees/worktrees_test.go (2)
1547-1598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestCleanHonorsTouchLivenessdoesn't test touch liveness, and carries another test's doc comment.Two things here:
- Lines 1547-1552 document
worktreeIsDirty's--ignoredbehavior — that belongs onTestWorktreeIsDirtyCountsIgnoredFilesAsDirty(line 1600), not on this test.- The fixture is locked with
leaseReason(os.Getpid()), soCleanshort-circuits on the live lease before ever reachingworktreeIsStale. Theos.Chtimesrefresh at line 1586 is inert; the test would pass identically without it, so it duplicatesTestCleanHonorsLiveLeaserather than covering the touch path. To actually exercise touch liveness, drop thelockedline from the porcelain output and rely solely on the refreshed mtime (plus a queued status result if the flow reaches it).🤖 Prompt for 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. In `@internal/worktrees/worktrees_test.go` around lines 1547 - 1598, The TestCleanHonorsTouchLiveness fixture currently uses a live lease, so Clean exits before checking refreshed mtime. Move the worktreeIsDirty/--ignored documentation to TestWorktreeIsDirtyCountsIgnoredFilesAsDirty, remove the locked lease line from this test’s fake porcelain output, and provide any queued status result required for Clean to reach worktreeIsStale while retaining the aged-then-touched directory setup.
1268-1277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrphaned doc comment describes a different test.
Lines 1268-1271 document the ignored-files/
--ignoreddirty behavior, then the actual doc comment forTestCleanPreservesUnreachableCommitBeforeRemovalstarts at 1272. The stray paragraph belongs toTestWorktreeIsDirtyCountsIgnoredFilesAsDirty(line 1600).♻️ Drop the stray paragraph
-// A worktree whose only content is matched by .gitignore (credentials, -// generated drafts, task artifacts) must still block force-removal: plain -// `git status --porcelain` reports such a worktree as clean, but -// worktreeIsDirty now also passes --ignored, so Clean must treat it as dirty. // TestCleanPreservesUnreachableCommitBeforeRemoval pins the fix for a real🤖 Prompt for 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. In `@internal/worktrees/worktrees_test.go` around lines 1268 - 1277, Remove the orphaned introductory paragraph about ignored files and --ignored from the comment before TestCleanPreservesUnreachableCommitBeforeRemoval. Keep only the documentation describing preservation of unreachable commits, and retain the paragraph with TestWorktreeIsDirtyCountsIgnoredFilesAsDirty.
🤖 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/worktrees/worktrees_posix.go`:
- Around line 15-28: Validate PIDs before OS interaction in both osProcessAlive
implementations: in internal/worktrees/worktrees_posix.go lines 15-28 and
internal/worktrees/worktrees_windows.go lines 17-22, return false immediately
when pid <= 0, before os.FindProcess/Signal(0) on POSIX and before uint32
conversion/OpenProcess on Windows.
In `@internal/worktrees/worktrees.go`:
- Around line 284-294: The git worktree lock handling around defaultRunGit must
not rely on parsing the localized “already locked” message. Update the git
invocation to use a C locale, or identify an already-locked worktree through
exit status and locale-independent lock metadata, while preserving the existing
false,nil result for benign lock races and actionable error propagation for
other failures.
---
Nitpick comments:
In `@internal/secrets/scanner.go`:
- Around line 48-52: Increase the minimum length constraint only for the bare
`sk-ant-` branch in the `openai_key` regular expression, leaving the
`sk-ant-apiNN-` branch unchanged. Update `TestScanDetectsAnthropicKeys` so its
bare Anthropic key fixture meets the new threshold while preserving detection
coverage.
In `@internal/worktrees/worktrees_test.go`:
- Around line 1547-1598: The TestCleanHonorsTouchLiveness fixture currently uses
a live lease, so Clean exits before checking refreshed mtime. Move the
worktreeIsDirty/--ignored documentation to
TestWorktreeIsDirtyCountsIgnoredFilesAsDirty, remove the locked lease line from
this test’s fake porcelain output, and provide any queued status result required
for Clean to reach worktreeIsStale while retaining the aged-then-touched
directory setup.
- Around line 1268-1277: Remove the orphaned introductory paragraph about
ignored files and --ignored from the comment before
TestCleanPreservesUnreachableCommitBeforeRemoval. Keep only the documentation
describing preservation of unreachable commits, and retain the paragraph with
TestWorktreeIsDirtyCountsIgnoredFilesAsDirty.
In `@internal/worktrees/worktrees.go`:
- Around line 161-168: Use the injected clock from Options.Now throughout
worktree staleness handling: replace direct wall-clock reads in the reuse touch
around os.Chtimes and Clean’s cutoff calculation with the shared now() value.
Preserve the existing staleness window and ensure both paths use the same
deterministic timestamp source.
🪄 Autofix (Beta)
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 Plus
Run ID: 575492c4-5d1e-4a87-a383-e6f38f2b2543
📒 Files selected for processing (16)
internal/cli/app.gointernal/cli/completions.gointernal/cli/completions_test.gointernal/cli/exec.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/redaction/audit_fixes_test.gointernal/redaction/redaction.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/tools/bash_secrets_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_posix.gointernal/worktrees/worktrees_test.gointernal/worktrees/worktrees_windows.gointernal/worktrees/worktrees_windows_test.go
| func osProcessAlive(pid int) bool { | ||
| proc, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| err = proc.Signal(syscall.Signal(0)) | ||
| if err == nil { | ||
| return true | ||
| } | ||
| if errors.Is(err, os.ErrProcessDone) { | ||
| return false | ||
| } | ||
| return errors.Is(err, syscall.EPERM) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Neither osProcessAlive implementation validates the PID before handing it to the OS. A lease reason carrying 0 or a negative PID reaches the platform syscall directly: on POSIX that signals the caller's own process group (reported alive, lock never reclaimed), and on Windows it wraps through an unchecked uint32 narrowing. One shared guard fixes both.
internal/worktrees/worktrees_posix.go#L15-L28: returnfalseimmediately whenpid <= 0, beforeos.FindProcess/Signal(0).internal/worktrees/worktrees_windows.go#L17-L22: returnfalseimmediately whenpid <= 0, beforeuint32(pid)andwindows.OpenProcess.
📍 Affects 2 files
internal/worktrees/worktrees_posix.go#L15-L28(this comment)internal/worktrees/worktrees_windows.go#L17-L22
🤖 Prompt for 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.
In `@internal/worktrees/worktrees_posix.go` around lines 15 - 28, Validate PIDs
before OS interaction in both osProcessAlive implementations: in
internal/worktrees/worktrees_posix.go lines 15-28 and
internal/worktrees/worktrees_windows.go lines 17-22, return false immediately
when pid <= 0, before os.FindProcess/Signal(0) on POSIX and before uint32
conversion/OpenProcess on Windows.
Source: Linters/SAST tools
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
I like the direction here. The secret redaction changes look close to me, and the worktree lifecycle is a real step up from what @anandh8x and @gnanam1990 were blocking on — I think you've addressed locking, path containment, git exit handling, and the tail-leak patterns.
What I'm actually blocking on is two code fixes: path-canonical test fixtures, and Release using git's registered path when it unlocks. The rest I'd like you to fix or consciously accept before merge, and a few smaller things can wait.
Findings
What I need before merge
-
[P1] macOS/Windows Smoke is red because the fake-runner worktree tests aren't path-canonical
GitHub Actions run 30522781574;internal/worktrees/worktrees_test.go(~783, ~901, ~1849);internal/worktrees/worktrees.go(~804, ~931-945)
I seeSmoke (macos-latest)andSmoke (windows-latest)failing 11 tests ininternal/worktrees—TestCleanPrunesStaleWorktrees,TestPrepareValidatesNameAndExistingDirectory, andTestCleanRecoversExpiredLeaseamong them — while Ubuntu passes. I traced this to the fake-runner fixtures still using lexicalt.TempDir()paths andrepoKey(repoRoot)while production code canonicalizes viaparseWorktreeList/canonicalizePathand keysrepoDiroffrepoKey(entries[0].path). On macOS/varvs/private/var(and Windows short paths) those hashes diverge,isUnderDirnever matches, andCleanbails afterrev-parse+list+prune. You've already shown the fix in newer tests (physicalTestPath, ~676-679). I'd update the remaining fake-runner fixtures the same way, or centralize a helper. -
[P1]
Releaseverifies ownership with canonical paths but unlocks with the caller's raw spelling
internal/worktrees/worktrees.go(~327, ~438-459);internal/cli/workflows.go(~153-177)
I can pass verification with/var/.../taskwhile git registered/private/var/.../taskbecause matching usescanonicalizePath, butgit worktree unlockstill gets the unresolvedpathargument. Verify succeeds, unlock can fail, and the lease stays. The CLI only doesfilepath.Abs, not the same canonicalization you use for verification. I'd unlock with the matched porcelainentry.path(or whatever spelling git actually registered).
What I'd fix or explicitly accept
-
[P2]
Result.RepoRootdisagrees with the repo bucket used forResult.Path
internal/worktrees/worktrees.go(~123-128)
If I runPreparefrom a linked worktree,Pathlands underzero-worktree-<hash(main)>butRepoRootcomes fromrev-parsefor my cwd. Anything deriving layout fromRepoRootgets the wrong bucket. I'd either setRepoRoottoprimaryRootor document that onlyPathis authoritative. -
[P2] Crashed
exec --worktreeblocks name reuse until the worktree goes stale (~24h)
internal/worktrees/worktrees.go(~829-860, ~153-158)
If the owning process dies before deferredrelease, I seeCleanmarkexpiredLeasebut only unlock/remove insideworktreeIsStale. A fresh locked worktree blocks the nextPrepareon that name until I runzero worktrees releasemanually or wait out the staleness window. I'd unlock dead PID leases without waiting for staleness, or reclaim them on reuse inPrepare. -
[P2] Pre-marker locked worktrees can't be released via
zero worktrees release
internal/worktrees/worktrees.go(~481-487, ~391-404)
Legacy locked worktrees without azero-ownermarker failReleasewhile the directory still exists; migration only runs on theCleanstale-prune path. I'd mirror that inRelease, or write the marker on first post-upgrade reuse. -
[P2] Prepare lock-collision errors aren't redacted at the CLI boundary
internal/worktrees/worktrees.go(~158, ~217);internal/cli/workflows.go(~94);internal/cli/exec.go(~209)
You redactzero worktrees releaseerrors, butprepareandexec --worktreestill forwardPrepareerrors verbatim. The collision message prints the full worktree path twice. I'd redact those the same way. -
[P2]
Cleancan remove a manual worktree I placed under Zero's predictable layout
internal/worktrees/worktrees.go(~391-404, ~884-892)
isLegacyZeroWorktreedoesn't require a Zero lease or marker for unlocked entries. If Igit worktree addunderzero-worktree-<repoKey>/and it goes stale,Cleancan force-remove it even thoughReleasewould refuse. I'd tighten the heuristic or document that namespace as Zero-only. -
[P2] Custom
--dirworktrees only get cleaned whenPrepareuses the sameBaseDir
internal/worktrees/worktrees.go(~86-87, ~759-804)
Stale worktrees under a custom base stick around until something callsPreparewith that sameBaseDiragain. I'd document that coupling or persist enough to prune across bases. -
[P2] macOS upgrades may orphan old worktree buckets
internal/worktrees/worktrees.go(~104-123, ~804)
After switching toprimaryWorktreeRoot,repoKey("/var/foo/repo")andrepoKey("/private/var/foo/repo")hash differently. Oldzero-worktree-<old-hash>/trees fall outside whatCleansees. I'd normalizerepoKeythe same wayparseWorktreeListdoes, and/or prune legacy buckets inClean.
Follow-ups I'd file separately
-
[P3] Text-mode
worktrees preparedoesn't tell me I need to release
internal/cli/workflows.go(~725-741)
JSON haslockAcquired; plain text doesn't. If I'm scripting without--json, I won't know a release is required. -
[P3] Anthropic keys show up as
[REDACTED:openai_key]
internal/secrets/scanner.go(~48-52)
Detection works, but the label is wrong forsk-ant-*. I'd give it its own type if anyone filters on placeholder names. -
[P3] Some hyphenated
sk-keys slip through
internal/secrets/scanner.go(~52)
Something likesk-live-1234567890abcdef1234567890abcdefdoesn't match either branch. The old pattern caught more hyphenated bodies. I'd extend the prefix list if you mean to cover more providers. -
[P3]
Cleanswallows unlock/prune errors when the directory is already gone
internal/worktrees/worktrees.go(~848-853)
The stale-removal path records unlock failures (~902-904); the missing-directory path uses_, _ = …. I'd join those errors intolastErrfor consistency. -
[P3] The PR description doesn't match what you shipped
PR body;internal/cli/workflows.go(~887-888)
Three things I'd fix in the description: (1) issue 1 says you added trailing\b, butscanner.godeliberately omits it; (2) the "Changes" list skips the CLI, redaction, platform shims, and bash tests; (3) release help says-Cis required when the directory was deleted, butrunWorktreesReleasewires cwd without it (~174-175).
…ot to primaryRoot, and add anthropic_key pattern Unlock using matched porcelain entry path, set Result.RepoRoot to primaryRoot in Prepare, add dedicated anthropic_key secret pattern, and canonicalize fake-runner test paths. Refs Gitlawb#632
…worktrees (#855) * fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees 1. Prevent trailing redaction leaks in github_token, aws_access_key_id, and google_api_key by adding trailing word boundaries and allowing variable lengths. Refine the openai_key pattern to cleanly distinguish legacy keys and modern prefixed keys (sk-proj-, sk-svcacct-) from ordinary kebab-case phrases. 2. Implement auto-pruning of zero-owned git worktrees older than 24 hours at the start of worktrees.Prepare to prevent indefinite disk space leaks. * fix(secrets,worktrees): close tail-leak edge case and worktree data-loss risk Drop the trailing \b anchor on the four secret patterns whose body class allows "-" (slack_token, google_api_key, the modern openai_key branch, jwt). \b requires a word/non-word transition, so a secret ending in "-" right before a delimiter has none, and the engine backtracked the greedy quantifier to drop that last character instead of failing the match, leaking it. The body character class already provides the real stopping boundary, so the anchor was unnecessary. Fix two issues in worktree Clean flagged in review: - Staleness was decided by the worktree directory's own mtime, which only changes when an entry is added/removed/renamed directly inside it, not when a long-running task edits existing files deeper in the tree. Clean now walks the tree and treats any recently modified entry as live, and also skips any worktree a caller has explicitly locked via git worktree lock. - baseDir ownership used a raw strings.HasPrefix, so a sibling like "<baseDir>-other" would false-match. Replaced with a filepath.Rel path-boundary check. * fix(worktrees): check exit codes on removal, fail closed on inspection errors defaultRunGit deliberately returns a nil error alongside a nonzero CommandResult.ExitCode for a failed git invocation, so the worktree remove call must check ExitCode itself instead of trusting a nil error to mean success. Route it through gitOutput, which already does that. worktreeIsStale treated an inspection failure (an unreadable file, a WalkDir error) the same as "keep walking," which can let an incompletely-inspected worktree be judged stale. Any inspection error now makes it ineligible for removal instead. * fix(secrets,worktrees): catch appended-suffix keys and scope pruning to owned worktrees The scanner's trailing \b anchors made a credential vanish entirely when followed by a word character outside its body class (an appended suffix like AKIA...EXTRA, or ghp_..._suffix): the fixed or unbounded-greedy quantifier had no valid word boundary to land on and the whole match failed, so the real secret reached the redaction output unredacted. Dropping the trailing anchors lets the body class itself stop the match, so the credential prefix still gets redacted even when noise follows it. Also recognize sk-admin- alongside sk-proj-/sk-svcacct- so OpenAI admin keys aren't skipped by the narrowed modern-key branch. Clean pruned any worktree under the caller-supplied BaseDir, but Prepare only ever creates worktrees under a per-repository zero-worktree-<repoKey> subtree of it. Scope pruning to that subtree so a worktree a user manages by hand elsewhere under a shared BaseDir is never force-removed. Also refuse to force-remove a worktree whose mtime looks stale but that still has uncommitted or untracked changes: a task can hold live work while waiting on a model, network, or user for longer than the staleness window without writing to the tree again. * fix(worktrees): make nested-activity test exercise the deep walk it claims to activePath/internal was created by the same MkdirAll as the nested pkg dir but never backdated, so it kept a fresh mtime and worktreeIsStale's walk reported "not stale" as soon as it hit that directory, before ever reaching the freshly-written file two levels deeper. The test passed without actually exercising recursion past the first directory. * fix(worktrees): lock zero-created worktrees and treat ignored files as dirty Prepare never called git worktree lock, so the entry.locked skip in Clean only ever protected worktrees a human locked by hand, never zero's own; a worktree that finished committing and sat idle (e.g. waiting on a slow model or network retry) for more than 24h looked clean-and-stale and got force-removed by the mtime+dirty heuristic alone. Lock every worktree Prepare creates so it gets the same protection. worktreeIsDirty also used git status --porcelain with no --ignored, so a worktree holding only .gitignore-matched task data (credentials, generated drafts, artifacts) reported as clean and got force-removed with --force, silently discarding it. Add --ignored so those files count as dirty too. * fix(worktrees): release Zero's Prepare lock so Clean can reclaim finished worktrees Prepare locks every worktree it creates so Clean's mtime+dirty staleness heuristic never force-removes one Zero is still using, but nothing ever unlocked it, making the automatic disk-space cleanup permanently inert. Add Release (git worktree unlock) and wire it in two ways: zero exec --worktree defers a release once its own run finishes, since that flow's use of the worktree is bound to its own process. zero worktrees prepare hands the path to a longer-lived external caller with no defined end-of-life, so a new zero worktrees release <path> subcommand lets that caller release it explicitly when done. * fix(worktrees): normalize release path, aggregate Clean errors, unlock deleted worktrees Address CodeRabbit's review on the lock-release fix: - zero worktrees release now resolves its path argument to absolute before calling Release, since git worktree unlock matches against the path git recorded at creation, not whatever directory the caller happens to be running from. - Clean now aggregates removal failures with errors.Join instead of overwriting lastErr, so multiple stale worktrees failing removal in the same pass are all reported, not just the last one. - Release falls back to options.Cwd as the git working directory when the worktree path itself no longer exists (e.g. a caller deleted a locked worktree by hand instead of releasing it first), so the orphaned lock can still be cleared. Added regression coverage for all three. * style(worktrees): align aggregation test comments to gofmt output * fix(worktrees,cli): restore reuse lease and scope unlock to owned locks - Prepare re-locks a reused worktree so Clean's staleness heuristic cannot force-remove it while the new caller is still using it; a lock already held by a live external caller is kept in place and reported through the new Result.LockAcquired field. - exec --worktree only releases the lock its own Prepare call acquired and surfaces a failed release on stderr with the affected path instead of discarding the error. - worktrees release wires the resolved workspace root into Options.Cwd so the deleted-path recovery works outside the worktree directory. - The relative-path release test derives its expected value via filepath.Abs, matching the resolution the CLI uses, so macOS /var vs /private/var spellings no longer break it. * fix(worktrees,cli): reject in-use leases, validate before cleanup, add release -C - Prepare rejects a worktree whose lock another run still holds, on both the reuse and the create-race paths, instead of handing a second live caller an unprotected shared checkout whose sole Git lock the first caller's exit would release. - The automatic stale-worktree pruning runs only after the request itself validates, so a rejected command (an invalid --name) has no destructive cleanup side effect; covered end to end with real git in both directions. - worktrees release accepts -C/--cwd naming the source repository, which the deleted-path recovery needs when launched outside the repo (the deleted worktree path is a one-way hash with no way back to its source). * test(worktrees): canonicalize test roots to physical spelling git records worktree paths in physical form, so the CI runners' symlinked (/var -> /private/var) and 8.3-short (RUNNER~1) temp spellings made Clean's containment check skip the test's stale entry and the pruning assertion fail on macOS and Windows. * fix(worktrees): preserve orphaned commits, verify release ownership, canonicalize base dir Four fixes from the latest review round: - Clean now creates a durable ref (refs/zero/orphaned-worktree/<sha>) for a detached worktree's HEAD before force-removing it, when that commit isn't already reachable from any other ref. Prepare always creates worktrees with `worktree add --detach`, so a commit made there had no ref pointing at it once the worktree was deleted, making it immediately eligible for git gc despite never having been merged/pushed elsewhere. - Clean resolves its configured base directory through EvalSymlinks before comparing it against git's reported worktree paths: `git worktree list --porcelain` reports each worktree's PHYSICAL location (resolving symlink components), so a symlinked --worktree-dir made every worktree created under it permanently unprunable. - Release now verifies path has a zero-worktree-<repoKey> ancestor directory component before running `git worktree unlock`, so it can't be used to clear the lock on a worktree a user or another tool manages by hand. The check doesn't need to know which --dir a given Prepare call used (nothing records that against a specific worktree, and the CLI never threads BaseDir through to Release) — the repoKey component is Prepare's actual ownership signature regardless of which directory it was created under. - Route the release command's printed path and exec's release-failure diagnostic through the existing CLI redaction helper, and split the worktrees help text into prepare-specific and release-specific flag sections (release only ever supported -C/--cwd, not the --name/--dir/ --json the shared block advertised). All four have regression tests confirmed to fail without their fix (two using real git worktrees, not just fakeRunner sequences). Build, vet, and gofmt clean on linux/windows/darwin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(worktrees): recoverable PID leases and reclaimable released worktrees Two review findings on the cleanup lifecycle: - A lock left by an abnormal exit (SIGKILL, crash, power loss) was skipped by Clean forever, recreating the permanent disk leak this PR set out to fix. exec --worktree now records its PID in the lease reason; Clean expires a lease whose recorded owner is provably dead, unlocking only after the staleness, dirty, and HEAD-preservation guards all pass. Human locks and PID-less leases (external `worktrees prepare` owners) remain permanent until explicit release, and any ambiguity in the liveness probe counts as alive. - An explicitly released worktree holding only gitignored residue (node_modules, build output) was skipped at every age. Release is the owner's completion signal, so unlocked entries now block removal only on tracked/untracked changes; expired crashed leases keep the conservative --ignored probe since they never signaled completion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(worktrees): address review feedback on lease detection and release safety Split dead-lease PID checking into posix/windows implementations so Windows can reliably tell a dead process from a live one. Fix a path canonicalization mismatch in two release tests. Derive release ownership from git worktree list instead of the git-dir parent, which was wrong for repos with a separate git-dir. Refuse to clear a lock that was not taken by Zero in the first place. * fix(worktrees): canonicalize paths for clean/release ownership checks Compare Clean containment and Release ownership against physical path spellings so macOS /var vs /private/var and symlink TMPDIR layouts match git worktree list. Require a registered porcelain entry and a Zero lease reason before unlock; treat an already-unlocked Zero worktree as a no-op. * fix(worktrees,secrets): address jatmn review findings on #632 - Prepare and Release now agree on repoKey regardless of which worktree (main or linked) Prepare runs from, by keying off git worktree list's first entry (always the main worktree) instead of --show-toplevel. A worktree prepared from a linked checkout previously failed its own ownership check on release and its lease could never be cleared. - osProcessAlive on Windows no longer treats every OpenProcess failure as "process is dead": only ERROR_ACCESS_DENIED (a live process this caller lacks rights to query) is now distinguished from a genuinely missing PID, so Clean can no longer force-remove an active worktree whose owning process it simply couldn't query. - The openai_key redaction pattern now recognizes sk-or-v1- (OpenRouter) alongside the existing sk-proj-/sk-svcacct-/sk-admin- prefixes, so hyphenated OpenAI-compatible provider keys are redacted again without reopening the sk-<kebab-case-phrase> false-positive this pattern was narrowed to avoid. - Release/exec --worktree error text is redacted before reaching stderr, matching the already-redacted success path; ownership errors interpolate the caller-supplied path, which could carry a key-shaped segment. - canonicalizePath resolves symlinks through the nearest existing ancestor when the target itself no longer exists, so the documented `release -C` recovery path works again for a worktree deleted by hand under a symlinked --worktree-dir. - Prepare rolls back the worktree `git worktree add` just created if the subsequent lock call fails for a reason other than a concurrent racer, instead of leaking an unleased checkout until Clean's 24h staleness window reclaims it. Not addressed here: the P2 finding that worktree ownership is provable only by a directory-name convention plus a lock-reason prefix, both of which a user can reproduce by hand. A durable per-worktree ownership marker would close that gap, but internal/worktrees has been on main since #70, so a marker requirement could reject worktrees an already-installed zero created before this change existed. Needs a decision on migration before implementing. * fix(worktrees): prove Prepare ownership with a git-admin marker Address review findings that path convention plus lease-reason prefix are forgeable by hand. Prepare now writes a zero-owner marker into the worktree admin dir; Release and Clean require it before force-touching a path. Clean also keys its owned subtree off the main worktree root so linked-checkout calls still prune the same bucket Prepare uses. Tests plant the marker and list the main worktree first so fixtures match production. * fix(cli): complete worktrees release in shell completions * test(worktrees,cli): add coverage for linked-worktree Clean, forged lease rejection, and completions - TestCleanFromLinkedWorktreePrunesStaleWorktree: pins Clean deriving its owned-subtree key from the main worktree root (not the invoking linked checkout's --show-toplevel) so Prepare/Clean run from a linked worktree actually reclaim the worktrees Prepare created there. - TestReleaseRejectsForgedZeroLeaseWithoutOwnershipMarker: pins the ownership-marker requirement against the exact forgery jatmn described - a worktree under the predictable zero-worktree-<repoKey> path, manually locked with a reason that merely starts with the zero lease prefix. - Fix TestPrepareCreatesDetachedGitWorktree: its fake git runner ran out of canned results at Prepare's post-lock ownership-marker write, so writeOwnershipMarker resolved gitDir to "" and os.WriteFile wrote "zero-owner" as a relative path into the test process's real working directory instead of failing loudly. Give it autoAbsoluteGitDir like the other Prepare-exercising tests use and assert on the marker-write call. - completions_test.go: assert `worktrees`/`worktree` completions include `release` alongside `prepare`. * fix(redaction,worktrees): sort extra secret values by length descending and canonicalize worktree paths * fix(secrets,worktrees): restore Anthropic key redaction and handle legacy worktree cleanup * fix(secrets): add bash output redaction regression test for Anthropic API keys * fix(worktrees): touch worktree mtime on reuse to prevent stale pruning When Prepare reuses an existing worktree, it now touches the directory to refresh its mtime. This prevents Clean from force-removing a long-running but idle worktree (e.g. waiting on a model) that has no recent file changes but is still actively in use. Addresses the data-loss risk flagged in the PR review where mtime-only staleness + --force removal could discard live worktrees. Co-authored-by: cairn-code Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com> * fix(worktrees): properly handle os.Chtimes error on reused worktree path Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com> * fix(worktrees,secrets): unlock using porcelain entry path, set RepoRoot to primaryRoot, and add anthropic_key pattern Unlock using matched porcelain entry path, set Result.RepoRoot to primaryRoot in Prepare, add dedicated anthropic_key secret pattern, and canonicalize fake-runner test paths. Refs #632 * fix(tools): correct redaction placeholder assertion in Anthropic key test TestFormatBashOutputRedactsAnthropicKey checked for the openai_key placeholder instead of anthropic_key, a copy-paste leftover. The redaction itself was already correct; only the assertion was wrong. * test(worktrees): physicalize test paths for macOS tempdir symlink resolution * fix(secrets,worktrees): address CodeRabbit review findings Align RedactString token boundaries with secrets.Scan, strengthen the Anthropic bash redaction assertion, fix Clean fixtures so they reach the guards under test, and fail closed on ambiguous processAlive probes. * fix(secrets,worktrees): restore broad key redaction and legacy Clean safety Address human review on #855: keep sk- bodies with a digit filter instead of enumerated vendor prefixes, add a looser JWT form, restore the sk-test fixture, probe legacy ownership before dirty, treat non-INVALID Windows OpenProcess errors as alive, redact Abs/cwd release errors, and reclaim dead-owner leases on Prepare reuse. * fix(secrets,redaction): always redact known OpenAI key prefixes Alphabet-only sk-proj-/sk-svcacct-/sk-admin- tokens are still credentials; keep the digit filter only for unknown sk- vendor forms so kebab phrases like sk-learn-… stay un-redacted. * Preserve digit-free legacy keys during redaction. Refs #855 * fix(secrets,worktrees): address CodeRabbit findings on PR #855 Redact full compact JWE tokens, pin git locale for lock parsing, write the ownership marker atomically, surface missing-dir unlock failures, and fail closed when HEAD probes are indeterminate. Align Clean fixtures and digit-free known-prefix redaction tests with the shipped behavior. Refs #855 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: cairn-code <282421612+cairn-code@users.noreply.github.com> Co-authored-by: euxaristia <euxaristia@users.noreply.github.com>
5 participants
Summary
Fixes two Medium-severity issues:
Partial Redaction / Secret Leak on Longer or Appended Keys:
github_token,aws_access_key_id, andgoogle_api_keywere matching fixed lengths with no trailing boundary anchor. Slightly longer or appended keys would only have their prefix redacted, leaking the tail.\band allowing variable lengths ({36,}etc.) solves this.openai_keypattern to cleanly distinguish legacy keys (sk-+ 20+ alnum characters) and modern prefixed keys (sk-proj-/sk-svcacct-) from ordinary kebab-case phrases.Git Worktrees Disk Space Leak:
~/.local/state/zero/worktreesaccumulated indefinitely on the user's system, consuming substantial disk space.Cleanfunction which lists worktrees viagit worktree list --porcelain, identifies zero-owned worktrees, and callsgit worktree remove --forceon those older than 24 hours.Cleanautomatically at the start ofPreparein production.Changes
internal/secrets/scanner.goandinternal/secrets/scanner_test.go.internal/worktrees/worktrees.goandinternal/worktrees/worktrees_test.go.Test plan
go test -race ./internal/secrets/... ./internal/worktrees/...— okSummary by CodeRabbit
zero worktrees release <path>and updated shell completions to includerelease.exec --worktreenow records and releases locks after the command finishes.--exec-profileand--no-completion-gate;zero --versionoutput adapts for TTY vs non-TTY.