fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees - #855
fix(secrets,worktrees): fix secret redaction leakage and prune stale worktrees#855euxaristia wants to merge 32 commits into
Conversation
…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.
…oss 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.
…n 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.
…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.
…laims 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.
…s 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.
…shed 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.
…k 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.
- 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.
…d 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).
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.
…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>
…rees 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>
…e 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.
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.
- 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 Gitlawb#70, so a marker requirement could reject worktrees an already-installed zero created before this change existed. Needs a decision on migration before implementing.
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.
…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`.
…ng and canonicalize worktree paths
…gacy worktree cleanup
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>
…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
WalkthroughThe change adds worktree locking, release, stale cleanup, lease recovery, and CLI integration. It also expands secret detection and redaction for modern credential formats, overlapping values, suffixes, and false-positive cases. ChangesWorktree lifecycle
Secret detection and redaction
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant WorktreeService
participant Git
participant Lease
CLI->>WorktreeService: prepare worktree
WorktreeService->>Git: resolve repository and worktree
WorktreeService->>Lease: acquire lock and record PID
WorktreeService-->>CLI: return worktree and LockAcquired
CLI->>WorktreeService: release worktree
WorktreeService->>Git: validate and unlock worktree
WorktreeService-->>CLI: report release result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
…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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/worktrees/worktrees_posix.go (1)
20-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefault unknown signal errors to alive, so the fail-closed contract holds.
processAlivedocuments that any ambiguity counts as alive, so an uncertain answer can never expire a lease. This implementation returns alive only fornilandEPERM. Every other error, including an unexpected one, returns false and marks the lease expired. That path letsCleanunlock and force-remove a worktree based on an inconclusive probe.Treat only the definite "no such process" answers as dead.
🛡️ Proposed fix
err = proc.Signal(syscall.Signal(0)) if err == nil { return true } - if errors.Is(err, os.ErrProcessDone) { + // Only ESRCH / ErrProcessDone prove the PID names no live process. Any + // other error is inconclusive, so report alive and keep the lease. + if errors.Is(err, os.ErrProcessDone) || errors.Is(err, syscall.ESRCH) { return false } - return errors.Is(err, syscall.EPERM) + return true }🤖 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 20 - 27, Update the error handling in processAlive so only the definite no-process result (os.ErrProcessDone) returns false; retain true for successful probes and permission errors, and default all other unexpected signal errors to true to preserve the fail-closed lease behavior.internal/cli/workflow_test.go (2)
325-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo secret scanners flag these synthetic key literals.
The literals are clearly fake fixtures for the redaction assertions, and the tests are valuable. Both Betterleaks and ast-grep report them as hard-coded credentials, which adds recurring noise to every scan of this file. Assemble the fixture at runtime from fragments, or add the scanners' inline allow annotation, so the finding does not have to be triaged again.
♻️ Proposed fix
- secret := "sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu" + // Assembled at runtime so secret scanners do not report this synthetic + // redaction fixture as a hard-coded credential. + secret := "sk-" + "proj-abcDEF123_ghiJKL456-mnoPQR789stu"Also applies to: 933-933
🤖 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/workflow_test.go` at line 325, Update the synthetic secret fixtures in the workflow tests, including both occurrences of the key literal, so scanners no longer classify them as hard-coded credentials. Assemble each fixture at runtime from non-secret fragments, or apply the repository-supported inline allow annotation while preserving the existing redaction assertions.Source: Linters/SAST tools
187-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Chdirfor the workflow test temporary directory.
go.moddeclares Go 1.26.5, sotesting.T.Chdiris available. Use it instead of manualos.Chdirplus deferred restore; it restores the working directory automatically during cleanup.♻️ Proposed fix
- origWd, err := os.Getwd() - if err != nil { - t.Fatal(err) - } parent := t.TempDir() worktreeDir := filepath.Join(parent, "task-a") if err := os.Mkdir(worktreeDir, 0o700); err != nil { t.Fatal(err) } - if err := os.Chdir(parent); err != nil { - t.Fatal(err) - } - defer func() { - if err := os.Chdir(origWd); err != nil { - t.Fatal(err) - } - }() + t.Chdir(parent)🤖 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/workflow_test.go` around lines 187 - 203, Update the workflow test’s working-directory setup to use testing.T.Chdir with the temporary parent directory, and remove the manual os.Getwd, os.Chdir, and deferred restoration logic. Preserve the existing worktreeDir creation and test behavior.internal/cli/workflows.go (1)
133-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree small CLI UX inconsistencies in the release path.
- Line 180 prints the raw
pathargument, but line 177 releasesabsPath. A user who runszero worktrees release task-aseesreleased task-awhile git unlocked an absolute path. Report the path that was acted on.- Line 134 does not trim the
--cwd=value, whileparseWorktreeCommandArgstrims the same flag at line 405.--cwd=" "then reachesresolveWorkspaceRootas whitespace.- In the help text,
-h, --helpis now the last entry of therelease flags:block, so it reads as release-only. It applies to both subcommands.♻️ Proposed fixes
case strings.HasPrefix(arg, "--cwd="): - cwdFlag = strings.TrimPrefix(arg, "--cwd=") + cwdFlag = strings.TrimSpace(strings.TrimPrefix(arg, "--cwd="))- if _, err := fmt.Fprintf(stdout, "released %s\n", redactCLIString(path)); err != nil { + if _, err := fmt.Fprintf(stdout, "released %s\n", redactCLIString(absPath)); err != nil { return exitCrash }release flags: -C, --cwd <path> Source repository directory (required if the worktree directory was already deleted) + +common flags: -h, --help Show this helpAlso applies to: 180-182
🤖 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/workflows.go` around lines 133 - 134, Update the release workflow to report the acted-on absolute path variable (absPath) instead of the raw path argument, trim whitespace from the --cwd= value when assigning cwdFlag in argument parsing, and move the -h, --help entry in the release flags help text so it clearly applies to both release subcommands.internal/worktrees/worktrees_test.go (1)
1268-1272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo doc comments describe tests that are not below them.
At lines 1268-1272 a comment about
--ignoredblocking force-removal sits directly aboveTestCleanPreservesUnreachableCommitBeforeRemoval, which tests commit preservation. At lines 1547-1553 a comment aboutworktreeIsDirtyand--ignoredsits aboveTestCleanHonorsTouchLiveness. It looks like the tests those paragraphs belonged to moved. Move each paragraph onto its own test or delete it, so the next reader does not trust the wrong description.Also applies to: 1547-1553
🤖 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 - 1272, Correct the misplaced doc comments in internal/worktrees/worktrees_test.go: move the --ignored/worktreeIsDirty force-removal explanation to the test that verifies ignored-only content is treated as dirty, and move the commit-preservation explanation to TestCleanPreservesUnreachableCommitBeforeRemoval. Ensure each paragraph directly precedes the test it describes, or remove it if no matching test exists.internal/worktrees/worktrees.go (1)
284-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a locale-stable signal for
git worktree lockcollisions.
lockWorktreetreatsalready lockedas a non-lock failure, but that message is translated underLANG/LC_MESSAGES. A localized message can cause Prepare to classify a live worktree collision as a real failure and roll back/remove the newly created worktree. Run the lock throughdefaultRunGitwithLC_ALL=C, or use a locale-independent signal such as the lock exit code plus another stable Git state (for example, querying the worktree list before runningadd).🤖 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 284 - 294, Update lockWorktree to classify existing-lock collisions using a locale-stable signal instead of matching the localized “already locked” text. Run git worktree lock through defaultRunGit with LC_ALL=C, or use the lock exit code together with another stable Git-state check, while preserving successful locks and genuine error propagation.
🤖 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/redaction/redaction.go`:
- Around line 160-168: Update the textSecretPatterns used by RedactString to
match secrets.Scan’s end-boundary behavior and supported secret formats,
removing trailing \b behavior that misses appended suffixes or leaves terminal
hyphens after backtracking. Add parity tests exercising RedactString with
appended suffixes and secrets ending in hyphens.
In `@internal/tools/bash_secrets_test.go`:
- Around line 28-36: Update TestFormatBashOutputRedactsAnthropicKey to store the
complete Anthropic key fixture in a key variable, pass it into formatBashOutput,
and assert that the resulting output does not contain the full key while
retaining the typed redaction-placeholder assertion.
In `@internal/worktrees/worktrees_test.go`:
- Around line 1474-1481: Update the Clean-related fixtures, including the test
covering ignored data and TestCleanSkipsWorktreeWithRecentNestedActivity,
TestCleanHonorsLiveLease, TestCleanHonorsTouchLiveness, and
TestCleanSkipsDirtyStaleWorktree, to list the main worktree rooted at repoRoot
before the target entry, matching TestCleanRecoversExpiredLease. Pass tempDir
through physicalTestPath where required, and add positive assertions for the
expected git call sequence so the tests genuinely reach the lease, status, and
removal logic.
---
Nitpick comments:
In `@internal/cli/workflow_test.go`:
- Line 325: Update the synthetic secret fixtures in the workflow tests,
including both occurrences of the key literal, so scanners no longer classify
them as hard-coded credentials. Assemble each fixture at runtime from non-secret
fragments, or apply the repository-supported inline allow annotation while
preserving the existing redaction assertions.
- Around line 187-203: Update the workflow test’s working-directory setup to use
testing.T.Chdir with the temporary parent directory, and remove the manual
os.Getwd, os.Chdir, and deferred restoration logic. Preserve the existing
worktreeDir creation and test behavior.
In `@internal/cli/workflows.go`:
- Around line 133-134: Update the release workflow to report the acted-on
absolute path variable (absPath) instead of the raw path argument, trim
whitespace from the --cwd= value when assigning cwdFlag in argument parsing, and
move the -h, --help entry in the release flags help text so it clearly applies
to both release subcommands.
In `@internal/worktrees/worktrees_posix.go`:
- Around line 20-27: Update the error handling in processAlive so only the
definite no-process result (os.ErrProcessDone) returns false; retain true for
successful probes and permission errors, and default all other unexpected signal
errors to true to preserve the fail-closed lease behavior.
In `@internal/worktrees/worktrees_test.go`:
- Around line 1268-1272: Correct the misplaced doc comments in
internal/worktrees/worktrees_test.go: move the --ignored/worktreeIsDirty
force-removal explanation to the test that verifies ignored-only content is
treated as dirty, and move the commit-preservation explanation to
TestCleanPreservesUnreachableCommitBeforeRemoval. Ensure each paragraph directly
precedes the test it describes, or remove it if no matching test exists.
In `@internal/worktrees/worktrees.go`:
- Around line 284-294: Update lockWorktree to classify existing-lock collisions
using a locale-stable signal instead of matching the localized “already locked”
text. Run git worktree lock through defaultRunGit with LC_ALL=C, or use the lock
exit code together with another stable Git-state check, while preserving
successful locks and genuine error propagation.
🪄 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: 06ea9d09-39af-4492-995c-e3afd7f986f2
📒 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
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.
|
Addressed CodeRabbit findings in
@coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes. The worktree lease/ownership work is genuinely good — the marker-in-the-git-admin-dir trick is the right ownership proof, the fail-closed staleness and dirty probes are careful, and I mutation-tested a couple of the guards (deleting the ownership-marker rejection and forcing includeIgnored=false both turn the right tests red, so they're real). But two things need fixing before this can land: the redaction change is a net loss of coverage relative to main, and the new auto-Clean destroys data on upgrade.
The openai_key pattern got narrower, not wider
internal/secrets/scanner.go:49 (and its mirror at internal/redaction/redaction.go:79) went from
\bsk-[A-Za-z0-9_-]{20,}
to
\bsk-(?:proj-|svcacct-|admin-|or-v1-)[A-Za-z0-9_-]{20,}|\bsk-[A-Za-z0-9]{20,}
The fallback branch no longer allows - or _, so any sk-<vendor>-<body> key whose vendor segment isn't one of the four you enumerated matches nothing at all. I drove this through the real entry point (formatBashOutput → secrets.Redact) with sentinel tokens:
sk-fw-SENTINEL_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa -> leaked verbatim
sk-live-SENTINELaaaaaaaaaa_bbbbbbbbbb-cccc -> leaked verbatim
Both are matched by main's regex. Same result through redaction.RedactString; I compiled origin/main:internal/redaction/redaction.go standalone and ran the identical inputs — main returns value: [REDACTED] for both.
What makes this blocking rather than a judgement call is internal/cli/sandbox_check_test.go:155. TestRunSandboxCheckMatchedGrantRedactsReason already existed on main and pinned that a grant reason containing sk-test-secret1234567890 never reaches zero sandbox check --json. This PR rewrites that fixture to sk-proj-testsecret1234567890ab. On this branch:
RedactString("approved with sk-test-secret1234567890", Options{})
== "approved with sk-test-secret1234567890"
So the fixture was changed because the behaviour regressed, and the test that would have caught it can't anymore. Please restore the original fixture.
I understand the motivation — TestScanIgnoresKebabCaseStartingWithSk shows sk-learn-machine-learning-model was a false positive on main. But you can kill that without giving up the general branch. RE2 has no lookahead, so do it in Go: keep \bsk-[A-Za-z0-9_-]{20,} and drop an openai_key match whose body contains no digit. sk-learn-machine-learning-model has none; every real key does. Tighten further if you want, but the enumerated-prefix approach means every new vendor prefix is a silent miss until someone notices.
Same file, internal/redaction/redaction.go:87: the JWT pattern now requires the second segment to start with eyJ. That's fine for a standard JWS but not for a compact JWS with a non-JSON payload or for a JWE, where segment two is an encrypted key. Probed:
eyJhbGciOiJIUzI1NiJ9.U0VOVElORUxwYXlsb2Fk.SENTINELsignature123
main: [REDACTED] this branch: leaked in full
5-part JWE
main: [REDACTED].SENTINELciphertext.SENTINELtag12345 this branch: leaked in full
Please keep the looser alternative alongside the strict one.
Clean destroys pre-upgrade worktrees' gitignored contents
internal/worktrees/worktrees.go:858 runs worktreeIsDirty(..., expiredLease) before the ownership/legacy determination at :871-880. For a legacy entry — unlocked, no marker, created by the version of Prepare that's on main today — expiredLease is false, so the probe omits --ignored and the worktree is treated as explicitly released. But release didn't exist in the version that created it; unlocked was the only state there ever was.
Reproduced end to end with real git: created a worktree exactly the way main's Prepare does (git worktree add --detach under <base>/zero-worktree-<key>/legacy-task, no lock, no marker), dropped a .gitignored .env in it, asserted git status --porcelain is clean and --ignored is not, aged everything 48h, ran Clean(24h). The worktree and the .env are both gone. And Prepare auto-invokes Clean at worktrees.go:86-88, which I also confirmed removes a pre-existing legacy worktree — so this fires on the very first zero worktrees prepare or zero exec --worktree after upgrade, with no warning. Clean doesn't exist on main at all, so this is entirely introduced by this PR.
Fix is a reorder: determine ownership/legacy first, then pass includeIgnored = expiredLease || migratedLegacy. A worktree that predates the release protocol should get the same benefit of the doubt as a crashed lease.
Related: internal/worktrees/worktrees_test.go:1935 annotates its third fakeRunner result // status --porcelain --ignored: clean, but the command actually issued on that path is git status --porcelain — I added a runner.commandLine(2) assertion to the same fixture to confirm. The test asserts nothing about it, so nothing pins this either way.
Smaller things I'd want fixed in the same pass
internal/worktrees/worktrees_windows.go:39 — openProcessErrorMeansAlive returns alive only for ERROR_ACCESS_DENIED; everything else reads as dead. Probed:
ERROR_ACCESS_DENIED -> alive=true
ERROR_INVALID_PARAMETER -> alive=false
ERROR_NOT_ENOUGH_MEMORY -> alive=false
ERROR_NO_SYSTEM_RESOURCES -> alive=false
ERROR_TOO_MANY_OPEN_FILES -> alive=false
processAlive's own doc at worktrees.go:206-212 promises the opposite ("any ambiguity ... counts as alive"), and the POSIX sibling honours it. Under handle or memory pressure a live run's lease reads as expired and Clean can unlock and worktree remove --force the workspace out from under it. return !errors.Is(err, windows.ERROR_INVALID_PARAMETER) restores the contract.
internal/cli/workflows.go:171 — runWorktreesRelease carefully redacts the release error and the success line, but forwards resolveWorkspaceRoot's error (and filepath.Abs's at :155) verbatim:
$ zero worktrees release -C /no/such/sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu <path>
[zero] cwd must be an existing directory: ...\no\such\sk-proj-abcDEF123_ghiJKL456-mnoPQR789stu
Wrap both in redactCLIString.
internal/worktrees/worktrees.go:153 — the reuse path never reclaims a dead-owner lease even though Clean knows how at :860-865. Probed: Prepare with a dead LeasePID, then Prepare again with the same name and a live PID, and the second call fails with "locked by another active run". So a SIGKILLed zero exec --worktree --worktree-name X bricks that name until someone runs release by hand. The error message is actionable so this isn't severe, but the asymmetry is worth closing: if leasePID(reason) names a dead process, unlock and retry.
Notes, no action needed
Result.LockAcquired's comment says it's false when a reused worktree is already locked by another live caller, but that case returns an error at:157/:217, so it's never observably false. Theif preparedWorktree.LockAcquiredguard atinternal/cli/exec.go:222is dead in practice — either make the reuse path really return false, or simplify both.refs/zero/orphaned-worktree/<sha>accumulates with nothing pruning it, which also pins the objects forever.Prepareskips the auto-Cleanwhenoptions.RunGit != nil, which is exactly what every unit test sets, so that wiring has no coverage — I needed a real-git probe to see it.TestBuildServeScopeKeepsLexicalPathsfails on my box for lack of symlink privilege; pre-existing and unrelated. Everything else ininternal/secrets,internal/redaction,internal/worktrees,internal/toolsandinternal/clipasses, andgo build ./.../go vetare clean.
Unrelated to the code: I approved #857 earlier today and meant what I said there. Two of the things below are the classes that section names — a test fixture adjusted to fit a narrowed pattern, and a claim in the description that's wider than what shipped — so this is me applying your own list, not moving goalposts. Everything I found is above in one pass; there's no second round waiting behind it.
…safety Address human review on Gitlawb#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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/redaction/redaction.go`:
- Around line 71-74: Update the redaction logic around openaiKeyPattern and
secretMatchHasDigit so known OpenAI prefixes such as sk-proj-, sk-svcacct-, and
sk-admin- are redacted even when the token contains no digits, while retaining
the digit requirement for unknown sk- forms. Add a regression case covering an
alphabet-only sk-proj- token.
🪄 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: a571c6c5-c539-40ba-86b2-e95d54bb3bd2
📒 Files selected for processing (26)
internal/cli/backends_test.gointernal/cli/extensions_test.gointernal/cli/hooks_manage_test.gointernal/cli/mcp_commands_test.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/redaction/audit_fixes_test.gointernal/redaction/redaction.gointernal/redaction/redaction_test.gointernal/secrets/boundary_test.gointernal/secrets/scanner.gointernal/secrets/scanner_test.gointernal/selfverify/contracts_test.gointernal/selfverify/selfverify_test.gointernal/sessions/replay_test.gointernal/verify/contracts_test.gointernal/verify/verify_test.gointernal/worktrees/worktrees.gointernal/worktrees/worktrees_test.gointernal/worktrees/worktrees_windows.gointernal/worktrees/worktrees_windows_test.gointernal/zerocommands/backend_doctor_test.gointernal/zerocommands/backend_snapshots_test.gointernal/zerocommands/contracts_test.gointernal/zerogit/contracts_test.gointernal/zerogit/zerogit_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/cli/workflow_test.go
- internal/secrets/scanner.go
- internal/worktrees/worktrees_test.go
- internal/cli/workflows.go
- internal/worktrees/worktrees_windows.go
- internal/worktrees/worktrees.go
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.
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/secrets/scanner_test.go (1)
149-170: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRequire complete secret replacement in all new redaction assertions.
These tests can pass when a matcher removes only one fragment. Require the complete credential or JWT to be absent and the expected replacement marker to be present.
internal/secrets/scanner_test.go#L149-L170: assert full compact JWS/JWE replacement, including payload, signature, ciphertext, and tag.internal/secrets/scanner_test.go#L287-L303: assertRedact("token="+key)equals"token=[REDACTED:openai_key]".internal/redaction/audit_fixes_test.go#L164-L174: assertRedactString("token="+secret, Options{})equals"token="+RedactedSecret.🤖 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_test.go` around lines 149 - 170, Strengthen the redaction assertions across all listed sites: in TestScanDetectsLooseJWTForms at internal/secrets/scanner_test.go:149-170, require the complete compact JWS/JWE token—including payload, signature, ciphertext, and tag—to be absent and the expected replacement marker to be present; at internal/secrets/scanner_test.go:287-303, require Redact("token="+key) to equal "token=[REDACTED:openai_key]"; and at internal/redaction/audit_fixes_test.go:164-174, require RedactString("token="+secret, Options{}) to equal "token="+RedactedSecret.
🤖 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/secrets/scanner_test.go`:
- Around line 149-170: Strengthen the redaction assertions across all listed
sites: in TestScanDetectsLooseJWTForms at
internal/secrets/scanner_test.go:149-170, require the complete compact JWS/JWE
token—including payload, signature, ciphertext, and tag—to be absent and the
expected replacement marker to be present; at
internal/secrets/scanner_test.go:287-303, require Redact("token="+key) to equal
"token=[REDACTED:openai_key]"; and at
internal/redaction/audit_fixes_test.go:164-174, require
RedactString("token="+secret, Options{}) to equal "token="+RedactedSecret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c70f92e-c56d-4c0a-bbf2-3386a9e85c9e
📒 Files selected for processing (4)
internal/redaction/audit_fixes_test.gointernal/redaction/redaction.gointernal/secrets/scanner.gointernal/secrets/scanner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/secrets/scanner.go
- internal/redaction/redaction.go
|
Review items 1-6 are on tip ( Ready for re-review. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks — most of this is genuinely fixed, and the worktrees half I have no complaints about. Two things are still open on the secrets side, and they're the same two.
How I checked the redaction change: I built a differential harness and ran the same sentinel corpus through secrets.Redact and redaction.RedactString on origin/main (8e26679) and on this head (ebc83b7), then diffed the two outputs key by key. That's the only way to tell a widening from a narrowing when both are happening in the same patch.
1. Broadening the pattern fixed most of it, but one shape still leaks.
The good news first — everything the previous head dropped is back: sk-proj-, sk-svcacct-, sk-admin-, sk-or-v1-, sk-fw-, and plain digit-bearing sk-. And the false positives genuinely improved: sk-learn-machine-learning-model-pipeline, sk-this-is-a-normal-kebab-case-identifier and /home/user/projects/sk-utils-library-v2/main.go were all redacted on main and are correctly left alone here. That's a real win.
What the digit filter costs: a digit-free sk- body now passes straight through, both in Scan and in RedactString.
in: prefix sk-AAAAAA…(48 A's) suffix
main: prefix [REDACTED:openai_key] suffix
head: prefix sk-AAAAAA…(48 A's) suffix
Legacy OpenAI keys are sk- + 48 base62 characters, so about one in 4,500 of them contains no digit at all — (52/62)^48. That's rare, but it's a silent full-credential leak, not a truncation, and it's the exact failure mode this PR set out to close.
The filter is aimed at kebab phrases, and kebab phrases have hyphens. Real legacy keys don't. So gate on that instead:
if p.typ == "openai_key" && !knownOpenAIKeyPrefix(m) && !containsDigit(m) &&
strings.Contains(strings.TrimPrefix(m, "sk-"), "-") {
continue
}I applied that and re-ran the corpus: the two digit-free legacy shapes come back, and nothing else moves — every kebab false positive above stays un-redacted. Mirror it in redaction.go's secretMatchHasDigit twin.
2. The fixture is still weakened, and it's what's hiding item 1.
internal/secrets/boundary_test.go still differs from main. I checked out main's copy of the file onto this head and ran it:
boundary_test.go:35: real secret NOT caught in "token: \"sk-abcdefghijklmnopqrstuvwxyz\""
That's the same shape as the leak above. Appending 0 to the fixture is what made the test agree with the new pattern rather than the other way round — which is what I asked us not to do last time. With the hyphen gate applied, main's original fixture passes unmodified, so nothing needs changing there at all.
While I was in it, I probed the other three fixture edits individually. All three pass with main's original values:
findings=1 in="export OPENAI_API_KEY=sk-proj-abcdefghijklmnopqrstuvwx"
findings=1 in="key is sk-svcacct-abcdefghijklmnopqrstuvwx at the end"
findings=1 in=" secret_test.go:12: token sk-proj-abcdefghijklmnopqrstuvwxyz"
Those are known prefixes, so the digit filter never applied to them. The 0s in boundary_test.go, redaction_test.go and audit_fixes_test.go are churn — please revert them so the diff shows only the one fixture that genuinely had to move (and after the fix, none of them do).
One thing that isn't in my original list but should be in the PR body. Aligning RedactString to secrets.Scan also raised its floors relative to main: github_pat 12→22, gh[pousr]_ 12→36 with _ dropped from the body class, AIza 12→35, sk- 12→20 with . dropped, JWT segments 8→10. Each of those is a shape main redacted and this head doesn't — e.g. ghp_abcd_efghijklmnopqrstuvwxyz0123456789 now passes through whole. I'm fine with all of it, since the new floors match real issued key lengths and a single source of truth is the right call. But it arrived unannounced inside a fix for the opposite problem, and that's how a narrowing slips past review. Please say so in the summary.
3, 4, 5, 6, 7, 8 — all closed. I mutation-tested each one I'm accepting:
- Ordering ownership/legacy detection ahead of the dirty probe is right, and
includeIgnored := expiredLease || legacyis load-bearing: dropping|| legacyturns both legacy tests red with the correct message. firstStatusCommand()is a real assertion now, and it fails under the same mutation.- JWT: the loose alternative does what it should. A non-JSON-payload JWS is detected where main detected nothing, and a five-segment JWE now has its header, encrypted CEK and IV redacted. Ciphertext and tag survive, which I'm happy with — without the CEK they're inert.
openProcessErrorMeansAliveis correctly fail-closed. Reverting it toerrors.Is(err, ERROR_ACCESS_DENIED)failsTestOpenProcessErrorMeansAliveOnAmbiguousErrors. Good call includingTestOsProcessAliveReportsDeadAfterExitagainst a real exited process rather than only errno constants — that distinction has bitten us on Windows before.- Release error redaction: I ran
-C <bad>,--cwd=<bad>, thereleaseWorktreefailure and the success echo through the real CLI entry point. All four redact. - Dead-owner reclaim: stubbing the reclaim branch to
if falseturnsTestPrepareReclaimsDeadOwnerLeaseOnReusered.
One gap on 8. Nothing tests the safety direction — that a lease held by a live pid is still rejected and never unlocked. TestPrepareRejectsWorktreeLockedByAnotherRun doesn't get there: its fakeRunner runs out of queued results, so reclaimDeadOwnerLease sees an empty worktree list and returns early. It never reaches the processAlive(pid) branch. I wrote that test locally and it passes, so this is about pinning behaviour, not a bug — but over-reclaiming would hand one checkout to two live runs, which is worse than the bug we started with. Please add it.
Two nits, take or leave: the strict jwt pattern is a strict subset of the loose one, so the first entry is dead code; and unknown worktrees release flag %q echoes the raw argument unredacted, though that matches what every other command in main already does.
Fix 1 and 2 and I'll approve. Everything else here is solid work.
Local run: go test ./internal/worktrees/... ./internal/secrets/... ./internal/redaction/... ./internal/cli/... ./internal/tools/... green apart from TestBuildServeScopeKeepsLexicalPaths, which needs symlink privileges and fails on my box regardless of branch. gofmt clean.
Everything above is one pass — six of the eight are genuinely closed and I've said which. The two that remain are really one thing: the digit filter and the fixture that hides it. The hyphen gate closes both, and then main's original fixture passes untouched, so this should be a small change rather than another round of pattern tuning.
|
@euxaristia one thing from the #855 review I want to pull out on its own, because it's easy to lose in a list of eight and it's the bit that actually matters. The fixture in That's a genuinely easy trap. When a test fails after a pattern change, the fixture is usually the fastest thing to move, and it's often right to move it. Here it happened to be the alarm rather than the noise. Worth saying: the other three fixture edits I asked about turned out to be unnecessary None of this changes how the week has gone from my side. You've turned around seven PRs in a day and taken the harder fix over the easy one more than once the |
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/tui/command_output_test.go (1)
101-114: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the original regression inputs and verify complete secret removal.
All changed assertions reject only the complete new value. A prefix-only replacement can leave a credential tail while these tests pass.
internal/tui/command_output_test.go#L101-L114: retain the shorter Google-token case and assert that no token suffix remains.internal/tui/command_output_test.go#L190-L198: apply the same complete-removal check to formatted command output.internal/tui/command_polish_test.go#L256-L256: retain the previous shorter sandbox-grant fixture.internal/tui/command_polish_test.go#L315-L315: reject both the complete credential and any leaked suffix.internal/tui/model_test.go#L375-L375: retain the previous shorter sandbox-grant fixture.internal/tui/model_test.go#L409-L409: reject both the complete credential and any leaked suffix.🤖 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/tui/command_output_test.go` around lines 101 - 114, Preserve the original shorter regression fixtures and strengthen complete-removal assertions: in internal/tui/command_output_test.go lines 101-114, retain the shorter Google-token case and reject its leaked suffix; in lines 190-198, apply the same suffix check to formatted command output; retain the previous shorter sandbox-grant fixtures in internal/tui/command_polish_test.go line 256 and internal/tui/model_test.go line 375; update internal/tui/command_polish_test.go line 315 and internal/tui/model_test.go line 409 to reject both each complete credential and any leaked suffix.Source: Coding guidelines
🤖 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/tui/command_output_test.go`:
- Around line 101-114: Preserve the original shorter regression fixtures and
strengthen complete-removal assertions: in internal/tui/command_output_test.go
lines 101-114, retain the shorter Google-token case and reject its leaked
suffix; in lines 190-198, apply the same suffix check to formatted command
output; retain the previous shorter sandbox-grant fixtures in
internal/tui/command_polish_test.go line 256 and internal/tui/model_test.go line
375; update internal/tui/command_polish_test.go line 315 and
internal/tui/model_test.go line 409 to reject both each complete credential and
any leaked suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6790eed6-418f-4af2-8f35-fc89b6f03afc
📒 Files selected for processing (7)
internal/redaction/audit_fixes_test.gointernal/redaction/redaction.gointernal/secrets/scanner.gointernal/tui/command_output_test.gointernal/tui/command_polish_test.gointernal/tui/model_test.gointernal/worktrees/worktrees_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/secrets/scanner.go
- internal/worktrees/worktrees_test.go
- internal/redaction/redaction.go
- internal/redaction/audit_fixes_test.go
2 participants
Summary
anthropic_keypattern; extra secret values are sorted by length descending so a shorter match can't shadow a longer one, and worktree paths are canonicalized before comparisonPrepareownership with a git-admin marker, and scopesunlock/release/cleanto locks actually owned by the calleros.Chtimeserrors and legacy worktree cleanup on reuse-Ctoworktrees release, completesworktreesin shell completions, and adds regression tests for the aggregation, lease-detection, and nested-activity paths this touchesTest plan
go test ./internal/worktrees/... ./internal/secrets/... ./internal/cli/...Summary by CodeRabbit
worktrees releaseto safely unlock and release worktrees, with path and working-directory support.