Skip to content

docs(spec): record the CLI-119 lane-next review (FAIL) #2573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mlorentedev merged 1 commit into from
Oct 16, 2026
Merged

docs(spec): record the CLI-119 lane-next review (FAIL) #2573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions specs/CLI-119-lane-next/review-request.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"reviewed_sha": "ff0b1612e816b4adc355874d0cd2c09ca89419e3",
"reviewer": "nan/qwen3.8-flash",
"requested_at": "2026-10-15T06:55:21Z",
"review_digest_before": "",
"base_sha": "cfffe79de47b31b326ae1222248f78414bd6e55a",
"contract_digests": {
"features.json": "b5e9549dfbc3af5ff0ff8ce5c0f9544e1910b1714a3495dfccdedeffb20dbe27",
"proposal.md": "7b14f9fecd4915e1dbbe94349b2255015b1352a5ee8fc3ae9e157890057672b1",
"tasks.md": "d617197cac87d0953afd87ee2316529cdf0c2e7cf561c5b61ef9344a720de531"
}
}
79 changes: 79 additions & 0 deletions specs/CLI-119-lane-next/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
---
spec: "CLI-119-lane-next"
verdict: "FAIL"
reviewed_sha: "ff0b1612e816b4adc355874d0cd2c09ca89419e3"
reviewer: "nan/qwen3.8-flash"
date: "2026-10-15"
---

## Adversarial review

**Scope**: CLI-119-lane-next — `dotf lane exec` + `dotf lane next` (herdr backend, `.dotf/lane.json` queue, prompt template).
**Sources**: `specs/CLI-119-lane-next/{proposal,tasks,verification}.md` + `features.json`; diff `cfffe79de47b31b326ae1222248f78414bd6e55a...ff0b1612` (2 commits: `58909de7` = this spec, `ff0b1612` = CLI-118-worktree-one-root).

Everything below was run fresh at `ff0b1612` on this Mac (herdr 0.9.3, dotf release 0.70.0). Working tree left clean.

**Re-ran the whole gate, and it is green**: `go build ./...` exit 0 · `go vet ./...` exit 0 · `go test ./internal/lane ./internal/cmd ./internal/worktree -count=1` → `ok` ×3 · `GOOS=windows go vet ./...` exit 0 · `golangci-lint` 2.12.2 on `./internal/lane/... ./internal/cmd/...` → `0 issues.` · `bats tests/herdr-config.bats` → 6/6 ok (incl. the new WORKTREE_ROOT↔herdr row) · all five Go `features.json` verification commands pass, and f6's `grep '^RESULT <agent> PASS'` passes. So `verification.md`'s "Test status" claims hold; the findings are not about the tests lying, they are about what the tests do not look at.

**Live behaviour also checks out on the happy path**: `dotf lane next --dry-run` from this linked worktree picked `#2279` (the first OPEN/unassigned of `[2501, 2434, 2279]`, so `Next` is right against the real forge), resolved the parent `/Users/manu/Projects/dotfiles`, and typed `/tmp/dotf-review lane exec --agent claude --prompt-file …` — the deciding binary's absolute path, i.e. lesson 438 applied. No `[AGENT-DRAFT]`/`[AGENT-SUGGESTION]` tags remain.

### Spec and task alignment

- AC1 (argv as one element, three agents, unknown refused): code + tests verified (`TestLaunchArgv_…`, `TestLaneExec_…` re-run green); real launches recorded in `verification.md`. The refusal half is met in-process but **not visible to a user** — finding 1.
- AC2 (linked worktree → create through parent, caller untouched): verified; `Backend` has no remove method, so the guarantee is structural, and `TestNext_FromALinkedWorktreeCreatesThroughTheParentAndLeavesTheCallerAlone` re-ran green.
- AC3 (twice → one agent): verified sequentially (`TestNext_RunTwiceStartsOneAgent` + the live second run in `verification.md`). Concurrency is not — finding 6.
- AC4 (`--agent` default from pane; unknown refused *naming the known ones*): met by code, **not met as the user experiences it** — finding 1.
- AC5 ("With no backend it prints the commands and exits 0"): **not met as written** for the plain `dotf lane next` — finding 2.
- AC6 (end-to-end finisher → successor for claude, pi, agy): evidence is prose in `verification.md`, not re-runnable; and the instruction the successor actually reads is broken today — finding 4.
- `proposal.md`'s six AC boxes are all `[ ]` and every `tasks.md` "Closing" box is `[ ]`, including "PR opened", although both PRs are merged — finding 11.
- Scope: the launcher's base spans two specs. `ff0b1612` is CLI-118's change (`worktree/root.go`, `worktree.go`, `env-contract.json`, `specs/CLI-118-worktree-one-root/`), which `proposal.md` names *out of scope*. It is not creep by this spec's author — it is a second merged PR inside the stated review window — but it is a second spec's change with no `review.md` of its own (finding 12), and CLI-119's no-backend path is inconsistent with the rule it establishes (finding 3).

### Findings

| Severity | Reality | Area | Finding | Evidence | Test (named, or UNTESTED) | Fix location |
|---|---|---|---|---|---|---|
| Blocker | REAL | cli error surface | `lane exec` and `lane next` swallow every error: exit 1 with **empty stdout and empty stderr**. Both leaf commands set `SilenceErrors: true` (`cli/internal/cmd/lane.go:44,80`) and nothing else prints the returned error, so a refusal never reaches the terminal — AC4's "refused with the list of known ones" and AC1's guards are invisible; in a pane the successor just dies at a blank prompt. | `/tmp/dotf-review lane exec --agent copilot --prompt-file /tmp/p.md` → `exit=1 out:[] err:[]`; `--prompt-file /tmp/nope.md` → `exit=1` silent; prompt `-x flag like` → `exit=1` silent; `lane next` no-backend/no-agent → `exit=1` silent. Same class prints for a command that does not silence: `dotf worktree done no-such-slug` → `Error: no worktree "no-such-slug"…`. Mutation: deleting both `SilenceErrors` lines leaves `go test ./internal/cmd -run TestLane` **green** → the display path is uncovered. | UNTESTED (`TestLaneExec_HandsTheAgentTheFilePromptAsOneArgument` asserts the returned `err`, never the printed output) | code + tests (drop `SilenceErrors`, or print; add a test capturing stderr) |
| Major | REAL | AC5 / spec-vs-code | `dotf lane next` with no backend and no `--agent` **errors instead of printing the commands and exiting 0**: `resolveAgent` needs a backend to ask the pane, and there is no documented default, so the natural no-backend invocation fails. AC5 as written has no escape for "agent unknown". | live, PATH stripped of herdr: `lane next --dry-run` → exit 1, no output; `lane next --dry-run --agent claude` → exit 0 with the four manual commands. | UNTESTED (`TestNext_NoBackendPrintsTheCommands` always passes an explicit agent) | code (a documented default, or print-and-note) **or** spec (word AC5 as `--agent`-required — a contract edit ⇒ re-review) |
| Major | REAL | worktree layout (cross-change with CLI-118 in this same diff) | The no-backend manual commands contradict the one-root rule this very diff establishes: `manualCommands` hardcodes `<repo parent>/worktrees/<repo>/<Slug(title)>` instead of resolving `$WORKTREE_ROOT` + `<branch-slug>`. Wrong root whenever `WORKTREE_ROOT` is set off its default (env-contract declares it overridable), and the directory name is **always** wrong: no branch prefix. A person following the printed commands lands a worktree `dotf worktree done` will not find under the branch, and two tools put siblings in two trees — the exact #2503 failure CLI-118 exists to close. | `WORKTREE_ROOT=/tmp/wtrepro/root dotf worktree add demo -b fix/x` → `Created worktree at /tmp/wtrepro/root/repo/fix-x`; `lane next --dry-run` (no backend) prints `git … worktree add -b fix/harness-bind-… /Users/manu/Projects/worktrees/dotfiles/harness-bind-…-so-an` — `x` vs `fix-x` mismatch confirmed by `TestResolveWorktreePath` row `{"fix/x--y", "/w/repo/fix-x-y"}` | UNTESTED (`internal/lane/handoff_test.go` never asserts the printed paths against the layout rule) | code (`worktree.ResolveWorktreePath` + `env.ResolvePath("WORKTREE_ROOT")`) + tests |
| Major | REAL | rollout / the shipped prompt | `.dotf/lane-prompt.md` — committed by this change — tells every successor "run `dotf lane next`". The `dotf` on a pane's PATH is release 0.70.0, cut 2026-10-15T01:32Z, **before** this merge, and has no `lane`. So the loop the spec exists to create breaks at its own last step, and `verification.md` records this exact failure already happening once ("unknown command \"lane\"") — fixed for the machine-typed command by using the absolute path, left in the prose the agent reads. AC6's chain is therefore proven only with a dev-built binary. | `dotf --version` → 0.70.0; `dotf lane --help` → `Error: unknown command "lane" for "dotf"`; `gh release list` → latest v0.70.0; `gh pr list --search "release in:title"` → **#2520 `chore(main): release 0.71.0` still open** | UNTESTED (nothing pins that the installed release carries `lane`) | docs/ops (outside the contract set: cut/confirm 0.71.0 before running a lane, or make the template name the binary the successor was started with) |
| Major | THEORETICAL | silent fallback | When the deciding binary's path is not a plain word (`/home/a b/dotf`) or `os.Executable()` fails (`self, _ :=` discards the error at `cmd/lane.go:114`), `Handoff` **silently substitutes bare `dotf`** and then prints "started the successor" while the pane runs a different dotf — the failure class lesson 438 (added by this diff) names. The asymmetry is stark: an untypeable *prompt* path is a hard error, an untypeable *binary* path is a quiet lie. | code read of `handoff.go:69-75`; the behaviour is pinned by `TestNext_TheSuccessorRunsTheTwoBinaries`… — precisely by `TestNext_TheSuccessorRunsTheBinaryThatDecided` rows `"/home/a b/dotf" → "dotf lane exec "` and `"" → "dotf lane exec "`; trigger not observed on this box (clean paths) | `TestNext_TheSuccessorRunsTheBinaryThatDecided` names the behaviour but *endorses* it, so nothing proves it safe — add a test that the outcome states a substitution happened | code + docs |
| Minor | THEORETICAL | concurrency | `Find` → `Create` is a check-then-act race: two sessions started at once both see no worktree and both create, so AC3's "or from two sessions" is proven only sequentially. | `handoff.go:88-101`; `TestNext_RunTwiceStartsOneAgent` replays one run then the other against a fake | UNTESTED | tests (parallel-run case) |
| Minor | THEORETICAL | resilience | If `Create` succeeds and `Type` fails, the branch is left with an open workspace and no agent; every later run then answers "a workspace already shows <branch>; no second agent started", so the queue stalls until a human closes the workspace. No retry or "type again" path. | `handoff.go:110-117` (the error returns after `Create`, and `found && wt.Open` short-circuits next time) | UNTESTED | code + tests |
| Minor | THEORETICAL | naming | `Slug` collides: two issues whose titles slug the same (or both yield `"lane"` when a title has no `[a-z0-9]` word) share one branch, so the second is never started — `Find` sees the first's worktree. | `queue.go:78-96` (`maxSlug` word-boundary cut, `if len(words)==0 {return "lane"}`) | `TestSlug_DropsTheTicketIDAndKeepsAWordBoundedBranchName` covers shape, not collision | tests |
| Minor | THEORETICAL | config validation | `base` without a slash (`"base": "main"`) makes `Fetch` run `git fetch main` (remote=`main`, branch=``) and `manualCommands` print `git … fetch main`; `LoadConfig` validates neither, so the error surfaces at the fetch with a confusing message. | `cmd/lane.go:118-121` (`strings.Cut(cfg.Base, "/")`), `handoff.go:163`; `LoadConfig` defaults only | UNTESTED | code + tests |
| Minor | SPECULATIVE | maintainability | `Handoff` is 72 lines and `runLaneNext` 51 — over the repo's <40-line law; CI stays green because the pin does not enable `funlen`. Measured with an awk line count over the diff's files, and `golangci-lint run` → `0 issues.` | — | code (split decide/act/report) |
| Minor | REAL | spec artifacts stale | All six AC boxes in `proposal.md` are `[ ]` and every `tasks.md` Closing box is `[ ]` ("PR opened", "Lint passes", "No unrelated changes in the diff"), while both PRs are merged and the gate re-ran green here. The contract says one thing, reality another. | read of the two files at `ff0b1612`; `git log --oneline cfffe79..HEAD` shows both merges | n/a | spec (contract set ⇒ fixing these **here** invalidates this review; carry to the next round) |
| Question | — | process | (a) `features.json` carries `"state": "passing"` on all six features with prose evidence (`"go test run 2026-10-14"`), while `tasks.md` says the agent *cannot* write `passing` and only the harness may, after capturing exit 0. The states are **true** — I re-ran all six commands and each passes — but was the harness the author? (b) `ff0b1612` inside this review window is CLI-118's change and `specs/CLI-118-worktree-one-root/` has no `review.md`, so `dotf spec archive CLI-118-worktree-one-root` will refuse; that is that spec's gate, noted because the launcher put its code in this scope. | `grep` of the two files; `ls specs/CLI-118-worktree-one-root/` → features/proposal/tasks/verification only | n/a | process / vault |

### Evaluator rubric

| Dimension | Grade (A-D) | Rationale (one line) |
|-----------|-------------|----------------------|
| Correctness | C | AC1/AC2/AC3 verified against code, tests and a live dry run, but AC4 and AC5 are not met as written — both reproduced. |
| Verification | B | Every stated gate re-ran green here (build, vet, three test packages, `GOOS=windows` vet, golangci-lint 2.12.2, bats 6/6, all six `features.json` commands); AC6's evidence is prose and is not re-runnable. |
| Scope | B | The lane commit matches the proposal (plus `.dotf/` data and a CI path-filter row); the review window additionally carries a whole second spec's change, which is the launcher's base, not the author's creep. |
| Reliability | D | A brand-new public CLI surface fails **silently** on every error path (exit 1, no output), and a create/type split leaves the queue stalled with no retry. |
| Maintainability | C | Clean seams (`Runner`, `Backend`, `IssueSource`) and good WHY comments, but `Handoff` at 72 lines against the repo's 40-line law, and the layout rule duplicated instead of reused. |
| Handoff-readiness | C | Lessons captured (438 + the naming-rule lesson), promotions answered, `verification.md` filled — while `proposal.md`'s ACs and all `tasks.md` Closing boxes remain unticked, so the contract reads as unfinished. |

Aggregation: one **D** (Reliability) → FAIL by the mechanical rule; the severity path reaches the same verdict independently (one **REAL Blocker**, plus three REAL Majors).

### Verdict

**FAIL**

The mechanism is sound and the queue/parent/argv design does what the proposal says — I watched it decide correctly against the live forge and the live herdr. It fails the gate on the last metre: a user cannot see *why* a lane command refused (Blocker, reproduced four ways), AC5's no-backend promise does not hold for the plain invocation (reproduced), and the printed no-backend commands would create a worktree this same release's `dotf worktree done` cannot find under its branch (reproduced against `dotf worktree add`).

### Recommended next steps

Minimum set to flip to PASS. Items 1, 3 and 4 land outside the contract set (code, tests, `.dotf/lane-prompt.md`, `verification.md`), so they can be done without invalidating this verdict; item 2 may land in `proposal.md`, and item 6 is a contract edit by nature — either way, a re-review follows the FAIL, so do them together:

1. **Stop swallowing errors** — remove `SilenceErrors: true` from `newLaneExecCmd`/`newLaneNextCmd` (or print the returned error), and add a named test that asserts the refusal text reaches stderr, not just the returned `err`: `dotf lane exec --agent copilot` must print `no launch command for agent "copilot" (known: agy, claude, pi)`. Verification: re-run the four commands in finding 1 and see the messages.
2. **Close AC5 honestly** — either give the no-backend path a documented agent default so `dotf lane next` prints the commands and exits 0, or (contract edit, so pair it with the re-review) reword AC5 as "with `--agent`, or when the pane's agent is known". A test must cover the no-backend/no-agent case whichever way it goes; today none does.
3. **Reuse the layout rule** — build `manualCommands`' path from `env.ResolvePath("WORKTREE_ROOT")` and `worktree.ResolveWorktreePath(root, repo, branch)`, so `fix/x` prints `…/<root>/<repo>/fix-x` like `dotf worktree add` does, and name it with a test that pins the printed command against `TestResolveWorktreePath`'s rule.
4. **Close the release gap before the first real lane** — confirm `dotf lane next` resolves from the installed release once #2520 (0.71.0) is merged and the local binary refreshed, and record that in `verification.md`; or make the template tell the successor to use the binary it was started with. Until then AC6's chain is proven only with a dev build.
5. **Disposition the tracked ones** (each in `verification.md`, applied / ticketed / declined with a reason): the silent bare-`dotf` substitution (prefer a loud note in the outcome line), the Find→Create race, the create-then-type-fails stall, `Slug` collisions, `base` without a slash, and `Handoff`'s length.
6. **Tick the contract boxes** — `proposal.md`'s AC list and `tasks.md`'s Closing section are both stale. These are contract-set edits and therefore belong to the round *after* this verdict, not to a passing one.
7. Answer the two process questions: who set `passing` in `features.json`, and CLI-118's missing `review.md` (its own archive gate, inside this review window).

**Is `dotf spec archive CLI-119-lane-next` advisable now? No.** It would record a FAIL-signed review; run items 1–4, then re-review at the new sha (`dotf spec review CLI-119-lane-next`, which draws a fresh pool member — the fixes above touch no contract file, so the delta is small and the same reviewer set is valid).
Loading