From cc7ecb9b5a517f1266aef1114307b74de507a5cd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 19 Sep 2026 20:20:30 +0000 Subject: [PATCH] fix(ci): classify G13 per queued PR on merge_group Run live isolation on the merge-queue event so ci-complete actually gates mixed change sets, without treating the group union as one PR. --- .github/workflows/ci.yaml | 3 +- QUALITY_GATES.md | 44 ++++++------ README.md | 7 +- package.json | 2 +- scripts/g13-live-isolation.mjs | 29 ++++++++ scripts/governance-check.mjs | 103 ++++++++++++++++++++-------- scripts/test-g13-live-isolation.mjs | 73 ++++++++++++++++++++ terraform/README.md | 2 +- 8 files changed, 207 insertions(+), 56 deletions(-) create mode 100644 scripts/g13-live-isolation.mjs create mode 100644 scripts/test-g13-live-isolation.mjs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f44095b9..5e2c75db 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -86,7 +86,8 @@ jobs: # Repo-owned gates: godfile ratchet, changed-file maintainability, # Terraform fmt/validate, import-plan guard, HCP run guard, CloudFront # verify, GitHub workflow shell, isolation classifier tests, and live G13 - # (pull_request and local only). Format, lint, build, and unit tests run + # (pull_request, merge_group per queued PR, and local). Format, lint, build, + # and unit tests run # in the parallel jobs above, not here. # # GOVERNANCE_BASE points the changed-file gate at the right diff: diff --git a/QUALITY_GATES.md b/QUALITY_GATES.md index 7dcc3bbc..a5596f8b 100644 --- a/QUALITY_GATES.md +++ b/QUALITY_GATES.md @@ -14,24 +14,24 @@ isolation). A task is not done until this is green. ## Gate matrix -| Gate | Command / rule source | Enforced by | Scope | -| ----------------------------------- | ----------------------------------------------------------------------------------------------------------- | ---------------------- | ------------------------------------------------------------------ | -| Formatting | `npm run format:check` (Prettier) | `verify` + lint-staged | Whole repo | -| Lint, zero warnings | `npm run lint` → `eslint . --max-warnings=0` | `verify` + CI | Governed TS/TSX (`eslint.config.js`) | -| Type-check + production build | `npm run build` → `tsc -b && vite build` | `verify` + CI | Whole app | -| Unit tests | `npm test` → `vitest run` | `verify` + CI | `src/test/**`, `config/**/*.test.ts` | -| Conditional rendering (no `: null`) | `no-restricted-syntax` in `eslint.config.js` | lint | Governed TSX | -| Boolean-only JSX `&&` | `seahaven/no-non-boolean-jsx-and` (type-aware) in `eslint-rules/` | lint | Governed TSX | -| Shared `Text` typography | `no-restricted-syntax` (raw `p`/`h1`–`h6`) + `seahaven/no-vp-error-outside-text` | lint | Governed TSX | -| Hooks correctness | `eslint-plugin-react-hooks` recommended (incl. `exhaustive-deps`) under zero-warnings | lint | Governed TS/TSX | -| Godfile ratchet (file length) | `scripts/governance-check.mjs` + `scripts/governance-baseline.json` | `governance` | `src/**`, `config/**` (non-test) | -| Changed-file maintainability | `scripts/governance-check.mjs` → ESLint (`complexity`, `max-lines-per-function`, `max-params`, `max-depth`) | `governance` | Changed TS/TSX vs base ref | -| Terraform import-plan contract | `npm run test:terraform-import-plan` → `scripts/test-terraform-import-plan-check.py` | `governance` + CI | Synthetic plan JSON + canonical maps | -| Terraform formatting/validation | `npm run test:terraform` → `scripts/terraform-validate.mjs` | `governance` + CI | `terraform/live/dev`, `terraform/live/staging` | -| HCP run guard | `npm run test:hcp-run-guard` → `scripts/test-hcp-run-guard.py` | `governance` + CI | Workspace invariants + apply reconcile | -| CloudFront release verify | `npm run test:cloudfront-release-verify` → `scripts/test-verify-cloudfront-release.sh` | `governance` + CI | Stubbed aws/curl | -| GitHub workflow shell | `npm run test:github-workflows` → `scripts/check-github-workflows.sh` | `governance` + CI | `bash -n` + actionlint | -| G13 App/Terraform isolation | `python3 scripts/check_app_terraform_isolation.py` vs merge-base of `GOVERNANCE_BASE` | `governance` + CI | Deployable app files vs `terraform/` (live classifier: PR + local) | +| Gate | Command / rule source | Enforced by | Scope | +| ----------------------------------- | ----------------------------------------------------------------------------------------------------------------------------- | ---------------------- | ------------------------------------------------------------------- | +| Formatting | `npm run format:check` (Prettier) | `verify` + lint-staged | Whole repo | +| Lint, zero warnings | `npm run lint` → `eslint . --max-warnings=0` | `verify` + CI | Governed TS/TSX (`eslint.config.js`) | +| Type-check + production build | `npm run build` → `tsc -b && vite build` | `verify` + CI | Whole app | +| Unit tests | `npm test` → `vitest run` | `verify` + CI | `src/test/**`, `config/**/*.test.ts` | +| Conditional rendering (no `: null`) | `no-restricted-syntax` in `eslint.config.js` | lint | Governed TSX | +| Boolean-only JSX `&&` | `seahaven/no-non-boolean-jsx-and` (type-aware) in `eslint-rules/` | lint | Governed TSX | +| Shared `Text` typography | `no-restricted-syntax` (raw `p`/`h1`–`h6`) + `seahaven/no-vp-error-outside-text` | lint | Governed TSX | +| Hooks correctness | `eslint-plugin-react-hooks` recommended (incl. `exhaustive-deps`) under zero-warnings | lint | Governed TS/TSX | +| Godfile ratchet (file length) | `scripts/governance-check.mjs` + `scripts/governance-baseline.json` | `governance` | `src/**`, `config/**` (non-test) | +| Changed-file maintainability | `scripts/governance-check.mjs` → ESLint (`complexity`, `max-lines-per-function`, `max-params`, `max-depth`) | `governance` | Changed TS/TSX vs base ref | +| Terraform import-plan contract | `npm run test:terraform-import-plan` → `scripts/test-terraform-import-plan-check.py` | `governance` + CI | Synthetic plan JSON + canonical maps | +| Terraform formatting/validation | `npm run test:terraform` → `scripts/terraform-validate.mjs` | `governance` + CI | `terraform/live/dev`, `terraform/live/staging` | +| HCP run guard | `npm run test:hcp-run-guard` → `scripts/test-hcp-run-guard.py` | `governance` + CI | Workspace invariants + apply reconcile | +| CloudFront release verify | `npm run test:cloudfront-release-verify` → `scripts/test-verify-cloudfront-release.sh` | `governance` + CI | Stubbed aws/curl | +| GitHub workflow shell | `npm run test:github-workflows` → `scripts/check-github-workflows.sh` | `governance` + CI | `bash -n` + actionlint | +| G13 App/Terraform isolation | `python3 scripts/check_app_terraform_isolation.py` vs merge-base of `GOVERNANCE_BASE` (merge_group: each first-parent commit) | `governance` + CI | Deployable app files vs `terraform/` (live: PR, merge_group, local) | ## No-false-pass guarantees @@ -66,9 +66,11 @@ isolation). A task is not done until this is green. (Playwright visual), and `governance` (`npm run governance`, with Terraform 1.16.0) run in parallel. `ci-complete` fails unless all of those jobs succeeded and is the required merge-queue check. Live G13 runs on - `pull_request` and locally; it skips `merge_group` and `push`. Terraform - fmt/validate and the related unit tests run inside `governance` on every - event. + `pull_request` (merge-base range), `merge_group` (each queued PR as a + first-parent commit), and locally. It skips `push`. A Terraform-only PR + stacked with an app-only PR still passes; a mixed change set still fails. + Terraform fmt/validate and the related unit tests run inside `governance` on + every event. ## Toolchain pin diff --git a/README.md b/README.md index 76040f38..974e0bf1 100644 --- a/README.md +++ b/README.md @@ -136,9 +136,10 @@ commitlint enforces conventional commit messages. Run `npx tsc --noEmit` (or approvals. PRs merge through the merge queue, so a branch does not need to be updated with `main` before it merges. Merged branches are deleted automatically. -- PRs cannot mix `terraform/` with deployable application files (G13). Workflow, - docs, and gate-script changes may travel with either side. `deploy-web.yaml` - still ignores `terraform/**` so a Terraform-only merge does not sync the bucket. +- A change set cannot mix `terraform/` with deployable application files (G13), + including each queued PR on the merge-group check. Workflow, docs, and + gate-script changes may travel with either side. `deploy-web.yaml` still + ignores `terraform/**` so a Terraform-only merge does not sync the bucket. - Promotion flow: merge to `main` deploys `dev.seahaven.com`. A person cuts `vX.Y.Z-staging` for `staging.seahaven.com`. Core `vX.Y.Z` waits until a prod distribution exists. diff --git a/package.json b/package.json index 3ce507f5..b1fc3700 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "test:hcp-run-guard": "python3 scripts/test-hcp-run-guard.py", "test:cloudfront-release-verify": "bash scripts/test-verify-cloudfront-release.sh", "test:github-workflows": "bash scripts/check-github-workflows.sh", - "test:app-terraform-isolation": "python3 scripts/test_check_app_terraform_isolation.py", + "test:app-terraform-isolation": "python3 scripts/test_check_app_terraform_isolation.py && node --test scripts/test-g13-live-isolation.mjs", "lint": "eslint . --max-warnings=0", "lint:fix": "eslint . --fix --max-warnings=0", "format": "prettier --write .", diff --git a/scripts/g13-live-isolation.mjs b/scripts/g13-live-isolation.mjs new file mode 100644 index 00000000..3e2d8f65 --- /dev/null +++ b/scripts/g13-live-isolation.mjs @@ -0,0 +1,29 @@ +/** + * Live G13 scheduling and how a GitHub event is split into change sets. + * + * Isolation is a per-change rule. pull_request and local runs classify the + * merge-base...HEAD range (one PR). merge_group classifies each first-parent + * commit vs its parent (one queued PR per squash or merge-commit). The group + * union is not a change set: a Terraform-only PR stacked with an app-only PR + * must still pass. push is not classified; landing already happened. + */ + +export function shouldRunLiveIsolation(eventName) { + return !eventName || eventName === "pull_request" || eventName === "merge_group"; +} + +export function usesPerCommitIsolation(eventName) { + return eventName === "merge_group"; +} + +/** + * File lists to run through check_app_terraform_isolation.py. + * `commitDiffs` is first-parent order (oldest first); ignored except on + * merge_group. + */ +export function liveIsolationFileSets(eventName, rangeFiles, commitDiffs) { + if (usesPerCommitIsolation(eventName)) { + return commitDiffs.map((commit) => commit.files); + } + return [rangeFiles]; +} diff --git a/scripts/governance-check.mjs b/scripts/governance-check.mjs index 84b4c5cf..025489cb 100644 --- a/scripts/governance-check.mjs +++ b/scripts/governance-check.mjs @@ -3,6 +3,8 @@ import { existsSync, readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { shouldRunLiveIsolation, usesPerCommitIsolation } from "./g13-live-isolation.mjs"; + const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); const ROOT = path.resolve(SCRIPT_DIR, ".."); const BASELINE_PATH = path.join(SCRIPT_DIR, "governance-baseline.json"); @@ -218,23 +220,77 @@ function runRepositoryGate(label, script) { return { label, status: result.status, error: result.error }; } -function shouldRunLiveIsolation() { - const eventName = process.env.GITHUB_EVENT_NAME; - return !eventName || eventName === "pull_request"; +function classifyIsolationPaths(files) { + return spawnSync("python3", ["scripts/check_app_terraform_isolation.py"], { + cwd: ROOT, + encoding: "utf8", + input: files.length > 0 ? `${files.join("\n")}\n` : "", + }); } -function runIsolationGate(baseRef) { +function firstParentCommitDiffs(baseRef) { + const shas = gitLines(["rev-list", "--reverse", "--first-parent", `${baseRef}..HEAD`]); + return shas.map((sha) => ({ + sha, + files: gitLines(["diff", "--name-only", "--diff-filter=ACMR", `${sha}^`, sha]), + })); +} + +function runIsolationGate(baseRef, eventName) { + if (usesPerCommitIsolation(eventName)) { + const commits = firstParentCommitDiffs(baseRef); + return { + mode: "per-commit", + results: commits.map((commit) => ({ + ...classifyIsolationPaths(commit.files), + sha: commit.sha, + })), + }; + } // Diff from the merge base, not the moving base tip. A two-dot diff against // a branch that has advanced reports everything the base gained after the // branch point as if this change reverted it. const mergeBase = gitText(["merge-base", baseRef, "HEAD"]); const files = gitLines(["diff", "--name-only", "--diff-filter=ACMR", mergeBase, "HEAD"]); - const result = spawnSync("python3", ["scripts/check_app_terraform_isolation.py"], { - cwd: ROOT, - encoding: "utf8", - input: `${files.join("\n")}\n`, - }); - return { ...result, mergeBase }; + return { + mode: "range", + mergeBase, + results: [{ ...classifyIsolationPaths(files), sha: null }], + }; +} + +function recordIsolationFailures(isolation, failures) { + const startError = isolation.results.find((result) => result.error); + if (startError) { + failures.push(`G13: could not start: ${startError.error.message}`); + } + if (isolation.results.some((result) => !result.error && result.status !== 0)) { + failures.push("G13: do not mix deployable application files with terraform/"); + } +} + +function logIsolationGate(baseRef, isolation) { + if (isolation.mode === "per-commit") { + console.log( + `G13: application and Terraform isolation (merge_group, ${plural(isolation.results.length, "queued PR")} vs ${baseRef.slice(0, 7)})`, + ); + for (const result of isolation.results) { + const output = `${result.stdout ?? ""}${result.stderr ?? ""}`.trim(); + const prefix = result.sha ? result.sha.slice(0, 7) : "commit"; + if (output) { + console.log(` ${prefix}: ${output.replaceAll("\n", "\n ")}`); + } + } + return; + } + const mergeBase = isolation.mergeBase ?? "unresolvable"; + console.log( + `G13: application and Terraform isolation (${baseRef}...HEAD, merge base ${mergeBase.slice(0, 7)})`, + ); + const result = isolation.results[0]; + if (!result) return; + const output = `${result.stdout ?? ""}${result.stderr ?? ""}`.trim(); + if (output) console.log(` ${output.replaceAll("\n", "\n ")}`); } function main() { @@ -336,9 +392,10 @@ function main() { } console.log("─".repeat(64)); - if (!shouldRunLiveIsolation()) { + const eventName = process.env.GITHUB_EVENT_NAME; + if (!shouldRunLiveIsolation(eventName)) { console.log( - `G13: skipped — live isolation is a pull-request property (event: ${process.env.GITHUB_EVENT_NAME})`, + `G13: skipped — live isolation runs on pull_request, merge_group, and local (event: ${eventName})`, ); } else if (!baseRef) { console.log("G13: application and Terraform isolation (no base...HEAD)"); @@ -349,28 +406,16 @@ function main() { } else { let isolation; try { - isolation = runIsolationGate(baseRef); + isolation = runIsolationGate(baseRef, eventName); } catch (error) { console.log(`G13: application and Terraform isolation (${baseRef}...HEAD)`); - console.log(" FAIL (could not resolve merge base)"); + console.log(" FAIL (could not resolve isolation diffs)"); const message = error instanceof Error ? error.message : String(error); - failures.push(`G13: could not resolve merge base against HEAD: ${message}`); + failures.push(`G13: could not resolve isolation diffs against HEAD: ${message}`); } if (isolation) { - const mergeBase = isolation.mergeBase ?? "unresolvable"; - console.log( - `G13: application and Terraform isolation (${baseRef}...HEAD, merge base ${mergeBase.slice(0, 7)})`, - ); - if (isolation.error) { - console.log(" FAIL (could not start)"); - failures.push(`G13: could not start: ${isolation.error.message}`); - } else { - const output = `${isolation.stdout ?? ""}${isolation.stderr ?? ""}`.trim(); - if (output) console.log(` ${output.replaceAll("\n", "\n ")}`); - if (isolation.status !== 0) { - failures.push("G13: do not mix deployable application files with terraform/"); - } - } + logIsolationGate(baseRef, isolation); + recordIsolationFailures(isolation, failures); } } diff --git a/scripts/test-g13-live-isolation.mjs b/scripts/test-g13-live-isolation.mjs new file mode 100644 index 00000000..5e402e4c --- /dev/null +++ b/scripts/test-g13-live-isolation.mjs @@ -0,0 +1,73 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import path from "node:path"; +import { describe, it } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { + liveIsolationFileSets, + shouldRunLiveIsolation, + usesPerCommitIsolation, +} from "./g13-live-isolation.mjs"; + +const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); +const CLASSIFIER = path.join(SCRIPT_DIR, "check_app_terraform_isolation.py"); +const TERRAFORM_ONLY = ["terraform/live/dev/main.tf"]; +const APP_ONLY = ["src/app/routes.tsx"]; + +function classify(files) { + const result = spawnSync("python3", [CLASSIFIER, ...files], { encoding: "utf8" }); + return result.status; +} + +function anySetFails(fileSets) { + return fileSets.some((files) => classify(files) !== 0); +} + +describe("shouldRunLiveIsolation", () => { + it("runs locally and on pull_request", () => { + assert.equal(shouldRunLiveIsolation(undefined), true); + assert.equal(shouldRunLiveIsolation(""), true); + assert.equal(shouldRunLiveIsolation("pull_request"), true); + }); + + it("runs on merge_group so the required check classifies the candidate", () => { + assert.equal(shouldRunLiveIsolation("merge_group"), true); + assert.equal(usesPerCommitIsolation("merge_group"), true); + }); + + it("skips push and other CI events", () => { + assert.equal(shouldRunLiveIsolation("push"), false); + assert.equal(shouldRunLiveIsolation("workflow_dispatch"), false); + assert.equal(usesPerCommitIsolation("pull_request"), false); + }); +}); + +describe("liveIsolationFileSets", () => { + it("classifies the PR range as one change set", () => { + const range = [...TERRAFORM_ONLY, ...APP_ONLY]; + const sets = liveIsolationFileSets("pull_request", range, [ + { files: TERRAFORM_ONLY }, + { files: APP_ONLY }, + ]); + assert.deepEqual(sets, [range]); + assert.equal(anySetFails(sets), true); + }); + + it("classifies each queued PR, not the merge-group union", () => { + const union = [...TERRAFORM_ONLY, ...APP_ONLY]; + const sets = liveIsolationFileSets("merge_group", union, [ + { files: TERRAFORM_ONLY }, + { files: APP_ONLY }, + ]); + assert.deepEqual(sets, [TERRAFORM_ONLY, APP_ONLY]); + assert.equal(anySetFails(sets), false); + assert.equal(classify(union), 1); + }); + + it("fails a single queued PR that mixes terraform and app files", () => { + const mixed = [...TERRAFORM_ONLY, ...APP_ONLY]; + const sets = liveIsolationFileSets("merge_group", mixed, [{ files: mixed }]); + assert.equal(anySetFails(sets), true); + }); +}); diff --git a/terraform/README.md b/terraform/README.md index ddeb0bc5..40b78a09 100644 --- a/terraform/README.md +++ b/terraform/README.md @@ -57,7 +57,7 @@ bash scripts/test-verify-cloudfront-release.sh PRs cannot mix `terraform/` with deployable application files. Workflow, docs, and gate-script changes may travel with either side. G13 is `python3 scripts/check_app_terraform_isolation.py` against the merge base of -the PR. +the PR, and against each queued PR (first-parent commit) on `merge_group`. `npm run test:terraform` and `npm run verify` wrap the same gates. They never create an HCP run or touch AWS.