Cut the committed-artifact currency test; copy-artifacts is the check - #98
Cut the committed-artifact currency test; copy-artifacts is the check#98thedavidmeister wants to merge 5 commits into
Conversation
testBuildFileForContractCommittedArtifactIsCurrent compared only the first bytes(header).length bytes of src/generated/CodeGennable.sol, so drift in the body the artifact carries was invisible to forge test. It now reruns script/Build.sol, compares every byte of what that writes against what is committed, and writes the committed bytes back so the working tree is left as it was found. Closes #57 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
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 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 (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
`cleanup` is called by the tests that generate under a name of their own. The committed artifact is now regenerated and written back by its own test instead, so the blanket claim that every test removes its file again no longer described what the file does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
`build-pointers / copy-artifacts` already regenerates the whole tree and diffs it on every push, which is a strictly stronger currency check than anything `forge test` can assert about the same file. The test's own mutation matrix showed it uniquely killed nothing: the mutants it caught in `LibFs` and `LibCodeGen` are killed by existing siblings, and the artifact and `script/Build.sol` mutants are exactly what copy-artifacts catches. Its carrying cost was real: it mutated a committed file mid-suite, and a revert inside `Build.run()` before the restore would leave the tree dirty. The pre-PR header-only version goes too. Issue #57's finding is that its docstring lies about what it checks, and the honest thing it could be narrowed to is already covered elsewhere. `cleanup()`'s docstring returns to its original text, which is true again now nothing writes the committed path back. The narrowed `InvalidContractName` import stays: `LibCodeGen` was only used by the deleted test.
`main` removed `testBuildFileForContractCommittedArtifactIsCurrent` in #138, which is the entire deliverable of this branch, and narrowed the `LibCodeGen` import in the same way. The only lines this branch still had over `main` were regressions of #138's other changes to the same file — dropping the `GENERATED_DIR` import and the `setUp` that creates `src/generated/` on a fresh clone, and reverting `cleanup`'s docstring to the pre-#138 text that calls `src/generated/` a committed directory holding a committed artifact. Taking `main`'s file wholesale. This branch is now a no-op against `main`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closing: this PR is now an empty diff, and its subject was deleted twice over. #138 deleted The branch's own history records the reversal: Closing rather than merging, because the delta it still carried before that last Nothing here is lost. The branch is left in place as the audit trail. |
Important
OBSOLETE against post-#138
main— this branch is now an empty diff.Left open for the human to rule on; nothing here is a close decision.
This PR's entire deliverable was deleting
testBuildFileForContractCommittedArtifactIsCurrent. #138 deleted it, alongwith the committed artifact and
script/Build.solthe test existed to guard,and closed #57 as completed
(#57).
Evidence, on the merge commit:
git grep testBuildFileForContractCommittedArtifactIsCurrent origin/main→no match;
git diff origin/main→ empty.Before
mainwas merged in, the only lines this branch still had overmainwere regressions of #138, not deletions of the stale test:
import {LibFs} from "src/lib/LibFs.sol";— dropping theGENERATED_DIRimport Strip the
script/Build.solworked example, leave the library lean #138 added.script/Build.solworked example, leave the library lean #138'ssetUp() { vm.createDir(GENERATED_DIR, true); }, which iswhat makes this suite pass on a fresh clone and under a filtered run now
that
src/generated/holds nothing committed.cleanup's docstring to the pre-Strip thescript/Build.solworked example, leave the library lean #138 text — "src/generated/,which is a committed directory in this repo … none of them is
CodeGennable(the committed artifact)" — both claims now false.
Those three were resolved in
main's favour. Also note the PR text belownames
.github/workflows/build-pointers.yaml/build-pointers / copy-artifactsas the check that replaces the deleted test. #138 deletedthat workflow too, so the replacement it points at no longer exists either.
This repo generates nothing, so there is nothing left to check the currency
of.
Original PR description, kept for the record. Every claim in it about the
committed artifact,
script/Build.solandbuild-pointers.yamldescribes atree that no longer exists.
Closes #57
What this PR does now
It deletes
testBuildFileForContractCommittedArtifactIsCurrentfromtest/lib/LibFs.buildFileForContract.t.sol— both the header-only version thatwas on
mainand the whole-file rewrite this PR originally proposed. Nothingreplaces it.
Net against
main: the test function and its docstring go (25 lines), and theimport narrows to
import {InvalidContractName} from "src/lib/LibCodeGen.sol";because
LibCodeGenwas only used by the deleted body. No production code, noconfig and no CI change.
No test guards this, and it can regress silently
Say it plainly: after this PR nothing in
forge testchecks that the committedsrc/generated/CodeGennable.solis current.script/Build.solhas no coverageunder
test/at all. That is a deliberate accepted gap, not an oversight.The currency check lives in CI instead:
build-pointers / copy-artifacts(
.github/workflows/build-pointers.yaml, which callsrainlanguage/rainix/.github/workflows/rainix-copy-artifacts.yaml@main) runsscript/Build.soland asserts the committed generated sources still match, onevery push. That is strictly stronger than what the deleted test did — it
regenerates and diffs the whole tree, not one file.
Why cutting it, and not the rewrite
The rewrite's own mutation matrix was the argument against it. M7–M10 mutate
LibFs.buildFileForContract,LibCodeGen.filePrefix,commentPrefixand thewrap threshold — all of which existing sibling tests already kill. M1–M6 mutate
the committed artifact and
script/Build.sol, which is precisely whatcopy-artifactscatches on every push. So the test uniquely killed nothing.Against that, its carrying cost was real and paid on every run: it wrote the
real committed
src/generated/CodeGennable.solmid-suite and restored itafterwards, and the restore is not revert-safe.
What issue #57 actually found, and how it is answered
#57's finding is that the test's name and docstring lie: they claim the
committed artifact is current and that drift "reds the suite too", while the
body compares only
bytes(header).lengthbytes and never looks at the artifactbody. Its measurement stands — tampering
SOME_BYTES_CONSTANTtohex"deadbeef"left all tests green.QA (as run against the pre-#138 tree)
forge fmt --check— exit 0.forge testbefore the deletion: 142 tests passed.forge testafter the deletion: 141 tests passed.Preserved mutation matrix
10 applied / 10 killed / 0 survived, each mutant applied alone then reverted,
with the driver refusing to score a mutant whose
sedmatched nothing:And #57's finding reproduced — the header-only version survives what the
rewrite killed:
CodeRabbit
CodeRabbit was rate limited and did not review this PR. Its first comment is
Review limit reached — you've used all free OSS reviews for now. TheCodeRabbit: SUCCESScheck is therefore an absence of review, not a passed one.