fix(ci): keep Ready blocked while deterministic hygiene fails

.github/scripts/enforce-pr-target.test.cjs

@@ -49,14 +49,16 @@ describe("enforce-pr-target workflow", () => {
     assert.match(workflow, /synchronize/);
   });
 
-  it("uses label events for GUI waivers and a trusted CodeRabbit status signal", () => {
+  it("uses label events for GUI waivers, hygiene sponsorship, and a trusted CodeRabbit status signal", () => {
     assert.doesNotMatch(workflow, /^  issue_comment:/m);
     assert.match(workflow, /- labeled/);
     assert.match(workflow, /- unlabeled/);
     assert.match(workflow, /^  status:/m);
     assert.match(workflow, /github\.event\.context == 'CodeRabbit'/);
     assert.match(workflow, /github\.event\.state == 'success'/);
     assert.match(workflow, /github\.event\.label\.name == 'gui-screenshot-waived'/);
+    assert.match(workflow, /github\.event\.label\.name == 'intake: hygiene-blocked'/);
+    assert.match(workflow, /github\.event\.label\.name == 'maintainer-sponsored'/);
     assert.match(workflow, /listPullRequestsAssociatedWithCommit/);
     assert.match(workflow, /candidate\.head\?\.sha === statusSha/);
     assert.match(workflow, /candidates\.length !== 1/);
@@ -180,6 +182,9 @@ describe("enforce-pr-target workflow", () => {
   it("loads pr-quality via require from the checked-out scripts", () => {
     assert.match(workflow, /pr-quality\.cjs/);
     assert.match(workflow, /collectPrQualityFailures/);
+    assert.match(workflow, /pr-hygiene\.cjs/);
+    assert.match(workflow, /collectDeterministicHygieneFailures/);
+    assert.match(workflow, /pulls\.listFiles/);
   });
 
   it("checks stacked bases via open PR heads before wrong_base enforcement", () => {

.github/scripts/pr-hygiene.cjs

@@ -1,5 +1,7 @@
 "use strict";
 
+const { assessSponsoredSurface } = require("./pr-sponsored-surface.cjs");
+
 const GENERATED_PREFIXES = [
   "gui/dist/",
   "dist/",
@@ -218,9 +220,76 @@ function assessHygiene({ files = [], labels = [] }) {
   return failures;
 }
 
+/**
+ * Human-readable one-liners for each deterministic hygiene failure code.
+ * Shared by the hygiene workflow comment and the PR quality gate actions.
+ */
+const HYGIENE_FAILURE_HINTS = {
+  missing_regression_test:
+    "Behavior changed under `src/` or `gui/src/` without a test change. Add focused coverage or obtain `test-exception-approved`.",
+  generated_output:
+    "Generated build output is committed. Remove it or obtain `generated-change-approved`.",
+  orphan_lockfile:
+    "`bun.lock` changed without `package.json`. Revert accidental churn or obtain `dependency-change-approved`.",
+  new_suppression:
+    "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.",
+  focused_or_skipped_test:
+    "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.",
+  empty_catch:
+    "An empty catch block was added. Handle, report, or deliberately propagate the error.",
+  unsponsored_surface:
+    "This changes an authentication, workflow, release-automation, or dependency surface. `MAINTAINERS.md` requires security review for these; ask a maintainer to apply `maintainer-sponsored` once they have reviewed it.",
+};
+
+/**
+ * Labels that can clear or reinstate a hygiene failure. The quality gate must
+ * wake on these so READY / DRAFT tracks sponsorship and exception approvals
+ * without waiting for an unrelated synchronize.
+ */
+const HYGIENE_GATE_LABELS = [
+  "intake: hygiene-blocked",
+  "maintainer-sponsored",
+  "test-exception-approved",
+  "suppression-approved",
+  "generated-change-approved",
+  "dependency-change-approved",
+];
+
+/**
+ * Combine patch-hygiene and sponsored-surface failures into one list so the
+ * hygiene workflow and the PR quality gate cannot disagree about Ready.
+ */
+function collectDeterministicHygieneFailures({
+  files = [],
+  labels = [],
+  authorHasPushPermission = false,
+}) {
+  // Renames must keep the source path: moving a restricted file to a
+  // non-restricted destination must not drop the sponsorship requirement.
+  const changedFiles = [
+    ...new Set(
+      files.flatMap((file) => [
+        file.filename,
+        ...(file.previous_filename ? [file.previous_filename] : []),
+      ]),
+    ),
+  ];
+  return [
+    ...assessHygiene({ files, labels }),
+    ...assessSponsoredSurface({
+      authorHasPushPermission,
+      changedFiles,
+      labels,
+    }),
+  ];
+}
+
 module.exports = {
   addedLines,
   assessHygiene,
+  collectDeterministicHygieneFailures,
+  HYGIENE_FAILURE_HINTS,
+  HYGIENE_GATE_LABELS,
   hasEmptyCatch,
   hasDeletions,
   isBehaviorPath,

.github/scripts/pr-hygiene.test.cjs

@@ -5,6 +5,9 @@ const assert = require("node:assert/strict");
 const {
   addedLines,
   assessHygiene,
+  collectDeterministicHygieneFailures,
+  HYGIENE_FAILURE_HINTS,
+  HYGIENE_GATE_LABELS,
   hasEmptyCatch,
   resultLines,
   resultLinesByHunk,
@@ -203,3 +206,78 @@ describe("assessHygiene", () => {
     assert.deepEqual(failures, []);
   });
 });
+
+describe("collectDeterministicHygieneFailures", () => {
+  it("combines patch hygiene and sponsored-surface failures", () => {
+    const failures = collectDeterministicHygieneFailures({
+      files: [
+        { filename: "src/codex/auth-api.ts", patch: "+change" },
+      ],
+      authorHasPushPermission: false,
+    });
+    assert.deepEqual(
+      failures.map((failure) => failure.code).sort(),
+      ["missing_regression_test", "unsponsored_surface"],
+    );
+  });
+
+  it("skips sponsorship for maintainers with push permission", () => {
+    const failures = collectDeterministicHygieneFailures({
+      files: [
+        { filename: "src/codex/auth-api.ts", patch: "+change" },
+        { filename: "tests/codex-auth-api.test.ts", patch: "+test" },
+      ],
+      authorHasPushPermission: true,
+    });
+    assert.deepEqual(failures, []);
+  });
+
+  it("requires sponsorship when renaming away from a restricted path", () => {
+    const failures = collectDeterministicHygieneFailures({
+      files: [
+        {
+          filename: "docs/moved-release.yml",
+          previous_filename: ".github/workflows/release.yml",
+          status: "renamed",
+          patch: "+moved",
+        },
+      ],
+      authorHasPushPermission: false,
+    });
+    const unsponsored = failures.find((failure) => failure.code === "unsponsored_surface");
+    assert.ok(unsponsored, "expected unsponsored_surface for a restricted rename source");
+    assert.deepEqual(unsponsored.paths, [".github/workflows/release.yml"]);
+  });
+
+  it("exposes hints and gate labels for the Ready coupling", () => {
+    assert.equal(typeof HYGIENE_FAILURE_HINTS.unsponsored_surface, "string");
+    assert.ok(HYGIENE_GATE_LABELS.includes("maintainer-sponsored"));
+    assert.ok(HYGIENE_GATE_LABELS.includes("intake: hygiene-blocked"));
+  });
+});
+
+describe("pr-hygiene workflow trust boundary", () => {
+  const fs = require("node:fs");
+  const path = require("node:path");
+  const workflow = fs.readFileSync(
+    path.join(__dirname, "../workflows/pr-hygiene.yml"),
+    "utf8",
+  );
+
+  it("checks out the PR base SHA, not the repository default branch", () => {
+    assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/);
+    assert.doesNotMatch(
+      workflow,
+      /Checkout trusted hygiene script[\s\S]*?ref:\s*\$\{\{\s*github\.event\.repository\.default_branch/,
+    );
+  });
+
+  it("uses repository permission level for the sponsorship exemption", () => {
+    assert.match(workflow, /getCollaboratorPermissionLevel/);
+    assert.match(workflow, /authorHasPushPermission\(authorPermission\)/);
+    assert.doesNotMatch(
+      workflow,
+      /authorHasPushPermission:\s*\["OWNER",\s*"MEMBER",\s*"COLLABORATOR"\]\.includes\(\s*pr\.author_association/,
+    );
+  });
+});

.github/workflows/enforce-pr-target.yml

@@ -38,7 +38,13 @@ jobs:
        github.event.sender.id == 136622811) ||
       (github.event_name == 'pull_request_target' &&
        ((github.event.action != 'labeled' && github.event.action != 'unlabeled') ||
-        github.event.label.name == 'gui-screenshot-waived'))
+        github.event.label.name == 'gui-screenshot-waived' ||
+        github.event.label.name == 'intake: hygiene-blocked' ||
+        github.event.label.name == 'maintainer-sponsored' ||
+        github.event.label.name == 'test-exception-approved' ||
+        github.event.label.name == 'suppression-approved' ||
+        github.event.label.name == 'generated-change-approved' ||
+        github.event.label.name == 'dependency-change-approved'))
     runs-on: ubuntu-latest
     permissions:
       contents: read
@@ -139,6 +145,12 @@ jobs:
             } = require(
               path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"),
             );
+            const {
+              collectDeterministicHygieneFailures,
+              HYGIENE_FAILURE_HINTS,
+            } = require(
+              path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"),
+            );
             const {
               parseGateState,
               gateStateMarker,
@@ -536,6 +548,26 @@ jobs:
               guiOverrideComments: comments
             });
 
+            // Hygiene is a separate workflow that owns the blocked label and
+            // the Hygiene comment section, but Ready / review-ready must not
+            // clear while those checks fail. Re-assess here from the same
+            // trusted scripts so the gate cannot race ahead of hygiene.
+            const changedFiles = await github.paginate(
+              github.rest.pulls.listFiles,
+              { owner, repo, pull_number, per_page: 100 }
+            );
+            const labelNames = (pr.labels ?? []).map(label => label.name);
+            failures = [
+              ...failures,
+              ...collectDeterministicHygieneFailures({
+                files: changedFiles,
+                labels: labelNames,
+                authorHasPushPermission:
+                  !permissionLookupFailed &&
+                  authorHasPushPermission(authorPermission),
+              }),
+            ];
+
             // A maintainer issue comment saying the change does not touch
             // the GUI waives the screenshot gate. The flag is what tells the
             // author the screenshot is not required, even though the failure
@@ -943,6 +975,14 @@ jobs:
                   "Add a screenshot of the UI change to the PR description."
                 );
               }
+              for (const failure of failures) {
+                const hint = HYGIENE_FAILURE_HINTS[failure.code];
+                if (!hint) continue;
+                const paths = failure.paths?.length
+                  ? ` Paths: ${failure.paths.map(p => inlineCode(p)).join(", ")}.`
+                  : "";
+                actions.push(`Fix **${failure.code}** — ${hint}${paths}`);
+              }
               if (checklistRequired && !checklistComplete) {
                 actions.push(
                   `Tick all four boxes in the PR description once you're done (currently ${readiness.checked}/${readiness.total}).`
@@ -1028,6 +1068,9 @@ jobs:
                     if (failure.code === "missing_ui_screenshot") {
                       return "UI screenshot required.";
                     }
+                    if (HYGIENE_FAILURE_HINTS[failure.code]) {
+                      return `hygiene: ${failure.code}.`;
+                    }
                     return failure.code;
                   })
                   .join(" ");

.github/workflows/pr-hygiene.yml

@@ -4,8 +4,8 @@ on:
   pull_request_target:
     types: [opened, reopened, synchronize, labeled, unlabeled]
 
-# Trusted default-branch script only. Patches are read through the GitHub API;
-# PR-head code is never checked out or executed.
+# Trusted scripts from the PR base revision only. Patches are read through the
+# GitHub API; PR-head code is never checked out or executed.
 # Least privilege: no default permissions; the hygiene job grants only what it needs.
 permissions: {}
 
@@ -30,7 +30,9 @@ jobs:
       - name: Checkout trusted hygiene script
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
         with:
-          ref: ${{ github.event.repository.default_branch }}
+          # pull_request_target must pin scripts to the PR base SHA, not the
+          # repository default branch: PRs target `dev` while `main` may lag.
+          ref: ${{ github.event.pull_request.base.sha }}
           persist-credentials: false
           sparse-checkout: .github/scripts
 
@@ -39,11 +41,14 @@ jobs:
         with:
           script: |
             const path = require("node:path");
-            const { assessHygiene } = require(
+            const {
+              collectDeterministicHygieneFailures,
+              HYGIENE_FAILURE_HINTS,
+            } = require(
               path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"),
             );
-            const { assessSponsoredSurface } = require(
-              path.join(process.cwd(), ".github", "scripts", "pr-sponsored-surface.cjs"),
+            const { authorHasPushPermission } = require(
+              path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"),
             );
             const {
               GATE_MARKER,
@@ -107,16 +112,31 @@ jobs:
             // Sponsorship is head-independent: it is about which surfaces the
             // change touches, not about the state of a particular revision, so
             // it is NOT cleared by the synchronize sweep above.
-            const failures = [
-              ...assessHygiene({ files, labels: [...labels] }),
-              ...assessSponsoredSurface({
-                authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes(
-                  pr.author_association,
-                ),
-                changedFiles: files.map((file) => file.filename),
-                labels: [...labels],
-              }),
-            ];
+            // author_association is not enough: a read/triage collaborator can
+            // be COLLABORATOR without write access. Match the PR quality gate.
+            let authorPermission = null;
+            let permissionLookupFailed = false;
+            try {
+              const { data: permissionData } =
+                await github.rest.repos.getCollaboratorPermissionLevel({
+                  owner,
+                  repo,
+                  username: pr.user.login,
+                });
+              authorPermission = permissionData.permission;
+            } catch (error) {
+              permissionLookupFailed = true;
+              core.warning(
+                `Could not look up collaborator permission: ${error.message}`,
+              );
+            }
+            const failures = collectDeterministicHygieneFailures({
+              files,
+              labels: [...labels],
+              authorHasPushPermission:
+                !permissionLookupFailed &&
+                authorHasPushPermission(authorPermission),
+            });
 
             async function setBlocked(blocked) {
               if (blocked && !labels.has(blockedLabel)) {
@@ -169,20 +189,11 @@ jobs:
               return;
             }
 
-            const explanations = {
-              missing_regression_test: "Behavior changed under `src/` or `gui/src/` without a test change. Add focused coverage or obtain `test-exception-approved`.",
-              generated_output: "Generated build output is committed. Remove it or obtain `generated-change-approved`.",
-              orphan_lockfile: "`bun.lock` changed without `package.json`. Revert accidental churn or obtain `dependency-change-approved`.",
-              new_suppression: "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.",
-              focused_or_skipped_test: "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.",
-              empty_catch: "An empty catch block was added. Handle, report, or deliberately propagate the error.",
-              unsponsored_surface: "This changes an authentication, workflow, release-automation, or dependency surface. `MAINTAINERS.md` requires security review for these; ask a maintainer to apply `maintainer-sponsored` once they have reviewed it.",
-            };
             const lines = failures.map((failure) => {
               const paths = failure.paths?.length
                 ? ` Paths: ${failure.paths.map((p) => `\`${p}\``).join(", ")}.`
                 : "";
-              return `- **${failure.code}** — ${explanations[failure.code]}${paths}`;
+              return `- **${failure.code}** — ${HYGIENE_FAILURE_HINTS[failure.code] ?? failure.code}${paths}`;
             });
 
             await setBlocked(true);