Reach the per-tag frozen-snapshot layout through LibFs

[thedavidmeister]

Closes #78

What this adds

LibFs could not produce the path the org's release convention uses.
requireContractName refuses any name carrying a /, and pathForContract's
NatSpec states every path it returns is a direct child of GENERATED_DIR, so
src/generated/<tag>/<Contract>.sol — the layout frozen-snapshots-append-only
in the shared CI exists to police — was unreachable through the library.

Added to src/lib/LibFs.sol:

• error InvalidTag(string tag) — a distinct error type, not InvalidContractName.
• requireTag(string tag) — the tag rule.
• dirForTag(string tag) → src/generated/<tag>.
• pathForTaggedContract(string tag, string contractName) → src/generated/<tag>/<Contract>.sol.
• buildFileForTaggedContract(Vm, address, string tag, string contractName, string body).

pathForContract and buildFileForContract are untouched. isPresent, which
main added while this branch was open, is also untouched — but
buildFileForTaggedContract now uses it for the same reason main's untagged
write does. That is a semantic conflict a textual merge does not surface, so it
has its own section below.

Where the issue's proposed fix was wrong

The issue proposes LibCodeGen.requireContractName(tag) and states "numeric tags
such as 0_1_1 are already identifiers under the existing rule". They are
not. requireContractName admits a digit only at i > 0, so 0_1_1 reverts —
and every release tag the org freezes opens with a digit. The proposed fix would
have compiled, passed a naive test, and rejected 100% of real frozen snapshot
tags: the exact thing the issue exists to make reachable.

rainix-static/src/frozen_snapshots.rs::is_tag is the authority on what a tag is:

/// True if `seg` is a release-tag dir name: three `_`-separated non-empty numeric
/// parts, e.g. `0_1_4` or `12_0_255`.

So requireTag is a separate rule: at least one character, each of them an
ASCII letter, a digit, _ or $. That is the Solidity identifier alphabet with
the leading-digit restriction dropped, which is the only difference that matters —
a tag names a directory, not a declaration. It admits 0_1_1, 12_0_255 and the
rolling candidate directory a deploy repo keeps beside the frozen ones, and it
still contains no /, no \, no . and no NUL, so no tag is . or .. and no
tag can add or remove a path segment.

Two further deviations from the issue's sketch:

• A distinct error type. 0_1_1 is a valid tag and an invalid contract
  name — the same string, two verdicts. Reporting both through
  InvalidContractName would make a build failure unable to say which argument
  was wrong. InvalidTag is exported from LibFs.sol, where the rule lives.
• dirForTag as well. buildFileForTaggedContract needs the directory to
  createDir, and the live consumer already hand-builds
  string.concat("src/generated/", deployTag()) for createDir and readDir.
  Putting the check on the directory too means every string this library hands a
  caller for its own IO carries the confinement, not just the file path.

Also correcting the issue's factual claim about the live consumer: see the
"consumer migration" section below. S01-Issuer/st0x.deploy does not
reimplement the header or the address-constant emitter — it calls
LibCodeGen.addressConstantString and LibFs.buildFileForContract from
rain-sol-codegen-0.1.3, and the "old" header in its frozen files is that
version's filePrefix() output verbatim. The citation the issue gives for the
reimplementation, addressConstantString at script/BuildPointers.sol:54-60, does
not point at one: at st0x.deploy@main those lines are the NatSpec of
buildContractPointers, and line 73 is a call into this library.

Confinement, which is the point

test/src/lib/LibFs.pathForTaggedContract.t.sol states it over the whole input
domain rather than as a list of escapes. assertConfined checks one path for
every way out at once — it begins with GENERATED_DIR followed by /, it adds
exactly separators(GENERATED_DIR) + 2, no segment is empty, and the only .
anywhere is the appended extension. .. needs a dot, a deeper directory needs a
third separator, and an absolute or doubled-separator path needs an empty
segment, so all of them are excluded together.

testPathForTaggedContractAcceptedArgumentsAreConfined fuzzes each argument as
either arbitrary bytes or a value constructed from its alphabet (chosen by the
fuzzer), so the rejected domain is reached by the raw branches and the accepted
domain — which arbitrary bytes essentially never reach — by the constructed ones.
Nothing is assumed away: a pair that reverts proves confinement by producing no
path at all. testPathForTaggedContractOneBadByteIsConfined covers the
neighbourhood of the accepted domain, which uniform fuzzing never reaches.

QA

• Discriminating tests: testPathForTaggedContractProducesTheOrgLayout,
  testPathForTaggedContractAcceptedArgumentsAreConfined,
  testPathForTaggedContractOneBadByteIsConfined, testRequireTagMatchesAlphabet,
  testRequireTagEveryLeadingByte, testRequireTagEveryTrailingByte,
  testRequireTagAcceptsEveryNumericReleaseTag,
  testRequireTagAcceptsLeadingDigitContractNameDoesNot,
  testDirForTagStructure, testPathForTaggedContractStructure,
  testBuildFileForTaggedContractCreatesTheTagDir,
  testBuildFileForTaggedContractLeavesOtherTagsAlone,
  testBuildFileForTaggedContractReplacesDanglingSymlink (+ 43 more, 56 new in
  total) — none of the first 55 can pass on base, because requireTag,
  dirForTag, pathForTaggedContract and buildFileForTaggedContract do not
  exist there. The failure was observed on base in the form the API allowed,
  transcribed under "TDD" below: [FAIL: InvalidContractName("0_1_1/StoxReceipt")].
  The 56th, …ReplacesDanglingSymlink, is the merge with main and has its own
  observed red against the tagged write as this branch first wrote it,
  transcribed under "TDD: the semantic conflict with main".
• Mutations applied: 25 mutants over every added line, run with mutation-probe,
  full matrix with the killing tests per mutant transcribed under "Mutation
  matrix" below. 25/25 KILLED — 0 survived, 0 no-run, 0 harness errors, probe
  exit code 0, baseline re-verified green (198 passed) before the pass, on the
  merged tree.
• Suite and formatting: nix develop -c forge test → 198 passed, 0 failed, up
  from 142 on main at the merge point (b422d97); nix develop -c forge fmt --check → exit 0. Both transcribed under "After the fix".
• Oracle: LibCodeGenSlow.isTagSlow / tagFromSeedSlow decide tag membership by
  scanning SLOW_TAIL_ALPHABET, which is spelled out character by character,
  against requireTag's range arithmetic — the two sides never share a
  derivation. Paths are asserted positionally against byte literals
  ("src/generated/", "/", ".sol") and against the argument bytes, never by
  re-running the library's own string.concat. File content is asserted against
  the literal SPDX/pragma/header text and address.codehash, not by calling
  filePrefix() / bytecodeHashConstantString(). The tag shapes that must be
  accepted come from rainix-static/src/frozen_snapshots.rs::is_tag and from
  S01-Issuer/st0x.deploy's committed src/generated/ tree, not from this
  library.
• Category check: The per-tag frozen-snapshot layout the org enforces is unreachable through this library #78 asks for (A) pathForTaggedContract, (B)
  buildFileForTaggedContract beside buildFileForContract, (C) the confinement
  invariant that no tag or name traverses out of GENERATED_DIR. Covered A, B, C.
  The issue's fourth claim — that requireContractName(tag) suffices because
  numeric tags are already identifiers — is refuted rather than implemented; see
  "Where the issue's proposed fix was wrong". The issue's fifth element,
  migrating S01-Issuer/st0x.deploy, is deliberately NOT done: reported under
  "Consumer migration" with the blocker that makes it a human ruling.

TDD: the failing test, before the fix

test/src/lib/LibFs.pathForTaggedContract.t.sol first asserted the org layout
against the API as it stood:

assertEq(LibFs.pathForContract("0_1_1/StoxReceipt"), "src/generated/0_1_1/StoxReceipt.sol");

nix develop -c forge test --match-path 'test/src/lib/LibFs.pathForTaggedContract.t.sol' -vvv:

Ran 1 test for test/src/lib/LibFs.pathForTaggedContract.t.sol:LibFsPathForTaggedContractTest
[FAIL: InvalidContractName("0_1_1/StoxReceipt")] testPathForTaggedContractProducesTheOrgLayout() (gas: 929)
Traces:
  [929] LibFsPathForTaggedContractTest::testPathForTaggedContractProducesTheOrgLayout()
    └─ ← [Revert] InvalidContractName("0_1_1/StoxReceipt")

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 244.54µs
Ran 1 test suite in 15.67ms: 0 tests passed, 1 failed, 0 skipped (1 total tests)

TDD: the semantic conflict with main

main landed LibFs.isPresent while this branch was open. vm.exists answers
for whatever a path resolves to, so it reports a symlink whose target does not
exist as absent; isPresent also asks vm.readLink, which answers for the path
itself. main swapped buildFileForContract's unlink guard to isPresent for
exactly that reason.

The merge is textually conflicted in LibFs.sol only around isPresent's
placement, but the semantic conflict is elsewhere and no marker points at it:
buildFileForTaggedContract, added on this branch, guarded its unlink with
vm.exists, so a dangling symlink at the tagged path was reported absent, the
unlink was skipped, and vm.writeFile followed the link and created the
target — while the function's own NatSpec claimed a symlink there is
replaced. A textual merge resolves clean and ships that.

testBuildFileForTaggedContractReplacesDanglingSymlink (added to main's
test/src/lib/LibFs.isPresent.t.sol) asserts the guarantee at the tagged path.
Against the tagged write as this branch first had it:

Ran 1 test for test/src/lib/LibFs.isPresent.t.sol:LibFsIsPresentTest
[FAIL: the write followed the link to its target] testBuildFileForTaggedContractReplacesDanglingSymlink() (gas: 119585)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.81ms (4.56ms CPU time)

The tagged write now guards with isPresent(vm, path), matching the untagged
write, and its NatSpec is aligned with main's wording. M25 in the matrix below
is that guard put back to vm.exists; it is KILLED.

After the fix

nix develop -c forge test on the merged tree — 198 passed, 0 failed, up from
142 on main at the merge point b422d97 (56 new tests, 0 pre-existing tests
changed):

Ran 15 tests for test/src/lib/LibFs.requireTag.t.sol:LibFsRequireTagTest
Ran  7 tests for test/src/lib/LibFs.dirForTag.t.sol:LibFsDirForTagTest
Ran 15 tests for test/src/lib/LibFs.pathForTaggedContract.t.sol:LibFsPathForTaggedContractTest
Ran 18 tests for test/src/lib/LibFs.buildFileForTaggedContract.t.sol:LibFsBuildFileForTaggedContractTest
Ran  7 tests for test/src/lib/LibFs.isPresent.t.sol:LibFsIsPresentTest
...
Ran 22 test suites in 2.33s (19.38s CPU time): 198 tests passed, 0 failed, 0 skipped (198 total tests)

nix develop -c forge fmt --check — clean, exit 0.

Mutation matrix

Run with mutation-probe (rainlanguage/adversarial-mutation-test) rather than a
hand-rolled loop, so a zero-match "mutation" or a suite that never ran cannot read
as SURVIVED. Baseline verified green (198 passed) before any probe; every mutant's
target verified to match exactly once; every restore verified byte-exact. Suite
command per verdict: rm -rf cache/fuzz/failures && nix develop -c forge test;
proof-of-run regex (\d+) tests passed, (\d+) failed read from forge's own
tally. The fuzz-failure cache is dropped before every verdict because forge
replays counterexamples ahead of searching, which otherwise lets one mutant be
killed on evidence discovered under the previous one.

== 25/25 killed; survived: 0; no-run: 0; harness errors: 0

The probe names up to five killing tests per mutant; the first is transcribed
here. This column is regenerated from the pass that ran on the merged tree, not
carried over. Which tests kill a given mutant is not stable across runs — the
fuzz tests draw different inputs each pass, so a mutant killed by five tests one
pass can be killed by a different five the next. Only the verdict is stable.

#	Mutation	Verdict	Killed by
M01	requireTag drops the empty-tag rejection	KILLED	testBuildFileForTaggedContractRejectsEmptyTag
M02	requireTag uppercase range opens one early (admits @)	KILLED	testRequireTagEveryLeadingByte
M03	requireTag uppercase range closes one late (admits [)	KILLED	testRequireTagEveryLeadingByte
M04	requireTag lowercase range opens one early (admits `)	KILLED	testRequireTagEveryLeadingByte
M05	requireTag lowercase range closes one late (admits {)	KILLED	testRequireTagEveryLeadingByte
M06	requireTag digit range opens one early (admits /)	KILLED	testRequireTagEveryLeadingByte
M07	requireTag digit range closes one late (admits :)	KILLED	testRequireTagEveryLeadingByte
M08	requireTag drops digits from the tag alphabet	KILLED	testDirForTag
M09	requireTag collapses into the contract-name rule (no leading digit)	KILLED	testDirForTag
M10	requireTag drops _ from the tag alphabet	KILLED	testDirForTag
M11	requireTag drops $ from the tag alphabet	KILLED	testBuildFileForTaggedContractBodyVerbatim
M12	requireTag checks only the first byte of the tag	KILLED	testRequireTagErrorCarriesTheTag
M13	dirForTag drops the tag check	KILLED	testBuildFileForTaggedContractRejectsEmptyTag
M14	dirForTag drops the separator before the tag	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink
M15	pathForTaggedContract drops the contract-name check	KILLED	testBuildFileForTaggedContractRejectsNameEscapes
M16	pathForTaggedContract drops the separator before the name	KILLED	testBuildFileForTaggedContractWritesToPathForTaggedContract
M17	pathForTaggedContract drops the appended extension	KILLED	testBuildFileForTaggedContractWritesToPathForTaggedContract
M18	pathForTaggedContract ignores the tag, returns the untagged path	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink
M19	pathForTaggedContract checks the name before the tag	KILLED	testPathForTaggedContractRejectsTheTagFirst
M20	buildFileForTaggedContract drops the tag-directory creation	KILLED	testBuildFileForTaggedContractBodyVerbatim
M21	buildFileForTaggedContract creates the tag directory non-recursively	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink
M22	buildFileForTaggedContract creates the generated dir, not the tag dir	KILLED	testBuildFileForTaggedContractBodyVerbatim
M23	buildFileForTaggedContract removes the file without checking it exists	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink
M24	buildFileForTaggedContract writes to the untagged path	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink
M25	buildFileForTaggedContract guards the unlink with vm.exists, not isPresent	KILLED	testBuildFileForTaggedContractReplacesDanglingSymlink

M25 is the semantic conflict with main stated as a mutant: it is the code this
branch had before the merge, and …ReplacesDanglingSymlink is the only test in
the whole 198 that kills it.

What M21 actually proves, and what it does not. It flips recursive from
true to false on vm.createDir(dirForTag(tag), true). A non-recursive
createDir fails on two conditions — a missing parent, and a directory that is
already there — and all five tests that kill it in this pass kill it on the
second: …ReusesAnExistingTagDir and …ReplacesExistingContent open with
vm.createDir(LibFs.dirForTag(tag), true), …ReplacesDanglingSymlink mkdir -ps
the tag directory to put the symlink in it, and …Idempotent and
…LeavesSiblingsAlone each call the function twice for one tag. So the flag is
covered for the behaviour it has at this call site:
generating a second contract into a snapshot that already exists must not fail.
The parent-creating half is not reachable from dirForTag, whose only possible
parent is GENERATED_DIR — committed in this repo and written concurrently by
every sibling suite, so no test can make it absent. #112 reaches that half for
buildFileForContract by making the directory an argument; there is no
equivalent for a directory the tag alone determines.

Why the first pass of this probe was not the matrix above. Two of these
tests wrote a sentinel that is not Solidity (UNTAGGED, and STALE repeated) to
a .sol path under src/, and removed it on the way out. A mutant that makes
the write revert skips that removal, so the residue survived into the next
mutant's compile and M21-M24 came back NO-RUN — unscorable, and one keystroke
away from reading as "survived". Both sentinels are now formatted Solidity
comments, which is a distinct content for the assertions either way, so a run
that dies part way through leaves a tree that still compiles and still passes
forge fmt --check. Checked, not assumed: after the pass-3 run above, with all
32 residue paths the probe left in src/generated/ still in place,
nix develop -c forge fmt --check and nix develop -c forge build both exit 0.
test/lib/LibFs.buildFileForContract.t.sol on main writes its STALE sentinel
the same way; that one is untouched here and noted below.

Consumer migration — reported, not done

No consumer is touched by this PR. S01-Issuer/st0x.deploy is a live consumer of
the per-tag layout — it is the one this was read against, at main — and
migrating it is not a call-site swap. It is not established to be the only
one: every gh search code query tried for the layout returned zero results, so
the org-wide sweep never produced an answer either way, and there may be
consumers this did not find.

What is actually there today, correcting #78's description of it:

• script/BuildPointers.sol imports rain-sol-codegen-0.1.3 and calls
  LibFs.buildFileForContract(vm, deployed, string.concat(deployTag(), "/", name), …)
  and LibCodeGen.addressConstantString(…). The pointer files are library
  output, not hand-rolled. The header in the frozen files
  (// THIS FILE IS AUTOGENERATED BY ./script/BuildPointers.sol plus the
  circular-dependency paragraph) is filePrefix() at sol-v0.1.3 verbatim.
• What is hand-rolled there is the deploy-lib generation — genV4 /
  genCurrent emitting src/generated/LibProdDeployV4.sol and
  LibProdDeployCurrent.sol line by line with vm.writeLine under their own
  header. That is an alias/index library, a shape this library has no surface for
  at all, and nothing in this PR changes that.

What migrating the pointer files would involve:

1. The extension changes and that is the blocker. pathForContract at
   0.1.3 appended .pointers.sol; it appends .sol today, and
   pathForTaggedContract inherits that. All 22 committed pointer files would be
   renamed. The 12 under candidate/ rename freely. The 10 under 0_1_1/ are
   frozen, and frozen_snapshots::check diffs with --no-renames precisely so a
   rename surfaces as D + A and the D is flagged. So the frozen snapshot
   cannot be renamed without a human ruling on the append-only rule.
2. Regenerating the frozen snapshot is blocked for the same reason, and
   independently: the current filePrefix() differs from 0.1.3's, so any
   regeneration rewrites those 10 files byte for byte. The append-only rule's
   answer is that they are never regenerated — which leaves two header formats
   coexisting in the repo, correctly.
3. Every import … from "./<tag>/<Name>.pointers.sol" line genV4 emits, and
   the pointerExists probe, carry the extension and move with it.
4. Only then is the call-site swap itself trivial:
   buildFileForContract(vm, addr, string.concat(tag, "/", name), body) →
   buildFileForTaggedContract(vm, addr, tag, name, body), and the explicit
   vm.createDir(string.concat("src/generated/", deployTag()), true) becomes
   redundant.

candidate needs no special handling: it is letters only, so requireTag
accepts it.

The honest summary is that this PR makes the layout reachable, and a migration
of st0x.deploy is a separate piece of work whose first question — whether a
frozen snapshot may be renamed — is a human ruling, not a code change.

Also found, not touched

• Overlap with Cover the generated directory being created, by making the directory injectable #112. That PR adds a five argument
  buildFileForContract(vm, instance, dir, contractName, body) overload whose
  dir is deliberately unchecked, on the reasoning that fs_permissions is
  what confines it. If both land, LibFs offers two ways to reach
  src/generated/<tag>/ — one that checks the directory segment and one that
  does not — and the unchecked one is the more convenient. Whether that is the
  intended end state is a design call for a human, not something this PR should
  settle on a sibling's branch. Flagged, not touched.
• testRequireTagErrorCarriesTheTag may be the test main just deleted. In
  the same window, main removed testRequireContractNameErrorCarriesTheName
  from test/lib/LibCodeGen.requireContractName.t.sol on the reasoning that
  assertRejected already asserts the whole error — selector and argument — so
  every rejection in the file pins that claim and a separate test for it is a
  second surface for one fact. This branch's LibFs.requireTag.t.sol has the
  same shape: its assertRejected pins InvalidTag.selector, tag, and
  testRequireTagErrorCarriesTheTag re-states it for one input. It is the only
  mutant killer list in the pass-3 matrix below that names it, M12, and that
  mutant is killed by four other tests as well. It is left in place because adopting a
  sibling's convention change is a reviewer's call, not a merge resolution;
  say the word and it goes.
• test/lib/LibFs.buildFileForContract.t.sol on main writes a STALE sentinel
  that is not Solidity to src/generated/<name>.sol and removes it on the way
  out, so a run that fails before the removal leaves a tree that neither compiles
  nor passes forge fmt --check, and the original failure is then buried under a
  parse error. That is the same defect the two sentinels in this PR's own tests
  had, and it is what made the first pass of this probe unscorable. It is a
  sibling's file, so it is reported here rather than changed.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 58 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: 267d85f7-1952-4efd-9a79-ff0f347f3e76

📥 Commits

Reviewing files that changed from the base of the PR and between b422d97 and ffe7076.

📒 Files selected for processing (8)

• src/lib/LibFs.sol
• test/concrete/LibFsExternal.sol
• test/lib/LibCodeGenSlow.sol
• test/src/lib/LibFs.buildFileForTaggedContract.t.sol
• test/src/lib/LibFs.dirForTag.t.sol
• test/src/lib/LibFs.isPresent.t.sol
• test/src/lib/LibFs.pathForTaggedContract.t.sol
• test/src/lib/LibFs.requireTag.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.}