Refuse to generate beside another artifact for the same contract

[thedavidmeister]

From the whole-repo audit at 7aa85a4. There is no issue for it, so there is no
Closes line.

What was wrong

LibFs.pathForContract names src/generated/<Contract>.sol. Consumers commit
that file and import it by path from src/**, which is what the library's own
docstring means by "a cross repo contract rather than an internal detail".

It has not always named that file. 7cf0992 ("fix: generated files are not
pointers") moved it from src/generated/<Contract>.pointers.sol, and the
earliest tag containing that commit is sol-v0.1.4 — a patch step, because the
merge-driven autopublish lifecycle only takes patch steps.

Nothing in the library noticed the old file. A consumer that bumps
rain-sol-codegen and re-runs its build script gets a fresh
src/generated/<Contract>.sol written beside its committed
<Contract>.pointers.sol, a success exit, and src/** still importing the old
file — whose BYTECODE_HASH and function-pointer constants are frozen at
whatever the contract was the last time the old library generated it. Nothing
regenerates that file and nothing fails while it sits there.

What it does now

buildFileForContract runs the orphan check before it unlinks or writes
anything at the path. That reverts OrphanedGeneratedArtifact(path) when the
generated directory holds a direct child named for the contract in full, then a
., then anything other than the name pathForContract currently produces.

• The current name is read back out of pathForContract, not respelled. The
  check follows that function wherever it goes: the next time the generated path
  moves, the previous one is refused with nothing here to edit. That is the half
  of this that is structural rather than about one extension.
• The . is the boundary, and it is load-bearing. rainlang holds
  Rainlang.pointers.sol and RainlangInterpreter.pointers.sol in one
  directory; raindex holds RaindexV6.pointers.sol and
  RaindexV6SubParser.pointers.sol. A prefix match without the . refuses each
  of those for the other.
• Only direct children are read. pathForContract never names anything
  deeper, so a generation into src/generated/ never reads inside the
  per-release snapshot directories that raindex, rain.deploy,
  rain.factory.deploy and st0x.deploy keep there, and never refuses one of
  those directories either, because a tag carries no .. Those directories do
  hold artifacts this library wrote — through the dir overload — and a
  generation into one of them reads that directory instead, against its own
  contents.
• It is a hard error, not a warning. A green build is what hid this.

Where it sits in the write

buildFileForContract is now the six-argument call applied to GENERATED_DIR
over a seven-argument dir overload (#112), and the shared body builds the
whole file content before touching disk (#100) and unlinks in a while loop
(#127). Since #137 landed, buildFileForTaggedContract is a third entry point
into the same body, applied to dirForTag(tag). The check goes into that shared
body, between the create and the unlink, so all three inherit it:

vm.createDir(dir, true);
requireNoOrphanedArtifactIn(vm, dir, contractName);
while (isPresent(vm, path)) { vm.removeFile(path); }
vm.writeFile(path, content);

• After the create, because the check is a read of that directory.
• Before the unlink, because cheatcode filesystem effects are not rolled
  back by the revert that follows them. A refusal that landed after the unlink
  would leave a consumer with neither artifact — the current one deleted and the
  orphan still there. testBuildFileForContractRefusesToOrphanAnotherArtifact
  puts a file at the generated path and asserts it is still there, byte for
  byte, after the refusal.

requireNoOrphanedArtifact(vm, contractName) is that check applied to
GENERATED_DIR, over a private requireNoOrphanedArtifactIn(vm, dir, contractName) — the same pairing pathForContract / pathForContractIn
already uses, and for the same reason: the only directory a consumer of this
library writes to is GENERATED_DIR. The overload reads the directory it writes
into rather than always GENERATED_DIR, so the snapshot directories a consumer
generates into are checked against their own contents and not against the live
one.

What it does not reach: renaming a contract (Foo → Bar) orphans Foo.sol
while Bar generates cleanly, and no per-contract API can see that. CI's
artifact-currency step is the right place for that one.

The decision, and the ledger

The finding named two routes. Neither was taken; the third one below was.

Route 1 — restore the .pointers.sol suffix. Build cost: one line. But
every artifact generated since 0.1.4 is already flat — rain.deploy pins 0.1.6
and generates flat artifacts into per-release directories — so the revert
orphans those by the identical mechanism, in the other direction. It therefore
still needs an orphan check to be safe, which is this PR's work plus a second
break. It also re-entrenches a name 7cf0992 argues is wrong: the generated
file is a set of constants, not a function-pointer table.

Route 2 — hardcode the legacy .pointers.sol path as a special case. Build
cost: three lines. Carrying cost: a dead convention named forever inside a live
library, and never provably removable, because soldeer consumers outside these
orgs cannot be enumerated. That is a stopgap by the org's own definition, so it
was not built.

Taken — refuse on any orphaned sibling, keyed to the current name. Build
cost: ~110 lines of library plus the tests below. Carrying cost: one directory
read per generated contract, and no name anywhere that has to be kept updated as
the convention moves. Removal cost, if it is ever wrong: delete the two
functions and the one call site — nothing else depends on them, and while the
check passes it changes nothing about what gets written.

[package].version is not touched

An earlier revision of this branch set [package].version to 0.2.0 by hand
and documented that in the README, on the argument that a consumer-breaking
change needs a minor step the patch-only autopublish lifecycle cannot take.
Both are dropped. Autopublish owns the version in this repo; it is never set by
hand here. This branch leaves foundry.toml untouched at whatever main says.

Consumer migration — reported, not performed

Surveyed: every repo in the eight in-scope orgs of the org registry that has a
commit at all — 272 of 275, the other three empty — read at its default branch
through the GitHub trees API, with each repo's foundry.toml fetched and the
affected repos cloned for their contents.

Fourteen repos keep a src/generated/, holding 42 direct children between
them: 26 files and 16 directories.

• 24 are <Contract>.pointers.sol, across eleven repos. Every one is the
  legacy name for a contract that repo still generates: per repo, the artifact
  count and the number of LibFs.buildFileForContract call sites in its build
  script match one for one, and at each repo's pinned version the library itself
  produced that name. Each is refused on that repo's next generation of that
  contract, once it is on a version carrying this check.
• 2 are already current-shape — S01-Issuer/st0x.deploy's
  LibProdDeployCurrent.sol and LibProdDeployV4.sol — and are accepted.
• 16 are directories, spread over four repos and drawn from the names
  candidate, 0_1_1, and 0_1_3 through 0_1_13. No tag carries a ., so
  none can match a <Contract>. prefix and none is ever refused.

Fourteen repos pin rain-sol-codegen through soldeer: twelve at 0.1.0,
S01-Issuer/st0x.deploy at 0.1.3, rainlanguage/rain.deploy at 0.1.6. The
earliest tag containing 7cf0992 is sol-v0.1.4, so the thirteen pinned below
it still generate .pointers.sol today — their committed artifacts agree with
their pins, and the split only opens when they bump. rain.deploy, above it,
already generates flat: src/generated/candidate/AddressRegistry.sol and
MigrationRegistry.sol.

rainlanguage/rain.uniswap is the one consumer with no pin. It reaches this
library through a nested git submodule, remapping
rain.sol.codegen/=lib/rain.interpreter/lib/rain.sol.codegen/src/ in
foundry.toml and generating from script/BuildPointers.sol. That chain
resolves to d0fbb76 (2024-08-03), which predates 7cf0992 and writes
.pointers.sol, so its committed artifact agrees with it too. Its upgrade is a
submodule bump rather than a soldeer one.

The import surface to repoint is 58 .sol files carrying 158 references to
a .pointers.sol artifact across those eleven repos. None of the 58 is an
artifact referring to itself.

repo	pin	legacy artifacts	.sol files referencing	references
rainlanguage/raindex	0.1.0	6	11	85
rainlanguage/rainlang	0.1.0	6	10	23
rainlanguage/dvin.deploy	0.1.0	3	2	4
rainlanguage/rain.verify	0.1.0	2	5	13
rainlanguage/rain.dia	0.1.0	1	4	4
rainlanguage/rain.erc4626.words	0.1.0	1	11	13
rainlanguage/rain.flare	0.1.0	1	5	5
rainlanguage/rain.math.float	0.1.0	1	2	3
rainlanguage/rain.merkle	0.1.0	1	3	3
rainlanguage/rain.pyth	0.1.0	1	3	3
rainlanguage/rain.uniswap	submodule	1	2	2
total		24	58	158

Where the check does not reach. A further 85 .pointers.sol sit one level
down, inside snapshot directories: raindex 60, S01-Issuer/st0x.deploy 22,
rain.factory.deploy 3. A generation into src/generated/ never reads them. A
generation into one of those directories reads that directory instead and
applies the same rule to it, so a snapshot directory regenerated in place is
refused on its own contents. Frozen release snapshots are not regenerated; the
live case is st0x.deploy's candidate/, which holds 12.

Migrating a repo is: delete the orphan, repoint the imports that named it,
regenerate. None of that is done here. This PR is what makes the build say so
instead of passing.

QA

main moved three times during this branch — to b422d97 (#124), 1c81613
(#135 and the 0.1.32 bump), and b9e7ac7 (#137's tag layout — requireTag,
dirForTag, pathForTaggedContract, buildFileForTaggedContract — and the
0.1.33 bump). Each was merged in, never rebased over, and the suite, the
formatter and the whole mutation matrix were re-run after each. #137 conflicted
in src/lib/LibFs.sol and test/concrete/LibFsExternal.sol; both conflicts are
two additions at the same point and both sides are kept.

The semantic half of that merge is that buildFileForTaggedContract delegates
to #112's dir overload, so it enters the same shared body and inherits this
check without a line here: a tagged generation is checked against its own tag
directory. That inheritance is structural, not incidental: the delegation is a single
line, so there is no second code path to keep in step.

• Discriminating test for the refusal:
  testRequireNoOrphanedArtifactRejectsTheLegacyName, which puts
  <Name>.pointers.sol — the exact shape 24 artifacts across eleven repos are
  in — in the generated directory and asserts
  OrphanedGeneratedArtifact("src/generated/<Name>.pointers.sol").
• Oracle for what is refused: .pointers.sol is one row of a table rather than
  the subject. testRequireNoOrphanedArtifactRejectsEveryOtherSuffix walks ten
  suffixes — pointers.sol, so, soll, SOL, asol, sol0, sol.sol,
  json, pointers, and the empty one — and each must be refused, so a check
  that hardcoded the legacy name would fail nine of them. The accepted side is
  asked of the library rather than spelled out:
  testRequireNoOrphanedArtifactAcceptsTheFileItWrites writes to
  LibFs.pathForContract(name), so whatever that function returns is what must
  survive. testRequireNoOrphanedArtifactRejectsEveryNonIdentifierName(bytes)
  fuzzes the name.
• Category check: the finding is that a generated artifact under an old name
  survives a bump, unregenerated and still imported. Covered on all three of its
  parts — the refusal itself, the ordering that makes the refusal safe (nothing
  is unlinked before it), and the boundary that keeps Foo and FooBar apart.
• Full suite: nix develop -c forge test → Ran 29 test suites in 4.31s (58.91s CPU time): 262 tests passed, 0 failed, 0 skipped (262 total tests).
• nix develop -c forge fmt --check → exit 0.
• Coverage 100% on every src/lib file. LibFs.sol: 100% lines (67/67),
  statements (82/82), branches (9/9), funcs (12/12). Repo total 100% lines
  (153/153), statements (188/188), branches (20/20), funcs (28/28).
• Mutation matrix, run with mutation-probe, which reads the verdict from the
  suite's own tally rather than from an exit code, and re-checks each file
  byte-pristine before the next mutant. Baseline green at 262, then 18/18
  KILLED, 0 survived, 0 no-run, 0 harness errors.

Four mutants cover where the call sits, and the rest cover what it does.

mutation-probe caps each killer list at five names, and forge runs suites in
parallel, so which five it records varies between runs — two passes over the
same tree reported different families for several rows. The names below are the
ones this run recorded, not the complete set of tests that kill each mutant.

#	mutation	killed by (this run)
M01	the call is deleted	testBuildFileForContractChecksTheDirectoryItWritesTo, testBuildFileForContractRefusesToOrphanAnotherArtifact
M02	the check moves after the unlink and the write	testBuildFileForContractRefusesToOrphanAnotherArtifact
M03	the check moves before vm.createDir	testBuildFileForContractReadsTheDirectoryNotItsOwnName
M04	the check reads GENERATED_DIR instead of the directory written to	testBuildFileForContractChecksTheDirectoryItWritesTo, testBuildFileForContractReadsTheDirectoryNotItsOwnName
M05, M06	lastPathSegment keeps the separator / stops recognising it	all four testLastPathSegment*, plus testRequireNoOrphanedArtifactRejectsADirectory
M07, M08	lastPathSegment copies from the front / stops at the first separator rather than the last	the four testRequireNoOrphanedArtifactRejects*, plus testLastPathSegmentAbsolutePath
M09	the current artifact is keyed on the whole path, not its final segment	testRequireNoOrphanedArtifactAcceptsTheFileItWrites, testBuildFileForContractIdempotent, testBuildFileForTaggedContractIdempotent
M10	the prefix drops its ., so a longer name matches	testRequireNoOrphanedArtifactIgnoresOtherContracts, testRequireNoOrphanedArtifactIgnoresTheBareName
M11	the length guard is off by one, so a bare <Name>. is skipped	testRequireNoOrphanedArtifactRejectsEveryOtherSuffix
M12	the length guard is dropped	testRequireNoOrphanedArtifactIgnoresTheBareName
M13	the skip for the artifact this library writes is dropped	testRequireNoOrphanedArtifactAcceptsTheFileItWrites, testBuildFileForContractIdempotent, testBuildFileForTaggedContractIdempotent
M14	the per-byte comparison is inverted	testRequireNoOrphanedArtifactRejectsTheLegacyName and three siblings
M15	a mismatched byte still counts as this contract's artifact	testRequireNoOrphanedArtifactIgnoresOtherContracts, testRequireNoOrphanedArtifactIgnoresShorterContracts
M16	the comparison stops one byte short, so the . is never read	testRequireNoOrphanedArtifactIgnoresOtherContracts, testBuildFileForContractGeneratesWhenNoOtherArtifactExists
M17	the refusal names the bare file instead of its path	testRequireNoOrphanedArtifactRejectsADirectory and three siblings
M18	the read descends below the direct children	testRequireNoOrphanedArtifactIgnoresSubdirectories

M03 is the one that needed a new test. vm.readDir does not revert on a
directory that is not there — it returns one entry whose path is that
directory and whose errorMessage says why — so the two orderings only disagree
where the directory's own final segment reads as an artifact for the contract.
testBuildFileForContractReadsTheDirectoryNotItsOwnName generates
LibFsBuildDirName into a directory called LibFsBuildDirName.snapshot: run
before the create, the check refuses the directory it was about to make; run
after it, the read is of an empty directory and the file is written.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 20 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: 48e45321-3375-40fa-ae0d-2bca729ed22f

📥 Commits

Reviewing files that changed from the base of the PR and between b9e7ac7 and 905640d.

📒 Files selected for processing (6)

• README.md
• src/lib/LibFs.sol
• test/concrete/LibFsExternal.sol
• test/src/lib/LibFs.buildFileForContract.t.sol
• test/src/lib/LibFs.lastPathSegment.t.sol
• test/src/lib/LibFs.requireNoOrphanedArtifact.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.}