Backstop against the qa-pro.yml/qa-suite.yml synchronize cost drift

.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:

.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

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:

plugins/claudegrill/scripts/setup-product.mjs

@@ -23,6 +23,7 @@
  *   node scripts/setup-product.mjs write-suite    --root <p>
  *   node scripts/setup-product.mjs write-env      --root <p>      # values via env
  *   node scripts/setup-product.mjs write-workflow --root <p>
+ *   node scripts/setup-product.mjs check-workflow --root <p>
  */
 
 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;
+  }
 }