Stop the matchstick run from executing a cached wasm - #292
Conversation
Closes #230. `subgraph/tests/.bin/metaboard.wasm` was a tracked matchstick build, last written in 2024, still exporting a `MetaBoard#hash` entry point deleted from the sources in 2026. Matchstick 0.6.0 reuses that file unless `--recompile` is passed, the wasm is absent, or an mtime says a source is newer, so a checkout where no import lands newer than the committed binary runs the 2024 build instead of the current mappings. The binary is untracked and `tests/.bin` ignored, so there is nothing to reuse and a local run cannot stage one back. `docker-compose.yml` passes `-r` through the image's `ARGS`, so the decision never consults an mtime. `test/subgraph/SubgraphMatchstick.t.sol` holds both, in the sol lane rather than inside the suite a stale binary would answer for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Closes #230.
Matchstick 0.6.0 compiles each suite to
tests/.bin/<suite>.wasmand then reuses that file.Compiler::execute(src/compiler/mod.rsat tag0.6.0) compiles only whenand
is_source_modified(src/compiler/sources.rs) is an mtime comparison — the test file or a transitively imported file has to be strictly NEWER than the wasm. An mtime is not a fact about content, so a tree where nothing happens to land newer than the cached binary runs that binary and reports its results as the suite's.subgraph/tests/.bin/metaboard.wasmwas tracked in git and last written 2024-07-31.subgraph/testsandsubgraph/srchave changed as recently as 2026-08-20.stringson the committed blob shows it still exportsgenerated/metaboard0/MetaBoard/MetaBoard#hashand linksmatchstick-as/index/MockedFunction—MetaBoard.hash()and its subgraph mocks were deleted in e6e191c (#106). So the binary a fresh checkout landed was a build of tests that no longer exist. Today CI is defused by accident, not by design:graph codegenwritessubgraph/generated/after checkout, the test file imports../generated/..., and that fresh mtime forces a recompile. Nothing guarantees that ordering.What changed
Two independent things have to hold for the cached path to be unreachable, and neither implies the other, so the issue's "and/or" is taken as and.
subgraph/tests/.bin/metaboard.wasmis removed from git andtests/.binadded tosubgraph/.gitignore. A fresh checkout now has no wasm at all, so the!out_file.exists()arm forces compilation whatever the mtimes say — and a local run no longer dirties the tree with an opaque 45KB binary that the nextgit add -Are-commits. Deleting without the ignore rule leaves the churn; ignoring without the deletion changes nothing, because ignore rules do not apply to tracked paths.subgraph/docker-compose.ymlsetsARGS: -r, so the docker run never consults an mtime.rainix'ssubgraph-testisnpm ci && docker compose up --abort-on-container-exitfromsubgraph/, and the pinned image config forrainprotocol/matchstick:mainisENV ARGS=plusCMD ["/bin/sh","-c","/binary-linux-22 ${ARGS}"]—ARGSis the extension point the image exists to offer, so this needs nocommand:override.-ris matchstick's short spelling of--recompile("Force-recompiles the tests",src/cli.rs).test/subgraph/SubgraphMatchstick.t.solpins both, in therainix-sollane. Deliberately not a check inside the matchstick suite: the question is whether that suite executes the current sources at all, and a check shipped inside the cached binary is answered by the stale build along with everything else. It needs no docker, so it runs on every push. The compose file is read as parsed YAML overyq/vm.ffifor the reasonSubgraphManifest.t.solgives — a commented-out# ARGS: -rsatisfies a text search while the compose parser never sees it, which is exactly how this protection would be lost.foundry.tomlandCLAUDE.mdnamegitas a thirdvm.ffibinary, so "nothing else here shells out" stays true.gitandyqare both in rainix'ssol-build-inputs, i.e. onPATHfor theforge testtherainix-sollane runs.The now-absent
subgraph/tests/.bindirectory is created on demand: matchstick shells out to<lib>/assemblyscript/bin/asc --outFile tests/.bin/<suite>.wasm, andassemblyscript@0.19.23(the versionsubgraph/package-lock.jsonresolves at the top level) doesif (!fs.existsSync(dirPath)) mkdirp(dirPath)inwriteFileNode.Relation to the other open work on this suite
#282 (issue #206) changes
subgraph/src/metaBoard.tsand three files undersubgraph/tests/. This PR touches none of those, and #282 touches none of these, so there is no textual conflict in either merge order.They meet at exactly one point, and it is the point this PR is about. #282's QA notes that
subgraph/tests/.bin/metaboard.wasm"is a tracked matchstick build artifact last written in 2024 and already stale against several test changes since. My runs rewrote it; I restored it rather than commit a rebuild." That restore was the right call — committing a rebuild would have swapped one opaque binary for another — but it is a manual step that every agent and every human who runs matchstick locally has to remember, and forgetting it commits a binary nobody can review. After this, there is nothing to restore. If #282 merges first, this deletes the same unchanged blob and applies cleanly; if this merges first, #282's local runs write into an ignored directory and its diff is unaffected.#289 (issue #229) and #291 (issue #227) are also in
subgraph/tests/andsubgraph/src/, and likewise share no file with this. #229 in particular is about mutants the matchstick suite would not kill; this is about the suite not being run against the current sources at all, which is upstream of any question about what it covers. Neither is a fix for the other.QA
subgraph-testis docker and there is no docker here. I also did not run the matchstick binary directly, for the specific reason that doing so writessubgraph/tests/.bin/metaboard.wasm— the artifact this PR removes — and the run would have had to be undone by hand, which is the failure mode being fixed. TheMetaBoard Subgraph CIlane on this branch is the authority for the matchstick half, and it is the run that proves the two claims I could not execute locally: that the missingtests/.binis created rather than fatal, and thatARGS: -rreaches the binary without breaking the run. It has since run green on this branch and its log settles both —💬 Compiling metaboard...thenAll 10 tests passed! 😎, with noskipped!line, against a checkout that contains no wasm.forgefrom the nix store andyq/gitonPATH:forge test --match-path 'test/subgraph/*'— 6 passed (2 new + the 4 existingSubgraphManifestTest, which shares thevm.ffisurface this PR extends).forge fmt --checkclean. Not run: the rest of the forge suite (5096-run fuzz, nothing here is Solidity behavior) and the rust lanes.testMatchstickBinariesAreNotInGit,testMatchstickRunForcesRecompilation— each fails on base, verified by restoring main's exact state for the file it is about and re-running: M1 restores the tracked wasm and M3 restores main'sdocker-compose.ymlverbatim, below.test/subgraph/SubgraphMatchstick.t.sol, baseline 2 passed:git checkout HEAD -- subgraph/tests/.bin/metaboard.wasm, i.e. main's exact index state -> killed bytestMatchstickBinariesAreNotInGit,git ls-filesreturning the wasm alongside the test source. This is the mutation that IS the issue.tests/.binfromsubgraph/.gitignore, keeping the deletion -> killed by the same test ongit check-ignore --no-indexexiting 1. Confirms the two assertions are not redundant.environment:block fromdocker-compose.yml, i.e. main's exact file -> killed bytestMatchstickRunForcesRecompilationonkeyExistsJson.ARGS: -c— a real matchstick flag that is not--recompile-> killed on-c != -r. A presence-only check would have survived this.# environment:/# ARGS: -r-> killed. This is the mutation the parsed-YAML read exists for: a text search forARGS: -rfinds it and passes.0.6.0for when it recompiles and what-rmeans; the registry image config forrainprotocol/matchstick:mainfor howARGSreaches the binary; rainix'sflake.nixfor whatsubgraph-testruns and forgit/yqbeing insol-build-inputs. The test's expected values are spelled as literals, not derived from the files they are checked against.crates/,subgraph/subgraph.yamland the published interface are untouched.REUSE.tomlalready blanketssubgraph/**, so removing a file there needs no licensing change.tests/.binshould be gitignored and the committed wasm removed, and/orARGS=-rset for the docker matchstick run". Both are done, argued above as independent rather than alternative. The issue's remaining paragraphs are the analysis, not further asks.🤖 Generated with Claude Code