v0.2: GitHub Action for independent CI verification - #4
Merged
Conversation
New internal/ci package + `proofrun run-all`: reads .proofrun.yml and executes every declared check directly, no separate CLI-supplied command to compare against (unlike `run <name> -- <cmd>`, the command comes from exactly one place here, so the whole argv-vs-declaration mismatch class of bug structurally can't happen on this path). Two edge cases handled per the plan, both with dedicated tests: - Receipt is saved after each check, not batched at the end. Verified with a real integration test that builds the actual binary, starts `run-all` as a subprocess, kills its whole process tree partway through a long-running check, and confirms the earlier-completed check's result survived on disk. - .proofrun.yml missing or declaring zero checks is a hard error, not a silent no-op — an empty check set trivially satisfies `status --strict`, which would gate on nothing. Exported internal/runner's KillTree/SetProcessGroup (were internal to Run's own timeout handling) for reuse by the above test: killing only the top-level proofrun process from outside orphans whatever check command it was running, which isn't just untidy — the run-all kill test initially failed because an orphaned windows ping.exe held a file handle that broke t.TempDir()'s cleanup. Killing the same real-process tree the test simulates (matching what a real CI system does when it tears down a timed-out job) fixed it and is a more accurate simulation regardless. Manually smoke-tested `--only`, both error paths, and dogfooded `proofrun run-all` + `status --strict` on this repo itself.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
macos-latest failed with a real, 100% reproducible crash on this PR's new cmd/proofrun tests: `dyld: missing LC_UUID load command` / `signal: abort trap` when running the coverage-instrumented test binary for cmd/proofrun specifically (a package main, and one that never had any test files — hence never had a test binary built on macOS CI — until this PR). Every other package's test binary built and ran fine on the same runner. CI was pinned to 1.22.12; every bit of local verification throughout this whole project has been done with 1.26.5 — go.mod's `go 1.22` is only a minimum language-version requirement, not a pin on which toolchain patch actually builds/tests the code, so this gap was silent until a coverage-instrumented package main test binary finally exercised whatever linker path triggers it. Bumping CI to 1.26 across test/release/demo workflows for consistency with what's actually been verified locally this whole time.
Composite action that independently re-verifies every check declared in .proofrun.yml against a PR's real head commit — the deliverable the whole v0.2 plan exists for. Key design decision: the action performs its own authoritative checkout (ref: github.event.pull_request.head.sha, falling back to github.sha for non-PR triggers) rather than only validating whatever the calling workflow already checked out. A validate-only version would correctly catch the most common integration mistake (forgetting to pin the PR head, defaulting to GitHub's synthetic merge-preview commit on pull_request events) — but the natural fix for a confusing validation error is to delete the validation, which reopens exactly the hole this project exists to close. Doing the checkout here removes the whole failure class instead of just documenting around it. The head-sha assertion step stays anyway as cheap defense in depth. Also: a non-blocking warning (::warning::, not a failure) when .proofrun.yml itself differs from the base branch — re-running the declared checks proves they really executed, it doesn't protect the declarations from being weakened by the same PR. This is a known, disclosed limitation (README's "Known limitations" section is next), not something this version tries to architecturally solve. Binary download verifies against checksums.txt with a sha256sum / shasum / openssl fallback chain (macOS ships shasum, not sha256sum — exactly the kind of platform gotcha this project has been bitten by before), and never runs a download that doesn't match. dogfood.yml applies this action to ProofRun's own PRs via `uses: ./`, so this PR is the first real test of the whole chain against a live PR (not a synthetic scenario) — pulled forward from Day 4 since it's the most direct way to exercise the real checkout/PR-head logic that a throwaway test repo can't fully replicate locally. Published a temporary `v1` prerelease (manually built, not through release.yml) so the download step has something real to fetch during Day 2/3 testing — Day 4 replaces this with the actual automated floating-tag maintenance in release.yml.
Found via actually testing the download step end-to-end (dogfood.yml against this repo's own PR), not by inspection: a checksums.txt with Windows line endings breaks the `grep " archive$"` match — the trailing \r sits between the filename and grep's end-of-line anchor, so a correct checksum silently fails to match and the action errors out with "no checksum entry found" even though one is right there. The real goreleaser-built checksums.txt (built on ubuntu in CI) won't have this problem, but the parsing shouldn't be silently fragile to line-ending variance regardless — strip \r defensively before matching.
…DMEs Adds a GitHub Action section (usage snippet + how the authoritative checkout/receipt-clearing/run-all/strict-gate chain works) to README.md and README.zh-CN.md, with the .proofrun.yml-tampering known limitation called out explicitly rather than only living in action.yml's description field. Also updates "what ProofRun deliberately does not do" and Commands/Roadmap to reflect that the Action now exists and that run-all is a real subcommand.
The download step's runner.os/runner.arch mapping and the checksum-verification fallback chain (sha256sum/shasum/openssl) are exactly the kind of logic that only breaks on a specific platform. Verified via v0.2 Day 3 adversarial testing that a naive checkout lands on GitHub's synthetic merge-preview commit while action.yml's authoritative checkout does not (see PR #7, closed).
Found via the new 3-platform dogfood matrix (PR #4, run 31724014695): Windows runner failed with "checksum mismatch ... got \<hash>" — a literal leading backslash baked into the parsed sum. sha256sum/shasum prepend \ to their whole output line (not just the filename) when the path being hashed contains a backslash, which RUNNER_TEMP-derived paths do on Windows. The naive `awk '{print $1}'` parse picked up that marker as part of the hash.
…TS.md release.yml now moves the v1 release/tag to whatever a real, non-prerelease version tag just published (delete+recreate, since release immutability is off for this repo) — replacing the temporary, manually-published v1 prerelease used to test action.yml during Day 2/3. Skipped for prereleases so a release candidate never becomes what @v1 users get. AGENTS.md now documents action.yml's boundary (never trusts a checked-out receipt, does its own authoritative checkout, warns but doesn't block on .proofrun.yml tampering) instead of listing "no GitHub Action" as out-of-scope.
…action.yml
MUST FIX: status --strict exited 0 when .proofrun.yml was missing or
declared zero checks. run-all already treated both as hard errors, but
action.yml's "continue-on-error: true" on the run-all step swallowed
that, and the final `status --strict` gate then saw zero evaluations
and returned success — a PR could delete or empty .proofrun.yml and
get a fully green Action with nothing actually verified. status.go now
exits 1 under --strict when there's nothing to evaluate. Reproduced
both the delete-the-file and checks:{} exploit paths against the fixed
binary before and after to confirm; added two CLI-level regression
tests (missing config, empty checks) plus one confirming non-strict
`status` is unaffected.
MUST FIX: RunAll computed one fingerprint up front and bound every
check in the pass to it, including checks that ran after an earlier
check had already mutated the working tree (codegen, fixture setup,
cleanup steps are all ordinary .proofrun.yml commands). A check that
actually ran against a mutated tree could still read back as PASS
against the pre-mutation state. Fingerprint is now recomputed
immediately before each check runs. Replaced the test that had locked
in the old behavior (TestRunAll_AllChecksShareTheSameFingerprint) with
one that mutates the tree between two checks and confirms the affected
check reads STALE once the tree is restored to its original state.
Also from the same review round (SHOULD FIX, not false-PASS but real
hardening):
- action.yml's checkout now sets persist-credentials: false — the PR's
own declared checks execute later in the same job via run-all, and
there's no reason to leave a token in .git/config for PR-supplied
commands to find. READMEs' Action usage example adds
`permissions: contents: read`.
- action.yml's binary download now derives the release tag from
github.action_ref (falling back to v1 for the local `uses: ./`
dogfood case) instead of hardcoding v1 unconditionally. Previously,
pinning an exact Action version (@v0.2.0) still downloaded whatever
the floating v1 tag currently pointed to, defeating the whole point
of pinning once a later release moved v1.
…rom action_ref External review round 2 caught a real gap in the previous fix: github.action_ref is only ever a release tag for a @vX.Y.Z-style reference. A consumer who pins by full commit SHA or by branch (SHA-pinning is GitHub's own recommended safest way to consume a third-party action) gets an action_ref that isn't a release tag at all, and the tag-shaped download URL breaks outright. Also, GitHub advises against interpolating context values directly into `run:` scripts in the first place. Replaced with a literal pin_version in action.yml, bumped as part of a documented release-prep step (added to AGENTS.md) before tagging each real release — so the tagged commit is self-consistent regardless of whether a consumer pins by tag, resolved SHA, or the floating v1 tag once it points there. Doesn't fail closed into a false PASS either way — a bad URL just fails the download loudly — so this was corretly scored SHOULD FIX, not MUST FIX, but is real and worth fixing before the actual v0.2.0 cut.
Owner
Author
|
Review closed at head |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the v0.2 plan (independently re-verify checks in CI rather than trusting a receipt.json checked out from the PR branch). Still in progress — Draft, per project workflow.
Day 1:
proofrun run-allinternal/cipackage +run-allsubcommand: executes every check declared in.proofrun.ymldirectly, no separate CLI-supplied command to compare against (command comes from exactly one place, so the argv-vs-declaration mismatch class of bug from v0.1 structurally can't happen here)..proofrun.ymlmissing or declaring zero checks is a hard error, not a silent no-op.internal/runner'sKillTree/SetProcessGroupfor reuse by the kill-mid-run test — found and fixed a real bug in the process during this: killing only the top-level PID orphaned a grandchild process, which is a less accurate simulation of a real CI kill (which tears down the whole tree/cgroup) and concretely broke test cleanup on Windows.Day 2:
action.ymlcomposite action +dogfood.yml::warning::when.proofrun.ymlitself differs from the PR base, checksum-verified binary download,rm -rf .proofrun/before re-running, thenrun-all+status --strictas the gate.dogfood.yml(uses: ./against this repo's own PRs) — realbuild/test/vetPASS output confirmed in run logs, not just "the job went green."dyld: missing LC_UUID load command) on the first-ever compiledcmd/proofruntest binary for that platform — CI/local toolchain mismatch, unrelated to this PR's logic but blocking it..proofrun.ymlitself from being weakened by the same PR, only warns) in both README.md and README.zh-CN.md, not just action.yml's description field.Day 3: adversarial tests
.proofrun/receipt.json(force-added past.gitignore) claiming build/test/vet all PASS, with a fingerprint matching the exact commit, while the code actually failed to compile. Confirmed action.yml'srm -rf .proofrun/+ realrun-alldefeated it — job failed for real (exit 1), the forged PASS never surfaced. Run: https://github.com/yebiguo/ProofRun/actions/runs/31723398795.proofrun.yml(PR [adversarial test — do not merge] weakened .proofrun.yml #6, closed): swapped the requiredtestcheck for a no-op ([true]). Confirmed a real::warning::annotation fires (checked via the check-run annotations API, not log text) and — as documented — does not block the job. Run: https://github.com/yebiguo/ProofRun/actions/runs/31723633000actions/checkout@v4(noref:override) empirically landed on a different commit than the real PR head sha, proving the premise behind action.yml's own authoritative checkout. action.yml's own assert-HEAD step on the same PR passed silently. Run: https://github.com/yebiguo/ProofRun/actions/runs/31723812908dogfood.ymlto aubuntu-latest/macos-latest/windows-latestmatrix (kept permanently, not just for this test). This caught a real bug: on Windows,sha256sum/shasumprepend a literal\to their output line when the path being hashed contains a backslash (asRUNNER_TEMP-derived paths do on Windows), corrupting the naiveawk '{print $1}'parse and producing a spurious checksum-mismatch failure. Fixed by stripping a leading\from the parsed sum; re-run confirmed green on all three platforms.refs/pull/<n>/headmechanism — there's no code path in this action that behaves differently for fork vs. same-repo PRs.Day 4: v1 floating tag automation + docs
release.ymlgets a newfloat-tagjob (runs aftergoreleaser, skipped on prerelease tags likev0.2.0-rc1): downloads the just-published release's assets, deletes the oldv1release+tag, recreatesv1pointing at the same commit with the same assets. Replaces the manually-published temporaryv1prerelease used to test action.yml during Day 2/3.ghCLI flags (--cleanup-tag,--target, etc.) verified againstgh release --help; confirmed this repo has release immutability off, which the delete+recreate approach depends on.AGENTS.mdnow documents action.yml's boundary (never trusts a checked-out receipt, does its own authoritative checkout, warns-but-doesn't-block on.proofrun.ymltampering) instead of listing "no GitHub Action" under out-of-scope.dogfood.yml's three matrix legs to required branch-protection checks onmain— doing it now, beforedogfood.ymlexists onmain, would deadlock any other PR that doesn't happen to carry it on its own branch. Also deferred: actually watching thefloat-tagjob run against a real version-tag push, since cutting a real release isn't dev work. Tracked as a follow-up task.External review round 1: 2 MUST FIX, 2 SHOULD FIX — all fixed
Both MUST FIXes verified against the actual code before fixing (not taken on faith), then confirmed closed by reproducing the exact exploit locally against the fixed binary, not just via unit tests.
.proofrun.ymlgave a green Action with zero checks run.run-allalready hard-errors on a missing config orchecks: {}, but action.yml'scontinue-on-error: trueon the run-all step swallowed that, andstatus --strictthen saw zero evaluations and returned exit 0 regardless of--strict. Fixed:status --strictnow exits 1 when there's nothing to evaluate (cmd/proofrun/status.go). Reproduced both exploit paths (delete.proofrun.yml;checks: {}) against the pre-fix and post-fix binary directly — pre-fix:status --strictexit 0; post-fix: exit 1. Added 3 regression tests.run-allbound every check in a pass to one fingerprint computed up front, so a check that mutates the working tree (codegen, fixtures, cleanup — ordinary things a check's command can do) could leave a later check's real execution against the mutated tree recorded against the pre-mutation fingerprint — a genuine false PASS once the tree returned to its original state. Fixed: fingerprint is now recomputed immediately before each check runs (internal/ci/ci.go). Replaced the test that had locked in the old behavior with one that mutates the tree between two checks and confirms the affected check readsSTALE, notPASS, once the tree is restored.persist-credentials: true, so the PR's own declared checks (executed later in the same job viarun-all) could read theGITHUB_TOKENout of.git/config. Fixed:persist-credentials: false; READMEs' usage example now showspermissions: contents: read.releases/download/v1regardless of which ref the Action itself was pinned to, so pinning@v0.2.0for reproducibility still silently tracked whateverv1moved to. Fixed round 1: derived the download tag fromgithub.action_ref.External review round 2: confirmed both MUST FIXes closed (0 remaining), 3 of 4 findings fully closed. Caught a real gap in round 1's
action_reffix:action_refis only ever a release tag for a@vX.Y.Z-style reference — a consumer pinning by full commit SHA (GitHub's own recommended safest way to consume a third-party action) or by branch gets a non-tagaction_ref, and the tag-shaped download URL breaks outright. Also flagged interpolating a context value directly into arun:script as against GitHub's own guidance. Re-scored SHOULD FIX, not MUST (fails loudly on a bad URL, not a silent false PASS), but flagged as worth fixing before the real v0.2.0 cut.Fixed: replaced the
action_refruntime lookup with a literalpin_versionin action.yml, bumped as part of a new documented release-prep step (added to AGENTS.md) before tagging each real release. This makes a tagged commit self-consistent regardless of whether a consumer pins by tag, resolved SHA, or the floatingv1tag once it points there — no ref-sniffing needed.Both review rounds' status: 0 MUST FIX, 0 SHOULD FIX remaining.
Important caveat — not yet true in production: these fixes are committed to this branch and verified at the source/unit/local-binary level, but action.yml downloads a pre-built binary from the
v1release rather than building from source. The currently-publishedv1release predates this fix round, souses: yebiguo/proofrun@v1today still downloads the vulnerable pre-fix binary. This only becomes true for real users once a real version tag is cut (with the release-prep step applied) andrelease.yml'sfloat-tagjob movesv1to it (tracked in the post-merge follow-up task, not part of this PR's dev-time scope).Test plan
go test ./...passes locallyrun-all,--only, both error paths, dogfooded on this repo itselffloat-tagjob'sghinvocations checked againstgh release --help; not yet exercised against a real tag pushdogfood.ymlrequired check + realfloat-tagrun + release-prep step verified (this is also when the v1 binary actually starts carrying today's fixes)