From 973da593c48d03ac73023b852b2ea5e483ea8cb1 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Mon, 14 Sep 2026 16:42:54 -0400 Subject: [PATCH 1/5] ci(core-web): block merges that add strict-mode violations (#37536) The strict-gate harness merged with #37403 was wired into nothing and has never executed once. Since 2026-09-08 no pull request has been annotated, reported on, or blocked by it, and main has kept accumulating non-strict TypeScript exactly as before. This wires it into CI as a blocking gate: a change that adds a strict-mode violation on a line it wrote does not merge. A `strict-gate` execution in core-web/pom.xml, inside the existing `validate` profile beside lint-test and format-test. `successCodes` lists 0 and nothing else -- findings (1) and a harness that could not run (2) both fail. A 180s timeout fails the same way, bounding the harness's unshallow git-fetch fallback. It runs in two contexts, and both are load-bearing for different reasons. `pull_request` is where the author reads it: the harness emits `::error file=,line=,col=` lines that GitHub renders inline on the changed lines, plus a job summary. `merge_group` is where it is ENFORCED. This repository declares no required status checks on main -- the ruleset requires a pull request, one approval, thread resolution and signed commits -- so a red check on a pull request does not by itself stop a merge. A job that fails in the merge queue ejects the pull request, and that does. Wiring only the first would have produced a gate that goes red and merges anyway. Trunk and nightly stay skipped: HEAD equals origin/main there, so the diff is empty. Maven property activation has no OR, hence two profiles rather than one condition. No workflow file changes; activation reads the runner's own GITHUB_EVENT_NAME. `useMavenLogger` is pinned false with a comment even though it is the default, because that default is the only reason the `::error` lines reach the log at column 0 where GitHub can render them. Setting it true kills every annotation while leaving the build green. One output-only harness change: every run states its cost in the job summary. The gate now sits on the critical path of every frontend merge, so the real runtime distribution matters; the harness already measured every run and simply never printed it. What it decides is unchanged. Known gap, accepted knowingly: there is no escape hatch. A frontend change that legitimately must add a violation cannot merge until it is fixed or the gate is switched off repository-wide. 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. Scope: CI only. No local git hook; core-web/.husky/ and lint-staged.config.mjs are untouched, so no contributor's local workflow changes. No new dependency. Co-Authored-By: Claude Opus 5 (1M context) --- core-web/pom.xml | 145 +++++++++ core-web/tools/scripts/strict-gate/README.md | 60 +++- .../strict-gate/format.duration.test.mjs | 111 +++++++ .../tools/scripts/strict-gate/lib/format.mjs | 25 +- .../contracts/ci-hooks.md | 78 +++++ specs/37536-wire-strict-gate-ci/data-model.md | 106 +++++++ specs/37536-wire-strict-gate-ci/spec.md | 280 ++++++++++++++++++ 7 files changed, 798 insertions(+), 7 deletions(-) create mode 100644 core-web/tools/scripts/strict-gate/format.duration.test.mjs create mode 100644 specs/37536-wire-strict-gate-ci/contracts/ci-hooks.md create mode 100644 specs/37536-wire-strict-gate-ci/data-model.md create mode 100644 specs/37536-wire-strict-gate-ci/spec.md diff --git a/core-web/pom.xml b/core-web/pom.xml index b64a9dae9050..d8cd31dddb21 100644 --- a/core-web/pom.xml +++ b/core-web/pom.xml @@ -23,6 +23,22 @@ false false true + + true origin/main --base=${git.origin.branch} --head=HEAD --branch=${git.origin.branch} @@ -431,6 +447,135 @@ false + + + + org.codehaus.mojo + exec-maven-plugin + + ${node.install.dir}/pnpm + ${project.basedir} + + ${node.install.dir}:${env.PATH} + + + + + + strict-gate + + exec + + generate-resources + + ${skip.strict.gate} + + + 0 + + + false + + 180000 + + exec + node + tools/scripts/strict-gate/run.mjs + --base=${git.origin.branch} + --flags=strict + --granularity=line + --scope=core-web + --format=github + + + + + + + + + + + + + strict-gate-pull-request + + + env.GITHUB_EVENT_NAME + pull_request + + + + false + + + + + + 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..9fde6b45dc88 100644 --- a/core-web/tools/scripts/strict-gate/README.md +++ b/core-web/tools/scripts/strict-gate/README.md @@ -67,18 +67,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..146e793ced9c --- /dev/null +++ b/core-web/tools/scripts/strict-gate/format.duration.test.mjs @@ -0,0 +1,111 @@ +/** + * The job summary must state how long the run took (issue #37536, FR-025). + * + * Why this is worth pinning: shipping the gate non-blocking is justified entirely by the promise + * to measure its real cost on real pull requests and revisit the blocking decision with data + * (SC-005). The harness already measures every run — `durationMs.total` — and simply never + * printed it, so the only way to honour that promise was to time runs by hand from CI logs. That + * is the class of post-merge chore that does not get done, and the decision then gets retaken + * with no more information than before. + * + * 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 the + * blocking decision turns on. + */ +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 = [] }) => + 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' +}; + +test('the clean summary states the elapsed time', () => { + const report = reportWith({ + durationMs: { total: 7088, typescript: 6900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])] + }); + + const summary = formatMarkdown(report); + + assert.equal(report.exitCode, 0, 'guard: this fixture must be the clean case'); + assert.match( + summary, + /7\.1\s*s/, + 'a passing run must still report its duration — the tail is what the blocking decision turns on' + ); +}); + +test('the findings summary states the elapsed time', () => { + const report = reportWith({ + findings: [finding], + durationMs: { total: 12400, typescript: 12100, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/dot-utils.ts'])] + }); + + const summary = formatMarkdown(report); + + assert.equal(report.exitCode, 1, 'guard: this fixture must be the findings case'); + assert.match(summary, /12\.4\s*s/, 'the findings summary must report its duration too'); +}); + +test('the duration is paired with the size of the diff that produced it', () => { + 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']) + ] + }); + + const summary = formatMarkdown(report); + + // 3 files across 2 project configs. A duration without the diff size it came from is not + // comparable across pull requests, which makes it useless for the measurement SC-005 wants. + assert.match(summary, /\b3\b[^|\n]*file/i, 'the summary must state how many files were checked'); +}); + +test('a sub-second run is not reported as 0s', () => { + // The no-op case — a pull request touching no frontend file — costs ~0.3s. Rounding that to + // "0s" would make the cheapest and most common case invisible in the evidence. + const report = reportWith({ durationMs: { total: 331, typescript: 0, templateAware: 0 } }); + + const summary = formatMarkdown(report); + + assert.doesNotMatch(summary, /\b0\.0\s*s\b/, 'sub-second runs must not round to zero'); + assert.match(summary, /0\.3\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..5a46a487b459 100644 --- a/core-web/tools/scripts/strict-gate/lib/format.mjs +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -114,6 +114,25 @@ export function formatGithub(report) { .join('\n'); } +/** + * What the run cost, paired with the diff size that produced it. + * + * Emitted on every run, passing ones included. Shipping this gate non-blocking is justified + * entirely by the promise to measure its real cost on real pull requests and revisit the blocking + * decision with data; a duration printed only when there are findings would sample the fast and + * slow cases unevenly, and it is the tail that the decision turns on. 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) { + const seconds = (report.durationMs.total / 1000).toFixed(1); + const files = report.targets.reduce((n, t) => n + t.files.length, 0); + return `_Checked ${files} file(s) across ${report.targets.length} project config(s) in ${seconds}s._`; +} + /** Markdown for the job summary — what a human opening the run sees first. */ export function formatMarkdown(report) { const total = ignoredCount(report); @@ -122,7 +141,9 @@ export function formatMarkdown(report) { '## ✅ strict-gate: pass', '', `No new strict-mode violations. ${total} pre-existing or dependency diagnostic(s) ignored, ` + - `across ${report.targets.length} project config(s).` + `across ${report.targets.length} project config(s).`, + '', + costLine(report) ].join('\n'); } @@ -135,6 +156,8 @@ 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), + '', '| 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..f3b41d8975bd --- /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 | `report.files.length` | Pairs the cost with what produced it; a duration without it is not comparable across pull requests | + +**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. From db56f5bc817fa7bf7bedfb35b3f41041dc5a4706 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Mon, 14 Sep 2026 17:33:53 -0400 Subject: [PATCH 2/5] fix(core-web): correct the strict-gate cost line and drop a dangling hook reference (#37536) Addresses review on #37545. The cost line counted (config -> file) assignments, not changed files. `selectConfigs` claims a source under EVERY eligible config, so a project whose lib and spec configs both include a file produced two target entries for one changed file, and `targets[].files.length` summed them -- reporting a one-file diff as two. Unmapped files were dropped from the count entirely. Both defeat the only reason that line exists: to be the cost-versus-diff-size evidence for the runtime question. Now counts distinct paths across targets and unmapped. The original test never caught this because it used distinct files across targets. Two new cases pin the two failure modes directly. The job summary also never mentioned unmapped files, so a changed TypeScript file that no project compiles read as a clean pass with nothing said about it -- the edge case the spec names, and a worse one now the gate blocks. The summary now lists them with the reason, in both the passing and the failing branch. Not a failure signal: the difference between "nothing was wrong" and "nothing was looked at". Also removed a comment in core-web/pom.xml pointing at core-web/.husky/pre-push, a file this pull request does not add. It was left behind when the local hook was cut, and would have sent a developer looking for something that was never there. Reconciled two related staleness bugs: the harness README called itself "not production tooling" next to a section saying it is wired and live, and data-model.md documented `report.files.length` as the diff-size source when buildReport exposes no such field. Co-Authored-By: Claude Opus 5 (1M context) --- core-web/pom.xml | 5 +- core-web/tools/scripts/strict-gate/README.md | 4 +- .../strict-gate/format.duration.test.mjs | 77 ++++++++++++++++--- .../tools/scripts/strict-gate/lib/format.mjs | 42 +++++++--- specs/37536-wire-strict-gate-ci/data-model.md | 2 +- 5 files changed, 105 insertions(+), 25 deletions(-) diff --git a/core-web/pom.xml b/core-web/pom.xml index d8cd31dddb21..b3ce67c927f4 100644 --- a/core-web/pom.xml +++ b/core-web/pom.xml @@ -468,8 +468,9 @@ Declared inside the `validate` profile on purpose: without -Pvalidate the execution does not exist at all, so a plain local - ./mvnw never pays for it. Developers are served by the pre-push hook - instead (core-web/.husky/pre-push). + ./mvnw never pays for it. Nothing runs this on a developer machine; + to get the answer before pushing, run the harness yourself (see its + README). --> strict-gate diff --git a/core-web/tools/scripts/strict-gate/README.md b/core-web/tools/scripts/strict-gate/README.md index 9fde6b45dc88..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? diff --git a/core-web/tools/scripts/strict-gate/format.duration.test.mjs b/core-web/tools/scripts/strict-gate/format.duration.test.mjs index 146e793ced9c..7065550158fc 100644 --- a/core-web/tools/scripts/strict-gate/format.duration.test.mjs +++ b/core-web/tools/scripts/strict-gate/format.duration.test.mjs @@ -1,16 +1,19 @@ /** - * The job summary must state how long the run took (issue #37536, FR-025). + * The job summary must state how long the run took, over how much diff (issue #37536, FR-018). * - * Why this is worth pinning: shipping the gate non-blocking is justified entirely by the promise - * to measure its real cost on real pull requests and revisit the blocking decision with data - * (SC-005). The harness already measures every run — `durationMs.total` — and simply never - * printed it, so the only way to honour that promise was to time runs by hand from CI logs. That - * is the class of post-merge chore that does not get done, and the decision then gets retaken - * with no more information than before. + * 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 the - * blocking decision turns on. + * 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. */ import test from 'node:test'; import assert from 'node:assert/strict'; @@ -20,14 +23,14 @@ import { formatMarkdown } from './lib/format.mjs'; const SHA_A = 'a'.repeat(40); const SHA_B = 'b'.repeat(40); -const reportWith = ({ findings = [], durationMs, targets = [] }) => +const reportWith = ({ findings = [], durationMs, targets = [], unmapped = [] }) => buildReport({ base: SHA_A, head: SHA_B, flagSet: 'strict', granularity: 'line', targets, - unmapped: [], + unmapped, findings, discarded: { byOrigin: { dependency: 0, untouched: 0, infrastructure: 0 }, @@ -99,6 +102,56 @@ test('the duration is paired with the size of the diff that produced it', () => assert.match(summary, /\b3\b[^|\n]*file/i, 'the summary must state how many files were checked'); }); +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 SC-005 evidence the cost line + // exists to provide. + 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' } + ] + }); + + const summary = formatMarkdown(report); + + assert.match(summary, /\b1\b[^|\n]*file/i, 'one changed file, claimed twice, is still one file'); + assert.doesNotMatch(summary, /\b2\s*file/i); +}); + +test('a changed file no project claimed is counted, not dropped', () => { + // An unmapped file was changed and NOT examined. Leaving it out of the count understates the + // diff and, worse, hides that something went unchecked behind a passing run. + const report = reportWith({ + durationMs: { total: 4000, typescript: 3900, templateAware: 0 }, + targets: [target('utils', ['libs/utils/src/lib/a.ts'])], + unmapped: [{ path: 'libs/orphan/src/b.ts', reason: 'no configuration includes this file' }] + }); + + const summary = formatMarkdown(report); + + assert.match(summary, /\b2\b[^|\n]*file/i, 'one mapped + one unmapped = two changed files'); +}); + +test('unmapped files are named in the summary, not silently passed', () => { + // 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: [{ path: 'libs/orphan/src/b.ts', reason: 'no configuration includes this file' }] + }); + + 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, /unexamined|not examined|no project/i, 'and it must say it was not checked'); +}); + test('a sub-second run is not reported as 0s', () => { // The no-op case — a pull request touching no frontend file — costs ~0.3s. Rounding that to // "0s" would make the cheapest and most common case invisible in the evidence. diff --git a/core-web/tools/scripts/strict-gate/lib/format.mjs b/core-web/tools/scripts/strict-gate/lib/format.mjs index 5a46a487b459..5ac3c3e8a1b5 100644 --- a/core-web/tools/scripts/strict-gate/lib/format.mjs +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -117,20 +117,42 @@ export function formatGithub(report) { /** * What the run cost, paired with the diff size that produced it. * - * Emitted on every run, passing ones included. Shipping this gate non-blocking is justified - * entirely by the promise to measure its real cost on real pull requests and revisit the blocking - * decision with data; a duration printed only when there are findings would sample the fast and - * slow cases unevenly, and it is the tail that the decision turns on. The diff size travels with - * it because a duration alone is not comparable between a one-file pull request and a forty-file - * one. + * Emitted on every run, passing ones included. 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) { const seconds = (report.durationMs.total / 1000).toFixed(1); - const files = report.targets.reduce((n, t) => n + t.files.length, 0); - return `_Checked ${files} file(s) across ${report.targets.length} project config(s) in ${seconds}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; + return `_Checked ${changed} changed file(s) across ${report.targets.length} project config(s) in ${seconds}s._`; +} + +/** + * 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. */ @@ -143,7 +165,8 @@ export function formatMarkdown(report) { `No new strict-mode violations. ${total} pre-existing or dependency diagnostic(s) ignored, ` + `across ${report.targets.length} project config(s).`, '', - costLine(report) + costLine(report), + ...(unmappedNote(report) ? ['', unmappedNote(report)] : []) ].join('\n'); } @@ -157,6 +180,7 @@ 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), + ...(unmappedNote(report) ? ['', unmappedNote(report)] : []), '', '| File | Line | Code | Message |', '|---|---|---|---|', diff --git a/specs/37536-wire-strict-gate-ci/data-model.md b/specs/37536-wire-strict-gate-ci/data-model.md index f3b41d8975bd..db420144115e 100644 --- a/specs/37536-wire-strict-gate-ci/data-model.md +++ b/specs/37536-wire-strict-gate-ci/data-model.md @@ -84,7 +84,7 @@ nobody can read a run's cost — and now that the gate blocks, that cost is on e | Field | Source | Notes | |---|---|---| | Elapsed time | `report.durationMs.total`, already computed by the harness | Never printed before this feature — FR-018 surfaces it | -| Diff size | `report.files.length` | Pairs the cost with what produced it; a duration without it is not comparable across pull requests | +| 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 From 78980a72eecb330ef29abf6c4b2eb5c31b739839 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 15 Sep 2026 10:26:09 -0400 Subject: [PATCH 3/5] fix(core-web): harden the strict-gate summary and correct three POM comments (#37536) Second round of review on #37545. Nothing here changes the gate's decision logic; it is all output correctness, test strength, and comments that taught mechanisms that do not exist. The test suite was weak in a way worth describing, because the reviewer found it by mutation testing rather than by reading. Four separate mutations of the implementation passed the whole suite: replacing the config count with a distinct-project count, splitting the duration onto its own line (defeating the stated purpose of pairing it with the diff size), removing the blank line that makes the findings table render, and dropping the unmapped note from the findings branch entirely. The common cause was whole-document regexes: formatMarkdown returns a multi-line document, and the unmapped note renders the same number-then-"file" shape the assertions searched for, so an assertion could be satisfied by a line other than the one under test. One assertion -- `doesNotMatch(/\b2\s*file/i)` -- could never fail for the bug it targeted, because the real output reads "2 changed file" and `\s*` does not match " changed ". Assertions are now equality against an extracted cost line, which is how the sibling suites already work. Two real bugs came out of that. The sub-second test asserted a value three rounding steps clear of the boundary; `toFixed(1)` renders anything under 50ms as "0.0s", and the measured no-op path is ~175ms -- a factor of four, reachable on a faster runner. Sub-second runs now render in milliseconds. And `durationMs.total` was unguarded: formatMarkdown is exported and buildReport's default only fires on undefined, so a partial object rendered "NaNs" in the job summary. The cost line also read "Checked N changed file(s)" directly above a note saying some of them were not examined -- two adjacent lines asserting opposite things about the same file. The verb is gone; the count is what it always was. Three POM comments taught mechanisms that are not there: - the skip was described as trunk/nightly having an empty diff. It is by event name. Trunk and nightly also accept workflow_dispatch from a feature branch, nightly disables change detection entirely, and cicd_5-lts.yml runs the frontend suite on release-* pushes -- so "it would be a harmless no-op" is false for cases that are nonetheless skipped. - the timeout was described as failing through successCodes. The watchdog throws directly and never consults them, so adding a success code to "allow timeouts" would do nothing. - the "no required status checks" premise holds today, but the ruleset providing it is an org-level `2026-08-24_incident-response`, and a separate `Default Merge Queue` ruleset does define required checks with enforcement disabled. Whoever re-enables it should see that this design assumed the opposite. Also removed the plugin-level duplicated into the validate profile: verified against `help:effective-pom` that the base entry already supplies executable, workingDirectory, PATH and NODE_OPTIONS -- the copy omitted NODE_OPTIONS and forked the PATH comment, so a future edit to the base would have silently missed it. Co-Authored-By: Claude Opus 5 (1M context) --- core-web/pom.xml | 43 +++-- .../strict-gate/format.duration.test.mjs | 151 +++++++++++++----- .../tools/scripts/strict-gate/lib/format.mjs | 40 +++-- 3 files changed, 173 insertions(+), 61 deletions(-) diff --git a/core-web/pom.xml b/core-web/pom.xml index b3ce67c927f4..8390667a2010 100644 --- a/core-web/pom.xml +++ b/core-web/pom.xml @@ -27,6 +27,12 @@ The diff-scoped strict typecheck gate (issue #37536). Default ON-skip; the two activation profiles below flip it to false, on pull_request and on merge_group. + Its own property, NOT ${skip.validate}. `-Pvalidate -Dskip.validate=true` turns off + lint-test and format-test and leaves this running, by design: a blocking gate that the + documented way to silence its two neighbours also silences is not much of a gate. The + flip side is that a plain local `./mvnw -Pvalidate` runs lint and prettier but not this, + unlike its neighbours. + Both are needed, for different reasons. The pull-request run is where inline annotations render on the diff, so it is what an author actually reads. The merge-queue run is what ENFORCES: this repository declares no required status checks @@ -35,8 +41,20 @@ stop a merge. A job that fails in the merge queue ejects the pull request, and that is the only place a failing gate actually blocks. - Trunk and nightly runs stay skipped: HEAD equals origin/main there, so the diff is - empty and the run could only ever pass. Nothing to report, nothing to enforce. + Every other event leaves this at true. On a trunk push or a nightly that is also what + you would want (HEAD is origin/main, so there would be nothing to report), but the + skip is by EVENT NAME, not by diff emptiness. Do not "fix the coverage gap" by adding a + `push` profile on the belief it would be a harmless no-op: cicd_3-trunk.yml and + cicd_4-nightly.yml also accept workflow_dispatch (HEAD != origin/main from a feature + branch), cicd_4-nightly.yml disables change detection so the frontend suite runs every + night, and cicd_5-lts.yml runs it on release-* pushes, where diffing against origin/main + would be meaningless. + + One caveat on the "no required status checks" claim above: it holds today, but the + ruleset providing it is an org-level one named `2026-08-24_incident-response`. A separate + `Default Merge Queue` ruleset DOES define required_status_checks and is currently + `enforcement: disabled`. Whoever re-enables it should know this design assumed the + opposite. --> true origin/main @@ -452,13 +470,13 @@ org.codehaus.mojo exec-maven-plugin - - ${node.install.dir}/pnpm - ${project.basedir} - - ${node.install.dir}:${env.PATH} - - + 180000 diff --git a/core-web/tools/scripts/strict-gate/format.duration.test.mjs b/core-web/tools/scripts/strict-gate/format.duration.test.mjs index 7065550158fc..159be535fb82 100644 --- a/core-web/tools/scripts/strict-gate/format.duration.test.mjs +++ b/core-web/tools/scripts/strict-gate/format.duration.test.mjs @@ -14,6 +14,13 @@ * 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'; @@ -57,36 +64,46 @@ const finding = { layer: 'source' }; -test('the clean summary states the elapsed time', () => { +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'])] }); - const summary = formatMarkdown(report); - assert.equal(report.exitCode, 0, 'guard: this fixture must be the clean case'); - assert.match( - summary, - /7\.1\s*s/, - 'a passing run must still report its duration — the tail is what the blocking decision turns on' + assert.equal( + costLineOf(formatMarkdown(report)), + '_1 changed file(s) across 1 project config(s), 7.1s._' ); }); -test('the findings summary states the elapsed time', () => { +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'])] }); - const summary = formatMarkdown(report); - assert.equal(report.exitCode, 1, 'guard: this fixture must be the findings case'); - assert.match(summary, /12\.4\s*s/, 'the findings summary must report its duration too'); + assert.equal( + costLineOf(formatMarkdown(report)), + '_1 changed file(s) across 1 project config(s), 12.4s._' + ); }); -test('the duration is paired with the size of the diff that produced it', () => { +test('files are counted across every target', () => { const report = reportWith({ durationMs: { total: 9000, typescript: 8800, templateAware: 0 }, targets: [ @@ -95,18 +112,16 @@ test('the duration is paired with the size of the diff that produced it', () => ] }); - const summary = formatMarkdown(report); - - // 3 files across 2 project configs. A duration without the diff size it came from is not - // comparable across pull requests, which makes it useless for the measurement SC-005 wants. - assert.match(summary, /\b3\b[^|\n]*file/i, 'the summary must state how many files were checked'); + 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 SC-005 evidence the cost line - // exists to provide. + // 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 }, @@ -116,49 +131,109 @@ test('a file claimed by two configs of one project counts once', () => { ] }); - const summary = formatMarkdown(report); - - assert.match(summary, /\b1\b[^|\n]*file/i, 'one changed file, claimed twice, is still one file'); - assert.doesNotMatch(summary, /\b2\s*file/i); + 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 of the count understates the - // diff and, worse, hides that something went unchecked behind a passing run. + // 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: [{ path: 'libs/orphan/src/b.ts', reason: 'no configuration includes this file' }] + unmapped: [unmappedEntry] }); - const summary = formatMarkdown(report); - - assert.match(summary, /\b2\b[^|\n]*file/i, 'one mapped + one unmapped = two changed files'); + 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 in the summary, not silently passed', () => { +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: [{ path: 'libs/orphan/src/b.ts', reason: 'no configuration includes this file' }] + 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, /unexamined|not examined|no project/i, 'and it must say it was not checked'); + assert.match(summary, /not examined/i, 'and it must say it was not checked'); }); -test('a sub-second run is not reported as 0s', () => { - // The no-op case — a pull request touching no frontend file — costs ~0.3s. Rounding that to - // "0s" would make the cheapest and most common case invisible in the evidence. - const report = reportWith({ durationMs: { total: 331, typescript: 0, templateAware: 0 } }); +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.doesNotMatch(summary, /\b0\.0\s*s\b/, 'sub-second runs must not round to zero'); - assert.match(summary, /0\.3\s*s/); + 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 5ac3c3e8a1b5..3b93db223868 100644 --- a/core-web/tools/scripts/strict-gate/lib/format.mjs +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -117,17 +117,29 @@ export function formatGithub(report) { /** * What the run cost, paired with the diff size that produced it. * - * Emitted on every run, passing ones included. 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. + * 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) { - const seconds = (report.durationMs.total / 1000).toFixed(1); + // 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 @@ -137,7 +149,10 @@ function costLine(report) { ...report.targets.flatMap((t) => t.files), ...report.unmapped.map((u) => u.path) ]).size; - return `_Checked ${changed} changed file(s) across ${report.targets.length} project config(s) in ${seconds}s._`; + // 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}._`; } /** @@ -158,15 +173,18 @@ function unmappedNote(report) { /** 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), - ...(unmappedNote(report) ? ['', unmappedNote(report)] : []) + ...(note ? ['', note] : []) ].join('\n'); } @@ -180,7 +198,7 @@ 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), - ...(unmappedNote(report) ? ['', unmappedNote(report)] : []), + ...(note ? ['', note] : []), '', '| File | Line | Code | Message |', '|---|---|---|---|', From 61f3fde2ed4d647f54c5d78e9e92b6338d9b17a3 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 15 Sep 2026 19:11:30 -0400 Subject: [PATCH 4/5] fix(ci): give the strict gate a real merge base (#37536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the finding that would have made the gate unusable, caught in review on #37545 and reproduced in this PR's own CI run (job 104166954128): strict-gate: warning — no merge base between 'origin/main' and head; comparing against the tip of 'origin/main' instead. The cause is structural, not incidental. cicd_comp_test-phase.yml checks out at fetch-depth: 1 and compensates with `git fetch --depth=1`, so both sides of the comparison are truncated, `git merge-base` cannot traverse, and the harness takes its documented base-tip fallback. That fallback is survivable for `nx affected` -- over-including projects for lint is harmless on a lint-clean main -- which is why it went unnoticed while that was the only consumer. It is not survivable for a BLOCKING strict check, because main is deliberately not strict-clean while #37198 is in flight: every commit main moved ahead of the branch gets attributed to the pull request and strict-checked, so authors would be rejected for violations they never wrote. The harness's own comment on that code path records it producing 50 findings, essentially none of them the branch's own. The spike's "0 false positives" figure was measured with a correct merge base and does not transfer. Fix: resolve the base from the event payload (pull_request.base.sha, or merge_group.base_sha), fetch that one commit shallow, and hand it to the gate through a new `strict.gate.base` property. With the base already AT the divergence point, the base-tip fallback stops being a degradation and becomes exactly right -- no deepening, no extra fetch cost. `strict.gate.base` is deliberately its own property rather than a change to ${git.origin.branch}: lint-test and format-test tolerate an approximate base, this gate does not, and widening the blast radius to the two neighbouring checks is not something this change should do quietly. It defaults to ${git.origin.branch}, so a local run behaves exactly as before. Side effect worth naming: this also settles the hardcoded-origin/main concern raised separately in review. A pull request targeting a branch other than main now compares against its own base rather than against main. If the base commit cannot be fetched the step warns and leaves the gate on origin/main, which is today's behaviour rather than a new failure mode. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/cicd_comp_test-phase.yml | 32 ++++++++++++++++++-- core-web/pom.xml | 35 +++++++++++++++++++++- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cicd_comp_test-phase.yml b/.github/workflows/cicd_comp_test-phase.yml index 9fc4df4e85d6..3bc70280f0b2 100644 --- a/.github/workflows/cicd_comp_test-phase.yml +++ b/.github/workflows/cicd_comp_test-phase.yml @@ -261,9 +261,37 @@ 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 fetch the event's real base commit and hand it to the gate. With the base + # already AT the divergence point, the harness's base-tip fallback stops being + # a degradation and becomes exactly right. It also makes the base branch's name + # irrelevant, so a pull request targeting something other than `main` compares + # against its own base rather than against main. + - 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 + env: + BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} + run: | + git fetch --depth=1 origin main:refs/remotes/origin/main + if [ -n "$BASE_SHA" ]; then + # Not fatal: if the base object cannot be fetched the gate still runs + # against origin/main and warns, which is the behaviour before this step. + if git fetch --depth=1 origin "$BASE_SHA" 2>/dev/null; then + echo "STRICT_GATE_BASE=$BASE_SHA" >> "$GITHUB_ENV" + echo "strict-gate base: $BASE_SHA" + else + echo "::warning::could not fetch base $BASE_SHA; strict-gate will fall back to origin/main" + fi + 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 8390667a2010..fa61e7f911ca 100644 --- a/core-web/pom.xml +++ b/core-web/pom.xml @@ -57,6 +57,22 @@ opposite. --> true + + ${git.origin.branch} origin/main --base=${git.origin.branch} --head=HEAD --branch=${git.origin.branch} @@ -542,7 +558,7 @@ exec node tools/scripts/strict-gate/run.mjs - --base=${git.origin.branch} + --base=${strict.gate.base} --flags=strict --granularity=line --scope=core-web @@ -579,6 +595,23 @@ + + + strict-gate-explicit-base + + + env.STRICT_GATE_BASE + + + + ${env.STRICT_GATE_BASE} + + +