From 111d7b0f712e6bc1bb8a7cf4b0a4364178676d36 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 17:45:19 +0000 Subject: [PATCH] fix: pre-landing review fixes (GST-1024: refuse a paid-only selection; validate skipped) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-1024-remote-gate-runs.md | 4 ++-- docs/pregate.md | 6 ++++-- lib/pregate/index.ts | 1 + test/pregate-remote-gate-runs.test.ts | 16 ++++++++++++++-- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/changelog.d/gst-1024-remote-gate-runs.md b/changelog.d/gst-1024-remote-gate-runs.md index 2c75bb63a..b16fda722 100644 --- a/changelog.d/gst-1024-remote-gate-runs.md +++ b/changelog.d/gst-1024-remote-gate-runs.md @@ -2,7 +2,7 @@ ### Added - **`.gstack/pregate.json` `remote_gate_runs`:** the `ci-reuse/free-tests` profile, with `scripts/test-free-shards.ts --list` as the inventory (the complete free suite the Free Tests plan shards). -- **`remote_gate_runs.skipped`** (optional, `lib/run-evidence.ts`, `lib/pregate/index.ts`, `lib/pregate/run-check.ts`): a command that reads the selected files from `PREGATE_FILES_FILE` and prints the ones the remote gate itself never runs. Those files need no run, and the receipt records them as `skipped=`. An inventory file stays required even when `skipped` names it, and a failing command refuses with `skipped_failed`. gstack's command prints the selected paid test files, the same split `scripts/ubicloud/pregate-remote.sh` makes. Without it, run 41484634592 still refused: all 71 of the selected files it didn't cover are paid. +- **`remote_gate_runs.skipped`** (optional, `lib/run-evidence.ts`, `lib/pregate/index.ts`, `lib/pregate/run-check.ts`): a command that reads the selected files from `PREGATE_FILES_FILE` and prints the ones the remote gate itself never runs. Those files need no run, and the receipt records them as `skipped=`. An inventory file stays required even when `skipped` names it, and a failing command refuses with `skipped_failed`. When every selected file is skipped, the clear refuses with `paid_only`, just as the gate's own `tests=0` report never clears. gstack's command prints the selected paid test files, the same split `scripts/ubicloud/pregate-remote.sh` makes. Without it, run 41484634592 still refused: all 71 of the selected files it didn't cover are paid. ### Tests -- `test/pregate-remote-gate-runs.test.ts`: gstack's inventory equals the Free Tests plan's file set, and its skipped command prints exactly the paid files of a mixed list. In a fixture with gstack's real profile, a green Free Tests `pull_request` run clears the gate. A run at another tree, a run of another workflow, a failed or cancelled run, a fork run, a merge commit behind its base, an uncovered inventory file, a skipped list naming an inventory file, a failing skipped command, and a declaration without `skipped` all refuse. On main, all three tests fail. +- `test/pregate-remote-gate-runs.test.ts`: gstack's inventory equals the Free Tests plan's file set, and its skipped command prints exactly the paid files of a mixed list. In a fixture with gstack's real profile, a green Free Tests `pull_request` run clears the gate. A run at another tree, a run of another workflow, a failed or cancelled run, a fork run, a merge commit behind its base, an uncovered inventory file, a skipped list naming an inventory file, a failing skipped command, a paid-only selection, and a declaration without `skipped` all refuse. Malformed `skipped` values fail config validation. On main, all three tests fail. diff --git a/docs/pregate.md b/docs/pregate.md index d3504c213..cd48942d0 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -740,7 +740,9 @@ Then, for the lane: (gstack's gate leaves paid test files to the paid PR gate). Those files need no run, and the receipt counts them as `skipped=`. A file the inventory prints stays required even when `skipped` names it, and a failing `skipped` - command refuses (`skipped_failed`). Without `skipped`, every selected file + command refuses (`skipped_failed`). When `skipped` names every selected file, + no selected test ran, so the clear refuses (`paid_only`), just as the gate's + own `tests=0` report never clears. Without `skipped`, every selected file must appear in a run's `files`. - Every run is read once more after the inventory: a run that moved to a new attempt or stopped being a success meanwhile refuses (`run_changed`). @@ -753,7 +755,7 @@ verification is named with its own reason (`fork_run`, `event_mismatch`, `os_mismatch`, `api_error`, …); the clear-level codes are `base_mismatch`, `no_verified_run`, `tree_mismatch` (runs on different trees), `tree_not_stamp_only`, `no_tests`, `inventory_failed`, `inventory_empty`, -`skipped_failed`, `inventory_uncovered` (the remote gate) and `tests_uncovered` (a lane's own +`skipped_failed`, `paid_only`, `inventory_uncovered` (the remote gate) and `tests_uncovered` (a lane's own selected tests). A wrong run count, a repeated run or a lane with no profile is a usage error (exit 2), not a REUSE refusal. diff --git a/lib/pregate/index.ts b/lib/pregate/index.ts index f79023491..41f678423 100644 --- a/lib/pregate/index.ts +++ b/lib/pregate/index.ts @@ -371,6 +371,7 @@ function clearByRuns(file: PregateFile, lane: string, refs: string[], check: Run if ('error' in skipped) return { refused: 'PREGATE_RECEIPT_INVALID', message: `REUSE: refused reason=skipped_failed — the remote gate's skipped command failed: ${skipped.error}; ${lane} stays uncleared` }; const inventory = new Set(listed); for (const f of skipped) if (!inventory.has(f) && required.delete(f)) skippedCount++; + if (!required.size) return { refused: 'PREGATE_RECEIPT_INVALID', message: `REUSE: refused reason=paid_only — the remote gate skips every selected test file (${skippedCount}), so no selected test ran; like the gate's own tests=0 report, that never clears; ${lane} stays uncleared` }; } for (const f of listed) required.add(f); } diff --git a/test/pregate-remote-gate-runs.test.ts b/test/pregate-remote-gate-runs.test.ts index 27b1def21..7eb901c56 100644 --- a/test/pregate-remote-gate-runs.test.ts +++ b/test/pregate-remote-gate-runs.test.ts @@ -121,7 +121,7 @@ describe.skipIf(process.platform === 'win32')('clear --run with the Free Tests p expect(msg(d)).toStartWith(`cleared remote-gate tree=${file.tree.slice(0, 12)} tests=${files.length * 20} skipped=1 runs=41484634592/1 profiles=${PROFILE} heads=${head.slice(0, 12)} verified=`); }, 120_000); - test('refusals: no skipped command, the wrong tree, the wrong workflow, a failed or cancelled run, a fork run, a merge behind its base, an uncovered inventory file, a skipped list that names an inventory file', async () => { + test('refusals: no skipped command, a paid-only selection, the wrong tree, the wrong workflow, a failed or cancelled run, a fork run, a merge behind its base, an uncovered inventory file, a skipped list that names an inventory file', async () => { const repo = fixture({ profiles: [PROFILE], inventory: INVENTORY, skipped: SKIPPED }); const older = repo.git('rev-parse', 'HEAD~1'); const head = repo.git('rev-parse', 'HEAD'); @@ -136,7 +136,19 @@ describe.skipIf(process.platform === 'win32')('clear --run with the Free Tests p expect(clear({ head, files, headRepo: 'attacker/fx' })).toContain('reason=fork_run run 41484634592 attempt 1 ran for attacker/fx, not fx/fx'); expect(clear({ head, files, mergeTree: repo.git('rev-parse', `${older}^{tree}`) })).toContain('reason=tree_mismatch'); expect(clear({ head, files: files.filter(f => f !== 'test/other.test.js') })).toContain("reason=inventory_uncovered — 1 of the"); - expect(clear({ head, files: files.filter(f => f !== 'test/other.test.js') }, { gateRuns: { profiles: [PROFILE], inventory: INVENTORY, skipped: 'cat "$PREGATE_FILES_FILE"; echo test/other.test.js' } })).toContain('(test/other.test.js)'); + expect(clear({ head, files: files.filter(f => f !== 'test/other.test.js') }, { gateRuns: { profiles: [PROFILE], inventory: INVENTORY, skipped: `${SKIPPED}; echo test/other.test.js` } })).toContain('(test/other.test.js)'); expect(clear({ head, files }, { gateRuns: { profiles: [PROFILE], inventory: INVENTORY, skipped: 'exit 3' } })).toContain('reason=skipped_failed'); + const paidOnly = structuredClone(file); + paidOnly.requires_remote!.find(r => r.lane === LANE)!.tests = [PAID]; + expect(msg(clearDecision(paidOnly, LANE, { runs: [RUN], evidence: gateCheck(repo, { head, files }) }, repo.work))).toBe(`PREGATE_RECEIPT_INVALID: REUSE: refused reason=paid_only — the remote gate skips every selected test file (1), so no selected test ran; like the gate's own tests=0 report, that never clears; ${LANE} stays uncleared`); }, 120_000); + + test('remote_gate_runs.skipped must be one command line, and an unknown key names it as valid', () => { + const profiles = { p: { workflow: 'w.yml', events: ['push'], jobs: ['j'], artifact: 'a' } }; + for (const skipped of [3, '', ' ', 'a\nb', 'x'.repeat(513)]) { + expect(runProfileErrors(profiles, { profiles: ['p'], inventory: 'echo t', skipped }).map(e => e.path)).toEqual(['remote_gate_runs.skipped']); + } + expect(runProfileErrors(profiles, { profiles: ['p'], inventory: 'echo t', skipped: 'echo s' })).toEqual([]); + expect(runProfileErrors(profiles, { profiles: ['p'], inventory: 'echo t', skip: 'echo s' })).toEqual([{ path: 'remote_gate_runs.skip', message: 'unknown key; valid keys: profiles, inventory, skipped' }]); + }); });