nntrivi2001
57dc91585d
Internal SmartGift build of a Claude Code monitoring dashboard. Lanes: a durable unit of parallel agent work, one per working directory, tracked across session restarts. Managed lanes are git worktrees the dashboard provisions and can reset or remove behind a three-check destroy guard and a counted preflight; adopted lanes are directories you already own and are never destroyable. Pipelines: a lane moves through pipeline stages. A stage the agent declares with evidence renders green; a stage inferred from the tool-event stream renders dashed amber and never counts as done. Detection is forward-only within a 30-minute window, and never writes the declared stage. Workspace: one page at /run with a lane grid, the selected lane's pipeline, and a full Claude console behind a disclosure.
205 lines
10 KiB
Markdown
205 lines
10 KiB
Markdown
# 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.
|