test(cli): cover restore.ts HTTP branches and the repair-path purge notice

[thecodedrift]

Stack (root → tip):

• test(cli): cover PlatformBinaryResolution.source and asMatchMode's refusal #304
  • test(cli): fingerprint the reference shape, cover the wizard's reload banner #305
    • ➡️ test(cli): cover restore.ts HTTP branches and the repair-path purge notice #306 (you are here)
      • test(cli): cover the tested/failed/refused split on a mixed verify/test run #307

Summary

Closes part of #284: two named coverage gaps in the runtime-rule repair path.

1. packages/cli/src/api/restore.ts:96-118 (five HTTP branches, zero coverage).
Added a restoreRule's HTTP contract" describe block in repair-integration.test.ts that drives restoreRule directly against a bare http server (bypassing the built-CLI subprocess, since these are unit-shaped questions about one function's mapping from HTTP outcome to RestoreOutcome):

• HTTP 401 → unauthorized
• non-ok status (500) → unavailable / HTTP 500
• response.json() throwing on an unparsable body → unavailable / invalid response body
• a body without an array rules field → unavailable / response carried no `rules`
• fetch itself rejecting (network error) → unavailable / message containing network error

Also added, in the existing CLI-integration describe block:

• planRuntime's unauthorized branch (plan.ts) — reconcile itself returning 401, distinct from restore's 401 — asserted via the --json envelope's skipped field.
• planRuntime's materialize-failure catch — forced by replacing .taskless/.gitignore with a directory after fixture migration, so materializeRuntimeRules → addToGitignore → writeFile throws EISDIR. Asserts the run still exits 0 and reports the rule as skipped rather than failing.

2. PurgeIncompleteError's repair-path message (plan.ts's repairWithheldRules).
deliver.test.ts already proved the raw PurgeIncompleteError (message + .failures) in isolation. Added two integration tests that force a restore to write the blessed rule but leave a stale, unremovable file behind (a chmod 0o500 directory), and assert on the rendered notice plan.ts builds around the error — singular ("1 stale entry ... reads it") and plural ("2 stale entries ... reads them") — which the existing suite never exercised.

Mutation-check table

Every assertion was verified to fail against the corresponding regression, then the source was reverted (git diff was empty against main for all non-test files at the end — only packages/cli/test/repair-integration.test.ts changed).

Test	Mutation	Caught?
maps HTTP 401 to unauthorized	response.status === 401 → === 402	Yes — assertion failed, got unavailable/HTTP 401
maps a non-ok status to unavailable	!response.ok → false	Yes — assertion failed, got unavailable/response carried no \rules`` (fell through)
maps a body response.json() cannot parse to unavailable	removed the try/catch around response.json()	Yes — promise rejected with SyntaxError instead of resolving
maps a body without an array rules to unavailable	removed the Array.isArray(rules) guard	Yes — assertion failed, got {status: "ok", rules: undefined}
maps a fetch that throws to unavailable	removed the try/catch around fetch(...)	Yes — promise rejected (TypeError: fetch failed) instead of resolving
skips every runtime rule when reconcile's own authentication is rejected	outcome.status === "unauthorized" → === "nope-not-it"	Yes — assertion failed, output.skipped was undefined (run crashed to SCAN_FAILED instead)
skips every runtime rule when materialization fails	removed the try/catch around materializeRuntimeRules(...)	Yes — assertion failed, exitCode was 1 instead of 0
says "1 stale entry ... it"	swapped the singular/plural ternaries (length === 1 ? "entries" : "entry" etc.)	Yes — both the singular and plural tests failed, each reporting the other's wording
says " stale entries ... them"	(same mutation as above)	Yes

Notes / findings

No production defect found; all mutations behaved as documented and production code is unchanged (only the test file is diffed from main).

The issue's line numbers for plan.ts (129-135, 139-148, 170-181, 280-289) point a little differently in current main (the same branches now sit around 165-176, 182-192, and 329-341) — no discrepancy in behavior, just line drift since the issue was filed.

Test plan

• NODE_OPTIONS= pnpm test — 1334 tests pass (up from 1328)
• NODE_OPTIONS= pnpm typecheck — clean
• NODE_OPTIONS= pnpm lint — clean
• Mutation-checked every new assertion (see table above)

No changeset: tests only, no user-visible behavior change.

Refs #284

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 47s —— View job

---

Review complete

• Read .prior-review.json (no prior threads/reviews/comments found — this is the first review)
• Fetched PR diff (gh pr diff 306) and metadata
• Reviewed the new/changed test code in packages/cli/test/repair-integration.test.ts, plus the production code it exercises (src/api/restore.ts, src/rules/runtime/plan.ts, src/rules/deliver.ts)
• Posted findings

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no reviewThreads, reviews, or comments beyond the trigger comment itself, so there is nothing previously raised to carry forward — this is a full first-pass review of the whole diff.

---

Overall this is solid coverage work: the mutation-check table in the PR body is a real strength, and I traced each new assertion against restore.ts and plan.ts's repairWithheldRules — the HTTP-branch mapping and the singular/plural notice wording both match the production code exactly (plan.ts:329-337 reproduced verbatim by the two new tests). Only production-code diff is none; this is test-only, as claimed.

One real issue found:

[New] Missing root-skip guard on the two new permission-based purge-failure tests

File: packages/cli/test/repair-integration.test.ts, lines 486-533 (says '1 stale entry ... it'...) and 535-585 (says '<n> stale entries ... them'...)

Both new tests force writeRuleFile's purge to fail by chmod-ing a stray subdirectory to 0o500 so the child file can't be unlinked, then assert on the PurgeIncompleteError notice wording. This is the same failure-injection technique already used in deliver.test.ts:562 and vale-verify.test.ts:177 for the identical scenario (a directory whose mode bits block removal) — but both of those existing call sites guard the whole describe block against running as root:

// deliver.test.ts:543-546
// Skipped as root, which ignores the permission bits the failure is staged
// with. The same convention as `vale-verify.test.ts`.
const asUser = process.getuid?.() === 0 ? describe.skip : describe;

// vale-verify.test.ts:151-152
// Root ignores the mode bits, so there is no unreadable directory to make.
const asUser = process.getuid?.() === 0 ? describe.skip : describe;

The two new tests in this PR use the exact same chmod(0o500) mechanism but have no such guard. Under a root-run environment, rm/unlink ignores the permission bits, the purge in purgeUndeliveredFiles (deliver.ts:492-556) would succeed instead of throwing PurgeIncompleteError, and both tests would then fail deterministically: notices would contain the "was restored: its rule directory now holds exactly the files..." message instead of "...but 1 stale entry could not be removed...", and toContain("blocked/a.txt") / toContain("blocked-one/a.txt") would also fail since that text never appears in the alternate message.

This repo's GitHub Actions CI (ubuntu-latest, no container: override) runs as a non-root user today, so this won't currently break CI — but it's a real gap against an established, repo-wide convention for exactly this failure-injection pattern (also see vale-orchestration.test.ts:28, which additionally skips on win32), and it means these two tests are silently reliant on an environment property none of their neighbors assume. Worth adding the same asUser/describe.skip (or it.skip) guard before these two tests, consistent with deliver.test.ts.

Minor / non-blocking

• packages/cli/test/repair-integration.test.ts:216-268 ("skips every runtime rule when materialization fails...") re-implements runCli's try/catch-around-execFileAsync exec/exit-code handling inline rather than reusing it, because it needs migrateFixture to run before corrupting .taskless/.gitignore rather than as part of the call (well explained in the comment). A small, optional simplification would be to give runCli an option to skip its internal migrateFixture call (e.g. runCli(args, env, { skipMigration: true })) so this test doesn't duplicate the subprocess/exit-code handling logic. Not a correctness issue, just a bit of avoidable duplication.

No other correctness, security, or test-adequacy issues found. The HTTP-contract tests for restoreRule (401 / non-ok / unparsable body / missing rules / network error) each map 1:1 to a branch in restore.ts:69-100, and the TASKLESS_API_URL env mutation in the new describe block is properly scoped via beforeEach/afterEach and doesn't leak into the sibling describe's subprocess-based tests (those always pass TASKLESS_API_URL explicitly per-call).
· branch test/restore-and-repair-coverage

[thecodedrift]

Clean review, thanks. The HTTP fixture binds an ephemeral port on loopback (listen(0, "127.0.0.1")) with an awaited close per test, which was the specific thing I checked before accepting it, given #262 tracks flaky subprocess tests here.

— AI Coding Agent