fix(worktree): sweep every repo root known to hold managed trees (#761) - #771
fix(worktree): sweep every repo root known to hold managed trees (#761)#771griffinwork40 wants to merge 10 commits into
Conversation
The sweep engine is per-root: it anchors on `<repoRoot>/.afk-worktrees` and skips anything outside it. The daemon derived that single root from its own cwd (or AFK_WORKTREE_SWEEP_ROOT), so managed worktrees created under any OTHER repo were reachable by no sweep at all and leaked permanently. - New `worktree-root-registry.ts` records each repo root that has had a managed worktree created in it, at `$AFK_STATE_DIR/worktree-roots.json`. - Registration happens in `createManagedWorktree`, the single chokepoint both the `worktree` tool and the `isolation: "worktree"` path funnel through. - The daemon's prune tick now sweeps the union of its own resolved root and every registered root, sequentially (runSweep takes one machine-global advisory lock, so parallel roots would only contend), aggregating removals, warnings and candidates into the existing telemetry summary. Bookkeeping is best-effort by contract and never fails a create: a missed entry degrades to the previous single-root behaviour. Roots are pruned on read, so an entry whose directory was deleted or moved self-heals out of the file, and a corrupt file reads as empty rather than throwing. Retention is capped at 64 roots, oldest-seen dropped first. An explicitly supplied primary root is always visited first and is included even when unregistered, so `AFK_WORKTREE_SWEEP_ROOT` never depends on bookkeeping.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d289bf64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (repoRoot === '') return; | ||
| const absolute = resolve(repoRoot); | ||
| try { | ||
| const entries = await readEntries(); |
There was a problem hiding this comment.
When managed worktrees are created concurrently in different repositories, each call can read the same old registry snapshot here and then overwrite the other call's addition; a parallel 20-root registration retained only the last root. The daemon will consequently never sweep the lost roots, recreating the leak this registry is intended to fix, so the read-modify-write transaction must be serialized across processes.
Useful? React with 👍 / 👎.
| // lock, so parallel roots would just contend and the losers short-circuit. | ||
| const results = []; | ||
| for (const repoRoot of roots) { | ||
| results.push(await runSweep({ |
There was a problem hiding this comment.
When a registered path still exists as a directory but is no longer a valid Git repository, registry pruning retains it and runSweep() rejects on its initial git worktree list command. Because this await is covered only by the outer task-level catch, that root aborts the loop and every valid root after it is skipped on every daemon tick; handle failures per root and aggregate the warning so later roots are still swept.
Useful? React with 👍 / 👎.
| const payload: RegistryFile = { version: REGISTRY_VERSION, roots: next }; | ||
| const target = getWorktreeRootsRegistryPath(); | ||
| await fs.mkdir(dirname(target), { recursive: true }); | ||
| await fs.writeFile(target, JSON.stringify(payload, null, 2), 'utf-8'); |
There was a problem hiding this comment.
If this direct write is interrupted or fails after truncating the file, such as during process termination or a partial disk-full write, the persistent registry is left malformed. parseRegistry() then treats it as empty, so all previously known repositories stop being swept until each happens to create another managed worktree; write a temporary sibling and atomically rename it over the last good registry instead.
Useful? React with 👍 / 👎.
…of hanging `fs.mkdir` under a /proc path does not error on Linux — it hangs, so the best-effort assertion timed out at 15s on ubuntu instead of exercising the contract. Point AFK_STATE_DIR beneath a regular FILE instead: mkdir then fails immediately with ENOTDIR on every platform. Production behaviour is unchanged; only the test's chosen path was unsound.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Merge decision: DO NOT MERGE — 2 blocking findings.
Reviewed ref 45b751ac9a2baca06c1cfcd9c6a57c517d22f4c6. Dimensions: security, api-compat, correctness, spec-compliance, test-coverage, perf-observability. Every file:line below was re-read at that ref; both high findings were independently re-derived by adversarial verifiers.
The shape of this change is right, and the safety framing in the description is unusually careful. The two blockers are both about what happens at the seams the multi-root fan-out newly creates.
Blocking
1. high · correctness · src/agent/daemon/scheduler.ts:495-508
One bad root starves the sweep for every root ordered after it. The new for loop has no per-iteration error handling, and runSweep does not catch a failure of its own opening git -C <repoRoot> worktree list --porcelain — that call sits in a try whose only handler is a finally that releases the lock (worktree-sweep.ts:473-475, :778-780; the catch at :465 belongs to the already-closed lock-acquisition try). builtinPruneExecFile is a bare promisify(execFileCallback) (scheduler.ts:52), so a nonzero git exit rejects, escapes runSweep, escapes the loop, and lands in the outer catch at :536, which records status: 'error' and returns.
for (const repoRoot of roots) {
results.push(await runSweep({
execFile: builtinPruneExecFile,
repoRoot,Reachability is concrete: the registry's entire liveness gate is stat().isDirectory() (worktree-root-registry.ts:110-112) — nothing checks the path is still a git repo. A directory whose .git was deleted, or whose path was reused by a non-repo, persists across ticks (self-heal at :120-130 prunes only vanished paths), so the failure is sticky, not transient. That reintroduces the leak #761 exists to close.
Blast radius is new: before this PR there was exactly one root, so a throw could only affect that one. Note also that the primary root is git-validated each tick via resolveWorktreePruneRoot and is swept first — so the exposure is specifically the newly-registered roots. Second-order: on a throw, the earlier roots' removed[] are discarded from telemetry even though those removals already hit disk.
Suggestion: wrap the per-root runSweep in try/catch inside the loop, push [ERROR] sweep failed for <root>: … into the aggregated warnings, and continue.
2. high · spec-compliance · src/cli/commands/interactive/worktree.ts:579,616-619,755
Unmet intent. The description's central claim is that createManagedWorktree() is "the single chokepoint that BOTH the worktree tool's create action AND createIsolatedWorktree() funnel through." That claim is true of the two paths it names (worktree.ts:215; subagent-executor.ts:555 → worktree-managed.ts:246). But a third production path creates a first-class managed tree and never registers the root:
579: const worktreePath = join(repoRoot, '.afk-worktrees', slug);
616: const addArgs = ['-C', repoRoot, 'worktree', 'add', '-b', branch, worktreePath];
755: await fs.writeFile(join(worktreePath, '.afk-worktree-meta.json'), …It writes the meta with owner: 'interactive' — the exact protocol worktree-sweep.ts:562 consumes. Confirmed negative: git grep 'worktree-root-registry\|registerWorktreeRoot' 45b751ac -- src/cli/commands/interactive/ returns zero matches. Production-reachable, no experimental gate: -w, --worktree at interactive.ts:210, and interactive is Commander's isDefault (:187).
Fairness caveat that narrows this: interactive/boot-prune.ts:64-70 re-resolves the repo root on every REPL launch and sweeps it registry-independently, so a repo the user keeps returning to via afk -w still gets reclaimed locally. The unclosed hole is cross-repo daemon reclamation for repos only ever touched via afk -w. Worth noting the shipped comment at worktree-managed.ts:99-104 ("the single chokepoint every managed create funnels through") is strictly false at this ref.
Suggestion: call registerWorktreeRoot(repoRoot) alongside the meta write at interactive/worktree.ts:755, or route that path through createManagedWorktree.
Non-blocking
- medium · correctness ·
worktree-root-registry.ts:76-90— read-modify-write with a non-atomicfs.writeFileand no lock. Two concurrent registrations of different roots race and one is lost; a process death mid-write truncates the JSON, whichparseRegistrythen reads as[], silently discarding every registered root and restoring the #761 leak. Fix: write<target>.tmp.<pid>thenfs.rename. - medium · security ·
worktree-sweep.ts:355-375— the soft-launch dry-run valve counts prior runs from one machine-global telemetry file keyed ontaskId === 'worktree-prune', not per root. A root registered today inherits an already-exhausted counter and gets a live destructive sweep on first contact, never the 3-run dry-run preview the valve exists to provide. With one root that guarantee held. Fix: record the root on the telemetry line and count per-root. - medium · security ·
worktree-root-registry.ts:87-88,124— the registry is written with nomode, so the default umask leaves the absolute path of every repo the user works in world-readable. The codebase already treats this data class as0o600inawareness/presence.ts:240-246, with a comment naming this exact failure. Fix:{ encoding: 'utf-8', mode: 0o600 }on both writes,{ recursive: true, mode: 0o700 }on the mkdir. - medium · test-coverage ·
src/agent/daemon/scheduler.test.ts— the multi-root loop, theroots.length === 0guard, and theflatMapmerge ship untested.git grep -c sweepRootSet 45b751ac -- src/agent/daemon/scheduler.test.ts→ 0; the file's only sweep-adjacent coverage isresolveWorktreePruneRoot(:947). The "scheduler.test.ts 30 passed" row attests to pre-existing tests that never touch the new path. Fix: stubsweepRootSetto return two roots, assert onerunSweepper root and thatremoved/warningsconcatenate. - low · test-coverage ·
worktree-root-registry.test.ts— no test drivesMAX_ROOTS = 64or the oldest-first eviction that.slice(-MAX_ROOTS)depends on (confirmed: 0 matches forMAX_ROOTS/64in the new test file). Fix: register 65 roots, assert length 64 with the first absent. - low · api-compat ·
scheduler.ts:482-484— the skip excerpt now fires for two distinct causes but names one; an operator with an empty registry reads a diagnosis about daemon cwd that does not apply. Fix: branch the excerpt onprimaryRoot === nullvs. empty registry. - low · perf-observability ·
scheduler.ts:524-526— the summary is the only field persisted, and now reports bare cross-root totals, soremoved 7, warned 3cannot be attributed to a root. With one root attribution was implicit; with up to 64 it is lost. Fix: appendrootsSwept: Nor emit one record per root. - low · security ·
worktree-sweep.ts:557—.afk-worktreescontainment is a rawstartsWithprefix, not a path-boundary test, so a sibling<root>/.afk-worktrees-scratchpasses the guard. Pre-existing, but this PR multiplies the roots where it holds.isPathWithinat:222already does this correctly. - nit ·
scheduler.ts:497,510—const results = []drops theSweepResult[]annotation that would catch a future field addition to the hand-rolled aggregate;dryRun: results.some(...)would label a tick dry-run when only one root previewed. Unreachable today since the valve resolves uniformly per tick.
Verified, not defects
- Aggregate shape is correct.
SweepResultis exactly{removed, warnings, dryRun, candidates}(worktree-sweep.ts:150-155), and the only downstream reads —result.dryRun,result.candidates.filter,result.removed.length,result.warnings.length(scheduler.ts:524-526) — are all satisfied. No field is silently dropped.const results = []is TypeScript's evolving-array form, sonoImplicitAnyis not defeated. - Null-skip semantics preserved.
sweepRootSet(null)with an empty registry returns[], taking the same skip branch; the skip body, itsresponseExcerpt, and itsreturn skippedare untouched (scheduler.ts:474-488). - No argv injection. Registry paths reach git only through
execFile(no shell) and always in the value position after-C, so a leading--path is consumed as an operand. - PII premise partially refuted. The prune task's telemetry writes counts, not paths (
scheduler.ts:526) — worktree paths do not newly flow into telemetry. The registry file itself is the only new on-disk path exposure. - "Cannot fail the create" holds.
resolve(repoRoot)outside the try cannot throw for astring, and both call sites pass a typedrepoRoot. - The 14-test claim reconciles exactly — 5 + 4 + 5 across the three describe blocks, verified at the ref.
What was not checked
Citations verified inline against branch HEAD 45b751ac9a2baca06c1cfcd9c6a57c517d22f4c6; 0 fabricated, 0 dropped, 3 absence claims independently confirmed at the ref. Stated intent: PR title + body — spec-compliance assessed against it. Tests were not executed and CI was not re-run (CI is green on this head). That git -C <non-repo> exits nonzero is standard git behavior, asserted rather than read from source. Not examined: the Telegram surface, worktree-sweep.ts internals beyond the paths cited, and whether a stale registered root nested inside another repo would succeed and sweep that outer repo instead of failing.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Merge decision: DO NOT MERGE
One blocking finding: the load-bearing "single chokepoint" claim in the PR body is falsified by a third managed-create path, which leaves #761 partially unfixed for the afk -w launcher. Everything else is sound — the module is careful, the best-effort contract holds, and the 145-line suite is genuinely substantial. Citations below were verified inline against branch HEAD 45b751ac.
🔴 high · spec-compliance / correctness — a third create path never registers its root
src/cli/commands/interactive/worktree.ts:579,616,619 · ref:45b751ac
The PR body states registration happens at "the single chokepoint that both the worktree tool's create action and createIsolatedWorktree() funnel through." There is a third production path: the afk -w session launcher builds an identically-shaped managed tree with a direct git worktree add, never calling createManagedWorktree and therefore never registerWorktreeRoot.
const worktreePath = join(repoRoot, '.afk-worktrees', slug); // :579
const addArgs = ['-C', repoRoot, 'worktree', 'add', '-b', branch, worktreePath]; // :616
await execFile('git', addArgs); // :619A grep of that file for createManagedWorktree|registerWorktreeRoot returns zero matches (verified at the ref). The tree lands under .afk-worktrees/, so the sweep engine would reclaim it — the root just never enters the registry.
Mitigations weighed, and why they do not close it: that path has its own session-shutdown cleanup(), and boot-prune.ts sweeps the cwd repo — but cleanup() does not run for a crashed or kill -9'd session, and boot-prune only fires when the user later reopens a REPL in that same repo. A -w session killed in a repo the daemon never sits in and the user does not revisit is exactly the #761 leak the PR sets out to eliminate.
Suggestion: call registerWorktreeRoot(repoRoot) right after the git worktree add succeeds in createWorktreeAt, or route that path through createManagedWorktree. If the gap is deliberate, say so explicitly in the PR body — the "single chokepoint" wording is what makes this blocking rather than a nit.
🟠 high · correctness — unlocked read-modify-write drops a root under parallel creates
src/agent/worktree-root-registry.ts:80-90 · ref:45b751ac
const entries = await readEntries();
const others = entries.filter((e) => resolve(e.path) !== absolute);
const next = [...others, { path: absolute, lastSeenAt: now }].slice(-MAX_ROOTS);
await fs.writeFile(target, JSON.stringify(payload, null, 2), 'utf-8');No lock, and fs.writeFile is not temp-file + rename. Two concurrent creates each read the same snapshot and the later write silently discards the earlier one's new root — and a dropped root means its trees leak, reintroducing the bug being fixed.
This is reachable, not theoretical: dispatcher.ts:989-1010 runs agent forks concurrently under settleWithConcurrencyLimit, and each isolation:"worktree" child calls createIsolatedWorktree → createManagedWorktree → registerWorktreeRoot independently (subagent-executor.ts:555). The registry module contains no lock/mutex reference at all. The same race also exists between a create and the self-heal rewrite in readRegisteredWorktreeRoots (:129).
Suggestion: serialize the read-modify-write behind the existing advisory lock (getWorktreeSweepLockPath) or a dedicated lock file, and write via temp-file + rename so a torn write is impossible.
🟡 medium · correctness — no per-root error isolation in the sweep loop
src/agent/daemon/scheduler.ts:497-508 · ref:45b751ac
const results = [];
for (const repoRoot of roots) {
results.push(await runSweep({ execFile: builtinPruneExecFile, repoRoot, ... }));
}The loop has no try/catch; the only handler is the function-wide one (:465 → :536). One runSweep rejection — a corrupt repo, a git lock, a permissions error in any registered root — aborts the whole tick and starves every remaining root, recording a single error. The single-root design could not starve anything, so this is a new availability failure mode that scales with root count.
Suggestion: wrap each runSweep in try/catch, collect per-root failures into warnings, and continue.
🟡 medium · correctness — re-registration reorders, so the 64-cap can evict a live root
src/agent/worktree-root-registry.ts:82-84 · ref:45b751ac
Re-registering moves an entry to the end of the array, and .slice(-MAX_ROOTS) drops from the front. The doc comment says "oldest-seen dropped first," which holds for last-seen order — but a cold-yet-live root with existing worktrees can be pushed past the cap by 64 busier repos and then silently vanish from every future sweep. Requires >64 distinct roots on one machine: reachable, not common-case.
Suggestion: emit a telemetry event when the cap evicts an entry, so a silently-dropped root is diagnosable rather than invisible.
🟡 medium · perf-observability — tick time scales linearly with roots, unmeasured
src/agent/daemon/scheduler.ts:495-508 · ref:45b751ac
N sequential runSweep calls, each taking the machine-global advisory lock and shelling out to git, grow tick wall-clock linearly — up to 64× at the cap — with no telemetry field recording root count or per-root duration. The serial choice is right (the comment's lock reasoning is correct); the missing signal is the issue: nothing warns before a tick overruns its cron interval.
Suggestion: record rootCount and per-root durations in the telemetry record.
🟡 medium · test-coverage — the new multi-root wiring is untested
src/agent/daemon/scheduler.ts:497-514 · ref:45b751ac
Verified at the ref: scheduler.test.ts has no worktree-prune test at all (zero hits for runBuiltinWorktreePrune|worktree-prune|sweepRootSet), and the PR does not touch that file. So the sequential loop, the error propagation above, and the flatMap aggregation all ship uncovered. The new registry suite also has no test for MAX_ROOTS eviction or concurrent registration (zero hits for MAX_ROOTS|64|evict|concurrent|Promise.all).
Suggestion: add a scheduler test with two stubbed roots where the first runSweep rejects, asserting the second is still swept; add a 65-root eviction test and a two-concurrent-registerWorktreeRoot test.
🔵 low · perf-observability — flatMap erases per-root provenance
src/agent/daemon/scheduler.ts:511-512, consumed at :524-526 · ref:45b751ac
removed/warnings are flattened into untagged path arrays and reduced to bare counts in the summary, so a spike in warnings cannot be attributed to a repo without eyeballing path prefixes. Suggestion: keep the per-root results array in the telemetry record, or tag each entry with its source root.
🔵 low · security — every failure path is silent
src/agent/worktree-root-registry.ts:57,65,89,113,129 · ref:45b751ac
Five catch blocks swallow errors with only a comment. The best-effort contract is right, but a persistent write failure (read-only state dir, disk full, bad permissions) degrades to the exact #761 leak with zero operator-visible signal. Suggestion: a rate-limited console.warn on the write-failure paths.
🔵 low · api-compat — version is written but never read
src/agent/worktree-root-registry.ts:28,49-50 · ref:45b751ac
REGISTRY_VERSION = 1 is persisted, but parseRegistry never branches on parsed.version (verified: zero reads at the ref) — a future v2 falls through the generic shape filter with no migration or reset path. Forward-compat gap, not a break today. Suggestion: branch on parsed.version and route unrecognized versions explicitly.
✅ Verified sound
- Claim 5 (unchanged empty-set behaviour) holds.
sweepRootSet(:145) unconditionally includes a non-null, non-empty primary, soroots.length === 0occurs only when the primary is null/empty — the same condition as the oldrepoRoot === null, with the same skip path and telemetry message. - No dropped
SweepResultfield.SweepResult(worktree-sweep.ts:150-155) has exactlyremoved,warnings,dryRun,candidates; the synthesized literal reproduces all four, andresultis consumed only at:524-526. - Purely additive API. All four new exports are new symbols; no existing signature changed.
- No scope creep. Every hunk is load-bearing for the stated intent.
- Blast radius is bounded. The sweep anchors on
join(repoRoot,'.afk-worktrees')(:479) and skips anything outside it (:556), so a bogus registry entry cannot steer removals at arbitrary directories. Symlink following infs.stat(:111-112) stays inside that bound and within the same-user trust boundary that already governs$AFK_STATE_DIR— not a finding. - Farms are not a regression.
src/agent/worktree.ts:319creates under$AFK_HOME/farms/, outside.afk-worktrees/— never swept before or after this PR.
What was not checked
- Citations verified inline against branch HEAD
45b751ac; all five absence claims re-verified at that ref and confirmed absent. Both Wave 1 agents reported diff-file offsets rather than source line numbers — every line number above was corrected against the ref, and each evidence snippet was confirmed verbatim. - Stated intent: PR #771 title + body — spec-compliance was assessed against it.
- Tests were not executed; the author's reported gate results are taken at face value. CI is green on this head.
- The two
highfindings were shadow-verified by independent re-derivation. Not reviewed: the Telegram surface,worktree-sweep.tsinternals beyond the anchor/skip guards, and Windows path behaviour.
# Conflicts: # src/agent/tools/handlers/worktree-managed.ts
…iew) Registry writes were an unlocked read-modify-write with a non-atomic fs.writeFile. Reviewers flagged three consequences, all of which reintroduce the #761 leak the registry exists to close: - Concurrent registrations each read the same snapshot and the last write won. Reachable in-process: the tool dispatcher runs agent forks concurrently and every isolation:"worktree" child registers independently. A 20-root parallel registration retained ONE root. - An interrupted write left truncated JSON, which parseRegistry reads as [], silently discarding every known root. - The file listing every repo the user works in was written with the default umask, i.e. world-readable. Writes now go through an in-process promise chain, then a degradable advisory lock, then temp-file + rename, at 0o600. The lock yields after 2s and writes anyway: registration runs inside worktree creation, so blocking a create is worse than a rare lost update. Self-heal now expresses its prune as 'drop these dead paths' rather than overwriting with a precomputed list, so it cannot clobber a concurrent registration. Cap eviction now emits a debug line naming the dropped roots — it was previously invisible, and an evicted root's trees stop being swept. Tests: 14 -> 20. The parallel-registration test reproduces the reported symptom (1 of 20 retained) against the unserialized code.
…ep (#771 review) The new fan-out loop had no per-iteration error handling. runSweep does not catch a failure of its own opening `git -C <root> worktree list`, and builtinPruneExecFile is a bare promisify(execFile), so a nonzero git exit rejects, escapes the loop, and lands in the tick-level catch. That starves every root ordered behind the bad one, on every tick. It is sticky rather than transient: the registry's liveness gate is a bare stat().isDirectory(), so a directory whose .git was deleted persists across ticks. With one root a throw could only affect that root; the fan-out turned it into a fleet-wide availability failure and reintroduced the #761 leak. Failures are now caught per root, demoted to an [ERROR] warning, and the loop continues — so removals that already hit disk are still reported. Also from review: - the empty-root-set skip blamed daemon cwd alone; an empty set means both causes hold at once, so the message now names both. - the summary reports swept/attempted roots, so cross-root totals can be attributed and per-root failures are visible. - results[] carries its SweepResult[] annotation again; the some() vs every() ambiguity on dryRun is documented at the call site. Tests: new scheduler-worktree-prune.test.ts (6). Kept out of scheduler.test.ts because vi.mock is file-scoped and would re-point worktree-sweep for its 30 unrelated tests. Three of the six fail against the unguarded loop (second root never swept).
…771 review) The PR body claimed registration sat at 'the single chokepoint that both the worktree tool's create action and createIsolatedWorktree() funnel through'. A third production path falsified that: the `afk -w` session launcher builds a managed tree with its own `git worktree add` and writes the same .afk-worktree-meta.json protocol the sweep engine consumes, but never registered its root — so a daemon that does not sit in that repo never sweeps it. The mitigations do not close it. The launcher's own cleanup() never runs for a kill -9'd session, and boot-prune.ts only fires if the user later reopens a REPL in that same repo. A -w session killed in a repo the user does not revisit is precisely the #761 leak. Registration now happens right after a successful add, so a failed create leaves no root behind. The comment in worktree-managed.ts that asserted a single chokepoint is corrected: there are two registration sites, and any future managed-create path must add a third. Tests: new worktree-root-registration.test.ts (2) — the positive case fails without the fix. worktree.test.ts (58), worktree-autoname (37), worktree-managed (16), worktree tool (34) all still pass.
…nment (#771 review) Two findings in the sweep engine that the multi-root fan-out amplified. Soft-launch valve (medium, security). The valve forces dry-run for the first 3 runs so an operator sees a preview before anything is deleted. Its counter was machine-global — every telemetry line with taskId === 'worktree-prune', regardless of which repo that tick swept. With one root that was equivalent to a per-root count. Fanning out over every registered root broke the equivalence: a repo registered today inherited an exhausted counter from months of ticks against a DIFFERENT repo and got a live destructive sweep on first contact, never seeing a single preview. The count now lives with the root it describes, in <repoRoot>/.afk-worktrees/.sweep-runs (the orphan scan only considers directory entries, so a marker file there is inert). A root with no usable marker — no .afk-worktrees/, read-only checkout — reports null rather than 0 and falls back to the legacy global count; reporting 0 would pin such a root in dry-run forever and leak its worktrees, which is the #761 bug wearing a different hat. New module rather than more lines in an 850-line file. Containment (low, security). The .afk-worktrees membership test was a raw startsWith, so a sibling named .afk-worktrees-scratch passed the guard and could reach the removal verdicts. Now uses the isPathWithin boundary test the file already defines. Tests: worktree-sweep.test.ts 47 -> 49. Two of the new valve tests fail against the global counter, including the fresh-root case that was being swept live. The suite-wide seed now primes the per-root marker; the telemetry-filtering test was rewritten to exercise the fallback path, which is the only place that count still governs. Gates: pnpm lint clean; full suite 14,007 passed / 731 files; audit:env:check, scan:env:check, audit:sdk:check all pass.
Review remediation — pushed
|
| # | Finding | Status |
|---|---|---|
| 1 | scheduler.ts — one bad root starves every root after it |
fixed — per-root try/catch, failure demoted to an [ERROR] warning, loop continues |
| 2 | interactive/worktree.ts — third managed-create path never registers its root |
fixed — registerWorktreeRoot() after a successful git worktree add; the "single chokepoint" comment in worktree-managed.ts is corrected rather than left false |
High / medium
| # | Finding | Status |
|---|---|---|
| 3 | Registry read-modify-write is unlocked and non-atomic | fixed — in-process promise chain → degradable advisory lock → temp-file + rename |
| 4 | Registry world-readable | fixed — 0o600 on both writes (set on the temp file before rename), 0o700 on mkdir |
| 5 | Soft-launch valve counted machine-globally, so a new root got a live destructive sweep on first contact | fixed — counter moved per-root into .afk-worktrees/.sweep-runs (new worktree-sweep-valve.ts) |
| 6 | Cap eviction is invisible | fixed — eviction emits a debug line naming the dropped roots |
| 7 | Cross-root totals unattributable | fixed — summary reports swept/attempted root(s) |
| 8 | Skip excerpt named one of two causes | fixed — an empty set means both hold at once, so the message now says so |
| 9 | const results = [] lost its annotation; some vs every on dryRun |
fixed — annotated; some kept deliberately with the ambiguity documented at the call site |
| 10 | Multi-root wiring untested | fixed — new scheduler-worktree-prune.test.ts (6) |
| 11 | No MAX_ROOTS / concurrency coverage |
fixed — registry suite 14 → 20 |
| 12 | .afk-worktrees containment was a raw startsWith |
fixed — uses the isPathWithin boundary test the file already defines |
Two design notes worth a reviewer's eye:
- The advisory lock is deliberately degradable. It yields after 2s and writes anyway. Registration runs inside worktree creation, so blocking a create is worse than a rare lost update; the in-process chain still orders every writer in the process that actually races.
readRootSweepCountreturnsnull, never0, when a marker is unusable (no.afk-worktrees/, read-only checkout), falling back to the legacy global count. Reporting0there would pin such a root in dry-run forever and leak its worktrees — the worktree sweep only ever reaches one repo root — managed trees under other roots are never reclaimed #761 bug wearing a different hat.
Every fix carries a test that fails without it. The three that reproduce the reported symptoms directly: parallel registration retained 1 of 20 roots before the fix; the scheduler's second root was never swept; a fresh root was swept live instead of previewed.
Also merged main (conflict was the adjacent-import collision in worktree-managed.ts; both imports kept) — the PR is MERGEABLE again.
Gates: pnpm lint clean · full suite 14,007 passed / 731 files · audit:env:check, scan:env:check, audit:sdk:check all pass.
re-review) Three findings from the re-review pass. The self-heal transform closed over a `kept` Set built outside it. It is applied exactly once today, so the bug is latent rather than live — but a second application would find the set already full and drop every entry instead of de-duplicating. Built inside the transform now. Two contracts shipped untested: - The advisory lock is DEGRADABLE by design: a contended lock must cost a rare lost update, never a blocked or failed worktree create. Nothing pinned that. A lock held by a live foreign pid (process.pid, so the stale-lock reclaim cannot fire) must still end with the root recorded. - `.afk-worktrees` containment. The test registers a worktree inside `<root>/.afk-worktrees-scratch` — a lookalike sibling the engine does not own. Verified it FAILS against the previous `startsWith` guard, which swept the intruder: the prefix bug was reachable, not theoretical. Also corrected two now-false claims in the PR description, the same defect class review flagged as blocking earlier: it still described `createManagedWorktree()` as 'the single chokepoint' (there are two registration sites since the launcher was wired), and still claimed the empty-root-set telemetry message was unchanged (it was deliberately changed to name both causes). Verification table refreshed. Gates: lint clean; full suite 14,009 passed / 731 files; env, scan and sdk audits pass.
…registry Two conflicts, both adjacent-addition collisions in the sweep engine: - src/agent/worktree-sweep.ts: import block. Kept both sides — this branch's worktree-sweep-valve.js (per-root soft-launch counter) and main's worktree-orphan-guard.js (#794 age/content gate). - src/agent/worktree-sweep.test.ts: this branch added a new containment test to describe('orphaned-dir-cleanup') immediately above the test main renamed. Kept the new containment test AND took main's rewritten header ('detects but PRESERVES a freshly-created unregistered directory' + its invariant comment), since the shared body below the conflict is main's preserve-asserting version. The semantic seam (main's classifyOrphanDir orphan loop consuming this branch's effectiveDryRun from the per-root valve) auto-merged and is verified by the suites below.
Code Review — PR #771
Verification posture: 2 Wave-1 dimension agents → inline citation + absence-claim verification at the ref → 3 adversarial shadow verifiers on the top claims. All 3 verifiers returned CONFIRMED via Blocking (2)H1 · high · confidence high · security + correctness ·
|
1. worktree-sweep.ts: an explicit dry-run (options.dryRun) no longer
spends the per-root soft-launch preview budget, while a valve-forced
preview (effectiveDryRun without options.dryRun) still credits the
counter — otherwise three preview-only callers would exhaust the
budget and the daemon's first sweep of that root would be destructive
with zero daemon previews ever having run.
2. worktree-root-registry.ts: readRegisteredWorktreeRoots() now
errno-discriminates the liveness stat. Confirmed-dead (ENOENT/
ENOTDIR, or not-a-directory) still prunes; any other errno (EACCES,
EIO, ESTALE, ...) is "unknown" — excluded from this pass' result but
kept in the on-disk file for retry, so a transient permission error
can never permanently drop a live root. The persistence trigger is
now gated on "at least one confirmed-dead path" instead of a length
mismatch, and the dead set contains only confirmed-dead paths.
Classification logic extracted to the new sibling module
worktree-root-registry-liveness.ts.
3. worktree-sweep-valve.ts: readRootSweepCount() now discriminates the
write failure's errno. ENOENT (.afk-worktrees/ absent) still returns
null (legacy machine-global fallback, unchanged). Any other errno
(directory exists, marker unusable/wrong-owner) now returns 0,
forcing previews, instead of silently inheriting an already-exhausted
machine-global count and running a zero-preview destructive sweep.
The module Contract docstring is rewritten to record the refined
contract and why the trade is right.
4. worktree-root-registry.ts / worktree-root-registry-liveness.ts: the
liveness prune now has debugLog coverage mirroring capRoots' style —
names pruned paths, and for retained-unknown entries names the path
plus its errno.
5. daemon/scheduler.ts: the builtin worktree-prune tick now reports
status: 'error' (with errorMessage naming every root's failure) when
every root rejected (results.length === 0 && roots.length > 0). A
tick where some roots succeeded remains status: 'success'.
6. worktree-root-registry.ts: mutateRegistry() now mkdirs the registry's
parent directory (best-effort) before acquiring the lock, so a fresh
install (or a removed state dir) no longer silently proceeds
unlocked because the lock's fs.open('wx') failed ENOENT instead of
EEXIST.
7. scheduler-worktree-prune.test.ts: 'still errors the tick if every
root fails' now asserts status === 'error' with both root failures
present in errorMessage, matching item 5's behavior change and
making the test name accurate.
Added regression tests: two new soft-launch-valve cases in
worktree-sweep.test.ts (explicit dry-run does not credit; valve-forced
preview does credit), one EACCES-retains-then-reappears case in
worktree-root-registry.test.ts, and a new worktree-sweep-valve.test.ts
covering the errno-discriminated readRootSweepCount plus a runSweep
integration check that an unusable marker forces dry-run even against
an exhausted machine-global telemetry count.
1 participant
Fixes #761.
Problem
The sweep engine is per-root: it anchors on
<repoRoot>/.afk-worktreesand skips anything outside it. The daemon's prune tick resolved exactly one root — from its own cwd, orAFK_WORKTREE_SWEEP_ROOT— and swept only that. Managed worktrees created under any other repo were therefore reachable by no sweep, and leaked permanently.This is not hypothetical: it is the mechanism behind long-lived accumulations of unswept managed trees in repos the daemon never happens to sit in.
Shape chosen: a state registry
Per the issue's proposal, roots are recorded rather than configured:
worktree-root-registry.tsmaintains$AFK_STATE_DIR/worktree-roots.json({ version, roots: [{ path, lastSeenAt }] }).createManagedWorktree()— which theworktreetool'screateaction andcreateIsolatedWorktree()(theagenttool'sisolation: "worktree"path) both funnel through — and theafk -wsession launcher incli/commands/interactive/worktree.ts, which builds its tree with its owngit worktree add. An earlier revision of this description claimed the first was the single chokepoint; review correctly falsified that, and the launcher is now wired too. Any future managed-create path must register as well — the comment at thecreateManagedWorktreecall site says so.sweepRootSet(primary)— its own resolved root followed by every registered root, de-duplicated — and aggregates removals, warnings and candidates into the existing telemetry summary.The alternative shape (letting
AFK_WORKTREE_SWEEP_ROOTtake a list) was not taken: it makes reclamation depend on the operator remembering to enumerate repos, which is the failure mode the bug already demonstrates.Safety properties
These are the parts worth reviewing closely, since this writes to user state from inside a create path:
AFK_WORKTREE_SWEEP_ROOTnever depends on bookkeeping.runSweeptakes a single machine-global advisory lock, so sweeping roots in parallel would only contend and let losers short-circuit. The loop is deliberately serial.status: 'skipped'and no sweep attempted. The telemetry message changed on review: an empty root set means the daemon has no primary root AND nothing is registered, so it now names both causes instead of blaming daemon cwd alone.Verification
pnpm test src/agent/worktree-root-registry.test.tspnpm test src/agent/worktree-sweep.test.tspnpm test src/agent/daemon/scheduler-worktree-prune.test.tspnpm test src/agent/daemon/scheduler.test.tspnpm test src/cli/commands/interactive/worktree-root-registration.test.tspnpm test src/cli/commands/interactive/worktree.test.tspnpm test src/agent/tools/handlers/worktree-managed.test.tspnpm test src/agent/tools/handlers/worktree.test.tspnpm test(full suite)pnpm lint(tsc --noEmit)pnpm audit:env:checkpnpm scan:env:checkpnpm audit:sdk:checkNew coverage: idempotent registration, accumulation across distinct roots, pruning a vanished root and persisting the prune, corrupt-file tolerance, a path that exists but is a file, best-effort behaviour on an unwritable state dir, and the primary-first / de-duplication / no-primary / empty cases of
sweepRootSet.No new env var is introduced, so the generated
docs/env-registry.*are untouched. Test isolation is already guaranteed bysrc/__test-utils__/redirect-paths-env.ts, which repointsAFK_HOMEper test file — verified that a full run of the touched suites leaves no~/.afk/state/worktree-roots.jsonbehind.Review remediation (2026-08-01)
All findings from the two
/reviewpasses and the Codex inline comments are addressed — see the per-item table in the PR comments. Summary of behaviour changes beyond the original design:0o600.<repoRoot>/.afk-worktrees/.sweep-runs) instead of machine-global, so a newly registered repo gets its three previews instead of inheriting an exhausted counter and being swept destructively on first contact..afk-worktreescontainment uses a path-boundary test; the previousstartsWithaccepted a sibling like.afk-worktrees-scratch.main(conflict was an adjacent-import collision inworktree-managed.ts; both imports kept).