Plan: the quality pass before others consume — decompose the entry closures, cover the last inch, hunt bugs (#131)

On this page

Status: Done (2026-09-14) — six units merged; two defects found and fixed (memory self-eviction, silent-stream hang); coverage bars are thresholds

Branch: one per unit, refactor/quality-<package> · Issue: #131 · Close step: the last unit’s MR carries Closes #131; after merge one line on the issue with the merge SHA, the before/after numbers, and the bugs found.

Design

What is wrong

Measured on main at 1d776c898 (vitest --coverage, v8; test/ and eval/ excluded; the scripts are one-off and not kept):

Package Lines Branches Where the gap is

pi-workflow

96.7 %

89.0 %

index.ts 87 % — the commit handler’s rarer branches

pi-vertex

95.7 %

92.7 %

adc-token.ts 67 % — the mint’s failure paths

pi-modes

89.2 %

79.0 %

judge-wiring.ts 53 %, memory-wiring.ts 59 %, index.ts 71 %, live-vertex.ts 0 %

pi-guidance

87.8 %

87.9 %

payload.ts 85 %

pi-agents

76.8 %

73.1 %

index.ts 203-230 — the factory’s seeding and override paths

model-battery (tools)

85.5 %

74.2 %

agent-loop.mjs 14 % (spawns pi), matchers.mjs 43 %, single-call.mjs 0 %

The uncovered code is not random: it is the last inch between pi’s API and our logic. The pure functions are tested to the high nineties; what is not is the code that pi calls — the judge’s authorizer callback adjudicate (judge-wiring.ts:264, never invoked by a test; the permissions:ready registration at 232–254 likewise), the remember and count tools' execute (memory-wiring.ts:181, index.ts:323), the entry renderers (index.ts:133–149), the model_select handler (index.ts:200), the agents factory’s seeding under a real agent dir. Those are exactly the places a wiring bug lives, and today the consumer simulation is the only thing that runs them.

The reason they are untested is shape. Thirty-eight functions exceed the TS profile’s forty lines; the top nine are entry closures — gadhsPiWorkflow 584 lines, registerSubagentTracking 473, createModeController 396, gadhsPiModes 342, the commit tool_call handler 306, registerPlanTools 301, registerJudge 255, registerMemory 188, authorizeAsk 161 — and a closure that size can only be exercised through the whole extension, which is why the files that hold them are the low-coverage files. Duplication is negligible (jscpd ≥ 60 tokens: extractText twice across the ADR-004 boundary, one 8-line clone inside the judge, three clones between tools/consumer-sim.mjs and tools/landstrip-probe.mjs). No export is dead; 97 are referenced only in their own file (mostly types, a few helpers carrying a superfluous export).

What changes

One method, applied package by package, in the order of risk carried:

for each entry closure:
  1. PIN     run the package's wiring suite; record coverage           (nothing moves yet)
  2. EXTRACT lift each handler body into a named function taking a deps object
             (the pattern authorize.ts / subagent-wiring.ts already use);
             the closure becomes a list of registrations that delegate
  3. PROVE   same suite green, same prompts/texts byte for byte where a test can pin them
  4. COVER   unit tests on the extracted functions that assert RISK: the error
             branch, the empty input, the concurrent second call - never a
             test whose only property is "was called"
  5. HUNT    adversarial tests on guards and parsers; property tests (fast-check)
             on every parser; a bug found is fixed HERE with its regression case

The bar. Per shipped package: lines ≥ 95 %, branches ≥ 90 %, on the package’s own files with test/ and eval/ excluded. No function with branching logic over forty lines; a composition root (the extension entry, a register* that only registers) may be longer but every statement in it delegates. Named exceptions, stated in testing.adoc: live-vertex.ts (under test-support/ today) moves under test/ and out of the measured set (it is a live-test helper, not shipped logic); agent-loop.mjs spawns pi and is covered by the battery’s own lanes, its pure parts extracted and tested. The bars become vitest coverage.thresholds per package so they hold after this pass — a red run, not a number on a page (decision: yes, at the achieved level rounded down to the whole percent).

What "assert risk" means here, concretely. For adjudicate: a fake permissions:ready service registers the callback; the test drives it with pi-permission-system’s positional (ask, context, log) shape and asserts the verdict returned, the stats moved, the trace written only for non-allow, the denial tracker built from the mode’s limits on the first call and kept after; a forwarded child ask in #125’s shape; a missing log argument. For the remember tool: the store unavailable, a rejected near-duplicate, a refresh, persistence called once. For the agents factory: an agent dir with a user’s own Explore.md (unmanaged — must not be overwritten), an unwritable dir, an override for an agent that does not exist. For adc-token: the mint rejecting, the refresh margin, a concurrent second get() during a mint (one mint, not two). For the commit handler’s rare branches: -a with an unreadable diff, a wrapped commit in a -c string reaching the pause, the disposition refusal’s log line.

Property tests, by parser, each with its invariant named:

Parser Invariant

bash-structure.ts splitUnits / analyze

never throws on any string; every unit’s tokens concatenate back inside the input; a quoted heredoc body is never a unit

git-guard.ts collectCommitFlags

never throws; -m/-F values round-trip; a --trailer Review-Response is always seen

review-findings.ts

strip(strip(m)) === strip(m); parse(render(responses)) round-trips; strip then parse finds nothing

review-context.ts trailer

a path list round-trips through the trailer; no path escapes the root

think-tags.ts

chunking invariance: any split of the same source text into deltas yields the same blocks and the same final message; join(split(x)) === x for the closed two-newline shape; never throws

judge.ts parseJudgeVerdict

never throws; a verdict is one of three or a parse failure, never a fourth

config.ts / workflow-config.ts

an arbitrary JSON object is refused or loaded, never partially applied

Bugs. The pass exists to find them. Each one found is fixed in the unit that found it, with the regression case, named in that commit’s message and listed on #131 at the close. A bug outside the unit’s package is an issue, not scope growth.

What does not change

Behaviour. Every extraction is pinned by the suite that exists before it moves, and a unit whose tests had to change to pass is a unit that changed behaviour — that is a finding, not a refactor. Prompts, block texts, log event names and shapes, config keys, the wire. No new dependency: fast-check and @vitest/coverage-v8 are in the workspace. No new gate beyond the coverage thresholds, which the operator asked for by asking for maximal coverage.

Risks, by failure shape

Silent. An extraction that changes behaviour the suite did not pin. The pin step records coverage and the suite result before the move; where a handler produces text a test can compare (prompts, block reasons, notices), a byte-equality test is added before the extraction and kept. The cold review reads the snapshot and is asked, per commit, whether the extraction is behaviour-preserving.

Fail-open. A coverage threshold set below the bar, or a file excluded to make a number. Thresholds are set from the measured result of the unit, per package, and the exclusions are the two named above and no others.

Scope. A pass like this invites redesign. The rule is EXTRACT, not redesign: names, deps objects, module boundaries — no new abstractions, no new options. An improvement noticed is an issue.

Time. Six units, each one to three commits; days, not hours. Each unit lands on its own branch and MR so a slow one does not hold the others.

Release. One release at the end (@gadhs/pi 0.40.0): pi-modes, pi-workflow, pi-vertex, pi-agents, pi-guidance each a patch unless a bug fix inside changed behaviour on the wire, then as the fix demands. The pack-based consumer-sim before the tag, the registry one after.

Scope

Order is risk: the judge and guards first.

  1. U1 — pi-modes, the judge and the gate (judge-wiring.ts, authorize.ts, denial-tracking.ts). Extract adjudicate, recordVerdict, traceVerdict, warmJudge, the permissions:ready registration into named functions over a JudgeDeps; authorizeAsk split by its numbered stages (gate, plan-draft, rules, high-consequence, writes, reads, threshold, judge) into functions of one stage each. Tests as above with a fake permission service; property test on parseJudgeVerdict; adversarial: an ask with value an object, a path with a NUL, a command 100 KB long. Bar: judge-wiring.ts ≥ 95 % lines.

  2. U2 — pi-workflow, the commit path (index.ts, git-guard.ts, reflection-review.ts, bash-structure.ts). The 306-line commit handler becomes commit-pause.ts (subject-names-paths, elsewhere, context, key, claim/disposition, review, conclude — one function each); gadhsPiWorkflow a registration list; runColdReview split into ask / converse / verdict / envelope; guardGitCommit, collectCommitFlags, commitTargetsElsewhere, splitUnits under forty. Property tests on the four parsers in the table. Bar: index.ts ≥ 95 %.

  3. U3 — pi-modes, the rest (index.ts, mode-controller.ts, subagent-wiring.ts, plan-tools.ts, memory-wiring.ts, count-tool.ts, config.ts validateModesConfig). Tools' execute handlers, renderers and the model_select handler as named functions driven by the fake pi the wiring suite already has; registerSubagentTracking into spawn gate / lifecycle tracking / spend; createModeController into apply / switch / restore / status; validateModesConfig by section. Property test on config loading. Bar: package ≥ 95 % lines, ≥ 90 % branches; live-vertex.ts under test/.

  4. U4 — pi-vertex (adc-token.ts, index.ts pumpWithAdcRetry, validateEntry, think-tags.ts). The mint’s failure and concurrency paths; the chunking-invariance property on the rewriter (fast-check over random cut points of a corpus of real MiniMax outputs from the store); validateEntry by field. Bar: adc-token.ts ≥ 95 %.

  5. U5 — pi-agents, pi-guidance (pi-agents/index.ts, payload.ts). The factory’s seeding under the dirty-dir cases above; the override file’s malformed shapes; the payload’s composition edges (an empty repo file, a profile the language map lacks). Bar: both ≥ 95 % lines.

  6. U6 — tools, thresholds, docs, release. matchers.mjs and single-call.mjs covered or their pure parts extracted; the consumer-sim / landstrip-probe clones into tools/lib/stage.mjs; superfluous export on in-file helpers dropped (the test-only exports stay: that pattern is deliberate); coverage.thresholds in every package’s vitest config at the achieved level; testing.adoc gains the bars, the exclusions, and how to measure; CHANGELOG; the release. The durable copy of this plan under docs/modules/ROOT/pages/plans/ with nav (first commit, before U1).

Out of scope, filed or dropped: the cross-package extractText duplicate (ADR-004 forbids the import; a @gadhs/pi-shared package is a decision, not a pass); agent-loop.mjs beyond its pure parts; redesigning any module’s API; the battery’s stores and cases; anything in tools/ that is not exercised by validate.

Edit this page · latest