diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 733297e08..05d500a31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -121,9 +121,12 @@ jobs: # # A failure inside this step means RUN, never SKIP. objectstack#4928 named # that the filter contract, after a filter job that skipped when it could - # not tell produced a fully green, zero-gate pull request; the `|| echo ""` - # in the `docs` job below is the fail-CLOSED spelling and is why that job's - # own gate is reported separately (see the PR). + # not tell produced a fully green, zero-gate pull request. Every gate in + # this workflow is spelled that way — including the `docs` job below, whose + # capture swallowed its own failure into an empty result until + # objectui#3723. `scripts/__tests__/merge-queue-reporting.test.ts` holds + # every `CHANGED=$(git diff …)` in this file and in `lint.yml` to the + # fail-open form, so a new gate cannot be added in the closed spelling. - name: Decide whether this change needs a full run id: relevant run: | @@ -552,27 +555,46 @@ jobs: fetch-depth: 0 submodules: true + # Fails OPEN: if the diff cannot be computed the job builds the site, + # rather than reporting green having built nothing (objectstack#4928). + # Until objectui#3723 this step swallowed a failed `git diff` into an empty + # result (`2>/dev/null || echo ""`), which is indistinguishable from + # "nothing docs-related changed" — a checkout that did not fetch deep + # enough, a transient git failure or a malformed sha skipped the whole + # site build and the job still reported success. Measured against a + # fixture repository: on an unreachable base sha the old spelling yielded + # `should_run=false`, this one yields `true`; on a reachable base sha both + # agree, so which pull requests pay for a site build is unchanged. - name: Check for docs changes id: docs-changes run: | # `!= pull_request` covers `push` and `merge_group` alike # (objectui#3523). A queue build has no `github.event.pull_request`, - # so the else-branch below would diff an empty revision range and - # skip the site build on the last check before `main`. - if [ "${{ github.event_name }}" != "pull_request" ]; then - echo "should_run=true" >> "$GITHUB_OUTPUT" + # so the diff below would run on an empty revision range — and that + # case is NOT caught by failing open: git reads a bare `...` as + # `HEAD...HEAD` and exits 0 with no output (measured), so it would + # skip the site build on the last check before `main`. This early + # return is what covers it. + if [ "${{ github.event_name }}" != 'pull_request' ]; then + echo 'should_run=true' >> "$GITHUB_OUTPUT" + echo 'Not a pull request: push is filtered at the trigger, and a merge_group build is the last validation before main. Building the site.' + exit 0 + fi + if ! CHANGED=$(git diff --name-only \ + '${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}' -- \ + 'apps/site/' \ + 'content/'); then + echo 'should_run=true' >> "$GITHUB_OUTPUT" + echo 'Could not diff against the merge base. Building the site rather than skipping silently.' + exit 0 + fi + if [ -n "$CHANGED" ]; then + echo 'should_run=true' >> "$GITHUB_OUTPUT" + echo 'Docs-related files changed:' + echo "$CHANGED" else - # Check if docs-related files changed in this PR - CHANGED=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} -- \ - 'apps/site/' 'content/' 2>/dev/null || echo "") - if [ -n "$CHANGED" ]; then - echo "should_run=true" >> "$GITHUB_OUTPUT" - echo "Docs-related files changed:" - echo "$CHANGED" - else - echo "should_run=false" >> "$GITHUB_OUTPUT" - echo "No docs-related files changed, skipping build" - fi + echo 'should_run=false' >> "$GITHUB_OUTPUT" + echo 'No docs-related files changed. Skipping the steps below; this check still reports.' fi - name: Enable Corepack diff --git a/scripts/__tests__/merge-queue-reporting.test.ts b/scripts/__tests__/merge-queue-reporting.test.ts index fd6b9366f..31c72af93 100644 --- a/scripts/__tests__/merge-queue-reporting.test.ts +++ b/scripts/__tests__/merge-queue-reporting.test.ts @@ -111,6 +111,42 @@ const quotedEntries = (block: string): string[] => const excludePathspecs = (yaml: string): string[] => [...yaml.matchAll(/':\(exclude,glob\)([^']+)'/g)].map((m) => m[1]); +/** + * Every line capturing a `git diff` into `CHANGED` — one per gate, trimmed. + * + * Lines rather than whole commands, because the fail-open decision is made + * entirely by how the capture OPENS: `if ! CHANGED=$(…` is what turns a nonzero + * `git diff` into `should_run=true`. A capture continued over further lines + * (every one of these is) still contributes exactly one entry, so the count is + * the number of gates. + * + * Callers must strip comments first (`withoutComments`) — both workflows now + * discuss this exact shape in prose, and counting the prose would report gates + * no job has. + */ +const diffCaptureLines = (yaml: string): string[] => + yaml + .split('\n') + .filter((line) => /CHANGED=\$\(\s*git diff/.test(line)) + .map((line) => line.trim()); + +/** + * `file -> the jobs whose diff capture must fail open`. + * + * Hand-maintained for the same reason as `MUST_SUBSCRIBE_MERGE_GROUP`, and used + * only as a FLOOR: the assertion below checks the shape of every capture it + * finds, so a sixth gate is covered the moment it is written, while the floor is + * what makes DELETING one red. Without it, the shape check would pass an empty + * list — green because nothing was produced, not because anything is right. + */ +const FAIL_OPEN_GATES = new Map([ + // `type-check`, `test` and `e2e` share `id: relevant` (objectui#3523 / PR + // #3722); `docs` has its own `id: docs-changes` and predates them (#3450), + // which is how it kept the fail-CLOSED spelling until objectui#3723. + ['ci.yml', ['type-check', 'test', 'e2e', 'docs']], + ['lint.yml', ['lint']], +]); + describe('every requirable context reports on a merge-queue build (#3523 step 1)', () => { it('subscribes merge_group in each workflow that produces one', () => { const missing = [...MUST_SUBSCRIBE_MERGE_GROUP.keys()].filter( @@ -242,24 +278,66 @@ describe('every context reports on every pull request (#3523 step 2)', () => { } }); - it('fails OPEN: a gate that cannot compute the diff runs everything', () => { + it('fails OPEN: EVERY gate that cannot compute the diff runs everything', () => { // The direction matters more than the code. objectstack#4928 named this the // filter contract after the opposite spelling — a diff whose failure was // swallowed into an empty result — produced a fully green pull request with - // no job having run and no red signal anywhere. `ci.yml`'s older `docs` gate - // still uses that fail-CLOSED spelling (`|| echo ""`); the gates added by - // objectui#3523 must not copy it. + // no job having run and no red signal anywhere. + // + // This assertion used to be a single `toMatch` per FILE, which is why it + // reported green while `ci.yml`'s `docs` gate was still fail-CLOSED + // (objectui#3723): the three gates PR #3722 added satisfied the regex on + // ci.yml's behalf, and a whole-file match cannot tell four captures from + // three plus a swallow. It is now per CAPTURE, so each gate answers for + // itself and a fifth spelled the closed way cannot hide behind the others. for (const file of FILTER_MOVED_INTO_JOBS) { - const yaml = read(file); - const gates = [...yaml.matchAll(/^\s*id: relevant$/gm)]; - expect(gates.length, `${file} must still carry at least one \`id: relevant\` gate`).toBeGreaterThan(0); + const expected = FAIL_OPEN_GATES.get(file) ?? []; + const captures = diffCaptureLines(withoutComments(read(file))); expect( - yaml, - `${file}'s short-circuit swallows a failed \`git diff\` into an empty result, which reads ` + - `as "nothing relevant changed" and skips every gate. When the filter cannot tell, it ` + - `must RUN (objectstack#4928).`, - ).toMatch(/if ! CHANGED=\$\(git diff --name-only/); + captures.length, + `${file} declares ${captures.length} \`CHANGED=$(git diff …)\` captures, fewer than the ` + + `${expected.length} gates that must have one (${expected.join(', ')}). A gate whose diff ` + + `capture is gone either no longer filters at all, or filters by some other means this ` + + `test cannot check the direction of — and with none left, the shape assertion below ` + + `passes an empty list (objectui#3523, objectui#3723).`, + ).toBeGreaterThanOrEqual(expected.length); + + const failClosed = captures.filter((line) => !line.startsWith('if ! CHANGED=$(git diff')); + expect( + failClosed, + `${file} captures a \`git diff\` without letting its failure mean RUN:\n` + + failClosed.map((line) => ` - ${line}`).join('\n') + + `\n\nEvery gate must open its capture as \`if ! CHANGED=$(git diff …); then\` and treat ` + + `the failure branch as should_run=true. Swallowing the failure instead — the ` + + `\`2>/dev/null || echo ""\` this file's \`docs\` job used until objectui#3723 — makes ` + + `"the diff could not be computed" indistinguishable from "nothing changed", so a ` + + `shallow checkout or a malformed sha skips the job's real work and still reports ` + + `success. When the filter cannot tell, it must RUN (objectstack#4928).`, + ).toEqual([]); + } + }); + + it('fails OPEN in effect, not only in shape: no gate swallows its diff failure', () => { + // `if !` is necessary but not sufficient. `if ! CHANGED=$(git diff … || echo + // "")` reads as fail-open and is not: `|| echo ""` makes the command succeed + // whatever git did, so the failure branch becomes unreachable and the gate is + // fail-CLOSED again with the safe spelling wrapped around it. Same for + // `2>/dev/null`, which additionally deletes git's own explanation of what + // went wrong from the run log — the one diagnostic a future reader gets. + for (const file of FILTER_MOVED_INTO_JOBS) { + const code = withoutComments(read(file)); + for (const [shape, why] of [ + [/\|\|\s*echo\s*""/, '`|| echo ""` makes the capture succeed even when git failed'], + [/2>\s*\/dev\/null/, '`2>/dev/null` hides why git failed'], + ] as const) { + expect( + code, + `${file} still contains ${why}. Both halves of the fail-CLOSED spelling objectui#3723 ` + + `removed must stay out: the gate's failure branch has to be reachable, and the run ` + + `log has to say what happened (objectstack#4928).`, + ).not.toMatch(shape); + } } }); });