diff --git a/.github/workflows/pr-codex-review.yml b/.github/workflows/pr-codex-review.yml index dc62e250164..1c702feb038 100644 --- a/.github/workflows/pr-codex-review.yml +++ b/.github/workflows/pr-codex-review.yml @@ -28,14 +28,26 @@ name: PR Codex Review on: pull_request: - types: [opened, synchronize, reopened] + # `ready_for_review` is load-bearing: every draft-phase run (opened, + # synchronize) skips on the `draft == false` gate below, so without this + # trigger a PR opened as a draft and later marked ready never gets Codex + # review — the ready click fires no run, and only an incidental post-ready + # push would. An explicit `types:` list is a denylist by omission. + # Mirrors ci-workflows/pr-codex-review.yml. See ci-workflows#115. + types: [opened, synchronize, reopened, ready_for_review] permissions: contents: read pull-requests: write +# The group key partitions runs by the payload's draft flag, not just per-PR. A +# `synchronize` run whose payload was snapshotted while the PR was still draft is +# a guaranteed no-op (the draft gate below), yet in a shared group it can cancel +# — or, if queued, replace regardless of cancel-in-progress — the +# `ready_for_review` run, so a sensitive/blocked PR taken draft → ready would +# still merge with zero Codex review (attaxion_dev#303, 2026-07-06). concurrency: - group: codex-${{ github.event.pull_request.number }} + group: codex-${{ github.event.pull_request.number }}-${{ github.event.pull_request.draft && 'draft' || 'live' }} cancel-in-progress: true jobs: diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 3d1b92cb491..20a6099862c 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -2,7 +2,15 @@ name: PR Review on: pull_request: - types: [opened, synchronize, reopened] + # `ready_for_review` is load-bearing: the reusable + # (ci-workflows/claude-review.yml) gates its job on `draft == false`, so a + # PR opened as a draft skips the review at `opened`. Without this trigger, + # `gh pr ready` fires nothing this workflow listens for and the review NEVER + # runs — `review / Claude Review` sits at `skipped`, which GitHub counts as + # SATISFIED, so the PR merges green and unreviewed. An explicit `types:` list + # is a denylist by omission. Fleet audit 2026-07-15; template root cause + # fixed in dotclaude#156. See ci-workflows#115, domain-rank#35. + types: [opened, synchronize, reopened, ready_for_review] # No paths-ignore by default. Claude self-skips docs/config-only diffs # via the reusable workflow's prompt and posts a "Skipped: ..." summary, # which keeps the review/Claude Review check green. Adding paths-ignore @@ -18,8 +26,15 @@ permissions: pull-requests: write id-token: write +# The group key partitions runs by the payload's draft flag, not just per-PR. A +# `synchronize` run whose payload was snapshotted while the PR was still draft is +# a guaranteed no-op (the reusable's draft gate), yet in a shared group it can +# cancel the `ready_for_review` run — so a PR taken draft → ready would still +# merge with zero review, which is the very failure the trigger above fixes. +# Non-draft runs keep today's supersede semantics: a newer push still cancels an +# in-flight stale run. Mirrors ci-workflows/pr-review.yml (attaxion_dev#303). concurrency: - group: claude-review-${{ github.event.pull_request.number }} + group: claude-review-${{ github.event.pull_request.number }}-${{ github.event.pull_request.draft && 'draft' || 'live' }} cancel-in-progress: true jobs: diff --git a/tests/regression/test_workflow_ready_for_review.test.ts b/tests/regression/test_workflow_ready_for_review.test.ts new file mode 100644 index 00000000000..4a9d9001b09 --- /dev/null +++ b/tests/regression/test_workflow_ready_for_review.test.ts @@ -0,0 +1,138 @@ +// Lesson 2026-07-16: a workflow that gates on `draft == false` but does not listen +// for `ready_for_review` FAILS OPEN — its required check reports `skipped`, and +// GitHub counts a skipped REQUIRED context as SATISFIED, so the PR merges green and +// UNREVIEWED. +// +// The sequence, for a caller whose `types:` omits `ready_for_review`: +// 1. PR opened as a draft -> `opened` fires -> job skips on the draft gate (correct) +// 2. `gh pr ready` -> `ready_for_review` fires -> caller is not listening +// 3. nothing runs; `review / Claude Review` stays `skipped` -> counted as SATISFIED +// +// Fleet policy makes DRAFT the standard auto-merge opt-out, so every manual-merge PR +// is draft->ready — the recommended workflow was the one guaranteed to skip the review. +// +// The transitive case is why this test PARSES instead of grepping: pr-review.yml +// contains no `draft` expression at all, because the gate lives in the reusable it +// calls (ci-workflows/claude-review.yml). Grepping the caller cannot see it. +// +// An explicit `types:` list is a DENYLIST BY OMISSION, and this exact mistake has now +// been made twice fleet-wide (`reopened` was dropped first, then `ready_for_review` was +// dropped directly below the comment warning about `reopened`). A comment did not +// prevent the recurrence, so the rule is pinned to the runtime here. +// +// See: ci-workflows#115 (central reusable), dotclaude#156 (caller template), +// domain-rank#35 (reference fix). Fleet sweep 2026-07-16 found 21 repos still vulnerable. + +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +import yaml from 'js-yaml'; +import { describe, expect, it } from 'vitest'; + +const workflowsDir = path.join( + path.dirname(fileURLToPath(import.meta.url)), + '../../.github/workflows', +); + +// GitHub's default `pull_request` activity types. `ready_for_review` is NOT among +// them, which is why omitting an explicit `types:` list is not a safe default for a +// draft-gated workflow: the default is itself a denylist. +const GITHUB_DEFAULT_PR_TYPES = ['opened', 'synchronize', 'reopened']; + +// Reusables in topcoder1/ci-workflows whose jobs gate on `draft == false`. A caller +// inherits that gate transitively. Verified against every `workflow_call` reusable on +// 2026-07-16; extend this set if a new draft-gating reusable is adopted. +const DRAFT_GATED_REUSABLES = new Set([ + 'claude-review.yml', + 'claude-author-automerge.yml', + 'safe-paths-automerge.yml', + 'codex-review.yml', + 'claude-adversarial-review.yml', +]); + +interface Workflow { + on?: unknown; + jobs?: Record; +} + +function triggers(wf: Workflow): Record { + // js-yaml keeps `on` as the string key "on" (unlike PyYAML, which resolves the bare + // key to boolean true under YAML 1.1). Accept both so this cannot silently read + // undefined and pass vacuously. + const raw = (wf.on ?? (wf as Record)['true']) as unknown; + if (typeof raw === 'string') return { [raw]: {} }; + if (Array.isArray(raw)) + return Object.fromEntries(raw.map((k) => [String(k), {}])); + return (raw ?? {}) as Record; +} + +function prTypes(wf: Workflow): string[] { + const pr = triggers(wf)['pull_request']; + if (pr === null || typeof pr !== 'object') return GITHUB_DEFAULT_PR_TYPES; + const types = (pr as { types?: unknown }).types; + return Array.isArray(types) && types.length + ? types.map(String) + : GITHUB_DEFAULT_PR_TYPES; +} + +function hasDraftGate(wf: Workflow): boolean { + for (const job of Object.values(wf.jobs ?? {})) { + if (!job) continue; + if (String(job.if ?? '').includes('draft == false')) return true; + const base = String(job.uses ?? '') + .split('@')[0] + .split('/') + .pop(); + if (base && DRAFT_GATED_REUSABLES.has(base)) return true; + } + return false; +} + +const workflowFiles = fs + .readdirSync(workflowsDir) + .filter((f) => f.endsWith('.yml') || f.endsWith('.yaml')) + .sort(); + +describe('draft-gated workflows listen for ready_for_review', () => { + it('finds the workflows directory', () => { + // Guards the rest of the suite: a wrong path would make every case below + // vacuously pass and silently retire this regression test. + expect(workflowFiles.length).toBeGreaterThan(0); + }); + + it('detects a draft gate inherited from a called reusable', () => { + // The load-bearing assertion. A caller with no `draft` text of its own still + // inherits the gate; if this ever regresses to a text search over the caller, the + // rule below would pass vacuously on exactly the files it exists to protect. + const caller = yaml.load( + [ + 'on:', + ' pull_request:', + ' types: [opened]', + 'jobs:', + ' review:', + ' uses: topcoder1/ci-workflows/.github/workflows/claude-review.yml@main', + ].join('\n'), + ) as Workflow; + expect(JSON.stringify(caller)).not.toContain('draft'); + expect(hasDraftGate(caller)).toBe(true); + }); + + it.each(workflowFiles)('%s', (file) => { + const wf = yaml.load( + fs.readFileSync(path.join(workflowsDir, file), 'utf8'), + ) as Workflow; + if (!('pull_request' in triggers(wf))) return; // not a pull_request workflow + if (!hasDraftGate(wf)) return; // no draft gate, locally or via a reusable + + expect( + prTypes(wf), + `${file} gates on \`draft == false\` but does not list \`ready_for_review\` in its ` + + `pull_request types [${prTypes(wf).join(', ')}]. A draft PR skips its job at ` + + `\`opened\`, and \`gh pr ready\` would fire nothing this workflow hears — so its ` + + `check reports \`skipped\`, which GitHub counts as a SATISFIED required context. ` + + `The PR merges green and unreviewed. See ci-workflows#115, domain-rank#35.`, + ).toContain('ready_for_review'); + }); +});