feat: Claude Code Monitor — lanes, pipelines and a merged workspace
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.
This commit is contained in:
@@ -0,0 +1,53 @@
|
||||
# SDD ledger — plan: docs/superpowers/plans/2026-07-27-lanes-pipeline.md
|
||||
|
||||
Base commit: 8e41e80 (branch feat/lanes-pipeline)
|
||||
|
||||
Task 1: review — spec MET, quality SOUND. 1 Important, 1 Minor.
|
||||
Task 1: ruled — Important (.gitignore `data/` -> `/data/`) does NOT enter fix loop. Plan's Global Constraints say "Preserve existing behavior. Additive schema only"; a gitignore anchor changes no behavior, and the reviewer's "purely additive" alternative edits the same file AND cannot work alone (git cannot re-include a path under an excluded directory without also negating the parent). `/data/` is the correct minimal fix. Stands.
|
||||
Task 1: minor (deferred): lanes-lib.test.js test title says "amber" where it means "without evidence" (wording inherited from the plan).
|
||||
Task 1: complete (commits 8e41e80..8b9e477, review clean after ruling)
|
||||
|
||||
Task 2: review — spec MET except one deviation; all 5 load-bearing behaviors verified correct. 1 Important, 1 Minor.
|
||||
Task 2: ruled — Important (lane SQL prepared inline in lanes.js instead of added to db.js `stmts`) does NOT enter fix loop: the plan contradicted itself (File Structure said "stmts entries", the Task 2 code block and its Interfaces line "Owns all SQL for lanes" say inline). The code block is authoritative; the stale File Structure line has been corrected in the plan so later reviews do not re-raise it.
|
||||
Task 2: minor (deferred): lanes-lib.test.js @file comment still says it only covers pipelines.js.
|
||||
Task 2: complete (commits 8b9e477..f29d904, review clean after ruling)
|
||||
|
||||
Task 3: review — spec MET; 1 "Critical" (WS payload asymmetry) ruled plan-mandated, 3 Important, 3 Minor.
|
||||
Task 3: ruled — delete broadcasting `{removed: id}` instead of `{lane}` is deliberate and consumed by Task 7's Lanes.tsx; the plan's Produces bullet was stale prose and has been corrected. Not a defect.
|
||||
Task 3: ruled — per-lane `SELECT MAX(created_at)` stands: events(session_id) is indexed and lanes number in the dozens (one per worktree), not thousands.
|
||||
Task 3: ruled — broadcast-before-response stands: `broadcast()` in server/websocket.js is already defensive.
|
||||
Task 3: minor (deferred): double payload() call per mutation (broadcastLane re-reads the lane); lanes-api.test.js tests share state via an outer laneId.
|
||||
Task 3: fix round 1/5 (2 addressed, 0 open — 409 now branches on SQLITE_CONSTRAINT_UNIQUE with message fallback; new WS test asserts lane_update on create and delete; commits 8e76c90..76c6f26)
|
||||
Task 3: complete (commits a69b3d8..76c6f26, review clean)
|
||||
|
||||
Task 4: review — spec FAIL (1 Critical: needs_action cleared by any session), 2 Important, 1 Minor.
|
||||
Task 4: ruled — the Critical was real. The plan's own contract said "cleared on the next non-Notification hook for that lane"; amended to "from the session currently bound to that lane, evaluated before rebinding" so two agents sharing one worktree cannot cancel each other's "needs you".
|
||||
Task 4: fix round 1/5 (3 addressed, 0 open — clear now gated on the pre-existing lane.session_id; cross-session + default-message tests added, verified to fail against the buggy code; per-hook lane-scan ceiling documented, no cache; commits 83aa655..3f7121d)
|
||||
Task 4: complete (commits 76c6f26..3f7121d, review clean) — 780 server tests pass
|
||||
|
||||
Task 5: first attempt reported DONE_WITH_CONCERNS claiming the sandbox blocks loopback — WRONG, and nothing was committed (pre-commit test gate held). Real cause: the plan's own test harness used blocking spawnSync while the test HTTP server ran in the same process, so the event loop stalled and the CLI child's request was never served. Verified loopback works between processes (detached node server + curl + separate node fetch, all exit 0). Plan's code block corrected to async spawn.
|
||||
Task 5: review — spec PASS, 2 Important (ccam lanes add untested; temp DB leaked), 3 Minor.
|
||||
Task 5: fix round 1/5 (2 addressed, 1 open — lanes-add test added, health poll replaced the 100ms sleep, DB cleanup added but not exception-safe; commits 19dd31b..3deb420)
|
||||
Task 5: fix round 2/5 (1 addressed, 0 open — after() teardown wrapped in try/finally; commit 35138b4; verified inline)
|
||||
Task 5: complete (commits 3f7121d..35138b4, review clean) — 783 server tests pass. `ccam lanes add` was an authorised addition so the CLI's empty-state hint names a command that exists.
|
||||
|
||||
Task 6: review — spec MET; security verified adversarially (cross-origin POST to /:id/start returns 403; prompt travels via stdin, model/effort/resumeSessionId are separate argv with no shell; /:id/stage not captured by /:id/:action; unknown action rejected before any lookup or mutation). 1 Important, 2 Minor.
|
||||
Task 6: fix round 1/5 (2 addressed, 0 open — message now consults getRun and returns 409 for a recorded-but-dead run instead of 500; unknown-action test asserts stage/status unchanged; new 409 test; commits f225fe8..b3f9e2f)
|
||||
Task 6: minor (deferred): lane.cwd is not re-validated at action time — a vanished directory fails cleanly at spawn.
|
||||
Task 6: complete (commits 35138b4..b3f9e2f, review clean) — 788 server tests pass
|
||||
|
||||
Task 7: review — 4 Critical, 2 Important, 2 Minor. Three of the Criticals were the PLAN's fault (it mandated wrapping LaneCard in a <button>, an unconfirmed remove, and hardcoded English strings). Review was right; plan was wrong.
|
||||
Task 7: fix round 1/5 (7 addressed, 0 open — selection is now a keyboard-operable role="button" div with stopPropagation on actions; remove goes through the existing ConfirmModal; every string i18n'd with real zh/vi/ko translations; Lanes case added to screens.snapshot.test.tsx; unknown-lane lane_update refetches so counters stay server-truthful; amber-vs-green test now asserts the colour tokens; start's empty-prompt behaviour documented in a tooltip; commits f29f597..a05065d)
|
||||
Task 7: minor (deferred): the Lanes screen snapshot captures the empty state only — a populated card + pipeline map is not snapshotted.
|
||||
Task 7: minor (deferred): the card cannot send a prompt — driving a lane from the UI needs a prompt/message input; today `start` opens a promptless conversation run and `message` has no input field. Follow-up feature, not a defect.
|
||||
Task 7: complete (commits b3f9e2f..a05065d, review clean) — verified by controller: 279/279 client tests, `npm run build` clean
|
||||
|
||||
Task 8: review — every documented command/flag/env var/endpoint/state-rule fact-checked against the shipped code and correct, EXCEPT one Critical: docs advertised `ccam lanes add --pipeline <id>` which the CLI never parsed.
|
||||
Task 8: ruled — fix the CODE, not the docs: a pipeline template you cannot select from the CLI is a template nobody uses, and the server already accepted the field. Authorised a scoped code change inside the docs task.
|
||||
Task 8: fix round 1/5 (1 addressed, 0 open — --pipeline parsed and forwarded only when provided, usage string updated, two tests via a DASHBOARD_PIPELINES_DIR fixture with cleanup, docs parenthetical corrected; commits 6cca59c..3b196ef)
|
||||
Task 8: minor (deferred): README VN/CN/KO mirrors are now behind the English README; the repo's update-project-docs convention expects them synced.
|
||||
Task 8: complete (commits a05065d..3b196ef, review clean) — 790 server tests pass
|
||||
|
||||
Final whole-branch review (Opus): READY — no Critical/Important/Minor findings. Cross-cutting types, CLI-vs-server cwd resolution, WS payload branches, migration safety on an existing DB, the same-origin guard on the spawning routes, and both confirmation gates all verified. All 8 deferred/parked items triaged acceptable-to-defer, none load-bearing.
|
||||
Controller end-to-end smoke test (real server, real CLI, real hook POSTs) — lane created, `ccam stage plan --evidence` then `ccam stage review` reported: node states came back intake=passed-no-evidence, plan=done, implement/tests=passed-no-evidence, review=current, rest=pending; progress 57%; stage_seconds live; `ccam lanes` table and counters correct; a Notification hook bound the lane and a later hook from the newly-bound session cleared needs_action exactly as the amended contract specifies.
|
||||
Side effect found and REVERTED: starting the fork's server auto-configured Claude Code hooks in ~/.claude/settings.json (8 entries pointing at this fork). Removed surgically; the pre-existing `rtk hook claude` PreToolUse entry was left untouched. Polluted copy kept at /tmp/settings.before-ccam-cleanup.json.
|
||||
@@ -0,0 +1,31 @@
|
||||
# SDD ledger — plan: docs/superpowers/plans/2026-07-28-stage-detection.md
|
||||
|
||||
Base commit: d0044d1 (branch feat/stage-detection)
|
||||
|
||||
Task 1 (B1): written by Codex terra (foreground; four earlier background runs were killed by the harness's background wall-clock limit). Review — spec MET, all 9 scenarios covered by 5 tests; totality verified by a reviewer that actually threw null/numbers/arrays/circular refs/symbols/malformed rules/a throwing property getter at it (zero throws), and regex compilation confirmed once-per-pipeline (1 RegExp construction across 1000 events, WeakMap keyed on pipeline identity). 2 Important, 1 Minor.
|
||||
Task 1: fix round 1/5 (3 addressed — the signal was unbounded at 5002 chars for a real Bash command and is now whitespace-collapsed and capped at 120 with an ellipsis; `flattenInput` had concatenated EVERY top-level string, so a real Edit event's `old_string`/`new_string` (whole code blocks) would have been matched against and then shown as the reason for the inference — now restricted to an allowlist of identifying fields; compileRules gained direct totality tests. Totality re-verified by the fixer with its own script.) A Codex attempt at this fix hit the 540 s wall and produced nothing, so it was done by a Claude subagent.
|
||||
Task 1: complete (869 server tests) — commits 6bb445e + the fix commit
|
||||
Tooling note: Codex completed 3 of 10 attempted runs in this environment; when it finishes, its work is good (it was the only implementer that ran clause-deletion experiments unprompted), but each failure costs ~9 minutes, so implementation moved to Claude subagents. The useful half of the Codex protocol was kept for every implementer: the agent runs only its own focused test file and does NOT commit; the controller runs the full suite and commits, so the 861-test pre-commit hook runs once per task instead of twice.
|
||||
|
||||
Task 2 (B2): rules on plan/implement/tests/review/ship in the default template (intake/gate/done deliberately bare — a gate is a judgement, `done` is a claim); three per-column probes for detected_stage/detected_signal/detected_at; `recordDetection` with the forward-only + declared-wins guard; `detected` decorating nodeStates' output without touching how any state is computed.
|
||||
Task 2: the implementer found an integration bug in the PLAN, not in its own work, and flagged it instead of fixing outside its file list: `loadAll()` in pipelines.js normalises each node to {id,label,icon,gate,aliases} and DROPPED the new `detect` array, so every rule shipped in the template was inert. Controller reproduced it directly — raw template matched `tests`, `getPipeline('default')` matched null. Neither B1's review nor B2's own tests could have caught it: B1 tested the matcher with raw fixtures that bypass the loader, and B2 tested the rules as JSON. Fixed by preserving `detect` defensively in the loader, plus an END-TO-END test that calls detect(getPipeline('default'), event) through the real loader — the pin that was missing.
|
||||
Task 2: controller verified through the real path afterwards: rules live, an Edit's signal is its file_path (not the code from old_string/new_string), `gate` still uninferrable, and getPipeline() still returns a stable object so stage-detect's WeakMap regex cache keeps compiling once per pipeline.
|
||||
Task 2: complete (878 server tests)
|
||||
|
||||
Task 3 (B3): detect + recordDetection + broadcast placed inside the existing fail-safe try/catch in touchLaneFromHook, right after the lane is resolved; broadcast only when recordDetection reports written:true, with a comment naming the 29,470-Bash-event volume behind that rule. 5 new API tests including the premise guard at the API level.
|
||||
Task 3: controller smoke-tested on a real server. First attempt looked like a total failure (detected_stage null) — cause was NOT the code: a server from an earlier branch still held port 4820, so the new instance refused to start and the probes hit stale code. After stopping it: detected_stage=tests with the right signal; 15 identical events plus one backward Edit left updated_at completely unchanged (write-on-change holds under real traffic); the backward Edit did not drag the lane back; every node still `pending` with a `detected` flag and no node `done`.
|
||||
Task 3: complete (883 server tests)
|
||||
|
||||
Task 4 (B4): detected nodes render amber-dashed via a class that REPLACES the state-driven one, so even a hypothetical {detected:true, state:"done"} payload renders amber and never green — the safe direction. `auto:` chip only when the detection leads the declaration. tsc (not vitest) caught a missing field in an unrelated test fixture, which is why `npm run build` is in every client task's verification list.
|
||||
Task 4: complete (308 client tests)
|
||||
Task 5 (B5): `ccam lanes` gains a detected suffix using the same lead-comparison as the card, so terminal and browser never disagree; docs/LANES.md documents the signals, the field allowlist, the 120-char cap, the rules node by node, the deliberate blanks on intake/gate/done, forward-only + write-on-change, and the evidence boundary with its reason. The implementer flagged rather than papered over the fact that GET /api/lanes has NO OpenAPI path docs at all and no Lane schema — it added a standalone schema for the three fields instead of inventing either.
|
||||
Task 5: complete (885 server tests)
|
||||
|
||||
FINAL whole-branch review (Opus): READY WITH FIXES — 1 Critical, 5 Important, 7 Minor. The evidence boundary itself held: no lane state could be constructed in which a detection renders green or writes lanes.stage, proved by mutation and live probing rather than by reading, with per-event cost measured (1 µs for an ordinary Bash event, zero queries when nothing changes).
|
||||
FINAL Critical — clearLane() did not reset the detection columns. A lane reset back to base kept claiming `auto: ship` with 7 of 8 nodes amber for an empty tree, and then refused every subsequent detection with `behind-detected` — and since no node past `ship` carries a rule, detection was dead for that lane permanently.
|
||||
FINAL corrected MY OWN RULING: I had called the ENOLANE race in the hook path a non-issue "because the outer try/catch swallows it". Right conclusion, wrong reasoning — entering the catch skips the rest of the function, so `needs_action` was left lit and a rebound `session_id` unwritten. The fix is ordering/isolation, not error handling.
|
||||
FINAL found a second inert rule, the same defect class as the loader bug: `plan`'s `Write docs/.*plan.*\.md` was shadowed by `implement`'s unconditional `Write` because detect() takes the LAST match, so writing a plan document reported `implement`. Documented as live, could never fire.
|
||||
FINAL proved the boundary was unpinned: removing `&& !stages[n.id]` from withDetected left 56/56 tests passing, because both tests calling themselves PREMISE GUARD sat on lanes with no declarations, making "no node is done" vacuously true.
|
||||
Fix wave (one commit, 9da58ea, Opus): all 9 items fixed with seven verify-by-deletion experiments and their exact failure messages. It DECLINED my instruction to move the detection block below the bookkeeping, correctly: that block ends in `if (!Object.keys(patch).length) return;`, so detection would have become unreachable on the common hook. It used an inner try/catch instead and corrected the docs sentence. `implement`'s Write rule is now constrained with a negative lookahead, verified at the boundaries (mydocs/, docs2/ still implement).
|
||||
Fix-wave re-review: READY. Re-ran the G-1 mutation itself in a throwaway copy (32 pass / 1 fail, same tally), rebuilt the I-2 resolution table independently, and traced the alias mechanism to confirm the I-3 test would fail against the old code. All three of the fixer's open items judged non-blocking.
|
||||
B complete: 10 commits c78e7f7..9da58ea, 892 server tests, 308 client tests.
|
||||
@@ -0,0 +1,59 @@
|
||||
# SDD ledger — plan: docs/superpowers/plans/2026-07-28-worktree-lanes.md
|
||||
|
||||
Base commit: 7195741 (branch feat/worktree-lanes)
|
||||
|
||||
Task 1: first attempt committed with --no-verify after misdiagnosing a hook failure as "test isolation". Controller reproduced it via `git commit --amend`: git hooks export GIT_DIR/GIT_INDEX_FILE, every git child inherited them, and in a worktree `.git` is a FILE so `.git/index` gave ENOTDIR. Real bug in worktree.js, not the environment. Fixed by scrubbing 9 GIT_* vars (+ GIT_TERMINAL_PROMPT=0) in the git() helper and the test fixture, with a regression test that sets bogus GIT_DIR/GIT_INDEX_FILE. Recommitted through the hook.
|
||||
Task 1: review — spec MET (one accepted deviation: reset-in-place instead of delete+recreate); adversarial checks all passed: path containment defeats symlink/prefix/`..`/missing-path attacks via realpath on both sides + path.relative boundary; branch deletion cannot be tricked into main/master/base even by a lying lane row; env scrub complete; no vacuous tests. 2 Important.
|
||||
Task 1: fix round 1/5 (2 addressed, 0 open — ERESETBRANCH verifies the worktree really landed on the feature branch; ENOBASE verifies the base ref before ANY mutation, with a test proving the worktree and its dirty files are untouched on that path; commits 436adfa..03a6e63)
|
||||
Task 1: complete (commits 7195741..03a6e63, review clean) — 801 server tests pass, every commit through the pre-commit gate
|
||||
|
||||
Task 2: review — spec met on the surface, 2 Critical underneath. (a) the migration probed only `kind` while adding four columns, so a crash after the first ALTER left three columns permanently missing on a real install — the plan's own fault; (b) `kind` was validated in createLane only, so updateLane could silently corrupt the boundary that decides whether CCAM may delete a directory. Plus 3 Important (lock Map never pruned, two missing tests).
|
||||
Task 2: fix round 1/5 (5 addressed, 0 open — per-column independent probes that self-heal a partial migration; shared validateKind() used by both create and update, rejecting before any write so a mixed patch cannot half-apply; lock entry deleted when its chain settles if still current; regression tests for update-kind, per-lane (not global) locking, and an old-schema database plus a simulated mid-migration crash; commits fdf4da4..6138944)
|
||||
Task 2: complete (commits 03a6e63..6138944, review clean) — 808 server tests pass
|
||||
|
||||
Task 3: review — read-only confirmed, shapes/route order correct. 3 Important: purge WHERE clause duplicated between counter and deleter; the purge test could not fail (every session it created was `active`, so all three exclusions were deletable with the test still green); `unpushed: 0` was a confident lie when no upstream is configured — the exact under-report the preflight exists to prevent.
|
||||
Task 3: fix round 1/5 (2 addressed, 1 NOT — shared `purgeCandidateSessions()` now the single expression of the rule; `unpushedCount` counts commits on no remote via `--not --remotes` and surfaces a distinct `no-remote` fact; but the purge test got WEAKER, not stronger: the implementer deleted the sessions entirely and asserted zeros against an empty table, while its report claimed it had added direct DB inserts. Controller verified: zero INSERTs in the file. commits 51c1c4c..85aa098)
|
||||
Task 3: fix round 2/5 (1 addressed, 0 open — handed to Codex gpt-5.6-terra effort medium, which built the discriminating fixture (counted / active / bound / sibling-prefix sessions + seeded events and token rows), ran all three clause-deletion experiments and reported the failure each produced; commit d247c37)
|
||||
Task 3: controller re-verified independently — removed the bound-session exclusion by hand, test failed `2 !== 1`, file restored, `git diff server/lib/lanes.js` empty. 7/7 lifecycle tests, 815 server tests.
|
||||
Task 3: complete (commits 6138944..d247c37, review clean)
|
||||
Tooling note: Codex cannot run through `codex-rescue` here — the subagent's Bash sandbox makes Codex's own bwrap fail with `loopback: Failed RTM_NEWADDR`. Working invocation, with the user's explicit approval to drop the sandbox for it: `codex exec --dangerously-bypass-approvals-and-sandbox -m gpt-5.6-terra -c model_reasoning_effort=medium "<prompt>"` run from the controller's Bash with dangerouslyDisableSandbox.
|
||||
|
||||
Task 4: implemented by Codex gpt-5.6-terra (effort medium) via `codex exec`, review by Claude. Review — no Critical; same-origin guard genuinely applied, slug cannot escape LANES_ROOT, failure path leaves the lane managed/failed/removable with git's stderr and no orphan directory (reviewer reproduced it against a real repo). Codex also updated ARCHITECTURE.md, docs/API.md, server/README.md and added an OpenAPI fragment — ruled NOT scope creep: .claude/skills/update-project-docs mandates exactly those files for an API change. 1 Important, 3 Minor.
|
||||
Task 4: fix round 1/5 (4 addressed, 0 open — boot sweep marks any still-'provisioning' lane failed with an explanatory note (a lane killed mid-provision could otherwise never age into 'dead', since a fresh managed lane has no session_id for classifyLiveness to measure); 409 EDUPCWD and the `base` default now documented in both OpenAPI and docs/API.md; directory-suffix loop capped at 50 with a 409 and a test that pre-creates exactly the 51 colliding directories needed to reach it; commits d524cd3..cf8ebbf)
|
||||
Task 4: re-review traced the boot ordering — the sweep runs as a microtask off `server.listen`'s resolve, before the poll phase can dispatch a connection, and is one synchronous better-sqlite3 UPDATE with no yield point, so no request can slip a new provisioning lane into the sweep. Confirmed it writes only status/notes/updated_at, leaving kind/cwd/branch/source_repo/base_branch/slug intact.
|
||||
Task 4: minor (deferred): recoverInterruptedProvisioning writes raw SQL instead of going through updateLane/PATCHABLE; the boot handler logs err.message without the stack.
|
||||
Task 4: complete (commits d247c37..cf8ebbf, review clean) — 821 server tests pass
|
||||
|
||||
Task 5: implemented by Codex terra, reviewed by Claude Opus. Review — guard work (three checks, force gating, purge honesty, error mapping, lock) clean, but 2 Critical: (a) "kill the run and await its exit" was vacuous — killRun sets status='killed' synchronously after SIGTERM, so the poll on status returned instantly and `git clean -fd` / `worktree remove --force` ran milliseconds later while a live Claude could still be writing into that directory; (b) `remove` had been silently narrowed to managed lanes, breaking the shipped Remove button for adopted lanes with no client path at all (api.ts had no DELETE method), and the existing test that encoded the old contract was rewritten instead of the regression being reported.
|
||||
Task 5: fix round 1/5 (7 addressed, 2 NEW breakages — added `actualExitedAt`, written only in the child's exit handler, polled with a 7.5 s deadline (> killRun's 5 s escalation) and failing loudly via ERUNTIMEOUT before any git; restored `remove` for both kinds (managed tears down the worktree, adopted deletes the row only); made `expect` mandatory with the full field set; guarded DELETE; ESTALE now carries expected/current. commits 24841d4..58a94e8)
|
||||
Task 5: fix round 2/5 (4 addressed, 0 open — the mandatory `expect` (my requirement) had broken the Remove button for EVERY lane with 400 EEXPECT, so the client now fetches preflight and echoes it, and surfaces errors instead of swallowing them; a child that fails to spawn emits only `error`, never `exit`, so `actualExitedAt` also set there — otherwise `claude` missing from PATH froze every destructive action for the whole reap window; killRun's SIGKILL escalation tested `!child.killed`, which Node sets true on a successful SIGTERM, so it could never fire — now keyed on real exit; PATCH guarded. commits 58a94e8..fa77725)
|
||||
Task 5: controller committed one leftover line Codex left uncommitted (`preflight: r({})` in the screens-snapshot API mock, cac0d2a) and re-ran the client suite from a clean tree: 279/279.
|
||||
Task 5: minor (deferred to C7): LaneCard has no Force affordance, so an unpushed reset/remove 409s with no way to retry from the UI; reset and purge are not on the card at all yet; adopted `remove` still demands force when the adopted directory has unpushed commits even though nothing is destroyed.
|
||||
Task 5: complete (commits cf8ebbf..cac0d2a, review clean) — 834 server tests, 279 client tests
|
||||
|
||||
Task 6: written by Codex terra, whose process was killed by a wall-clock limit TWICE before it could run a single test or commit — the work survived staged. A Claude subagent then read the staged diff, verified it and committed it unchanged (00fc9fa). So the code reached review having never been run by its author; the reviewer was told that and given permission to run the CLI test file itself.
|
||||
Task 6: review — no Critical, no Important. Verified field-by-field that the CLI's LANE_PREFLIGHT_FIELDS matches the server's expectedFields exactly and that `expect` is built from the freshly fetched preflight (not client-guessed); `--yes` cannot be bypassed (returns with exit 1 before any POST is constructed); `--force` only ever unlocks the unpushed gate; the adopted-lane refusal is plain language and its test asserts the file's CONTENTS survive; no spawnSync, no bare sleeps, no vacuous tests. Reviewer independently ran the CLI suite: 10/10.
|
||||
Task 6: minor (deferred): `ccam help` shows `[--yes]` while the prose shows it unbracketed; no test covers the ESTALE 409 or the provisioning failed/timeout print paths; the destructive tests share a mutable `managedLane` across describe blocks.
|
||||
Task 6: complete (commits cac0d2a..00fc9fa, review clean) — 839 server tests
|
||||
|
||||
Task 7: Codex wrote the component work and was killed by a wall-clock limit a third time, leaving it uncommitted and missing both the modal tests and all the documentation; a Claude agent audited it, wrote those, and committed (2a58f5a).
|
||||
Task 7: that agent also found and fixed a real pre-existing bug OUTSIDE its brief: client/src/i18n/index.ts never registered the `lanes` namespace, so no string on the Lanes page had ever resolved — every locale, including the vi/ko/zh translations shipped in the earlier plan's Task 7, rendered as raw keys. Both that task's reviewer and its re-reviewer had passed i18n as complete; both had only checked that the locale files contained the keys, never that the namespace was loaded.
|
||||
Task 7: review — spec MET throughout; docs fact-checked line by line against the code (three safety checks, `clean -fd` without `-x`, preflight field lists, bytesEstimate derivation) and found truthful, including correctly documenting that LANE_BASE_BRANCH / LANE_BRANCH_PREFIX do NOT exist rather than inventing support. 1 Critical.
|
||||
Task 7: Critical (open, goes to C8) — `no-remote` sits in the same `blocked[]` array as hard blockers, so a managed lane in a repo with no remote can never be reset or removed from the UI: unpushedCount counts every commit when there is no remote, giving `blocked = ["unpushed-commits","no-remote"]`, the modal treats anything but `unpushed-commits` as a hard block, and its Force checkbox only appears when `blocked.length === 1`. The server gates nothing on `no-remote` and the CLI works fine — the UI locks only itself, permanently, for a common case.
|
||||
Task 7: minor (goes to C8): LaneCard renders `t("status.<status>")` but no locale has any `status.*` key, and i18next returns the key on a miss, so the `||` fallback never fires — every lane shows literal text like `status.active` right now.
|
||||
Task 7: complete (commits 00fc9fa..2a58f5a, 1 Critical carried into C8) — 287 client tests, 840 server tests
|
||||
|
||||
Task 8 (added after Task 7's Critical): implemented by a Claude subagent. Root-cause fix rather than a patch — `blocked[]` had been conflating three kinds of thing, so it now carries only the action-preventing conditions (adopted, missing, unreadable) plus `unpushed-commits` (the one Force overrides), while purely informational facts (`no-remote`) moved to a new `warnings[]`. The modal needed no logic change at all: it was locked out purely because the server mislabelled `no-remote` as blocking. Also landed: status.* i18n keys for all four locales, real support for LANE_BASE_BRANCH and LANE_BRANCH_PREFIX (the spec had promised them, the code had hardcoded main and feat/), CLI tests for the ESTALE print path and for failed provisioning, and `--yes` presented as mandatory everywhere.
|
||||
Task 8: review — APPROVE, no Critical or Important. Reviewer enumerated every blocked[] combination the server can emit and confirmed the modal and the server now agree on all eight; ran the ESTALE test three times (12/12 each, ~300 ms) and confirmed it is structural rather than timing-based, since GET /preflight takes no lock while POST does; verified the env-var tests actually set the variables and observe their values rather than asserting a default that would pass anyway; fact-checked the docs against the code and found no inaccurate sentence.
|
||||
Task 8: minor (deferred, for final-review triage): `lanes.status` is not validated at the API layer — `POST /:id/stage` and `ccam stage --status <s>` accept an arbitrary string, so a rogue value would reproduce the raw-key badge bug this task fixed. The robust fix is a t() defaultValue in LaneCard rather than restricting what an agent may declare.
|
||||
Task 8: complete (commits 2a58f5a..3139de9, review clean) — 844 server tests, 292 client tests
|
||||
|
||||
FINAL whole-branch review (Opus): READY WITH FIXES — 2 Critical, 6 Important, 9 Minor. Safety core sound: every caller of resetWorktree/removeWorktree/purgeLaneSessions/deleteLane enumerated across server/, bin/, scripts/, mcp/ (the CLI goes over HTTP, so nothing outside the routes reaches the git layer), the guard sits INSIDE both destructive functions rather than only upstream, and the migration was verified against real SQLite including a simulated mid-migration crash.
|
||||
FINAL: the reviewer earned its verdict by mutation — it deleted `await assertDestroyable(lane)` from removeWorktree and all 844 tests stayed green, proving the remove-path guard was pinned by nothing. The same deletion in resetWorktree failed a test.
|
||||
FINAL: controller raised the reviewer's Important I3 to Critical — `cwd LIKE ? || '/%'` was unescaped and `_` is a LIKE wildcard, while every managed lane directory this branch creates is named `<repo>__<slug>`. A lane at /root/myrepo__feat-foo purged sessions belonging to /root/myrepoXXfeat-foo, and the preflight counted the victims too, so the confirmation was consistently wrong rather than detectably wrong. The old fixture (/tmp/wt-purge) had no underscore, so nothing could have caught it.
|
||||
Fix wave (one commit, 513235a, Opus): all 16 findings addressed, +997/-151 across 28 files, 844→857 server tests, 292→297 client tests. Deletion experiments run and reported for C2, C3, I1 and I2. The fixer also found a FOURTH instance of "the UI refusing what the server permits" in bin/ccam.js and disclosed it rather than fixing it silently.
|
||||
Fix-wave re-review (Opus): READY. Verified 857/857 itself, re-ran the C2 mutation in a throwaway copy, and built the four-way table (adopted/missing/unreadable/no-remote × reset/remove/purge × modal/server/CLI) — no square where the UI is stricter than the server, no third instance of the defect.
|
||||
FINAL: the re-reviewer found what neither the fix wave nor I had: a FOURTH refusal shape for C2 that git does NOT incidentally refuse — an adopted lane legitimately pointing at a worktree the user registered themselves. With the guard removed it destroyed the directory silently with no error at all. Check 1 is the sole protection for that case, so the guard was load-bearing well beyond the three shapes the tests cover.
|
||||
FINAL: it also disproved the fix wave's own reasoning on `unreadable` + `remove` by reproducing a corrupt-.git worktree: `git worktree remove --force` and even `--force --force` both refuse (code 128), so the promise written into `destructive.notice.unreadable` in all four locales and into two doc lines — that removal is forced and git's entry cleared — is FALSE for that shape. Decision to unblock stands (blocking recreates the defect class), but the copy over-promises.
|
||||
FINAL: registered as a known fact about the branch, not a surprise — `removeWorktree`'s new prune path deliberately does NOT call assertDestroyable; it runs check 1 plus a lexical check 2 and substitutes `worktree prune` for check 3, fires only when the directory does not exist, and is pinned by a test that still refuses a missing cwd outside LANES_ROOT.
|
||||
Open follow-ups, none blocking, surfaced to the user: (1) reword the `unreadable` notice + two doc lines to promise an attempt rather than success, add a prune-style fallback so an unreadable lane is genuinely removable, add the two missing modal tests; (2) wrap `start` in withLaneLock so its atomicity is an invariant rather than a property of the current await-free code; (3) add an openapi.yaml drift check to CI; (4) separate triage for the pre-existing unguarded `POST /api/lanes/` and `POST /api/lanes/:id/stage`.
|
||||
@@ -0,0 +1,204 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user