Claude-Code-Monitor/docs/superpowers/logs/2026-07-28-worktree-lanes-followups.md

Worktree-lanes: final-review follow-ups

Base: bf312c6 (branch feat/worktree-lanes). One commit for the whole set.

Follow-up 1: unreadable worktrees were genuinely unremovable

(a) Made an unreadable managed lane genuinely removable

server/lib/worktree.js: removeWorktree unconditionally called git worktree remove --force, which git refuses outright — even with a second --force — when the worktree's OWN .git pointer fails its own validation (a corrupt/garbage .git file). All three assertDestroyable checks pass for this shape (the directory exists, resolves inside LANES_ROOT, and is still listed by the source repo), so the lane was stuck: removeWorktree threw, the route 500'd, the lane row survived.

Fix: wrapped the git worktree remove --force call in a try/catch. On failure, findWorktreeAdminDir(sourceRepo, cwd) locates the worktree's administrative directory under the source repo's common dir (<common>/worktrees/<name>) by reading each entry's gitdir file — that file's content is the absolute path to the worktree's own .git file, read from the source repo's side, so it still resolves correctly even though the worktree's own .git file is corrupt. If found, fs.rmSync deletes only that administrative directory (never the worktree directory itself), which deregisters the worktree from git worktree list. Branch delete and lane-row deletion then proceed exactly as for every other remove. If no matching admin directory is found (paranoid case — should not happen for a lane that passed all three checks), the original git error is rethrown rather than silently continuing, so the failure is never swallowed.

This is a genuine deregistration, not a workaround: it never touches the worktree's own files, and after it runs, git worktree remove --force on the same path correctly reports '...' is not a working tree — proof the repo no longer thinks it manages that directory.

Experiment (raw output)

Built a real corrupt worktree under a throwaway repo and ran the exact sequence a reviewer had already reproduced, then the new fallback:

=== remove --force (expect fail) ===
fatal: validation failed, cannot remove working tree: '/tmp/tmp.4kZLeHvThU/lanes/src__corrupt/.git' is not a .git file, error code 5
exit=128

=== manually delete admin dir ===
(no output)

=== worktree list after manual admin removal ===
worktree /tmp/tmp.4kZLeHvThU/src
HEAD 0a1ed2bb01a8ac88b758180d569997b8c0bb22e8
branch refs/heads/main

=== branch still exists? ===
  feat/corrupt

=== worktree directory + corrupted .git file still present? ===
total 16
drwxrwxr-x 2 smartgiftailab smartgiftailab 4096 ... .
drwxrwxr-x 3 smartgiftailab smartgiftailab 4096 ... ..
-rw-rw-r-- 1 smartgiftailab smartgiftailab    8 ... .git
-rw-rw-r-- 1 smartgiftailab smartgiftailab    6 ... README.md
garbage

=== git worktree remove again now (expect: not a working tree, confirms deregistered) ===
fatal: '/tmp/tmp.4kZLeHvThU/lanes/src__corrupt' is not a working tree
exit=128

This confirms: (1) remove --force genuinely refuses a corrupt worktree, matching the earlier report exactly (error code 5 here vs 7 in the original report — git version difference, same refusal); (2) deleting only .git/worktrees/<name> deregisters the worktree from git worktree list without touching the worktree directory or its files; (3) after deregistration git itself confirms the path is no longer a working tree.

The actual code path (removeWorktree against a real corrupt worktree inside LANES_ROOT, through findWorktreeAdminDir) was then exercised by the new server test below and produced the same result end to end.

(b) Reworded the false promise

destructive.notice.unreadable in client/src/i18n/locales/{en,zh,vi,ko}/lanes.json no longer promises forced success. New English text: "The lane directory cannot be read as a Git worktree. Removal is attempted; if Git itself refuses, its worktree record is cleared directly instead. Either way, the directory itself is never touched." zh/vi/ko were translated with the same meaning (not machine-transliterated word-for-word), matching each locale's existing terminology for "lane"/"worktree"/"record" already used elsewhere in the same file.

docs/API.md (blocked/remove paragraph) and docs/LANES.md (the three safety checks section) both had the same "force-removes an unreadable one" claim; both now describe the attempt-then-fallback behavior and name the corrupt-.git-pointer case explicitly.

(c) Added the two missing modal tests

client/src/components/lanes/__tests__/DestructiveLaneModal.test.tsx:

• disables RESET when the worktree directory is unreadable — pins unreadable staying in HARD_BLOCKERS.reset.
• ENABLES remove when the worktree directory is unreadable — pins unreadable staying absent from HARD_BLOCKERS.remove, and that the reworded notice text renders and the modal still echoes back the exact expect block on confirm.

Regression test for (a)

server/__tests__/worktree.test.js: new test builds a real worktree, writes garbage into its .git file (reproducing the exact corruption), asserts git status inside it fails (sanity), calls removeWorktree directly, then asserts: the source repo no longer lists it, its branch is gone, and the directory plus a file written into it still exist afterward untouched.

Follow-up 2: start was atomic by accident, not by invariant

server/routes/lanes.js: the start case read lane.run_id and later wrote a new one with no await in between — atomic today only because nothing yields the event loop in that stretch. Wrapped the whole check-then-spawn in withLaneLock(lane.id, async () => {...}), re-fetching the lane inside the lock (a concurrent remove could have deleted the row while queued, so a missing case now returns 404 ENOLANE instead of dereferencing a null lane — a real edge case introduced by the lock itself, not present before). The 409 ERUNLIVE code and message are unchanged.

Regression test (server/__tests__/lane-lifecycle.test.js, "start is serialized behind the per-lane lock..."): holds the same lane's lock directly from the test (withLaneLock(lane.id, () => new Promise(...))), fires POST .../start while that lock is held, confirms the request has NOT settled 50ms later and that no run_id was written, then releases the held lock and confirms start only proceeds (spawns, returns 200) after release. This proves the route genuinely shares the per-lane lock rather than proving only that the current zero-await code happens to be atomic.

Follow-up 3: openapi.yaml drift check in CI

.github/workflows/ci.yml: added a step to the existing "🧹 Check Formatting" job — npm run openapi:yaml (regenerate) followed by git diff --exit-code openapi.yaml. No new job, no git hook (the pre-commit hook is already slow, per instruction). Verified locally: regenerating produces zero diff against the currently committed file.

Follow-up 4: missing sameOriginGuard on two mutating routes

server/routes/lanes.js: added sameOriginGuard to POST /api/lanes/ (lane creation) and POST /api/lanes/:id/stage (stage reporting) — previously the only two mutating lane routes without it.

Checked both callers before changing anything:

• bin/ccam.js's post() helper (backing ccam lanes add and ccam stage) only ever sends a Content-Type header, never Origin or Referer — the guard passes any request with no Origin header through unconditionally (same rule already relied on by every other guarded lane route). Verified end to end by re-running server/__tests__/lanes-cli.test.js (13/13) after the change — both ccam lanes add --repo and the destructive-lifecycle CLI flows (which call stage indirectly via clear) still pass.
• grep -rn "api/lanes" mcp/ scripts/ — no hits. Nothing else calls these routes.

No legitimate caller was broken; no need to weaken the guard or stop and ask.

Regression tests added to server/__tests__/lanes-api.test.js: "rejects cross-origin lane creation" and "rejects cross-origin stage reporting", mirroring the existing cross-origin PATCH/DELETE tests exactly (assert 403 EBADORIGIN, and for stage, that the lane's stage was not changed).

Commands run, with tallies

• node --test server/__tests__/worktree.test.js → 19/19 (was 18)
• node --test server/__tests__/lane-lifecycle.test.js → 29/29 (was 28)
• node --test server/__tests__/lanes-api.test.js → 22/22 (was 20)
• node --test server/__tests__/lanes-cli.test.js → 13/13 (unchanged, re-run as a caller check for follow-up 4)
• npm run test:server → 861/861 (baseline 857)
• cd client && npx vitest run src/components/lanes/__tests__/DestructiveLaneModal.test.tsx → 16/16 (was 14)
• npm run test:client → 299/299 (baseline 297)
• npm run build → succeeded (client build, tsc -b && vite build)
• bash .claude/skills/file-headers/scripts/check-headers.sh → exit 0
• node scripts/generate-openapi-yaml.js && git diff --exit-code openapi.yaml → exit 0, no diff
• npm run format:check → all files pass

Left out / deferred, with reasons

• Did not touch the "Each verb → remove" bullet in docs/LANES.md (a different paragraph from the one named in the brief) — it only describes the ordinary managed-removal steps and makes no claim about the corrupt-.git case, so it was not inaccurate and editing it would have been unrequested scope.
• Did not add a CLI-level test reproducing the corrupt-.git removal through ccam lanes remove — the brief asked for "a test proving (a)" which the new worktree.test.js case already does directly against removeWorktree (the same function the route and CLI both call); adding a second, slower end-to-end CLI version would duplicate coverage without proving anything new.
• Did not add a real concurrent-claude-process test for follow-up 2 (two genuine POST /start calls racing against real spawned processes) — the codebase's own existing tests avoid ever spawning claude twice concurrently for exactly this reason (slow, and the real claude binary's behavior isn't what's under test). The lock-holding test proves the same invariant deterministically without that cost.