LibFs: build the generated file content before touching disk - #100
Conversation
`buildFileForContract` unlinked whatever was at the path before it computed the content to write. Building the content reverts for a codeless instance, and cheatcode filesystem effects are not rolled back by the revert, so a failed build deleted the previously generated file and wrote nothing back. Closes #61 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 13 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 (2)
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 |
|
@coderabbitai review |
|
… the run Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#100 rebuilt buildFileForContract so the whole file content is computed into a local before any disk mutation. Resolved onto that ordering rather than restoring compute-at-write: the local is built from filePrefix(spdxLicenseIdentifier, copyrightText), so a refused licence or copyright now reverts before createDir, the unlink and the write, exactly as a codeless instance does. Its docstring paragraph is widened to say so, since the build step this branch adds a revert path to is the one that paragraph is about. #100's testBuildFileForContractFailedBuildKeepsExistingFile is kept and passes the two values like every other call site.
Comment-only. The clause added when resolving onto #100's ordering left the paragraph wrapping mid-sentence at a short line.
Re-sites the orphaned-artifact refusal onto main's current `LibFs`: - `buildFileForContract` is now the six-arg call applied to `GENERATED_DIR` over a seven-arg `dir` overload (#112), builds the whole file content before touching disk (#100), and unlinks in a `while` loop (#127). The check goes into the shared body, after `vm.createDir` because it is a read of that directory, and before the unlink so a refusal leaves the existing artifact where it found it. - `requireNoOrphanedArtifact(vm, contractName)` is that check applied to `GENERATED_DIR`, over a private `requireNoOrphanedArtifactIn`, mirroring `pathForContract` / `pathForContractIn`. The overload reads the directory it writes into rather than always `GENERATED_DIR`. - The test moves from `test/lib/` to `test/src/lib/` (#56), and its calls carry the licence and copyright `filePrefix` now takes (#135). - `InvalidContractName` / `isContractNameSlow` are `InvalidIdentifier` / `isIdentifierSlow`, and forge-std is 1.16.2. - The README's "Generated paths" section anchors ahead of "Formatter requirements": the worked-example section it sat under is gone (#138) and the publish section it appended to was rewritten (#140). Drops the hand-set `[package].version = "0.2.0"` and the README paragraph that justified it. Autopublish owns the version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #61
What was wrong
LibFs.buildFileForContractunlinked whatever was atpathForContract(contractName)as its own statement, before the content it wasgoing to write existed. The content was only built as an argument to
vm.writeFile, and building it callsLibCodeGen.bytecodeHashConstantString,which reverts with
CodelessInstancefor an instance that holds no code.Cheatcode filesystem effects are not rolled back by the EVM revert, so a build
that failed that way deleted the previously generated file and wrote nothing
back. The visible symptom was a missing or drifted committed artifact rather
than the
CodelessInstancethat caused it.The change
src/lib/LibFs.sol: the whole file content is built into a local before thefirst cheatcode, so
createDir, the unlink and the write are all downstream ofevery revert the build can produce. Nothing is created, unlinked or written
unless there is content to write. The docstring states that ordering and why it
is load bearing.
Since #112 landed, the statements this reorders live in the five-arg
buildFileForContract(vm, instance, dir, contractName, body)overload, and thefour-arg function is that overload applied to
GENERATED_DIR. The hoist istherefore made in the overload, which is where the unlink and the write are, and
both entry points get the ordering. The docstring paragraph sits on the four-arg
function, where the behavioural narrative lives and which the overload's
docstring points at as identical in every other respect.
The issue's proposed fix is what landed, adapted only to that re-siting: the
issue's snippet names
GENERATED_DIRandvm.exists(path)where the currenttree has
dirandisPresent(vm, path), both of which are #112's and #127'schanges to the surrounding statements rather than anything this PR touches.
Two things were added on top of the issue's proposed test: an explicit
codeless.code.length == 0precondition, so the test proves it is exercisingthe codeless path rather than assuming
0xdeadis empty, and mutant M04 below,which is what makes the byte-identical assertion load bearing rather than
decorative. The test lives at
test/src/lib/LibFs.buildFileForContract.t.sol,the post-#56 mirror path for a test whose subject is
src/lib/LibFs.sol.QA
testBuildFileForContractFailedBuildKeepsExistingFile- fails with the fix's ordering reverted (M01 below, trace attached), passes with it in placesrc/lib/LibFs.solcontent-before-cheatcodes ordering -> restore the pre-fix ordering (content built inline atvm.writeFile) -> killed bytestBuildFileForContractFailedBuildKeepsExistingFile;vm.writeFile(path, content)->vm.writeFile(path, "")-> killed bytestBuildFileForContractExactContent+ 4 more;if (isPresent(vm, path))guard -> unconditionalvm.removeFile(path)-> killed bytestBuildFileForContractReplacesDanglingSymlink+ 4 more; content assignment -> preceded byvm.writeFile(path, LibCodeGen.filePrefix())-> killed bytestBuildFileForContractFailedBuildKeepsExistingFileandtestBuildFileForContractReplacesDanglingSymlink. 4/4 killed, 0 survived, 0 no-run, 0 harness errors, baseline green at 156 passed before each"PRE-EXISTING"are written by the test itself and asserted back verbatim, and the expected revert isabi.encodeWithSelector(CodelessInstance.selector, address(uint160(0xdead)))built from the error declaration rather than from whatbuildFileForContracthappens to throw. The codeless precondition is asserted (codeless.code.length == 0) rather than assumednix develop -c forge test,nix develop -c forge fmt --check,nix develop -c forge coverageand the mutation pass were all run in a fresh clone of thisbranch merged with
main, forge 1.7.2-nightly (43923a4) from the flake.The fix's ordering reverted
nix develop -c forge test --match-test testBuildFileForContractFailedBuildKeepsExistingFile -vvvwith M01 applied to the merge commit — the content built inline at
vm.writeFileand the hoisted local deleted, which is exactly the orderingmainhas. The trace shows the unlink landing and then the revert, exactly asthe issue describes. The
VM:address is shortened.src/lib/LibFs.solwas restored from git afterwards and the restore verifiedbyte-exact by sha256
(
c0ed3f0ec736a7a9d4d88483476ca7ece571ffe55a1e440f16e363658b30271ebefore andafter).
Passing with the fix
Full suite, formatting and coverage
On the merge commit of this branch with
main:mainalone at that same point runs 155 tests across the same 23 suites, so thedelta is the one test this PR adds and nothing else.
nix develop -c forge fmt --checkexits 0 with no diff reported.nix develop -c forge coverageholdssrc/libat 100% on every column, and thehoisted local is covered rather than merely compiled —
LibFs.solgoes from20/20 lines and 17/17 statements on
mainto 22/22 and 19/19 here:Mutation matrix
Run with
mutation-probe(rainlanguage/adversarial-mutation-test), whichproves the suite ran from its own tally rather than from an exit code. Baseline
green at 156 passed / 0 failed before every mutant, and every mutated file is
restored byte-identical afterwards. Every mutant targets the five-arg overload,
which is where the unlink and the write live.
src/lib/LibFs.soltestBuildFileForContractFailedBuildKeepsExistingFilevm.writeFile(path, content)→vm.writeFile(path, "")testBuildFileForContractBodyVerbatim,…CreatesTheDirectory,…EmptyBody,…ExactContent,…FreshPathif (isPresent(vm, path))guard dropped in front of the unlinktestBuildFileForContractReplacesDanglingSymlink,…BodyVerbatim,…CreatesTheDirectory,…EmptyBody,…ExactContenttestBuildFileForContractFailedBuildKeepsExistingFile,testBuildFileForContractReplacesDanglingSymlinkM01 is the fix's own behaviour, and it is discriminating: it is killed by the
new test and by nothing else in the suite. M02 and M03 are there because
hoisting the content into a local is a silent place to drop it, and because the
existence guard sits between the two statements that moved. M04 leaves a file at
the path but not the bytes that were there, so
assertTrue(vm.exists(path))still passes and only the
assertEqon the content can see it — that is whatmakes the byte-identical half of the new test load bearing rather than
decorative.
The suite provably ran on every one of these: each verdict carries the test
names that failed, and the run reports
no-run: 0; harness errors: 0, so noverdict here is a compile error or a zero-match filter reading as a pass.