From 2dca14f97f47c49131838b1c504e224151e7dd18 Mon Sep 17 00:00:00 2001 From: achebe Date: Mon, 31 Aug 2026 19:04:42 -0700 Subject: [PATCH] ci: close three blind spots found investigating why CI missed sable-l10k MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/publish.yaml | 6 +++ .github/workflows/test-comprehensive.yml | 49 ++++++++++++++++++++---- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/.github/workflows/publish.yaml b/.github/workflows/publish.yaml index 176b1899..4984dd70 100644 --- a/.github/workflows/publish.yaml +++ b/.github/workflows/publish.yaml @@ -143,6 +143,12 @@ jobs: run: npm publish --access public --provenance publish-python: + # sable-bm5k — publish-node has needed the test jobs since it was written; + # this one never did, so a red suite blocked the npm release and shipped + # the PyPI one anyway. In a dual-implementation product that means the two + # runtimes could diverge at the registry, which is the one place users + # cannot see it. + needs: [test-node, test-package] runs-on: ubuntu-latest defaults: run: diff --git a/.github/workflows/test-comprehensive.yml b/.github/workflows/test-comprehensive.yml index 2813130e..5fd6c4ba 100644 --- a/.github/workflows/test-comprehensive.yml +++ b/.github/workflows/test-comprehensive.yml @@ -12,11 +12,11 @@ permissions: jobs: # ── Who gets the suite ──────────────────────────────────────────── - # PRs into prod are the release gate and always run. - # 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 running the full matrix again - # would just burn runner minutes. + # The two unit-test jobs (test-node, test-python) run on EVERY PR — see + # sable-bm5k. The rest of the matrix runs for PRs into prod (the release + # gate) and for outside contributions; for our own PRs into main it is + # skipped, because re-running the 6-way cross-platform grid on work that + # was reviewed before it was pushed mostly burns runner minutes. # # Note this is `pull_request`, not `pull_request_target` — fork PRs run with # a read-only token and no access to secrets. Do not "fix" that. @@ -24,6 +24,7 @@ jobs: runs-on: ubuntu-latest outputs: run: ${{ steps.decide.outputs.run }} + run_core: ${{ steps.decide.outputs.run_core }} steps: - id: decide # Values go through env rather than direct ${{ }} interpolation into @@ -34,11 +35,25 @@ jobs: HEAD_OWNER: ${{ github.event.pull_request.head.repo.owner.login }} AUTHOR: ${{ github.event.pull_request.user.login }} run: | + # `run` — the full matrix, including the 6-way cross-platform grid. + # `run_core` — the two unit-test jobs. These now run on EVERY PR. + # + # sable-bm5k: the original gate skipped everything on internal PRs into + # main, on the premise that our own work is tested locally first. On + # #220 — which changed both the Node and the Python client — that meant + # neither test-node nor test-python ran. The premise 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 you + # can act on. test-node (234s) and test-python (100s) run in parallel, + # so this costs ~4 minutes of wall clock. The expensive part — the + # cross-platform grid, 6 jobs and 3 of them macOS — stays gated. + echo "run_core=true" >> "$GITHUB_OUTPUT" + if [ "$EVENT" != "pull_request" ] || [ "$BASE" != "main" ]; then echo "run=true" >> "$GITHUB_OUTPUT" elif [ "$HEAD_OWNER" = "Raftersecurity" ] || [ "$AUTHOR" = "Rome-1" ]; then echo "run=false" >> "$GITHUB_OUTPUT" - echo "Internal PR into main (author=$AUTHOR, head repo owner=$HEAD_OWNER) — suite skipped." >> "$GITHUB_STEP_SUMMARY" + echo "Internal PR into main (author=$AUTHOR, head repo owner=$HEAD_OWNER) — unit tests still run; extended matrix skipped." >> "$GITHUB_STEP_SUMMARY" else echo "run=true" >> "$GITHUB_OUTPUT" fi @@ -46,7 +61,7 @@ jobs: # ── Unit & integration tests (both languages) ───────────────────── test-node: needs: gate - if: needs.gate.outputs.run == 'true' + if: needs.gate.outputs.run_core == 'true' runs-on: ubuntu-latest defaults: run: @@ -91,7 +106,7 @@ jobs: test-python: needs: gate - if: needs.gate.outputs.run == 'true' + if: needs.gate.outputs.run_core == 'true' runs-on: ubuntu-latest defaults: run: @@ -228,6 +243,24 @@ jobs: if: ${{ env.RAFTER_API_KEY != '' }} run: pnpm exec vitest run tests/backend-api.test.ts + # sable-bm5k — without this the job renders identically whether it tested + # the backend or tested nothing. RAFTER_API_KEY has never been set on this + # repo, so "backend-api ✓" has always meant "checked out and built". + # A skipped step must not look like a passing one. + - name: Say so when the backend tests did not run + if: ${{ env.RAFTER_API_KEY == '' }} + run: | + echo "::warning::backend-api tested NOTHING — RAFTER_API_KEY is not set, so tests/backend-api.test.ts was skipped." + { + echo "### :warning: backend-api ran no tests" + echo "" + echo "\`RAFTER_API_KEY\` is unset, so \`tests/backend-api.test.ts\` was skipped." + echo "This job checked out and built the package and nothing else." + echo "" + echo "The remote scan path is covered without a key by the mock-backed jobs" + echo "in \`test-github-action.yml\`. See sable-bm5k." + } >> "$GITHUB_STEP_SUMMARY" + # ── Package build verification ───────────────────────────────────── package-integrity: needs: gate