Add test coverage for defensive branches in JSON parser - #1536
Conversation
endArray/endObject's null/non-object top guards and tokenToValue's default arm are unreachable through parse() itself (the state machine guarantees the invariants), so exercise them with direct calls, matching the existing pushKey.nonObjectTop pattern. Brings media/json/parser/module.f.mjs to 100% line/branch/function coverage.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | a2a2375 | Commit Preview URL Branch Preview URL |
Aug 13 2026, 10:34 PM |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving.
Head note: the head moved while I was reviewing, from d92da09b to
b9ae16f3. I re-verified everything below at b9ae16f3. The move was a merge
of main, and main had advanced to fddc5dca (#1535). Against that new
baseline the PR's entire delta is still exactly the 33-line proof addition to
fjs/media/json/parser/module.f.mjs — git diff $(git merge-base origin/main b9ae16f3) b9ae16f3 is that one file — so the merge brought in nothing of its
own. Baseline for every number below is origin/main = fddc5dca.
Are these branches actually unreachable?
Each case documents its branch as unreachable through parse. I tested that
rather than taking it: I replaced all four defensive arms (the three new ones
plus the pre-existing pushKey one) with throw, then ran parse over every
token sequence up to length 6 over the full 13-kind JsonToken alphabet —
5,229,043 sequences. Zero reached any of the four.
Negative control: instrumenting the live arm of endArray instead fired 22
times within depth 3 alone, so the harness does detect reachability.
The comments are therefore accurate, and calling the internals directly is the
right way in — same pattern as btree/remove's proof.throw group. No branch
turned out to be reachable, so there is no more interesting finding hiding here.
Two-sided mutation testing
Six mutations, each applied to the PR tree and then identically to
origin/main:
| mutation | kills on PR | kills on main@fddc5dca |
|---|---|---|
endArray: drop the top === null arm |
endArray.nullTop (TypeError) |
nothing — 2549/2549 pass |
endArray: defensive arm yields [] not null |
endArray.nullTop |
nothing — 2549/2549 |
endObject: drop the non-object arm |
endObject.nonObjectTop |
nothing — 2549/2549 |
endObject: defensive arm yields {} not null |
endObject.nonObjectTop |
nothing — 2549/2549 |
tokenToValue: delete the default arm |
tokenToValue.nonValueToken |
nothing — 2549/2549 |
tokenToValue: default yields false not null |
tokenToValue.nonValueToken |
nothing — 2549/2549 |
Every mutation kills exactly one case, and it is the case that targets it — 50
of 51 parser proofs still pass in each run. So each case is doing work no
existing test was doing.
The two weaknesses this PR family keeps hitting
-
#1518, message-pinning. Not present. None of the three new cases asserts
on an error message;endArray/endObjectassertstatus === 'result'and
tokenToValueasserts the returned value directly. -
#1525/#1528, pinning one field while a mutant clobbers another. Also not
present, and this is the part I most expected to find. BothendArrayand
endObjectassertstatusandvalue. Rows 2 and 4 above are exactly the
"right tag, wrong payload" mutant: the arm still returnsstatus: 'result'
but yields[]/{}instead ofnull, and both are killed.assertEqis
===(fjs/asserts/module.f.mjs:31-34), so the reference comparison is what
catches them. The'result'state carries no third field to clobber.This is a different situation from #1535, where
proof.throwasserting only
that a call throws was correctly judged fine: here the cases return normally,
so the value assertion is the only thing standing between a mutant and a pass,
and it is present.
Battery
npx tsc --noEmit→ 0.npm run prepackfrom a clean tree → 0, both trees.npm test: 2552 / 0 fail vs 2549 on main. +3 = the three new cases.- Coverage delta (
npm run cov, real numbers — the tool is not vacuous in
this environment, it runs the full suite):
fjs/media/json/parser/module.f.mjsbranches 97.22% → 100.00%; lines and
functions were already 100% and stay there. All files: branches
98.27% → 98.35%, lines 99.94% unchanged. - Public surface after
prepack: 527 type aliases and 889 consts, identical
on both trees (46,199 / 64,198 bytes, non-empty). No unprefixed type added,
nothing widened. The parser's emitted.d.mtskeeps its@moduleheader. linkcheck: 159 on both trees, identical sets.- CHANGELOG (§8.3): none needed and none added — this is proof-only, touching
nothing but theproofblock, with an unchanged public surface. Correct.
No findings.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12c87f705f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `endArray` is only ever invoked while `state.top` is the array | ||
| // being closed (set by `startArray`), so `top === null` is a | ||
| // defensive branch unreachable through `parse`. Call it directly. | ||
| nullTop: () => { |
There was a problem hiding this comment.
These comments establish that top === null is unreachable through parse, yet the test bypasses the parser to preserve and cover that dead branch. This makes the coverage result depend on behavior for an impossible internal state rather than simplifying the implementation; remove the branch and its direct test, and apply the same correction to the newly tested unreachable branches in endObject and tokenToValue.
AGENTS.md reference: AGENTS.md:L150-L158
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bb79b67: restructured endArray/endObject to type-assert state.top instead of branching on it (the state machine already guarantees the container type there), and narrowed tokenToValue's parameter to a new _ValueToken type derived from isValueToken's type guard, so its switch is exhaustive with no defensive default arm. All three unreachable branches from this PR are gone, along with their direct-invocation tests; coverage stays at 100% line/branch/function for the file.
Generated by Claude Code
Per AGENTS.md §3.2, genuinely unreachable branches should be eliminated rather than covered by direct-invocation tests. Address review feedback on PR #1536 (chatgpt-codex-connector) by: - endArray/endObject: drop the top===null / non-object guards and type-assert state.top instead, since the state machine already guarantees the container type at those call sites. - tokenToValue: narrow its parameter to a new _ValueToken type (the union isValueToken already establishes as a type guard) so the switch is exhaustive without a defensive default arm. This removes the three direct-call proof cases added for those branches; the pre-existing pushKey.nonObjectTop case is untouched. media/json/parser/module.f.mjs stays at 100% line/branch/function coverage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb79b674d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| const endArray = state => { | ||
| const array = state.top !== null ? toArray(state.top.values) : null | ||
| const array = toArray(/** @type {_JsonArray} */ (state.top).values) |
There was a problem hiding this comment.
This PR changes authored parser code, but the ## Unreleased section of CHANGELOG.md has no entry for PR #1536. Add the required short entry with the real PR number before merging; the repository workflow explicitly requires a CHANGELOG entry for every code-changing PR.
AGENTS.md reference: AGENTS.md:L133-L135
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
3 participants
This change adds test cases to cover defensive branches in the JSON parser module that are unreachable through normal
parseexecution but exist as safeguards.Summary
Added three new test suites to the
proofobject that exercise defensive code paths in the JSON parser:Key changes made
endArraytests: AddednullToptest case that directly invokesendArraywith a nulltopstate, which is normally set bystartArrayand should never be null during regular parsingendObjecttests: AddednonObjectToptest case that directly invokesendObjectwith a non-objecttop(an array instead), which is normally guaranteed to be an object during regular parsingtokenToValuetests: AddednonValueTokentest case that directly invokestokenToValuewith an EOF token, which should never occur sinceisValueTokenguards against non-value tokens during normal parsingNotable implementation details
parseexecutionhttps://claude.ai/code/session_01BmWCv5YGRXToq26xPoSXjX