ci: prove a PR's tests fail without its fix (bug-fix-gate) - #442
Merged
Conversation
Adds an advisory PR check that runs every test file a PR adds or changes against the merge-base with ONLY the test files overlaid — the fix absent — and requires it to fail there and pass on the head. A test that passes on the base does not pin the behaviour the PR changed. This is the evidence an automated merge needs. "CI is green" cannot tell a fix that works from a fix whose test never exercised it, and Bug tickets filed by a QA pass are headed for merges no human reads. The local `gh pr merge` gate will refuse a Bug PR whose proof is not green. Deliberately advisory, not a required check: a human merging by hand is the judgement this stands in for, and a refactor with no behavioural delta legitimately has no regression test to prove. It is also not path-filtered, so the merge gate always reads a conclusion rather than an absent check; a PR touching no tests passes as "no proof available". Also names the two image-scan jobs. Both workflows called their job `scan` with no `name:`, so both reported the same status-check context and neither could be required by branch protection. Verified against XERK-268's real merge (05fa50d): overlaying only turma/tests/server.test.js onto its merge-base reproduces 19 assertion failures, and the same file passes on the head — "proven". That run also exposed two defects in the first draft, both fixed here: the overlay step copied every changed file (carrying the fix across and voiding the proof), and an unbounded run can hang rather than fail, so each run is now wrapped in `timeout` and a hang is reported rather than counted as a clean failure.
Semgrep's run-shell-injection rule flagged `${{ github.base_ref }}`
spliced into the detect step's script. code-scan runs that rule over
every workflow in this repo, so the fix is the general one: every `${{ }}`
now reaches these scripts through `env:` and is referenced as a shell
variable.
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
1 participant
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.
What
Adds
.github/workflows/bug-fix-gate.yml: an advisory PR check that, for every test file a PR adds or changes, runs that test against the merge-base with only the test files overlaid — the fix absent — and requires it to FAIL there and pass on the head.A test that passes on the base does not pin the behaviour the PR changed.
Why
This is the prerequisite for letting a
Bugticket's PR merge without a human reading the diff. "CI is green" cannot distinguish a fix that works from a fix whose test never exercised it, and QA-filed Bug tickets are headed for exactly those unattended merges. A localgh pr mergegate (not in this PR) will refuse a Bug PR whose proof is not green.Advisory, not required. A human merging by hand is the judgement this stands in for, so a red result never blocks a person — a refactor with no behavioural delta legitimately has no regression test to prove. Automation is the only consumer that must obey it.
Not path-filtered, on purpose: it must report on every PR so the merge gate always reads a conclusion rather than an absent check. A PR touching no test files passes with "no proof available".
Also gives the two image-scan jobs explicit names. Both workflows called their job
scanwith noname:, so both reported the same status-check context and neither could ever be required by branch protection.How it was verified
Dry-run against XERK-268's real merge (
05fa50d) before pushing:turma/tests/server.test.jsoverlaidprovenThe extracted
prove/classifylogic was executed end-to-end against that case and produced| node | failed — assertion (then hung) | passed | proven |. Everyrun:block passesbash -n; the YAML parses.That dry run also found two defects in the first draft, both fixed here:
server.test.jsproduced 19 real failures on the base and then never exited, on an open handle. Each run is now wrapped intimeout, and a hang is reported (assertion (then hung)/weak (timed out)) rather than silently counted as a good failure.Known limit, documented in the workflow header and
release.mdOn android/glasses/veiller a base failure is usually a build error — the test references a symbol the fix introduces — not a failed assertion. That proves the test reaches new code but not that its assertion discriminates. The summary labels those
weak (build error)so a person knows to look.Not verified
The
qaagent was not run: this session is configured not to spawn subagents. The android/glasses/veiller paths of the gate have not been exercised against a real PR in those components — only the node path was dry-run. First live PRs touching those will be the real test.