diff --git a/.github/workflows/cicd_comp_test-phase.yml b/.github/workflows/cicd_comp_test-phase.yml index 9fc4df4e85d6..b180ee5d913e 100644 --- a/.github/workflows/cicd_comp_test-phase.yml +++ b/.github/workflows/cicd_comp_test-phase.yml @@ -261,9 +261,50 @@ jobs: # Frontend Unit Tests runs `nx affected` (lint-test, -Pvalidate) which diffs # against origin/main (core-web/pom.xml git.origin.branch). The shallow # checkout above has no origin/main ref, so fetch just that ref, shallow. - - name: Fetch origin/main for Nx affected + # + # The strict-gate (#37536) additionally needs a base it can diff against + # HONESTLY. Both sides here are depth-1, so `git merge-base` cannot traverse + # and the harness falls back to comparing against the TIP of origin/main -- + # which attributes every commit main moved ahead to the pull request. That is + # survivable for `nx affected` (over-including projects for lint is harmless + # on a lint-clean main) but not for a blocking strict check, because main is + # deliberately NOT strict-clean while #37198 is in flight: authors would be + # rejected for violations they never wrote. + # + # So hand the gate the divergence point itself. That point is NOT + # github.event.pull_request.base.sha: GitHub pins that field when the pull + # request is opened and never moves it as the base branch advances, so on a + # long-lived branch it is hundreds of commits stale -- and diffing a stale base + # against this checkout blames the author for every one of them. + # + # It is the checkout's own FIRST PARENT. Both events check out a merge commit + # (`refs/pull/N/merge` on pull_request, the queue branch head on merge_group) + # whose first parent is the current base tip and whose second is the branch, so + # `parent1..HEAD` is precisely the pull request's contribution. Deepening by one + # is all it takes to see it, and it needs no knowledge of the base branch's name, + # so a pull request targeting something other than `main` compares against its + # own base. Requiring two parents keeps this honest: if the checkout is ever + # changed to the branch head, HEAD^1 is the previous COMMIT, not the base, and + # the gate would silently narrow to the last commit instead. + - name: Fetch base refs for Nx affected and the strict gate if: contains(matrix.maven_args, '-Pvalidate') - run: git fetch --depth=1 origin main:refs/remotes/origin/main + run: | + git fetch --depth=1 origin main:refs/remotes/origin/main + # Not fatal: if the parent cannot be resolved the gate still runs against + # origin/main and warns, which is the behaviour before this step. + git fetch --no-tags --depth=2 origin "$GITHUB_SHA" 2>/dev/null || true + # `rev-list --parents` prints " ...", and a shallow boundary + # prints no parents at all -- so the second parent being present is also the + # proof that the deepen above landed. + PARENTS=$(git rev-list --parents -n 1 HEAD) + FIRST_PARENT=$(echo "$PARENTS" | awk '{print $2}') + SECOND_PARENT=$(echo "$PARENTS" | awk '{print $3}') + if [ -n "$SECOND_PARENT" ]; then + echo "STRICT_GATE_BASE=$FIRST_PARENT" >> "$GITHUB_ENV" + echo "strict-gate base: $FIRST_PARENT (first parent of $GITHUB_SHA)" + else + echo "::warning::could not resolve the first parent of $GITHUB_SHA; strict-gate will fall back to origin/main" + fi # The libvips image engine (IMAGE_API_USE_LIBVIPS) is exercised by VipsParityTest. # Install native libvips on the JVM unit-test runner so those tests run instead of diff --git a/core-web/pom.xml b/core-web/pom.xml index b64a9dae9050..f173c4325660 100644 --- a/core-web/pom.xml +++ b/core-web/pom.xml @@ -23,6 +23,56 @@ false false true + + true + + ${git.origin.branch} origin/main --base=${git.origin.branch} --head=HEAD --branch=${git.origin.branch} @@ -431,6 +481,154 @@ false + + + + org.codehaus.mojo + exec-maven-plugin + + + + + strict-gate + + exec + + generate-resources + + ${skip.strict.gate} + + + 0 + + + false + + 180000 + + exec + node + tools/scripts/strict-gate/run.mjs + --base=${strict.gate.base} + --flags=strict + --granularity=line + --scope=core-web + --format=github + + + + + + + + + + + + + strict-gate-pull-request + + + env.GITHUB_EVENT_NAME + pull_request + + + + false + + + + + + strict-gate-explicit-base + + + env.STRICT_GATE_BASE + + + + ${env.STRICT_GATE_BASE} + + + + + + strict-gate-merge-queue + + + env.GITHUB_EVENT_NAME + merge_group + + + + false + diff --git a/core-web/tools/scripts/strict-gate/README.md b/core-web/tools/scripts/strict-gate/README.md index 081fed1fe1ca..7cf4f14f3c6c 100644 --- a/core-web/tools/scripts/strict-gate/README.md +++ b/core-web/tools/scripts/strict-gate/README.md @@ -6,7 +6,9 @@ > Procedure, inventory and preconditions: > [`specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md`](../../../../specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md) -**This is spike output, not production tooling.** It exists to answer one question: +**This began as spike output and is now a live, blocking CI gate** — still temporary by design +(see the banner above), but not optional and not safe to delete casually. It exists to answer one +question: > Can a diff-scoped strict typecheck block new non-strict TypeScript from landing on `main`, > without requiring the dependency libraries to be strict first? @@ -67,18 +69,66 @@ node tools/scripts/strict-gate/run.mjs \ - `--granularity line` — whole-file makes an author inherit 83 % of what it reports from lines they did not write. New files are unaffected: every line of an added file is a changed line. -**Recommendation: ship non-blocking first.** Precision is better than the spec asked for; runtime -is the open issue (8.4–9.4 s average, 12 s at the tail, against a 10 s budget). The cost is entirely -dependency-closure recompilation and has untried optimisations. Templates are a **no-go for -blocking** for now — 2.2× the compiler time on the largest application. +**The spike recommended shipping non-blocking first** — precision was better than the spec asked +for, but runtime was the open question (8.4–9.4 s average, 12 s at the tail, against a 10 s budget), +and the cost is entirely dependency-closure recompilation with untried optimisations. *(Superseded +by #37536: the gate ships blocking. The runtime question is unchanged and now matters more, not +less — see Status.)* Templates remain a **no-go** either way: 2.2× the compiler time on the largest +application. Full measurements, adjudication of every finding, and the go/no-go: `specs/37401-diff-scoped-strict-typecheck-gate/findings.md` and issue #37401. +## Where this runs + +**Wired and live since #37536**: a `strict-gate` execution in `core-web/pom.xml`, inside the +`validate` profile beside `lint-test` and `format-test`. + +**It blocks.** A new strict-mode violation on a line your pull request wrote fails the check, and +the change does not merge until it is fixed. You get the violation annotated inline on the diff, +plus a job summary — so the fix is usually a type annotation on the line the annotation points at. +Read the scope note in the output before you touch anything else: only the lines you changed are +checked, and diagnostics from dependencies and untouched code were discarded on purpose. + +It runs in two places, doing different jobs: + +| Event | Role | +|---|---| +| `pull_request` | Where you read it. Annotations render inline on the diff. | +| `merge_group` | Where it is enforced. `main` declares no required status checks, so a red check on a pull request does not by itself stop a merge; a job failing in the merge queue ejects the pull request, and that does. | + +Trunk and nightly runs skip it — `HEAD` equals `origin/main` there, so the diff is empty. + +Nothing runs it on your machine. To get the answer before pushing, run it yourself with the +command under "Running it" above. + +There is **no escape hatch**. If you have a change that legitimately must add a violation, it +cannot merge until the violation is fixed or the gate is turned off repository-wide. That gap is +known and accepted; if you hit it, say so on #37536 rather than working around it quietly. + +Two things worth knowing before you change it: + +- **Two activation profiles, not one.** `strict-gate-pull-request` and `strict-gate-merge-queue` + each flip `skip.strict.gate` on their own `GITHUB_EVENT_NAME`. Maven property activation has no + OR, so both are needed — and dropping either one breaks a different half: without the first + nobody sees the annotations, without the second nothing is enforced. +- **`successCodes` lists `0` and nothing else.** `1` (findings) and `2` (the harness could not + run) both fail, and both must: a gate nobody has to obey is a report, and a gate that reports + "clean" without having looked is worse than no gate, because it is indistinguishable from a + clean pull request. Do not add codes here to get a build through. + +**Run the tests with `--test-concurrency=1`.** `corpus.acceptance.test.mjs` additionally needs +network access to the GitHub API (it resolves the corpus pull requests through `gh`) and carries +machine-dependent timing assertions; it goes red offline or on a slow machine while every other +file stays green. + ## Status -Pending the follow-up task's decision to **promote** this into the real gate (durable script + -CI hook in `core-web/pom.xml` + local hook in `lint-staged.config.mjs`) or **delete** it. +Live and blocking. The open question is cost, not correctness: the spike measured 8.4–9.4 s +average with a 12 s tail against a 10 s budget, on a synthetic corpus. That cost now sits on the +critical path of every frontend merge, so every run prints its own elapsed time in the job summary +— collect it on #37536, and if the real distribution is worse than the corpus suggested, the +conversation is about making the harness faster, not about switching the gate off. Either way the end state is the same: **#37198 merging retires this gate.** Promotion only changes how much there is to remove — see §3.3 of diff --git a/core-web/tools/scripts/strict-gate/format.duration.test.mjs b/core-web/tools/scripts/strict-gate/format.duration.test.mjs new file mode 100644 index 000000000000..159be535fb82 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/format.duration.test.mjs @@ -0,0 +1,239 @@ +/** + * The job summary must state how long the run took, over how much diff (issue #37536, FR-018). + * + * Why this is worth pinning: the gate blocks, so its cost is on the critical path of every + * frontend merge. The harness already measures every run — `durationMs.total` — and simply never + * printed it, so the only way to know the real distribution was to time runs by hand from CI + * logs, which is the class of chore that does not get done. + * + * Both the clean and the findings summary are covered on purpose: a duration emitted only when + * there are findings would sample the fast and slow cases unevenly, and it is the tail that + * matters. + * + * The count is of DISTINCT changed paths. Two of the cases below exist because the obvious + * implementation — summing `targets[].files.length` — is wrong in two ways at once: it + * double-counts a file claimed by two configs of one project, and it drops unmapped files + * entirely. + * + * **Assert on the extracted cost line, by equality — not with a regex over the whole summary.** + * The first version of this suite used whole-document regexes and four separate mutations of the + * implementation survived it, including one where the assertion was satisfied by the unmapped + * note rather than the cost line, because both render a number followed by the word "file". + * Equality on one extracted line is what the sibling suites do (`hunks.test.mjs`, + * `report.contract.test.mjs`), and it is what catches those. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { buildReport } from './lib/report.mjs'; +import { formatMarkdown } from './lib/format.mjs'; + +const SHA_A = 'a'.repeat(40); +const SHA_B = 'b'.repeat(40); + +const reportWith = ({ findings = [], durationMs, targets = [], unmapped = [] }) => + buildReport({ + base: SHA_A, + head: SHA_B, + flagSet: 'strict', + granularity: 'line', + targets, + unmapped, + findings, + discarded: { + byOrigin: { dependency: 0, untouched: 0, infrastructure: 0 }, + byLayer: { source: 0, template: 0 } + }, + durationMs + }); + +const target = (project, files) => ({ + project, + root: `libs/${project}`, + configPath: `libs/${project}/tsconfig.lib.json`, + mode: 'typescript', + files +}); + +const finding = { + file: 'libs/utils/src/lib/dot-utils.ts', + line: 270, + column: 35, + code: 'TS7006', + message: "Parameter 'value' implicitly has an 'any' type.", + origin: 'changed', + layer: 'source' +}; + +const unmappedEntry = { + path: 'libs/orphan/src/b.ts', + reason: 'no configuration includes this file' +}; + +/** The one line under test, isolated so an assertion cannot be satisfied by some other line. */ +const costLineOf = (summary) => { + const line = summary.split('\n').find((l) => l.startsWith('_') && l.endsWith('s._')); + assert.ok(line, 'the summary must carry a cost line'); + return line; +}; + +test('the clean summary states the elapsed time and the diff size', () => { + const report = reportWith({ + durationMs: { total: 7088, typescript: 6900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])] + }); + + assert.equal(report.exitCode, 0, 'guard: this fixture must be the clean case'); + assert.equal( + costLineOf(formatMarkdown(report)), + '_1 changed file(s) across 1 project config(s), 7.1s._' + ); +}); + +test('the findings summary states them too', () => { + const report = reportWith({ + findings: [finding], + durationMs: { total: 12400, typescript: 12100, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])] + }); + + assert.equal(report.exitCode, 1, 'guard: this fixture must be the findings case'); + assert.equal( + costLineOf(formatMarkdown(report)), + '_1 changed file(s) across 1 project config(s), 12.4s._' + ); +}); + +test('files are counted across every target', () => { + const report = reportWith({ + durationMs: { total: 9000, typescript: 8800, templateAware: 0 }, + targets: [ + target('utils', ['libs/utils/src/lib/a.ts', 'libs/utils/src/lib/b.ts']), + target('ui', ['libs/ui/src/lib/c.ts']) + ] + }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_3 changed file(s) across 2 project config(s), 9.0s._' + ); +}); + +test('a file claimed by two configs of one project counts once', () => { + // config-select claims a source under EVERY eligible config, so a lib/spec pair both holding + // the same file produces two target entries for one changed file. Summing `files.length` + // reports a one-file diff as two, which silently inflates the evidence this line exists for. + const file = 'libs/utils/src/lib/dot-utils.ts'; + const report = reportWith({ + durationMs: { total: 5000, typescript: 4800, templateAware: 0 }, + targets: [ + { ...target('utils', [file]), configPath: 'libs/utils/tsconfig.lib.json' }, + { ...target('utils', [file]), configPath: 'libs/utils/tsconfig.spec.json' } + ] + }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_1 changed file(s) across 2 project config(s), 5.0s._', + 'one changed file, claimed twice, is still one file' + ); +}); + +test('a changed file no project claimed is counted, not dropped', () => { + // An unmapped file was changed and NOT examined. Leaving it out understates the diff this + // duration came from. + const report = reportWith({ + durationMs: { total: 4000, typescript: 3900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/a.ts'])], + unmapped: [unmappedEntry] + }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_2 changed file(s) across 1 project config(s), 4.0s._', + 'one mapped + one unmapped = two changed files' + ); +}); + +test('unmapped files are named on a passing run', () => { + // The gate blocks now. A changed TypeScript file that no project compiles must not read as a + // clean pass with nothing said about it. + const report = reportWith({ + durationMs: { total: 4000, typescript: 3900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/a.ts'])], + unmapped: [unmappedEntry] + }); + + const summary = formatMarkdown(report); + + assert.equal(report.exitCode, 0, 'guard: this is the passing case'); + assert.match(summary, /libs\/orphan\/src\/b\.ts/, 'the unexamined file must be named'); + assert.match(summary, /not examined/i, 'and it must say it was not checked'); +}); + +test('unmapped files are named on a failing run too', () => { + // The branch an author actually reads. Covered separately because the first version of this + // suite exercised only the passing branch, which left the note removable from the findings + // branch with every test still green. + const report = reportWith({ + findings: [finding], + durationMs: { total: 8000, typescript: 7900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])], + unmapped: [unmappedEntry] + }); + + const summary = formatMarkdown(report); + + assert.equal(report.exitCode, 1, 'guard: this is the findings case'); + assert.match(summary, /libs\/orphan\/src\/b\.ts/, 'the unexamined file must be named here too'); + assert.match(summary, /not examined/i); +}); + +test('the findings table is separated from what precedes it', () => { + // A Markdown table renders as a table only when a blank line precedes it. Pinned because the + // unmapped note is inserted immediately above it, and losing that separator degrades the + // summary to a row of pipes without failing anything else. + const report = reportWith({ + findings: [finding], + durationMs: { total: 8000, typescript: 7900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])], + unmapped: [unmappedEntry] + }); + + const lines = formatMarkdown(report).split('\n'); + const header = lines.findIndex((l) => l.startsWith('| File |')); + assert.ok(header > 0, 'the findings table must be present'); + assert.equal(lines[header - 1], '', 'a blank line must precede the table'); +}); + +test('a sub-second run keeps its precision instead of rounding to zero', () => { + // The real boundary, not a value comfortably clear of it: `toFixed(1)` renders anything under + // 50ms as "0.0s". The measured no-op path is ~175ms, within a factor of four of that cliff. + const report = reportWith({ durationMs: { total: 40, typescript: 0, templateAware: 0 } }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_0 changed file(s) across 0 project config(s), 40ms._', + 'the cheapest and most common outcome must not be invisible in the evidence' + ); +}); + +test('a run at or over a second renders in seconds', () => { + const report = reportWith({ durationMs: { total: 1000, typescript: 900, templateAware: 0 } }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_0 changed file(s) across 0 project config(s), 1.0s._' + ); +}); + +test('a malformed duration renders as unknown, not NaN', () => { + // `formatMarkdown` is exported, and buildReport's default only fires on `undefined`, so a + // partial object gets through. A plausible-looking wrong number is the failure mode this file + // exists to avoid. + const report = reportWith({ durationMs: {} }); + + assert.equal( + costLineOf(formatMarkdown(report)), + '_0 changed file(s) across 0 project config(s), ?s._' + ); +}); diff --git a/core-web/tools/scripts/strict-gate/lib/format.mjs b/core-web/tools/scripts/strict-gate/lib/format.mjs index 14d3e0440979..3b93db223868 100644 --- a/core-web/tools/scripts/strict-gate/lib/format.mjs +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -114,15 +114,77 @@ export function formatGithub(report) { .join('\n'); } +/** + * What the run cost, paired with the diff size that produced it. + * + * Emitted on every markdown-formatted run, passing ones included. Note where it does NOT appear: + * `formatGithub` — the format this repository's CI invocation passes — never calls this, so the + * cost line reaches the run page only through `run.mjs`'s separate `GITHUB_STEP_SUMMARY` write, + * never stdout and never an annotation. Anyone reading the Maven log will not see it. + * + * The gate blocks, so its cost sits on the critical path of every frontend merge and the real + * distribution is worth knowing; a duration printed only when there are findings would sample the + * fast and slow cases unevenly, and it is the tail that matters. The diff size travels with it + * because a duration alone is not comparable between a one-file pull request and a forty-file one. + * + * One decimal place: the no-op case costs ~0.3s and rounding it to "0s" would make the cheapest + * and most common outcome invisible in the evidence. + */ +function costLine(report) { + // Guarded because this file's whole stance is that a plausible-looking wrong number is the + // dangerous failure mode, and `formatMarkdown` is exported: a partial `durationMs` object + // slips past buildReport's default (which only fires on undefined) and renders "NaNs". + const ms = report.durationMs?.total; + // Sub-second runs render in milliseconds. `toFixed(1)` turns anything under 50ms into "0.0s", + // and the cheapest case — a pull request touching no frontend file at all — measures ~175ms, + // within a factor of four of that cliff. Rounding the most common outcome to zero would make + // it invisible in exactly the evidence this line exists to provide. + const elapsed = !Number.isFinite(ms) ? '?s' : ms < 1000 ? `${Math.round(ms)}ms` : `${(ms / 1000).toFixed(1)}s`; + // Distinct paths, not (config -> file) assignments. `selectConfigs` claims a source under + // EVERY eligible config, so a project whose lib and spec configs both include a file yields + // two target entries for one changed file; summing `files.length` would report a one-file + // diff as two. Unmapped files count as well: they were changed, they just were not examined, + // and leaving them out understates the diff this duration came from. + const changed = new Set([ + ...report.targets.flatMap((t) => t.files), + ...report.unmapped.map((u) => u.path) + ]).size; + // Deliberately not "Checked": `changed` includes unmapped files, which by definition were not + // examined, and the note below says so. Two adjacent lines asserting opposite things about the + // same file is worse than a plainer verb. + return `_${changed} changed file(s) across ${report.targets.length} project config(s), ${elapsed}._`; +} + +/** + * Changed files no project claimed. They were NOT examined, and with the gate blocking, silence + * about them reads as a clean pass — the edge case the spec calls out by name. Naming them is not + * a failure signal; it is the difference between "nothing was wrong" and "nothing was looked at". + */ +function unmappedNote(report) { + if (report.unmapped.length === 0) return null; + const rows = report.unmapped.map((u) => `- \`${u.path}\` — ${u.reason}`).join('\n'); + return [ + `**${report.unmapped.length} changed file(s) were not examined** — no project configuration claims them:`, + '', + rows + ].join('\n'); +} + /** Markdown for the job summary — what a human opening the run sees first. */ export function formatMarkdown(report) { const total = ignoredCount(report); + // `costLine` already carries the project-config count, and it carries the file count too, so + // it is the more informative of the two places that used to state it. + const note = unmappedNote(report); + if (report.exitCode === 0) { return [ '## ✅ strict-gate: pass', '', - `No new strict-mode violations. ${total} pre-existing or dependency diagnostic(s) ignored, ` + - `across ${report.targets.length} project config(s).` + `No new strict-mode violations. ${total} pre-existing or dependency diagnostic(s) ignored.`, + '', + costLine(report), + ...(note ? ['', note] : []) ].join('\n'); } @@ -135,6 +197,9 @@ export function formatMarkdown(report) { '', `**Scope.** ${scopeRule(report.granularity)} ${total} diagnostic(s) from dependencies and untouched code were ignored — fix only what is listed.`, '', + costLine(report), + ...(note ? ['', note] : []), + '', '| File | Line | Code | Message |', '|---|---|---|---|', rows, diff --git a/specs/37536-wire-strict-gate-ci/contracts/ci-hooks.md b/specs/37536-wire-strict-gate-ci/contracts/ci-hooks.md new file mode 100644 index 000000000000..2c7d3619ecca --- /dev/null +++ b/specs/37536-wire-strict-gate-ci/contracts/ci-hooks.md @@ -0,0 +1,78 @@ +# Contract: the CI invocation + +**Feature**: [../spec.md](../spec.md) | **Plan**: [../plan.md](../plan.md) | **Date**: 2026-09-14 + +The harness's own command contract is unchanged and lives at +`specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md`. This file specifies only the one +place this feature invokes it from, and the exact guarantees it must uphold. + +**Scope note.** An earlier draft of this feature also added a local `pre-push` git hook that +refused pushes on findings. It was cut: this change is continuous-integration verification only. +The reasoning that produced it is preserved in the spec's clarifications C-001 and C-002 for +whoever picks up a local hook later — in particular the finding that the harness compares two +*committed* points, so a commit-time hook would examine the previous commit and report "clean" on +the very violation being committed. + +--- + +## The `strict-gate` execution — `core-web/pom.xml` + +### Shape + +An `` with id `strict-gate`, goal `exec`, phase `generate-resources`, declared **inside +the existing `validate` profile** so it does not exist unless `-Pvalidate` is active (R-005). + +The exec plugin's `` for this module is `${node.install.dir}/pnpm`, so the argument list +begins with `exec` — the same shape `lint-test` and `format-test` already use. + +``` +exec node tools/scripts/strict-gate/run.mjs + --base=${git.origin.branch} + --flags=strict + --granularity=line + --scope=core-web + --format=github +``` + +`parseArgs` in `run.mjs` splits on `=`, so the inline `--flag=value` form works as-is. Unknown +options are rejected by name — do not invent flags. + +### Required configuration, and why each element is load-bearing + +| Element | Value | Consequence if wrong | +|---|---|---| +| `` | `${skip.strict.gate}` | Governs FR-006. Defaults `true`; **two** profiles flip it to `false`, on `env.GITHUB_EVENT_NAME` = `pull_request` and = `merge_group`. Maven activation has no OR, hence two. Lose the first and nobody sees the annotations; lose the second and nothing is enforced, because `main` declares no required status checks. | +| `` | `0` — **and nothing else** | The entire Run Outcome model. `1` (findings) and `2` (could not run) both fail. Adding `1` turns the gate back into a report; adding `2` makes a broken harness look like a clean pull request. | +| `` | `180000` | FR-013. Verified (R-002) to kill with exit `143`, which is outside `successCodes`, so the build fails and the log names the timeout. | +| `` | `false` | **Pin explicitly, with a comment, even though it is the default.** Verified (R-001): the default streams stdout raw, which is why `::error` lines reach column 0 and GitHub renders them. Flipping it to `true` prefixes every line `[INFO] `, silently killing every annotation while the build stays green and the job summary still looks fine. | + +### Guarantees this invocation must uphold + +- **Findings fail the check** (FR-010) — exit `1` is not a success code, so a violation on a line the pull request wrote cannot merge. +- **A harness that could not run also fails it** (FR-011) — exit `2` and `143` likewise. Separate requirement, same consequence; keep them separate in the reasoning even though the configuration cannot distinguish them today. +- **Nothing runs on trunk or nightly** (FR-006) — the diff is empty there. +- **No retry or classification logic of our own** (FR-012) — whatever the harness reports reaches + the build unaltered. Its own three-step base-ref recovery is the only recovery. +- **No workflow file is touched** (FR-005). `cicd_comp_test-phase.yml` already fetches `origin/main` + for `-Pvalidate` jobs and `.github/filters.yaml` already gates the job on `core-web/**`. + +### Verification + +Before the change, both come back empty. After it, both must match: + +```bash +git grep -n "strict-gate" -- core-web/pom.xml +``` + +A green build is **not** evidence the gate ran — see [../research.md](../research.md) R-003. Confirm +the gate's own output is present. + +## What this invocation may not do + +- Add a dependency, to Maven or to npm. +- Change what the harness *decides* — what it examines, what counts as a violation, how the + comparison is made (FR-017). The elapsed-time line (FR-018) is output, not a decision, and is the + only permitted edit. +- Register the harness as an Nx project. That would place it inside the graph it measures. +- Add any local git hook. Commit-time and push-time checks are both out of scope for this change; + `core-web/lint-staged.config.mjs` and `core-web/.husky/` are untouched. diff --git a/specs/37536-wire-strict-gate-ci/data-model.md b/specs/37536-wire-strict-gate-ci/data-model.md new file mode 100644 index 000000000000..db420144115e --- /dev/null +++ b/specs/37536-wire-strict-gate-ci/data-model.md @@ -0,0 +1,106 @@ +# Data Model: Wire the diff-scoped strict typecheck gate into CI + +**Feature**: [spec.md](./spec.md) | **Plan**: [plan.md](./plan.md) | **Date**: 2026-09-14 + +This feature stores nothing. It has no database table, no index mapping, no serialized state, and no +persisted record of any kind. What it does have is a small set of entities that move between the +harness, the build and the reader — and one of them, **Run Outcome**, is the whole feature's +correctness condition. That is why this file exists rather than being skipped. + +The harness's own data shapes were defined by PR #37403 and are unchanged here +(`specs/37401-diff-scoped-strict-typecheck-gate/data-model.md`, +`contracts/report.schema.json`). What follows is only what *this* feature adds or constrains. + +--- + +## Run Outcome — three states that must never collapse into two + +The single most important structure in the feature. Every requirement about blocking, failing and +reporting is a statement about this enum. + +| State | Harness exit | Build result | What the reader sees | +|---|---|---|---| +| **Clean** | `0` | pass | Nothing. No annotations, no findings in the summary. Includes the no-op case where the diff touches no frontend file. | +| **Findings** | `1` | **fail** | Inline annotations on the changed lines, plus a summary listing them. The change does not merge until they are fixed (FR-010). | +| **Could not run** | `2`, or killed at the time limit (`143`) | **fail** | A failed check naming the cause (FR-011, FR-013). | + +**Invariant — the one that defines the feature**: *Clean* must be distinguishable from both other +states. *Findings* and *Could not run* share a consequence today (both fail) but are different +facts, and must stay separate in the reasoning even where the configuration cannot separate them. The failure mode this guards against is +a harness that reports "clean" without having looked, which is indistinguishable from a genuinely +clean pull request and would silently defeat the gate — the same class of defect as the gate +existing and never executing, which is why issue #37536 exists. + +**Encoding** (see [contracts/ci-hooks.md](./contracts/ci-hooks.md)): `` lists `0` and +nothing else. The absence of everything else is the mechanism; a comment in the POM must say so, +because a well-meaning "let's tolerate 1 while we clean up" would be invisible in review and would +turn the gate back into a report. + +**State transitions**: none. A run produces exactly one outcome and nothing survives it. + +--- + +## Execution Context — where the gate is allowed to run + +Derived, not stored. Read from the environment at build time (R-005). + +| Field | Source | Values | +|---|---|---| +| Event | `env.GITHUB_EVENT_NAME` | `pull_request` → run (annotations); `merge_group` → run (enforcement); anything else → skip | +| Validation profile | `-Pvalidate` | active → the execution exists; absent → it does not | +| Path relevance | `.github/filters.yaml` `frontend` filter | decides whether the containing job runs at all | + +The three are independent and all must hold. Note the layering: the path filter is evaluated by CI +*before* the job starts, the profile decides whether the execution is declared, and the event name +decides whether a declared execution is skipped. A backend-only pull request is stopped by the first +of the three and never reaches the other two — which is why SC-002 asks for *zero* measurable cost, +not merely a fast no-op. + +--- + +## Finding + +Produced by the harness; this feature only routes it. One strict violation. + +| Field | Meaning | Used by | +|---|---|---| +| `file` | Repository-relative path | annotation target, summary row | +| `line`, `column` | Position within the changed hunk | annotation target | +| `code` | The TypeScript diagnostic code (e.g. `TS4111`) | fix guidance lookup in `format.mjs` | +| `message` | What is wrong | annotation body | + +**Constraint carried from the spec (FR-002)**: every finding's `line` falls inside a hunk this +branch added or modified. A finding outside the diff is a defect, not a strict violation — the spike +measured 99.1 % of diagnostics being discarded on a representative portlet precisely to make this +hold. + +--- + +## Observed Duration — the evidence artifact + +The only entity this feature *adds*, and it is added for one reason: SC-005 cannot be satisfied if +nobody can read a run's cost — and now that the gate blocks, that cost is on every frontend merge. + +| Field | Source | Notes | +|---|---|---| +| Elapsed time | `report.durationMs.total`, already computed by the harness | Never printed before this feature — FR-018 surfaces it | +| Diff size | distinct paths across `report.targets[].files` **and** `report.unmapped[].path` | There is no `report.files`. The count must be of unique paths: `selectConfigs` claims a source under every eligible config, so one changed file can appear in two targets, and unmapped files were changed too — they just were not examined. | + +**Lifecycle**: emitted into the job summary on every run, read by a human, transcribed onto issue +#37536 for at least five real pull requests (FR-022). Nothing is persisted by the system — the issue +comment *is* the record. That is deliberate: building storage for five numbers would cost more than +the decision they inform. + +**Why it must appear on clean runs too**: a duration recorded only when findings exist would sample +the fast cases and the slow cases unevenly — and with the gate blocking, the tail is what sits on +the critical path of every frontend merge. + +--- + +## Not modelled here + +- **The report JSON** — defined and versioned by PR #37403; this feature neither extends nor reads + it beyond the two fields above. +- **Any per-developer state.** An earlier draft carried a local push-time check with an opt-out + environment variable; that was cut, and this change is continuous-integration verification only. + Nothing here is per-developer, so there is nothing to model. diff --git a/specs/37536-wire-strict-gate-ci/spec.md b/specs/37536-wire-strict-gate-ci/spec.md new file mode 100644 index 000000000000..4883698896d5 --- /dev/null +++ b/specs/37536-wire-strict-gate-ci/spec.md @@ -0,0 +1,280 @@ +# Feature Specification: Wire the diff-scoped strict typecheck gate into CI + +**Feature Branch**: `nicobytes/37536-wire-the-diff-scoped-strict-typecheck-gate-into-ci-merged-on-main-but-never-executes` + +**Created**: 2026-09-14 + +**Status**: Draft + +**Type**: New Feature (activation of previously merged, never-executed capability) + +**Issue**: [#37536](https://github.com/dotCMS/core/issues/37536) + +**Input**: User description: "https://github.com/dotCMS/core/issues/37536 — the strict typecheck gate added in PR https://github.com/dotCMS/core/pull/37403 is merged on main but never executes in CI" + +--- + +## Context + +Spike [#37401](https://github.com/dotCMS/core/issues/37401) / PR [#37403](https://github.com/dotCMS/core/pull/37403) delivered a working diff-scoped strict typecheck harness and merged it to `main` on 2026-09-08. The harness was wired into nothing — no build hook, no local hook, no workflow. Since then every pull request has passed without it, and `main` has kept accumulating non-strict TypeScript exactly as before. + +Re-verified on this worktree at `de342798f4`: + +| Check | Result | +|---|---| +| References to the gate outside its own directory and spec directory | none | +| Build executions in the frontend module | `validate-dist-paths`, `pnpm-install`, `lint-test`, `format-test`, `build-test`, `build-analytics`, `unit-test`, `nx-reset`, `prod`, `do-nx-reset`, `validate`, `format`, `auto-format`, `auto-lint` — the gate is absent | +| Local pre-commit configuration | lint and format only — the gate is absent | +| Workflows mentioning the gate | none | + +The capability is therefore dormant, not missing. This feature turns it on **as a blocking gate**: a change that adds a strict-mode violation on a line it wrote does not merge. + +--- + +## Clarifications + +Both were raised by inspecting the merged harness, not by reading the issue. Neither had a safe default, and both change what gets built. + +### C-001 — The local check refuses the push; the pull request only reports *(SUPERSEDED TWICE — the local check was cut (US4), and the pull-request check now blocks too (FR-010). Kept as a record of how the decision moved.)* + +**Asked because**: the issue specifies reporting-only for continuous integration and is silent on the local hook. Left to its defaults, a local hook aborts on findings — making local enforcement stricter than the pull request without anyone deciding so. + +**Decided**: deliberately stricter locally. The pull request never blocks; the local push check does. Blocking at push is cheap to reverse (a developer-machine setting, not continuous-integration configuration), it is the only enforcement in this release, and it stops the debt before a reviewer ever sees it. The standard bypass remains available and documented, so a developer who genuinely needs to push failing work still can. + +**Affects**: US4 (cut — see the story for what was kept). + +### C-002 — The local check runs at push time, not commit time *(SUPERSEDED — the local check was cut; the finding behind it is still load-bearing, see US4)* + +**Asked because**: the issue's acceptance list places the hook in the commit-time staged-files configuration. The harness as merged compares two committed points; it has no notion of staged-but-uncommitted content and rejects unrecognised options. A commit-time hook reusing that comparison would examine the *previous* commit and report "clean" on the very violation being committed — the exact case the hook exists to catch. Closing that would mean adding capability to a harness the spike declared finished. + +**Decided**: run at push time instead. Pushing compares committed branch content against the trunk — precisely the comparison the harness already performs and the same one the pull request will make, so local and remote answers agree by construction. It costs one run per push rather than one per commit, and requires no new harness capability. + +**Consequence — a deliberate deviation from the issue's acceptance list**: the commit-time staged-files configuration is *not* modified. The issue's related concern about non-overlapping glob keys racing on git's index lock disappears with it, since the check no longer shares that execution path. This deviation must be stated in the pull request description (FR-020). + +**Affects**: US4 (cut), FR-017, Edge Cases. + +### Session 2026-09-14 + +- Q: The gate runs inside a required check. What happens when the harness cannot run at all (unresolvable comparison point, unreadable project graph, unparseable configuration)? → A: Fail the check, always. A gate that reports "clean" without having looked is the one failure mode that defeats the whole feature; the harness already retries the shallow-checkout case itself, so the residual transient surface is small. Downgrading later is trivial; upgrading later means re-litigating the decision. +- Q: The frontend test suite runs on pull requests, in the merge queue, on trunk pushes and nightly. In which of those should the gate execute? → A: Pull requests only. Inline annotations can only render on a pull request, so anywhere else the gate is cost without value — and a merge-queue run would add 9–12 s plus the FR-011 merge-blocking failure mode to the most contention-sensitive part of the pipeline. Trunk and nightly would be near-free no-ops (the comparison yields an empty diff), but are excluded for the same reason: nothing reads the result. +- Q: *(superseded — the local check was cut)* How is the blocking pre-push check rolled out to developers? → A: On by default, individually disableable. It installs with the repository's hooks and blocks from day one, but a documented, permanent per-machine opt-out exists alongside the per-push bypass. Real enforcement without becoming something people tear out by hand — anyone who opts out still has the pull-request check as the backstop, and the spec already treats the local check as an accelerator rather than the primary mechanism. +- Q: Should the gate carry its own time limit in CI, and what happens when it is reached? → A: Yes, and exhausting it fails the check. An explicit cap, generous against the measured 9–12 s (on the order of a couple of minutes). Exhausting it is indistinguishable from "the harness could not run", so it follows the same rule as FR-011: fail, visibly. This bounds the worst case of a deep re-fetch without inventing error classification, and gives SC-005 a concrete number to compare the observed tail against. +- Q: How is each run's observed duration captured so SC-005 can be satisfied? → A: One duration line in the job summary the gate already writes. The harness already measures the run; it simply never prints it. This puts the number where a human already looks, with no artifact and no new step. It requires relaxing the "no harness changes" rule to permit OUTPUT-only changes — stated explicitly rather than left to slip through. + +--- + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - A frontend author sees strict violations on the lines they wrote (Priority: P1) + +A developer opens a pull request that changes TypeScript under the frontend workspace. The pull request's checks run as usual. Where their changed lines would fail under the project's strict convention, the pull request shows those violations directly on the affected lines of the diff, and the run page carries a summary listing them. The pull request **does not merge** until those lines are fixed: the same check runs again in the merge queue, where a failure ejects it. The author sees the debt at the moment they add it, and cannot ship it. + +**Why this priority**: This is the entire point of the feature. Without it the gate remains dead code and the strict-debt leak stays open. Every other story is a guard rail around this one. + +**Independent Test**: Open a pull request that introduces a known strict violation on a changed line of frontend TypeScript; confirm the violation is reported on that line, appears in the run summary, and that the check fails. + +**Acceptance Scenarios**: + +1. **Given** a pull request whose diff adds a line of frontend TypeScript that violates the strict convention, **When** the frontend checks run, **Then** the violation is reported against that exact file and line on the pull request diff, and a summary of all violations appears on the run page. +2. **Given** a pull request whose frontend TypeScript changes are all strict-clean, **When** the frontend checks run, **Then** no violations are reported and no summary noise is produced. +3. **Given** a pull request that reports violations, **When** it is added to the merge queue, **Then** the queue run fails and the pull request is ejected — it does not merge until the violations are fixed. +4. **Given** a pull request that modifies an existing file whose surrounding, untouched lines already violate the strict convention, **When** the gate runs, **Then** only the lines this pull request changed are reported; the pre-existing violations on untouched lines are not attributed to the author. +5. **Given** a stale branch that has fallen many commits behind the trunk, **When** the gate runs, **Then** the reported violations are limited to what the branch itself changed and do not include changes the trunk made in the meantime. + +--- + +### User Story 2 - A backend-only pull request is untouched (Priority: P1) + +A developer opens a pull request that changes only backend, documentation, or build files. Nothing about the gate is visible to them: no added wait, no annotations, no summary, no new failure mode. + +**Why this priority**: The repository is predominantly backend. If activation taxes every unrelated pull request, the change is a net regression regardless of how well it works on frontend pull requests. This must be verified, not assumed. + +**Independent Test**: Open a pull request touching only backend files; confirm no gate output appears anywhere and no measurable time is added to the pull request's checks. + +**Acceptance Scenarios**: + +1. **Given** a pull request that changes no frontend files, **When** its checks run, **Then** the gate produces no annotations and no summary. +2. **Given** a pull request that changes no frontend files, **When** its checks run, **Then** no additional wall-clock time attributable to the gate is added. + +--- + +### User Story 3 - A broken gate fails loudly instead of silently passing (Priority: P1) + +The harness cannot run — the comparison point is unresolvable, the project graph is unreadable, or configuration cannot be parsed. Rather than reporting "clean" and letting the pull request through, the checks fail and say so. + +**Why this priority**: A gate that reports nothing is indistinguishable from a clean pull request. This is the one failure mode that would quietly defeat the whole feature, and it stays dangerous even with blocking on: "the harness could not run" and "nothing was wrong" must never collapse into the same outcome. + +**Independent Test**: Force a harness failure (for example, an unresolvable comparison point) and confirm the checks fail with a message identifying the harness as the cause — not a silent pass. + +**Acceptance Scenarios**: + +1. **Given** the harness cannot resolve its comparison point, **When** the checks run, **Then** the run fails and the reason is stated in the log. +2. **Given** the harness completes and finds violations, **When** the checks run, **Then** the run passes. +3. **Given** the harness completes and finds nothing, **When** the checks run, **Then** the run passes. +4. **Given** a run exceeds the gate's time limit, **When** the limit is reached, **Then** the gate is aborted and the check fails — an unfinished check is treated exactly like one that could not start. +5. **Given** a change moving through the merge queue, **When** the frontend validation runs there, **Then** the gate does not execute at all and therefore cannot fail the queue. + +> The distinction between "found problems" and "could not look" must be explicit in the wiring, not an accident of which exit values happen to be tolerated. "Ran out of time" belongs on the "could not look" side. + +--- + +### User Story 4 - A developer is stopped before pushing new strict debt — **CUT, deferred** + +**Status: removed from this feature on 2026-09-14.** This change is continuous-integration +verification only; it adds no local git hook. + +The story was specified, approved and built — a `pre-push` hook that ran the same +branch-versus-trunk comparison and refused the push on findings — and then cut before merge on +the decision that the feature should deliver CI verification alone. + +Kept here rather than deleted because the work produced two findings a future local hook will hit +again, both recorded in Clarifications C-001 and C-002: + +- **The harness compares two committed points.** A commit-time hook reusing that comparison + examines the *previous* commit and reports "clean" on the very violation being committed. This + was verified, not theorised — the harness reported `checked 0 project config(s)` against an + uncommitted change. Any local hook must therefore sit at push time, or the harness must grow a + notion of staged content. +- **Blocking locally while the pull request only reports** is a coherent position (C-001), but it + is a separate decision from wiring the gate into CI and should be taken on its own. + +Consequence for this release: **enforcement lives in CI, not on the developer's machine.** The +merge-queue run is what stops a violation from merging (see FR-006). A developer who wants the +answer before pushing runs the harness themselves; nothing does it for them automatically. + +--- + +### User Story 5 - The team has measured evidence for the blocking decision (Priority: P2) + +Once the gate is live, its real cost on real pull requests is recorded so the later decision to make it blocking rests on observed data rather than on the spike's synthetic corpus. + +**Why this priority**: The gate now blocks, so its cost sits on the critical path of every frontend merge. The spike's numbers (8.4–9.4 s average, 12 s tail) were measured on a synthetic corpus; what matters now is the real distribution, because a gate that is both blocking and slow is the one that gets switched off. + +**Independent Test**: After the gate is live, confirm that observed durations from at least five distinct real pull requests are recorded on the issue. + +**Acceptance Scenarios**: + +1. **Given** the gate has run on at least five real pull requests, **When** the evidence is gathered, **Then** each run's observed duration is recorded on the issue alongside the size of the diff it examined. +2. **Given** the recorded durations, **When** the blocking decision is revisited, **Then** both the typical and worst observed durations are available. + +--- + +### Edge Cases + +- **Annotations must actually reach the pull request.** Reported violations travel through the build tool's log. If that log decorates each line, the pull request may show nothing on the diff even though the gate ran correctly and the summary is fine. This must be confirmed on a real pull request before the work is considered done; a green run is not evidence that annotations rendered. +- **Shallow checkouts.** Test runners check out with minimal history. The harness deepens the checkout itself when the comparison point is missing, but a branch that has diverged far from the trunk can force a progressively deeper fetch — on a repository this size, far more expensive than the gate itself. This is the pathological case the time limit exists to catch: the run is aborted and the check fails, rather than the job being held hostage. +- **Renamed and deleted files.** A pull request that only deletes or renames frontend TypeScript has changed lines that no longer exist in a compilable form; this must be a clean pass, not an error. +- **A pull request touching frontend files that belong to no project.** The workspace has projects with nothing that compiles them; files outside any mapped project must be reported as unexamined rather than silently treated as clean. +- **Very large frontend pull requests.** A sweeping refactor touching hundreds of frontend files must not exceed the check's time budget or produce a summary so large it is unusable. +- **A long-lived branch's first run** is judged on the branch's combined effect against the trunk, not on its newest commit — so it can surface violations from work done days earlier. This is the same question the pull request asks and is intended, but it is the case most likely to surprise an author. + +--- + +## Requirements *(mandatory)* + +### Functional Requirements + +**Activation** + +- **FR-001**: The frontend build MUST invoke the diff-scoped strict typecheck as part of the same validation stage that already runs frontend lint and format checks, so that it executes on every pull request that runs frontend validation and on no other build. +- **FR-002**: The gate MUST compare the pull request's branch against the trunk and report only violations on lines the pull request added or modified. +- **FR-003**: The gate MUST be evaluated against the repository's established strict convention — the same standard the workspace-wide strict migration is moving toward — so that what it reports today is exactly what will be required tomorrow. +- **FR-004**: The gate MUST examine only frontend files. A change outside the frontend workspace MUST be a no-op pass. +- **FR-005**: No continuous-integration workflow definition changes are required; if the plan concludes otherwise, the reason MUST be stated explicitly rather than the change being made silently. +- **FR-006**: The gate MUST execute on pull requests **and in the merge queue**, and nowhere else. Trunk and scheduled runs MUST skip it: the diff there is empty and the run could only ever pass. Both contexts are required and they do different jobs — the pull-request run is where annotations render and the author reads them; the merge-queue run is what actually prevents a merge, because this repository declares no required status checks on `main`. This MUST be achieved without modifying workflow definitions (FR-005). + +**Reporting semantics** + +- **FR-007**: Violations MUST appear inline on the pull request diff, on the exact file and line reported. +- **FR-008**: Violations MUST additionally appear as a summary on the run page, readable without opening the raw log. +- **FR-009**: Reported output MUST state its own scope rule — that only changed lines were examined and that violations in dependencies were discarded deliberately — before listing violations, so that a reader (human or automated) does not respond by refactoring untouched code. +- **FR-010**: Findings MUST fail the check. A violation on a line the pull request wrote MUST prevent the change from merging, and the failure MUST name the file, the line and the rule so the author can act without opening the raw log. +- **FR-011**: A harness that could not complete MUST fail the check — **every time, with no exception for transient causes.** This is a separate requirement from FR-010 and must stay separate in the configuration: findings and "could not look" are different outcomes that happen to share a consequence today. A gate reporting "clean" without having looked is indistinguishable from a clean pull request, which defeats the feature entirely. +- **FR-012**: The wiring MUST NOT introduce retry, fallback, or failure-classification logic of its own. The harness's own recovery behavior is the only recovery; anything it surfaces as "could not run" reaches the build unaltered. +- **FR-013**: The gate MUST be bounded by an explicit time limit, set generously above the measured typical run so that it is reached only by a pathological case, never by a slow-but-healthy one. Reaching it MUST abort the gate and fail the check under the same rule as FR-011 — an unfinished check is not a passing one. The limit MUST be stated as a number the team can revisit against the evidence gathered under SC-005. + +**Integrity of the existing harness** + +- **FR-014**: The harness's existing test suite MUST still pass in full after this change. +- **FR-015**: The requirement that the harness's tests run serially MUST be documented wherever those tests are invoked, so that a parallel run's intermittent timeout is never mistaken for a real failure. +- **FR-016**: A full run of the gate MUST leave every version-controlled file byte-identical. +- **FR-017**: This feature MUST NOT change what the harness *decides*. It wires up what PR #37403 delivered; if the plan concludes a behavioral change — what is examined, what is reported as a violation, how the comparison is made — is unavoidable, that MUST be raised as a scope change rather than absorbed. **Changes to what the harness *prints* are permitted** and are the sole exception (see FR-018). +- **FR-018**: Every run MUST report its own elapsed time in the same summary it already writes, so the evidence FR-022 requires can be read off a run page rather than reconstructed from logs. This is an output-only change and is the only harness edit this feature allows. + +**Documentation and issue hygiene** + +- **FR-019**: The harness's own documentation MUST no longer state that the gate is wired into nothing; it MUST state where it is invoked from, that the pull-request check only reports, and that the local check refuses pushes. +- **FR-020**: The pull request description MUST state what is deliberately excluded: making the pull-request check blocking, checking templates, and the untried performance optimisations. It MUST also state that the local hook runs at push time rather than commit time, and why — see Clarifications, C-002. +- **FR-021**: The superseded predecessor issue MUST be confirmed closed and cross-referenced. *(Verified already closed as of 2026-09-14; confirm the cross-reference exists.)* + +**Evidence** + +- **FR-022**: Observed durations from at least five real pull requests MUST be recorded on the issue, each paired with the size of the diff examined, so the later blocking decision has a measured distribution rather than a single corpus figure. + +### Key Entities + +- **Diff scope**: the set of lines a pull request added or modified within the frontend workspace. Everything reported must fall inside it; everything outside it is discarded by design. +- **Finding**: one strict violation, identified by file, line and the rule it breaks. +- **Run outcome**: three distinct states that must never collapse into two — clean, violations found, harness could not run. +- **Observed duration**: the wall-clock cost of one gate run, paired with the diff size that produced it; the raw material for the blocking decision. + +--- + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Within one week of activation, at least one real pull request has displayed a strict violation inline on its diff — demonstrating the mechanism works end to end on production traffic, not just in a test. +- **SC-002**: 100% of pull requests that change no frontend files complete with zero gate output and no measurable added time. +- **SC-003**: No change carrying a new strict-mode violation on a line it wrote reaches `main` after this ships. Measured by the absence of such violations in `main`'s history from the merge date onward. +- **SC-004**: 100% of runs in which the harness cannot complete result in a failed check, with the cause identifiable from the log alone. +- **SC-005**: Across at least five observed real pull requests, both the typical and the worst duration are recorded, and the worst is stated against the 10-second budget the spike set. +- **SC-006**: Every violation reported on a real pull request during the first week is adjudicated as genuine; any false positive is recorded and triaged before the blocking decision is revisited. +- **SC-007**: Zero merge-queue, trunk or scheduled runs execute the gate — verified by its absence from those runs' output, not assumed from configuration. +- **SC-008**: No gate run exceeds its stated time limit during the observation period; if one does, it is recorded as the pathological case the limit exists to catch, with its cause identified. +- **SC-009**: A full gate run leaves the working tree clean — zero modified version-controlled files, verified after the run. +- **SC-010**: The harness's documentation and the pull request description together let a reader unfamiliar with the spike answer, without opening the code: where the gate runs, what it checks, what it ignores, and what it will never do until a separate decision is taken. + +--- + +## Legacy Considerations *(dotCMS-specific — mandatory)* + +- **Existing behavior touched**: The frontend validation stage of the build — the same stage that already carries lint and format. Nothing else: no local git hook, no workflow file. This is developer-facing build tooling, not product surface: no runtime behavior, no content, no API, no database. It sits adjacent to continuous-integration configuration, an area where a mistake is felt by every contributor at once rather than by one feature's users. +- **Backward-compatibility expectations**: Every existing check must behave exactly as it does today. The frontend validation job is a required check. The gate must never turn it red on *findings*; it deliberately can and will turn it red when the harness itself could not run or ran past its time limit, which is a real merge block and an accepted trade. It must not slow the job enough to matter. Contributors who change no frontend code must see no difference whatsoever, and no contributor's local workflow changes at all — `core-web/.husky/` is untouched, so what a commit or a push produces is exactly what it produces today. +- **Known related decisions**: The frontend merge-time reduction this repository already paid for is the reason the gate's runtime budget exists at all, and is why the blocking flip is deferred rather than taken now. The workspace-wide strict migration ([#37198](https://github.com/dotCMS/core/issues/37198)) is the eventual replacement for this gate — but only if it also gains a mechanism that actually type-checks the projects with no build target and the test files the build excludes, which it does not today. The harness carries its own decommission procedure describing what to remove once that condition is met. The plan phase will formally consult `dotCMS/platform-adrs`. + +--- + +## Assumptions + +- The harness merged in PR #37403 is correct as delivered. This feature activates it; it does not re-validate the spike's findings or redesign the check. +- The existing path filter that already restricts frontend jobs to frontend changes is sufficient to keep the gate off unrelated pull requests, and needs no modification. +- Test runners already obtain the trunk reference they need for the existing frontend checks; the gate reuses the same reference rather than introducing a new fetch. +- The evidence-gathering requirement (FR-022, SC-005) is satisfied after the change merges, by observing live pull requests. It is a post-merge obligation recorded on the issue, not a code deliverable that could gate the pull request itself. +- Making the pull-request check blocking, checking Angular templates, and the three untried performance optimisations are all deliberately excluded. Each is its own decision with its own evidence; none is a follow-on task implied by this one. +- Developers have the repository's local git hooks installed. Those who do not — and those who use the documented opt-out — simply lose Story 4; the pull-request check (Stories 1–3) is unaffected and remains the backstop. Opt-out rate is worth watching: if most of the team disables it, the local check is not earning its cost and should be reconsidered rather than tolerated. +- Excluding merge-queue, trunk and scheduled runs costs nothing in coverage: every change reaches the trunk through a pull request, where the gate does run. +- "Strict convention" means the standard already established for this repository and targeted by the workspace-wide migration — not the broader, stricter set the spike also measured and explicitly did not propose. +- Adjudicating reported violations (SC-006) is a human judgement made by the frontend team, not an automated check. + +--- + +## Out of Scope + +- **An escape hatch for a legitimate exception.** There is none. A frontend change that must add + a strict violation — porting legacy code, an unavoidable third-party shape — cannot merge until + the violation is fixed or the gate is turned off repository-wide. No label, no opt-out, no + per-file suppression. This is a real gap, accepted knowingly: the spike measured 0 false + positives across its corpus, so the expected frequency is low, and inventing a bypass before + anyone needs one tends to produce the bypass everybody uses. Revisit if it bites. +- Checking Angular templates. +- Performance work on the harness. +- Changing what the harness decides — what it examines, what counts as a violation, how the comparison is made — including any notion of staged-but-uncommitted content (C-002). Adding the elapsed-time line to its output (FR-018) is the single permitted exception. +- Running the gate on trunk or scheduled builds: the diff there is empty, so it could only ever pass. +- Any local git hook — commit-time or push-time. `core-web/lint-staged.config.mjs` and + `core-web/.husky/` are untouched. A push-time hook was built and then cut on 2026-09-14 when the + feature was narrowed to continuous-integration verification only; what the work established is + preserved in US4 and in C-001/C-002. +- Changes to workflow definitions, unless the plan phase demonstrates they are unavoidable and says why. +- Any change to the workspace-wide strict migration (#37198) or to the projects' own type configuration. +- Removing or relocating the harness. It remains where PR #37403 put it.