LibCodeGen: refuse a codeless instance, require an identifier name, and stop emitting a blank comment line

[thedavidmeister]

Closes#44
Closes#45
Closes#46
Closes#47
Closes#48

Five findings, all in src/lib/LibCodeGen.sol. They are one PR because they
touch the same two functions: #46 changes how bytecodeHashConstantString
assembles its declaration while #44 adds a guard to the same function, and
#45's two halves and #48's rewrite land in the same file.

The issues frame the problem and deliberately do not pick the fix. Each
decision and its reasoning is below.

---

#44 — bytecodeHashConstantString refuses an address with no code

Decision: refuse, not document.

The constant exists so a consumer can assert addr.codehash == BYTECODE_HASH.
For an address with no code extcodehash returns bytes32(0) when the account
does not exist and keccak256("") when it does — and addr.codehash returns
those same two values for every codeless address. So the emitted constant was
satisfied by any codeless address. The check read as verifying a deployment
and verified nothing, and the failure landed in a consumer repo asserting
against a constant that could not fail.

Documenting the precondition leaves that silent. LibFs.buildFileForContract
includes this constant unconditionally, so there is no path where a caller opts
out of it, and generation is a build-time step where a revert costs nothing and
is loud.

The behaviour change costs no existing caller. Every live call site in the org
constructs the instance on the line above and passes it:

repo	call
raindex	LibRainDeploy.deployZoltu(...) then the address
rainlang	same, twice
rain.flare / rain.pyth / rain.merkle / rain.erc4626.words	new XWords() then address(x)

The guard reads extcodesize, not "is the hash one of the two sentinel
values", so it says what it means and does not depend on either sentinel.
bytes32(0) is not special-cased into a pass anywhere: bytes32ConstantString
still emits zero as plainly as any other value, which its own test pins.

#45 — keep the function, sanitise the name, take the grant

Decision: keep and document, not remove. Sanitise with the Solidity
identifier rule.

Removal was on the table in the issue because nothing in this repo calls it.
Checked rather than assumed — GitHub code search over rainlanguage:
describedByMetaHashConstantString has seven live call sites in six repos
(raindex, rainlang ×2, rain.flare, rain.pyth, rain.merkle,
rain.erc4626.words). Removing it breaks all of them. It stays.

The same check settles the permission half. All six of those repos already
grant meta in fs_permissions — { access = "read-write", path = "meta" }
in five, { access = "read", path = "./meta" } in raindex. So meta/ is an
established convention that only this library's own docs and config failed to
state. The NatSpec now states the grant a consumer needs (read), and
foundry.toml here takes read-write — read is all the library needs, the
write is so this repo's own suite can lay down and clear its fixtures.

For the sanitisation, the rule is the Solidity identifier: non-empty, ASCII
letters / digits / _ / $, not starting with a digit. That is what a
contract name is, and it is also a character set that cannot express a
separator, a parent directory or an empty basename — so .., / and "" are
refused by the rule rather than by special cases for them. All seven live call
sites pass a plain identifier ("RaindexV6SubParser", "PythWords",
"FlareFtsoWords", …), so this breaks nobody.

The check lives in LibCodeGen rather than a shared file on purpose.
LibFs imports LibCodeGen, so LibCodeGen cannot import LibFs back, and a
new shared LibContractName.sol would be a file two parallel branches both
create. requireContractName is internal and callable, so if the LibFs
side lands its own copy the two collapse into one shared file in a follow-up
rather than blocking either now.

One existing test file changed shape, and it should be reviewed as such.
test/lib/LibCodeGen.describedByMetaHashConstantString.t.sol could only reach
the function by traversing ../src/generated/… out of meta/, because the
repo granted no read under meta. The identifier rule forbids exactly that, so
the fixtures move to real files under meta/. No assertion was weakened or
removed: testDescribedByMetaHashConstantStringPath still observes the path
through the caught read failure and is unchanged, because foundry quotes the
full path in its missing-file error too — verified directly, not assumed:

vm.readFileBinary: failed to open file ".../meta/CodeGennable.rain.meta": No such file or directory (os error 2)

meta/ holds no committed file. The suite creates it and every fixture is
removed again, which leaves the directory empty and so invisible to git.

#46 — both bytes32 declarations go through bytes32ConstantString

Decision: route them, rather than accept the two-character margin.

DESCRIBED_BY_META_HASH emitted a 118-character line against a 120 limit. The
names are string literals in this file with nothing pinning them, and the wrap
arithmetic already existed one function away, so the fix is to have one
definition of the wrap threshold instead of three call sites of which two
skipped it.

The output is byte-identical today, which is what makes this safe: both
names fit (109 and 118 characters), so both take the space rather than the
wrap. Verified two ways — testBuildFileForContractCommittedArtifactIsCurrent
stays green, and forge script script/Build.sol regenerates
src/generated/CodeGennable.sol to no diff. Nothing was regenerated because
nothing moved.

#47 — an empty comment emits no comment line

Decision: emit no comment line, rather than reject "".

Rejecting turns a formatting helper into a reverting one for an input that
produces valid Solidity. Emitting no comment line gives the caller what they
asked for, keeps the function total, and removes the blank line that
forge fmt would collapse. A declaration is now preceded by exactly one blank
line whether or not it has a comment.

commentPrefix is shared by all four builders so there is one statement of the
rule rather than four.

One thing to look at: LibCodeGenSlow, the naive differential reference the
…MatchesMeasuredLine fuzz tests measure against, states the same rule on its
own side. It is the one place the two sides agree by construction rather than
by independent derivation, so its docstring says so, and the exact text of both
the empty and non-empty case is pinned separately by literal assertions in all
four suites — the reference is not the only thing holding it.

#48 — @param vm says what each function does with the Vm

Ten of eleven get The Vm instance used to format values as strings.
describedByMetaHashConstantString — the one that touches disk — says so and
carries the fs_permissions block from #45. MAX_LINE_LENGTH's comment says
forge fmt; there is no foundry fmt.

---

QA

• Discriminating tests: 110 tests, up from 84, none removed, all asserting exact emitted text
  or an exact revert rather than that a call did not revert.
  testRequireContractNameAcceptedNamesAreIdentifiers derives the expected
  verdict from the alphabet independently and fuzzes the whole string domain
  against it, and …AcceptedNamesCannotTraverse states the safety property
  directly — no accepted name carries /, \, . or a nul — rather than
  listing the sequences that would escape. The bytecodeHashConstantString emits a plausible BYTECODE_HASH for an address with no code, so the consumer's codehash check passes for any codeless address #44 refusal is pinned for a
  non-existent account (codehash == bytes32(0)), a funded but codeless
  account (codehash == keccak256("")) and address(0) separately, with the
  precondition on codehash asserted in each so the test fails if the two
  values it is about ever stop being those values. …AcceptsAnyNonEmptyCode
  pins that one byte of code is enough, so the guard is on there being no code
  rather than on the code being short. Both bytes32 declarations are measured
  against LibCodeGenSlow, which builds the one-line form and measures it, so
  the wrap decision they now share is checked rather than assumed.

• Mutation: 33 mutants over src/lib/LibCodeGen.sol, 33 killed, 0 survived,
  0 no-run, 0 harness errors, against a green 110-test baseline. Each of the
  three identifier ranges is broken at both ends and the empty check disabled;
  the empty-comment branch is forced both ways and its blank line dropped; the
  codeless guard is disabled and inverted, and both the extcodesize and the
  extcodehash it sits on are pinned to constants; the meta directory is
  renamed and the hash taken over the path instead of the contents; all four
  wrap thresholds are moved off by one and the bytes32 wrap disabled outright;
  the wrap indentation, both explicit type wrappers, and the file prefix's
  pragma and autogenerated warning are each broken.

  One mutant survived the first pass — the uppercase range run one past Z,
  which accepts [ — and that survivor is what
  testRequireContractNameRangeBoundaries was written for. The fuzzer almost
  never draws a name that is a valid identifier apart from one boundary
  character, so nothing else in the suite separated [ from Z. The new test
  pins all three ranges from both sides and kills that mutant and four others.

  The first pass also scored five mutants NO-RUN, which was the harness lying
  rather than a fact about the code, and is worth a reviewer's attention on its
  own. The LibFs tests write real files into src/generated/ and the
  describedByMeta tests write fixtures into meta/, and a test that fails
  aborts before its own cleanup — so the pragma solidity ^0.8.26 mutant left
  nine generated sources behind that no installed solc could compile, and the
  five mutants after it were scored on its residue rather than on themselves.
  The suite command now clears both directories to their committed state before
  every run and the whole matrix was re-run under it; the numbers above are from
  that run. The same residue is left by any failing run of this suite, in CI as
  much as locally.

• Oracle: intent came from the consumers, not from this repo. describedBy…'s
  disposition was decided by GitHub code search over rainlanguage — seven
  call sites in six repos, every one passing a plain identifier, every one
  already granting meta — which is what rules out removal and what makes the
  identifier rule non-breaking. The two codeless codehash values were checked
  against the EVM in-test rather than recalled. Foundry's missing-file error
  text was measured before relying on it, because an existing test reads the
  path out of it. forge fmt --check and a full forge script script/Build.sol
  regeneration are what establish that the bytecodeHashConstantString and describedByMetaHashConstantString hand-roll their bytes32 declaration and skip the wrap arithmetic — 2 characters of headroom #46 routing and the An empty comment makes every *ConstantString emit a blank line, so the generated file is not stable under forge fmt #47 change leave
  the committed artifact untouched.

• Category check: the ask is five issues in one file, and the category is every
  function in that file that the change reaches, not the two the issues name.
  All four *ConstantString builders take the empty-comment change and all
  four have a literal empty-comment test plus a declaration-unchanged test; all
  eleven @param vm lines were rewritten, not the ten that were wrong; both
  hand-rolled declarations were routed, not just the one with two characters of
  headroom. Deliberately not done here: the identical unvalidated-name problem
  in LibFs.pathForContract (LibFs.pathForContract accepts any string as a contract name: empty writes a hidden dotfile, a slash targets a subdirectory, and .. escapes under a normal fs_permissions grant #38) and the README half of the docs (README tells consumers to write script/BuildPointers.sol, which rainix CI rejects outright — plus three more stale claims #42),
  which are other branches — a second edit to those files is a conflict rather
  than help.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in:28 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3e303ba3-aaf8-4226-9c3e-e450500115ad

📥 Commits

Reviewing files that changed from the base of the PR and between 127d9a0 and 429cee3.

📒 Files selected for processing (10)

• foundry.toml
• src/lib/LibCodeGen.sol
• test/lib/LibCodeGen.addressConstantString.t.sol
• test/lib/LibCodeGen.bytecodeHashConstantString.t.sol
• test/lib/LibCodeGen.bytes32ConstantString.t.sol
• test/lib/LibCodeGen.bytesConstantString.t.sol
• test/lib/LibCodeGen.describedByMetaHashConstantString.t.sol
• test/lib/LibCodeGen.requireContractName.t.sol
• test/lib/LibCodeGen.uint8ConstantString.t.sol
• test/lib/LibCodeGenSlow.sol

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thedavidmeister]

Reviewed 429cee3: ready — Closes #44, #45, #46, #47, #48.

33 mutants, 33 killed, 0 survived, 0 no-run, 0 harness-error, baseline green at 110. 84 tests on main, 26 added, 0 removed — verified by diff rather than by the handoff's claim.

The two decisions that reach beyond this repo were settled by looking outside it, not by reasoning. #44 refuses a codeless instance rather than documenting the precondition, and the org search found all 7 live call sites across 6 repos deploy on the line above, so nothing breaks. #45 keeps describedByMetaHashConstantString rather than removing it, because those same 6 repos already grant meta in fs_permissions — so meta/ is convention, and the fix is stating the required grant rather than deleting a function six consumers call. That refutes the "or remove it if unused" half of the issue, which I wrote after checking only this repo.

#46 routes both hand-rolled bytes32 declarations through bytes32ConstantString, and the output is byte-identical today at 109 and 118 characters against the 120 threshold — confirmed by forge script script/Build.sol producing no diff and by build-pointers passing, so the committed artifact needed no regeneration.

Three things the handoff did not predict, each caught rather than absorbed. The uncommitted test did not compile — its NatSpec contained a backtick-@, which solc reads as a doc tag. extra.toml never existed, so M28's corrected target and M29 through M33 were authored fresh against behaviours the first 28 never reached, including extcodehash itself, which M15 had only pinned through extcodesize.

And the first re-run scored M29 to M33 as NO-RUN, which was the harness lying rather than a result: M28's ^0.8.26 mutant made the LibFs tests write nine files into src/generated/ carrying that pragma, the failing assertion aborted before their cleanup, and the residue outlived the restore so no installed solc could compile the next five runs. The suite command now clears src/generated/ and meta/ to committed state before each run, and all 33 were re-run under it — no reported verdict is inherited from the broken pass.

That residue is a live defect beyond this PR and is disclosed in the QA block rather than buried: any failing run of this suite leaves those files behind for the next one, in CI as much as locally.

CI green. CodeRabbit reports Review rate limited, so the zero threads behind that green is not review coverage.

One follow-up this creates: error InvalidContractName and requireContractName now live in LibCodeGen, while #53 defines the same rule as LibContractName.sol. #53 should import from here rather than carry a second definition — LibFs already imports LibCodeGen, so there is no cycle.