diff --git a/.github/workflows/pro-suite.yml b/.github/workflows/pro-suite.yml index 60ac9e8..5eaf0d6 100644 --- a/.github/workflows/pro-suite.yml +++ b/.github/workflows/pro-suite.yml @@ -205,11 +205,22 @@ jobs: gate: # The comment gate, identical to suite.yml's. When it is false, `run` # skips because it needs this job, and `answer` skips by testing for it. + # + # The `pull_request` clause is a cost backstop, not a courtesy check — see + # the long comment on suite.yml's `suite` job for the incident that made it + # necessary. `qa-pro.yml` is a per-repo caller with no central version lock, + # and one already drifted to include `synchronize`: 15+ full pro-suite runs + # in two days on one release PR, ~$5.4/day in billed Actions minutes, from + # a single re-added trigger type. Allow-listing here means that drift costs + # one skipped `gate` job — $0 — regardless of what any caller's own `on:` + # block says, because `run` needs `gate` and inherits the skip. if: >- - github.event_name != 'issue_comment' || ( + (github.event_name != 'pull_request' || + contains(fromJSON('["opened","reopened","ready_for_review"]'), github.event.action)) && + (github.event_name != 'issue_comment' || ( github.event.issue.pull_request != null && startsWith(github.event.comment.body, '@claudegrill suite') && - contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association))) runs-on: ubuntu-latest timeout-minutes: 2 permissions: diff --git a/.github/workflows/suite.yml b/.github/workflows/suite.yml index 80e1a13..85140b3 100644 --- a/.github/workflows/suite.yml +++ b/.github/workflows/suite.yml @@ -133,11 +133,26 @@ jobs: # and comes from someone with a stake in the repo. Anyone can comment on a # public PR; without the last clause anyone could spend runner minutes in a # loop. The caller filters on the command too, but this is the gate. + # + # The `pull_request` clause is a cost backstop, not a courtesy check. The + # design is "opened/reopened/ready_for_review, then on request" — a caller's + # own `on.pull_request.types` is SUPPOSED to leave `synchronize` out, so a + # push spends nothing. That is enforced by a file copied into every product + # repo individually, with no central version lock, and it already drifted + # once: a caller that re-added `synchronize` re-ran the full suite on every + # push to one release PR — 15+ full runs in two days, ~$5.4/day in billed + # Actions minutes, caught only when it showed up on the org bill. Allow-list + # here means that drift, in THIS repo or any future one, costs one skipped + # job — $0, no runner started — instead of a full run, even if the caller's + # own trigger is wrong. Fix the caller too (`check-workflow` below finds + # this without waiting for a bill), but do not rely on it alone. if: >- - github.event_name != 'issue_comment' || ( + (github.event_name != 'pull_request' || + contains(fromJSON('["opened","reopened","ready_for_review"]'), github.event.action)) && + (github.event_name != 'issue_comment' || ( github.event.issue.pull_request != null && startsWith(github.event.comment.body, '@claudegrill suite') && - contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association))) runs-on: ubuntu-latest # Above `timeout_ms` (30 min), not equal to it. The job ceiling kills the # process tree outright and reports nothing; `run-suite.mjs`'s own ceiling diff --git a/CLAUDE.md b/CLAUDE.md index d39336d..ee7f20d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -578,6 +578,45 @@ decision in the suite layer. against ColorMag Pro's real suite: with a 1.5s ceiling the run finished in 3.4s with the offending test named, rather than hanging. +- **The `synchronize` drift that actually showed up on a bill — and why it + costs $0 now regardless of which caller drifts.** `qa-pro.yml` in + `user-registration-pro` re-added `synchronize` to `on.pull_request.types`, + which both `suite.yml` and `pro-suite.yml` explicitly document callers must + drop. Every push to release PR #1572 re-ran the full pro suite — 15+ full + runs in two days, themegrill org Actions billing going from $0/day to + ~$5.4/day. Found via the org bill, not via anything in this repo, and + reported as [claudegrill#5](https://github.com/themegrill/claudegrill/issues/5). + The repo's own file was already hand-fixed 12 minutes after the issue was + filed; what was still missing was anything that stops it recurring, since + `qa-pro.yml`/`qa-suite.yml` are copied into each product repo individually + with no central version lock — two repos had already disagreed on this exact + line before this fix. + + Two changes, checked against real fixtures and the three live callers: + - **A cost backstop in the reusable workflows themselves**, not just the + callers. `suite.yml`'s `suite` job and `pro-suite.yml`'s `gate` job now + allow-list `pull_request` events to `opened`/`reopened`/`ready_for_review` + in their job-level `if:`; anything else (`synchronize` included) is a + skipped job — $0, no runner started — regardless of what any caller's own + `on:` block says. This is centralised on purpose: it protects every + current and future pro repo without needing every copy of the caller file + to be correct. + - **`setup-product.mjs check-workflow`**, a new read-only action: reads an + existing `qa-pro.yml`/`qa-suite.yml`, strips comment lines (the header + prose itself contains the string `pull_request_target` as a warning, which + a naive substring scan flagged as the violation — caught by testing against + the templates themselves, not assumed), and flags `synchronize` in + `pull_request.types` or any use of `pull_request_target`. Verified against + five fixtures (clean `qa-suite.yml`, clean `qa-pro.yml`, both with + `synchronize` added, and one with `pull_request_target`) plus a live pull of + all three real pro-repo callers (colormag-pro, user-registration-pro, + zakra-pro) — all three report `clean: true` today. This is Option A from + the issue: drift found on request, before a bill, rather than after one. + + Not done: Option D (surfacing runner-minutes somewhere more visible than the + org billing page) — the per-job "Report elapsed minutes" step already + reports to the run summary, but nothing rolls that up across runs or repos. + - **Three faults in this repo's own cost controls, found by them failing.** Worth recording because each one turned a diagnosable failure into an opaque one: diff --git a/plugins/claudegrill/scripts/setup-product.mjs b/plugins/claudegrill/scripts/setup-product.mjs index 682ea49..d628e5a 100644 --- a/plugins/claudegrill/scripts/setup-product.mjs +++ b/plugins/claudegrill/scripts/setup-product.mjs @@ -23,6 +23,7 @@ * node scripts/setup-product.mjs write-suite --root

* node scripts/setup-product.mjs write-env --root

# values via env * node scripts/setup-product.mjs write-workflow --root

+ * node scripts/setup-product.mjs check-workflow --root

*/ import { execFileSync } from "node:child_process"; @@ -45,6 +46,7 @@ const ACTIONS = [ "write-suite", "write-env", "write-workflow", + "check-workflow", ]; const opt = { type: null, slug: null, root: null, site: null, force: false }; @@ -574,6 +576,100 @@ function writeWorkflow() { return { ok: true, root, written: `.github/workflows/${name}`, pro: Boolean(proEntry) }; } +/** + * Does an existing caller workflow still match the cost-avoidance design? + * + * `write-workflow` never touches a file that already exists (without + * `--force`), which is correct — it is "safe to edit", per its own header, and + * a product may have reasons to diverge. But that also means nothing re-checks + * it after the fact, and the one caller written from this template has already + * drifted once: `qa-pro.yml` in a pro repo picked up `synchronize` in its + * `pull_request.types`, and every push to one release PR re-ran the full suite + * — 15+ full runs in two days, found only on the org's Actions bill days + * later. `suite.yml` and `pro-suite.yml` now refuse that event centrally (see + * their `gate`/`suite` job `if:`), so the same drift costs $0 even if a caller + * is wrong — but a wrong caller is still wrong, and this is how to find it + * without waiting for either a bill or the backstop to matter. + * + * Deterministic and read-only: a regex over the `on:` block, not a YAML parser + * (no dependencies — same convention as `envKeys` above), because every caller + * this repo has written shares the same one-line `types: [...]` shape. + */ +function checkWorkflow() { + const root = resolveRoot(); + const registry = loadRegistry(qaHome); + const proEntry = registry[path.basename(root)] ?? null; + const name = proEntry ? "qa-pro.yml" : "qa-suite.yml"; + const dest = path.join(root, ".github", "workflows", name); + + if (!fs.existsSync(dest)) { + return { ok: false, root, error: `no ${name} at .github/workflows/ — run write-workflow first` }; + } + + const body = fs.readFileSync(dest, "utf8"); + const findings = []; + + // Every template in this repo explains itself in prose above the YAML it + // documents — the header here literally says "never pull_request_target" + // — so a substring scan of the raw file flags its own warning comment as + // the violation. Strip full-line comments first; nothing in these callers + // puts a trailing comment on a code line, so this loses no real YAML. + const code = body + .split(/\r?\n/) + .filter((l) => !/^\s*#/.test(l)) + .join("\n"); + + // The design: opened/reopened/ready_for_review on pull_request, then only + // `@claudegrill suite` on request. Anything else in `types` costs metered + // minutes on every matching event a push produces. + const ALLOWED_PR_TYPES = ["opened", "reopened", "ready_for_review"]; + const m = code.match(/pull_request:\s*\n(?:[^\n]*\n)*?\s*types:\s*\[([^\]]*)\]/); + if (!m) { + findings.push({ + severity: "warning", + message: "no `pull_request: types: [...]` line found — could not verify the trigger at all", + }); + } else { + const types = m[1] + .split(",") + .map((s) => s.trim().replace(/^["']|["']$/g, "")) + .filter(Boolean); + const extra = types.filter((t) => !ALLOWED_PR_TYPES.includes(t)); + if (extra.includes("synchronize")) { + findings.push({ + severity: "error", + message: + "`synchronize` is in `pull_request.types` — every push to an open PR will re-run the " + + "full suite and spend metered runner minutes. Remove it; a push is meant to spend " + + "nothing, and `@claudegrill suite` is the way to re-run on demand.", + }); + } else if (extra.length) { + findings.push({ + severity: "warning", + message: `pull_request type(s) not in the canonical template: ${extra.join(", ")} — confirm this is deliberate`, + }); + } + } + + // Forbidden outright — see the reusable workflow's own header on this. + if (/pull_request_target/.test(code)) { + findings.push({ + severity: "error", + message: + "`pull_request_target` is present. It runs with this repo's secrets against fork-supplied " + + "code and must never replace `pull_request` here.", + }); + } + + return { + ok: true, + root, + path: `.github/workflows/${name}`, + clean: findings.length === 0, + findings, + }; +} + // ---------------------------------------------------------------------- main switch (action) { @@ -607,4 +703,11 @@ switch (action) { if (!r.ok) process.exit(2); break; } + case "check-workflow": { + const r = checkWorkflow(); + out(r); + if (!r.ok) process.exit(2); + if (r.findings.some((f) => f.severity === "error")) process.exit(1); + break; + } }