Add test case for writeFile over JsModule entries - #1528
Conversation
Covers the `file !== undefined && !Array.isArray(file)` branch of writeFileOp, which was never exercised: writing to a path currently occupied by a JsModule (function) entry rather than a file or directory.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 8104c49 | Commit Preview URL Branch Preview URL |
Aug 13 2026, 06:38 PM |
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. Draft status is not a blocker. Proof-only, and the case earns its place.
Gates at d7188d9d (base 283db73a)
npx tsc --noEmit 0 · npm run prepack 0 from a clean tree · npm test
2533 pass / 0 fail against 2532 on origin/main, i.e. exactly the one new case.
Dual-axis surface diff (bin/extract.mjs 46,202 bytes, bin/consts.mjs 64,198 bytes,
non-empty on both sides) is byte-identical to main; the only declaration that moves
anywhere in the tree is virtual/proof.f.d.mts gaining
writeFileOverJsModule: () => void;. Broken-link set identical to main. No CHANGELOG
entry, which is right for a proof-only change under §8.3.
The case is not redundant — I checked rather than assumed
My first reaction was that writeFileOnDirectory already puts a non-undefined,
non-array value at the path, so it must already cover
if (file !== undefined && !Array.isArray(file)). It does not, and the reason is the
operation wrapper: for a directory entry it descends (typeof subDir === 'object' && !Array.isArray(subDir)) and calls writeFileOp(inner, []), which exits at the earlier
path.length !== 1 guard. A JsModule is a function, so the wrapper stops and
writeFileOp is entered with path.length === 1.
Made observable by temporarily giving the two guards distinct error values:
x is a Dir (existing case) -> ["error","L121-path-length"]
x is a JsModule (new case) -> ["error","L124-not-an-array"]
x absent (control) -> ["ok",null]
x a regular file (control) -> ["ok",null]
So line 124's true branch really was unreachable by the existing suite, and the comment
in the new case is accurate.
Mutation test
Delete the guard outright (if (file !== undefined && !Array.isArray(file)) { … }):
- On this branch:
proof.writeFileOverJsModulefails (assertion failed); full suite
2532 pass / 1 fail / 2533 total. The new case is the only thing that dies. - The same mutation on
origin/mainkills nothing:tsc --noEmit0 and
npm test2532 pass / 0 fail, unchanged.
That is the property this kind of PR has to have, and it has it.
Real coverage delta from npm run cov
npm run cov was not vacuous here — 2469 real tests ran (2468 on main).
origin/main |
this PR | |
|---|---|---|
fjs/effects/node/virtual/module.f.mjs line / branch / func |
100.00 / 88.60 / 100.00 | 100.00 / 89.12 / 100.00 |
| repo total line / branch / func | 99.94 / 98.20 / 99.73 | 99.94 / 98.22 / 99.73 |
Branch-only movement, as expected for a case that reaches an already-executed line by a
new path.
Weaknesses I probed for, and one that is present
Checked against the two failure modes from earlier reviews in this series:
- #1518 (pins an error message, so a branch change that preserves the message
survives). Not present, and deliberately so: replacingwriteFileErroron that line
with a completely different error value leaves the case passing, because it asserts
result[0] === 'error'rather than the text. - #1525 (pins only one field, so a neighbouring change survives). Present. Mutate
the guard toreturn [{ ...dir, [name]: [payload] }, writeFileError]— report the error
but clobber the entry anyway — and the entire suite still passes: 2533 / 2533. The
case destructuresconst [, result]and throws the state away, so "rejected the write"
and "rejected the write and left the JsModule intact" are indistinguishable. One extra
assertion on the returned state (the entry at'a.f.ts'is still the function) would
close it, and would be strictly stronger than what the sibling error-path cases do
today.
Not a regression and not a reason to hold the PR — the surrounding cases share the shape —
but since the whole point of the case is to fence an error path, fencing the state it
leaves behind is the natural other half.
For completeness, one mutation I expected to survive did not: weakening the guard to
if (file !== undefined), which would reject overwriting an ordinary file, is caught by
the suite already (2532 / 1 fail). So that neighbour is fenced elsewhere.
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Re-review at 04486368 (previously approved at d7188d9d, now out of draft). Baseline: origin/main = 893b4eeb1d1562347770b8ee40ce6b30e787f195, confirmed equal to the remote main tip.
The delta since the last review is a merge of main and nothing else. 04486368 is a merge commit whose parents are d7188d9d and d986e0e0 (#1526). Merge base with origin/main is d986e0e0, and git diff d986e0e0 04486368 is one file, fjs/effects/node/virtual/proof.f.mjs, +9/-0 — byte-identical to what was reviewed and approved. No source file, no CHANGELOG, no docs touched by the branch itself. The branch is a few commits behind origin/main (bigfloat work landed after d986e0e0); mergeStateStatus is CLEAN.
Re-ran the checks that could plausibly move under the merge, at the new head:
npx tsc --noEmit— exit 0.npm test— 2533 pass / 0 fail.- Mutation, re-derived at this head. Deleting the guard on line 124 of
virtual/module.f.mjs(file !== undefined && !Array.isArray(file)) leaves 2532 pass / 1 fail, and running the proof cases individually under that mutant shows the single failure iswriteFileOverJsModulewhilewriteFileOnDirectorystill passes. The new case is genuinely non-redundant: the pre-existing directory case exits earlier atpath.length !== 1, so only the JsModule case reaches this guard. - Coverage, re-measured.
fjs/effects/node/virtual/module.f.mjsbranch 89.12, repo branch 98.22 — unchanged from the reviewed head, as expected since neither the proof nor the module under test moved. The run was not vacuous: 2469 tests executed.
Per §8.3 no CHANGELOG entry is needed for a proof-only change, matching the #1509 / #1515 / #1525 precedent. The branch adds no exported types (the delta adds one key to the already-exported proof object), so there is no unprefixed-export type exposure, and it touches no markdown, so the broken-link set is main's.
Still open, still non-blocking — the same finding as at d7188d9d, unchanged because the proof did not change. The case pins only result[0] === 'error' and drops the returned state on the floor. I built the exact clobbering mutant to confirm the gap is real at this head: rewriting the guard's return as
if (file !== undefined && !Array.isArray(file)) { return [{ ...dir, [name]: [payload] }, writeFileError] }— which reports the error and destroys the JsModule entry it was supposed to protect — passes the entire suite, 2533/2533. Destructuring the state and asserting the entry is still the original function would close it, the same way #1525's single-field pin let a null → undefined change survive. Not a reason to hold this PR: the case is a strict improvement over having no coverage of the branch at all, and writeFileOnDirectory next to it has the same shape.
Approving — the merge keeps it clean.
createExclusiveNestedMissing, writeBytesNestedMissing, and writeBytesMissingFile all started from emptyState and only checked the error tag, so a mutant returning the right tag alongside a wiped root would pass unnoticed (the #1525/#1528 shape, previously found in this same file). Start each from a non-empty root and assert it survives untouched. writeBytesNegativeOffset only checked the error tag too, so weakening the offset guard from `!Number.isInteger(offset) || offset < 0` to `!Number.isInteger(offset)` would still pass (a different, later guard catches -1 as a size mismatch instead). Pin the exact message.
Summary
Added a new test case to improve code coverage for the
writeFileOpfunction by testing the scenario wherewriteFileis called on a path that currently holds a JsModule (function).Key Changes
writeFileOverJsModuletest case that verifieswriteFilecorrectly handles attempts to overwrite a JsModule entry!Array.isArray(file)branch inwriteFileOp, which handles cases where an entry exists but is neither undefined nor an arrayImplementation Details
a.f.ts) that holds a function (JsModule) instead of file contenthttps://claude.ai/code/session_017JcPkoy1QpeZTFTvStYhMf