Uh oh!
There was an error while loading. Please reload this page.
LibCodeGen: cover every unprobed function and pin the four wrap decisions - #39
Conversation
Adds test files for `bytecodeHashConstantString`, `describedByMetaHashConstantString`, `bytesConstantString`, `uint8ConstantString` and the five function pointer wrappers, and adds wrap boundary coverage to the two existing constant string suites. `test/lib/LibCodeGenSlow.sol` is a reference that builds the declaration and measures it, instead of adding up magic numbers standing in for the literals, so the two agree only when every term of the library's sum is right. Boundary tests pin the decision at exactly 120 and 121 characters, which is where `forge fmt` itself changes its mind. `test/concrete/ToolingMock.sol` answers all five tooling builders with distinct data, so a wrapper asking the wrong builder emits the wrong hex. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Warning Review limit reached
Next review available in:37 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… other Two of the suites added in the previous commit were green only by luck. Both reproduce on a clean tree at HEAD and both were passing CI on a draw that happened to miss them. `bytecodeHashConstantString` fuzzes `bytes` straight into `vm.etch`, which refuses code shaped like an EIP-7702 delegation designator — leading bytes `0xef01` — at any length other than 23. The fuzzer reaches that shape on some seeds and not others, so the suite failed on the first run with `Eip7702 is not 23 bytes long`. The refused set was measured against the cheatcode rather than assumed — `0xef01` + 21 bytes is accepted, the same prefix at 22, 24 and 2 bytes is refused, and `0xef`, `0xef0000` and `0xef02…` are all accepted — so the fuzz domain now excludes exactly that shape and nothing else. `describedByMetaHashConstantString` had all four of its file-backed tests writing and then removing one shared fixture path. The fixture is real state that outlives the EVM, so one test's `removeFile` lands between another's `writeFileBinary` and the library's read, and that test fails on a file it had just written. It reproduced at a fixed `--fuzz-seed`, so the fuzzer was never involved. Each test owns a distinct path now. Verified over 30 consecutive full-suite runs — seeds 1..20 plus ten repeats at the seed the fixture race reproduced on — all 51 tests green, `forge fmt --check` clean. This is also a precondition for the mutation matrices: a flaky baseline scores mutants KILLED on the flake rather than on the mutation, and `mutation-probe` refuses to start against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedavidmeister
commented
Aug 16, 2026
Reviewed 2c71033: ready. Tests only, 13 files, suite 9 tests / 4 suites to 51 / 13. Covers every function the three existing test files left unprobed: Three matrices, and nothing survived:
The baseline pass is what makes the first row mean anything: the pre-existing suite killed 14 of 95, so this closes 81 real gaps rather than re-pinning what was already pinned. Zero HARNESS-ERRORs, confirmed structurally rather than asserted — the branch's The hypothesis this group was pointed at is REFUTED. All four wrap sums are exactly right, re-derived from the literals each function concatenates and then checked against real
Two defects in this branch's own tests were found and fixed before the matrices could run, and Stated rather than smoothed over: killer attribution names a test for only 8 of 95 kills, because the probe's fail-pattern cannot cross the newlines in multi-line Solidity assertion messages. The KILLED verdicts come from the pass/fail tally and are unaffected. The PR body says so instead of implying attribution it does not have. Findings from this group are filed separately and none is blessed by a test here: #44, #45, #46, #47, #48. All five checks green. CodeRabbit reports |
Uh oh!
There was an error while loading. Please reload this page.
Adversarial + mutation test pass over
src/lib/LibCodeGen.sol. Tests only — nosource change.
Before this, three of the library's eleven functions had a test file
(
filePrefix,bytes32ConstantString,addressConstantString) and the othereight had nothing. None of the four wrap decisions was probed at all.
What is added
test/lib/LibCodeGenSlow.sol— a reference for the four*ConstantStringdeclarations.
LibCodeGendecides whether a declaration fits on one line byadding up magic numbers standing in for the literals it is about to
concatenate (
15 + name + 2 + 1 + 4 + hexData + 2,17 + … + 8 + …). Thereference instead concatenates the literals, builds the line, and measures it.
The two agree only when every term of every sum is right.
120and the"\n "wrap are spelled out again there rather than imported, so thereference does not move when the library's constants do.
test/concrete/ToolingMock.sol— answers all five tooling builders withdistinct data, so a wrapper that asks the wrong builder emits the wrong hex
rather than passing on data that happens to be identical.
bytecodeHashConstantString,describedByMetaHashConstantString,bytesConstantString,uint8ConstantString,opcodeFunctionPointersConstantString,literalParserFunctionPointersConstantString,operandHandlerFunctionPointersConstantString,subParserWordParsersConstantString,integrityFunctionPointersConstantString.existing test was edited or deleted.
Suite goes from 9 tests / 4 suites to 51 tests / 13 suites.
What the wrap arithmetic turned out to be
Each of the four sums was re-derived from the literals the function actually
concatenates, and then checked against
forge fmtitself on generated fixturesat 116–123 characters. All four are exactly right, and
>rather than>=isalso exactly right:
forge fmtleaves a line of exactly 120 alone and breaksone of 121 after the
=, indenting the value by four. The tests now pin that at120 and 121 for all four, in both directions — by name length and by value
length — so an off-by-one in any single term fails rather than producing a file
forge fmt --checkrejects on some inputs and not others.Note on
describedByMetaHashConstantStringThis function reads
meta/<name>.rain.meta, and this repo'sfs_permissionsgrants read only on
foundry.tomland read-write onsrc/generated, so theread is refused under the repo's own config. The path it constructs is asserted
through the refusal, which quotes the path. The rest of the behaviour — the
comment, the constant name, and that the value is the keccak256 of the file's
bytes — is covered with a fixture written to the one directory the config does
grant, reached by a name that walks back out of
meta/. That the name reachesthe path at all is unsanitised interpolation, which the test states.
Two flakes in the first commit, fixed in the second
Both suites below were green on the seeds and interleavings CI happened to draw,
and red on a clean tree locally. Neither was a source defect; both were the tests
sampling states they had no business sampling.
bytecodeHashConstantStringfuzzedbytesstraight intovm.etch, whichrefuses code shaped like an EIP-7702 delegation designator — leading bytes
0xef01— at any length other than 23. On an unlucky seed the suite failed onthe very first fuzz run with
Eip7702 is not 23 bytes long. The refused setwas measured against the cheatcode rather than assumed:
0xef01+ 21 bytes isaccepted, the same prefix at 22, 24 and 2 bytes is refused, and
0xef,0xef0000and0xef02…are all accepted. The fuzz domain now excludes exactlythat shape and nothing wider.
describedByMetaHashConstantStringhad all four of its file-backed testswriting and then removing a single shared fixture path. That file is real state
outliving the EVM, so one test's
removeFilelands between another'swriteFileBinaryand the library's read, and that test fails on a file it hadjust written. It reproduced at a fixed
--fuzz-seed, so the fuzzer was neverinvolved. Each test owns a distinct path now.
Verified over 30 consecutive full-suite runs — seeds 1..20 plus ten repeats at
the seed the fixture race reproduced on — 51 tests green every time.
QA
that a call did not revert.
testBytesConstantStringAtMaxLength/…OverMaxLengthByName/…OverMaxLengthByDataand theiruint8,bytes32and
addressequivalents pin the wrap decision at exactly 120 and 121characters and assert the measured longest line, so both the constant and
every term of the sum are constrained. The five wrapper tests set five
distinct builder answers on one mock, so asking the wrong builder fails.
Full suite: 51 tests, 13 suites, all pass;
forge fmt --checkclean.src/lib/LibCodeGen.solin threemutation-probepasses, each against a baseline proved green first — a flakybaseline scores mutants on the flake rather than on the mutation, which is why
the two flakes above were fixed before any matrix was trusted.
builder call, each term of all four wrap sums, both wrap branches, the
comparison operator): 95 KILLED, 0 SURVIVED, 0 NO-RUN, 0 HARNESS-ERROR.
pre-existing 9 tests: 14 KILLED, 81 SURVIVED, 0 NO-RUN,
0 HARNESS-ERROR. So 81 of the 95 were live coverage gaps that this PR
closes; the 14 the old suite already caught are the
filePrefixtext andparts of the
bytes32/addressdeclarations.newlines, indentation, delimiters that pass A did not touch): 30 KILLED,
0 SURVIVED, 0 NO-RUN, 0 HARNESS-ERROR.
reported as covered on the strength of a harness error, and there is no
survivor left needing a discriminating test.
a killing test from a
[FAIL …] testName(regex, and.does not cross anewline, so a
forgeassertion message that spans lines — which most ofthese are, being multi-line generated Solidity — yields no name. 8 of 95
pass-A kills carry killer names; the other 87 are KILLED on the suite's own
passed/failed tally, which is the probe's actual verdict source and is
unaffected.
forge fmt, soforge fmtis the oracle for the wrap decision rather thanthe code's own arithmetic. Ran it directly: generated fixtures of
bytes,uint8,bytes32andaddressdeclarations at every length from 116 to 123were passed through
forge fmtandforge build, which is what fixes 120 asthe last line that stays put and 121 as the first that breaks. The same run
confirms
forge fmtdoes not split ahex"…"literal, so a wrappedbytesline longer than 120 is stable rather than a defect, and that
hex""forempty data compiles.
src/lib/LibCodeGen.sol, and thecategory is every function in it plus both file level constants — eleven
functions, all now with a test file, plus
MAX_LINE_LENGTHandNEWLINE_DUE_TO_MAX_LENGTHpinned through the boundary tests rather thanrestated as equalities. Not covered, deliberately: an address with no code
reaching
bytecodeHashConstantString, which currently emitsbytes32(0x00…00)fromextcodehashwith no guard. A passing test therewould entrench that, so it is raised separately instead.