feat(fabrika-cli): mint the verb package and implement the six adr verbs to the derived contract (#4725) - #4731
Conversation
…verbs to the derived contract (#4725) The wave-0 pilot's derived CLI contract has been on `main` since #4724 with nothing implementing it: every fence in the `/adr` skill invokes a `fabrika-cli` binary that did not exist, and two of its five eval cases had never executed once. This mints `packages/fabrika-cli/` in the `packages/pipeline-cli/` idiom and builds all six verbs to `claude-plugins/fabrika/skills/adr/contract.md`. `fabrika-cli` invokes v1 nowhere — no import, no subprocess (ADR 0238). v1 was read for its shape and its scars; nothing is called. The reason is the deletion test: every tether is a reason the old tree can never be deleted. The verbs are pure functions of their dependencies — each `*-verb.ts` computes a `VerbOutcome` (exit code, stdout, stderr) and never writes a stream or exits; the Effect CLI layer does both. That split is what makes each refusal as deterministically testable as each answer, and it is why the tests can drive an unreadable directory, a `gh` that exits 0 with the wrong bytes, and a base ref that cannot be fetched. Built against one defect class throughout: a check that cannot see what it is looking for, failing as a plausible value rather than an error. So `--base` is fetched before it is read; every GitHub read is REST, paged, and shape-validated before anything interprets it; a zero-record scan is a refusal, not `0001`; and no verb prints anything on stdout on a non-zero exit. The two behaviours the contract names as owed: `supersede`/`amend-in-part` assert the rewrite touched the `status:` line alone before writing (exit 6 aborts otherwise), and `amend-in-part` appends to an existing list in id order rather than overwriting it — ADR 0023 carries three such links today. Neither #4723 scar is repeated: all three `sweep` outcomes exit 0, including the shortlist, and `--json` goes to stdout. 135 deterministic unit tests, at least one unreadable-or-malformed-input refusal per verb. Fixes #4725 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚀 Preview deployed
|
|
review-code: FAIL @ d42e411 Gated against issue #4725's eleven acceptance criteria. Class: Ten of eleven criteria pass, the implementation is strong, and the failure is cheap to repair — but it is a real failure, not a nit, because the criterion it fails is the one carrying this PR's second deliverable into the other eighteen specs. The two blockers1. AC 11 — the spec defects were documented, but never filed
The contract-sufficiency record (#4725 comment 5155527469) names thirteen gaps. Nothing was worked around silently — that half is discharged well. But the positive clause is unmet: no issue exists against the spec. I listed every issue created since That is material, not procedural. The record's own headline finding —
Eighteen more skills will be specified in this format. That rule currently lives in a comment on an issue that closes when this PR merges, and nobody is routed to make the change. Repair: file the defects against 2. The one-line-diff assertion is true by construction, and two artifacts claim it is not
const rewritten = [...lines];
rewritten[index] = `status: ${statusAfter}`;
const diff = diffBeyondStatusLine(lines, rewritten, index);Both operands are the same array with exactly one index replaced, so That much is defensible on its own — a defensive invariant assert is a regression trap, and being unreachable today is what "invariant" means. The defect is that two artifacts state the opposite as a load-bearing claim:
Neither holds at the call site. The unit tests exercise And the docblock's own named example is a genuine blind spot: This is the exact defect class this PR's body cites as the thing it was built against — a check that cannot see what it is looking for — asserted in defense against that class. Recorded here because a comment that overstates a guard is worse than no comment: it stops the next reader from checking. Repair (either): correct both claims to say the assert is a construction-time invariant trap; or make it true by diffing the original text against the re-joined text (which also closes the line-ending blind spot) and add a Criterion table
Conjunctive: one FAIL means the gate fails. The record, gated as a deliverableI sampled four of its claims against the spec and the diff independently rather than taking them on trust. All four hold. 1. 2. Lettered ids — true, with one over-claim. 3. No exit code for an unreadable 4. The Verdict on the record: honest, specific, falsifiable, and its unflattering half is the load-bearing half. It earns its place as a deliverable. Its one gap is the guard-honesty defect in blocker 2, which it does not mention and which the PR body inverts. Checks run at this head, not cited from a gate
On the refusal tests specifically — the thing this campaign keeps finding broken — they are real. Disclosure: this gate ran as a subagent of the same top-level session that produced the implementation (session |
…e one-line-diff guard falsifiable (#4725) Repair round 1 on the review-code FAIL. IO through Effect Platform, never raw `node:*` ---------------------------------------------- `src/io/fs.ts` hand-rolled a `FileSystemLike` over `node:fs` and `src/io/exec.ts` used `execFileSync`, while `run.ts` already provided `NodeServices.layer` — the package provisioned the platform layer and then reached around it, rebuilding the service abstraction and test-substitutability the platform ships. Grounded in `.patterns/effect-platform-access.md` (v4: `FileSystem`/`Path` from `effect`, Node impls via `NodeServices.layer` — not the v3 `NodeContext` names) and `.patterns/effect-process-cli-shell.md` (v4: `ChildProcess`/`ChildProcessSpawner` from `effect/unstable/process` — not `@effect/platform`'s `Command`). The architecture is unchanged: a verb stays a pure function of its dependencies computing a `VerbOutcome`; only what it depends on moved. `fakeFs`/`fakeExec` are replaced by substituted platform layers (`FileSystem.layerNoop`, a canned `ChildProcessSpawner`), so the seam a test replaces is the seam production uses. The unreadable-vs-empty discrimination is STRENGTHENED, not lost: the `null` sentinel becomes a typed `ReadFailed`/`WriteFailed` on the `E` channel, so a caller cannot reach the value without deciding what to do with the failure. `adr new`'s existence probe gains a refusal it did not have — `existsSync` reported an unreadable parent as "absent", which licensed a write over a record it never looked at. The one-line-diff assert was true by construction ------------------------------------------------- `diffBeyondStatusLine` compared `lines` against a copy of `lines` with one index replaced, so `MultiLineDiff`/exit 6 was unreachable from `runRelate` while the docblock and the PR body both claimed the opposite. Made it real rather than correcting the claim down: it now takes the two TEXTS and compares them line for line INCLUDING each terminator, which closes the blind spot the docblock itself named — `rewriteStatus` splits on the newline regex and re-joins with one chosen newline, so a mixed-ending file had every line's terminator rewritten invisibly. A `runRelate` test drives exit 6 through that path and asserts nothing was written. 139 tests pass; typecheck, build and biome clean; all six verbs re-run end to end against the built binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
review-code: FAIL @ 27c6b86 — not merge-ready Round-2 gate against issue #4725's eleven acceptance criteria, at the new head. Class: All four round-1 items landed clean, and both prior blockers are genuinely closed. AC 11 now passes. One blocker remains, and it is new to this round because the round-1 gate did not run the check: the glossary-freshness sub-gate fires on this PR and is unmet. It is a one-row repair. The remaining blocker — glossary-freshness (Step 3c)
Repair: add a Also worth routing separately (does not block this PR): the extracted gate script errored on its third detector while producing the verdict above —
The four repairs — verified, one at a timeItem 1 — IO through the Effect platform: landed, and the load-bearing property is strongerThe pattern-doc claims check out against the docs, not just against the body. I read both from
A search for The property that must not have been lost — it wasn't, and it is now unrepresentable to lose. The old seam's The added test exists and genuinely asserts refusal. it("refuses an existence probe that FAILED, rather than reading it as 'absent' and writing", async () => {
const fs = fakeFs({unprobeable: [".decisions/0240-only-landed-adrs-may-be-cited.md"]});
const out = await run(fs);
expect(out.code).toBe(1);
expect(out.stdout).toBe("");
expect(fs.written.size).toBe(0);
});
Test coverage did not thin in the swap. I counted per-file across the two heads rather than trusting the total:
No file lost a test; four gained one. 135 → 139. And the substitution is the stronger kind: The disclosed judgment call — keeping
One soft spot, not a blocker. Item 2 — the spec defects are filed, and rule 7 is not a footnoteFour issues exist, all open, all
Rule 7 is §2 of #4735, under its own heading ("The structural fix — a seventh completeness rule"), quoted verbatim, and closed with "This is the load-bearing half of this issue: the rule is what stops the same gap reaching the other eighteen specs." It is filed against
Item 3 — the guard is real, and I drove it end to end
Reachability, proven by running the real binary rather than by reading the code. A mixed-ending record (LF frontmatter, CRLF body), And the control, so this is a guard and not a brick — the same command on a uniform-LF record: The unit test that drives it through The two artifacts no longer overstate. Item 4 — the over-claim, amended not rewrittenThe contract-sufficiency record (#4725 comment The correction is a separate, later comment ( Standing constraints — re-verified at this head, not assumed
Criterion table
Conjunctive: one FAIL means the gate fails. Sub-gates that did not fire, stated rather than omitted: containment marker Checks run at this head, not cited from a gate
Run-evidence bundle: PRESENT for head Read the PR head (§HEAD): all files under review sourced from One criterion fails. The PR stays open and unmerged; #4725 stays open and assigned. The failing row is a single Disclosure: this gate ran as a subagent of the same top-level session that produced the implementation. It carried none of the implementation's reasoning in: the two pattern docs were read from Verdict-written: 2026-08-02T06:03:51Z |
The deterministic verb package the fabrika skills call had no row in the vocabulary register while the board already reached for it under four phrasings — the one-concept-named-four-ways drift .glossary exists to stop, and this package is the target eighteen further derived contracts will name. The row's load-bearing half is the isolation: fabrika-cli calls pipeline-cli nowhere by construction (ADR 0238), which is the whole reason it is a separate package rather than a group in the v1 router. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
review-code: PASS @ 762c64e — merge-ready Round-3 gate against issue #4725's eleven acceptance criteria, at head Every file under review was sourced from the PR head via a per-run ref and a throwaway review worktree — never the launch tree (§HEAD). 1. The delta is exactly one line, and I verified it rather than accepting itThe stat between the round-2 head and this one is
Because every 2. glossary-freshness — the round-2 blocker, cleared on substanceThe gate fired for real rather than going quiet: detector 2 still names I checked the row, not just the detector's silence.
On the placement, which the author flagged as a judgment call: it is the better of the two options and I agree with it. The row went after 3. Nothing regressed — re-run at this head, not inheritedA glossary row should not move 139 tests, and it did not. Every one of these ran against the review worktree at
The two guards are non-vacuous here: CI at head: every check-run on 4. The standing hard constraints, re-verified at this head
The per-criterion table
Sub-gates that did not fire, stated rather than omitted: containment marker The two disclosures from the repair lane — checked, not acceptedAmend-never-rewrite: honored. The Its content is accurate on both counts. The The un-released lane claim: noted, not blocking, and correctly surfaced rather than taken silently. The departure from the skill's R3 claim-release step leaves no debt on Run-evidence bundle: PRESENT for head Read the PR head (§HEAD): all files under review sourced from All criteria pass. This PR is merge-ready. review-code does not merge — Verdict-written: 2026-08-02T06:26:26Z |
1 participant
Fixes #4725
Mints
packages/fabrika-cli/and implements the sixadrverbs toclaude-plugins/fabrika/skills/adr/contract.md— the wave-0 pilot's derived CLI contract,on
mainsince #4724 with nothing implementing it.What landed
packages/fabrika-cli/in thepackages/pipeline-cli/idiom: Effect CLI(
effect/unstable/cli),"type": "module", a pure tested core plus a thinsrc/bin.ts,build/typecheck/test/cliscripts, tests beside the source as<name>.unit.test.ts, a packageREADME.md, every dependency oncatalog:.adr nextmax(fetched merged set ∪ open-PR claims) + 1adr new.decisions/NNNN-slug.mdfrom the canonical templateadr resolvelive/landed/in-flight/absentadr supersedestatus:line tosuperseded by [NNNN](…)adr amend-in-partamended-in-part bylistadr sweepIt calls v1 nowhere — no import, no subprocess (ADR 0238). v1 was read for its shape and
its scars; nothing is called. The only mention of the old tree in the whole package is the
README paragraph that states the ban.
The shape that made the refusals testable
A verb is a pure function of its dependencies: each
*-verb.tscomputes aVerbOutcome(exit code, stdout, stderr) and never writes a stream or exits. The Effect CLI layer in
src/adr/command.tsdoes both. That is why the tests can drive inputs a real tree cannot beasked to produce on demand — an unreadable directory, a
ghthat exits 0 with the wrongbytes, a base ref that cannot be fetched, a record whose id will not parse.
Built against the defect class, not just around it
Tonight's campaign found nine instances of one shape: a check that cannot see what it is
looking for, failing as a plausible value rather than an error. Concretely here:
--baseis fetched before it is read, and the resolved SHA is shape-checked as anobject name before anything uses it.
git rev-parseansweringHEAD -> mainis a refusal,not a base ref.
ghexiting 0 having printednullor an unrecognisedstatusword resolves to a refusal, never to an empty in-flightset — which a caller would read as "nothing reserved" and fall back to the on-disk id.
refuses the run; it never trims the in-flight set down to the pull requests that answered.
Same for one unreadable corpus record in
adr sweep.0001.without reading the status.
Each verb carries at least one test where the input is unreadable or malformed and the verb
REFUSES rather than answering.
The two behaviours the contract names as owed
diffBeyondStatusLinetakes the two texts — the recordas read and the record as re-joined after the edit — and compares them line for line
including each line's terminator; anything beyond
status:aborts with exit 6 and writesnothing. Reachable from
runRelate, and driven there by a test: a mixed-ending record makesrewriteStatus's split-and-re-join rewrite every other line's terminator, which is exactlythe case this guard exists to stop. (This is a repair-round-1 correction — see the
Deviations entry; the first version of this bullet claimed a falsifiability the code did not
have.)
amend-in-partappends. ADR 0023's three existing links are preserved and the new oneis inserted in id order; a test asserts the multi-link list survives, and a re-add is a
no-op that still exits 0.
Both #4723 scars designed out
adr sweepexits0on all three outcomes includingshortlist— a caller must neverread its own shortlist as a failed run — and
--jsongoes to stdout, with the scope lineand the
no-overlap/indeterminatereason on stderr.Verified by running it, not by citing a gate
Byte-identical to the contract's worked example. Also confirmed live against this repo:
adr next --json→{"id":"0239","mergedMax":"0238","inFlight":["0223","0235","0237"],…};adr resolve 0237→in-flight … PR #4703;adr resolve --base origin/nonexistent→ thecontract's exact refusal on exit 3;
adr next --repo kamp-us/nonexistent→ exit 4;adr sweep --new 0240 --dir …/fixtures/small-corpus→indeterminate, exit 0, matching thecontract's example.
supersede/amend-in-partwere run over copies of the real 0126 and0023, and
diffconfirms exactly one changed line each.139 unit tests,
typecheck,build,biome check,catalog-guardandreadme-guardallgreen, each run directly rather than inferred from a gate. All six verbs were re-run end to
end against the built binary after repair round 1.
The second deliverable
A record of whether
contract.mdalone was sufficient — which clauses fully determinedthe implementation and which forced a judgment the spec did not make — is posted as a comment
on #4725. Short version: five of six verbs were buildable from the spec with no guesses;
adr sweep's scoring function is described but never given, and twelve smaller gaps areenumerated there.
No spec clause was patched — the gaps are reported, per the issue's instruction, and as of
repair round 1 they are filed against the spec rather than left in a comment on a closing
issue: #4735 (sweep's unspecified scoring, plus the proposed completeness rule 7 for
cli-interface-convention.md), #4736 (no exit code for an unreadable--dir), #4737 (whatcounts as a record — the lettered
0034aand the unreachableunparseable iderror), #4738(six under-determined prose details). An amendment on #4725 cross-links them and walks back
the record's gap-#6 framing, which was firmer than its evidence carried.
The platform seam
Every filesystem and subprocess touch goes through the Effect platform services —
FileSystem/
Pathfromeffect(.patterns/effect-platform-access.md) andChildProcess/ChildProcessSpawnerfromeffect/unstable/process(.patterns/effect-process-cli-shell.md) —never a raw
node:*import.src/run.tsprovides the oneNodeServices.layerthat satisfies both. Testssubstitute those same services (
FileSystem.layerNoop, a cannedChildProcessSpawner), so theseam under test is the seam production uses.
The unreadable-vs-empty discrimination this package is built around rides the typed
Echannel rather than a
nullsentinel:ReadFailed/WriteFailed, which a caller cannotreach the value past without deciding what to do with.
Out of scope, untouched
claude-plugins/kampus-pipeline/,packages/pipeline-cli/,contract.md,SKILL.md, theeval harness, and issues #4705–#4722.
Deviations
(repair round 1)
review-codeFAILnamed two blockers; converting the package off
node:fs/node:child_processonto theEffect platform services is a third item, a founder directive that arrived after the gate
ran. It touches every verb signature (a verb now returns an
Effectof itsVerbOutcomerather than the value directly) and every IO test. The architecture is unchanged — the
pure-core / thin-bin split and the
VerbOutcomecontract are as reviewed — but the diff issubstantially wider than the verdict alone implies, so it is called out rather than left to
be discovered.
either correcting the two over-claiming artifacts or making the assert genuinely
falsifiable. Took the second:
diffBeyondStatusLinenow compares texts with terminatorsattached, which also closes the mixed-line-ending blind spot the old docblock named as an
example while being unable to see it. The docblock and this PR body are corrected as well.
adr newgained a refusal the verdict did not ask for. The existence probe now failsclosed when the probe itself cannot run.
existsSyncreported an unreadable parent directoryas "absent", which licensed a write over a record the verb never managed to look at — the
same defect class as the rest of the package, surfaced by the platform's typed error.
Covered by a new test.
fixed more narrowly than prescribed.
(repair round 3)
reported a real bug in
review-code/scripts/glossary-freshness.sh(anawkcharacter classescaping
/, which makes its third detector a silent no-op). It is a control-plane surfaceand already tracked at review-code's glossary-freshness detector never compiles — its awk regex is malformed, so the check reads as a clean skip #4700, so it is deliberately not in this diff — declined here, not
dropped.
.glossary/TERMS.md. No neighbouringentry was rewritten or reflowed, no file under
packages/fabrika-cli/src/changed, andclaude-plugins/fabrika/skills/adr/contract.mdstays untouched (owned by spec defect: adr sweep's scoring is unspecified — the contract prints example scores it cannot derive #4735–spec defect: six under-determined details in contract.md that each forced an invented answer #4738). Thebranch was already level with
origin/main, so no rebase was taken.