From f4d44ea7dc5679522d67779a5478af298d5e053f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 18:36:41 +0000 Subject: [PATCH 1/7] Refuse to generate beside another artifact for the same contract `LibFs.pathForContract` names `src/generated/.sol`. Consumers commit that file and import it by path from `src/**`, so the path is a cross repo contract. A repo holding an artifact for the same contract under any other name got a fresh `.sol` written beside it and a green build, while its imports kept resolving to the file nothing regenerates. `buildFileForContract` now calls `requireNoOrphanedArtifact` before it creates or removes anything, and reverts `OrphanedGeneratedArtifact` when `src/generated` holds a direct child named for the contract in full, followed by a `.` and anything other than the current artifact's name. The current name is read from `pathForContract` rather than respelled, so the check follows that function wherever it goes. Only direct children are read, so per release snapshot subdirectories are untouched. `[package].version` takes the minor step by hand: the autopublish bump is always a patch step and this change breaks consumers holding an artifact at the old path. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 26 +- foundry.toml | 2 +- src/lib/LibFs.sol | 79 ++++++ test/concrete/LibFsExternal.sol | 4 + test/lib/LibFs.buildFileForContract.t.sol | 41 +++- .../lib/LibFs.requireNoOrphanedArtifact.t.sol | 231 ++++++++++++++++++ 6 files changed, 380 insertions(+), 3 deletions(-) create mode 100644 test/lib/LibFs.requireNoOrphanedArtifact.t.sol diff --git a/README.md b/README.md index 959e21e..ccb408e 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,23 @@ Regenerate the committed example artifact under `src/generated/`: forge script script/Build.sol ``` +## Generated paths + +`LibFs.pathForContract` names `src/generated/.sol`, and +`buildFileForContract` writes that file and only that file. Consumers commit it +and import it by path from `src/**`, so the path is a cross repo contract: +moving it, or moving `GENERATED_DIR`, breaks every repo holding the artifact at +the old path. + +`buildFileForContract` therefore refuses to generate while `src/generated/` +holds another artifact for the same contract — any direct child named for that +contract, in full, followed by a `.` and anything other than `sol`. Nothing +regenerates such a file, while the consumer's imports keep resolving to it, so +the build fails with `OrphanedGeneratedArtifact` naming the file rather than +generating beside it. Delete it and repoint the imports at +`src/generated/.sol` in the same commit. Only direct children are +read, so per release snapshot directories under `src/generated/` are untouched. + On top of the above, CI applies rainix's org-wide static checks via [`.github/workflows/rainix.yaml`](.github/workflows/rainix.yaml). @@ -67,7 +84,14 @@ differs from the latest published revision, that workflow pushes `[package].version` to Soldeer, tags `sol-v`, and bumps `[package].version` to the next version. `[package].version` in `foundry.toml` is therefore the next, unpublished version rather than the last published one. -Neither the version nor the tag is set by hand. + +The tag is never set by hand. The version is set by hand for exactly one reason: +the bump that workflow applies is always a patch step, so a change that breaks +consumers — the generated path contract above, or anything else a consumer +compiles against — carries the minor step in the PR that makes it. The gate +requires only that `[package].version` is ahead of the published revision, so it +publishes whatever version the merged tree names and resumes patch bumping from +there. ## License diff --git a/foundry.toml b/foundry.toml index 1e3e105..b51770d 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,6 +1,6 @@ [package] name = "rain-sol-codegen" -version = "0.1.13" +version = "0.2.0" [profile.default] src = 'src' diff --git a/src/lib/LibFs.sol b/src/lib/LibFs.sol index 9ad096f..9e579ad 100644 --- a/src/lib/LibFs.sol +++ b/src/lib/LibFs.sol @@ -10,6 +10,14 @@ import {LibCodeGen} from "./LibCodeGen.sol"; /// path, so it is a cross repo contract rather than an internal detail. string constant GENERATED_DIR = "src/generated"; +/// Thrown when `GENERATED_DIR` holds an artifact for a contract other than the +/// one `pathForContract` names for it. Consumers commit these files and import +/// them by path, so an artifact this library does not write is one nothing +/// regenerates: its imports keep resolving to a frozen codehash while the build +/// reports success. +/// @param path The artifact nothing regenerates, relative to the project root. +error OrphanedGeneratedArtifact(string path); + /// @title LibFs /// @notice A library for file system operations related to code generation. /// @dev Uses foundry's Vm cheat codes for file operations. Notably standardizes @@ -35,6 +43,72 @@ library LibFs { return string.concat(GENERATED_DIR, "/", contractName, ".sol"); } + /// The final segment of `path`: the bytes after its last `/`, or all of + /// `path` when it has none. `vm.readDir` reports absolute paths, so this is + /// what carries an entry's own name. + /// @param path The path to take the final segment of. + /// @return The final segment. + function lastPathSegment(string memory path) internal pure returns (string memory) { + bytes memory pathBytes = bytes(path); + uint256 start = 0; + for (uint256 i = 0; i < pathBytes.length; i++) { + if (pathBytes[i] == "/") { + start = i + 1; + } + } + bytes memory segment = new bytes(pathBytes.length - start); + for (uint256 i = 0; i < segment.length; i++) { + segment[i] = pathBytes[start + i]; + } + return string(segment); + } + + /// @notice Reverts if `GENERATED_DIR` holds an artifact for `contractName` + /// other than the one at `pathForContract(contractName)`. + /// + /// An artifact for a contract is a direct child of `GENERATED_DIR` named + /// for that contract: the name in full, then a `.`, then anything. The full + /// name up to the `.` is what separates one contract's artifact from + /// another's, so a repo generating both `Foo` and `FooBar` keeps both. + /// + /// `pathForContract` names exactly one such child, and that one is asked + /// for here rather than spelled out again, so whichever file that function + /// names is the current artifact and every other one is a file this library + /// does not write. Nothing regenerates those, while the consumer's `src/**` + /// keeps importing them, so they are refused rather than left frozen beside + /// a fresh generation. + /// + /// Only direct children are read. `pathForContract` never names anything + /// deeper, so a per release snapshot directory holds nothing this library + /// wrote and is left alone. + /// + /// Inherits `pathForContract`'s refusal to produce a path for a name that + /// is not a Solidity identifier. + /// @param vm The Vm instance for file operations. + /// @param contractName The name of the contract. + function requireNoOrphanedArtifact(Vm vm, string memory contractName) internal view { + bytes32 currentArtifact = keccak256(bytes(lastPathSegment(pathForContract(contractName)))); + bytes memory prefix = bytes(string.concat(contractName, ".")); + //forge-lint: disable-next-line(unsafe-cheatcode) + Vm.DirEntry[] memory entries = vm.readDir(GENERATED_DIR); + for (uint256 i = 0; i < entries.length; i++) { + bytes memory name = bytes(lastPathSegment(entries[i].path)); + if (name.length < prefix.length || keccak256(name) == currentArtifact) { + continue; + } + bool isArtifact = true; + for (uint256 j = 0; j < prefix.length; j++) { + if (name[j] != prefix[j]) { + isArtifact = false; + break; + } + } + if (isArtifact) { + revert OrphanedGeneratedArtifact(string.concat(GENERATED_DIR, "/", string(name))); + } + } + } + /// @notice Builds a file for a generated contract at /// `pathForContract(contractName)`. /// @@ -46,6 +120,10 @@ library LibFs { /// `GENERATED_DIR` is created if it does not exist, so the first generation /// in a repo does not need it committed already. /// + /// Another artifact for the same contract already in `GENERATED_DIR` + /// refuses the whole call, before anything is created or removed, so a + /// generation never lands beside a file that nothing regenerates. + /// /// Anything already at the path is unlinked before the write, so a symlink /// there is replaced by a regular file rather than written through to its /// target, and the path does not exist between the unlink and the write. @@ -65,6 +143,7 @@ library LibFs { string memory path = pathForContract(contractName); //forge-lint: disable-next-line(unsafe-cheatcode) vm.createDir(GENERATED_DIR, true); + requireNoOrphanedArtifact(vm, contractName); if (vm.exists(path)) { //forge-lint: disable-next-line(unsafe-cheatcode) vm.removeFile(path); diff --git a/test/concrete/LibFsExternal.sol b/test/concrete/LibFsExternal.sol index 40b99cc..9584213 100644 --- a/test/concrete/LibFsExternal.sol +++ b/test/concrete/LibFsExternal.sol @@ -16,4 +16,8 @@ contract LibFsExternal { function pathForContract(string memory contractName) external pure returns (string memory) { return LibFs.pathForContract(contractName); } + + function requireNoOrphanedArtifact(Vm vm, string memory contractName) external view { + LibFs.requireNoOrphanedArtifact(vm, contractName); + } } diff --git a/test/lib/LibFs.buildFileForContract.t.sol b/test/lib/LibFs.buildFileForContract.t.sol index a5ed2af..fee7d54 100644 --- a/test/lib/LibFs.buildFileForContract.t.sol +++ b/test/lib/LibFs.buildFileForContract.t.sol @@ -3,7 +3,7 @@ pragma solidity =0.8.25; import {Test} from "forge-std-1.16.1/src/Test.sol"; -import {LibFs} from "src/lib/LibFs.sol"; +import {LibFs, OrphanedGeneratedArtifact} from "src/lib/LibFs.sol"; import {LibCodeGen, InvalidContractName} from "src/lib/LibCodeGen.sol"; import {CodeGennable} from "test/concrete/CodeGennable.sol"; import {LibFsExternal} from "test/concrete/LibFsExternal.sol"; @@ -311,4 +311,43 @@ contract LibFsBuildFileForContractTest is Test { vm.assume(!LibCodeGenSlow.isContractNameSlow(contractName)); assertNameRejected(contractName); } + + /// A consumer holds the artifact this library used to write committed and + /// imported from `src/**`. Generating beside it would leave those imports + /// resolving to a file nothing regenerates, with the build reporting + /// success, so the generation is refused instead and nothing is written. + function testBuildFileForContractRefusesToOrphanAnotherArtifact() external { + string memory name = "LibFsBuildOrphan"; + string memory orphan = string.concat("src/generated/", name, ".pointers.sol"); + //REUSE-IgnoreStart + string memory existing = "// SPDX-License-Identifier: LicenseRef-DCL-1.0\npragma solidity ^0.8.25;\n"; + //REUSE-IgnoreEnd + cleanupPath(orphan); + cleanupPath(LibFs.pathForContract(name)); + vm.writeFile(orphan, existing); + address instance = address(new CodeGennable()); + + vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); + iExternal.buildFileForContract(vm, instance, name, "\n// body\n"); + + assertFalse(vm.exists(LibFs.pathForContract(name)), "a second artifact was written for the contract"); + assertEq(vm.readFile(orphan), existing, "the artifact that was already there was touched"); + cleanupPath(orphan); + } + + /// The refusal happens before the directory is read for the file's own + /// path, so a consumer whose generated directory does not exist yet is not + /// refused for that reason. + function testBuildFileForContractGeneratesWhenNoOtherArtifactExists() external { + string memory name = "LibFsBuildNoOrphan"; + cleanupPath(LibFs.pathForContract(name)); + cleanupPath(string.concat("src/generated/", name, ".pointers.sol")); + address instance = address(new CodeGennable()); + string memory body = "\n// no orphan\n"; + + LibFs.buildFileForContract(vm, instance, name, body); + + assertEq(vm.readFile(LibFs.pathForContract(name)), expectedFile(instance, body)); + cleanupPath(LibFs.pathForContract(name)); + } } diff --git a/test/lib/LibFs.requireNoOrphanedArtifact.t.sol b/test/lib/LibFs.requireNoOrphanedArtifact.t.sol new file mode 100644 index 0000000..d97c1a9 --- /dev/null +++ b/test/lib/LibFs.requireNoOrphanedArtifact.t.sol @@ -0,0 +1,231 @@ +// SPDX-License-Identifier: LicenseRef-DCL-1.0 +// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd +pragma solidity =0.8.25; + +import {Test} from "forge-std-1.16.1/src/Test.sol"; +import {LibFs, GENERATED_DIR, OrphanedGeneratedArtifact} from "src/lib/LibFs.sol"; +import {InvalidContractName} from "src/lib/LibCodeGen.sol"; +import {LibFsExternal} from "test/concrete/LibFsExternal.sol"; +import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol"; + +/// @dev A parseable Solidity source unit, so that a file one of these tests +/// leaves under `src/generated` on failure does not also break the compile. +//REUSE-IgnoreStart +string constant PLACEHOLDER_SOURCE = "// SPDX-License-Identifier: LicenseRef-DCL-1.0\npragma solidity ^0.8.25;\n"; + +//REUSE-IgnoreEnd + +/// @title LibFsRequireNoOrphanedArtifactTest +/// @notice Consumers commit what this library generates and import it by path, +/// so a second artifact for the same contract sitting beside the one being +/// generated is a file nothing regenerates while `src/**` keeps importing it. +/// These assert that such a file is refused, that the refusal is keyed on the +/// contract name rather than on any one extension, and that the file the +/// library does write is the one thing it is not refused for. +/// +/// Every test owns a contract name no other test uses, because the check reads +/// the whole generated directory and suites run in parallel. +contract LibFsRequireNoOrphanedArtifactTest is Test { + /// `vm.expectRevert` needs a call frame, and `requireNoOrphanedArtifact` is + /// an internal library function that is inlined into its caller. + LibFsExternal internal immutable iExternal; + + constructor() { + iExternal = new LibFsExternal(); + } + + /// Removes whatever is at `path`, so a test establishes its own + /// precondition rather than assuming one. + function cleanupPath(string memory path) internal { + if (vm.exists(path)) { + if (vm.isDir(path)) { + vm.removeDir(path, true); + } else { + vm.removeFile(path); + } + } + } + + /// The path an artifact for `contractName` occupies when its name carries + /// `suffix` in place of the `sol` the library appends. + function artifactPath(string memory contractName, string memory suffix) internal pure returns (string memory) { + return string.concat(GENERATED_DIR, "/", contractName, ".", suffix); + } + + /// No artifact at all for the contract is the first generation in a repo, + /// which must not be refused. + function testRequireNoOrphanedArtifactAcceptsNothingForTheContract() external { + string memory name = "LibFsOrphanNothing"; + cleanupPath(LibFs.pathForContract(name)); + cleanupPath(artifactPath(name, "pointers.sol")); + + LibFs.requireNoOrphanedArtifact(vm, name); + } + + /// The file the library writes is the one artifact for the contract that is + /// never an orphan, so regenerating over it is not refused. + function testRequireNoOrphanedArtifactAcceptsTheFileItWrites() external { + string memory name = "LibFsOrphanCurrent"; + vm.writeFile(LibFs.pathForContract(name), PLACEHOLDER_SOURCE); + + LibFs.requireNoOrphanedArtifact(vm, name); + + cleanupPath(LibFs.pathForContract(name)); + } + + /// The artifact name this library wrote before `src/generated/.sol`. + /// Consumers still hold these committed and imported, so this is the case + /// that reaches the check in the field. + function testRequireNoOrphanedArtifactRejectsTheLegacyName() external { + string memory name = "LibFsOrphanLegacy"; + string memory legacy = artifactPath(name, "pointers.sol"); + cleanupPath(legacy); + vm.writeFile(legacy, PLACEHOLDER_SOURCE); + + vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, legacy)); + iExternal.requireNoOrphanedArtifact(vm, name); + + cleanupPath(legacy); + } + + /// Having generated the current file does not excuse the orphan: adding the + /// new artifact without deleting the old one leaves `src/**` importing the + /// old one, which is the state the refusal exists to reject. + function testRequireNoOrphanedArtifactRejectsAlongsideTheCurrentFile() external { + string memory name = "LibFsOrphanBoth"; + string memory legacy = artifactPath(name, "pointers.sol"); + cleanupPath(legacy); + vm.writeFile(LibFs.pathForContract(name), PLACEHOLDER_SOURCE); + vm.writeFile(legacy, PLACEHOLDER_SOURCE); + + vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, legacy)); + iExternal.requireNoOrphanedArtifact(vm, name); + + cleanupPath(legacy); + cleanupPath(LibFs.pathForContract(name)); + } + + /// Writes an artifact for `contractName` carrying `suffix`, asserts it is + /// refused by the path it occupies, and removes it again. The contract name + /// is the caller's so that each case owns a file no other case names. + function assertSuffixRejected(string memory contractName, string memory suffix) internal { + string memory orphan = artifactPath(contractName, suffix); + cleanupPath(orphan); + vm.writeFile(orphan, PLACEHOLDER_SOURCE); + + vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); + iExternal.requireNoOrphanedArtifact(vm, contractName); + + cleanupPath(orphan); + } + + /// The refusal is keyed on the contract name, not on the one extension that + /// happens to be stale today, so the next time `pathForContract` moves + /// there is nothing here to update. Spelled out one suffix at a time rather + /// than fuzzed: fuzz cases run concurrently against the one generated + /// directory, two cases whose file names collide race on it, and random + /// identifiers essentially never land near `sol` anyway. These do — every + /// one of them differs from the appended `sol` by a single edit, which is + /// what a comparison that is nearly right survives. + function testRequireNoOrphanedArtifactRejectsEveryOtherSuffix() external { + assertSuffixRejected("LibFsOrphanSuffixLegacy", "pointers.sol"); + assertSuffixRejected("LibFsOrphanSuffixShort", "so"); + assertSuffixRejected("LibFsOrphanSuffixLong", "soll"); + assertSuffixRejected("LibFsOrphanSuffixUpper", "SOL"); + assertSuffixRejected("LibFsOrphanSuffixLead", "asol"); + assertSuffixRejected("LibFsOrphanSuffixDigit", "sol0"); + assertSuffixRejected("LibFsOrphanSuffixDouble", "sol.sol"); + assertSuffixRejected("LibFsOrphanSuffixOther", "json"); + assertSuffixRejected("LibFsOrphanSuffixBare", "pointers"); + assertSuffixRejected("LibFsOrphanSuffixEmpty", ""); + } + + /// An artifact is the contract name followed by a `.`, so the name on its + /// own is not one. Nothing this library writes is extensionless, and a file + /// that is holds no imports for `src/**` to resolve to. + function testRequireNoOrphanedArtifactIgnoresTheBareName() external { + string memory name = "LibFsOrphanBare"; + string memory bare = string.concat(GENERATED_DIR, "/", name); + cleanupPath(bare); + vm.writeFile(bare, PLACEHOLDER_SOURCE); + + LibFs.requireNoOrphanedArtifact(vm, name); + + cleanupPath(bare); + } + + /// An artifact belongs to the contract whose name it carries in full, up to + /// the `.`. A longer name that merely starts with this one is a different + /// contract's artifact and must not be refused, or a repo could not + /// generate both `Foo` and `FooBar`. + function testRequireNoOrphanedArtifactIgnoresOtherContracts() external { + string memory name = "LibFsOrphanPrefix"; + string memory sibling = artifactPath(string.concat(name, "Extra"), "pointers.sol"); + string memory unrelated = artifactPath("LibFsOrphanUnrelated", "pointers.sol"); + cleanupPath(sibling); + cleanupPath(unrelated); + vm.writeFile(sibling, PLACEHOLDER_SOURCE); + vm.writeFile(unrelated, PLACEHOLDER_SOURCE); + + LibFs.requireNoOrphanedArtifact(vm, name); + + cleanupPath(sibling); + cleanupPath(unrelated); + } + + /// A name that is a prefix of this one, and so a shorter contract's + /// artifact, is likewise not this contract's. + function testRequireNoOrphanedArtifactIgnoresShorterContracts() external { + string memory name = "LibFsOrphanLongerName"; + string memory shorter = artifactPath("LibFsOrphanLonger", "pointers.sol"); + cleanupPath(shorter); + vm.writeFile(shorter, PLACEHOLDER_SOURCE); + + LibFs.requireNoOrphanedArtifact(vm, name); + + cleanupPath(shorter); + } + + /// Consumers freeze per release snapshots into subdirectories of the + /// generated directory. `pathForContract` never names anything below a + /// direct child, so nothing down there is an artifact this library wrote + /// and none of it is refused. + function testRequireNoOrphanedArtifactIgnoresSubdirectories() external { + string memory name = "LibFsOrphanNested"; + string memory dir = string.concat(GENERATED_DIR, "/LibFsOrphanTag"); + cleanupPath(dir); + vm.createDir(dir, true); + vm.writeFile(string.concat(dir, "/", name, ".pointers.sol"), PLACEHOLDER_SOURCE); + vm.writeFile(string.concat(dir, "/", name, ".sol"), PLACEHOLDER_SOURCE); + + LibFs.requireNoOrphanedArtifact(vm, name); + + cleanupPath(dir); + } + + /// A directory occupying an artifact's name is not something this library + /// wrote either, and leaving it there means the name is taken by something + /// no regeneration touches. + function testRequireNoOrphanedArtifactRejectsADirectory() external { + string memory name = "LibFsOrphanDir"; + string memory orphan = artifactPath(name, "pointers.sol"); + cleanupPath(orphan); + vm.createDir(orphan, true); + + vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); + iExternal.requireNoOrphanedArtifact(vm, name); + + cleanupPath(orphan); + } + + /// The check asks `pathForContract` which file is the current one, so it + /// inherits that function's refusal to produce a path for a name that is + /// not a Solidity identifier. + function testRequireNoOrphanedArtifactRejectsEveryNonIdentifierName(bytes memory nameBytes) external { + string memory contractName = string(nameBytes); + vm.assume(!LibCodeGenSlow.isContractNameSlow(contractName)); + + vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, contractName)); + iExternal.requireNoOrphanedArtifact(vm, contractName); + } +} From 442c150aa5b1c6e96f18d249e49446beb3c7217b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 18:52:45 +0000 Subject: [PATCH 2/7] Pin lastPathSegment directly, and say exactly what the refusal precedes `lastPathSegment` is what turns a directory entry into the name the check compares, so its result is the whole basis of that comparison. Its own tests cover the shapes a directory read never produces and so never reached it: a path with no separator at all, an empty path, a path ending in a separator, and repeated separators. The property over arbitrary bytes pins it to the LAST separator rather than any earlier one. `buildFileForContract` creates `GENERATED_DIR` before the refusal, so the refusal precedes the removal and the write rather than everything. Co-Authored-By: Claude Opus 5 (1M context) --- test/lib/LibFs.lastPathSegment.t.sol | 68 ++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 test/lib/LibFs.lastPathSegment.t.sol diff --git a/test/lib/LibFs.lastPathSegment.t.sol b/test/lib/LibFs.lastPathSegment.t.sol new file mode 100644 index 0000000..e102dd0 --- /dev/null +++ b/test/lib/LibFs.lastPathSegment.t.sol @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: LicenseRef-DCL-1.0 +// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd +pragma solidity =0.8.25; + +import {Test} from "forge-std-1.16.1/src/Test.sol"; +import {LibFs} from "src/lib/LibFs.sol"; + +/// @title LibFsLastPathSegmentTest +/// @notice `lastPathSegment` is what turns a directory entry into the name to +/// compare against, so its result is the whole basis of that comparison: a +/// segment that keeps any of the directories above it, or drops any of the name +/// itself, compares against something that is not the file's name. +/// +/// The property is asserted over arbitrary bytes rather than over paths, +/// because the function takes whatever `vm.readDir` reports and nothing +/// upstream of it constrains that to a shape. The cases below are the shapes +/// that carry a boundary: a name with no directory above it, an empty result, a +/// separator with nothing after it. +contract LibFsLastPathSegmentTest is Test { + /// The absolute path a directory read reports, which is the input the + /// library actually receives. + function testLastPathSegmentAbsolutePath() external pure { + assertEq(LibFs.lastPathSegment("/home/user/repo/src/generated/Foo.sol"), "Foo.sol"); + } + + /// The relative path `pathForContract` returns, which is the input the + /// current artifact's name is taken from. + function testLastPathSegmentRelativePath() external pure { + assertEq(LibFs.lastPathSegment("src/generated/Foo.sol"), "Foo.sol"); + } + + /// A path with no separator at all is entirely its own final segment. + function testLastPathSegmentNoSeparator() external pure { + assertEq(LibFs.lastPathSegment("Foo.sol"), "Foo.sol"); + assertEq(LibFs.lastPathSegment(""), ""); + } + + /// Only the last separator counts, so repeated and empty intermediate + /// segments are all above the name and none of them reach it. + function testLastPathSegmentRepeatedSeparators() external pure { + assertEq(LibFs.lastPathSegment("a//b///Foo.sol"), "Foo.sol"); + } + + /// A path ending in a separator has nothing after it, and an empty segment + /// is the honest answer rather than the segment before it. + function testLastPathSegmentTrailingSeparator() external pure { + assertEq(LibFs.lastPathSegment("src/generated/"), ""); + assertEq(LibFs.lastPathSegment("/"), ""); + } + + /// The three halves of the definition, over arbitrary bytes: the result + /// carries no separator, it is a suffix of the input, and it is the LONGEST + /// such suffix, which is what pins it to the last separator rather than to + /// any earlier one. + function testLastPathSegmentIsTheSuffixAfterTheLastSeparator(bytes memory pathBytes) external pure { + bytes memory segment = bytes(LibFs.lastPathSegment(string(pathBytes))); + + assertLe(segment.length, pathBytes.length, "segment is longer than the path"); + uint256 start = pathBytes.length - segment.length; + for (uint256 i = 0; i < segment.length; i++) { + assertTrue(segment[i] != "/", "segment carries a separator"); + assertTrue(segment[i] == pathBytes[start + i], "segment is not a suffix of the path"); + } + if (start > 0) { + assertTrue(pathBytes[start - 1] == "/", "segment does not start after a separator"); + } + } +} From 5c8b55f2843f0acc5cfb3b872f2a914389b7fd38 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sun, 16 Aug 2026 19:12:20 +0000 Subject: [PATCH 3/7] Remove the orphan fixtures before asserting on them, not after The orphan tests wrote a fixture, asserted, then removed it. A revert and a failed assertion both abort the test body where they happen, so the removal only ran on the runs that passed. Every fixture these tests write is a file `requireNoOrphanedArtifact` refuses on, and all of them share the one `src/generated`, so a run that failed handed the next one a directory it had to refuse. That is what the mutation matrix leaves behind: under any mutant killed by `testRequireNoOrphanedArtifactIgnoresOtherContracts`, that test reverts before its cleanup and leaks `LibFsOrphanPrefixExtra.pointers.sol` and `LibFsOrphanUnrelated.pointers.sol` into a committed directory. Each test now catches the outcome, reads whatever it needs off disk, removes its fixtures, and only then asserts. `vm.expectRevert` cannot express that ordering, so the refusal is compared as returned revert data instead. Co-Authored-By: Claude Opus 5 (1M context) --- test/lib/LibFs.buildFileForContract.t.sol | 21 +++++-- .../lib/LibFs.requireNoOrphanedArtifact.t.sol | 60 +++++++++++++++---- 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/test/lib/LibFs.buildFileForContract.t.sol b/test/lib/LibFs.buildFileForContract.t.sol index fee7d54..e8b0cdd 100644 --- a/test/lib/LibFs.buildFileForContract.t.sol +++ b/test/lib/LibFs.buildFileForContract.t.sol @@ -327,12 +327,25 @@ contract LibFsBuildFileForContractTest is Test { vm.writeFile(orphan, existing); address instance = address(new CodeGennable()); - vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); - iExternal.buildFileForContract(vm, instance, name, "\n// body\n"); + // Caught rather than expected, and everything read off disk before any + // assertion, so the orphan is removed on the runs that fail too. It is + // a file this refusal fires on, and every test here shares the one + // generated directory, so one left behind is a precondition the next + // run does not get to choose. + bytes memory outcome; + try iExternal.buildFileForContract(vm, instance, name, "\n// body\n") {} + catch (bytes memory reason) { + outcome = reason; + } + bool wroteSecondArtifact = vm.exists(LibFs.pathForContract(name)); + string memory orphanContent = vm.readFile(orphan); - assertFalse(vm.exists(LibFs.pathForContract(name)), "a second artifact was written for the contract"); - assertEq(vm.readFile(orphan), existing, "the artifact that was already there was touched"); cleanupPath(orphan); + cleanupPath(LibFs.pathForContract(name)); + + assertEq(outcome, abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), "wrong refusal"); + assertFalse(wroteSecondArtifact, "a second artifact was written for the contract"); + assertEq(orphanContent, existing, "the artifact that was already there was touched"); } /// The refusal happens before the directory is read for the file's own diff --git a/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol b/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol index d97c1a9..1c3ed96 100644 --- a/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol +++ b/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol @@ -52,6 +52,35 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { return string.concat(GENERATED_DIR, "/", contractName, ".", suffix); } + /// Runs the check and hands back what it reverted with, or empty bytes when + /// it did not revert. Nothing here asserts, so a caller removes its + /// fixtures before it asserts anything about them. + /// + /// A revert and a failed assertion both abort the test body at the point + /// they happen, so a fixture removed after either one is removed only on + /// the runs that pass. Every fixture these tests write is a file this check + /// refuses on, and they all share the one generated directory, so a fixture + /// left behind by a failing run is a precondition the next run does not get + /// to choose. + function checkOutcome(string memory contractName) internal returns (bytes memory) { + try iExternal.requireNoOrphanedArtifact(vm, contractName) { + return ""; + } catch (bytes memory reason) { + return reason; + } + } + + /// Asserts `outcome` is the check accepting the contract. + function assertAccepted(bytes memory outcome) internal pure { + assertEq(outcome.length, 0, "the check refused a contract it must accept"); + } + + /// Asserts `outcome` is the check refusing `orphan`, by the exact error and + /// path a consumer is shown. + function assertRefused(bytes memory outcome, string memory orphan) internal pure { + assertEq(outcome, abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), "wrong refusal"); + } + /// No artifact at all for the contract is the first generation in a repo, /// which must not be refused. function testRequireNoOrphanedArtifactAcceptsNothingForTheContract() external { @@ -68,9 +97,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { string memory name = "LibFsOrphanCurrent"; vm.writeFile(LibFs.pathForContract(name), PLACEHOLDER_SOURCE); - LibFs.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(LibFs.pathForContract(name)); + assertAccepted(outcome); } /// The artifact name this library wrote before `src/generated/.sol`. @@ -82,10 +112,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { cleanupPath(legacy); vm.writeFile(legacy, PLACEHOLDER_SOURCE); - vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, legacy)); - iExternal.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(legacy); + assertRefused(outcome, legacy); } /// Having generated the current file does not excuse the orphan: adding the @@ -98,11 +128,11 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { vm.writeFile(LibFs.pathForContract(name), PLACEHOLDER_SOURCE); vm.writeFile(legacy, PLACEHOLDER_SOURCE); - vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, legacy)); - iExternal.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(legacy); cleanupPath(LibFs.pathForContract(name)); + assertRefused(outcome, legacy); } /// Writes an artifact for `contractName` carrying `suffix`, asserts it is @@ -113,10 +143,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { cleanupPath(orphan); vm.writeFile(orphan, PLACEHOLDER_SOURCE); - vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); - iExternal.requireNoOrphanedArtifact(vm, contractName); + bytes memory outcome = checkOutcome(contractName); cleanupPath(orphan); + assertRefused(outcome, orphan); } /// The refusal is keyed on the contract name, not on the one extension that @@ -149,9 +179,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { cleanupPath(bare); vm.writeFile(bare, PLACEHOLDER_SOURCE); - LibFs.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(bare); + assertAccepted(outcome); } /// An artifact belongs to the contract whose name it carries in full, up to @@ -167,10 +198,11 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { vm.writeFile(sibling, PLACEHOLDER_SOURCE); vm.writeFile(unrelated, PLACEHOLDER_SOURCE); - LibFs.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(sibling); cleanupPath(unrelated); + assertAccepted(outcome); } /// A name that is a prefix of this one, and so a shorter contract's @@ -181,9 +213,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { cleanupPath(shorter); vm.writeFile(shorter, PLACEHOLDER_SOURCE); - LibFs.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(shorter); + assertAccepted(outcome); } /// Consumers freeze per release snapshots into subdirectories of the @@ -198,9 +231,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { vm.writeFile(string.concat(dir, "/", name, ".pointers.sol"), PLACEHOLDER_SOURCE); vm.writeFile(string.concat(dir, "/", name, ".sol"), PLACEHOLDER_SOURCE); - LibFs.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(dir); + assertAccepted(outcome); } /// A directory occupying an artifact's name is not something this library @@ -212,10 +246,10 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { cleanupPath(orphan); vm.createDir(orphan, true); - vm.expectRevert(abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan)); - iExternal.requireNoOrphanedArtifact(vm, name); + bytes memory outcome = checkOutcome(name); cleanupPath(orphan); + assertRefused(outcome, orphan); } /// The check asks `pathForContract` which file is the current one, so it From 0b9add3bcfa419fef0ebaf7a623b43932b7bd039 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sun, 16 Aug 2026 19:16:41 +0000 Subject: [PATCH 4/7] Say in words what the refusal bytes are meant to be Comparing raw revert data reports a mismatch as two hex strings. The message now names the error and the path expected, so the assertion that replaced `vm.expectRevert` reads as well as it did. Co-Authored-By: Claude Opus 5 (1M context) --- test/lib/LibFs.buildFileForContract.t.sol | 6 +++++- test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/lib/LibFs.buildFileForContract.t.sol b/test/lib/LibFs.buildFileForContract.t.sol index e8b0cdd..bdb6c6e 100644 --- a/test/lib/LibFs.buildFileForContract.t.sol +++ b/test/lib/LibFs.buildFileForContract.t.sol @@ -343,7 +343,11 @@ contract LibFsBuildFileForContractTest is Test { cleanupPath(orphan); cleanupPath(LibFs.pathForContract(name)); - assertEq(outcome, abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), "wrong refusal"); + assertEq( + outcome, + abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), + string.concat("not refused as OrphanedGeneratedArtifact(", orphan, ")") + ); assertFalse(wroteSecondArtifact, "a second artifact was written for the contract"); assertEq(orphanContent, existing, "the artifact that was already there was touched"); } diff --git a/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol b/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol index 1c3ed96..6e85ecf 100644 --- a/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol +++ b/test/src/lib/LibFs.requireNoOrphanedArtifact.t.sol @@ -76,9 +76,14 @@ contract LibFsRequireNoOrphanedArtifactTest is Test { } /// Asserts `outcome` is the check refusing `orphan`, by the exact error and - /// path a consumer is shown. + /// path a consumer is shown. The comparison is over the raw revert bytes, + /// so the message says in words what those bytes are meant to be. function assertRefused(bytes memory outcome, string memory orphan) internal pure { - assertEq(outcome, abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), "wrong refusal"); + assertEq( + outcome, + abi.encodeWithSelector(OrphanedGeneratedArtifact.selector, orphan), + string.concat("not refused as OrphanedGeneratedArtifact(", orphan, ")") + ); } /// No artifact at all for the contract is the first generation in a repo, From fcfa857cf7d1704674165111fe25d23b627ac85c Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 17 Aug 2026 06:50:50 +0000 Subject: [PATCH 5/7] Say what a directory that cannot be read does to the check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vm.readDir` does not revert on a read it cannot perform: it returns one entry naming the directory itself and carrying an `errorMessage`. Measured, not assumed — reading a missing directory under this repo's `fs_permissions` returns `len=1`, `path` the absolute directory path, and `errorMessage` the `os error 2`. No artifact name matches that entry, so a directory that cannot be read is accepted. That is the answer wanted for a repo with no generated directory yet, and it is what makes creating the directory first the honest ordering rather than a load-bearing one. The docstring said `dir` "must exist", which reads as a precondition something enforces; nothing does. Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/LibFs.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/lib/LibFs.sol b/src/lib/LibFs.sol index 794349d..33640a1 100644 --- a/src/lib/LibFs.sol +++ b/src/lib/LibFs.sol @@ -119,8 +119,14 @@ library LibFs { /// writes to is `GENERATED_DIR`, and `dir` is interpolated verbatim and is /// not checked. /// - /// `dir` must exist and be readable under `fs_permissions`, because the - /// whole check is one read of it. Callers create it first. + /// The whole check is one read of `dir`, and `vm.readDir` does not revert + /// when that read fails: it returns a single entry naming `dir` itself and + /// carrying an `errorMessage`. No artifact name matches that entry, so a + /// directory that cannot be read is accepted rather than refused. That is + /// the answer wanted for a repo with no generated directory yet, and it is + /// why `buildFileForContract` creates `dir` before calling this: after a + /// `vm.createDir` that did not itself revert, the read is of a directory + /// that is there. /// @param vm The Vm instance for file operations. /// @param dir The directory to read, without a trailing separator, /// interpolated verbatim. From b9ed2252ba6c6ec9143aa37c791ecc67bc07981b Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 17 Aug 2026 06:59:08 +0000 Subject: [PATCH 6/7] Cover the check reading the directory rather than its own name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. The check placed ahead of `vm.createDir` therefore reads that one entry, and the two orderings disagree only where the directory's own final segment reads as an artifact for the contract — so that is the shape this drives through. Co-Authored-By: Claude Opus 5 (1M context) --- test/src/lib/LibFs.buildFileForContract.t.sol | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/src/lib/LibFs.buildFileForContract.t.sol b/test/src/lib/LibFs.buildFileForContract.t.sol index dd1500a..25f7eea 100644 --- a/test/src/lib/LibFs.buildFileForContract.t.sol +++ b/test/src/lib/LibFs.buildFileForContract.t.sol @@ -583,4 +583,30 @@ contract LibFsBuildFileForContractTest is Test { string.concat("not refused as OrphanedGeneratedArtifact(", dirOrphan, ")") ); } + + /// The check reads what is inside the directory, never the directory's own + /// name, and it reads it after `vm.createDir` rather than before. + /// + /// `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. A check placed ahead of the create therefore + /// reads that one entry, and for a directory whose own final segment reads + /// as an artifact for the contract it refuses the very first generation — + /// naming, as the orphan, the directory it was about to create. Driven + /// through such a directory, because that is the only shape the two + /// orderings disagree on. + function testBuildFileForContractReadsTheDirectoryNotItsOwnName() external { + string memory name = "LibFsBuildDirName"; + string memory dir = string.concat(GENERATED_DIR, "/", name, ".snapshot"); + cleanupPath(dir); + assertFalse(vm.exists(dir), "dirty precondition"); + address instance = address(new CodeGennable()); + string memory body = "\n// dir name\n"; + + LibFs.buildFileForContract(vm, instance, dir, name, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body); + + string memory written = vm.readFile(string.concat(dir, "/", name, ".sol")); + cleanupPath(dir); + assertEq(written, expectedFile(instance, SPDX_LICENSE_IDENTIFIER, COPYRIGHT_TEXT, body)); + } } From 905640dedbfba67024b0c6208c84f18e8bf5c0c4 Mon Sep 17 00:00:00 2001 From: David Meister Date: Mon, 17 Aug 2026 07:14:24 +0000 Subject: [PATCH 7/7] Say what the snapshot directories the tagged write owns are checked against #137 landed `buildFileForTaggedContract`, which writes into exactly the per release snapshot directories this section called untouched. They are not: that write enters the same shared body, so it reads the directory it writes into and checks it against its own contents. Only a generation into `src/generated/` itself leaves them unread, and it never refuses one of them because a tag carries no `.`. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index b8d21ce..f402e29 100644 --- a/README.md +++ b/README.md @@ -51,8 +51,13 @@ contract, in full, followed by a `.` and anything other than `sol`. Nothing regenerates such a file, while the consumer's imports keep resolving to it, so the build fails with `OrphanedGeneratedArtifact` naming the file rather than generating beside it. Delete it and repoint the imports at -`src/generated/.sol` in the same commit. Only direct children are -read, so per release snapshot directories under `src/generated/` are untouched. +`src/generated/.sol` in the same commit. + +Only direct children are read, so a generation into `src/generated/` never reads +inside the per release snapshot directories that sit there, and never refuses +one of them either, because a tag carries no `.`. `buildFileForTaggedContract` +writes into one of those directories, and reads that directory rather than +`src/generated/`, so each is checked against its own contents. ## Formatter requirements