Noticed while driving #763; not a defect in that PR, and it affects five suites that predate it.
What happens
restore-default-branch-workflows (gha#598) rm -rfs .github/workflows/ and checks it out from origin/<default-branch> whenever the PR under review edits top-level workflow YAML. That is exactly right for the review's trust purpose. But it also means any test suite that parses a top-level workflow file is, inside that checkout, reading the default-branch copy rather than the PR's.
Six suites do that today:
| suite |
reads |
run-mention-filter-tests.py |
.github/workflows/claude.yml |
run-review-job-split-tests.py |
.github/workflows/claude-code-review.yml |
run-permissions-docs-tests.py |
discover_workflows (the whole directory) |
run-workflow-audit-tests.py |
discover_workflows |
run-workflow-job-guard-tests.py |
discover_workflows |
preview/tests/test_action_wiring.py |
.github/workflows/preview.yml (new in #763) |
Observed
On #763's second review round, the reviewer ran preview/tests/ and got 52 of 54 passing. It diagnosed the two failures correctly and unprompted:
The 2 failures [...] are not real — this checkout's .github/workflows/preview.yml was restored to the default-branch (pre-PR) copy per the gha#598 workflow-file-fallback mechanism [...] Confirmed by reading the actual PR diff for that file.
That is the reviewer doing the right thing. It should not have to.
Why this is worth fixing rather than tolerating
The false-failure direction is noise: a reviewer sees red, spends effort, and correctly discounts it. Annoying, and it erodes the signal every suite is supposed to carry.
The false-pass direction is the one that matters, and it points the other way. The three discover_workflows consumers are audits — action-pin coverage, SUBMODULES_TOKEN misuse, job-guard and timeout coverage. Inside a restored checkout they audit the default branch's workflows, not the PR's. So a PR that introduces an unpinned action or a token: ${{ secrets.SUBMODULES_TOKEN }} in a workflow file would be audited against the old content and read clean, in the reviewer's own run.
CI is unaffected: _selftest.yml does a plain checkout with no restore, so the audits there see the real PR content. The exposure is confined to a reviewer running the suites by hand and reporting what it got — which is precisely what happened above, and what this repo's review prompt encourages.
That asymmetry is the argument for acting: a reviewer's hand-run result is more trusted than a CI result, because it reads as independent verification, and here it is verification of the wrong artifact.
Options
Roughly in increasing order of effort:
-
Say so in the reviewer prompt. run-claude-review-attempt already tells the reviewer that on-disk workflow files are the default-branch copies and to take workflow diffs from the saved PR diff. Extend that to name the consequence for tests: a workflow-parsing suite run here is measuring the wrong file, so do not report its result as a finding or as a pass. Cheapest, and it only helps a reviewer that reads carefully.
-
Have the suites detect the restore and refuse. restore-default-branch-workflows could drop a marker (an env var, or a file under RUNNER_TEMP), and the parsing suites could skip-with-a-stated-reason rather than assert against a file they know is not the PR's. This is the fail-fast shape the repo prefers elsewhere: a stated skip beats a wrong answer in either direction. It also fixes the audits' false pass, which option 1 does not.
-
Point the suites at the saved PR diff rather than the working tree for workflow files. Most faithful, most work, and probably not worth it for the assertion styles these suites use.
My inclination is 2, with 1 alongside — but I have not surveyed how the audits would want to report a skip, and this is the review machinery rather than something #763 touched, so I have not implemented anything.
Not a regression in #763
test_action_wiring.py is a sixth instance rather than the cause; the five above have had this property since gha#598. #763 is merged, and its CI runs the suite in a normal checkout where it passes.
Posted by Claude Code (AI agent) --- not written by a human.
Noticed while driving #763; not a defect in that PR, and it affects five suites that predate it.
What happens
restore-default-branch-workflows(gha#598)rm -rfs.github/workflows/and checks it out fromorigin/<default-branch>whenever the PR under review edits top-level workflow YAML. That is exactly right for the review's trust purpose. But it also means any test suite that parses a top-level workflow file is, inside that checkout, reading the default-branch copy rather than the PR's.Six suites do that today:
run-mention-filter-tests.py.github/workflows/claude.ymlrun-review-job-split-tests.py.github/workflows/claude-code-review.ymlrun-permissions-docs-tests.pydiscover_workflows(the whole directory)run-workflow-audit-tests.pydiscover_workflowsrun-workflow-job-guard-tests.pydiscover_workflowspreview/tests/test_action_wiring.py.github/workflows/preview.yml(new in #763)Observed
On #763's second review round, the reviewer ran
preview/tests/and got 52 of 54 passing. It diagnosed the two failures correctly and unprompted:That is the reviewer doing the right thing. It should not have to.
Why this is worth fixing rather than tolerating
The false-failure direction is noise: a reviewer sees red, spends effort, and correctly discounts it. Annoying, and it erodes the signal every suite is supposed to carry.
The false-pass direction is the one that matters, and it points the other way. The three
discover_workflowsconsumers are audits — action-pin coverage,SUBMODULES_TOKENmisuse, job-guard and timeout coverage. Inside a restored checkout they audit the default branch's workflows, not the PR's. So a PR that introduces an unpinned action or atoken: ${{ secrets.SUBMODULES_TOKEN }}in a workflow file would be audited against the old content and read clean, in the reviewer's own run.CI is unaffected:
_selftest.ymldoes a plain checkout with no restore, so the audits there see the real PR content. The exposure is confined to a reviewer running the suites by hand and reporting what it got — which is precisely what happened above, and what this repo's review prompt encourages.That asymmetry is the argument for acting: a reviewer's hand-run result is more trusted than a CI result, because it reads as independent verification, and here it is verification of the wrong artifact.
Options
Roughly in increasing order of effort:
Say so in the reviewer prompt.
run-claude-review-attemptalready tells the reviewer that on-disk workflow files are the default-branch copies and to take workflow diffs from the saved PR diff. Extend that to name the consequence for tests: a workflow-parsing suite run here is measuring the wrong file, so do not report its result as a finding or as a pass. Cheapest, and it only helps a reviewer that reads carefully.Have the suites detect the restore and refuse.
restore-default-branch-workflowscould drop a marker (an env var, or a file underRUNNER_TEMP), and the parsing suites could skip-with-a-stated-reason rather than assert against a file they know is not the PR's. This is the fail-fast shape the repo prefers elsewhere: a stated skip beats a wrong answer in either direction. It also fixes the audits' false pass, which option 1 does not.Point the suites at the saved PR diff rather than the working tree for workflow files. Most faithful, most work, and probably not worth it for the assertion styles these suites use.
My inclination is 2, with 1 alongside — but I have not surveyed how the audits would want to report a skip, and this is the review machinery rather than something #763 touched, so I have not implemented anything.
Not a regression in #763
test_action_wiring.pyis a sixth instance rather than the cause; the five above have had this property since gha#598. #763 is merged, and its CI runs the suite in a normal checkout where it passes.Posted by Claude Code (AI agent) --- not written by a human.