LibFs: require a Solidity identifier, bootstrap the generated dir, document the unlink

[thedavidmeister]

Closes#38, closes#40, closes#41.

Three issues in the write path, all about what it is allowed to write and
where, so they are one change. main merged in at 72fddf4, which landed #54.

#38 — a contract name has to be a Solidity identifier

Decision: the check is inside pathForContract, not at the write.

pathForContract reverts unless contractName is a Solidity identifier, so
every path this library can produce is a direct child of GENERATED_DIR. There
is no name for which the library returns a path at all without returning a safe
one, and a caller that takes the path and does its own IO with it gets the same
confinement buildFileForContract does. All three failures in the issue are
refused:

• "" — the path is src/generated/.sol, valid generated Solidity at a name
  no compiler picks up as a contract file and that ls hides, written by a
  build that reports success.
• sub/Foo — a subdirectory, which the read-write grant on src/generated
  admits, and which dies on a bare No such file or directory when the
  subdirectory is not there.
• ../../ESCAPED — outside the tree entirely under the
  { access = "read-write", path = "." } grant consumers commonly write.

An earlier revision of this PR put the check at the write instead, on the
argument that every harm in the issue is a write and that a name which never
reaches disk is just a string. That is the weaker guarantee and it is reversed
here. The issue is titled for pathForContract, not for the write; the write
is only the first consumer of a bad path, not the only possible one.

The cost that argument gave for the stronger placement was that three fuzz
properties landed by #36 — testPathForContractStructure,
testPathForContractDistinctNamesDistinctPaths and
testPathForContractIsRelative — quantify over arbitrary strings, and a random
string is essentially never an identifier, so vm.assume could not be
satisfied. That is a reason to fix the generators, not a reason to weaken the
code. All three keep the property they were landed for — that a name is never
quoted, escaped, trimmed, case folded or truncated on its way into the path —
and now assert it over names CONSTRUCTED from the identifier alphabet
(LibCodeGenSlow.nameFromSeedSlow) rather than over strings filtered by
vm.assume. Constructing is what keeps them non-vacuous: filtering for an
identifier would have left them proving the property over almost nothing.

The residual an earlier revision recorded — pathForContract("..") returning
src/generated/../.sol — no longer exists. That call reverts.

Where the check lives.#54 landed LibCodeGen.requireContractName and
InvalidContractName on main while this branch was open, for
describedByMetaHashConstantString's own name parameter. This branch's
LibContractName was a second definition of the same rule, so it is deleted and
LibFs calls LibCodeGen.requireContractName. There is no import cycle:
LibFs already imports LibCodeGen for filePrefix and
bytecodeHashConstantString, so the check arrives with an import that was
already there. One definition of what a contract name is, used by both the path
and the meta hash constant.

#40 — first generation in a repo that has never generated

vm.createDir(GENERATED_DIR, true) before the write. That is create_dir_all,
so it is a no-op when the directory is already there, and it is allowed by the
read-write grant on src/generated that a consumer needs in order to write at
all — checked against this repo's own fs_permissions, not assumed.

#41 — decision: keep the removal, document what it does

The docstring said the removal "ensures idempotent file generation". It does
not, and that claim is gone: vm.writeFile truncates, so the bytes at the path
are identical with and without the removal. What idempotency there is comes from
the whole file being written on every call, and that is what the docstring says
now.

The line stays, for the reason that is actually true. removeFile unlinks, so a
symlink at the path is replaced by a regular file rather than followed and
written through to its target. With the name now confined to a single path
segment, that unlink is the only thing left between buildFileForContract and a
write that lands outside src/generated: foundry normalises paths lexically
before the permission check, so a symlink inside a granted directory is not
caught by the grant. Deleting the line closes the lexical half of #38 and leaves
the symlink half open, to save one cheatcode call.

The cost the issue names is real and is now documented instead of argued away:
between the unlink and the write the path does not exist, so a write that fails
takes the previous content with it. What is at that path is a generated file
that the build reproduces from source and that consumers commit, so that window
costs a rerun.

The argument against keeping it is that the docstring shows it was written for
idempotency rather than for symlinks, which makes the symlink guarantee a
justification found afterwards — and keeping a line because it happens to be
there is how a mistake gets entrenched. It survives that on its own merits: the
guarantee is real, it is the other half of the invariant the rest of this PR
establishes, and it is now stated in the NatSpec, so the next reader deletes it
deliberately or not at all.

Together, #38 and #41 make one statement that neither makes alone:
buildFileForContract writes only to a regular file that is a direct child of
src/generated.

Tests

110 → 123 over the merged main.

test/lib/LibFs.t.sol is where the guarantee now lives, and it is split by
domain rather than quantified over arbitrary strings. Over the ACCEPTED domain,
names are constructed from the identifier alphabet: the three #36 properties as
above, plus testPathForContractAcceptedNamesStayInGeneratedDir, which counts
separators and dots in the path so no accepted name can reach a subdirectory, a
parent directory or a hidden file. Over the REJECTED domain, arbitrary bytes are
exactly the right generator, because that is what the rejected domain is:
testPathForContractRejectsEveryNonIdentifierName fuzzes it, and
testPathForContractRejectsNamedEscapes pins the strings from the issue plus
the other shapes that stop the path being a single segment.

test/lib/LibFs.buildFileForContract.t.sol keeps the refusals at the write.
They are not duplicates of the above: what they assert is that the write
inherits the refusal and that nothing appears on disk when it does, each of them
removing the path first so it establishes its own precondition rather than
assuming one.

src/lib/LibContractName.sol and its suite are deleted in favour of #54's
definition. The coverage that suite held and test/lib/LibCodeGen.requireContractName.t.sol
did not is folded into the surviving file rather than dropped: exhaustive sweeps
over all 256 bytes in the leading position and again in a trailing position,
fuzzed agreement with an alphabet spelled out character by character in
LibCodeGenSlow (the file's existing fuzz oracle inlines the library's own
range arithmetic, so it moves when the library moves), fuzzed acceptance of
constructed identifiers so the accepted half of the domain is exercised at all,
and fuzzed rejection when a single byte anywhere in an otherwise valid name is
replaced.

Mutation pass

22 mutants — 8 on LibFs, 14 on LibCodeGen.requireContractName — applied with
mutation-probe, classifying from forge's own N tests passed, N failed tally.
20 killed, 2 survived, 0 no-run, 0 harness errors. The old matrix does not
carry over: moving the check changed what each mutant probes, and main moved
under the branch. One killer is named per mutant below; most were killed by
several, and the probe caps its report at five.

src/lib/LibFs.sol, line -> mutation -> killing test:

• L11 GENERATED_DIR = "src/generated" -> "src/generated_moved" ->
  testBuildFileForContractCommittedArtifactIsCurrent
• L34 requireContractName(contractName) -> requireContractName("Placeholder")
  -> testBuildFileForContractRejectsEmptyName
• L33-65 the check moved back out of pathForContract and into
  buildFileForContract -> testPathForContractRejectsNamedEscapes
• L35 concat(GENERATED_DIR, "/", name, ".sol") -> concat(GENERATED_DIR, name, ".sol")
  -> testBuildFileForContractCommittedArtifactIsCurrent
• L67 vm.createDir(GENERATED_DIR, true) -> vm.createDir(GENERATED_DIR, false)
  -> testBuildFileForContractFreshPath
• L67 vm.createDir(GENERATED_DIR, true) -> vm.isDir(GENERATED_DIR) -> SURVIVED
• L68 if (vm.exists(path)) -> if (!vm.exists(path)) -> testBuildFileForContractFreshPath
• L70 vm.removeFile(path) -> vm.exists(path) -> SURVIVED

The third of those is the one this PR turns on, so it is worth stating what
killed it and what did not. Its two killers are
testPathForContractRejectsNamedEscapes and
testPathForContractRejectsEveryNonIdentifierName, and nothing else in 123
tests. Every refusal test on the write still passes under it, because under it
the write still refuses. The placement is held by exactly the coverage added for
it, and by nothing that was already there.

src/lib/LibCodeGen.sol (requireContractName, landed by #54; mutated because
this PR makes LibFs depend on it and adds coverage over it):

• L43 if (nameBytes.length == 0) -> if (false) -> testBuildFileForContractRejectsEmptyName
• L46 i < nameBytes.length -> i < nameBytes.length - 1 -> testRequireContractNameEveryTrailingByte
• L46 uint256 i = 0 -> uint256 i = 1 -> testRequireContractNameEveryLeadingByte
• L48 char >= 0x41 -> char >= 0x40 -> testRequireContractNameEveryLeadingByte
• L48 char <= 0x5A -> char <= 0x5B -> testRequireContractNameMatchesAlphabet
• L48 char >= 0x61 -> char >= 0x62 -> testDescribedByMetaHashConstantString
• L48 char <= 0x7A -> char <= 0x7B -> testRequireContractNameEveryTrailingByte
• L49 char >= 0x30 -> char >= 0x31 -> testPathForContractStructure
• L49 char <= 0x39 -> char <= 0x3A -> testPathForContractRejectsEveryNonIdentifierName
• L50 char == 0x5F -> char == 0x5E -> testRequireContractNameAcceptsGeneratedIdentifiers
• L50 char == 0x24 -> char == 0x25 -> testRequireContractNameAcceptsGeneratedIdentifiers
• L51 isDigit && i > 0 -> isDigit && i >= 0 -> testRequireContractNameEveryLeadingByte
• L51 isDigit && i > 1 -> testPathForContractStructure
• L51 !(isLetter || isUnderscoreOrDollar || (isDigit && i > 0)) -> !(isDigit && i > 0)
  -> testDescribedByMetaHashConstantString

Two of those are the direct evidence that rewriting the three #36 generators
kept them meaningful rather than quietly retiring them. char >= 0x31 and
isDigit && i > 1 both shrink the accepted alphabet, and both are killed by
testPathForContractStructure, testPathForContractIsRelative and
testPathForContractDistinctNamesDistinctPaths — because those now build names
containing digits and feed them to a function that must accept them. Filtered
through vm.assume instead, they would have been vacuous and killed nothing.

Survivor 1 — deleting vm.createDir

The only state that tells it apart is src/generated being absent, and this
repo commits that directory. Producing the state inside forge test means
deleting it mid-run, which races every other suite (forge runs suites in
parallel) and takes the committed artifact that
testBuildFileForContractCommittedArtifactIsCurrent reads with it. Reported as
a survivor rather than covered by a flaky test.

Checked by hand instead, and this is the evidence for #40 rather than a test.
With src/generated moved out of the working tree,
forge test --match-test testBuildFileForContractFreshPath passes as written.
With the same directory absent and only that line replaced by the surviving
mutant, it fails with the exact error from the issue:
vm.writeFile: failed to open file ".../src/generated/LibFsBuildFresh.sol": No such file or directory (os error 2).

Survivor 2 — deleting vm.removeFile

The same mutant that survived the campaign, and the subject of #41. Nothing
reachable through cheatcodes distinguishes remove-then-write from write-alone:
writeFile truncates, so the bytes are identical, and the symlink behaviour the
removal actually provides cannot be set up without ffi, which this repo does
not enable. Left surviving rather than killed with a test asserting something the
suite cannot see. The guard around it is a different matter and is covered —
testBuildFileForContractFreshPath kills the inversion, because removeFile
reverts on a path that is not there.

Harness note

The probe's suite command restores src/generated from git before each run.
Without that, the first mutant that makes an fs-writing test fail part way leaves
its file behind — a stale fixture, or a half generated file — the next
forge test fails to compile it, and every later verdict is NO-RUN dressed up
as a result. The two tests that assert nothing was written also remove the path
first, so they establish their precondition rather than assume it.

QA

• Discriminating tests: 123 tests, 16 suites, all pass; forge fmt --check
  clean; forge lint reports only unsafe-typecast, on uint8(i) inside
  for (i = 0; i < 256; i++) sweeps — the same rule, unsuppressed, that
  main's own LibHexString.bytesToHex.t.sol and
  LibCodeGen.requireContractName.t.sol already report. Baseline on the merged
  main (72fddf4) was 110 tests, 16 suites, 0 failures, measured on a clean
  checkout of that commit before any probing.
• Mutations applied: 22 over every line this PR adds or changes plus the check it
  now depends on. 20 killed, 2 survived, 0 no-run, 0 harness errors; both
  survivors are named above with why no test can hold them, and one of the two is
  the subject of buildFileForContract's remove-then-write is unobservable, and its NatSpec credits it with idempotency that writeFile already provides #41 itself.
• Oracle: the definition of a Solidity identifier and the three failures named in
  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, not the implementation. The reference alphabet in LibCodeGenSlow is
  spelled out character by character while the library decides by range
  comparison, so an off-by-one at either end of a range is a disagreement rather
  than a change that moves both sides at once. buildFileForContract cannot bootstrap a repo that has never generated: vm.writeFile does not create src/generated #40 is judged against the issue's
  own reproduction, run both ways, above.
• Category check: the ask names three failing inputs; the category is every name
  that is not a Solidity identifier, and it is covered as a set at the path — all
  256 bytes in both positions, fuzzed agreement over arbitrary byte strings, and
  fuzzed rejection when a single byte anywhere in an otherwise valid name is
  replaced.
• The committed src/generated/CodeGennable.sol is untouched and
  testBuildFileForContractCommittedArtifactIsCurrent still passes, so nothing
  here moved filePrefix or the bytecode hash constant.

[coderabbitai]

Warning

Review limit reached

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

Next review available in:15 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: 7ab9a276-c4f2-4aec-84ef-106943ad805d

📥 Commits

Reviewing files that changed from the base of the PR and between 72fddf4 and 5a4a1b7.

📒 Files selected for processing (6)

• src/lib/LibFs.sol
• test/concrete/LibFsExternal.sol
• test/lib/LibCodeGen.requireContractName.t.sol
• test/lib/LibCodeGenSlow.sol
• test/lib/LibFs.buildFileForContract.t.sol
• test/lib/LibFs.t.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 5a4a1b7: ready — Closes #38, #40, #41.

The identifier check sits in pathForContract on the user's ruling, taking the stronger guarantee: no path this library can produce is unsafe, rather than no path it writes. buildFileForContract inherits it, because it calls pathForContract before touching any cheatcode. The residual the earlier shape carried — pathForContract("..") still returning an escaping string — no longer exists.

The three fuzz properties #36 landed are kept, and kept meaningful. testPathForContractStructure, testPathForContractDistinctNamesDistinctPaths and testPathForContractIsRelative now take a bytes memory seed and CONSTRUCT identifiers via nameFromSeedSlow rather than filtering arbitrary strings with vm.assume — filtering would have left them passing over a domain they almost never reach, which is the failure this repo spent the day auditing. They still exercise the boundary: mutants on char >= 0x31 and on the isDigit && i > 1 rule are killed by those three tests specifically.

LibContractName.sol is gone and LibCodeGen.requireContractName is the single definition, which resolves the collision this PR had with #54 — two InvalidContractName errors with different selectors, both compiling. LibFs already imports LibCodeGen, so there is no cycle.

22 mutants, 20 killed, 2 survived, 0 no-run, 0 harness errors, and both survivors are named rather than absorbed:

• vm.createDir(GENERATED_DIR, true) to vm.isDir(...) — verified by hand instead: with src/generated moved aside, testBuildFileForContractFreshPath passes; with it absent and the mutant applied it fails with buildFileForContract cannot bootstrap a repo that has never generated: vm.writeFile does not create src/generated #40's exact error. forge test cannot produce the absent-directory state without racing parallel suites against the committed artifact.
• vm.removeFile(path) to vm.exists(path) — the campaign's known survivor, subject of buildFileForContract's remove-then-write is unobservable, and its NatSpec credits it with idempotency that writeFile already provides #41, unobservable without ffi.

The placement mutant is worth recording: moving the check back to buildFileForContract is killed by exactly testPathForContractRejectsNamedEscapes and testPathForContractRejectsEveryNonIdentifierName, confirmed by hand over the full suite at 121 passed / 2 failed. No write-side test notices. The stronger placement is held only by the coverage added for it.

#41 keeps the unlink and deletes the idempotency claim — the NatSpec now says what it actually does, a symlink at the path replaced rather than written through, with the path absent between unlink and write. It stays because with the name confined to one segment it is the last thing preventing a write landing outside src/generated, since foundry normalises permission paths lexically and a symlink inside a granted directory is not caught.

110 to 123 tests, 16 suites, 0 failures. All four checks green. CodeRabbit reports Review rate limited, so the zero threads behind that green is not review coverage.