From 4b150ff5dc47062e91f78f245255989fd9f24cce Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Fri, 4 Sep 2026 16:37:05 -0400 Subject: [PATCH 1/3] ci(pr-checks): fail closed when the test job is skipped under pull_request_target --- .github/workflows/pr-checks.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index b9fe5b71..159b0590 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -128,3 +128,30 @@ jobs: - name: Run test suite run: npm test + + test-gate: + # Fail-closed aggregation. `test` is skipped (not failed) under + # pull_request_target by design (see its own `if:` comment above) — GitHub + # does not fail a run because a job was skipped, so a head that produces + # ONLY a pull_request_target run previously reported the whole workflow + # SUCCESS with the test suite never executed. This job closes that gap: + # it runs on every event this workflow listens to, including + # pull_request_target, and turns "test did not run" into an explicit + # failure instead of silence. + # + # SECURITY: this job MUST stay a pure metadata assertion. No checkout of + # PR code, no npm install, no execution of anything from the PR — it only + # reads the `test` job's reported `result`. Adding any of those under + # pull_request_target would reintroduce the exact secret-exposure risk + # the `if:` on the `test` job exists to prevent. + needs: test + if: always() + runs-on: ubuntu-latest + steps: + - name: Require test job to have actually run and passed + run: | + echo "test job result: ${{ needs.test.result }}" + if [ "${{ needs.test.result }}" != "success" ]; then + echo "::error::test job did not succeed (result: ${{ needs.test.result }}). A skipped or failed test job must not yield a green PR Checks run." >&2 + exit 1 + fi From 248d5a1d112ba5ccd7539c05e5c0e5c144270a5a Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Fri, 4 Sep 2026 16:58:40 -0400 Subject: [PATCH 2/3] test(pr-checks): demonstrate test-gate fails closed for non-success needs.test.result (lr-665c62) --- ...ks-test-gate-fail-closed-lr-665c62.test.js | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 test/pr-checks-test-gate-fail-closed-lr-665c62.test.js diff --git a/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js b/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js new file mode 100644 index 00000000..08ae10a2 --- /dev/null +++ b/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js @@ -0,0 +1,135 @@ +// pr-checks-test-gate-fail-closed-lr-665c62.test.js +// +// Regression coverage for lr-665c62: PR Checks (.github/workflows/pr-checks.yml) +// could report the whole run SUCCESS while the `test` job was SKIPPED (not +// failed) — GitHub does not fail a run because a job was skipped, so a head +// that produced only a pull_request_target run previously reported green +// with `npm test` never having executed. Commit 4b150ff added a `test-gate` +// job that asserts `needs.test.result == 'success'` and fails otherwise. +// +// PEACHES (PR #418, comment 5546266561) held this BLOCKING: the guard's +// correctness was reasoned from reading the YAML, never demonstrated by an +// actual failing execution. This file closes that gap the same way lr-243b +// closed an analogous one for release.yml (see +// test/release-workflow-promote-version-guard-lr-243b.test.js) — no YAML +// parser dependency (none in package.json), so it extracts the test-gate +// step's run: block VERBATIM from the workflow text and executes it under +// `sh -c` with `needs.test.result` substituted exactly as GitHub Actions' +// `${{ }}` expression interpolation would substitute it: a literal string, +// spliced into the script text before any shell ever runs it. That is the +// real substitution mechanism (GHA expressions are evaluated by the runner, +// not by the step's shell), so this test exercises the identical script the +// runner would execute for each value, not an approximation of it. + +var test = require("node:test"); +var assert = require("node:assert"); +var fs = require("fs"); +var path = require("path"); +var childProcess = require("child_process"); + +var workflowPath = path.join(__dirname, "..", ".github", "workflows", "pr-checks.yml"); +var yml = fs.readFileSync(workflowPath, "utf8"); + +function jobBody(jobName) { + var jobHeaderRe = new RegExp("^ " + jobName + ":", "m"); + var match = jobHeaderRe.exec(yml); + assert.ok(match, "expected to find job `" + jobName + "` in pr-checks.yml"); + var start = match.index; + var rest = yml.slice(start + match[0].length); + var nextJobMatch = /^\n {2}\S.*:\n/m.exec(rest); + var end = nextJobMatch ? start + match[0].length + nextJobMatch.index : yml.length; + return yml.slice(start, end); +} + +function stepBody(job, stepName) { + var stepHeaderRe = new RegExp("- name: " + stepName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")); + var match = stepHeaderRe.exec(job); + assert.ok(match, "expected to find step `" + stepName + "` in job"); + var start = match.index; + var rest = job.slice(start + match[0].length); + var nextStepMatch = /\n {6}- name:/.exec(rest); + var end = nextStepMatch ? start + match[0].length + nextStepMatch.index : job.length; + return job.slice(start, end); +} + +test("test-gate job needs the test job and runs on always()", function () { + var gate = jobBody("test-gate"); + assert.match(gate, /needs:\s*test/); + assert.match(gate, /if:\s*always\(\)/); +}); + +test("test job stays excluded from pull_request_target (safety property, must not regress)", function () { + var testJob = jobBody("test"); + assert.match(testJob, /if:\s*github\.event_name != 'pull_request_target'/); +}); + +test("test-gate job stays a pure metadata assertion: no checkout, no npm install", function () { + var gate = jobBody("test-gate"); + assert.doesNotMatch(gate, /actions\/checkout/); + // Check only the executed run: bodies, not the job's own header comments — + // the job's SECURITY comment names "npm install" in prose to explain the + // constraint, which must not make this check trip on its own documentation. + var runBlocks = gate.match(/run:\s*\|[\s\S]*?(?=\n {6}- name:|$)/g) || []; + runBlocks.forEach(function (block) { + assert.doesNotMatch(block, /npm (ci|install)/); + }); +}); + +function extractGateScript() { + var gate = jobBody("test-gate"); + var step = stepBody(gate, "Require test job to have actually run and passed"); + var runMatch = /run:\s*\|\n([\s\S]*)$/.exec(step); + assert.ok(runMatch, "expected to extract the test-gate run: block"); + // De-indent (the block is indented 10 spaces under `run: |` in the workflow). + var lines = runMatch[1].split("\n").map(function (line) { + return line.replace(/^ {10}/, ""); + }); + // Drop trailing blank lines left over from the step/job boundary trim. + while (lines.length && lines[lines.length - 1].trim() === "") { + lines.pop(); + } + var text = lines.join("\n"); + assert.match( + text, + /\$\{\{\s*needs\.test\.result\s*\}\}/, + "expected the extracted script to still reference the needs.test.result expression" + ); + return text; +} + +// GitHub Actions expression interpolation (`${{ ... }}`) is a literal text +// substitution performed by the runner BEFORE the shell ever sees the +// script — it is not a shell variable. Reproducing that substitution here +// (rather than sourcing needs.test.result through env:, which is not how +// the real step is written) keeps this test byte-faithful to what actually +// runs, including the direct-interpolation shape of the real step. +function runGate(resultValue) { + var script = extractGateScript().split("${{ needs.test.result }}").join(resultValue); + return childProcess.spawnSync("sh", ["-c", script], { encoding: "utf8" }); +} + +test("guard behavior: passes when the test job result is success", function () { + var result = runGate("success"); + assert.strictEqual(result.status, 0, "expected the guard to pass for success: " + result.stderr); +}); + +test("guard behavior: fails closed when the test job result is skipped", function () { + var result = runGate("skipped"); + assert.notStrictEqual(result.status, 0, "expected the guard to fail for skipped"); + assert.match(result.stdout + result.stderr, /did not succeed/); +}); + +test("guard behavior: fails closed when the test job result is failure", function () { + var result = runGate("failure"); + assert.notStrictEqual(result.status, 0, "expected the guard to fail for failure"); +}); + +test("guard behavior: fails closed when the test job result is cancelled", function () { + var result = runGate("cancelled"); + assert.notStrictEqual(result.status, 0, "expected the guard to fail for cancelled"); +}); + +test("guard behavior: fails closed when the test job result is the empty string", function () { + var result = runGate(""); + assert.notStrictEqual(result.status, 0, "expected the guard to fail for an empty/unset result"); +}); From 300e0e167a32da6bf4eb811f1c57739193829a81 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Mon, 7 Sep 2026 09:14:07 -0400 Subject: [PATCH 3/3] docs(pr-checks): correct false test-gate dispatch claims (lr-665c62) MILLER fnd-3973e2 (re-diagnosis) established that test-gate's needs:test + if: always() rescues a dependency that FAILED or was CANCELLED, but does NOT rescue one skipped by its own job-level if: -- the skip propagates. Under pull_request_target, test is skipped by its own if:, so test-gate is never scheduled at all on the event this defect actually manifests on. It produces no check-run, not even a skipped one. This is documentation repair, not a fix -- the workflow layer cannot express 'this SHA was tested by some sibling run', so no YAML edit here can close the gap. Closing it requires branch protection requiring the 'test' check by name (operator/repo-settings action, tracked on lr-665c62). Changes: - pr-checks.yml test-gate job comment: remove the false claim that it 'runs on every event this workflow listens to, including pull_request_target'. State plainly what it does cover (test dispatched but failed/cancelled) and what it does not (the pull_request_target skip-propagation case), and why that residual requires branch protection rather than a workflow edit. - test file header: remove the claim that this file closes the PEACHES demonstrated-failure gap. It proves the extracted shell conditional is correct for every needs.test.result value (a real, worth-keeping property) -- it does not and cannot prove the job is ever dispatched under pull_request_target, since it never models GitHub's job scheduling layer. Tests unchanged in behavior -- same assertions, same pass/fail outcomes. npm test: 1590 passed, 0 failed. TASK: lr-665c62 --- .github/workflows/pr-checks.yml | 30 ++++++++--- ...ks-test-gate-fail-closed-lr-665c62.test.js | 54 ++++++++++++------- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 159b0590..bd7c2f23 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -130,14 +130,28 @@ jobs: run: npm test test-gate: - # Fail-closed aggregation. `test` is skipped (not failed) under - # pull_request_target by design (see its own `if:` comment above) — GitHub - # does not fail a run because a job was skipped, so a head that produces - # ONLY a pull_request_target run previously reported the whole workflow - # SUCCESS with the test suite never executed. This job closes that gap: - # it runs on every event this workflow listens to, including - # pull_request_target, and turns "test did not run" into an explicit - # failure instead of silence. + # Fail-closed aggregation for the cases where `test` is DISPATCHED but + # does not succeed: `needs: test` + `if: always()` means this job still + # runs (and fails) when `test` fails or is cancelled, turning that + # failure into an explicit red check instead of a silent partial result. + # + # WHAT THIS DOES NOT COVER (lr-665c62, MILLER fnd-3973e2): under + # pull_request_target, `test` is skipped by its OWN `if:` above — and a + # dependent job's `if: always()` only overrides the default success() + # condition for a job GitHub actually SCHEDULES; it does not resurrect a + # dependent whose dependency was skipped by that dependency's own + # job-level `if:`. The skip PROPAGATES, so on a head that produces only a + # pull_request_target run, this job is never scheduled at all — it + # produces no check-run, not even a skipped one. That is precisely the + # case this job was believed to close, and it does not. + # + # A workflow-layer fix for that case is not possible: a run cannot see + # its sibling runs, so nothing expressible in this file can assert "this + # SHA was tested by some OTHER run." Closing it requires branch + # protection that requires the `test` check BY NAME — protection + # evaluates across the whole check suite for a SHA, and a required check + # that is absent or skipped is not satisfied. That is an operator/repo- + # settings action, not something any workflow edit here can express. # # SECURITY: this job MUST stay a pure metadata assertion. No checkout of # PR code, no npm install, no execution of anything from the PR — it only diff --git a/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js b/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js index 08ae10a2..6d8cd5ab 100644 --- a/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js +++ b/test/pr-checks-test-gate-fail-closed-lr-665c62.test.js @@ -1,25 +1,43 @@ // pr-checks-test-gate-fail-closed-lr-665c62.test.js // -// Regression coverage for lr-665c62: PR Checks (.github/workflows/pr-checks.yml) -// could report the whole run SUCCESS while the `test` job was SKIPPED (not -// failed) — GitHub does not fail a run because a job was skipped, so a head -// that produced only a pull_request_target run previously reported green -// with `npm test` never having executed. Commit 4b150ff added a `test-gate` -// job that asserts `needs.test.result == 'success'` and fails otherwise. +// Coverage for the `test-gate` job's shell conditional in +// .github/workflows/pr-checks.yml (lr-665c62). Commit 4b150ff added +// `test-gate`, which asserts `needs.test.result == 'success'` and fails +// otherwise, for the case where the `test` job is DISPATCHED but fails or +// is cancelled. // -// PEACHES (PR #418, comment 5546266561) held this BLOCKING: the guard's -// correctness was reasoned from reading the YAML, never demonstrated by an -// actual failing execution. This file closes that gap the same way lr-243b -// closed an analogous one for release.yml (see -// test/release-workflow-promote-version-guard-lr-243b.test.js) — no YAML -// parser dependency (none in package.json), so it extracts the test-gate -// step's run: block VERBATIM from the workflow text and executes it under -// `sh -c` with `needs.test.result` substituted exactly as GitHub Actions' -// `${{ }}` expression interpolation would substitute it: a literal string, -// spliced into the script text before any shell ever runs it. That is the -// real substitution mechanism (GHA expressions are evaluated by the runner, -// not by the step's shell), so this test exercises the identical script the +// WHAT THIS FILE PROVES, and no more: that the extracted shell conditional +// itself is correct for every value of `needs.test.result` (success, +// skipped, failure, cancelled, empty). No YAML parser dependency (none in +// package.json), so it extracts the test-gate step's run: block VERBATIM +// from the workflow text and executes it under `sh -c` with +// `needs.test.result` substituted exactly as GitHub Actions' `${{ }}` +// expression interpolation would substitute it: a literal string, spliced +// into the script text before any shell ever runs it. That is the real +// substitution mechanism (GHA expressions are evaluated by the runner, not +// by the step's shell), so this test exercises the identical script the // runner would execute for each value, not an approximation of it. +// +// WHAT THIS FILE DOES NOT PROVE, and this is the gap that matters (MILLER +// fnd-3973e2, lr-665c62 comment thread): it never models GitHub's job +// SCHEDULING layer, only the shell script's behavior once invoked. In +// production, under pull_request_target, `test` is skipped by its own +// job-level `if:` — and that skip PROPAGATES to `test-gate` through +// `needs: test` even though `test-gate` has `if: always()`. always() only +// rescues a dependent whose dependency FAILED or was CANCELLED; it does not +// resurrect a dependent whose dependency was skipped by the dependency's +// own `if:`. So on the exact event this job exists to guard, `test-gate` is +// never scheduled at all — it produces no check-run, not a skipped one, not +// a failed one. The case at :~116-120 below, "fails closed when the test +// job result is skipped", is the exact production scenario, and it passes — +// while production never reaches the script this test is exercising. This +// file therefore closes PEACHES's "unreached script" concern for the +// failed/cancelled cases only; it does NOT close lr-665c62 or demonstrate +// that the guard fires in the pull_request_target skip case, because it +// cannot: that gap is a job-dispatch property, not a script property, and +// is not something a shell-level test can exercise. Closing it requires +// branch protection requiring the `test` check by name (operator action, +// tracked on lr-665c62), not another test in this file. var test = require("node:test"); var assert = require("node:assert");