Bring effects/node/virtual to 100% coverage: 89.12% → 100% branch - #1559

Reviewed at 8c3a9287f6ef9ca30580fc59daccd0753d658708, baseline origin/main = cb1fcdc457ecb5ba3a20d994e4ba4e0c31d9577e (the PR's merge-base is that same commit, so no rebase skew).

The headline claim measured

npm run cov in both trees, reading the effects/node/virtual/module.f.mjs row:

So "89.12% → 100% branch" reproduces exactly. Repo-wide branch coverage moves 99.04 → 99.48. npx tsc --noEmit 0, npm run prepack 0 from a clean tree, npm test 2707 pass / 0 fail (main: 2691 under the same runner). linkcheck broken-link sets are byte-identical to main. No public-surface change (no types.ts touched, no new export type — so no §6.2 question), and changelog/unreleased/1559.md is a correct per-entry file linking only /pull/1559.

The deleted / asserted branches really are dead — including on main

Four guards are removed or converted to assert. I instrumented each of them on origin/main, where they still exist, replacing the guard body with throw new Error('DEAD-…'), and ran the full suite:

• readFile's !Array.isArray(file)
• readBytesOp's !Array.isArray(file)
• rmOp's !Array.isArray(entry) && typeof entry === 'object'
• insertEntityAt's path.length === 0

Result: 2691 pass / 0 fail — none of the four is reached. Negative control: making readFile's throw unconditional gives 24 failures, so the instrumentation is live. (First attempt also hit the identical line in statOp, which failed immediately via statOnJsModule — that one is genuinely reachable and correctly left alone in this PR. Good check on the method.)

The reasoning behind each also holds independently of the suite: operation's f calls op only when path is empty or when dir[path[0]] is not a plain non-array object, so after the function/undefined cases are handled, the remaining entity must be an array. For insertEntityAt I confirmed the specific premise: parse('') === [] and isProperPrefix([], ['a']) === true, so rename(src, '') with non-empty src is rejected as "onto an ancestor" first, and rename('', …) dies in extractEntity as "cannot extract root". The recursive self-calls only fire for path.length > 1. So this is not "100% by deleting live branches" — and unlike #1540 the deadness is not an artifact of something else this PR tightened, since it reproduces on unmodified main.

Mutation testing the new cases, two-sided

15 mutations, each applied to the PR head and then identically to origin/main, scored against the effects/node/virtual proof:

Killed on PR, survive on main (i.e. the new cases earn their keep): readdir's content === undefined skip; extractEntity's "cannot extract root"; insertEntityAt's 'not a directory' tag; writeBytesRawOp's "not a file"; statOp's missing-file enoent; readBytesOp's JsModule throw.

Equivalent mutants, correctly not a defect: descending into an array in extractEntity, and dropping either recursion's if (result[0] === 'error') re-wrap — in all three the produced value is structurally identical, and rename discards the returned dir on the error path anyway. Same for insertEntityAt's sub === undefined returning a clobbered dir: rename throws dstRoot away when dstResult is an error, so it is unobservable. These are unkillable by construction, not weak assertions.

One real weakness — the #1528 shape again, in this same module

Two of the new cases sit in front of ops that are wrapped by operation, whose result dir is threaded straight back into state.root. There, a wrong dir is observable, and the cases do not look at it:

createExclusiveNestedMissing: () => {
    const [, result] = virtual(emptyState)(createExclusive('a/b'))
    assert(result[0] === 'error')
},
writeBytesMissingFile: () => {
    const [, result] = virtual(emptyState)(writeBytes('missing', 0, vec8(0x1n)))
    assert(result[0] === 'error')
},

I mutated the two lines they newly cover to return the correct error tag but a wiped directory:

• createExclusiveOp: if (path.length !== 1) { return [dir, invalidPath] } → return [{}, invalidPath]
• writeBytesRawOp: if (file === undefined) { return [dir, enoent] } → return [{}, enoent]

Both are observable through the public entry point — with a populated root, createExclusive('a/b') and writeBytes('missing', …) now return state.root === {}, silently destroying the whole virtual filesystem while still reporting 'error'. With both mutations applied at once, the full suite is 2707 pass / 0 fail. Nothing catches it.

The cause is that both cases start from emptyState (root {}, so a wipe is invisible) and discard the returned state with const [, result] =. This is the same weakness found on #1525/#1528 — and #1528's instance was in this very file. It is distinguishable from the #1535 case, which was fine because proof.throw asserts only that a throw happens; here the case does return a state and simply declines to look at it.

Suggested fix: give these two a non-empty root and assert it survives, e.g.

const root = { keep: [vec8(0x1n)] }
const [state, result] = virtual({ ...emptyState, root })(createExclusive('a/b'))
assert(result[0] === 'error')
assertEq(Object.keys(state.root).length, 1)

A smaller version of the same thing: writeBytesNegativeOffset only asserts result[0] === 'error', and weakening the guard from !Number.isInteger(offset) || offset < 0 to !Number.isInteger(offset) still survives — -1 !== fileSizeBytes(chunks) produces a different error further down. Pinning result[1] would make the case actually about the negative-offset branch.

Everything else here is solid work, and the coverage claim is exact — but since the whole point of the PR is proof strength, the two lines that reach 100% while remaining insensitive to a filesystem-wiping mutant seem worth closing before merge.

Approving. Both findings from the previous round are fixed, and I killed the mutants myself rather than taking the new assertions on faith.

Baseline: origin/main at cb1fcdc4.

The state-clobbering weakness is closed

The previous review's concern was that createExclusiveNestedMissing / writeBytesMissingFile started from emptyState and asserted only result[0] === 'error', so a mutant that returned the right error tag while wiping state.root survived the whole suite. The three cases now start from root = { keep: [vec8(0x1n)] } and assert Object.keys(state.root).length === 1.

I constructed the clobbering mutants against module.f.mjs at the head and ran the full suite (node ./fjs/module.mjs t) for each. Baseline: 2707 pass / 0 fail.

Each mutant is caught by exactly the case that was supposed to catch it, and by no other — so the assertions are load-bearing and precisely aimed.

The offset < 0 guard is now covered

writeBytesNegativeOffset gained assertEq(result[1], 'Offset -1 is invalid'). Removing || offset < 0 from

if (!Number.isInteger(offset) || offset < 0) { return [dir, error(`Offset ${offset} is invalid`)] }

lets -1 fall through to the append-only check and produce a different message; the suite now goes to 2706 pass / 1 fail on writeBytesNegativeOffset. Previously this mutation survived. Killed.

Re-verified at this head

• npx tsc --noEmit → exit 0.
• node ./fjs/module.mjs t → 2707 pass, 0 fail.
• Coverage re-measured (the proof changed, so I did not carry the old number forward): fjs/effects/node/virtual/module.f.mjs → 100.00 line / 100.00 branch / 100.00 func.
• Broken-link sets identical to cb1fcdc4 (diff of the two linkcheck runs is empty), so the three new todo/*.md files strand nothing.
• changelog/unreleased/1559.md is a per-entry file carrying its own [#1559](…/pull/1559) link and no heading. The todo/ additions correctly get no entry (§8.3).

Carried forward from the 8c3a9287 review

fjs/effects/node/virtual/module.f.mjs is byte-identical to what I verified there, so those results stand unchanged: the four removed/asserted guards were proven dead on origin/main itself by throw instrumentation (2691 pass / 0 fail, with a negative control giving 24 failures), and of 15 two-sided mutations 6 were killed on the PR while surviving on main — the new cases earn their keep — with the 3 survivors provably equivalent mutants.

The last three commits add todo/commit-message-standard.md, todo/commit-message-enforcement.md and a cross-reference in todo/changelog-from-git-history.md. They are docs-only and touch no code, but they are also unrelated to this PR's subject; worth splitting out if you want the coverage change to stand alone in the history. Not a blocker.