# 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 (`/worktrees/`) 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/` 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.