Uh oh!
There was an error while loading. Please reload this page.
Point the generated-lib writers at a directory they are handed - #137
Conversation
`writeAliasLib` and `writeReleasedSuitesLib` derived their output path from `src/lib/` alone, so the only way to drive either was to overwrite a committed source file. Four tests in `LibRainDeploySnapshotTest` did that, two per path, and forge runs the tests in a contract concurrently. Both now take `libDir`, as `writeReleasedSuitesLib` already took `recordRoot`. `LIB_DIR` is the directory a build passes. The writer tests take a directory each under `fixture-lib/`, outside both compiled trees. What the committed libs hold is asserted by reading them and writing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Warning Review limit reached
Next review available in:55 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)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughGenerated library writers now accept explicit output directories. Build generation passes those directories, and tests write to isolated ChangesLibrary output isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk:⚪ Minimal · up to The change redirects generated-library writes to caller-provided directories and separates committed-file checks from write tests; targeted stress and mutation checks pass, and no actionable merge-blocking risk remains beyond normal checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
`#139` and `#140` landed under this branch, and `#126`'s aggregate writer came with them. Four files conflicted. - `foundry.toml`: both sides add the SAME `./fixture-lib` fs_permissions entry, predicted by this PR's "Not in this PR" note. One entry, with a comment covering all three writers that are pointed there. - `.soldeerignore`: both sides add `/fixture-lib`, in different positions. Deduped, keeping the sorted one. - `src/lib/LibRainDeploySnapshot.sol`: main introduced `LIB_DIR`, `pathForLib` and `releasedLibraryName` for the aggregate writer, so this branch's own `LIB_DIR` is a second declaration of main's and its hand-concatenated paths are main's helper spelled twice. Main's are kept, and the two writers this PR parameterises now build their path with `pathForLib(libDir, libraryName)`. The aggregate writer's NatSpec claimed the other two writers "take no directory and are always `LIB_DIR`", which this PR makes false; it now says they take the same `libDir`. - `test/script/Build.t.sol`: both sides reworded one paragraph. This branch's reason (both entry points rewrite committed files other contracts read) with main's conclusion (nothing below writes anything, which is what main's new reading tests made true). - `test/src/lib/LibRainDeploySnapshot.t.sol`: main added `generatedFilePrefix()` and the aggregate emitter tests where this branch deleted `testWriteReleasedSuitesLibWritesTheLibAtItsPath`. All of main's is kept and the deletion stands — that test wrote the committed `src/lib/LibAddressRegistryReleased.sol`, which is the race this PR removes, and `testTheCommittedReleasedLibIsWhatTheGeneratorEmits` carries its staleness half without writing. The three tests this PR added spell the generated header out literally; they use `generatedFilePrefix()` now. `testTheCommittedAggregateIsWhatTheGeneratorEmits` cited the deleted test by name and now cites its replacement. Return style is main's throughout: unnamed `returns (...)` with an explicit `return`, per #129 and the #140 sweep. Nothing added here names a return, and `forge build --force` emits no solc warning at all. `nix develop -c forge test`: 275 passed / 52 failed, every failure `*_RPC_URL not found`; `grep '[FAIL' | grep -vc '_RPC_URL. not found'` = 0. `git status` clean after the run and `fixture-lib/` empty, so no test writes a committed file. `nix develop -c forge fmt --check` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two notes the merge earned, neither of them behaviour. `writeReleasedSuitesLib` builds its body into a local inside a block rather than inlining it into the `writeFile` call the way `writeAliasLib` and `writeReleasedSuitesAggregate` do. That reads as gratuitous and the next person to tidy it will inline it: the `libDir` parameter puts the function one local past its frame, and solc 0.8.25 without `--via-ir` refuses the flat form as stack too deep. Measured, not guessed — the inlined form was compiled and is the error quoted. `testTheCommittedLibsAreInTheLibDir` said "both writers" when there are now three, and left a reader wondering where the aggregate's committed path is pinned. It is pinned by `testTheCommittedAggregateIsWhatTheGeneratorEmits`, which reads the file through `pathForLib` and asserts the path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#138 landed after the previous merge, replacing `LIB_FS_ROOT` with the `recordRoot()` hook while this branch adds `LIB_DIR`. The one conflict, in `script/Build.sol`, passes both: the writer takes the directory this branch hands it AND the record root #138 made a hook, matching the five-argument `writeReleasedSuitesLib(Vm, libDir, recordRoot, contractName, template)` overload. No `libDir()` hook was added to `BuildScript` — `regenerateLibs()` is fully abstract, so the concrete repo picks `LIB_DIR`. That would be a design change rather than a merge resolution. Verified: build clean, `forge fmt --check` exit 0, 280 passed / 52 failed with every failure `*_RPC_URL not found`, `git status` clean and `fixture-lib/` empty before and after.
Uh oh!
There was an error while loading. Please reload this page.
Closes#135
The defect
writeAliasLibandwriteReleasedSuitesLibbuilt their output path out of ahard-coded
src/lib/and the contract name. The only way to drive either wastherefore to overwrite a COMMITTED source file, and four tests in
LibRainDeploySnapshotTestdid exactly that — two oversrc/lib/LibAddressRegistryDeploy.sol, two oversrc/lib/LibAddressRegistryReleased.sol— each reading abefore, writing, andrestoring. forge runs the tests in a contract concurrently, so a read landing
between another test's truncate and its write hands one test an EMPTY
before,which it then commits back to the committed file.
Measured on
main(ed91bfa, i.e. after #131 and #134 landed)100 runs per pair,
nix develop -c forge test --mt <pair>,git checkout -- src/between runs so one corruption cannot be counted twice. Every runcompiled and executed both of its tests, so no result here is a filter that
matched nothing.
testWriteAliasLib*testWriteReleasedSuitesLib{WritesTheLibAtItsPath,DefaultsToTheOrgHeader}The issue measured the alias pair and stated the released pair was the same
shape but unmeasured. It is the same shape and it does fail — at 4%, not 26%.
Every released-pair failure is the same signature the alias pair produces,
assertion failed: != // SPDX-License-Identifier: ...: an EMPTY string readwhere the committed file should be. No run in either 100 left a file zeroed on
disk; the issue's run 2 did, and both pairs can, because whether the corruption
survives depends only on which test restores last.
The fix
Both writers take
libDir, exactly aswriteReleasedSuitesLibalready tookrecordRootand for the same reason: a writer that can only be pointed at thecommitted tree can only be tested by overwriting it.
LibRainDeploySnapshot.LIB_DIRis the directory a build passes, and
script/Build.solpasses it.The two things the old tests conflated are now separate:
testTheCommittedAliasLibIsWhatTheGeneratorEmitsandtestTheCommittedReleasedLibIsWhatTheGeneratorEmitsread the committed fileand write nothing. This is the staleness check, and it is stronger without the
write: the old alias test asserted
emitted == before, i.e. compared a fileagainst itself around a write, which is green on a stale file as long as the
writer is deterministic.
testWriteAliasLibWritesTheLibAtItsPathand
testWriteReleasedSuitesLibReadsTheRecordItIsHandedwrite intofixture-lib/<test>/, one directory per test, againstMockDeployableratherthan this repo's own contract so the derived library name in the path is
actually asserted to be derived.
fixture directory each.
fixture-lib/is outsidesrc/andtest/because both are compiled and agenerated lib imports
../generated/and../abstract/, which resolve fromsrc/liband nowhere else — the copy a failing test deliberately leaves behindwould otherwise break every suite. It is gitignored and
.soldeerignored.testWriteReleasedSuitesLibWritesTheLibAtItsPathis deleted rather thanconverted: with the committed-file half split out, what remained was
testWriteReleasedSuitesLibReadsTheRecordItIsHandeda second time.testWriteReleasedSuitesLibReadsTheRecordItIsHandedwas a fifth writer intosrc/lib/(LibMockDeployableReleased.sol, uncommitted, removed before itsassertions). It raced nothing, but it is the same hazard — a generated lib under
a compiled root — so it moved too.
LIB_DIRis only read byscript/Build.sol, which nothing may execute, sotestTheCommittedLibsAreInTheLibDirholds it equal to the directory thecommitted libs are actually in.
Measured on this branch
Same harness, 100 runs of every test that drives either writer
(
testWriteAliasLib|testWriteReleasedSuitesLib|testTheCommitted):fixture-lib/The seven are
testTheCommittedAliasLibIsWhatTheGeneratorEmits,testTheCommittedLibsAreInTheLibDir,testTheCommittedReleasedLibIsWhatTheGeneratorEmits,testWriteAliasLibDefaultsToTheOrgHeader,testWriteAliasLibWritesTheLibAtItsPath,testWriteReleasedSuitesLibDefaultsToTheOrgHeaderandtestWriteReleasedSuitesLibReadsTheRecordItIsHanded.Suite
nix develop -c forge test: 261 passed / 51 failed, every failure*_RPC_URL not found.grep '[FAIL' log | grep -vc '_RPC_URL. not found'= 0.nix develop -c forge fmt --checkexit 0.mainated91bfafor comparison: 259 passed / 51 failed. Net +2 tests: threeadded (
testTheCommittedLibsAreInTheLibDir,testTheCommittedAliasLibIsWhatTheGeneratorEmits,testTheCommittedReleasedLibIsWhatTheGeneratorEmits), one deleted.Adversarial mutation pass
Whole suite per mutant, never a filter: a filter that pulls in a bytecode or
CREATE2-address pin reports KILLED for every source mutation and measures
nothing. Each mutant is proven applied (
git diff --numstatnon-empty) andproven to have RUN (
compileFail=0, 363-365[PASS]/[FAIL]result linesagainst the baseline's 363). KILLED = a failure beyond the 51
*_RPC_URLbaseline. Raw logs:
.scratch/mutants/in the working clone,summary.txtcarries the per-mutant counts.
Baseline (unmutated):
compileFail=0, 363 result lines, 0 non-RPC failures.LIB_DIR = "src/lib"->"src/libs"testTheCommittedLibsAreInTheLibDirwriteAliasLibpath usesLIB_DIRinstead of thelibDirit was handedtestWriteAliasLibWritesTheLibAtItsPathwriteReleasedSuitesLibpath usesLIB_DIRinstead of thelibDirit was handedtestWriteReleasedSuitesLibReadsTheRecordItIsHandedwriteAliasLib5-arg forwardslibDir + "x"testWriteAliasLibDefaultsToTheOrgHeader,testWriteAliasLibWritesTheLibAtItsPathwriteReleasedSuitesLib5-arg forwardslibDir + "x"testWriteReleasedSuitesLibDefaultsToTheOrgHeadertestWriteReleasedSuitesLibReadsTheRecordItIsHanded"\n"between header and bodytestWriteReleasedSuitesLibReadsTheRecordItIsHandedwriteAliasLibpassesfilePrefix(copyrightText, spdxLicenseIdentifier)testWriteAliasLibWritesTheLibAtItsPathaliasLibraryBlockexports_DEPLOYED_ADDRrather than_DEPLOYED_ADDRESStestAliasLibraryBlockReExportsBothPins,testTheCommittedAliasLibIsWhatTheGeneratorEmitswriteAliasLibderivesLib<Contract>DeployedtestWriteAliasLibWritesTheLibAtItsPathLibAddressRegistryDeploy.soldrifts one word from the emitterstestTheCommittedAliasLibIsWhatTheGeneratorEmitsLibAddressRegistryReleased.soldrifts one word from the emitterstestTheCommittedReleasedLibIsWhatTheGeneratorEmits12 applied, 12 killed, 0 survived.
Mutants 11 and 12 are the point of the split: they are the staleness property
the four racing tests were carrying, and they are killed by tests that write
nothing. Mutant 8 is killed only by the writer test, not by the committed one —
correct, because the committed test never calls the writer.
Not in this PR
src/lib/LibMigrationRegistryDeploy.solandsrc/lib/LibMigrationRegistryReleased.solhave no committed-is-current check.Neither did they before: the deleted tests only ever covered
AddressRegistry. Pre-existing gap, unrelated to the race.script/Build.sol'sregenerateLibs()is still executed by nothing, so theLIB_DIRargument it passes is uncovered at that call site. That is thepre-existing state
BuildTest's NatSpec describes, not something this PRintroduced.
fixture-libroot andfs_permissionsentry for the aggregate writer; the two will conflict textually and agree
semantically, and whichever lands second resolves it.
QA
testWriteAliasLibWritesTheLibAtItsPath,testWriteAliasLibDefaultsToTheOrgHeader,testWriteReleasedSuitesLibDefaultsToTheOrgHeader,testWriteReleasedSuitesLibReadsTheRecordItIsHanded,testTheCommittedAliasLibIsWhatTheGeneratorEmits,testTheCommittedReleasedLibIsWhatTheGeneratorEmits,testTheCommittedLibsAreInTheLibDir. They cannot be compiled against base —base has neither
libDirnorLIB_DIR— so "fails on base" is shown twoother ways: the base tests these replace fail on base at 26/100 and 4/100 runs
(measured above, both pairs, 100 runs each, every run executing both tests),
and every one of the seven kills at least one mutant in the table above.
line -> mutation -> killing test, the applied-proof (
git diff --numstatnon-empty per mutant) and the ran-proof (
compileFail=0and 363-365 resultlines per mutant against the unmutated baseline's 363 with 0 non-RPC
failures). The whole suite runs per mutant rather than a filter, because a
filter that pulls in a bytecode or CREATE2-address pin reports KILLED for
every source mutation.
copyright line,
pragma, the autogenerated banner — and only the two blockemitters are called, each of which has its own test with a fully literal
oracle (
testAliasImportBlockAliasesBothPins,testAliasLibraryBlockReExportsBothPins). The path oracle is spelled out(
<libDir>/LibMockDeployableDeploy.sol), not derived from the writer. Therace oracle is the process itself: the file on disk after the run, and
git statusoversrc/.(a) the committed-file assertion split out so it needs no write, (b) a fixture
directory per writer test, (c) that directory outside
src/andtest/, and(d) the
libDirparameter the writers lack. All four are covered. Thecategory is wider than the four named tests —
testWriteReleasedSuitesLibReadsTheRecordItIsHandedwas a fifth writer intosrc/lib/, so it moved too. The issue's unmeasured claim about the releasedpair was measured before being fixed: 4/100, same signature, same shape.
Summary by CodeRabbit
Bug Fixes
Tests
Chores