ci: run the test suite on external PRs into main - #219
Merged
Conversation
The suite only triggered on `pull_request` into `prod`, so every contributor PR targeting `main` merged with no automated verification — tests ran later, at release time, when main was promoted to prod. #215, #216 and #218 all sat mergeable with an empty status-check rollup. Add `main` to the trigger and gate the jobs: PRs into prod always run (that is the release gate, unchanged), PRs into main run only for outside contributions. Our own work — Rome-1's PRs, or any branch living in the Raftersecurity repo — is reviewed and tested locally before it is pushed, so re-running the full matrix would only burn runner minutes. Uses `pull_request`, not `pull_request_target`: fork PRs run with a read-only token and no secrets. Tests that need RAFTER_API_KEY already skip when it is absent, so a fork PR gets a clean green rather than a spurious failure.
This was referenced Sep 1, 2026
Rome-1
added a commit
that referenced
this pull request
Sep 1, 2026
…10k (#221) The assignment was to explain why 8 of 17 checks skipped on PR #220. The skip pattern is real, but it is not why the bug reached a customer. Testing that claim rather than assuming it is what turned up the rest. WOULD ANY EXISTING CHECK HAVE CAUGHT IT, IF IT HAD RUN? No. Checked out main at 0996492 (the buggy tree) and ran the entire suite against it: 2065 passed, and the only failures were three files that fail for environmental reasons here and are unrelated to polling. tests/scan-remote.test.ts and test_scan_remote.py both cover the poll loop — with the HTTP layer mocked, and neither ever injected a non-2xx mid-poll. They pass against the bug. And nothing executed github-action/action.yml at all, which is the file the customer's error came from. test-action.yml drives the ROOT action.yml, a different action that scans locally. Of the jobs in test-github-action.yml, two run hand-copied reimplementations of the action's bash (their own headers admit the duplication) and one greps the YAML as text. So the workflow that fires on github-action/** changes ran, and still executed none of the code. The gap was COVERAGE, and #220 closed it. The skip pattern would not have mattered. Three things found on the way there do: 1. publish-python had no `needs:`. publish-node has needed the test jobs since it was written; the Python half published to PyPI in parallel with the tests, ungated. A red suite blocked the npm release and shipped the PyPI one anyway — in a dual-implementation product where the two versions must match, that diverges them at the registry, the one place users cannot see it. Now gated. (publish.yaml also runs no pytest anywhere; filed separately.) 2. backend-api rendered identically whether it tested the backend or nothing. Its only real step is gated on RAFTER_API_KEY, which has never been set on this repo, so "backend-api ✓" has always meant "checked out and built". It now says so, loudly, in the log and the step summary. 3. test-node and test-python were skipped on internal PRs into main. On #220 — which changed both clients — neither ran. They now run on every PR. The premise that our own work is tested locally first is also weaker than it looks: this repo has test files that fail locally for environmental reasons, so "green on my machine" is not a signal anyone can act on. Cost is ~4 minutes of wall clock (234s and 100s, in parallel). The expensive part, the 6-way cross-platform grid with 3 macOS runners, stays gated — this reverses part of #219 narrowly and deliberately, not wholesale. Also established, not changed here: main has no branch protection at all. The only ruleset targets refs/heads/prod and contains no required-status-checks rule, so no check is required anywhere and a red PR can merge into main. That is a policy call, not a workflow fix. Co-authored-by: achebe <hello@rafter.so>
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.
The comprehensive suite only triggered on
pull_requestintoprod, so contributor PRs targetingmainmerged with no automated verification at all — #215, #216 and #218 each sat MERGEABLE with an empty status-check rollup, and the last workflow run in the repo was 2026-07-29. Tests only ran later, at release time, whenmainwas promoted toprod.What changed
mainadded to thepull_requesttrigger.gatejob decides who gets the suite; the 8 existing jobs depend on it.Policy, per request:
prod(release gate)mainfrom a fork / outside contributormainauthored byRome-1mainfrom a branch in theRaftersecurityrepoOur own work is reviewed and tested locally before it's pushed, so re-running the full matrix would only burn runner minutes.
Security notes
pull_request, notpull_request_target— fork PRs run with a read-only token and no access to secrets. There's a comment in the file saying so, because "fixing" that is the classic way to hand a fork repo-write credentials.env:rather than interpolating${{ }}straight into the shell script, so nothing attacker-controlled reaches the command line.secrets.RAFTER_API_KEY. The tests that need it already skip when it's absent (backend-api.test.tsskips by design; the local suite passes with 9 skips), so an external PR gets a clean green rather than a spurious failure.Verification
YAML parses; all 8 jobs carry
needs: gate+ theifcondition. This PR is itself from aRaftersecuritybranch, so by its own policy the suite will skip on it — I'll confirm the workflow still runs end-to-end with aworkflow_dispatchonce it's onmain.🤖 Generated with Claude Code