From bff6ece4ce29b57c6daa4a657d99cf884fe5df0f Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Fri, 4 Sep 2026 10:36:21 -0400 Subject: [PATCH 1/8] docs(specs): add spike spec for diff-scoped strict typecheck gate Proposes a 4-hour timeboxed research spike to test whether filtering strict-mode diagnostics to changed files can stop new non-strict debt from landing on main, decoupled from the full workspace migration (PR #37198) that's blocked on QA. --- .../spec.md | 296 ++++++++++++++++++ 1 file changed, 296 insertions(+) create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/spec.md diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/spec.md b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md new file mode 100644 index 00000000000..ee24144d853 --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md @@ -0,0 +1,296 @@ +# Feature Specification: Diff-scoped strict typecheck gate for `core-web` + +**Feature Branch**: `nicobytes/37401-strict-mode-validate-a-diff-scoped-strict-typecheck-gate-to-stop-new-non-strict-code-landing-on-main` + +**Created**: 2026-09-04 + +**Status**: Draft + +**Type**: Spike (time-boxed research — 4h) + +**Related GitHub Issue**: dotCMS/core#37401 + +**Input**: User description: "https://github.com/dotCMS/core/issues/37401 — strict mode: validate a diff-scoped strict typecheck gate to stop new non-strict code landing on main" + +## Problem Statement *(mandatory)* + +The `core-web` workspace is only partly strict: the shared TypeScript baseline turns strict +mode **off**, and 22 of 55 project configs opt back in locally. The workspace-wide migration +(PR #37198, 1455 files) is waiting on full-team QA and is not imminent. + +While it waits, **new non-strict code keeps landing on `main`**. Every sync from `main` into +the migration branch imports fresh type errors that must be fixed by hand, so the branch's +diff grows and the QA target keeps moving. PR #37262 is the concrete, still-reproducible +example: it landed three strict violations in `sdk-create-app` that had to be repaired on the +branch after a merge. + +Per-project opt-in cannot close the gap, and the reason is mechanical, not a matter of will: +the workspace path aliases point at **sources**, not built output, so a project's dependencies +become part of its own compilation program and are checked with **its** flags. Checking the 8 +files of `libs/portlets/dot-locales/portlet` drags in 387 files from six dependency libs. Three +of those libs (`dotcms-models`, `data-access`, `ui`) are imported by 585–1130 files each, so +any opt-in upstream of them drowns in inherited errors. + +**The question this spike answers**: can the gate stop *counting* dependency errors instead of +waiting for them to be fixed? That is, run each project's existing config with strict forced on, +then discard every diagnostic whose file is not part of the pull request's diff. If that works, +`main` stops accumulating strict debt today, decoupled from the migration PR's timeline. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - The gate catches new strict debt (Priority: P1) + +A contributor opens a pull request that adds TypeScript with a strict-mode violation — an +implicit `any`, a possibly-null dereference, an index-signature access. The evaluation harness, +run over that pull request's diff, reports the violation and fails, naming the file, line and +diagnostic code. + +**Why this priority**: This is the whole premise. If diff-scoped filtering cannot surface a +known-real violation on a known-real pull request, the spike ends here with a documented no-go +and nothing else in this spec matters. + +**Independent Test**: Replay PR #37262 (base = its merge-base on `main`, head = its merge +commit) through the harness and confirm the three known violations in `sdk-create-app` are all +reported. Delivers a yes/no answer to the research question on its own. + +**Acceptance Scenarios**: + +1. **Given** PR #37262 replayed at its merge-base and merge commit, **When** the harness runs, + **Then** it reports all three known violations (one in `src/index.ts`, two in + `src/utils/readiness.spec.ts`) and exits non-zero. +2. **Given** at least two further merged pull requests that changed TypeScript inside + non-strict libraries, **When** the harness runs over each, **Then** every reported finding is + recorded and individually judged real or spurious, with the judgement written down. +3. **Given** any run that reports findings, **When** the output is read, **Then** each finding + identifies its file, line and diagnostic code, so a contributor can act on it without + re-running anything. + +--- + +### User Story 2 - The gate does not cry wolf (Priority: P1) + +A contributor opens a pull request that introduces no strict debt — a rename, a test-only +change, a change confined to already-strict code. The harness passes silently and costs them +nothing. + +**Why this priority**: A gate that blocks merges on noise is worse than no gate; it will be +disabled within a week. Precision is what decides whether this can block on day one, so it is +equal in priority to detection. + +**Independent Test**: Replay at least three merged pull requests known to carry no strict debt +and confirm all three pass. Yields the false-positive rate that the day-one blocking decision +turns on. + +**Acceptance Scenarios**: + +1. **Given** at least three merged pull requests that introduced no strict debt, **When** the + harness runs over each, **Then** all three pass with zero findings. +2. **Given** any harness run, **When** it completes, **Then** it reports how many diagnostics + originated in files outside the diff and were therefore discarded — evidencing that the + filter, not luck, is what makes the run pass. +3. **Given** the full sample of replayed pull requests, **When** the results are tallied, + **Then** a false-positive rate is recorded and an explicit **go / no-go for blocking on day + one** is stated, with a named fallback posture if the answer is no-go. + +--- + +### User Story 3 - The operating decisions are settled with measurements (Priority: P2) + +Whoever implements the real gate inherits three choices already made and backed by numbers, +rather than having to re-litigate them: which strictness flags to turn on, whether a finding is +scoped to the whole changed file or only the changed lines, and what the gate costs per pull +request. + +**Why this priority**: Detection and precision decide *whether* to build the gate; these +decide *what shape* it takes. Getting them wrong makes the gate either toothless or so +unadoptable that touching one line of a legacy file becomes a day's work. + +**Independent Test**: Re-run the sample pull requests under each candidate flag set and each +candidate granularity, and confirm the write-up carries a per-option finding count, a wall-clock +measurement, and a single recommendation for each of the three decisions. + +**Acceptance Scenarios**: + +1. **Given** the sample pull requests, **When** they are run under full strict and again under + the narrower null-checks/implicit-any subset, **Then** the finding count for each is recorded + and one flag set is recommended. +2. **Given** the sample pull requests, **When** findings are scoped whole-file and again + line-level, **Then** the cost of each is quantified — how many extra findings whole-file + inherits from untouched legacy code — and one granularity is recommended. +3. **Given** a pull request touching one to three projects, **When** the harness runs, **Then** + wall-clock time is measured and reported against the 2.4s single-project baseline. + +--- + +### User Story 4 - The finding is handed off (Priority: P3) + +The strict-mode effort's owner reads a single write-up on the issue and knows whether to build +the gate, in what shape, and where the work is tracked — without re-deriving anything. + +**Why this priority**: A spike whose result lives only in a throwaway script is a spike that +gets re-run in three months. The write-up is the deliverable that outlives the timebox. + +**Independent Test**: Read the issue after the spike closes and confirm it carries the +recommendation, the measurements behind it, and either a follow-up task link or a documented +reason the approach cannot work. + +**Acceptance Scenarios**: + +1. **Given** the spike is complete, **When** issue #37401 is read, **Then** it carries the + findings, the three decisions with their measurements, and an explicit recommendation. +2. **Given** the recommendation is "build it", **When** the issue is closed, **Then** a + follow-up task exists covering the production gate — the durable script, the continuous + integration hook, and the local pre-commit hook. +3. **Given** the recommendation is "do not build it", **When** the issue is closed, **Then** it + states the specific reason the approach fails, in enough detail that nobody re-opens the same + question blind. + +--- + +### Edge Cases + +- **A pull request changes no TypeScript at all** → the gate is a no-op and passes; it must not + fail, and must not spend meaningful time deciding there is nothing to do. +- **The diff contains deleted or renamed files** → no crash and no phantom failure against a + path that no longer exists at the head commit. +- **The diff touches a shared configuration file** (the workspace TypeScript baseline, or the + workspace task configuration) → these are declared shared inputs, so the affected-project + calculation expands to all 56 projects. The gate must stay scoped to the projects that own + changed files rather than fanning out to the whole workspace; the chosen behavior is recorded + either way. +- **A changed file belongs to a project with no conventional library config** — applications, + `.tsx` projects, framework-specific projects → the gate resolves the correct config or skips + the project **loudly**, never silently. +- **A changed file maps to no project at all** (workspace-root files, tooling scripts) → + explicitly reported as unmapped rather than dropped. +- **A shallow checkout, and the merge-queue context** → the base ref the diff is computed + against may not be present locally and must be fetched before use; the gate must not + mistakenly report "nothing changed" when the base ref is missing. +- **The same file is claimed by more than one project config** (a source file included by both + a library and a spec config) → the finding is reported once, not duplicated per config. + +## Requirements *(mandatory)* + +### Functional Requirements + +- **FR-001**: The evaluation harness MUST determine the set of changed TypeScript source files + for a pull request by comparing its head against its merge-base with the target branch, + including added, copied, modified and renamed files, and excluding deleted ones. +- **FR-002**: The harness MUST map each changed file to the workspace project that owns it, and + MUST report any changed file it cannot map rather than discarding it. +- **FR-003**: The harness MUST type-check each owning project under strict settings **without + requiring any edit to any existing TypeScript configuration file in the repository** — + strictness is imposed at invocation time only. +- **FR-004**: The harness MUST discard every diagnostic whose originating file is not in the + changed-file set, and MUST report the number discarded per run. +- **FR-005**: The harness MUST exit non-zero when at least one diagnostic survives the filter, + and zero otherwise, so it is usable as a gate. +- **FR-006**: The harness MUST report each surviving diagnostic with its file path, line and + diagnostic code. +- **FR-007**: The harness MUST support being run under both candidate flag sets — full strict, + and the narrower null-checks/implicit-any subset — so the two can be compared on the same + sample. +- **FR-008**: The harness MUST support both candidate granularities — every diagnostic in a + changed file, and only diagnostics on changed lines — so the two can be compared on the same + sample. +- **FR-009**: The harness MUST measure and report its own wall-clock runtime per run. +- **FR-010**: The harness MUST scope its work to the projects owning changed files, and MUST + NOT expand to the entire workspace when only a shared configuration file changed. +- **FR-011**: The harness MUST run correctly when the target branch ref is not already present + locally, fetching it if required. +- **FR-012**: The spike MUST produce a written record covering: the per-pull-request results, + the false-positive rate, the discarded-diagnostic counts, the three decisions with their + measurements, and an explicit go/no-go on blocking merges from day one. +- **FR-013**: The spike MUST end with either a follow-up task for the production gate or a + documented reason the approach does not work. + +### Out of Scope + +- Shipping the production gate itself — the durable script, its continuous-integration hook and + its local pre-commit hook. This spike produces the evidence and the decision; the build is the + follow-up task (FR-013). +- Any change to existing TypeScript configuration files, to the workspace build definition, or + to continuous-integration workflow files. +- Migrating any library to strict mode, and any dependency on PR #37198 landing. +- Angular template type checking — the type checker does not read HTML, and template strictness + is a separate gate. +- Catching loose types that *flow in* from non-strict dependencies. While the high-fan-in + libraries stay non-strict, sloppy types cross into strict files unflagged. This is a known, + accepted weakness of the approach, not a defect of it. +- Detecting pre-existing strict debt in files a pull request does not touch. + +### Key Entities + +- **Changed-file set**: the TypeScript files a pull request added, copied, modified or renamed, + relative to its merge-base with the target branch. The unit the whole gate is scoped by. +- **Owning project**: the workspace project whose configuration includes a given changed file; + the unit that type checking is actually invoked on. +- **Diagnostic**: a single type error, carrying an originating file, a line and a code. Either + survives the filter (it is in the changed-file set) or is discarded (it came from a dependency). +- **Sample pull request**: a merged pull request replayed at its merge-base and merge commit, + labelled up-front as carrying strict debt or not, and used as the evidence base for both the + detection and the false-positive claims. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: The harness reports 3 of the 3 known strict violations that PR #37262 introduced + into `sdk-create-app`, and fails on that pull request. +- **SC-002**: Across at least 3 replayed pull requests that carry no strict debt, the harness + produces **zero** findings — a measured false-positive rate of 0 on that sample. +- **SC-003**: Every finding reported across the full sample is individually adjudicated real or + spurious, with the adjudication written down; no finding is left unexplained. +- **SC-004**: For every run, the count of diagnostics discarded as dependency-origin is + reported, and at least one run demonstrates a program dominated by dependency files (on the + order of the measured 387-from-6-libs case) passing because of the filter. +- **SC-005**: A pull request touching 1–3 projects completes in **10 seconds or less** of + wall-clock time. +- **SC-006**: All 7 edge cases listed above are exercised and their observed behavior recorded; + none produces a crash, and none produces a silent skip. +- **SC-007**: Each of the three decisions — flag set, granularity, runtime cost — carries a + single stated recommendation backed by a number measured on the sample. +- **SC-008**: An explicit go / no-go on blocking merges from day one is recorded, with a named + fallback posture if the answer is no-go. +- **SC-009**: The spike is delivered within its 4-hour timebox, or the overrun and its cause are + recorded on the issue. +- **SC-010**: The repository's tracked TypeScript configuration files are byte-identical before + and after the spike. + +## Legacy Considerations *(dotCMS-specific — mandatory)* + +- **Existing behavior touched**: None at runtime. This is developer-tooling research against the + `core-web` frontend workspace; it produces no product behavior change and ships nothing to + users. The area it informs — the frontend build and validation pipeline — already carries + comparable gates for linting and formatting. +- **Backward-compatibility expectations**: Absolute. Nothing in this spike may alter existing + configuration, build definitions or workflows (SC-010). The eventual gate, when built, must not + block pull requests that do not introduce new strict debt (User Story 2). +- **Known related decisions**: The repository already accepts baselining accumulated debt rather + than blocking on it — eight lint-suppression files exist, declared as inputs to the lint task. + A type-checking equivalent would follow an established precedent, not introduce a new one. A + project-scoped typecheck gate already exists on the strict-mode branch but has never landed on + `main`; the plan phase will formally consult `dotCMS/platform-adrs`. + +## Assumptions + +- **Spike scope ends at evidence and a decision.** Issue #37401 describes the deliverable as a + throwaway script plus a write-up, with the production gate handed to a follow-up task. This + spec follows that framing; the durable script, CI hook and pre-commit hook are out of scope + here. +- **Command-line strictness overrides inherited configuration.** This was verified against a + synthetic project before the spike and is treated as a given; FR-003 depends on it, and + confirming it on a real workspace project is the first thing the spike does. +- **The PR #37262 case is still reproducible.** The three violations are reported as still + present on `main`. If they have since been repaired, an equivalent regression case is + substituted and the substitution recorded. +- **Sample pull requests are chosen from recently merged work** touching `core-web` TypeScript, + labelled as debt-carrying or clean *before* the harness is run against them, so the sample is + not selected to fit the result. +- **A sample of six or so pull requests is sufficient** for a 4-hour timebox to support a + go/no-go recommendation. It is not a statistical claim, and the write-up says so. +- **The base ref for diff computation is available or fetchable in every context the gate would + eventually run in.** The existing pipeline already fetches it for the affected-project + calculation, so no workflow change is anticipated. +- **Both candidate granularities are evaluated on the same sample**, so the whole-file adoption + cost is measured rather than argued. From 41ea8df94624978f5be78d7f5e00eac83d953aa1 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Fri, 4 Sep 2026 11:12:06 -0400 Subject: [PATCH 2/8] Extend spec for diff-scoped strict typecheck gate to cover Angular templ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add User Story 4 (P3) for template strictness, testing whether template diagnostics can be forced on and filtered the same way as TypeScript ones in the four apps that still have strictTemplates disabled (TODO #35930) - Add FR-014–018, SC-011–013, edge cases, and assumptions covering the template arm's distinct mechanism (config-based, not CLI-forceable) and its separate go/no-go - Renumber the handoff story to User Story 5 and generalize wording from "TypeScript configuration" to "version-controlled configuration" now that the spike also touches Angular compiler options --- .../spec.md | 204 ++++++++++++++---- 1 file changed, 158 insertions(+), 46 deletions(-) diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/spec.md b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md index ee24144d853..7a97990f552 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/spec.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md @@ -6,11 +6,11 @@ **Status**: Draft -**Type**: Spike (time-boxed research — 4h) +**Type**: Spike (time-boxed research) **Related GitHub Issue**: dotCMS/core#37401 -**Input**: User description: "https://github.com/dotCMS/core/issues/37401 — strict mode: validate a diff-scoped strict typecheck gate to stop new non-strict code landing on main" +**Input**: User description: "https://github.com/dotCMS/core/issues/37401 — strict mode: validate a diff-scoped strict typecheck gate to stop new non-strict code landing on main", extended in conversation to cover Angular template strictness (`angularCompilerOptions`) as a P3 arm of the same spike. ## Problem Statement *(mandatory)* @@ -31,10 +31,18 @@ files of `libs/portlets/dot-locales/portlet` drags in 387 files from six depende of those libs (`dotcms-models`, `data-access`, `ui`) are imported by 585–1130 files each, so any opt-in upstream of them drowns in inherited errors. -**The question this spike answers**: can the gate stop *counting* dependency errors instead of -waiting for them to be fixed? That is, run each project's existing config with strict forced on, -then discard every diagnostic whose file is not part of the pull request's diff. If that works, -`main` stops accumulating strict debt today, decoupled from the migration PR's timeline. +The same shape repeats one layer up, in Angular templates. 30 project configs already declare +`strictTemplates: true`, but the **four applications** — including `dotcms-ui`, the main one — +carry `strictTemplates: false` behind a `TODO(#35930): re-enable once Angular 22 template errors +are fixed per app`. Those apps cannot flip the flag wholesale, so today there is no gate at all +on the layer where the most user-facing code lives. + +**The question this spike answers**: can the gate stop *counting* dependency and pre-existing +errors instead of waiting for them to be fixed? That is, run each project's existing +configuration with strictness forced on, then discard every diagnostic whose file is not part of +the pull request's diff. If that works, `main` stops accumulating strict debt today, decoupled +from the migration PR's timeline — and the same filter may extend to template diagnostics, which +is the secondary question this spike also probes. ## User Scenarios & Testing *(mandatory)* @@ -122,7 +130,47 @@ measurement, and a single recommendation for each of the three decisions. --- -### User Story 4 - The finding is handed off (Priority: P3) +### User Story 4 - Angular template strictness is assessed and decided (Priority: P3) + +A contributor changes an Angular component template in one of the four applications where +template strictness is currently switched off. The harness reports the template's own strict +violations — and only those, not the app's accumulated template debt. + +Reaching this requires a second execution mode. Angular's strictness settings are **not** +TypeScript compiler options and cannot be forced from the command line the way `--strict` can; +they must be supplied through configuration the compiler reads. This story establishes whether +that second mode is worth its cost. + +**Why this priority**: Templates are where the most user-facing code lives and where there is +currently no gate at all, so the upside is real. But it is a distinct mechanism from the +TypeScript arm, its runtime cost is unmeasured and expected to be materially higher, and the +TypeScript arm must stand on its own regardless of how this resolves. It carries its own +go/no-go and may be deferred to the follow-up task without weakening Stories 1–3. + +**Independent Test**: Run the template-aware mode against a merged pull request that changed a +template in one of the four non-strict applications, and confirm it reports that template's +violations while discarding the app's pre-existing template debt. Produces the cost figure and +the recommendation on its own. + +**Acceptance Scenarios**: + +1. **Given** a project whose configuration disables template strictness, **When** the harness + runs in template-aware mode, **Then** template strictness is in force for that run **without + any edit to a version-controlled configuration file**. +2. **Given** a pull request that changed a template in one of the four non-strict applications, + **When** the harness runs, **Then** violations in the changed template are reported and the + application's pre-existing template debt is discarded, with the discarded count reported. +3. **Given** the template-aware mode, **When** it runs on a project that is not an Angular + project, **Then** it falls back to the TypeScript-only mode explicitly, never silently. +4. **Given** the sample pull requests, **When** the template-aware mode runs, **Then** its + wall-clock cost is measured against the TypeScript-only mode, and the additional Angular + strictness options are each assessed for whether they belong in a blocking gate. +5. **Given** the measurements, **When** the write-up is produced, **Then** it states an explicit + go / no-go on including templates in the gate, separate from the Story 2 decision. + +--- + +### User Story 5 - The finding is handed off (Priority: P3) The strict-mode effort's owner reads a single write-up on the issue and knows whether to build the gate, in what shape, and where the work is tracked — without re-deriving anything. @@ -137,10 +185,11 @@ reason the approach cannot work. **Acceptance Scenarios**: 1. **Given** the spike is complete, **When** issue #37401 is read, **Then** it carries the - findings, the three decisions with their measurements, and an explicit recommendation. + findings, every decision with its measurements, and an explicit recommendation. 2. **Given** the recommendation is "build it", **When** the issue is closed, **Then** a follow-up task exists covering the production gate — the durable script, the continuous - integration hook, and the local pre-commit hook. + integration hook, and the local pre-commit hook — and states whether templates are in or out + of its first version. 3. **Given** the recommendation is "do not build it", **When** the issue is closed, **Then** it states the specific reason the approach fails, in enough detail that nobody re-opens the same question blind. @@ -149,8 +198,8 @@ reason the approach cannot work. ### Edge Cases -- **A pull request changes no TypeScript at all** → the gate is a no-op and passes; it must not - fail, and must not spend meaningful time deciding there is nothing to do. +- **A pull request changes no TypeScript and no template at all** → the gate is a no-op and + passes; it must not fail, and must not spend meaningful time deciding there is nothing to do. - **The diff contains deleted or renamed files** → no crash and no phantom failure against a path that no longer exists at the head commit. - **The diff touches a shared configuration file** (the workspace TypeScript baseline, or the @@ -168,18 +217,30 @@ reason the approach cannot work. mistakenly report "nothing changed" when the base ref is missing. - **The same file is claimed by more than one project config** (a source file included by both a library and a spec config) → the finding is reported once, not duplicated per config. +- **A component's template is inline rather than a separate file** → the diagnostic's + originating file is the component source, not a template file, and must still be matched + against the changed-file set correctly. +- **A pull request changes only a template file and no source file** → the owning project is + still identified and checked; a template-only change must not slip through as "no TypeScript + changed". +- **A framework upgrade introduces new diagnostics** → the gate must not start failing pull + requests for diagnostics unrelated to what they changed; the chosen configuration is assessed + for this fragility (see FR-018). ## Requirements *(mandatory)* ### Functional Requirements -- **FR-001**: The evaluation harness MUST determine the set of changed TypeScript source files - for a pull request by comparing its head against its merge-base with the target branch, - including added, copied, modified and renamed files, and excluding deleted ones. +#### Core gate (Stories 1–3) + +- **FR-001**: The evaluation harness MUST determine the set of changed files for a pull request + by comparing its head against its merge-base with the target branch, including added, copied, + modified and renamed files, and excluding deleted ones. The set MUST cover TypeScript sources, + and MUST cover Angular template files when the template-aware mode is in use. - **FR-002**: The harness MUST map each changed file to the workspace project that owns it, and MUST report any changed file it cannot map rather than discarding it. - **FR-003**: The harness MUST type-check each owning project under strict settings **without - requiring any edit to any existing TypeScript configuration file in the repository** — + requiring any edit to any version-controlled configuration file in the repository** — strictness is imposed at invocation time only. - **FR-004**: The harness MUST discard every diagnostic whose originating file is not in the changed-file set, and MUST report the number discarded per run. @@ -198,35 +259,63 @@ reason the approach cannot work. NOT expand to the entire workspace when only a shared configuration file changed. - **FR-011**: The harness MUST run correctly when the target branch ref is not already present locally, fetching it if required. + +#### Template arm (Story 4) + +- **FR-014**: The harness MUST be able to impose Angular's template-strictness settings on a + project whose own configuration disables them, satisfying FR-003 — no version-controlled + configuration file is edited. +- **FR-015**: The harness MUST apply FR-004's filter to template diagnostics on the same terms + as source diagnostics, and MUST report the discarded count separately for them. +- **FR-016**: The harness MUST detect whether a project is an Angular project and select the + template-aware or TypeScript-only mode accordingly, reporting the choice rather than making + it silently. +- **FR-017**: The harness MUST measure the template-aware mode's wall-clock cost separately + from the TypeScript-only mode's, on the same sample. +- **FR-018**: The spike MUST assess each candidate Angular strictness option for whether it + belongs in a blocking gate, explicitly including whether promoting a whole category of + diagnostics to errors makes the gate fragile across framework upgrades. + +#### Deliverable + - **FR-012**: The spike MUST produce a written record covering: the per-pull-request results, - the false-positive rate, the discarded-diagnostic counts, the three decisions with their + the false-positive rate, the discarded-diagnostic counts, every decision with its measurements, and an explicit go/no-go on blocking merges from day one. -- **FR-013**: The spike MUST end with either a follow-up task for the production gate or a - documented reason the approach does not work. +- **FR-013**: The spike MUST end with either a follow-up task for the production gate — stating + whether templates are in scope for its first version — or a documented reason the approach + does not work. ### Out of Scope - Shipping the production gate itself — the durable script, its continuous-integration hook and its local pre-commit hook. This spike produces the evidence and the decision; the build is the follow-up task (FR-013). -- Any change to existing TypeScript configuration files, to the workspace build definition, or - to continuous-integration workflow files. -- Migrating any library to strict mode, and any dependency on PR #37198 landing. -- Angular template type checking — the type checker does not read HTML, and template strictness - is a separate gate. +- Any change to version-controlled configuration files, to the workspace build definition, or to + continuous-integration workflow files. +- Migrating any library or application to strict mode, re-enabling template strictness in the + four applications that disabled it, and any dependency on PR #37198 landing. +- Fixing the accumulated template debt that `TODO(#35930)` refers to. The gate's purpose is to + stop that debt growing, not to pay it down. - Catching loose types that *flow in* from non-strict dependencies. While the high-fan-in - libraries stay non-strict, sloppy types cross into strict files unflagged. This is a known, - accepted weakness of the approach, not a defect of it. + libraries stay non-strict, sloppy types cross into strict files unflagged — and the same + weakness suppresses template findings, since a value typed loosely upstream satisfies a strict + template check. This is a known, accepted limitation of the approach, not a defect of it, and + it means the template arm's signal is weakest in exactly the applications that need it most. - Detecting pre-existing strict debt in files a pull request does not touch. +- Framework settings that are not about strictness (message-identifier formats, emit behavior), + even where they appear alongside strictness settings in existing configuration. ### Key Entities -- **Changed-file set**: the TypeScript files a pull request added, copied, modified or renamed, - relative to its merge-base with the target branch. The unit the whole gate is scoped by. +- **Changed-file set**: the files a pull request added, copied, modified or renamed, relative to + its merge-base with the target branch. The unit the whole gate is scoped by. - **Owning project**: the workspace project whose configuration includes a given changed file; - the unit that type checking is actually invoked on. -- **Diagnostic**: a single type error, carrying an originating file, a line and a code. Either - survives the filter (it is in the changed-file set) or is discarded (it came from a dependency). + the unit that checking is actually invoked on. +- **Diagnostic**: a single reported error, carrying an originating file, a line and a code. + Either survives the filter (its file is in the changed-file set) or is discarded (it came from + a dependency or from untouched code). +- **Execution mode**: TypeScript-only, or template-aware. Determined per project by whether it + is an Angular project, and reported per run. - **Sample pull request**: a merged pull request replayed at its merge-base and merge commit, labelled up-front as carrying strict debt or not, and used as the evidence base for both the detection and the false-positive claims. @@ -241,21 +330,30 @@ reason the approach cannot work. produces **zero** findings — a measured false-positive rate of 0 on that sample. - **SC-003**: Every finding reported across the full sample is individually adjudicated real or spurious, with the adjudication written down; no finding is left unexplained. -- **SC-004**: For every run, the count of diagnostics discarded as dependency-origin is - reported, and at least one run demonstrates a program dominated by dependency files (on the - order of the measured 387-from-6-libs case) passing because of the filter. +- **SC-004**: For every run, the count of diagnostics discarded as dependency-origin or + untouched-code-origin is reported, and at least one run demonstrates a program dominated by + dependency files (on the order of the measured 387-from-6-libs case) passing because of the + filter. - **SC-005**: A pull request touching 1–3 projects completes in **10 seconds or less** of - wall-clock time. -- **SC-006**: All 7 edge cases listed above are exercised and their observed behavior recorded; + wall-clock time in TypeScript-only mode. +- **SC-006**: All 10 edge cases listed above are exercised and their observed behavior recorded; none produces a crash, and none produces a silent skip. -- **SC-007**: Each of the three decisions — flag set, granularity, runtime cost — carries a +- **SC-007**: Each of the three core decisions — flag set, granularity, runtime cost — carries a single stated recommendation backed by a number measured on the sample. - **SC-008**: An explicit go / no-go on blocking merges from day one is recorded, with a named fallback posture if the answer is no-go. -- **SC-009**: The spike is delivered within its 4-hour timebox, or the overrun and its cause are +- **SC-009**: The spike is delivered within its timebox, or the overrun and its cause are recorded on the issue. -- **SC-010**: The repository's tracked TypeScript configuration files are byte-identical before +- **SC-010**: The repository's version-controlled configuration files are byte-identical before and after the spike. +- **SC-011**: Template strictness is demonstrated in force on at least one of the four + applications that currently disable it, with SC-010 still holding. +- **SC-012**: For at least one pull request that changed a template in a non-strict application, + the changed template's violations are reported and the application's pre-existing template + debt is fully discarded, with both counts recorded. +- **SC-013**: The template-aware mode's wall-clock cost is recorded against the TypeScript-only + mode's on the same sample, and an explicit go / no-go on including templates in the gate is + stated — separate from SC-008, so a no-go here does not block the core gate. ## Legacy Considerations *(dotCMS-specific — mandatory)* @@ -270,7 +368,10 @@ reason the approach cannot work. than blocking on it — eight lint-suppression files exist, declared as inputs to the lint task. A type-checking equivalent would follow an established precedent, not introduce a new one. A project-scoped typecheck gate already exists on the strict-mode branch but has never landed on - `main`; the plan phase will formally consult `dotCMS/platform-adrs`. + `main`. Template strictness was deliberately switched off in the four applications during the + framework 22 upgrade, tracked as `TODO(#35930)`; this spike must not disturb that decision, + only measure whether a diff-scoped gate can coexist with it. The plan phase will formally + consult `dotCMS/platform-adrs`. ## Assumptions @@ -278,16 +379,27 @@ reason the approach cannot work. throwaway script plus a write-up, with the production gate handed to a follow-up task. This spec follows that framing; the durable script, CI hook and pre-commit hook are out of scope here. -- **Command-line strictness overrides inherited configuration.** This was verified against a - synthetic project before the spike and is treated as a given; FR-003 depends on it, and - confirming it on a real workspace project is the first thing the spike does. +- **Command-line strictness overrides inherited configuration for TypeScript options.** This was + verified against a synthetic project before the spike and is treated as a given; FR-003's + TypeScript arm depends on it, and confirming it on a real workspace project is the first thing + the spike does. +- **The same is *not* true of Angular's strictness settings.** They are not TypeScript compiler + options and are rejected by the compiler's command-line parser, which accepts only a small + fixed set of non-TypeScript options. FR-014 therefore requires a different mechanism — + supplying the settings through configuration the compiler reads, without editing any + version-controlled file. Two viable approaches are known; choosing between them is plan-phase + work, not spec-phase. +- **The template arm raises the timebox.** The issue's 4 hours cover Stories 1–3. Story 4 is + expected to add roughly 2 hours. If the core arm consumes the original budget, Story 4 is + deferred to the follow-up task with its findings-to-date recorded — it is P3 precisely so this + is possible without weakening the deliverable. - **The PR #37262 case is still reproducible.** The three violations are reported as still present on `main`. If they have since been repaired, an equivalent regression case is substituted and the substitution recorded. -- **Sample pull requests are chosen from recently merged work** touching `core-web` TypeScript, - labelled as debt-carrying or clean *before* the harness is run against them, so the sample is - not selected to fit the result. -- **A sample of six or so pull requests is sufficient** for a 4-hour timebox to support a +- **Sample pull requests are chosen from recently merged work** touching `core-web` TypeScript + and templates, labelled as debt-carrying or clean *before* the harness is run against them, so + the sample is not selected to fit the result. +- **A sample of six or so pull requests is sufficient** for a time-boxed spike to support a go/no-go recommendation. It is not a statistical claim, and the write-up says so. - **The base ref for diff computation is available or fetchable in every context the gate would eventually run in.** The existing pipeline already fetches it for the affected-project From a5d8a8e8fa5680cb074e4dd5f974493172f37ceb Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Mon, 7 Sep 2026 18:55:39 -0400 Subject: [PATCH 3/8] =?UTF-8?q?feat(build):=20diff-scoped=20strict=20typec?= =?UTF-8?q?heck=20gate=20=E2=80=94=20spike=20harness=20and=20findings=20(#?= =?UTF-8?q?37401)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validates that a diff-scoped strict typecheck can block new non-strict TypeScript from landing on main without the dependency libraries being strict first. Result: it can. 217 of 219 diagnostics discarded (99.1%) on a representative portlet; 11 findings across a 5-PR corpus, all 11 adjudicated real, 0 false positives. The harness runs each project's existing configuration with strictness forced in memory — no overlay file, no edit to any version-controlled config — then discards every diagnostic outside the pull request's diff. Adds core-web/tools/scripts/strict-gate/: plain ESM following the publish.mjs convention, no new dependency, not registered as an Nx project, 93 node:test tests. Run them with --test-concurrency=1. Decisions settled with measurements (see findings.md): - granularity: line — whole-file makes an author inherit 83% of what it reports from lines they did not write - flag set: the repo convention (strict + noPropertyAccessFromIndexSignature, noImplicitOverride, noImplicitReturns, noFallthroughCasesInSwitch), which costs one extra finding over the narrow set across the whole corpus - runtime: 8.4-9.4s average, 12s at the tail — SC-005's 10s budget is not met, and is the only thing between this and a blocking gate Recommendation: ship non-blocking first, optimise the dependency-closure cost, then flip. Templates are a no-go for blocking today (2.2x compiler time on dotcms-ui) though the mechanism works. Corrects four premises in the issue, including that TS4111 is not a --strict error at all. Follow-up: #37448 Co-Authored-By: Claude Opus 5 (1M context) --- core-web/tools/scripts/strict-gate/README.md | 75 ++++ .../strict-gate/changed-files.test.mjs | 191 ++++++++ .../strict-gate/config-select.test.mjs | 293 ++++++++++++ .../strict-gate/corpus.acceptance.test.mjs | 303 +++++++++++++ core-web/tools/scripts/strict-gate/corpus.mjs | 162 +++++++ .../tools/scripts/strict-gate/corpus.test.mjs | 133 ++++++ .../tools/scripts/strict-gate/filter.test.mjs | 258 +++++++++++ .../strict-gate/fixtures/make-ng-project.mjs | 129 ++++++ .../strict-gate/fixtures/make-repo.mjs | 106 +++++ .../strict-gate/fixtures/make-workspace.mjs | 121 +++++ .../tools/scripts/strict-gate/hunks.test.mjs | 45 ++ .../scripts/strict-gate/lib/changed-files.mjs | 102 +++++ .../scripts/strict-gate/lib/check-ng.mjs | 95 ++++ .../scripts/strict-gate/lib/check-ts.mjs | 79 ++++ .../scripts/strict-gate/lib/config-select.mjs | 146 ++++++ .../tools/scripts/strict-gate/lib/exec.mjs | 61 +++ .../tools/scripts/strict-gate/lib/filter.mjs | 75 ++++ .../tools/scripts/strict-gate/lib/format.mjs | 150 +++++++ .../tools/scripts/strict-gate/lib/hunks.mjs | 26 ++ .../scripts/strict-gate/lib/mode-select.mjs | 67 +++ .../scripts/strict-gate/lib/project-map.mjs | 63 +++ .../tools/scripts/strict-gate/lib/report.mjs | 62 +++ .../scripts/strict-gate/lib/resolve-tools.mjs | 82 ++++ .../strict-gate/lib/validate-report.mjs | 136 ++++++ .../scripts/strict-gate/mode-select.test.mjs | 56 +++ .../scripts/strict-gate/project-map.test.mjs | 74 +++ core-web/tools/scripts/strict-gate/replay.mjs | 182 ++++++++ .../strict-gate/report.contract.test.mjs | 115 +++++ core-web/tools/scripts/strict-gate/run.mjs | 212 +++++++++ .../strict-gate/strict-override.test.mjs | 201 +++++++++ .../scripts/strict-gate/writeup.check.mjs | 56 +++ .../contracts/cli.md | 84 ++++ .../contracts/report.schema.json | 136 ++++++ .../data-model.md | 134 ++++++ .../findings.md | 420 ++++++++++++++++++ 35 files changed, 4630 insertions(+) create mode 100644 core-web/tools/scripts/strict-gate/README.md create mode 100644 core-web/tools/scripts/strict-gate/changed-files.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/config-select.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/corpus.acceptance.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/corpus.mjs create mode 100644 core-web/tools/scripts/strict-gate/corpus.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/filter.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/fixtures/make-ng-project.mjs create mode 100644 core-web/tools/scripts/strict-gate/fixtures/make-repo.mjs create mode 100644 core-web/tools/scripts/strict-gate/fixtures/make-workspace.mjs create mode 100644 core-web/tools/scripts/strict-gate/hunks.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/changed-files.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/check-ng.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/check-ts.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/config-select.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/exec.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/filter.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/format.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/hunks.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/mode-select.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/project-map.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/report.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs create mode 100644 core-web/tools/scripts/strict-gate/lib/validate-report.mjs create mode 100644 core-web/tools/scripts/strict-gate/mode-select.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/project-map.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/replay.mjs create mode 100644 core-web/tools/scripts/strict-gate/report.contract.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/run.mjs create mode 100644 core-web/tools/scripts/strict-gate/strict-override.test.mjs create mode 100644 core-web/tools/scripts/strict-gate/writeup.check.mjs create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/contracts/report.schema.json create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/data-model.md create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/findings.md diff --git a/core-web/tools/scripts/strict-gate/README.md b/core-web/tools/scripts/strict-gate/README.md new file mode 100644 index 00000000000..d213ea27422 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/README.md @@ -0,0 +1,75 @@ +# strict-gate — spike harness (issue #37401) + +**This is spike output, not production tooling.** 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? + +It runs each project's existing configuration with strictness forced on, then discards every +diagnostic whose file is not part of the pull request's diff. If that works, `main` stops +accumulating strict debt today, independently of when the workspace-wide strict PR (#37198) +merges. + +Spec, plan and decisions: `specs/37401-diff-scoped-strict-typecheck-gate/`. + +## Running it + +```bash +cd core-web +nvm use # Node pinned in .nvmrc +node tools/scripts/strict-gate/run.mjs --base origin/main --head HEAD +node --test --test-concurrency=1 'tools/scripts/strict-gate/*.test.mjs' +``` + +Full command contract: `specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md`. + +**Run the tests with `--test-concurrency=1`.** `node --test` runs files in parallel by default, and +each of these spins up TypeScript or Angular programs over the real workspace. Under that pressure +a heavy acceptance case can time out and report a failure that does not reproduce in isolation — +93/93 pass serially in ~120s. Left as a flag rather than papered over, because a suite that fails +intermittently is a suite people stop trusting. + +## Guarantees + +- Writes nothing into the working tree. Version-controlled files stay byte-identical, including + after an interrupted run — strictness is forced in memory, never through a temporary config. +- Adds no dependency. TypeScript, the Angular compiler and Nx come from the workspace; tests use + `node:test`, built into Node. +- Not an Nx project. Registering one would put the harness into the project graph it measures. +- Every child process is invoked with an argument array, never a shell string. Refs and paths + come from pull-request metadata and are untrusted input. + +## Result + +**The mechanism works.** On a representative portlet, 217 of 219 diagnostics are discarded (99.1 %) +and only the 2 belonging to the project itself survive. Across a 5-pull-request corpus the gate +reported 11 findings, **all 11 real**, every one on a line its pull request wrote — 0 false +positives after excluding three module-resolution codes that are never strictness violations. + +**Recommended invocation:** + +```bash +node tools/scripts/strict-gate/run.mjs \ + --base origin/main --head HEAD \ + --flags strict --granularity line --scope core-web --format github +``` + +- `--flags strict` — the repo convention (`strict` + `noPropertyAccessFromIndexSignature`, + `noImplicitOverride`, `noImplicitReturns`, `noFallthroughCasesInSwitch`), the same yardstick as + `tsconfig.base.json` on the strict-mode branch. At line granularity it costs **one** extra + finding over the narrow set across the whole corpus. +- `--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. + +Full measurements, adjudication of every finding, and the go/no-go: +`specs/37401-diff-scoped-strict-typecheck-gate/findings.md` and issue #37401. + +## 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. diff --git a/core-web/tools/scripts/strict-gate/changed-files.test.mjs b/core-web/tools/scripts/strict-gate/changed-files.test.mjs new file mode 100644 index 00000000000..4fffd13a6f1 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/changed-files.test.mjs @@ -0,0 +1,191 @@ +/** + * T009, T010 — changed-file resolution. + * + * Runs entirely against fixture repositories in temp dirs. The harness's contract is that it + * writes nothing; a test that mutated the real tree could not tell a genuine breach of that + * contract from its own residue. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { makeRepo } from './fixtures/make-repo.mjs'; +import { resolveChangedFiles } from './lib/changed-files.mjs'; +import { git } from './lib/exec.mjs'; + +test('includes added, copied, modified and renamed files; excludes deleted ones', async (t) => { + const repo = await makeRepo({ + 'src/keep.ts': 'export const keep = 1;\n', + 'src/gone.ts': 'export const gone = 1;\n', + 'src/move-me.ts': 'export const moved = 1;\n' + }); + t.after(() => repo.cleanup()); + + const base = await repo.revParse(); + await repo.commit( + { + 'src/added.ts': 'export const added = 1;\n', + 'src/keep.ts': 'export const keep = 2;\n', + 'src/gone.ts': null + }, + 'add, modify, delete' + ); + await repo.rename('src/move-me.ts', 'src/moved.ts', 'rename'); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base, head: 'HEAD' }); + const paths = files.map((f) => f.path).sort(); + + assert.deepEqual(paths, ['src/added.ts', 'src/keep.ts', 'src/moved.ts']); + assert.ok(!paths.includes('src/gone.ts'), 'a deleted file has nothing to check'); + assert.ok(!paths.includes('src/move-me.ts'), 'a rename is reported at its new path only'); +}); + +test('classifies each changed file as source or template', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'export const a = 1;\n' }); + t.after(() => repo.cleanup()); + + const base = await repo.revParse(); + await repo.commit( + { + 'src/b.ts': 'export const b = 1;\n', + 'src/b.component.html': 'hi\n', + 'README.md': '# not code\n' + }, + 'mixed' + ); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base, head: 'HEAD' }); + const byPath = Object.fromEntries(files.map((f) => [f.path, f.kind])); + + assert.equal(byPath['src/b.ts'], 'source'); + assert.equal(byPath['src/b.component.html'], 'template'); + assert.equal(byPath['README.md'], undefined, 'files no compiler reads are not changed files'); +}); + +test('records changed line ranges as 1-based inclusive spans', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'const a = 1;\nconst b = 2;\nconst c = 3;\n' }); + t.after(() => repo.cleanup()); + + const base = await repo.revParse(); + await repo.commit({ 'src/a.ts': 'const a = 1;\nconst b = 99;\nconst c = 3;\n' }, 'edit line 2'); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base, head: 'HEAD' }); + assert.deepEqual(files[0].changedLines, [[2, 2]]); +}); + +test('fetches the base ref on a shallow clone instead of reporting no changes', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'export const a = 1;\n' }); + t.after(() => repo.cleanup()); + + const base = (await repo.commit({ 'src/a.ts': 'export const a = 2;\n' }, 'base point')).slice(0, 40); + await repo.commit({ 'src/new.ts': 'export const n = 1;\n' }, 'after base'); + + const shallow = await repo.shallowClone(1); + t.after(() => shallow.cleanup()); + + // The whole failure mode being guarded: a missing base ref must NOT look like an empty diff. + // Reporting "nothing changed" here would make the gate pass every pull request in CI. + const { files } = await resolveChangedFiles({ repoDir: shallow.dir, base, head: 'HEAD' }); + assert.ok(files.length > 0, 'must fetch the base ref, not silently report an empty diff'); +}); + +test('throws rather than passing when the base ref cannot be resolved at all', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'export const a = 1;\n' }); + t.after(() => repo.cleanup()); + + await assert.rejects( + () => resolveChangedFiles({ repoDir: repo.dir, base: 'refs/heads/does-not-exist', head: 'HEAD' }), + /base ref/i, + 'an unresolvable base is a harness failure (exit 2), never a clean run' + ); +}); + +/* ── T053 (US4) — template-only changes ───────────────────────────────────── + * A pull request that edits only a .html file must still be checked. Treating "no TypeScript + * changed" as "nothing to do" would let every template regression through, and the four + * applications where template strictness is switched off are exactly where that matters. + */ + +test('a diff containing only template files still yields changed files', async (t) => { + const repo = await makeRepo({ 'src/a.component.html': 'one\n' }); + t.after(() => repo.cleanup()); + + const base = await repo.revParse(); + await repo.commit({ 'src/a.component.html': 'two\n' }, 'template only'); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base, head: 'HEAD' }); + + assert.equal(files.length, 1); + assert.equal(files[0].kind, 'template'); + assert.deepEqual(files[0].changedLines, [[1, 1]]); +}); + +test('a new template file has every line attributable to its author', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'export const a = 1;\n' }); + t.after(() => repo.cleanup()); + + const base = await repo.revParse(); + await repo.commit({ 'src/new.component.html': '\n\n\n' }, 'added template'); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base, head: 'HEAD' }); + const added = files.find((f) => f.path === 'src/new.component.html'); + + assert.equal(added.status, 'A'); + assert.deepEqual(added.changedLines, [[1, 3]], 'nothing in a new file is inherited debt'); +}); + +/* ── Merge-base semantics ─────────────────────────────────────────────────── + * A pull request's diff is `base...head` (three dots) — everything since the two diverged — not + * `base..head`, which compares the two trees. The difference is invisible while a branch is fresh + * and catastrophic once it is stale: a tree comparison reports every file the BASE moved on as + * changed, so the gate blames the author for violations someone else merged into main. + * + * Measured on this very branch before the fix: 50 findings, essentially none of them its own. + */ + +test('only the branch’s own changes are reported when the base has moved on', async (t) => { + const repo = await makeRepo({ 'src/shared.ts': 'export const shared = 1;\n' }); + t.after(() => repo.cleanup()); + + const divergedAt = await repo.revParse(); + + // The branch writes one file. + await repo.commit({ 'src/mine.ts': 'export const mine = 1;\n' }, 'branch work'); + const branchHead = await repo.revParse(); + + // Meanwhile the base moves on: a NEW file (which a tree diff hides as a deletion, filtered by + // ACMR) and — the case that actually bites — a MODIFIED shared file, which a tree diff reports + // as changed and blames on this branch. + await git(['-C', repo.dir, 'checkout', '-q', '-b', 'base-line', divergedAt]); + await repo.commit( + { + 'src/theirs.ts': 'export const theirs = 1;\n', + 'src/shared.ts': 'export const shared = 999;\n' + }, + 'someone else' + ); + const baseHead = await repo.revParse(); + + const { files } = await resolveChangedFiles({ repoDir: repo.dir, base: baseHead, head: branchHead }); + const paths = files.map((f) => f.path).sort(); + + assert.deepEqual( + paths, + ['src/mine.ts'], + 'src/shared.ts was modified by the BASE; a tree diff blames this branch for it' + ); +}); + +test('the reported base is the merge base, so the numbers are reproducible', async (t) => { + const repo = await makeRepo({ 'src/a.ts': 'export const a = 1;\n' }); + t.after(() => repo.cleanup()); + + const divergedAt = await repo.revParse(); + await repo.commit({ 'src/mine.ts': 'export const mine = 1;\n' }, 'branch work'); + const branchHead = await repo.revParse(); + + await git(['-C', repo.dir, 'checkout', '-q', '-b', 'other', divergedAt]); + await repo.commit({ 'src/theirs.ts': 'export const theirs = 1;\n' }, 'someone else'); + const baseHead = await repo.revParse(); + + const { base } = await resolveChangedFiles({ repoDir: repo.dir, base: baseHead, head: branchHead }); + assert.equal(base, divergedAt, 'the report must cite the point of divergence, not the base tip'); +}); diff --git a/core-web/tools/scripts/strict-gate/config-select.test.mjs b/core-web/tools/scripts/strict-gate/config-select.test.mjs new file mode 100644 index 00000000000..a879f7fe7d5 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/config-select.test.mjs @@ -0,0 +1,293 @@ +/** + * T012 — choosing the configuration that actually includes the changed file. + * + * This is the test that guards the spike's most expensive possible mistake. Two of the five real + * violations in the acceptance case live in a `.spec.ts`, and a third is visible from BOTH the lib + * and spec configurations. A filename-convention heuristic ("lib first") reports zero on that case + * — the harness looks like it works while silently under-reporting, and the spike ships a false + * number. Selection is therefore by resolved file list, never by naming. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { makeWorkspace } from './fixtures/make-workspace.mjs'; +import { selectConfigs } from './lib/config-select.mjs'; + +const project = { + name: 'thing', + root: 'libs/thing', + shape: 'references', + files: { + 'src/index.ts': 'export const a = 1;\n', + 'src/thing.spec.ts': 'export const s = 1;\n' + } +}; + +test('a source file selects the lib configuration', async (t) => { + const ws = await makeWorkspace({ projects: [project] }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'thing', root: 'libs/thing' }, + files: ['libs/thing/src/index.ts'] + }); + + assert.equal(selected.length, 1); + assert.match(selected[0].configPath, /tsconfig\.lib\.json$/); +}); + +test('a spec file selects the spec configuration', async (t) => { + const ws = await makeWorkspace({ projects: [project] }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'thing', root: 'libs/thing' }, + files: ['libs/thing/src/thing.spec.ts'] + }); + + assert.equal(selected.length, 1, 'a lib-first heuristic would return zero configs here'); + assert.match(selected[0].configPath, /tsconfig\.spec\.json$/); +}); + +test('a references-only configuration resolving to zero files is never selected', async (t) => { + const ws = await makeWorkspace({ projects: [project] }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'thing', root: 'libs/thing' }, + files: ['libs/thing/src/index.ts', 'libs/thing/src/thing.spec.ts'] + }); + + for (const target of selected) { + assert.doesNotMatch( + target.configPath, + /libs\/thing\/tsconfig\.json$/, + 'the root config owns no files and must exclude itself with no special-casing' + ); + } +}); + +test('a file claimed by two configurations produces both targets so diagnostics can be deduplicated', async (t) => { + // Real case: src/utils/index.ts in sdk-create-app reports TS7030 under BOTH the lib and the + // spec configuration. Selection must surface both; report assembly deduplicates by + // file/line/code so the finding is counted once. + const shared = { + name: 'shared', + root: 'libs/shared', + shape: 'lib', + files: { 'src/index.ts': 'export const a = 1;\n', 'src/a.spec.ts': "import './index';\n" } + }; + const ws = await makeWorkspace({ projects: [shared] }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'shared', root: 'libs/shared' }, + files: ['libs/shared/src/index.ts', 'libs/shared/src/a.spec.ts'] + }); + + assert.equal(selected.length, 2); + assert.deepEqual( + selected.map((s) => s.configPath.split('/').pop()).sort(), + ['tsconfig.lib.json', 'tsconfig.spec.json'] + ); +}); + +/* ── Template files ───────────────────────────────────────────────────────── + * A tsconfig's resolved file list contains only TypeScript. A template is never in it, so the + * file-list rule that works for sources finds nothing for a .html — and a pull request that + * touches only templates resolves ZERO projects and passes silently. That is the exact failure + * the spec's "template-only change" edge case names, and it is invisible without these tests: + * the run reports PASS with no targets, which reads like "nothing to check". + * + * A template belongs to the component that references it, and Angular convention colocates the + * two. Attaching a template to the config that owns TypeScript in its own directory is cheap and + * correct in practice; resolving templateUrl properly would mean compiling to find out what to + * compile. + */ + +test('a template file selects the config that owns TypeScript in its directory', async (t) => { + const ws = await makeWorkspace({ + projects: [ + { + name: 'ngish', + root: 'libs/ngish', + shape: 'lib', + files: { + 'src/index.ts': 'export const a = 1;\n', + 'src/thing.component.ts': 'export class Thing {}\n', + 'src/thing.component.html': '\n' + } + } + ] + }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'ngish', root: 'libs/ngish' }, + files: ['libs/ngish/src/thing.component.html'] + }); + + assert.equal(selected.length, 1, 'a template-only change must still resolve a config'); + assert.match(selected[0].configPath, /tsconfig\.lib\.json$/); +}); + +test('a template with no sibling TypeScript still resolves to the project’s primary config', async (t) => { + const ws = await makeWorkspace({ + projects: [ + { + name: 'ngish', + root: 'libs/ngish', + shape: 'lib', + files: { + 'src/index.ts': 'export const a = 1;\n', + 'src/templates/orphan.html': '\n' + } + } + ] + }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'ngish', root: 'libs/ngish' }, + files: ['libs/ngish/src/templates/orphan.html'] + }); + + assert.ok(selected.length >= 1, 'never drop a template silently — that reads as "nothing to check"'); +}); + +test('a mixed diff attaches the template alongside its sources', async (t) => { + const ws = await makeWorkspace({ + projects: [ + { + name: 'ngish', + root: 'libs/ngish', + shape: 'lib', + files: { + 'src/index.ts': 'export const a = 1;\n', + 'src/thing.component.ts': 'export class Thing {}\n', + 'src/thing.component.html': '\n' + } + } + ] + }); + t.after(() => ws.cleanup()); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'ngish', root: 'libs/ngish' }, + files: ['libs/ngish/src/thing.component.ts', 'libs/ngish/src/thing.component.html'] + }); + + const lib = selected.find((s) => s.configPath.endsWith('tsconfig.lib.json')); + assert.ok(lib.files.includes('libs/ngish/src/thing.component.html')); + assert.ok(lib.files.includes('libs/ngish/src/thing.component.ts')); +}); + +/* ── Entry-point configs ──────────────────────────────────────────────────── + * `apps/dotcms-ui/tsconfig.app.json` declares `"files": ["src/main.ts", "src/polyfills.ts"]`. + * Its RESOLVED file list is therefore two entries — every component arrives through the import + * graph, not through a glob. The file-list rule never selects it, so the app's own sources and + * templates fall through to `tsconfig.editor.json`, an IDE-only config Nx generates that carries + * no `angularCompilerOptions`. The file still gets checked, which is why this hid: the run looks + * healthy while template strictness is silently unreachable for the largest application. + * + * Selection therefore ranks candidates rather than taking the first that matches. + */ + +test('an entry-point config is preferred over an IDE-only config for its own sources', async (t) => { + const ws = await makeWorkspace({ projects: [{ name: 'app', root: 'apps/app', files: {} }] }); + t.after(() => ws.cleanup()); + + const fs = await import('node:fs/promises'); + const path = await import('node:path'); + const write = (rel, obj) => + fs.writeFile(path.join(ws.dir, 'apps/app', rel), JSON.stringify(obj, null, 4), 'utf8'); + + // An application has an app config and an editor config — not the lib/spec pair the generic + // fixture emits. Remove them so the layout matches apps/dotcms-ui, which is what this covers. + for (const generated of ['tsconfig.lib.json', 'tsconfig.spec.json', 'tsconfig.json']) { + await fs.rm(path.join(ws.dir, 'apps/app', generated), { force: true }); + } + + await fs.mkdir(path.join(ws.dir, 'apps/app/src/feature'), { recursive: true }); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/main.ts'), "import './feature/x.component';\n"); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/feature/x.component.ts'), 'export class X {}\n'); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/feature/x.component.html'), '\n'); + + await write('tsconfig.app.json', { + extends: '../../tsconfig.base.json', + files: ['src/main.ts'], + angularCompilerOptions: { strictTemplates: false } + }); + await write('tsconfig.editor.json', { + extends: '../../tsconfig.base.json', + include: ['src/**/*.ts'] + }); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'app', root: 'apps/app' }, + files: ['apps/app/src/feature/x.component.ts', 'apps/app/src/feature/x.component.html'] + }); + + const chosen = selected.map((s) => s.configPath.split('/').pop()); + assert.ok( + chosen.includes('tsconfig.app.json'), + `expected the app config to be selected; got ${chosen.join(', ')}` + ); + assert.ok( + !chosen.includes('tsconfig.editor.json'), + 'an IDE-only config must never stand in for the build config — it carries no Angular settings' + ); +}); + +test('a template is never attached to a spec config just because a spec file sits beside it', async (t) => { + // Reproduces apps/dotcms-ui exactly: the app config lists only entry points, so it can never + // be found by "owns TypeScript in this directory" — while the spec config CAN, because Angular + // colocates x.component.ts, x.component.html and x.component.spec.ts. The spec config carries + // no angularCompilerOptions, so the template silently goes unchecked while the run reports a + // target and a PASS. Alphabetical candidate order hid this in a lib-shaped fixture. + const ws = await makeWorkspace({ projects: [{ name: 'app', root: 'apps/app', files: {} }] }); + t.after(() => ws.cleanup()); + + const fs = await import('node:fs/promises'); + const path = await import('node:path'); + for (const generated of ['tsconfig.lib.json', 'tsconfig.json']) { + await fs.rm(path.join(ws.dir, 'apps/app', generated), { force: true }); + } + await fs.mkdir(path.join(ws.dir, 'apps/app/src/feature'), { recursive: true }); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/main.ts'), "import './feature/x.component';\n"); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/feature/x.component.ts'), 'export class X {}\n'); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/feature/x.component.spec.ts'), "import './x.component';\n"); + await fs.writeFile(path.join(ws.dir, 'apps/app/src/feature/x.component.html'), '\n'); + await fs.writeFile( + path.join(ws.dir, 'apps/app/tsconfig.app.json'), + JSON.stringify({ + extends: '../../tsconfig.base.json', + files: ['src/main.ts'], + angularCompilerOptions: { strictTemplates: false } + }) + ); + await fs.writeFile( + path.join(ws.dir, 'apps/app/tsconfig.spec.json'), + JSON.stringify({ extends: '../../tsconfig.base.json', include: ['src/**/*.spec.ts'] }) + ); + + const selected = await selectConfigs({ + workspaceDir: ws.dir, + project: { name: 'app', root: 'apps/app' }, + files: ['apps/app/src/feature/x.component.html'] + }); + + assert.equal(selected.length, 1); + assert.match( + selected[0].configPath, + /tsconfig\.app\.json$/, + `a template belongs to the build config, not the spec config; got ${selected[0].configPath}` + ); +}); diff --git a/core-web/tools/scripts/strict-gate/corpus.acceptance.test.mjs b/core-web/tools/scripts/strict-gate/corpus.acceptance.test.mjs new file mode 100644 index 00000000000..e3cd47b4104 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/corpus.acceptance.test.mjs @@ -0,0 +1,303 @@ +/** + * T016 — SC-001, the primary acceptance case, replayed against real history. + * + * ── Correction to the issue's framing, verified before this test was written ── + * Issue #37401 names "PR #37262" with "3 strict errors: src/index.ts (TS4111) and + * src/utils/readiness.spec.ts x2 (TS2345)". Three things about that are wrong: + * + * 1. #37262 is an ISSUE, not a pull request. The pull request that merged the work is #37264, + * merge commit 788795e915. + * 2. TS4111 is NOT a `--strict` error. `noPropertyAccessFromIndexSignature` is not among the + * flags `--strict` enables (verified against ts.optionDeclarations). It only appears under + * the repo's own strict convention, which the 22 opted-in projects all declare. + * 3. The real count under that convention is FIVE, not three — and there are two TS4111, not one. + * + * Under bare `--strict` the case yields 2 findings; under the repo convention, 5. That gap is + * itself a spike result: it lands the flag-set decision (FR-007) with evidence. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import path from 'node:path'; +import { workspaceRoot } from './lib/resolve-tools.mjs'; +import { checkTypeScript } from './lib/check-ts.mjs'; +import { CORPUS, TEMPLATE_CASES } from './corpus.mjs'; +import { INFRASTRUCTURE_CODES } from './lib/filter.mjs'; +import { resolveChangedFiles } from './lib/changed-files.mjs'; +import { runGate } from './run.mjs'; +import { resolveMergeRange } from './replay.mjs'; + +const repoRoot = path.resolve(workspaceRoot, '..'); + + +/** + * One gate run per (pull request, flag set, granularity), shared across every test in this file. + * Without it the suite replays the whole corpus once per assertion — minutes of wall clock spent + * recomputing identical results, which makes people stop running it. + */ +const runCache = new Map(); +async function gateRunTemplates(pr, flagSet = 'strict', granularity = 'line') { + return gateRun(pr, flagSet, granularity, true); +} + +async function gateRun(pr, flagSet = 'strict', granularity = 'line', templates = false) { + const key = `${pr}|${flagSet}|${granularity}|${templates}`; + if (!runCache.has(key)) { + runCache.set( + key, + (async () => { + const { base, head } = await resolveMergeRange({ repoDir: repoRoot, pr }); + return runGate({ repoDir: repoRoot, base, head, flagSet, granularity, templates }); + })() + ); + } + return runCache.get(key); +} + +/** PR #37264 — "fix(create-app): design contracts for local Docker start failure..." (#37262). */ +const ACCEPTANCE_PR = 37264; + +/** Verified with `tsc` against the merged tree before this test existed. */ +const EXPECTED_UNDER_REPO_STRICT = [ + { file: 'core-web/libs/sdk/create-app/src/index.ts', line: 294, code: 'TS4111' }, + { file: 'core-web/libs/sdk/create-app/src/index.ts', line: 515, code: 'TS4111' }, + { file: 'core-web/libs/sdk/create-app/src/utils/index.ts', line: 41, code: 'TS7030' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 263, code: 'TS2345' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 271, code: 'TS2345' } +]; + +test('SC-001: the acceptance pull request is flagged, with every known violation reported', async () => { + const { base, head } = await resolveMergeRange({ repoDir: repoRoot, pr: ACCEPTANCE_PR }); + + const report = await runGate({ + repoDir: repoRoot, + base, + head, + flagSet: 'strict', + granularity: 'file' + }); + + assert.notEqual(report.exitCode, 0, 'a pull request that landed strict debt must fail the gate'); + + const actual = report.findings.map((f) => `${f.file}:${f.line}:${f.code}`).sort(); + const expected = EXPECTED_UNDER_REPO_STRICT.map((f) => `${f.file}:${f.line}:${f.code}`).sort(); + assert.deepEqual(actual, expected); +}); + +test('SC-001: the spec-file violations are found, which a lib-first heuristic would miss', async () => { + const report = await gateRun(ACCEPTANCE_PR, 'strict', 'file'); + + const specFindings = report.findings.filter((f) => f.file.endsWith('readiness.spec.ts')); + assert.equal(specFindings.length, 2, 'both TS2345 in the spec file must be reported'); +}); + +test('the acceptance case leaks nothing: every finding is in a changed file', async () => { + const report = await gateRun(ACCEPTANCE_PR, 'strict', 'file'); + + for (const finding of report.findings) assert.equal(finding.origin, 'changed'); + assert.equal(report.unmapped.length, 0, 'every changed file must map to a project'); +}); + +test('SC-004: a dependency-heavy project passes BECAUSE of the filter', async () => { + // sdk-create-app is a leaf package — its program pulls in no workspace sources, so it discards + // nothing and cannot evidence SC-004. The claim needs a project shaped like the one the issue + // measured: dot-locales/portlet checks 8 files of its own and 387 from six dependency libs. + const configPath = path.join(workspaceRoot, 'libs/portlets/dot-locales/portlet/tsconfig.lib.json'); + const { diagnostics } = await checkTypeScript({ configPath, flagSet: 'strict' }); + + const own = diagnostics.filter((d) => d.file.includes('/libs/portlets/dot-locales/')); + const fromDependencies = diagnostics.length - own.length; + + assert.ok(diagnostics.length > 0, 'the fixture premise: this program does report errors'); + assert.ok( + fromDependencies > own.length * 10, + `expected dependency errors to dominate; got ${fromDependencies} vs ${own.length} own` + ); +}); + +test('the narrow flag set under-reports this case — the flag-set decision, measured', async () => { + const { base, head } = await resolveMergeRange({ repoDir: repoRoot, pr: ACCEPTANCE_PR }); + const narrow = await runGate({ repoDir: repoRoot, base, head, flagSet: 'null-checks', granularity: 'file' }); + + assert.ok( + narrow.findings.length < EXPECTED_UNDER_REPO_STRICT.length, + 'null-checks is expected to miss the index-signature and code-path violations' + ); +}); + +/* ── T030 (US2) — SC-002: the gate does not cry wolf ──────────────────────── + * Structural reality of this workspace, measured across 42 recent frontend pull requests: + * only 2 touch exclusively projects that already meet the gate's bar, 1 touches only + * strict-without-the-extras projects, and 39 (93%) touch at least one non-strict project. + * The clean cases are therefore few by nature, not by cherry-picking — which is itself the + * strongest argument for a diff-scoped filter, since waiting for opt-in covers 7% of pull requests. + */ + +test('SC-002 as originally specified is REFUTED, and the refutation is the finding', async () => { + // The spec asked for clean pull requests to produce zero findings. Both pre-registered clean + // cases produce findings, and adjudication showed every one is REAL (findings.md §3). + // + // The pre-registration rule assumed "declares strict: true" implies "is strict-clean". It does + // not: the typecheck target exists on 3 of 57 projects, so a project can carry the strictest + // configuration in the workspace and accumulate errors indefinitely with nothing to notice. + // This test pins the refutation so nobody later "fixes" it back into a false expectation. + const clean = CORPUS.filter((s) => s.expectation === 'clean'); + let casesWithFindings = 0; + + for (const sample of clean) { + const report = await gateRun(sample.pr, 'strict', 'line'); + if (report.findings.length > 0) casesWithFindings += 1; + } + + assert.equal( + casesWithFindings, + clean.length, + 'if a structurally clean pull request ever DOES pass, revisit findings.md §4 — the ' + + 'workspace changed and the rule may now hold' + ); +}); + +test('SC-002 restated: the gate does not cry wolf — no finding is an infrastructure diagnostic', async () => { + // The measurable precision guarantee, and the one that actually matters: a reported finding is + // never a module-resolution or missing-file error dressed up as strict debt. + for (const sample of CORPUS) { + const report = await gateRun(sample.pr, 'strict', 'line'); + + for (const finding of report.findings) { + assert.ok( + !INFRASTRUCTURE_CODES.has(finding.code), + `#${sample.pr} reported ${finding.code} at ${finding.file}:${finding.line} — ` + + 'that is broken tooling, not strict debt' + ); + } + } +}); + +test('every reported finding sits on a line its pull request wrote', async () => { + // The precision property that replaces the refuted SC-002: under line granularity the gate + // may only blame code the author actually touched. This is what keeps it from making whoever + // edits a legacy file inherit that file's history. + for (const sample of CORPUS) { + const report = await gateRun(sample.pr, 'strict', 'line'); + const { files } = await resolveChangedFiles({ + repoDir: repoRoot, + base: report.base, + head: report.head + }); + const spans = new Map(files.map((f) => [f.path, f.changedLines])); + + for (const finding of report.findings) { + const ranges = spans.get(finding.file) ?? []; + assert.ok( + ranges.some(([a, b]) => finding.line >= a && finding.line <= b), + `#${sample.pr}: ${finding.file}:${finding.line} is not on a changed line` + ); + } + } +}); + +test('SC-005: a pull request touching 1-3 projects completes within budget', async () => { + const sample = CORPUS.find((s) => s.expectation === 'clean'); + const report = await gateRun(sample.pr, 'strict', 'line'); + + assert.ok( + report.durationMs.total <= 10_000, + `budget is 10s (ADR-0013 protects frontend merge time); took ${Math.round(report.durationMs.total)}ms` + ); +}); + +/* ── T041 (US3) — runtime, measured against ADR-0013's cost model ─────────── + * SC-005 set a 10s budget to protect what ADR-0013 bought: frontend merge time cut from ~45min + * to ~15min. Measured, the gate does NOT meet it universally — two of five corpus cases overrun. + * These tests pin what was measured so a regression is visible, rather than asserting a budget + * the implementation is known not to hold. The overruns are reported in findings.md §5, not + * hidden behind a test that happens to pick a fast case. + */ + +test('SC-005: small-program projects meet the 10s budget', async () => { + // dot-auth: one project, modest dependency closure. This is the shape the budget was set for. + const report = await gateRun(37405); + assert.ok( + report.durationMs.total <= 10_000, + `expected <=10s for a single small project; took ${Math.round(report.durationMs.total)}ms` + ); +}); + +test('SC-005 is NOT met for projects with a large dependency closure — measured, not assumed', async () => { + // libs/ui and libs/edit-content pull in thousands of dependency source files that are compiled + // only to be discarded. Pinned so that if an optimisation later brings these under budget, this + // test fails and findings.md §5 gets corrected instead of quietly going stale. + for (const pr of [37415, 37372]) { + const report = await gateRun(pr); + assert.ok( + report.durationMs.total > 10_000, + `#${pr} now completes in ${Math.round(report.durationMs.total)}ms — under budget. ` + + 'Update findings.md §5: the runtime finding has changed.' + ); + } +}); + +test('the cost is dominated by diagnostics computed only to be discarded', async () => { + // The optimisation lead, evidenced: the slow cases are exactly the ones discarding thousands. + const slow = await gateRun(37415); + const fast = await gateRun(37405); + const discarded = (r) => r.discarded.byOrigin.dependency + r.discarded.byOrigin.untouched; + + assert.ok(discarded(slow) > discarded(fast) * 2, 'slow runs discard far more than fast ones'); + assert.ok(slow.durationMs.total > fast.durationMs.total); +}); + +test('the narrow flag set is cheaper or comparable, and strictly less sensitive', async () => { + const full = await gateRun(37415, 'strict'); + const narrow = await gateRun(37415, 'null-checks'); + + assert.ok(narrow.findings.length <= full.findings.length, 'the narrow set cannot find more'); + const fullKeys = new Set(full.findings.map((f) => `${f.file}:${f.line}:${f.code}`)); + for (const f of narrow.findings) { + assert.ok(fullKeys.has(`${f.file}:${f.line}:${f.code}`), `${f.code} appeared only under null-checks`); + } +}); + +/* ── T054 (US4) — SC-011 / SC-012 / SC-013 ────────────────────────────────── + * The template arm against real history: an application that switched template strictness OFF, + * and a pull request that changed one of its templates. + */ + +test('SC-011: template strictness is in force on an application that disables it', async () => { + const sample = TEMPLATE_CASES[0]; + const report = await gateRunTemplates(sample.pr); + + const templateTargets = report.targets.filter((t) => t.mode === 'template-aware'); + assert.ok(templateTargets.length > 0, 'at least one project must have run template-aware'); +}); + +test('SC-012: the application’s pre-existing template debt is discarded, and counted', async () => { + const sample = TEMPLATE_CASES[0]; + const report = await gateRunTemplates(sample.pr); + + // dotcms-ui carries TODO(#35930) precisely because it has accumulated template errors. If the + // gate reported them all, it would be unusable; if it counted none, the filter did nothing. + assert.ok( + report.discarded.byLayer.template > 0, + 'the application’s existing template debt must be discarded, not reported' + ); + for (const finding of report.findings) { + assert.equal(finding.origin, 'changed'); + } +}); + +test('SC-013: the template arm’s cost is measured separately from the TypeScript arm’s', async () => { + const sample = TEMPLATE_CASES[0]; + const withTemplates = await gateRunTemplates(sample.pr); + const withoutTemplates = await gateRun(sample.pr); + + assert.ok(withTemplates.durationMs.templateAware > 0, 'the template mode must report its own cost'); + // No budget is asserted: the measurement IS the deliverable. Inventing a threshold here would + // prejudge the go/no-go this case exists to inform. + assert.ok(withoutTemplates.durationMs.templateAware === 0); +}); + +test('a template-only pull request is not treated as "nothing changed"', async () => { + const sample = TEMPLATE_CASES[0]; + const report = await gateRunTemplates(sample.pr); + assert.ok(report.targets.length > 0, 'a template-only diff must still resolve a project to check'); +}); diff --git a/core-web/tools/scripts/strict-gate/corpus.mjs b/core-web/tools/scripts/strict-gate/corpus.mjs new file mode 100644 index 00000000000..79b5c35c494 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/corpus.mjs @@ -0,0 +1,162 @@ +/** + * The replay corpus — the spike's evidence base. + * + * ── Pre-registration rule (fixed before any case was run) ──────────────────── + * A pull request is labelled `clean` if EVERY project it touches already declares the + * convention the gate enforces: `strict` plus noPropertyAccessFromIndexSignature, + * noImplicitOverride, noImplicitReturns and noFallthroughCasesInSwitch. Otherwise `debt`. + * The rule is structural — derivable from tsconfigs and the diff, never from a gate result — + * which is what keeps the sample from being fitted to the outcome. + * + * ── Why the clean set is small ─────────────────────────────────────────────── + * Measured across 42 recent frontend pull requests: 2 touch only full-convention projects, + * 1 touches only strict-without-the-extras projects, and 39 (93%) touch at least one non-strict + * project. Clean cases are rare BY STRUCTURE, not by cherry-picking. SC-002 asked for at least + * three; this workspace contains two. That gap is reported rather than papered over by padding + * the sample with a weak case — see INTERMEDIATE_TIER below. + */ + +/** @typedef {{pr:number, expectation:'clean'|'debt', rationale:string, + * knownFindings: object[] | {file?:object[], line?:object[]}}} SampleCase */ + +/** @type {SampleCase[]} */ +export const CORPUS = [ + { + pr: 37264, + expectation: 'debt', + rationale: + 'sdk-create-app inherits strict:false; five violations confirmed with tsc against the ' + + 'merged tree before this corpus existed', + // Qualified per granularity: two of the five sit on pre-existing lines the pull request + // did not write, so line-level correctly reports three. A flat list would show a mismatch + // under one granularity or the other no matter which numbers it held. + knownFindings: { + file: [ + { file: 'core-web/libs/sdk/create-app/src/index.ts', line: 294, code: 'TS4111' }, + { file: 'core-web/libs/sdk/create-app/src/index.ts', line: 515, code: 'TS4111' }, + { file: 'core-web/libs/sdk/create-app/src/utils/index.ts', line: 41, code: 'TS7030' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 263, code: 'TS2345' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 271, code: 'TS2345' } + ], + line: [ + { file: 'core-web/libs/sdk/create-app/src/index.ts', line: 294, code: 'TS4111' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 263, code: 'TS2345' }, + { file: 'core-web/libs/sdk/create-app/src/utils/readiness.spec.ts', line: 271, code: 'TS2345' } + ] + } + }, + { + pr: 37415, + expectation: 'debt', + rationale: 'touches libs/edit-content, which declares no strict setting', + knownFindings: [] + }, + { + pr: 37372, + expectation: 'debt', + rationale: 'touches dot-content-drive/portlet and libs/ui, neither of which is strict', + knownFindings: [] + }, + { + pr: 37405, + expectation: 'clean', + rationale: 'every changed file is in libs/portlets/dot-auth, which declares the full convention', + knownFindings: [] + }, + { + pr: 37339, + expectation: 'clean', + rationale: 'touches only dotcms-models and libs/portlets/dot-auth; both declare the full convention', + knownFindings: [] + } +]; + +/** + * Deliberately NOT in CORPUS. libs/sdk/angular declares `strict: true` without the four extra + * flags, so a finding there would be real debt the project never measured — not a false positive. + * Including it would contaminate the denominator of the very rate the blocking decision rests on. + * Reported separately in findings.md instead. + */ +export const INTERMEDIATE_TIER = [ + { + pr: 37086, + rationale: 'libs/sdk/angular: strict:true but none of the four extra flags', + note: 'any finding here is genuine unmeasured debt, not gate noise' + } +]; + +/** + * Template-arm cases. Kept OUT of CORPUS on purpose: the template arm has its own go/no-go + * (SC-013), and mixing its results into the false-positive denominator would make one number + * stand for two very different risks. + * + * dotcms-ui carries `strictTemplates: false` behind + * `TODO(#35930): re-enable strictTemplates once Angular 22 template errors are fixed per app`, + * which is precisely the situation the template arm exists to test: can a diff-scoped gate + * coexist with an application-wide opt-out? + */ +export const TEMPLATE_CASES = [ + { + pr: 37248, + rationale: 'one template file in apps/dotcms-ui, where template strictness is switched off', + expectation: 'unknown — the cost and the finding count are what this case measures' + } +]; + +const identity = (f) => `${f.file}:${f.line}:${f.code}`; + +/** + * @param {SampleCase} sample + * @param {{findings: object[]}} report + */ +export function adjudicate(sample, report, granularity = 'line') { + const found = report.findings ?? []; + const foundKeys = new Set(found.map(identity)); + + // `knownFindings` is either a flat list (granularity-independent) or keyed by granularity. + const raw = sample.knownFindings ?? []; + const known = Array.isArray(raw) ? raw : (raw[granularity] ?? []); + const knownKeys = new Set(known.map(identity)); + + const missed = known.filter((f) => !foundKeys.has(identity(f))); + const unexpected = found.filter((f) => !knownKeys.has(identity(f))); + + if (sample.expectation === 'clean') { + return { + matchedExpectation: found.length === 0, + falsePositives: found, + unexpected, + missed: [] + }; + } + + return { + matchedExpectation: knownKeys.size > 0 ? missed.length === 0 : found.length > 0, + falsePositives: [], + unexpected, + missed + }; +} + +/** @param {{sample: SampleCase, verdict: ReturnType}[]} results */ +export function summarize(results) { + const clean = results.filter((r) => r.sample.expectation === 'clean'); + const withFindings = clean.filter((r) => (r.verdict.falsePositives?.length ?? 0) > 0); + + return { + sampleSize: results.length, + cleanCases: clean.length, + cleanCasesWithFindings: withFindings.length, + falsePositiveRate: clean.length === 0 ? null : withFindings.length / clean.length, + debtCases: results.length - clean.length, + debtCasesDetected: results.filter( + (r) => r.sample.expectation === 'debt' && r.verdict.matchedExpectation + ).length, + unexpectedFindings: results.reduce((n, r) => n + (r.verdict.unexpected?.length ?? 0), 0), + // Stated in the data, not only in prose: a rate quoted without its denominator gets + // repeated as if it were a statistical claim. This sample cannot support one. + caveat: + `Measured on ${results.length} replayed pull request(s), ${clean.length} of them ` + + `pre-registered clean. This is not a statistical claim.` + }; +} diff --git a/core-web/tools/scripts/strict-gate/corpus.test.mjs b/core-web/tools/scripts/strict-gate/corpus.test.mjs new file mode 100644 index 00000000000..524ebe932cb --- /dev/null +++ b/core-web/tools/scripts/strict-gate/corpus.test.mjs @@ -0,0 +1,133 @@ +/** + * T029 — integrity of the replay corpus. + * + * The corpus is the spike's evidence base, so its one methodological rule is that every case's + * expectation is fixed BEFORE the gate runs against it. Without that, the sample gets fitted to + * the result and the false-positive rate measures nothing. These tests enforce the rule in code + * rather than trusting whoever edits corpus.mjs to remember it. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { CORPUS, adjudicate, summarize } from './corpus.mjs'; + +test('every case carries a pre-registered expectation and a stated reason', () => { + assert.ok(CORPUS.length >= 5, 'the corpus needs enough cases to say anything'); + for (const sample of CORPUS) { + assert.ok(Number.isInteger(sample.pr), 'each case identifies a pull request'); + assert.ok(['clean', 'debt'].includes(sample.expectation), `#${sample.pr}: bad expectation`); + assert.match(sample.rationale ?? '', /\S/, `#${sample.pr}: the label must justify itself`); + // The rationale must be structural — derivable before the gate runs — not a gate result. + assert.doesNotMatch( + sample.rationale, + /gate (found|reported)|after running/i, + `#${sample.pr}: the label was derived from a run, which defeats pre-registration` + ); + } +}); + +test('the corpus holds both clean and debt cases', () => { + const clean = CORPUS.filter((s) => s.expectation === 'clean'); + const debt = CORPUS.filter((s) => s.expectation === 'debt'); + assert.ok(clean.length >= 2, `expected clean cases, got ${clean.length}`); + assert.ok(debt.length >= 3, `expected debt cases, got ${debt.length}`); +}); + +test('a clean case reporting findings is counted as a false positive', () => { + const sample = { pr: 1, expectation: 'clean', rationale: 'all projects meet the bar', knownFindings: [] }; + const report = { findings: [{ file: 'a.ts', line: 1, code: 'TS2345' }] }; + + const verdict = adjudicate(sample, report); + assert.equal(verdict.matchedExpectation, false); + assert.equal(verdict.falsePositives.length, 1); +}); + +test('a debt case is judged against its known findings, and extras are flagged for adjudication', () => { + const sample = { + pr: 2, + expectation: 'debt', + rationale: 'touches a non-strict lib', + knownFindings: [{ file: 'a.ts', line: 10, code: 'TS4111' }] + }; + const report = { + findings: [ + { file: 'a.ts', line: 10, code: 'TS4111' }, + { file: 'b.ts', line: 3, code: 'TS7030' } + ] + }; + + const verdict = adjudicate(sample, report); + assert.equal(verdict.matchedExpectation, true); + assert.equal(verdict.unexpected.length, 1, 'an unexpected finding needs a human judgement'); + assert.equal(verdict.missed.length, 0); +}); + +test('a debt case that reports nothing is a miss, not a pass', () => { + const sample = { + pr: 3, + expectation: 'debt', + rationale: 'known violations', + knownFindings: [{ file: 'a.ts', line: 10, code: 'TS4111' }] + }; + const verdict = adjudicate(sample, { findings: [] }); + + assert.equal(verdict.matchedExpectation, false); + assert.equal(verdict.missed.length, 1); +}); + +test('summarize reports the false-positive rate with its sample size, never a bare percentage', () => { + const summary = summarize([ + { sample: { pr: 1, expectation: 'clean' }, verdict: { falsePositives: [], matchedExpectation: true } }, + { sample: { pr: 2, expectation: 'clean' }, verdict: { falsePositives: [{}], matchedExpectation: false } } + ]); + + assert.equal(summary.cleanCases, 2); + assert.equal(summary.cleanCasesWithFindings, 1); + assert.equal(summary.falsePositiveRate, 0.5); + // A rate without its denominator invites being quoted as if it were a statistical claim. + assert.equal(summary.sampleSize, 2); + assert.match(summary.caveat, /not a statistical claim/i); +}); + +/* ── Granularity-qualified known findings ─────────────────────────────────── + * The anchor case reports five violations under whole-file granularity and three under + * line-level: two of the five sit on pre-existing lines the pull request did not write. A single + * flat list of known findings therefore reports a MISMATCH under one granularity or the other, + * no matter which numbers it holds. The expectation has to name the granularity it belongs to. + */ + +test('known findings can be qualified per granularity', () => { + const sample = { + pr: 1, + expectation: 'debt', + rationale: 'non-strict project', + knownFindings: { + file: [ + { file: 'a.ts', line: 10, code: 'TS4111' }, + { file: 'a.ts', line: 99, code: 'TS4111' } + ], + line: [{ file: 'a.ts', line: 10, code: 'TS4111' }] + } + }; + const report = { findings: [{ file: 'a.ts', line: 10, code: 'TS4111' }] }; + + assert.equal(adjudicate(sample, report, 'line').matchedExpectation, true); + assert.equal(adjudicate(sample, report, 'file').missed.length, 1, 'whole-file expects both'); +}); + +test('a flat known-findings array still works for any granularity', () => { + const sample = { + pr: 2, + expectation: 'debt', + rationale: 'x', + knownFindings: [{ file: 'a.ts', line: 10, code: 'TS4111' }] + }; + const report = { findings: [{ file: 'a.ts', line: 10, code: 'TS4111' }] }; + assert.equal(adjudicate(sample, report, 'line').matchedExpectation, true); + assert.equal(adjudicate(sample, report).matchedExpectation, true); +}); + +test('the anchor case declares both granularities', () => { + const anchor = CORPUS.find((s) => s.pr === 37264); + assert.ok(anchor.knownFindings.file?.length === 5, 'five under whole-file'); + assert.ok(anchor.knownFindings.line?.length === 3, 'three under line-level'); +}); diff --git a/core-web/tools/scripts/strict-gate/filter.test.mjs b/core-web/tools/scripts/strict-gate/filter.test.mjs new file mode 100644 index 00000000000..2a719a906c0 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/filter.test.mjs @@ -0,0 +1,258 @@ +/** + * T014 — the diff-scoped filter. This is the spike's actual hypothesis in code form: + * the dependency's errors do not need to be fixed, they need to stop counting. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { filterDiagnostics } from './lib/filter.mjs'; + +const changed = [ + { path: 'libs/mine/src/a.ts', status: 'M', kind: 'source', changedLines: [[10, 12]] }, + { path: 'libs/mine/src/b.ts', status: 'A', kind: 'source', changedLines: [[1, 5]] } +]; + +const diag = (file, line, code, layer = 'source') => ({ + file, + line, + column: 1, + code, + message: `${code} at ${file}:${line}`, + layer +}); + +test('keeps diagnostics in changed files and marks them as changed', () => { + const { findings } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 11, 'TS2345')], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 1); + assert.equal(findings[0].origin, 'changed'); +}); + +test('discards diagnostics from another project and counts them as dependency-origin', () => { + const { findings, discarded } = filterDiagnostics({ + diagnostics: [ + diag('libs/dep/src/x.ts', 3, 'TS7006'), + diag('libs/dep/src/y.ts', 9, 'TS18047'), + diag('libs/mine/src/a.ts', 11, 'TS2345') + ], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 1, 'only the changed file survives'); + assert.equal(discarded.byOrigin.dependency, 2); +}); + +test('discards diagnostics in untouched files of the same project', () => { + const { findings, discarded } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/untouched.ts', 4, 'TS7006')], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 0); + assert.equal(discarded.byOrigin.dependency + discarded.byOrigin.untouched, 1); +}); + +test('reports discarded counts on a PASSING run, not only on a failing one', () => { + // A pass with no evidence is indistinguishable from the harness having checked nothing. + // This count is what shows the filter produced the pass (SC-004). + const { findings, discarded } = filterDiagnostics({ + diagnostics: [diag('libs/dep/src/x.ts', 3, 'TS7006')], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 0); + assert.ok(discarded.byOrigin.dependency > 0, 'a passing run must still show what it discarded'); +}); + +test('whole-file granularity keeps a diagnostic outside the changed lines', () => { + const { findings } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 99, 'TS2345')], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 1, 'whole-file inherits the file’s existing debt by design'); +}); + +test('line granularity discards a diagnostic outside the changed lines', () => { + const { findings, discarded } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 99, 'TS2345')], + changedFiles: changed, + granularity: 'line' + }); + + assert.equal(findings.length, 0); + assert.equal(discarded.byOrigin.untouched, 1); +}); + +test('line granularity keeps a diagnostic on a changed line', () => { + const { findings } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 11, 'TS2345')], + changedFiles: changed, + granularity: 'line' + }); + + assert.equal(findings.length, 1); +}); + +test('counts discarded diagnostics separately by layer', () => { + const { discarded } = filterDiagnostics({ + diagnostics: [ + diag('libs/dep/src/x.ts', 3, 'TS7006', 'source'), + diag('libs/dep/src/x.component.html', 2, 'NG8002', 'template') + ], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(discarded.byLayer.source, 1); + assert.equal(discarded.byLayer.template, 1); +}); + +/* ── T028 (US2) ───────────────────────────────────────────────────────────── + * A pass with no evidence is indistinguishable from the harness having checked nothing. These + * assert the evidence is present and correctly attributed even when the gate is green. + */ + +test('a passing run reports discarded counts in BOTH dimensions', () => { + const { findings, discarded } = filterDiagnostics({ + diagnostics: [ + diag('libs/dep/src/x.ts', 3, 'TS7006', 'source'), + diag('libs/dep/src/x.component.html', 2, 'NG8002', 'template'), + diag('libs/mine/src/untouched.ts', 4, 'TS7030', 'source') + ], + changedFiles: changed, + granularity: 'file' + }); + + assert.equal(findings.length, 0, 'this run must pass'); + assert.equal(discarded.byOrigin.dependency + discarded.byOrigin.untouched, 3); + assert.equal(discarded.byLayer.source, 2); + assert.equal(discarded.byLayer.template, 1); +}); + +test('every diagnostic is accounted for: findings + discarded equals the input', () => { + const diagnostics = [ + diag('libs/mine/src/a.ts', 11, 'TS2345'), + diag('libs/mine/src/a.ts', 99, 'TS2345'), + diag('libs/dep/src/x.ts', 3, 'TS7006'), + diag('libs/mine/src/untouched.ts', 4, 'TS7030') + ]; + for (const granularity of ['file', 'line']) { + const { findings, discarded } = filterDiagnostics({ diagnostics, changedFiles: changed, granularity }); + const total = findings.length + discarded.byOrigin.dependency + discarded.byOrigin.untouched; + assert.equal(total, diagnostics.length, `${granularity}: a diagnostic was silently lost`); + } +}); + +test('projectRoots, when supplied, classify untouched vs dependency exactly', () => { + const { discarded } = filterDiagnostics({ + diagnostics: [ + diag('libs/mine/deep/nested/other.ts', 4, 'TS7030'), + diag('libs/dep/src/x.ts', 3, 'TS7006') + ], + changedFiles: changed, + granularity: 'file', + projectRoots: ['libs/mine'] + }); + + assert.equal(discarded.byOrigin.untouched, 1, 'same project, file the diff did not touch'); + assert.equal(discarded.byOrigin.dependency, 1, 'another project entirely'); +}); + +/* ── Infrastructure diagnostics ───────────────────────────────────────────── + * Adjudication of the corpus turned up one false positive: TS2307 "Cannot find module + * '@openng/spectator/jest'" on a pre-registered clean pull request. It appears with plain `tsc` + * too, with no flags forced — it is a module-resolution problem, not a strictness violation, and + * it never will be one. A strictness gate that reports it is crying wolf. + * + * These are DISCARDED, not silently dropped: the count is reported like every other, because a + * gate that hides what it ignored cannot be audited. + */ + +test('module-resolution diagnostics are discarded as infrastructure, not reported', () => { + const { findings, discarded } = filterDiagnostics({ + diagnostics: [ + diag('libs/mine/src/a.ts', 11, 'TS2307'), + diag('libs/mine/src/a.ts', 11, 'TS2688'), + diag('libs/mine/src/a.ts', 11, 'TS6053'), + diag('libs/mine/src/a.ts', 11, 'TS2345') + ], + changedFiles: changed, + granularity: 'file' + }); + + assert.deepEqual(findings.map((f) => f.code), ['TS2345'], 'only the strictness violation survives'); + assert.equal(discarded.byOrigin.infrastructure, 3); +}); + +test('an infrastructure diagnostic is discarded even on a changed line', () => { + const { findings } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/b.ts', 3, 'TS2307')], + changedFiles: changed, + granularity: 'line' + }); + assert.equal(findings.length, 0, 'a missing module is never this gate’s business'); +}); + +test('infrastructure diagnostics are still counted in the layer totals', () => { + const { discarded } = filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 11, 'TS2307')], + changedFiles: changed, + granularity: 'file' + }); + assert.equal(discarded.byLayer.source, 1, 'discarded, but never invisible'); +}); + +/* ── T039 (US3) — line granularity boundaries ─────────────────────────────── + * The adoption argument rests entirely on this: touching one line of a legacy file must not make + * the author inherit the file's history. An off-by-one at either end of a span breaks that + * promise quietly — no error, just a wrong number in the write-up. + */ + +test('line granularity includes both endpoints of a span', () => { + const files = [{ path: 'a.ts', status: 'M', kind: 'source', changedLines: [[10, 12]] }]; + const kept = (line) => + filterDiagnostics({ + diagnostics: [diag('a.ts', line, 'TS2345')], + changedFiles: files, + granularity: 'line' + }).findings.length; + + assert.equal(kept(9), 0, 'one line before the span'); + assert.equal(kept(10), 1, 'first line of the span'); + assert.equal(kept(12), 1, 'last line of the span'); + assert.equal(kept(13), 0, 'one line after the span'); +}); + +test('a file with no changed lines contributes nothing under line granularity', () => { + // A pure rename: the file is in the diff, but the author wrote none of it. + const files = [{ path: 'a.ts', status: 'R', kind: 'source', changedLines: [] }]; + const { findings, discarded } = filterDiagnostics({ + diagnostics: [diag('a.ts', 1, 'TS2345'), diag('a.ts', 500, 'TS7006')], + changedFiles: files, + granularity: 'line' + }); + + assert.equal(findings.length, 0, 'renaming a file must not make you own its debt'); + assert.equal(discarded.byOrigin.untouched, 2); +}); + +test('whole-file granularity is a strict superset of line granularity', () => { + const files = [{ path: 'a.ts', status: 'M', kind: 'source', changedLines: [[10, 12]] }]; + const diagnostics = [diag('a.ts', 5, 'TS7006'), diag('a.ts', 11, 'TS2345'), diag('a.ts', 90, 'TS2531')]; + + const byFile = filterDiagnostics({ diagnostics, changedFiles: files, granularity: 'file' }).findings; + const byLine = filterDiagnostics({ diagnostics, changedFiles: files, granularity: 'line' }).findings; + + assert.equal(byFile.length, 3); + assert.equal(byLine.length, 1); + const fileKeys = new Set(byFile.map((f) => `${f.file}:${f.line}`)); + for (const f of byLine) assert.ok(fileKeys.has(`${f.file}:${f.line}`)); +}); diff --git a/core-web/tools/scripts/strict-gate/fixtures/make-ng-project.mjs b/core-web/tools/scripts/strict-gate/fixtures/make-ng-project.mjs new file mode 100644 index 00000000000..a6a7dc0088e --- /dev/null +++ b/core-web/tools/scripts/strict-gate/fixtures/make-ng-project.mjs @@ -0,0 +1,129 @@ +/** + * An Angular fixture project with template strictness switched OFF. + * + * This mirrors the state of the four real applications, which carry + * `TODO(#35930): re-enable strictTemplates once Angular 22 template errors are fixed per app`. + * The template arm's entire premise is that the harness can force strictness on a project shaped + * exactly like this without touching a single file it owns. + * + * Two components on purpose: + * - separate template → the diagnostic's originating file is the .html + * - inline template → the diagnostic's originating file is the .ts + * The filter has to attribute both correctly or template findings land on the wrong file. + */ +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; + +/** + * @param {{ strictTemplates?: boolean, withViolations?: boolean }} [options] + */ +export async function makeNgProject({ strictTemplates = false, withViolations = true } = {}) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-ng-')); + const root = 'libs/fixture-ng'; + + const write = async (relative, contents) => { + const target = path.join(dir, relative); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile( + target, + typeof contents === 'string' ? contents : JSON.stringify(contents, null, 4), + 'utf8' + ); + }; + + await write('tsconfig.base.json', { + compilerOptions: { + target: 'es2022', + module: 'esnext', + moduleResolution: 'bundler', + lib: ['es2022', 'dom'], + skipLibCheck: true, + experimentalDecorators: true, + strict: false, + baseUrl: '.' + } + }); + + await write('nx.json', { namedInputs: { sharedGlobals: ['{workspaceRoot}/nx.json'] } }); + await write(path.join(root, 'project.json'), { name: 'fixture-ng', root, targets: {} }); + + await write(path.join(root, 'tsconfig.json'), { + extends: '../../tsconfig.base.json', + files: [], + include: [], + references: [{ path: './tsconfig.lib.json' }], + angularCompilerOptions: { + // Deliberately off — the harness must override this without editing the file. + strictTemplates, + strictInjectionParameters: false + } + }); + await write(path.join(root, 'tsconfig.lib.json'), { + extends: './tsconfig.json', + include: ['src/**/*.ts'] + }); + + // A number bound to a string input: passes with strictTemplates off, fails with it on. + const badBinding = withViolations ? '[label]="count"' : '[label]="title"'; + + await write( + path.join(root, 'src/child.component.ts'), + `import { Component, Input } from '@angular/core'; + +@Component({ + selector: 'fx-child', + standalone: true, + template: '{{ label }}' +}) +export class ChildComponent { + @Input() label!: string; +} +` + ); + + await write( + path.join(root, 'src/separate.component.ts'), + `import { Component } from '@angular/core'; +import { ChildComponent } from './child.component'; + +@Component({ + selector: 'fx-separate', + standalone: true, + imports: [ChildComponent], + templateUrl: './separate.component.html' +}) +export class SeparateComponent { + title = 'hello'; + count = 42; +} +` + ); + await write(path.join(root, 'src/separate.component.html'), `\n`); + + await write( + path.join(root, 'src/inline.component.ts'), + `import { Component } from '@angular/core'; +import { ChildComponent } from './child.component'; + +@Component({ + selector: 'fx-inline', + standalone: true, + imports: [ChildComponent], + template: '' +}) +export class InlineComponent { + title = 'hello'; + count = 7; +} +` + ); + + return { + dir, + root, + separateTemplate: path.join(root, 'src/separate.component.html'), + inlineComponent: path.join(root, 'src/inline.component.ts'), + cleanup: () => fs.rm(dir, { recursive: true, force: true }) + }; +} diff --git a/core-web/tools/scripts/strict-gate/fixtures/make-repo.mjs b/core-web/tools/scripts/strict-gate/fixtures/make-repo.mjs new file mode 100644 index 00000000000..5de8995a884 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/fixtures/make-repo.mjs @@ -0,0 +1,106 @@ +/** + * Fixture git repositories, built in a temp dir and torn down afterwards. + * + * The unit tests must never touch the real repository: the harness's whole contract is that it + * writes nothing, and a test that mutates the working tree could not tell a real violation of + * that contract from its own mess. + */ +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { git } from '../lib/exec.mjs'; + +const AUTHOR = [ + '-c', 'user.name=strict-gate fixture', + '-c', 'user.email=fixture@example.invalid', + '-c', 'commit.gpgsign=false' +]; + +/** + * @typedef {Object} FixtureRepo + * @property {string} dir Absolute path to the repository. + * @property {(tree: Record, message: string) => Promise} commit + * Writes a file tree and commits it. A `null` value deletes the file. Returns the SHA. + * @property {(from: string, to: string, message: string) => Promise} rename + * @property {(depth?: number) => Promise} shallowClone + * @property {() => Promise} cleanup + */ + +/** + * @param {Record} [initialTree] + * @returns {Promise} + */ +export async function makeRepo(initialTree) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-repo-')); + const created = [dir]; + + await git(['init', '--initial-branch=main', dir]); + + async function writeTree(tree) { + for (const [relative, contents] of Object.entries(tree)) { + const target = path.join(dir, relative); + if (contents === null) { + await fs.rm(target, { force: true }); + continue; + } + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile(target, contents, 'utf8'); + } + } + + async function commit(tree, message) { + await writeTree(tree); + await git(['-C', dir, 'add', '--all']); + await git(['-C', dir, ...AUTHOR, 'commit', '--allow-empty', '-m', message]); + const { stdout } = await git(['-C', dir, 'rev-parse', 'HEAD']); + return stdout.trim(); + } + + async function rename(from, to, message) { + await fs.mkdir(path.dirname(path.join(dir, to)), { recursive: true }); + await git(['-C', dir, 'mv', from, to]); + await git(['-C', dir, ...AUTHOR, 'commit', '-m', message]); + const { stdout } = await git(['-C', dir, 'rev-parse', 'HEAD']); + return stdout.trim(); + } + + async function shallowClone(depth = 1) { + const cloneDir = await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-shallow-')); + created.push(cloneDir); + // file:// forces a real fetch protocol; a plain path clone would hardlink the full history + // and the shallow-checkout test would silently exercise nothing. + await git(['clone', '--depth', String(depth), `file://${dir}`, cloneDir]); + return { + dir: cloneDir, + commit: () => { + throw new Error('shallow clone fixtures are read-only'); + }, + rename: () => { + throw new Error('shallow clone fixtures are read-only'); + }, + shallowClone: () => { + throw new Error('cannot re-clone a shallow fixture'); + }, + revParse: async (ref = 'HEAD') => { + const { stdout } = await git(['-C', cloneDir, 'rev-parse', `${ref}^{commit}`]); + return stdout.trim(); + }, + cleanup: async () => fs.rm(cloneDir, { recursive: true, force: true }) + }; + } + + /** Resolves a ref to a SHA now. Tests must capture the base BEFORE committing: passing the + * literal 'HEAD' makes git resolve it at diff time, so base === head and the diff is empty. */ + async function revParse(ref = 'HEAD') { + const { stdout } = await git(['-C', dir, 'rev-parse', `${ref}^{commit}`]); + return stdout.trim(); + } + + async function cleanup() { + await Promise.all(created.map((d) => fs.rm(d, { recursive: true, force: true }))); + } + + if (initialTree) await commit(initialTree, 'initial'); + + return { dir, commit, rename, shallowClone, revParse, cleanup }; +} diff --git a/core-web/tools/scripts/strict-gate/fixtures/make-workspace.mjs b/core-web/tools/scripts/strict-gate/fixtures/make-workspace.mjs new file mode 100644 index 00000000000..cf62b5be3dd --- /dev/null +++ b/core-web/tools/scripts/strict-gate/fixtures/make-workspace.mjs @@ -0,0 +1,121 @@ +/** + * Miniature Nx-shaped workspaces for the mapping and configuration-selection tests. + * + * Reproduces the two structural facts the real workspace has and that the harness must cope with: + * a base config that turns strict OFF and is inherited by everyone, and path aliases that point at + * SOURCES rather than build output — which is what drags a dependency's files into your program. + */ +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; + +/** + * @typedef {Object} FixtureProject + * @property {string} name + * @property {string} root Workspace-relative, e.g. `libs/thing`. + * @property {Record} files Workspace-relative path → contents. + * @property {'lib'|'app'|'references'} [shape] Which tsconfig layout to emit. Default `lib`. + * @property {string[]} [dependsOn] Project names this one imports by alias. + * @property {Record} [angularCompilerOptions] + */ + +/** + * @param {{ projects: FixtureProject[], strict?: boolean }} spec + */ +export async function makeWorkspace({ projects, strict = false }) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-ws-')); + + const write = async (relative, contents) => { + const target = path.join(dir, relative); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile( + target, + typeof contents === 'string' ? contents : JSON.stringify(contents, null, 4), + 'utf8' + ); + }; + + // Aliases point at sources, exactly as tsconfig.base.json does in the real workspace. + const paths = {}; + for (const project of projects) { + paths[`@fixture/${project.name}`] = [`./${project.root}/src/index.ts`]; + } + + await write('tsconfig.base.json', { + compilerOptions: { + target: 'es2022', + module: 'esnext', + moduleResolution: 'bundler', + lib: ['es2022', 'dom'], + skipLibCheck: true, + strict, + baseUrl: '.', + paths + } + }); + + await write('nx.json', { + namedInputs: { + default: ['{projectRoot}/**/*', 'sharedGlobals'], + sharedGlobals: ['{workspaceRoot}/tsconfig.base.json', '{workspaceRoot}/nx.json'] + } + }); + + for (const project of projects) { + const { name, root, files, shape = 'lib', angularCompilerOptions } = project; + await write(path.join(root, 'project.json'), { name, root, targets: {} }); + + const depth = root.split('/').length; + const toBase = `${'../'.repeat(depth)}tsconfig.base.json`; + + if (shape === 'references') { + // The real portlets do this: a root config that owns no files and only points at others. + // It must resolve to zero files and exclude itself from selection with no special-casing. + await write(path.join(root, 'tsconfig.json'), { + extends: toBase, + files: [], + include: [], + references: [{ path: './tsconfig.lib.json' }, { path: './tsconfig.spec.json' }], + ...(angularCompilerOptions ? { angularCompilerOptions } : {}) + }); + await write(path.join(root, 'tsconfig.lib.json'), { + extends: './tsconfig.json', + compilerOptions: { outDir: '../../dist' }, + include: ['src/**/*.ts'], + exclude: ['**/*.spec.ts'] + }); + await write(path.join(root, 'tsconfig.spec.json'), { + extends: './tsconfig.json', + include: ['src/**/*.spec.ts'] + }); + } else { + const mainName = shape === 'app' ? 'tsconfig.app.json' : 'tsconfig.lib.json'; + await write(path.join(root, 'tsconfig.json'), { + extends: toBase, + files: [], + include: [], + references: [{ path: `./${mainName}` }, { path: './tsconfig.spec.json' }], + ...(angularCompilerOptions ? { angularCompilerOptions } : {}) + }); + await write(path.join(root, mainName), { + extends: './tsconfig.json', + include: ['src/**/*.ts'], + exclude: ['**/*.spec.ts'] + }); + await write(path.join(root, 'tsconfig.spec.json'), { + extends: './tsconfig.json', + include: ['src/**/*.spec.ts'] + }); + } + + for (const [relative, contents] of Object.entries(files)) { + await write(path.join(root, relative), contents); + } + } + + return { + dir, + projects: projects.map(({ name, root }) => ({ name, root })), + cleanup: () => fs.rm(dir, { recursive: true, force: true }) + }; +} diff --git a/core-web/tools/scripts/strict-gate/hunks.test.mjs b/core-web/tools/scripts/strict-gate/hunks.test.mjs new file mode 100644 index 00000000000..24939d2c5f0 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/hunks.test.mjs @@ -0,0 +1,45 @@ +/** + * T038 — parsing changed line ranges from a zero-context diff. + * + * This is what makes line-level granularity possible, and getting it wrong is silent in both + * directions: too-wide ranges make the gate blame untouched code, too-narrow ones make it miss + * real violations. Neither shows up as an error, only as a wrong number in the write-up. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { parseHunks } from './lib/hunks.mjs'; + +test('a single-line change yields a one-line span', () => { + assert.deepEqual(parseHunks('@@ -2 +2 @@\n-old\n+new\n'), [[2, 2]]); +}); + +test('an explicit count yields an inclusive span', () => { + assert.deepEqual(parseHunks('@@ -10,0 +10,3 @@\n+a\n+b\n+c\n'), [[10, 12]]); +}); + +test('several hunks yield several spans, in order', () => { + const diff = '@@ -1 +1 @@\n+a\n@@ -20,0 +21,2 @@\n+b\n+c\n@@ -50,2 +53 @@\n+d\n'; + assert.deepEqual(parseHunks(diff), [[1, 1], [21, 22], [53, 53]]); +}); + +test('a pure deletion hunk contributes no span', () => { + // `+50,0` means nothing was added at that point — there is no line to blame. + assert.deepEqual(parseHunks('@@ -50,3 +50,0 @@\n-a\n-b\n-c\n'), []); +}); + +test('an empty diff yields no spans', () => { + assert.deepEqual(parseHunks(''), []); + assert.deepEqual(parseHunks('\n'), []); +}); + +test('a rename with no content change yields no spans', () => { + const diff = 'diff --git a/old.ts b/new.ts\nsimilarity index 100%\nrename from old.ts\nrename to new.ts\n'; + assert.deepEqual(parseHunks(diff), [], 'nothing was written, so nothing is attributable'); +}); + +test('hunk headers appearing inside content are not mistaken for real hunks', () => { + // A test fixture or a markdown file can legitimately contain a line starting with "@@". + // Only headers at the start of a line in the diff stream count, and they must match the shape. + const diff = '@@ -1 +1 @@\n+const marker = "@@ -99,0 +99,5 @@";\n'; + assert.deepEqual(parseHunks(diff), [[1, 1]]); +}); diff --git a/core-web/tools/scripts/strict-gate/lib/changed-files.mjs b/core-web/tools/scripts/strict-gate/lib/changed-files.mjs new file mode 100644 index 00000000000..2b79fa8d3eb --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/changed-files.mjs @@ -0,0 +1,102 @@ +/** + * Resolves what a pull request actually changed. The unit the whole gate is scoped by. + */ +import path from 'node:path'; +import { git } from './exec.mjs'; +import { parseHunks } from './hunks.mjs'; + +const SOURCE_EXT = new Set(['.ts', '.tsx', '.mts', '.cts']); +const TEMPLATE_EXT = new Set(['.html']); + +/** @returns {'source'|'template'|null} null means no compiler reads this file. */ +export function classify(filePath) { + const ext = path.extname(filePath); + if (SOURCE_EXT.has(ext)) return 'source'; + if (TEMPLATE_EXT.has(ext)) return 'template'; + return null; +} + +async function resolves(repoDir, ref) { + const { exitCode } = await git(['-C', repoDir, 'rev-parse', '--verify', '--quiet', `${ref}^{commit}`], { + allowFailure: true + }); + return exitCode === 0; +} + +/** + * Makes the base ref usable, fetching it when the checkout is shallow. + * + * The failure being guarded is subtle and expensive: if the base is missing and we let git diff + * against nothing, the gate reports "no changes" and passes every pull request in CI. Silence + * here is worse than an error, so an unresolvable base throws. + */ +export async function ensureBaseRef({ repoDir, base }) { + if (await resolves(repoDir, base)) return; + + const attempts = [ + ['-C', repoDir, 'fetch', '--no-tags', '--depth=50', 'origin', base], + ['-C', repoDir, 'fetch', '--no-tags', 'origin', base], + ['-C', repoDir, 'fetch', '--no-tags', '--unshallow', 'origin'] + ]; + for (const args of attempts) { + await git(args, { allowFailure: true }); + if (await resolves(repoDir, base)) return; + } + + throw new Error( + `base ref '${base}' cannot be resolved even after fetching. Refusing to report an empty ` + + `diff, which would pass the gate for every pull request.` + ); +} + +/** Added/modified line spans, 1-based inclusive, from a zero-context diff. */ +export async function changedLinesFor({ repoDir, base, head, file }) { + const { stdout } = await git([ + '-C', repoDir, 'diff', '--unified=0', '--no-color', `${base}..${head}`, '--', file + ]); + return parseHunks(stdout); +} + +/** + * @param {{ repoDir: string, base: string, head?: string }} options + * @returns {Promise<{ files: object[], base: string, head: string }>} + */ +export async function resolveChangedFiles({ repoDir, base, head = 'HEAD' }) { + await ensureBaseRef({ repoDir, base }); + + const sha = async (ref) => (await git(['-C', repoDir, 'rev-parse', `${ref}^{commit}`])).stdout.trim(); + const headSha = await sha(head); + + // A pull request's diff is `base...head` — everything since the two diverged — not `base..head`, + // which compares two trees. The difference is invisible while a branch is fresh and wrong once + // it is stale: a tree comparison reports every file the BASE modified as changed, so the gate + // blames the author for violations someone else merged. Resolving the merge base up front means + // the report also CITES the point of divergence, which is what makes a re-run reproducible. + const mergeBase = await git(['-C', repoDir, 'merge-base', base, headSha], { allowFailure: true }); + const baseSha = mergeBase.exitCode === 0 && mergeBase.stdout.trim() + ? mergeBase.stdout.trim() + : await sha(base); + + // -M so a rename is reported at its new path; ACMR so deletions never appear — there is + // nothing to typecheck in a file that no longer exists at head. + const { stdout } = await git([ + '-C', repoDir, 'diff', '--name-status', '-M', '--diff-filter=ACMR', `${baseSha}..${headSha}` + ]); + + const files = []; + for (const line of stdout.split('\n').filter(Boolean)) { + const parts = line.split('\t'); + const status = parts[0][0]; + const filePath = parts[parts.length - 1]; // rename rows carry old\tnew + const kind = classify(filePath); + if (!kind) continue; + files.push({ + path: filePath, + status, + kind, + changedLines: await changedLinesFor({ repoDir, base: baseSha, head: headSha, file: filePath }) + }); + } + + return { files, base: baseSha, head: headSha }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/check-ng.mjs b/core-web/tools/scripts/strict-gate/lib/check-ng.mjs new file mode 100644 index 00000000000..9ca68765726 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/check-ng.mjs @@ -0,0 +1,95 @@ +/** + * Template-aware checking, via the Angular compiler API. + * + * A separate mechanism from check-ts by necessity, not preference. Angular's strictness settings + * are not TypeScript compiler options: `ngc` parses its arguments with `ts.parseCommandLine` and + * tolerates only five non-TypeScript options (i18nFile, i18nFormat, locale, missingTranslation, + * watch), so `--strictTemplates` is rejected outright. They can only reach the compiler through + * configuration — and `readConfiguration(project, existingOptions)` spreads `existingOptions` + * last, above everything read from the extends chain. That forces them in memory, with no + * overlay file to leave behind if the process dies. + */ +import path from 'node:path'; +import { loadAngularCompiler, loadTypeScript } from './resolve-tools.mjs'; +import { FLAG_SETS } from './check-ts.mjs'; + +/** + * The four settings the workspace already treats as its Angular convention: 30 project configs + * declare strictTemplates, 9 declare typeCheckHostBindings. `extendedDiagnostics` is deliberately + * NOT here — promoting a whole diagnostic category to errors makes a future framework minor able + * to fail pull requests for something they did not change (FR-018). + */ +export const ANGULAR_STRICT = { + strictTemplates: true, + strictInjectionParameters: true, + strictInputAccessModifiers: true, + typeCheckHostBindings: true +}; + +/** + * Angular encodes its error codes as negative TypeScript codes: NG8002 becomes -998002 + * (`'-99' + code`). Recovering the display form keeps the report readable and greppable. + */ +export function formatCode(code) { + if (code >= 0) return `TS${code}`; + const recovered = Math.abs(code) - 990000; + return recovered > 0 ? `NG${recovered}` : `NG${Math.abs(code)}`; +} + +function toDiagnostic(ts, diagnostic) { + const file = diagnostic.file; + const { line, character } = + file && diagnostic.start !== undefined + ? file.getLineAndCharacterOfPosition(diagnostic.start) + : { line: 0, character: 0 }; + const fileName = file ? path.resolve(file.fileName) : ''; + return { + file: fileName, + line: line + 1, + column: character + 1, + code: formatCode(diagnostic.code), + message: ts.flattenDiagnosticMessageText(diagnostic.messageText, ' '), + // A diagnostic is template-layer if it came from Angular, or if it landed in a template + // file. An inline template reports against the component source, so the code decides. + layer: diagnostic.code < 0 || fileName.endsWith('.html') ? 'template' : 'source' + }; +} + +/** + * @param {{ configPath: string, flagSet?: string, forceTemplates?: boolean }} input + * @returns {Promise<{ diagnostics: object[] }>} `file` is absolute; the caller relativizes. + */ +export async function checkAngularTemplates({ configPath, flagSet = 'strict', forceTemplates = true }) { + const ng = await loadAngularCompiler(); + const ts = await loadTypeScript(); + + const overrides = { + ...(FLAG_SETS[flagSet] ?? FLAG_SETS.strict), + ...(forceTemplates ? ANGULAR_STRICT : {}), + noEmit: true + }; + + const config = ng.readConfiguration(configPath, overrides); + if (config.errors?.length) { + throw new Error(`cannot read ${configPath}: ${ts.flattenDiagnosticMessageText(config.errors[0].messageText, ' ')}`); + } + + const host = ng.createCompilerHost({ options: config.options }); + const program = ng.createProgram({ rootNames: config.rootNames, options: config.options, host }); + + // Structural diagnostics must be requested before the semantic ones, or ngtsc has not yet + // analysed the component scopes the template check depends on. + const collected = []; + for (const method of [ + 'getNgStructuralDiagnostics', + 'getTsSyntacticDiagnostics', + 'getTsSemanticDiagnostics', + 'getNgSemanticDiagnostics' + ]) { + if (typeof program[method] === 'function') { + collected.push(...(await program[method]())); + } + } + + return { diagnostics: collected.filter((d) => d.file).map((d) => toDiagnostic(ts, d)) }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/check-ts.mjs b/core-web/tools/scripts/strict-gate/lib/check-ts.mjs new file mode 100644 index 00000000000..63a52171e25 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/check-ts.mjs @@ -0,0 +1,79 @@ +/** + * TypeScript-only checking, with strictness forced IN MEMORY. + * + * No overlay config is written: a crash mid-run would leave one behind and break the harness's + * central promise that the working tree is byte-identical afterwards (SC-010). + */ +import path from 'node:path'; +import { loadTypeScript } from './resolve-tools.mjs'; + +/** + * `--strict` is an umbrella over eight flags and does NOT include the four below. Verified against + * ts.optionDeclarations, and it matters: on the acceptance case bare --strict finds 2 of 5. + * The `strict` set here mirrors tsconfig.base.json on PR #37198 — the gate must measure with the + * same yardstick as the destination, or it passes debt that the migration will later have to fix. + */ +export const FLAG_SETS = { + 'null-checks': { + noImplicitAny: true, + strictNullChecks: true + }, + strict: { + strict: true, + noPropertyAccessFromIndexSignature: true, + noImplicitOverride: true, + noImplicitReturns: true, + noFallthroughCasesInSwitch: true + }, + // Beyond #37198. Measured for a future ratchet; not the blocking set — a gate stricter than + // the destination blocks pull requests for debt the destination does not consider debt. + 'strict-max': { + strict: true, + noPropertyAccessFromIndexSignature: true, + noImplicitOverride: true, + noImplicitReturns: true, + noFallthroughCasesInSwitch: true, + noUncheckedIndexedAccess: true, + exactOptionalPropertyTypes: true + } +}; + +export function toDiagnostic(ts, diagnostic, layer = 'source') { + const file = diagnostic.file; + const { line, character } = file && diagnostic.start !== undefined + ? file.getLineAndCharacterOfPosition(diagnostic.start) + : { line: 0, character: 0 }; + return { + file: file ? path.resolve(file.fileName) : '', + line: line + 1, + column: character + 1, + code: `TS${diagnostic.code}`, + message: ts.flattenDiagnosticMessageText(diagnostic.messageText, ' '), + layer + }; +} + +/** + * @param {{ workspaceDir: string, configPath: string, flagSet?: keyof typeof FLAG_SETS }} input + * @returns {Promise<{ diagnostics: object[] }>} `file` is absolute; the caller relativizes. + */ +export async function checkTypeScript({ configPath, flagSet = 'strict' }) { + const ts = await loadTypeScript(); + const overrides = FLAG_SETS[flagSet]; + if (!overrides) throw new Error(`unknown flag set '${flagSet}'`); + + const parsed = ts.getParsedCommandLineOfConfigFile(configPath, {}, { + ...ts.sys, + onUnRecoverableConfigFileDiagnostic: () => {} + }); + if (!parsed) throw new Error(`cannot parse ${configPath}`); + + const program = ts.createProgram({ + rootNames: parsed.fileNames, + options: { ...parsed.options, ...overrides, noEmit: true, incremental: false }, + projectReferences: parsed.projectReferences + }); + + const raw = [...program.getSyntacticDiagnostics(), ...program.getSemanticDiagnostics()]; + return { diagnostics: raw.filter((d) => d.file).map((d) => toDiagnostic(ts, d)) }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/config-select.mjs b/core-web/tools/scripts/strict-gate/lib/config-select.mjs new file mode 100644 index 00000000000..402e12b7c6e --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/config-select.mjs @@ -0,0 +1,146 @@ +/** + * Picks the configuration(s) that actually include a changed file. + * + * Selection is by RESOLVED FILE LIST, never by filename convention, and the acceptance case shows + * why in the bluntest possible way: two of its five violations live in a `.spec.ts`, and checking + * sdk-create-app's tsconfig.lib.json reports ZERO. A "lib first" heuristic would have found + * nothing while looking perfectly healthy, and the spike would have shipped a false number. + */ +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { loadTypeScript } from './resolve-tools.mjs'; + +/** + * @param {{ workspaceDir: string, project: {name:string,root:string}, files: string[], repoDir?: string }} input + * @returns {Promise<{project:string,root:string,configPath:string,files:string[]}[]>} + */ +export async function selectConfigs({ workspaceDir, project, files, repoDir = workspaceDir }) { + const ts = await loadTypeScript(); + const projectDir = path.resolve(repoDir, project.root); + + const candidates = (await fs.readdir(projectDir)) + .filter((name) => /^tsconfig\..*\.json$|^tsconfig\.json$/.test(name)) + .map((name) => path.join(projectDir, name)) + .sort(); + + // `tsconfig.editor.json` is generated by Nx for IDE language services. It globs the whole + // project, so it will happily claim any file — and it carries no `angularCompilerOptions`, + // which makes template strictness unreachable for anything it swallows. It is a last resort, + // never a stand-in for the build config. + const isIdeOnly = (configPath) => path.basename(configPath) === 'tsconfig.editor.json'; + + /** + * Preference order among configs that could host a file none of them names outright. + * + * The spec config is ranked LAST on purpose. Angular colocates `x.component.ts`, + * `x.component.html` and `x.component.spec.ts`, so any "owns TypeScript in this directory" + * rule matches the spec config as readily as the build config — and the spec config carries no + * `angularCompilerOptions`, so a template routed there goes unchecked behind a reported target + * and a PASS. + */ + const CONFIG_RANK = [ + 'tsconfig.app.json', + 'tsconfig.lib.json', + 'tsconfig.json', + 'tsconfig.spec.json', + 'tsconfig.editor.json' + ]; + const rankOf = (configPath) => { + const index = CONFIG_RANK.indexOf(path.basename(configPath)); + return index === -1 ? CONFIG_RANK.indexOf('tsconfig.spec.json') : index; + }; + + // Templates need a different rule: a tsconfig's resolved file list contains only TypeScript, + // so a .html is never in it. Matching sources by file list and then dropping templates would + // make a template-only pull request resolve zero configs and pass silently — which reads + // exactly like "nothing to check". + const isTemplate = (f) => path.extname(f) === '.html'; + const sources = files.filter((f) => !isTemplate(f)); + const templates = files.filter(isTemplate); + + const wanted = new Set(sources.map((f) => path.resolve(repoDir, f))); + const parsedConfigs = []; + + for (const configPath of candidates) { + const parsed = ts.getParsedCommandLineOfConfigFile(configPath, {}, { + ...ts.sys, + onUnRecoverableConfigFileDiagnostic: () => {} + }); + // A references-only config resolves to zero files and excludes itself with no special-casing. + if (!parsed || parsed.fileNames.length === 0) continue; + parsedConfigs.push({ configPath, fileNames: parsed.fileNames.map((f) => path.resolve(f)) }); + } + + const owned = new Map(); + const claim = (configPath, file) => { + if (!owned.has(configPath)) owned.set(configPath, new Set()); + owned.get(configPath).add(file); + }; + + // An IDE-only config globs the whole project, so it matches everything a build config also + // covers — and it carries no `angularCompilerOptions`, which makes template strictness + // unreachable for anything it swallows. Whenever the project has a real build config, the + // IDE one is ignored outright and its files are routed through the ranking below. + const hasBuildConfig = parsedConfigs.some((c) => !isIdeOnly(c.configPath)); + const eligible = hasBuildConfig ? parsedConfigs.filter((c) => !isIdeOnly(c.configPath)) : parsedConfigs; + + const matchedSources = new Set(); + for (const { configPath, fileNames } of eligible) { + for (const file of fileNames) { + if (wanted.has(file)) { + claim(configPath, path.relative(repoDir, file)); + matchedSources.add(file); + } + } + } + + /** + * Ranks the configs that could plausibly host a file none of them names outright. + * + * An app config that declares `files: ["src/main.ts"]` reaches every component through the + * import graph, so its resolved list names two entries while its PROGRAM contains thousands. + * Building the program to find out would cost the very compilation this step exists to scope. + */ + const hostFor = (absolutePath) => { + const dir = path.dirname(absolutePath); + const byRank = (a, b) => + rankOf(a.configPath) - rankOf(b.configPath) || b.fileNames.length - a.fileNames.length; + + // Siblings are ranked too, not taken in directory-listing order: the first match is as + // likely to be the spec config as the build config. + const siblings = eligible + .filter((c) => c.fileNames.some((f) => path.dirname(f) === dir)) + .sort(byRank); + if (siblings.length > 0 && rankOf(siblings[0].configPath) < rankOf('tsconfig.spec.json')) { + return siblings[0]; + } + + const ranked = [...eligible].sort(byRank); + return ranked[0] ?? siblings[0] ?? null; + }; + + // Sources an entry-point config owns transitively but never names. + for (const source of sources) { + const absolute = path.resolve(repoDir, source); + if (matchedSources.has(absolute)) continue; + const host = hostFor(absolute); + if (host) claim(host.configPath, source); + } + + // A template belongs to the component that references it, and Angular convention colocates the + // two. Attaching it to the config that owns TypeScript in the same directory is cheap and right + // in practice; resolving `templateUrl` properly would mean compiling in order to decide what to + // compile. Falling back to the config with the widest file list keeps an orphan template + // visible rather than dropping it. + for (const template of templates) { + const host = hostFor(path.resolve(repoDir, template)); + if (host) claim(host.configPath, template); + } + + return [...owned.entries()].map(([configPath, fileSet]) => ({ + project: project.name, + root: project.root, + configPath, + files: [...fileSet] + })); +} diff --git a/core-web/tools/scripts/strict-gate/lib/exec.mjs b/core-web/tools/scripts/strict-gate/lib/exec.mjs new file mode 100644 index 00000000000..b93c5d38e9e --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/exec.mjs @@ -0,0 +1,61 @@ +/** + * The only sanctioned way for this harness to run a child process. + * + * Every ref, branch name and file path the harness handles originates in pull-request metadata, + * which is untrusted input. A shell-interpolated branch name is a command-injection vector in a + * tool destined to run in CI, so nothing here ever builds a shell string: `execFile` receives an + * argument array and no shell is spawned. Constitution Principle III; guaranteed in contracts/cli.md. + */ +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); + +/** Raised when a child process exits non-zero and the caller did not allow it. */ +export class ExecError extends Error { + constructor(command, args, cause) { + super(`${command} ${args.join(' ')} failed: ${cause.message}`); + this.name = 'ExecError'; + this.command = command; + this.args = args; + this.exitCode = cause.code; + this.stderr = cause.stderr ?? ''; + this.cause = cause; + } +} + +/** + * @param {string} command + * @param {string[]} args Passed through verbatim; never concatenated into a shell string. + * @param {{ cwd?: string, allowFailure?: boolean, maxBuffer?: number }} [options] + * @returns {Promise<{ stdout: string, stderr: string, exitCode: number }>} + */ +export async function run(command, args, options = {}) { + if (!Array.isArray(args)) { + throw new TypeError('exec.run requires an argument array — never a shell string'); + } + const { cwd, allowFailure = false, maxBuffer = 64 * 1024 * 1024 } = options; + try { + const { stdout, stderr } = await execFileAsync(command, args, { + cwd, + maxBuffer, + shell: false, + encoding: 'utf8' + }); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + if (allowFailure) { + return { + stdout: error.stdout ?? '', + stderr: error.stderr ?? '', + exitCode: typeof error.code === 'number' ? error.code : 1 + }; + } + throw new ExecError(command, args, error); + } +} + +/** Convenience wrapper for git, which is most of what the harness shells out to. */ +export function git(args, options = {}) { + return run('git', args, options); +} diff --git a/core-web/tools/scripts/strict-gate/lib/filter.mjs b/core-web/tools/scripts/strict-gate/lib/filter.mjs new file mode 100644 index 00000000000..f537c2313d3 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/filter.mjs @@ -0,0 +1,75 @@ +/** + * The diff-scoped filter — the spike's hypothesis in code: + * the dependency's errors do not need to be FIXED, they need to stop COUNTING. + */ + +const inSpans = (line, spans) => spans.some(([start, end]) => line >= start && line <= end); + +/** + * Diagnostics that are never strictness violations, whatever the flags. + * + * Found by adjudicating the corpus: TS2307 fired on a pre-registered clean pull request, and it + * appears with plain `tsc` too — the module simply does not resolve. Reporting it as strict debt + * is crying wolf, and a gate that cries wolf gets switched off. Kept as a short, closed list of + * resolution failures rather than a heuristic; anything broader would start hiding real findings. + */ +export const INFRASTRUCTURE_CODES = new Set([ + 'TS2307', // Cannot find module + 'TS2688', // Cannot find type definition file + 'TS6053' // File not found +]); + +/** + * @param {{ + * diagnostics: object[], + * changedFiles: {path:string,changedLines:[number,number][]}[], + * granularity?: 'file'|'line', + * projectRoots?: string[] + * }} input + */ +export function filterDiagnostics({ diagnostics, changedFiles, granularity = 'file', projectRoots }) { + const changed = new Map(changedFiles.map((f) => [f.path, f])); + + // Distinguishing "another project's file" from "an untouched file of this project" needs to + // know what this project owns. When the caller supplies roots we use them; otherwise we fall + // back to the directories the diff touched, which is enough to keep the counts meaningful. + const owned = projectRoots?.length + ? (file) => projectRoots.some((r) => file === r || file.startsWith(`${r}/`)) + : (() => { + const dirs = new Set(changedFiles.map((f) => f.path.slice(0, f.path.lastIndexOf('/')))); + return (file) => [...dirs].some((d) => file.startsWith(`${d}/`)); + })(); + + const findings = []; + const discarded = { + byOrigin: { dependency: 0, untouched: 0, infrastructure: 0 }, + byLayer: { source: 0, template: 0 } + }; + + for (const diagnostic of diagnostics) { + const hit = changed.get(diagnostic.file); + let origin; + + if (INFRASTRUCTURE_CODES.has(diagnostic.code)) { + origin = 'infrastructure'; + } else if (!hit) { + origin = owned(diagnostic.file) ? 'untouched' : 'dependency'; + } else if (granularity === 'line' && !inSpans(diagnostic.line, hit.changedLines)) { + // Pre-existing debt on a line this pull request did not write. Whole-file granularity + // would make whoever touched the file inherit it; line-level does not. New files are + // unaffected — every line of an added file is a changed line. + origin = 'untouched'; + } else { + origin = 'changed'; + } + + if (origin === 'changed') { + findings.push({ ...diagnostic, origin }); + } else { + discarded.byOrigin[origin] += 1; + discarded.byLayer[diagnostic.layer === 'template' ? 'template' : 'source'] += 1; + } + } + + return { findings, discarded }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/format.mjs b/core-web/tools/scripts/strict-gate/lib/format.mjs new file mode 100644 index 00000000000..936e03576a9 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -0,0 +1,150 @@ +/** + * Renders a run report for the people and the agents that have to act on it. + * + * Design constraint that drives everything here: the primary consumer is often a coding agent + * reading CI output with no other context. It must be able to fix the failure from this text + * alone, and — just as important — it must NOT overreach. An agent that does not know the gate + * only counts changed lines will "helpfully" refactor an entire legacy file, producing a huge + * diff nobody asked for. So the output states the scope rule as loudly as it states the errors. + */ + +/** What to actually do about the codes this gate produces, by frequency in this workspace. */ +const GUIDANCE = { + TS4111: 'Access the property with bracket notation — `obj[\'KEY\']` instead of `obj.KEY`. It comes from an index signature.', + TS7030: 'Not all code paths return a value. Add the missing `return`, or give the function an explicit `: void`.', + TS7006: 'Parameter is implicitly `any`. Add an explicit type annotation.', + TS18047: 'Value may be `null`. Narrow it first (`if (x)`), or use `?.` / `??`.', + TS18048: 'Value may be `undefined`. Narrow it first, or use `?.` / `??`.', + TS2345: 'Argument type does not match the parameter type. Fix the value, or widen/correct the signature.', + TS2531: 'Object is possibly `null`. Narrow before use.', + TS2532: 'Object is possibly `undefined`. Narrow before use.', + TS7029: 'Switch case falls through. Add `break` / `return`, or mark it intentional.', + TS4114: 'This member overrides a base member — add the `override` modifier.', + TS2564: 'Property has no initializer and is not definitely assigned. Initialize it, or mark it `!`.' +}; + +const FLAG_SET_LABEL = { + strict: "the repo's strict convention (`strict` + noPropertyAccessFromIndexSignature, noImplicitOverride, noImplicitReturns, noFallthroughCasesInSwitch) — the same settings tsconfig.base.json carries on the strict-mode branch", + 'null-checks': '`strictNullChecks` + `noImplicitAny` only', + 'strict-max': "the repo's strict convention plus noUncheckedIndexedAccess and exactOptionalPropertyTypes" +}; + +const scopeRule = (granularity) => + granularity === 'line' + ? 'Only lines this pull request ADDED OR MODIFIED are checked. Pre-existing problems on untouched lines are deliberately ignored.' + : 'Every line of a changed file is checked, including pre-existing problems on lines this pull request did not touch.'; + +function groupByFile(findings) { + const byFile = new Map(); + for (const f of findings) { + if (!byFile.has(f.file)) byFile.set(f.file, []); + byFile.get(f.file).push(f); + } + for (const list of byFile.values()) list.sort((a, b) => a.line - b.line); + return byFile; +} + +/** Plain text — the default, and what an agent reading raw CI logs gets. */ +export function formatText(report) { + const lines = []; + const total = report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + + if (report.exitCode === 0) { + lines.push('strict-gate: PASS — no new strict-mode violations in this diff.'); + lines.push(''); + lines.push(` checked ${report.targets.length} project config(s) under ${report.flagSet}`); + lines.push(` ignored ${total} pre-existing/dependency diagnostic(s) outside this diff`); + if (report.unmapped.length > 0) { + lines.push(` unmapped ${report.unmapped.length} changed file(s) no project claimed (not a failure)`); + } + return lines.join('\n'); + } + + lines.push(`strict-gate: FAIL — ${report.findings.length} new strict-mode violation(s) introduced by this diff.`); + lines.push(''); + lines.push('WHY THIS FAILS'); + lines.push(` main is not strict yet, so these files compile today. This gate checks the code`); + lines.push(` THIS pull request writes against ${FLAG_SET_LABEL[report.flagSet] ?? report.flagSet},`); + lines.push(` so new code stops adding to the debt the strict-mode migration has to clear.`); + lines.push(''); + lines.push('SCOPE — READ BEFORE FIXING'); + lines.push(` ${scopeRule(report.granularity)}`); + lines.push(` ${total} diagnostic(s) from dependencies and untouched code were IGNORED on purpose.`); + lines.push(' Fix ONLY the violations listed below. Do not refactor surrounding code, do not'); + lines.push(' "clean up" the rest of the file, and do not edit any tsconfig to silence this.'); + lines.push(''); + lines.push('VIOLATIONS'); + + for (const [file, findings] of groupByFile(report.findings)) { + lines.push(''); + lines.push(` ${file}`); + for (const f of findings) { + lines.push(` ${f.line}:${f.column} ${f.code} ${f.message}`); + const hint = GUIDANCE[f.code]; + if (hint) lines.push(` fix: ${hint}`); + } + } + + lines.push(''); + lines.push('REPRODUCE LOCALLY'); + lines.push(' cd core-web'); + lines.push( + ` node tools/scripts/strict-gate/run.mjs --base origin/main --head HEAD ` + + `--flags ${report.flagSet} --granularity ${report.granularity}` + ); + return lines.join('\n'); +} + +/** GitHub Actions annotations — puts each violation inline on the pull request diff. */ +export function formatGithub(report) { + return report.findings + .map((f) => { + const hint = GUIDANCE[f.code] ? ` — ${GUIDANCE[f.code]}` : ''; + const message = `${f.code}: ${f.message}${hint}`.replace(/\r?\n/g, ' '); + return `::error file=${f.file},line=${f.line},col=${f.column},title=strict-gate ${f.code}::${message}`; + }) + .join('\n'); +} + +/** Markdown for the job summary — what a human opening the run sees first. */ +export function formatMarkdown(report) { + const total = report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + 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).` + ].join('\n'); + } + + const rows = report.findings + .map((f) => `| \`${f.file}\` | ${f.line}:${f.column} | \`${f.code}\` | ${f.message.replace(/\|/g, '\\|')} |`) + .join('\n'); + + return [ + `## ❌ strict-gate: ${report.findings.length} new strict-mode violation(s)`, + '', + `**Scope.** ${scopeRule(report.granularity)} ${total} diagnostic(s) from dependencies and untouched code were ignored — fix only what is listed.`, + '', + '| File | Line | Code | Message |', + '|---|---|---|---|', + rows, + '', + '
Reproduce locally', + '', + '```bash', + 'cd core-web', + `node tools/scripts/strict-gate/run.mjs --base origin/main --head HEAD --flags ${report.flagSet} --granularity ${report.granularity}`, + '```', + '', + '
' + ].join('\n'); +} + +export const FORMATTERS = { + text: formatText, + github: formatGithub, + markdown: formatMarkdown, + json: (report) => JSON.stringify(report, null, 2) +}; diff --git a/core-web/tools/scripts/strict-gate/lib/hunks.mjs b/core-web/tools/scripts/strict-gate/lib/hunks.mjs new file mode 100644 index 00000000000..65205412711 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/hunks.mjs @@ -0,0 +1,26 @@ +/** + * Parses added/modified line ranges out of a zero-context diff. + * + * Kept separate from the git plumbing because it is the one piece of pure logic in the + * changed-file path, and getting it wrong is silent in both directions: too-wide ranges make the + * gate blame untouched code, too-narrow ones make it miss real violations. + */ + +// Anchored to a line start and matched against the full header shape, so a "@@" that appears +// inside file content — a fixture string, a markdown table — is never mistaken for a hunk. +const HUNK = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/gm; + +/** + * @param {string} diff Output of `git diff --unified=0`. + * @returns {[number, number][]} 1-based inclusive spans of lines present at head. + */ +export function parseHunks(diff) { + const spans = []; + for (const match of diff.matchAll(HUNK)) { + const start = Number(match[1]); + const count = match[2] === undefined ? 1 : Number(match[2]); + // `+N,0` is a pure deletion: nothing was written there, so nothing is attributable. + if (count > 0) spans.push([start, start + count - 1]); + } + return spans; +} diff --git a/core-web/tools/scripts/strict-gate/lib/mode-select.mjs b/core-web/tools/scripts/strict-gate/lib/mode-select.mjs new file mode 100644 index 00000000000..cf805462c88 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/mode-select.mjs @@ -0,0 +1,67 @@ +/** + * Decides, per project, whether the Angular compiler runs — and always says why. + * + * The spec forbids a silent fallback, and the reason is concrete: a project that quietly drops to + * TypeScript-only has its templates unchecked while the run still reports PASS. That is + * indistinguishable from "the templates are fine", which is the failure a gate exists to prevent. + */ +import fs from 'node:fs/promises'; +import path from 'node:path'; + +/** Strips comments and trailing commas so a tsconfig with JSONC in it can be read. */ +function parseJsonc(text) { + const stripped = text + .replace(/\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g, (m, comment) => (comment ? '' : m)) + .replace(/,(\s*[}\]])/g, '$1'); + return JSON.parse(stripped); +} + +/** Walks the `extends` chain looking for `angularCompilerOptions`. */ +async function declaresAngular(configPath, seen = new Set()) { + const resolved = path.resolve(configPath); + if (seen.has(resolved)) return false; + seen.add(resolved); + + let config; + try { + config = parseJsonc(await fs.readFile(resolved, 'utf8')); + } catch { + return false; + } + if (config.angularCompilerOptions) return true; + if (!config.extends) return false; + + const parents = Array.isArray(config.extends) ? config.extends : [config.extends]; + for (const parent of parents) { + const candidate = parent.startsWith('.') + ? path.resolve(path.dirname(resolved), parent) + : null; + if (!candidate) continue; + const withExt = candidate.endsWith('.json') ? candidate : `${candidate}.json`; + if (await declaresAngular(withExt, seen)) return true; + } + return false; +} + +/** + * @param {{ configPath: string, templates?: boolean }} input + * @returns {Promise<{ mode: 'typescript'|'template-aware', reason: string }>} + */ +export async function selectMode({ configPath, templates = false }) { + if (!templates) { + return { + mode: 'typescript', + reason: 'template checking not requested (--templates off)' + }; + } + if (await declaresAngular(configPath)) { + return { + mode: 'template-aware', + reason: 'project declares angularCompilerOptions in its config chain' + }; + } + return { + mode: 'typescript', + reason: 'not an Angular project: no angularCompilerOptions found in the config chain' + }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/project-map.mjs b/core-web/tools/scripts/strict-gate/lib/project-map.mjs new file mode 100644 index 00000000000..8120a501be3 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/project-map.mjs @@ -0,0 +1,63 @@ +/** + * Maps changed files to the project that OWNS them. + * + * Deliberately not `nx affected`: that returns projects which DEPEND on what changed, and + * tsconfig.base.json / nx.json are declared under nx.json's sharedGlobals — so touching either + * makes all 56 projects affected. Ownership is a property of where a file lives (FR-010). + */ +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { run } from './exec.mjs'; +import { resolveBin } from './resolve-tools.mjs'; + +const owns = (root, filePath) => filePath === root || filePath.startsWith(`${root}/`); + +/** + * @param {{ projects: {name:string,root:string}[], files: object[] }} input + * @returns {{ targets: {project:string,root:string,files:string[]}[], unmapped: {path:string,reason:string}[] }} + */ +export function mapFilesToProjects({ projects, files }) { + // Longest root first so a nested project wins over its parent. + const ordered = [...projects].sort((a, b) => b.root.length - a.root.length); + const byProject = new Map(); + const unmapped = []; + + for (const file of files) { + const owner = ordered.find((p) => owns(p.root, file.path)); + if (!owner) { + unmapped.push({ + path: file.path, + reason: 'no project root is a path prefix of this file' + }); + continue; + } + if (!byProject.has(owner.name)) { + byProject.set(owner.name, { project: owner.name, root: owner.root, files: [] }); + } + byProject.get(owner.name).files.push(file.path); + } + + return { targets: [...byProject.values()], unmapped }; +} + +/** + * Reads project roots from the Nx graph — roots only, never the dependency edges. + * @returns {Promise<{name:string,root:string}[]>} roots relative to `repoDir`. + */ +export async function readProjects({ workspaceDir, repoDir }) { + const out = path.join(await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-graph-')), 'graph.json'); + await run('node', [resolveBin('nx'), 'graph', '--file', out], { cwd: workspaceDir }); + + const graph = JSON.parse(await fs.readFile(out, 'utf8')); + const nodes = graph.graph?.nodes ?? graph.nodes ?? {}; + const prefix = path.relative(repoDir, workspaceDir); + + const projects = Object.entries(nodes) + .map(([name, node]) => ({ name, root: node?.data?.root })) + .filter((p) => typeof p.root === 'string' && p.root.length > 0) + .map((p) => ({ name: p.name, root: prefix ? path.join(prefix, p.root) : p.root })); + + await fs.rm(path.dirname(out), { recursive: true, force: true }); + return projects; +} diff --git a/core-web/tools/scripts/strict-gate/lib/report.mjs b/core-web/tools/scripts/strict-gate/lib/report.mjs new file mode 100644 index 00000000000..978afd3b82f --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/report.mjs @@ -0,0 +1,62 @@ +/** + * Assembles the run report. Shape is pinned by contracts/report.schema.json so the follow-up task + * inherits a stable interface rather than whatever this implementation happened to emit. + */ + +const key = (f) => `${f.file}|${f.line}|${f.code}`; + +/** + * A diagnostic reported under two configurations is ONE defect. Real case: src/utils/index.ts in + * sdk-create-app reports TS7030 under both the lib and the spec configuration. + */ +export function dedupe(findings) { + const seen = new Map(); + for (const finding of findings) if (!seen.has(key(finding))) seen.set(key(finding), finding); + return [...seen.values()]; +} + +export function buildReport({ + base, + head, + flagSet, + granularity, + targets = [], + unmapped = [], + findings = [], + discarded = { + byOrigin: { dependency: 0, untouched: 0, infrastructure: 0 }, + byLayer: { source: 0, template: 0 } + }, + durationMs = { total: 0, typescript: 0, templateAware: 0 } +}) { + const unique = dedupe(findings); + return { + base, + head, + flagSet, + granularity, + targets: targets.map((t) => ({ + project: t.project, + root: t.root, + configPath: t.configPath, + mode: t.mode ?? 'typescript', + files: t.files + })), + unmapped: unmapped.map((u) => ({ path: u.path, reason: u.reason })), + findings: unique.map((f) => ({ + file: f.file, + line: f.line, + column: f.column, + code: f.code, + message: f.message, + origin: f.origin ?? 'changed', + layer: f.layer ?? 'source' + })), + discarded, + durationMs, + // The gate's whole output in one integer. Exit 2 (harness failure) is assigned by run.mjs + // and never conflated with 1 — a broken harness reporting "clean" is the one failure mode + // that would quietly defeat the gate. + exitCode: unique.length > 0 ? 1 : 0 + }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs b/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs new file mode 100644 index 00000000000..b9306b6f91c --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs @@ -0,0 +1,82 @@ +/** + * Resolves the compiler toolchain from the workspace, never from a version pinned here. + * + * The harness deliberately declares no dependency of its own: whatever TypeScript and Angular + * compiler the workspace is on is what the gate must measure against. Pinning a version inside + * the harness would let it drift from the code it checks, which is the one way its numbers could + * be quietly wrong. + */ +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; + +/** `core-web/` — the workspace root the harness resolves everything relative to. */ +export const workspaceRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '../../../..' +); + +const requireFromWorkspace = createRequire(path.join(workspaceRoot, 'package.json')); + +/** + * @param {string} specifier + * @returns {{ name: string, version: string, path: string }} + */ +function describe(specifier) { + const pkgPath = requireFromWorkspace.resolve(`${specifier}/package.json`); + const pkg = requireFromWorkspace(`${specifier}/package.json`); + return { name: specifier, version: pkg.version, path: path.dirname(pkgPath) }; +} + +/** Loads the workspace TypeScript. Throws a directed message when dependencies are missing. */ +export async function loadTypeScript() { + try { + return (await import(requireFromWorkspace.resolve('typescript'))).default; + } catch (cause) { + throw new Error( + `Cannot resolve 'typescript' from ${workspaceRoot}. Run 'pnpm install' in core-web/.`, + { cause } + ); + } +} + +/** Loads the workspace Angular compiler. Only needed by template-aware mode. */ +export async function loadAngularCompiler() { + try { + return await import(requireFromWorkspace.resolve('@angular/compiler-cli')); + } catch (cause) { + throw new Error( + `Cannot resolve '@angular/compiler-cli' from ${workspaceRoot}. Run 'pnpm install' in core-web/.`, + { cause } + ); + } +} + +/** + * Resolves an executable a package declares in its `bin` field. + * + * Never assume a conventional path: nx declares `./dist/bin/nx.js`, not `bin/nx.js`, and under + * pnpm the package lives inside `.pnpm//`. Guessing breaks on either. + * + * @param {string} specifier Package name, e.g. 'nx'. + * @param {string} [binName] Bin entry; defaults to the package name. + */ +export function resolveBin(specifier, binName = specifier) { + const pkgPath = requireFromWorkspace.resolve(`${specifier}/package.json`); + const { bin } = requireFromWorkspace(`${specifier}/package.json`); + const entry = typeof bin === 'string' ? bin : bin?.[binName]; + if (!entry) throw new Error(`package '${specifier}' declares no bin '${binName}'`); + return path.resolve(path.dirname(pkgPath), entry); +} + +/** + * Reports what the harness is actually running against. Recorded in the report so a measurement + * can always be traced back to the toolchain that produced it. + */ +export function toolchainInfo() { + return { + node: process.version, + typescript: describe('typescript').version, + angularCompiler: describe('@angular/compiler-cli').version + }; +} diff --git a/core-web/tools/scripts/strict-gate/lib/validate-report.mjs b/core-web/tools/scripts/strict-gate/lib/validate-report.mjs new file mode 100644 index 00000000000..47386cf019e --- /dev/null +++ b/core-web/tools/scripts/strict-gate/lib/validate-report.mjs @@ -0,0 +1,136 @@ +/** + * Validates a run report against contracts/report.schema.json. + * + * Hand-rolled rather than pulled from a library because the harness adds no dependency, and the + * schema uses a small, closed subset of JSON Schema. It covers exactly that subset and throws on + * anything it does not understand — a validator that silently ignores a keyword it cannot handle + * would report "valid" for a report it never actually checked. + */ +import fs from 'node:fs'; +import path from 'node:path'; +import { workspaceRoot } from './resolve-tools.mjs'; + +export const SCHEMA_PATH = path.join( + workspaceRoot, + '..', + 'specs/37401-diff-scoped-strict-typecheck-gate/contracts/report.schema.json' +); + +const SUPPORTED = new Set([ + '$schema', '$id', 'title', 'description', '$defs', + 'type', 'enum', 'const', 'properties', 'required', 'additionalProperties', + 'items', 'pattern', 'minimum', 'maximum', 'minItems', '$ref' +]); + +function typeOf(value) { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'array'; + if (Number.isInteger(value)) return 'integer'; + return typeof value; +} + +function resolveRef(ref, root) { + if (!ref.startsWith('#/')) throw new Error(`Unsupported $ref: ${ref}`); + return ref + .slice(2) + .split('/') + .reduce((node, key) => { + if (node === undefined) throw new Error(`Unresolvable $ref: ${ref}`); + return node[key]; + }, root); +} + +function check(value, schema, root, at, errors) { + for (const keyword of Object.keys(schema)) { + if (!SUPPORTED.has(keyword)) { + throw new Error(`validate-report does not implement JSON Schema keyword '${keyword}'`); + } + } + + if (schema.$ref) { + check(value, resolveRef(schema.$ref, root), root, at, errors); + return; + } + + if (schema.type) { + const actual = typeOf(value); + const ok = schema.type === 'number' ? actual === 'number' || actual === 'integer' : actual === schema.type; + if (!ok) { + errors.push(`${at}: expected ${schema.type}, got ${actual}`); + return; + } + } + + if (schema.enum && !schema.enum.includes(value)) { + errors.push(`${at}: ${JSON.stringify(value)} is not one of ${JSON.stringify(schema.enum)}`); + } + if (schema.const !== undefined && value !== schema.const) { + errors.push(`${at}: expected ${JSON.stringify(schema.const)}`); + } + if (schema.pattern && typeof value === 'string' && !new RegExp(schema.pattern).test(value)) { + errors.push(`${at}: ${JSON.stringify(value)} does not match /${schema.pattern}/`); + } + if (schema.minimum !== undefined && typeof value === 'number' && value < schema.minimum) { + errors.push(`${at}: ${value} < minimum ${schema.minimum}`); + } + if (schema.maximum !== undefined && typeof value === 'number' && value > schema.maximum) { + errors.push(`${at}: ${value} > maximum ${schema.maximum}`); + } + + if (typeOf(value) === 'array') { + if (schema.minItems !== undefined && value.length < schema.minItems) { + errors.push(`${at}: expected at least ${schema.minItems} items`); + } + if (schema.items) { + value.forEach((item, i) => check(item, schema.items, root, `${at}[${i}]`, errors)); + } + } + + if (typeOf(value) === 'object') { + for (const key of schema.required ?? []) { + if (!(key in value)) errors.push(`${at}: missing required property '${key}'`); + } + if (schema.additionalProperties === false && schema.properties) { + for (const key of Object.keys(value)) { + if (!(key in schema.properties)) { + errors.push(`${at}: unexpected property '${key}'`); + } + } + } + for (const [key, sub] of Object.entries(schema.properties ?? {})) { + if (key in value) check(value[key], sub, root, `${at}.${key}`, errors); + } + } +} + +/** + * @param {unknown} report + * @param {object} [schema] Defaults to the published contract. + * @returns {{ valid: boolean, errors: string[] }} + */ +export function validateReport(report, schema = loadSchema()) { + const errors = []; + check(report, schema, schema, 'report', errors); + + // Invariants the schema alone cannot express (data-model.md, RunReport). + if (errors.length === 0) { + const failing = (report.findings?.length ?? 0) > 0; + if (failing && report.exitCode === 0) { + errors.push('report.exitCode: must be non-zero when findings is non-empty'); + } + if (!failing && report.exitCode !== 0) { + errors.push('report.exitCode: must be 0 when findings is empty'); + } + for (const [i, finding] of (report.findings ?? []).entries()) { + if (finding.origin !== 'changed') { + errors.push(`report.findings[${i}].origin: survivors must be 'changed'`); + } + } + } + + return { valid: errors.length === 0, errors }; +} + +export function loadSchema() { + return JSON.parse(fs.readFileSync(SCHEMA_PATH, 'utf8')); +} diff --git a/core-web/tools/scripts/strict-gate/mode-select.test.mjs b/core-web/tools/scripts/strict-gate/mode-select.test.mjs new file mode 100644 index 00000000000..0a914e82d4f --- /dev/null +++ b/core-web/tools/scripts/strict-gate/mode-select.test.mjs @@ -0,0 +1,56 @@ +/** + * T051 (US4) — choosing the execution mode, out loud. + * + * The spec is emphatic that a fallback must never be silent, and the reason is concrete: if a + * project quietly drops to TypeScript-only, its templates go unchecked and the run still reports + * PASS. That is indistinguishable from "the templates are fine", which is exactly the failure a + * gate exists to prevent. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import path from 'node:path'; +import { makeWorkspace } from './fixtures/make-workspace.mjs'; +import { makeNgProject } from './fixtures/make-ng-project.mjs'; +import { selectMode } from './lib/mode-select.mjs'; + +test('an Angular project selects template-aware mode', async (t) => { + const ng = await makeNgProject({ strictTemplates: false }); + t.after(() => ng.cleanup()); + + const decision = await selectMode({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + templates: true + }); + + assert.equal(decision.mode, 'template-aware'); + assert.match(decision.reason, /\S/); +}); + +test('a non-Angular project falls back to TypeScript-only, and says so', async (t) => { + const ws = await makeWorkspace({ + projects: [{ name: 'plain', root: 'libs/plain', files: { 'src/index.ts': 'export const a = 1;\n' } }] + }); + t.after(() => ws.cleanup()); + + const decision = await selectMode({ + configPath: path.join(ws.dir, 'libs/plain/tsconfig.lib.json'), + templates: true + }); + + assert.equal(decision.mode, 'typescript'); + // Asserted on the REPORTED value, not on the absence of a crash: a silent skip would pass a + // test that only checked that nothing threw. + assert.match(decision.reason, /angular/i, 'the fallback must state why it happened'); +}); + +test('template-aware mode is never selected when templates are not requested', async (t) => { + const ng = await makeNgProject({ strictTemplates: false }); + t.after(() => ng.cleanup()); + + const decision = await selectMode({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + templates: false + }); + + assert.equal(decision.mode, 'typescript', 'the core arm stays independent of the template arm'); +}); diff --git a/core-web/tools/scripts/strict-gate/project-map.test.mjs b/core-web/tools/scripts/strict-gate/project-map.test.mjs new file mode 100644 index 00000000000..98d44d40b69 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/project-map.test.mjs @@ -0,0 +1,74 @@ +/** + * T011 — mapping changed files to the project that OWNS them. + * + * Ownership is a property of where a file lives, not of the dependency graph. `nx affected` + * answers a different question — it returns dependents — and for a shared config that is every + * project in the workspace. FR-010 exists because that difference is the whole cost model. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mapFilesToProjects } from './lib/project-map.mjs'; + +const PROJECTS = [ + { name: 'ui', root: 'core-web/libs/ui' }, + { name: 'portlet', root: 'core-web/libs/portlets/thing' }, + { name: 'portlet-ui', root: 'core-web/libs/portlets/thing/ui' }, + { name: 'core-web', root: 'core-web' } +]; + +const asChanged = (paths) => paths.map((p) => ({ path: p, status: 'M', kind: 'source', changedLines: [] })); + +test('assigns each file to the longest matching project root', () => { + const { targets } = mapFilesToProjects({ + projects: PROJECTS, + files: asChanged([ + 'core-web/libs/ui/src/a.ts', + 'core-web/libs/portlets/thing/src/b.ts', + 'core-web/libs/portlets/thing/ui/src/c.ts' + ]) + }); + + const owner = (p) => targets.find((t) => t.files.includes(p))?.project; + assert.equal(owner('core-web/libs/ui/src/a.ts'), 'ui'); + assert.equal(owner('core-web/libs/portlets/thing/src/b.ts'), 'portlet'); + // The nested project wins over its parent — otherwise every nested lib's files would be + // checked under the wrong configuration. + assert.equal(owner('core-web/libs/portlets/thing/ui/src/c.ts'), 'portlet-ui'); +}); + +test('reports a file no project claims instead of dropping it', () => { + const { targets, unmapped } = mapFilesToProjects({ + projects: PROJECTS.filter((p) => p.name !== 'core-web'), + files: asChanged(['docs/readme.ts', 'core-web/libs/ui/src/a.ts']) + }); + + assert.equal(targets.length, 1); + assert.equal(unmapped.length, 1); + assert.equal(unmapped[0].path, 'docs/readme.ts'); + assert.match(unmapped[0].reason, /\S/, 'an unmapped file must say why'); +}); + +test('a shared-config change does not fan out to every project', () => { + // tsconfig.base.json and nx.json are declared under nx.json's sharedGlobals, so `nx affected` + // returns all 56 projects for this diff. The gate must stay on the owning project. + const { targets } = mapFilesToProjects({ + projects: PROJECTS, + files: asChanged(['core-web/tsconfig.base.json', 'core-web/nx.json']) + }); + + assert.ok(targets.length <= 1, `expected no fan-out, got ${targets.length} targets`); + for (const target of targets) { + assert.notEqual(target.project, 'ui'); + assert.notEqual(target.project, 'portlet'); + } +}); + +test('groups multiple files of one project into a single target', () => { + const { targets } = mapFilesToProjects({ + projects: PROJECTS, + files: asChanged(['core-web/libs/ui/src/a.ts', 'core-web/libs/ui/src/b.ts']) + }); + + assert.equal(targets.length, 1); + assert.equal(targets[0].files.length, 2); +}); diff --git a/core-web/tools/scripts/strict-gate/replay.mjs b/core-web/tools/scripts/strict-gate/replay.mjs new file mode 100644 index 00000000000..11436e27060 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/replay.mjs @@ -0,0 +1,182 @@ +#!/usr/bin/env node +/** + * Replays merged pull requests through the gate — the spike's evidence engine. + */ +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; +import { run, git } from './lib/exec.mjs'; +import { workspaceRoot } from './lib/resolve-tools.mjs'; +import { runGate, collectDiagnostics, reportFrom } from './run.mjs'; +import { CORPUS, adjudicate, summarize } from './corpus.mjs'; + +const DEFAULT_REPO_DIR = path.resolve(workspaceRoot, '..'); + +async function nameWithOwner(repoDir) { + const { stdout } = await git(['-C', repoDir, 'remote', 'get-url', 'origin']); + const match = stdout.trim().match(/[:/]([^/:]+\/[^/]+?)(?:\.git)?$/); + if (!match) throw new Error(`cannot derive owner/repo from origin '${stdout.trim()}'`); + return match[1]; +} + +/** + * Base is the merge commit's FIRST PARENT: `main` exactly as it stood before the merge, so + * `M^1..M` is precisely what this pull request added. No merge-base computation, and no + * dependency on a branch that was deleted after merge. + */ +export async function resolveMergeRange({ repoDir = DEFAULT_REPO_DIR, pr }) { + const repo = await nameWithOwner(repoDir); + const { stdout } = await run('gh', [ + 'pr', 'view', String(pr), '--repo', repo, '--json', 'mergeCommit,state', '--jq', + '"\\(.state) \\(.mergeCommit.oid // "none")"' + ]); + const [state, oid] = stdout.trim().replace(/^"|"$/g, '').split(' '); + if (state !== 'MERGED' || !oid || oid === 'none') { + throw new Error(`pull request #${pr} is ${state} with no merge commit — cannot replay it`); + } + + const { stdout: parents } = await git(['-C', repoDir, 'rev-list', '--parents', '-n', '1', oid]); + const [, firstParent] = parents.trim().split(' '); + if (!firstParent) throw new Error(`commit ${oid} has no parent — cannot derive a base`); + + return { base: firstParent, head: oid }; +} + +function summaryTable(results) { + const pad = (v, n) => String(v).padEnd(n); + const lines = [ + '', + `${pad('PR', 9)}${pad('expected', 10)}${pad('findings', 10)}${pad('discarded', 11)}${pad('ms', 8)}verdict`, + '-'.repeat(60) + ]; + for (const { sample, report, verdict } of results) { + const discarded = report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + lines.push( + pad(`#${sample.pr}`, 9) + + pad(sample.expectation, 10) + + pad(report.findings.length, 10) + + pad(discarded, 11) + + pad(Math.round(report.durationMs.total), 8) + + (verdict.matchedExpectation ? 'as predicted' : 'MISMATCH — adjudicate') + ); + } + const s = summarize(results.map(({ sample, verdict }) => ({ sample, verdict }))); + lines.push(''); + lines.push(`clean cases: ${s.cleanCases} with findings: ${s.cleanCasesWithFindings} ` + + `false-positive rate: ${s.falsePositiveRate === null ? 'n/a' : s.falsePositiveRate}`); + lines.push(`debt cases: ${s.debtCases} detected: ${s.debtCasesDetected} ` + + `findings needing adjudication: ${s.unexpectedFindings}`); + lines.push(s.caveat); + return lines.join('\n'); +} + +const FLAG_SETS = ['strict', 'null-checks', 'strict-max']; +const GRANULARITIES = ['file', 'line']; + +/** + * Every combination of flag set and granularity over identical input (FR-007, FR-008). + * + * Compiles once per (pull request, flag set) and filters twice: granularity only affects the + * filter, so paying for a second identical compilation would double the matrix's cost for nothing. + */ +async function runMatrix(cases) { + const rows = []; + for (const sample of cases) { + const { base, head } = await resolveMergeRange({ pr: sample.pr }); + for (const flagSet of FLAG_SETS) { + const collected = await collectDiagnostics({ base, head, flagSet }); + for (const granularity of GRANULARITIES) { + const report = reportFrom(collected, { flagSet, granularity }); + rows.push({ pr: sample.pr, flagSet, granularity, report }); + } + } + } + return rows; +} + +function matrixTable(rows) { + const prs = [...new Set(rows.map((r) => r.pr))]; + const out = ['', 'findings by flag set x granularity', '']; + out.push(`${'PR'.padEnd(9)}${FLAG_SETS.map((f) => `${f}/file`.padEnd(16) + `${f}/line`.padEnd(16)).join('')}`); + out.push('-'.repeat(9 + FLAG_SETS.length * 32)); + for (const pr of prs) { + let line = `#${pr}`.padEnd(9); + for (const flagSet of FLAG_SETS) { + for (const granularity of GRANULARITIES) { + const row = rows.find((r) => r.pr === pr && r.flagSet === flagSet && r.granularity === granularity); + line += String(row?.report.findings.length ?? '-').padEnd(16); + } + } + out.push(line); + } + + out.push(''); + out.push('totals'); + for (const flagSet of FLAG_SETS) { + for (const granularity of GRANULARITIES) { + const subset = rows.filter((r) => r.flagSet === flagSet && r.granularity === granularity); + const findings = subset.reduce((n, r) => n + r.report.findings.length, 0); + const ms = Math.round(subset.reduce((n, r) => n + r.report.durationMs.total, 0) / subset.length); + out.push(` ${(flagSet + '/' + granularity).padEnd(22)}${String(findings).padStart(4)} findings ${String(ms).padStart(6)} ms avg`); + } + } + + // The adoption cost, which is the whole granularity argument: how much pre-existing debt does + // whole-file make an author inherit for touching the file at all? + const inherited = FLAG_SETS.map((flagSet) => { + const f = rows.filter((r) => r.flagSet === flagSet && r.granularity === 'file') + .reduce((n, r) => n + r.report.findings.length, 0); + const l = rows.filter((r) => r.flagSet === flagSet && r.granularity === 'line') + .reduce((n, r) => n + r.report.findings.length, 0); + return ` ${flagSet.padEnd(22)}${String(f - l).padStart(4)} extra findings inherited from untouched lines (${f} vs ${l})`; + }); + out.push('', 'whole-file adoption cost', ...inherited); + return out.join('\n'); +} + +async function main(argv) { + const prs = []; + const passthrough = {}; + for (let i = 0; i < argv.length; i += 1) { + const [flag, inline] = argv[i].split('='); + const value = inline ?? argv[i + 1]; + const consume = () => { if (inline === undefined) i += 1; }; + if (flag === '--pr') { prs.push(...value.split(',').map(Number)); consume(); } + else if (flag === '--all') { /* run the whole corpus */ } + else if (flag === '--matrix') { passthrough.matrix = true; } + else if (flag === '--flags') { passthrough.flagSet = value; consume(); } + else if (flag === '--granularity') { passthrough.granularity = value; consume(); } + else if (flag === '--templates') { passthrough.templates = value === 'on'; consume(); } + else if (flag === '--report') { consume(); } + else throw new Error(`unknown option '${flag}' — see contracts/cli.md`); + } + const cases = prs.length > 0 ? prs.map((pr) => CORPUS.find((c) => c.pr === pr) ?? { pr, expectation: 'debt', rationale: 'ad-hoc', knownFindings: [] }) : CORPUS; + + if (passthrough.matrix) { + process.stdout.write(`${matrixTable(await runMatrix(cases))}\n`); + return 0; + } + + const results = []; + for (const sample of cases) { + const { base, head } = await resolveMergeRange({ pr: sample.pr }); + const { matrix, ...gateOptions } = passthrough; + const report = await runGate({ base, head, ...gateOptions }); + results.push({ sample, report, verdict: adjudicate(sample, report, report.granularity) }); + } + + process.stdout.write(`${summaryTable(results)}\n`); + + // A mismatch is information, not a defect: every report is still produced so the per-finding + // adjudication SC-003 requires can proceed. + return results.every((r) => r.verdict.matchedExpectation) ? 0 : 1; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main(process.argv.slice(2)).then( + (code) => process.exit(code), + (error) => { + process.stderr.write(`strict-gate replay: ${error.message}\n`); + process.exit(2); + } + ); +} diff --git a/core-web/tools/scripts/strict-gate/report.contract.test.mjs b/core-web/tools/scripts/strict-gate/report.contract.test.mjs new file mode 100644 index 00000000000..ad5e380e3ca --- /dev/null +++ b/core-web/tools/scripts/strict-gate/report.contract.test.mjs @@ -0,0 +1,115 @@ +/** + * T015 — the report and exit-code contract. + * + * The follow-up task inherits these two interfaces, so they are pinned here rather than left to + * whatever the implementation happens to emit. Contract: contracts/cli.md + report.schema.json. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { validateReport } from './lib/validate-report.mjs'; +import { buildReport } from './lib/report.mjs'; + +const SHA_A = 'a'.repeat(40); +const SHA_B = 'b'.repeat(40); + +const baseInput = { + base: SHA_A, + head: SHA_B, + flagSet: 'strict', + granularity: 'file', + targets: [], + unmapped: [], + findings: [], + discarded: { + byOrigin: { dependency: 0, untouched: 0, infrastructure: 0 }, + byLayer: { source: 0, template: 0 } + }, + durationMs: { total: 1, typescript: 1, templateAware: 0 } +}; + +const finding = { + file: 'libs/mine/src/a.ts', + line: 11, + column: 3, + code: 'TS2345', + message: 'nope', + origin: 'changed', + layer: 'source' +}; + +test('an empty diff yields a valid report that passes', () => { + const report = buildReport(baseInput); + assert.deepEqual(validateReport(report), { valid: true, errors: [] }); + assert.equal(report.exitCode, 0); + assert.deepEqual(report.findings, []); +}); + +test('exit code is non-zero if and only if there are findings', () => { + const failing = buildReport({ ...baseInput, findings: [finding] }); + assert.notEqual(failing.exitCode, 0); + assert.deepEqual(validateReport(failing), { valid: true, errors: [] }); + + const passing = buildReport(baseInput); + assert.equal(passing.exitCode, 0); +}); + +test('every surviving finding carries origin "changed"', () => { + const report = buildReport({ ...baseInput, findings: [finding] }); + for (const f of report.findings) assert.equal(f.origin, 'changed'); +}); + +test('deduplicates a diagnostic reported by two configurations', () => { + // Real case: src/utils/index.ts in sdk-create-app reports TS7030 under both the lib and the + // spec configuration. One defect, one finding. + const report = buildReport({ ...baseInput, findings: [finding, { ...finding }] }); + assert.equal(report.findings.length, 1); +}); + +test('rejects a report whose exit code contradicts its findings', () => { + const broken = { ...buildReport({ ...baseInput, findings: [finding] }), exitCode: 0 }; + const { valid, errors } = validateReport(broken); + assert.equal(valid, false); + assert.match(errors.join('\n'), /exitCode/); +}); + +test('records the toolchain the measurement was produced with', () => { + const report = buildReport(baseInput); + assert.ok(report.durationMs.total >= 0); + assert.ok('typescript' in report.durationMs && 'templateAware' in report.durationMs); +}); + +/* ── T040 (US3) — the report must carry the settings that produced it ─────── + * Every number in findings.md is quoted alongside a flag set and a granularity. A report that + * does not say which produced it cannot be compared with another one, and the decision matrix + * SC-007 asks for is exactly a comparison across those two axes. + */ + +test('the report echoes the flag set and granularity it ran under', () => { + for (const flagSet of ['strict', 'null-checks', 'strict-max']) { + for (const granularity of ['file', 'line']) { + const report = buildReport({ ...baseInput, flagSet, granularity }); + assert.equal(report.flagSet, flagSet); + assert.equal(report.granularity, granularity); + assert.deepEqual(validateReport(report), { valid: true, errors: [] }); + } + } +}); + +test('durationMs separates the two execution modes so their costs can be compared', () => { + const report = buildReport({ + ...baseInput, + durationMs: { total: 12000, typescript: 9000, templateAware: 3000 } + }); + + assert.equal(report.durationMs.typescript, 9000); + assert.equal(report.durationMs.templateAware, 3000); + assert.ok(report.durationMs.total >= report.durationMs.typescript); +}); + +test('the base and head recorded are the resolved SHAs, not the refs asked for', () => { + // findings.md cites results by pull request; those must be traceable to exact commits, since + // origin/main moves and a re-run months later has to reproduce the same numbers. + const report = buildReport(baseInput); + assert.match(report.base, /^[0-9a-f]{40}$/); + assert.match(report.head, /^[0-9a-f]{40}$/); +}); diff --git a/core-web/tools/scripts/strict-gate/run.mjs b/core-web/tools/scripts/strict-gate/run.mjs new file mode 100644 index 00000000000..386c46e4b3c --- /dev/null +++ b/core-web/tools/scripts/strict-gate/run.mjs @@ -0,0 +1,212 @@ +#!/usr/bin/env node +/** + * strict-gate — diff-scoped strict typecheck. Spike harness for issue #37401. + * Command contract: specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md + */ +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { workspaceRoot } from './lib/resolve-tools.mjs'; +import { resolveChangedFiles } from './lib/changed-files.mjs'; +import { mapFilesToProjects, readProjects } from './lib/project-map.mjs'; +import { selectConfigs } from './lib/config-select.mjs'; +import { checkTypeScript } from './lib/check-ts.mjs'; +import { checkAngularTemplates } from './lib/check-ng.mjs'; +import { selectMode } from './lib/mode-select.mjs'; +import { filterDiagnostics } from './lib/filter.mjs'; +import { buildReport } from './lib/report.mjs'; +import { FORMATTERS } from './lib/format.mjs'; + +const DEFAULT_REPO_DIR = path.resolve(workspaceRoot, '..'); + +/** + * @param {{ repoDir?: string, base: string, head?: string, + * flagSet?: string, granularity?: 'file'|'line', templates?: boolean }} options + */ +/** + * Everything expensive: resolve the diff, map it, and compile. Granularity is NOT an input here — + * it only affects filtering, so the matrix can compile once and filter twice instead of paying + * for a second identical compilation. + */ +export async function collectDiagnostics({ + repoDir = DEFAULT_REPO_DIR, + base, + head = 'HEAD', + flagSet = 'strict', + templates = false, + scope = 'core-web' +} = {}) { + const started = performance.now(); + let typescriptMs = 0; + let templateAwareMs = 0; + + const { files: allFiles, base: baseSha, head: headSha } = await resolveChangedFiles({ repoDir, base, head }); + const files = scope ? allFiles.filter((f) => f.path === scope || f.path.startsWith(`${scope}/`)) : allFiles; + + if (files.length === 0) { + return { + files, baseSha, headSha, targets: [], unmapped: [], diagnostics: [], projectRoots: [], + durationMs: { total: performance.now() - started, typescript: 0, templateAware: 0 } + }; + } + + const projects = await readProjects({ workspaceDir: workspaceRoot, repoDir }); + const { targets, unmapped } = mapFilesToProjects({ projects, files }); + + const resolvedTargets = []; + const diagnostics = []; + + for (const target of targets) { + const configs = await selectConfigs({ + workspaceDir: workspaceRoot, + repoDir, + project: { name: target.project, root: target.root }, + files: target.files + }); + + if (configs.length === 0) { + for (const file of target.files) { + unmapped.push({ + path: file, + reason: `project '${target.project}' has no configuration that includes this file` + }); + } + continue; + } + + for (const config of configs) { + // Reported, never assumed: a project that falls back to TypeScript-only appears in the + // report as having done so, because a silent fallback means unchecked templates behind + // a PASS. + const decision = await selectMode({ configPath: config.configPath, templates }); + const started = performance.now(); + + const { diagnostics: raw } = + decision.mode === 'template-aware' + ? await checkAngularTemplates({ configPath: config.configPath, flagSet }) + : await checkTypeScript({ + workspaceDir: workspaceRoot, + configPath: config.configPath, + flagSet + }); + + const elapsed = performance.now() - started; + if (decision.mode === 'template-aware') templateAwareMs += elapsed; + else typescriptMs += elapsed; + + resolvedTargets.push({ + ...config, + configPath: path.relative(repoDir, config.configPath), + mode: decision.mode + }); + diagnostics.push(...raw.map((d) => ({ ...d, file: path.relative(repoDir, d.file) }))); + } + } + + return { + files, baseSha, headSha, + targets: resolvedTargets, + unmapped, + diagnostics, + projectRoots: targets.map((t) => t.root), + durationMs: { + total: performance.now() - started, + typescript: typescriptMs, + templateAware: templateAwareMs + } + }; +} + +/** Builds one report from a collected pass, at a given granularity. */ +export function reportFrom(collected, { flagSet, granularity }) { + const { findings, discarded } = filterDiagnostics({ + diagnostics: collected.diagnostics, + changedFiles: collected.files, + granularity, + projectRoots: collected.projectRoots + }); + return buildReport({ + base: collected.baseSha, + head: collected.headSha, + flagSet, + granularity, + targets: collected.targets, + unmapped: collected.unmapped, + findings, + discarded, + durationMs: collected.durationMs + }); +} + +export async function runGate({ + repoDir = DEFAULT_REPO_DIR, + base, + head = 'HEAD', + flagSet = 'strict', + granularity = 'line', + templates = false, + // Hard scope. The gate is a frontend concern: a pull request that touches only backend code + // must be a no-op, and the harness must never wander outside core-web even if a stray .ts + // exists elsewhere in the repo. CI additionally gates the whole job on the same path filter. + scope = 'core-web' +} = {}) { + const collected = await collectDiagnostics({ repoDir, base, head, flagSet, templates, scope }); + return reportFrom(collected, { flagSet, granularity }); +} + +function parseArgs(argv) { + const options = {}; + for (let i = 0; i < argv.length; i += 1) { + const [flag, inlineValue] = argv[i].split('='); + const value = inlineValue ?? argv[i + 1]; + const consume = () => { + if (inlineValue === undefined) i += 1; + }; + switch (flag) { + case '--base': options.base = value; consume(); break; + case '--head': options.head = value; consume(); break; + case '--flags': options.flagSet = value; consume(); break; + case '--granularity': options.granularity = value; consume(); break; + case '--templates': options.templates = value === 'on'; consume(); break; + case '--report': options.report = value; consume(); break; + case '--format': options.format = value; consume(); break; + case '--scope': options.scope = value === 'none' ? null : value; consume(); break; + default: throw new Error(`unknown option '${flag}' — see contracts/cli.md`); + } + } + return options; +} + +async function main(argv) { + const { report: reportPath, format = 'text', ...options } = parseArgs(argv); + if (!options.base) throw new Error('--base is required'); + + const render = FORMATTERS[format]; + if (!render) throw new Error(`unknown format '${format}' — one of ${Object.keys(FORMATTERS).join(', ')}`); + + const report = await runGate(options); + + // The JSON is the machine record; the chosen format is what a reader (human or agent) acts on. + if (reportPath && reportPath !== '-') { + const { writeFile } = await import('node:fs/promises'); + await writeFile(reportPath, JSON.stringify(report, null, 2), 'utf8'); + } + process.stdout.write(`${render(report)}\n`); + + // GitHub Actions: annotations go to the log, the summary goes to the run page. + if (process.env.GITHUB_STEP_SUMMARY && format === 'github') { + const { appendFile } = await import('node:fs/promises'); + await appendFile(process.env.GITHUB_STEP_SUMMARY, `${FORMATTERS.markdown(report)}\n`, 'utf8'); + } + return report.exitCode; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main(process.argv.slice(2)).then( + (code) => process.exit(code), + (error) => { + // Exit 2, never 1: a harness that could not run must never look like a clean gate. + process.stderr.write(`strict-gate: ${error.message}\n`); + process.exit(2); + } + ); +} diff --git a/core-web/tools/scripts/strict-gate/strict-override.test.mjs b/core-web/tools/scripts/strict-gate/strict-override.test.mjs new file mode 100644 index 00000000000..883f58713a5 --- /dev/null +++ b/core-web/tools/scripts/strict-gate/strict-override.test.mjs @@ -0,0 +1,201 @@ +/** + * T013 — strictness is genuinely in force despite an inherited `strict: false`. + * + * This is the premise the whole spike rests on (FR-003). If it ever stops holding — a TypeScript + * upgrade changing option precedence, say — everything downstream reports zero findings and looks + * healthy. That is why it is asserted directly rather than inferred from the end-to-end result. + */ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { makeWorkspace } from './fixtures/make-workspace.mjs'; +import { checkTypeScript } from './lib/check-ts.mjs'; +import { checkAngularTemplates } from './lib/check-ng.mjs'; +import { makeNgProject } from './fixtures/make-ng-project.mjs'; + +const violations = { + name: 'loose', + root: 'libs/loose', + files: { + 'src/index.ts': [ + 'export function implicitAny(value) {', // TS7006 under noImplicitAny + ' return value;', + '}', + '', + 'export function possiblyNull(input: string | null) {', + ' return input.length;', // TS18047 under strictNullChecks + '}', + '', + 'export function fromIndexSignature(env: Record) {', + ' return env.CI;', // TS4111 — NOT part of --strict + '}', + '' + ].join('\n') + } +}; + +test('forces strict on a project whose base config sets strict: false', async (t) => { + const ws = await makeWorkspace({ projects: [violations], strict: false }); + t.after(() => ws.cleanup()); + + const { diagnostics } = await checkTypeScript({ + workspaceDir: ws.dir, + configPath: path.join(ws.dir, 'libs/loose/tsconfig.lib.json'), + flagSet: 'strict' + }); + + const codes = diagnostics.map((d) => d.code); + assert.ok(codes.includes('TS7006'), `expected an implicit-any error, got ${codes.join(', ')}`); + assert.ok(codes.includes('TS18047'), `expected a possibly-null error, got ${codes.join(', ')}`); +}); + +test("the repo's strict convention includes noPropertyAccessFromIndexSignature", async (t) => { + // Measured, not assumed: TS4111 is NOT one of the flags `--strict` turns on. The 22 projects + // that opted into strict all declare noPropertyAccessFromIndexSignature alongside it, so the + // gate's "strict" must mean the repo's convention or it under-reports real debt. + const ws = await makeWorkspace({ projects: [violations], strict: false }); + t.after(() => ws.cleanup()); + + const { diagnostics } = await checkTypeScript({ + workspaceDir: ws.dir, + configPath: path.join(ws.dir, 'libs/loose/tsconfig.lib.json'), + flagSet: 'strict' + }); + + assert.ok( + diagnostics.some((d) => d.code === 'TS4111'), + 'the repo convention must catch index-signature property access' + ); +}); + +test('the narrow flag set reports strictly fewer codes than the full one', async (t) => { + const ws = await makeWorkspace({ projects: [violations], strict: false }); + t.after(() => ws.cleanup()); + + const configPath = path.join(ws.dir, 'libs/loose/tsconfig.lib.json'); + const full = await checkTypeScript({ workspaceDir: ws.dir, configPath, flagSet: 'strict' }); + const narrow = await checkTypeScript({ workspaceDir: ws.dir, configPath, flagSet: 'null-checks' }); + + const fullCodes = new Set(full.diagnostics.map((d) => d.code)); + const narrowCodes = new Set(narrow.diagnostics.map((d) => d.code)); + + assert.ok(narrowCodes.has('TS18047'), 'null-checks must still catch possibly-null'); + assert.ok(!narrowCodes.has('TS4111'), 'null-checks must not include the index-signature rule'); + for (const code of narrowCodes) { + assert.ok(fullCodes.has(code), `${code} appeared under the narrow set but not the full one`); + } +}); + +test('leaves every configuration file byte-identical', async (t) => { + const ws = await makeWorkspace({ projects: [violations], strict: false }); + t.after(() => ws.cleanup()); + + const configPath = path.join(ws.dir, 'libs/loose/tsconfig.lib.json'); + const read = async (p) => fs.readFile(p, 'utf8'); + const before = { + base: await read(path.join(ws.dir, 'tsconfig.base.json')), + lib: await read(configPath), + root: await read(path.join(ws.dir, 'libs/loose/tsconfig.json')) + }; + + await checkTypeScript({ workspaceDir: ws.dir, configPath, flagSet: 'strict' }); + + assert.equal(await read(path.join(ws.dir, 'tsconfig.base.json')), before.base); + assert.equal(await read(configPath), before.lib); + assert.equal(await read(path.join(ws.dir, 'libs/loose/tsconfig.json')), before.root); + + const stray = (await fs.readdir(path.join(ws.dir, 'libs/loose'))).filter((f) => + f.startsWith('tsconfig.') && !['tsconfig.json', 'tsconfig.lib.json', 'tsconfig.spec.json'].includes(f) + ); + assert.deepEqual(stray, [], 'no overlay config may be left behind — SC-010'); +}); + +/* ── T050 (US4) — Angular template strictness ─────────────────────────────── + * The premise of the template arm, and it is a DIFFERENT mechanism from the TypeScript one: + * Angular's settings are not TypeScript compiler options, and its command-line parser rejects + * them outright (verified against the pinned compiler: only i18nFile, i18nFormat, locale, + * missingTranslation and watch are tolerated). They can only be supplied through configuration + * the compiler reads — and this harness does that in memory, so nothing is written anywhere. + */ + +test('forces template strictness on a project whose config sets strictTemplates: false', async (t) => { + const ng = await makeNgProject({ strictTemplates: false, withViolations: true }); + t.after(() => ng.cleanup()); + + const { diagnostics } = await checkAngularTemplates({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + flagSet: 'strict' + }); + + assert.ok(diagnostics.length > 0, 'a number bound to a string input must fail under strictTemplates'); + assert.ok( + diagnostics.some((d) => d.layer === 'template'), + `expected a template-layer diagnostic; got ${diagnostics.map((d) => `${d.code}/${d.layer}`).join(', ')}` + ); +}); + +test('the same project reports nothing when template strictness is left off', async (t) => { + // Establishes that the findings above are CAUSED by forcing the setting, rather than being + // pre-existing breakage the fixture happened to contain. + const ng = await makeNgProject({ strictTemplates: false, withViolations: true }); + t.after(() => ng.cleanup()); + + const { diagnostics } = await checkAngularTemplates({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + flagSet: 'strict', + forceTemplates: false + }); + + assert.equal( + diagnostics.filter((d) => d.layer === 'template').length, + 0, + 'without forcing, the project compiles as it does today' + ); +}); + +test('a separate-file template diagnostic is attributed to the .html file', async (t) => { + const ng = await makeNgProject({ strictTemplates: false, withViolations: true }); + t.after(() => ng.cleanup()); + + const { diagnostics } = await checkAngularTemplates({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + flagSet: 'strict' + }); + + assert.ok( + diagnostics.some((d) => d.file.endsWith('separate.component.html')), + 'a violation in an external template belongs to the template file, not the component' + ); +}); + +test('an inline template diagnostic is attributed to the component source', async (t) => { + // There is no template file to blame, so the diagnostic must land on the .ts — otherwise the + // diff filter compares against a path that does not exist and silently drops a real finding. + const ng = await makeNgProject({ strictTemplates: false, withViolations: true }); + t.after(() => ng.cleanup()); + + const { diagnostics } = await checkAngularTemplates({ + configPath: path.join(ng.dir, ng.root, 'tsconfig.lib.json'), + flagSet: 'strict' + }); + + assert.ok( + diagnostics.some((d) => d.file.endsWith('inline.component.ts')), + 'an inline template violation belongs to the component source file' + ); +}); + +test('template-aware checking leaves every configuration file byte-identical', async (t) => { + const ng = await makeNgProject({ strictTemplates: false }); + t.after(() => ng.cleanup()); + + const configPath = path.join(ng.dir, ng.root, 'tsconfig.lib.json'); + const rootConfig = path.join(ng.dir, ng.root, 'tsconfig.json'); + const before = { lib: await fs.readFile(configPath, 'utf8'), root: await fs.readFile(rootConfig, 'utf8') }; + + await checkAngularTemplates({ configPath, flagSet: 'strict' }); + + assert.equal(await fs.readFile(configPath, 'utf8'), before.lib); + assert.equal(await fs.readFile(rootConfig, 'utf8'), before.root, 'strictTemplates:false must still say false'); +}); diff --git a/core-web/tools/scripts/strict-gate/writeup.check.mjs b/core-web/tools/scripts/strict-gate/writeup.check.mjs new file mode 100644 index 00000000000..67e7a75eaec --- /dev/null +++ b/core-web/tools/scripts/strict-gate/writeup.check.mjs @@ -0,0 +1,56 @@ +#!/usr/bin/env node +/** + * T064 — completeness check for findings.md. + * + * User Story 5 delivers a written record, not code, so no unit or integration test applies to it. + * Constitution Principle V allows that omission only as an explicit, recorded decision — and this + * check is what stands in its place: it fails while any figure the spike promised is still absent, + * so "the write-up is done" is a verifiable claim rather than an opinion. + */ +import fs from 'node:fs'; +import path from 'node:path'; +import { workspaceRoot } from './lib/resolve-tools.mjs'; + +const FINDINGS = path.join( + workspaceRoot, + '..', + 'specs/37401-diff-scoped-strict-typecheck-gate/findings.md' +); + +/** Each requirement names what must be present and why the write-up is incomplete without it. */ +const REQUIRED = [ + { id: 'FR-012', label: 'per-pull-request results', pattern: /#37264[\s\S]*#37415[\s\S]*#37372/ }, + { id: 'SC-003', label: 'per-finding adjudication', pattern: /\*\*real\*\*|verdict/i }, + { id: 'SC-002', label: 'false-positive measure', pattern: /false positive/i }, + { id: 'SC-004', label: 'discarded-diagnostic counts', pattern: /discard/i }, + { id: 'SC-007a', label: 'flag-set recommendation', pattern: /Decision 2 — Flag set/i }, + { id: 'SC-007b', label: 'granularity recommendation', pattern: /Decision 1 — Granularity/i }, + { id: 'SC-007c', label: 'runtime figure', pattern: /Decision 3 — Runtime/i }, + { id: 'SC-005', label: 'runtime against the 10s budget', pattern: /10s budget/i }, + { id: 'SC-006', label: 'edge cases exercised', pattern: /edge case/i }, + { id: 'SC-008', label: 'go / no-go on blocking merges', pattern: /go\s*\/\s*no-go on blocking/i }, + { id: 'SC-009', label: 'timebox outcome', pattern: /timebox/i }, + { id: 'SC-011', label: 'template strictness demonstrated', pattern: /SC-011/ }, + { id: 'SC-013', label: 'go / no-go on templates', pattern: /NO-GO|GO for day-one blocking/i }, + { id: 'FR-013', label: 'follow-up task or documented no-go', pattern: /follow-up/i } +]; + +export function checkWriteup(text) { + const missing = REQUIRED.filter((r) => !r.pattern.test(text)); + return { complete: missing.length === 0, missing }; +} + +if (process.argv[1] === new URL(import.meta.url).pathname) { + if (!fs.existsSync(FINDINGS)) { + process.stderr.write(`writeup.check: ${FINDINGS} does not exist yet\n`); + process.exit(1); + } + const { complete, missing } = checkWriteup(fs.readFileSync(FINDINGS, 'utf8')); + if (complete) { + process.stdout.write(`writeup.check: findings.md carries all ${REQUIRED.length} required figures\n`); + process.exit(0); + } + process.stderr.write('writeup.check: findings.md is incomplete\n'); + for (const item of missing) process.stderr.write(` missing ${item.id}: ${item.label}\n`); + process.exit(1); +} diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md b/specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md new file mode 100644 index 00000000000..b218722da82 --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/contracts/cli.md @@ -0,0 +1,84 @@ +# Contract: strict-gate command line + +**Feature**: [spec.md](../spec.md) | **Plan**: [plan.md](../plan.md) | **Date**: 2026-09-07 + +Two entry points. Both live in `core-web/tools/scripts/strict-gate/` and are run with the +workspace's pinned Node. + +--- + +## `run.mjs` — check one range + +``` +node tools/scripts/strict-gate/run.mjs --base --head [options] +``` + +| Option | Values | Default | Requirement | +|---|---|---|---| +| `--base` | git ref | *required* | Fetched if absent locally (FR-011) | +| `--head` | git ref | `HEAD` | | +| `--flags` | `strict` \| `null-checks` \| `strict-max` | `strict` | FR-007. `strict` is the repo convention (8+4) — the same yardstick as PR #37198. `strict-max` adds `noUncheckedIndexedAccess` + `exactOptionalPropertyTypes`: measured for a future ratchet, not the blocking set. | +| `--granularity` | `file` \| `line` | `line` | FR-008. `line` blames only lines the pull request wrote; `file` makes whoever touches a legacy file inherit its history. | +| `--templates` | `on` \| `off` | `off` | FR-014; `off` keeps the core arm independent of the template arm | +| `--report` | path | *(none)* | Writes the JSON record, conforming to [`report.schema.json`](./report.schema.json) | +| `--format` | `text` \| `github` \| `markdown` \| `json` | `text` | What goes to stdout | +| `--scope` | path prefix \| `none` | `core-web` | Hard scope; a diff outside it is a no-op pass | + +**Exit codes** + +| Code | Meaning | +|---|---| +| `0` | No surviving diagnostic. Includes the empty-diff no-op. | +| `1` | At least one surviving diagnostic — the gate failure the whole thing exists to produce. | +| `2` | The harness could not run: base ref unresolvable after fetch, project graph unreadable, configuration unparseable. **Never conflated with `1`** — a broken harness reporting "clean" is the one failure mode that would quietly defeat the gate. | + +**Output formats** + +| Format | For | Behavior | +|---|---|---| +| `text` | humans and **coding agents reading raw CI logs** | States why the gate failed, the scope rule, each violation with a concrete fix hint, and the local repro command | +| `github` | the pull request diff | `::error file=,line=,col=::` annotations, rendered inline on the changed lines; also appends a Markdown job summary when `GITHUB_STEP_SUMMARY` is set | +| `markdown` | job summary / comment | Table of violations with the scope rule stated first | +| `json` | machines | The full report | + +The `text` and `github` outputs deliberately lead with the **scope rule** — that only changed +lines (or changed files) are checked and that dependency diagnostics were ignored on purpose. An +agent that does not know this will "fix" an entire legacy file and produce a diff nobody asked +for. Telling it what NOT to touch is as load-bearing as telling it what broke. + +**Guarantees** + +- Writes nothing into the working tree. Version-controlled files are byte-identical afterwards + (SC-010), including if the process is interrupted. +- Every child process is invoked with an argument array, never a shell string. Refs, branch + names and file paths come from pull-request metadata and are untrusted input; a + shell-interpolated branch name would be a command-injection vector in a tool destined for CI. + This is a review checkpoint, not a style preference. +- Reports rather than assumes: the selected mode per project, unmapped files, and discarded + counts all appear in the report even on a passing run. +- Scoped to `core-web/` by default. A pull request touching only backend or docs is a no-op pass + that costs ~0.3s: it never reads the project graph and never starts a compiler. CI additionally + gates the job itself on the same path filter, so the usual case is that it does not run at all. + +--- + +## `replay.mjs` — run the corpus + +``` +node tools/scripts/strict-gate/replay.mjs --pr [,...] [run.mjs options] +``` + +Resolves each pull request's merge commit `M` via `gh`, then invokes `run.mjs` with +`--base M^1 --head M` (D-005). Emits one report per pull request plus a summary table carrying +the detection result, the false-positive count, the discarded counts and the timings — the raw +material for the write-up. + +| Option | Values | Default | Notes | +|---|---|---|---| +| `--pr` | comma-separated numbers | *required* | | +| `--matrix` | flag | off | Runs every combination of `--flags` and `--granularity` over the corpus, which is what FR-007 and FR-008 need in order to be compared on identical input | +| `--out` | directory | `./strict-gate-out` | Reports written outside the repository tree by default | + +**Exit codes**: `0` when every case matched its pre-registered expectation, `1` on any mismatch, +`2` on a harness error. A mismatch is information, not a defect — the run still writes every +report so the adjudication required by SC-003 can proceed. diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/contracts/report.schema.json b/specs/37401-diff-scoped-strict-typecheck-gate/contracts/report.schema.json new file mode 100644 index 00000000000..408ed42a826 --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/contracts/report.schema.json @@ -0,0 +1,136 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://dotcms.com/schemas/strict-gate/report.schema.json", + "title": "Strict gate run report", + "description": "Output of one strict-gate harness invocation. Enforced by report.contract.test.mjs.", + "type": "object", + "additionalProperties": false, + "required": [ + "base", + "head", + "flagSet", + "granularity", + "targets", + "unmapped", + "findings", + "discarded", + "durationMs", + "exitCode" + ], + "properties": { + "base": { + "type": "string", + "pattern": "^[0-9a-f]{40}$", + "description": "Resolved base commit SHA actually compared." + }, + "head": { + "type": "string", + "pattern": "^[0-9a-f]{40}$", + "description": "Resolved head commit SHA actually compared." + }, + "flagSet": { + "enum": ["strict", "null-checks", "strict-max"], + "description": "Candidate flag set: the repo strict convention (8+4), the narrower null-checks/implicit-any subset, or strict-max (adds noUncheckedIndexedAccess + exactOptionalPropertyTypes, measured but not the blocking set)." + }, + "granularity": { + "enum": ["file", "line"], + "description": "Whole changed file, or only changed lines." + }, + "targets": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["project", "root", "configPath", "mode", "files"], + "properties": { + "project": { "type": "string" }, + "root": { "type": "string" }, + "configPath": { "type": "string" }, + "mode": { + "enum": ["typescript", "template-aware"], + "description": "Always present: a fallback to typescript mode must be visible, never silent." + }, + "files": { "type": "array", "items": { "type": "string" } } + } + } + }, + "unmapped": { + "type": "array", + "description": "Changed files no project claimed. Reported, never dropped. Not a failure on its own.", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["path", "reason"], + "properties": { + "path": { "type": "string" }, + "reason": { "type": "string" } + } + } + }, + "findings": { + "type": "array", + "description": "Surviving diagnostics. Every entry must have origin 'changed'.", + "items": { "$ref": "#/$defs/diagnostic" } + }, + "discarded": { + "type": "object", + "additionalProperties": false, + "required": ["byOrigin", "byLayer"], + "properties": { + "byOrigin": { + "type": "object", + "additionalProperties": false, + "required": ["dependency", "untouched", "infrastructure"], + "properties": { + "dependency": { "type": "integer", "minimum": 0 }, + "untouched": { "type": "integer", "minimum": 0 }, + "infrastructure": { "type": "integer", "minimum": 0 } + } + }, + "byLayer": { + "type": "object", + "additionalProperties": false, + "required": ["source", "template"], + "properties": { + "source": { "type": "integer", "minimum": 0 }, + "template": { "type": "integer", "minimum": 0 } + } + } + } + }, + "durationMs": { + "type": "object", + "additionalProperties": false, + "required": ["total", "typescript", "templateAware"], + "properties": { + "total": { "type": "number", "minimum": 0 }, + "typescript": { "type": "number", "minimum": 0 }, + "templateAware": { "type": "number", "minimum": 0 } + } + }, + "exitCode": { + "type": "integer", + "minimum": 0, + "description": "0 if and only if findings is empty." + } + }, + "$defs": { + "diagnostic": { + "type": "object", + "additionalProperties": false, + "required": ["file", "line", "column", "code", "message", "origin", "layer"], + "properties": { + "file": { + "type": "string", + "description": "Repository-relative. For an inline template, the component source." + }, + "line": { "type": "integer", "minimum": 1 }, + "column": { "type": "integer", "minimum": 1 }, + "code": { "type": "string" }, + "message": { "type": "string" }, + "origin": { "enum": ["changed", "dependency", "untouched", "infrastructure"] }, + "layer": { "enum": ["source", "template"] } + } + } + } +} diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md b/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md new file mode 100644 index 00000000000..1ff762c0b71 --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md @@ -0,0 +1,134 @@ +# Phase 1 Data Model: Diff-scoped strict typecheck gate + +**Feature**: [spec.md](./spec.md) | **Plan**: [plan.md](./plan.md) | **Date**: 2026-09-07 + +These are the harness's in-memory entities and the shape they take in the JSON report. There is +no database and no persisted state; the report is the only durable artifact. + +--- + +## ChangedFile + +One file the pull request added, copied, modified or renamed. Deleted files never become a +`ChangedFile` — the gate has nothing to check in a file that no longer exists. + +| Field | Type | Notes | +|---|---|---| +| `path` | string | Repository-relative, forward slashes. For a rename, the **new** path. | +| `status` | `"A"` \| `"C"` \| `"M"` \| `"R"` | From the diff filter. | +| `changedLines` | array of `[start, end]` | 1-based, inclusive. Added and modified lines only, from hunk headers with zero context. Empty for a pure rename with no content change. | +| `kind` | `"source"` \| `"template"` | Drives which mode can produce diagnostics for it. | + +**Rules** + +- A path is normalized once, at construction, so every later comparison is a plain string match. +- `changedLines` is only consulted under line-level granularity; whole-file granularity ignores + it entirely. + +--- + +## ProjectTarget + +One unit of compilation: an owning project paired with one of its configurations that actually +includes at least one changed file. + +| Field | Type | Notes | +|---|---|---| +| `project` | string | Nx project name. | +| `root` | string | Project root, repository-relative. The longest matching root wins ownership. | +| `configPath` | string | The configuration whose resolved file list contains the changed file. | +| `mode` | `"typescript"` \| `"template-aware"` | Selected per project and always reported (FR-016). | +| `files` | array of string | The `ChangedFile` paths this target is responsible for. | + +**Rules** + +- A project may produce more than one `ProjectTarget` (for example a library configuration and a + spec configuration), and a changed file may appear in more than one of them. Diagnostics are + deduplicated afterwards, by file, line and code. +- A configuration resolving to zero files never becomes a `ProjectTarget`. + +--- + +## UnmappedFile + +A changed file that matched no project root. Reported, never discarded (FR-002). + +| Field | Type | Notes | +|---|---|---| +| `path` | string | Repository-relative. | +| `reason` | string | Why nothing claimed it — no matching project root, or an owning project with no configuration that includes it. | + +An unmapped file is **not** a gate failure on its own. It is a visible gap: workspace-root files +and tooling scripts belong here legitimately, and the report is what lets a reader tell those +apart from a mapping bug. + +--- + +## Diagnostic + +One reported error from either compiler, before or after filtering. + +| Field | Type | Notes | +|---|---|---| +| `file` | string | Originating file, repository-relative. For an inline template this is the component source, not a template path. | +| `line` | integer | 1-based. | +| `column` | integer | 1-based. | +| `code` | string | Compiler diagnostic code. | +| `message` | string | Single line; nested explanatory chains are flattened. | +| `origin` | `"changed"` \| `"dependency"` \| `"untouched"` | Why it survived or was discarded. | +| `layer` | `"source"` \| `"template"` | Which arm produced it; lets the report count them apart (FR-015). | + +**Rules** + +- `origin` is assigned by the filter and is the field the whole spike turns on: + `"changed"` survives; `"dependency"` (a file from another project) and `"untouched"` (a file in + this project the pull request did not touch) are discarded but **counted** (FR-004). +- Under line-level granularity, a diagnostic in a changed file whose line falls outside every + `changedLines` range is discarded as `"untouched"`. + +--- + +## RunReport + +The harness's output, one per invocation. Schema: [`contracts/report.schema.json`](./contracts/report.schema.json). + +| Field | Type | Notes | +|---|---|---| +| `base` / `head` | string | The resolved commit SHAs actually compared. | +| `flagSet` | `"strict"` \| `"nullChecks"` | Which candidate flag set ran (FR-007). | +| `granularity` | `"file"` \| `"line"` | Which candidate granularity ran (FR-008). | +| `targets` | array of `ProjectTarget` | Including each one's selected mode. | +| `unmapped` | array of `UnmappedFile` | | +| `findings` | array of `Diagnostic` | Survivors only, all with `origin: "changed"`. | +| `discarded` | object | Counts by origin and by layer — the evidence that the filter, not luck, produced a pass. | +| `durationMs` | object | Wall-clock totals, split by mode so the template arm's cost is separable (FR-017). | +| `exitCode` | integer | `0` when `findings` is empty, non-zero otherwise (FR-005). | + +**Invariants** + +- `exitCode === 0` if and only if `findings` is empty. +- Every entry in `findings` has `origin: "changed"`. +- An empty diff yields a valid report with no targets, no findings and `exitCode: 0` — the + no-op pass required by the spec's first edge case. + +--- + +## SampleCase + +One entry in the replay corpus. Its `expectation` is recorded **before** the harness runs +against it (D-009), which is what keeps the corpus from being fitted to the result. + +| Field | Type | Notes | +|---|---|---| +| `pr` | integer | Pull request number. | +| `mergeCommit` | string | Head. Base is its first parent. | +| `expectation` | `"debt"` \| `"clean"` | The pre-registered label. | +| `knownFindings` | array | For `"debt"` cases, the violations expected — for PR #37262, the three in `sdk-create-app`. | +| `observed` | `RunReport` | Filled in by the run. | +| `adjudication` | array | Per finding: real or spurious, with the reason (SC-003). | + +**Rules** + +- A `"clean"` case producing any finding is a false positive and counts against SC-002 — unless + adjudication shows the pre-registered label was wrong, in which case the label is corrected + **and the correction is recorded**, never quietly amended. diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md new file mode 100644 index 00000000000..f127c42f0bd --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md @@ -0,0 +1,420 @@ +# Findings: diff-scoped strict typecheck gate + +**Issue**: dotCMS/core#37401 · **Spec**: [spec.md](./spec.md) · **Date**: 2026-09-07 +**Status**: in progress — User Stories 1 and 2 complete; US3 (decision matrix) and US4 (templates) pending. + +Every number below was produced by `core-web/tools/scripts/strict-gate/`, replaying real merged +pull requests. Nothing here is estimated. + +--- + +## 1. The research question is answered: yes + +A diff-scoped strict typecheck **does** block new non-strict TypeScript without the dependency +libraries being strict first. The mechanism works, and the margin is not close. + +`libs/portlets/dot-locales/portlet`, checked under the repo's strict convention: + +| Origin of diagnostic | Count | +|---|---| +| `libs/ui/src` | 111 | +| `libs/dotcms-js/src` | 38 | +| `libs/data-access/src` | 36 | +| `libs/utils/src` | 32 | +| **the portlet itself** | **2** | +| **Total** | **219** | + +**217 of 219 discarded — 99.1%.** The dependency's errors do not need to be fixed; they need to +stop counting. That is the whole hypothesis, and it holds. + +--- + +## 2. The corpus + +Pre-registration rule, fixed before any case ran: a pull request is `clean` if **every** project +it touches already declares the convention the gate enforces. Structural, derivable from +tsconfigs and the diff, never from a gate result. + +Run at `--flags strict --granularity line`: + +| PR | Pre-registered | Findings | Discarded | Wall clock | Outcome | +|---|---|---|---|---|---| +| #37264 | debt | 3 | 3 | 6.3s | as predicted | +| #37415 | debt | 2 | 2 736 | 11.4s | as predicted | +| #37372 | debt | 3 | 2 064 | 12.0s | as predicted | +| #37405 | clean | 1 | 991 | 6.9s | **prediction wrong** | +| #37339 | clean | 2 | 990 | 7.1s | **prediction wrong** | + +--- + +## 3. Adjudication — all 11 findings (SC-003) + +| PR | Code | Location | Verdict | +|---|---|---|---| +| #37264 | TS4111 | `sdk/create-app/src/index.ts:294` | **real** — confirmed with `tsc` before the harness existed | +| #37264 | TS2345 | `create-app/src/utils/readiness.spec.ts:263` | **real** | +| #37264 | TS2345 | `create-app/src/utils/readiness.spec.ts:271` | **real** | +| #37415 | TS2531 | `dot-relationship-field.component.ts:394` | **real** — `strictNullChecks` | +| #37415 | TS18047 | `dot-relationship-field.component.spec.ts:332` | **real** — `strictNullChecks` | +| #37372 | TS18047 | `dot-content-drive-shell.component.spec.ts:1293` | **real** | +| #37372 | TS2769 | `dot-content-drive-shell.component.spec.ts:2894` | **real** — cascade, see below | +| #37372 | TS2345 | `dot-content-drive-shell.component.spec.ts:2894` | **real** — same defect as the row above | +| #37405 | TS2571 | `dot-auth-config.component.ts:144` | **real** — `useUnknownInCatchVariables` | +| #37339 | TS7006 | `dot-auth-oidc-connection.component.spec.ts:44` | **real** — `noImplicitAny` | +| #37339 | TS7006 | `dot-auth-oidc-connection.component.spec.ts:59` | **real** — `noImplicitAny` | + +**11 findings, 11 real, 0 false positives.** Every one sits on a line its pull request wrote. + +Two observations that qualify the count: + +- **One cascade.** The TS2769 and TS2345 at `:2894` are one defect reported twice — TypeScript + emits the overload failure and the specific argument mismatch separately. It inflates the count + without being wrong. A production gate should collapse diagnostics that share a file and line. +- **One false positive was found and eliminated during adjudication**: TS2307 + *Cannot find module `@openng/spectator/jest`* on #37339. It appears under plain `tsc` with no + flags forced — a module-resolution failure, never a strictness violation. The harness now + discards `TS2307`, `TS2688` and `TS6053` as infrastructure, counted but never reported. Without + that exclusion the false-positive rate would have been 1 in 12. + +--- + +## 4. The pre-registration rule was wrong, and why that matters + +Both "clean" predictions failed. `libs/portlets/dot-auth` declares the full convention — `strict`, +all four extras, and `strictTemplates` — and still contains real type errors on lines a merged +pull request wrote. + +**Declaring `strict: true` does not mean anything compiles it.** The `typecheck` target exists on +**3 of 57 projects**. Everything else is covered by lint only, and lint does not type-check. A +project can carry the strictest configuration in the workspace and accumulate type errors +indefinitely with nothing to notice. + +This enlarges the gate's value beyond the issue's framing. It is not only a ratchet against new +debt in non-strict libraries — **it is the first thing in CI that type-checks 54 of 57 projects +at all.** + +It also means SC-002 cannot be measured as written. "At least 3 merged pull requests that +introduced no strict debt" presumes such pull requests are identifiable in advance; in this +workspace they are close to nonexistent. Of 42 recent frontend pull requests: + +| Projects touched | PRs | +|---|---| +| all declare the full convention | 2 | +| all `strict: true`, none of the four extras | 1 | +| **at least one not strict at all** | **39 (93 %)** | + +Waiting for per-project opt-in covers 7 % of pull requests. The diff filter covers the rest. +That is the strongest argument for this approach that the spike produced, and it was not +anticipated in the issue. + +**The honest false-positive measure is therefore per finding, not per case: 0 of 11.** + +--- + +## 5. Runtime — SC-005 is not met + +| PR | Wall clock | Within 10s budget | +|---|---|---| +| #37264 | 6.3s | yes | +| #37339 | 7.1s | yes | +| #37405 | 6.9s | yes | +| #37415 | 11.4s | **no** | +| #37372 | 12.0s | **no** | + +Two of five exceed the budget SC-005 set to protect what ADR-0013 bought (frontend merge time +cut from ~45 min to ~15 min). Both overruns are pull requests touching `libs/ui` or +`libs/edit-content` — large programs whose dependency closure is recompiled in full. + +The cost is visible in the discard counts: 2 736 and 2 064 diagnostics computed and thrown away. +The gate currently pays to typecheck every dependency source in order to ignore it. Obvious +optimisations exist and are untried: reusing one program across a project's configurations, +skipping projects that already declare everything the flag set forces, and caching the dependency +closure between targets. None was attempted — measuring came first. + +--- + +## 6. The three decisions, settled with measurements (SC-007) + +Full matrix: 3 flag sets x 2 granularities x 5 pull requests, compiled once per +(pull request, flag set) and filtered twice — granularity only affects the filter. + +### Findings per combination + +| PR | null-checks
file / line | strict (8+4)
file / line | strict-max
file / line | +|---|---|---|---| +| #37264 | 2 / 2 | 5 / 3 | 13 / 8 | +| #37415 | 22 / 2 | 22 / 2 | 39 / 2 | +| #37372 | 17 / 3 | 23 / 3 | 71 / 4 | +| #37405 | 10 / 1 | 10 / 1 | 22 / 9 | +| #37339 | 3 / 2 | 3 / 2 | 14 / 2 | +| **Total** | **54 / 10** | **63 / 11** | **159 / 25** | + +Average wall clock: 8.4s (null-checks), 9.4s (strict), 9.0s (strict-max). **The flag set barely +affects cost** — the expense is building the program, not the rules applied to it. + +### Decision 1 — Granularity: **line-level**. Not close. + +| Flag set | whole-file | line-level | inherited from untouched lines | +|---|---|---|---| +| null-checks | 54 | 10 | **44 (81 %)** | +| strict | 63 | 11 | **52 (83 %)** | +| strict-max | 159 | 25 | **134 (84 %)** | + +Under whole-file, **83 % of what the gate reports is debt the author did not write**. #37415 goes +from 2 findings to 22, #37372 from 3 to 23, #37405 from 1 to 10 — touching one line of a file +makes you inherit roughly ten times your own work. No team adopts that; it converts every small +fix into an unbounded cleanup. + +Line-level costs nothing in coverage that matters: an added file has every line changed, so new +code is still held to the full bar. It only ever forgives pre-existing lines in modified files. + +### Decision 2 — Flag set: **the repo convention (8 + 4)**. + +At line granularity the whole corpus separates the candidates by **one finding**: 11 versus 10. +The four extra flags are, in practice, free. + +| | line-level findings | vs narrow | +|---|---|---| +| null-checks (`strictNullChecks` + `noImplicitAny`) | 10 | — | +| **strict (repo convention)** | **11** | +1 | +| strict-max | 25 | +15 | + +The narrow set is not meaningfully quieter, and it misses the TS4111 class outright — the very +violation the issue used as its reproducible case. Meanwhile `strict` matches `tsconfig.base.json` +on PR #37198 exactly, so the gate measures with the same yardstick as the destination. Choosing +the narrow set would buy one fewer finding across five pull requests at the cost of letting +through debt the migration must later fix by hand. + +`strict-max` more than doubles findings at line granularity (25 vs 11) and adds 134 inherited +findings whole-file. It exceeds what any project in the workspace has ever met and what #37198 +targets. **Measured and recorded for a future ratchet; not the blocking set.** + +### Decision 3 — Runtime: **8.4–9.4s average, and SC-005 is not met at the tail** + +Two of five corpus cases exceed the 10s budget (11.4s and 12.0s), both touching `libs/ui` or +`libs/edit-content`. Since the flag set barely moves the number, the cost is structural: the gate +compiles a project's entire dependency closure in order to discard it — 2 736 and 2 064 discarded +diagnostics on exactly those two runs. + +**Recommended gate invocation:** + +``` +--flags strict --granularity line +``` + +## 7. The template arm (SC-011 / SC-012 / SC-013) + +Case: **PR #37248**, one template file in `apps/dotcms-ui` — one of the four applications carrying +`TODO(#35930): re-enable strictTemplates once Angular 22 template errors are fixed per app`. + +| | TypeScript-only | Template-aware | +|---|---|---| +| Mode selected | `tsconfig.app.json` [typescript] | `tsconfig.app.json` [template-aware] | +| Source diagnostics discarded | 2 631 | 2 637 | +| **Template diagnostics discarded** | **0** | **547** | +| Findings reported | 0 | 0 | +| Compiler time | 7.4s | **16.3s** | +| Total wall clock | 12.7s | **21.2s** | + +**SC-011 — met.** Template strictness is forced on an application whose configuration sets +`strictTemplates: false`, with no version-controlled file edited. The Angular settings are supplied +through `readConfiguration(project, existingOptions)`, whose `existingOptions` outrank everything +in the extends chain — the same in-memory approach as the TypeScript arm, for the same reason: +an overlay file would survive a crash and break SC-010. + +**SC-012 — met, and the number is the point.** 547 template diagnostics were discarded and **zero** +reported. Those 547 are the `TODO(#35930)` backlog. A gate that reported them would be unusable +on day one; a gate that counted none would mean the filter did nothing. Discarding 547 to report 0 +is exactly the behaviour that lets a diff-scoped gate coexist with an application-wide opt-out. + +**SC-013 — the cost, and the recommendation: NO-GO for day-one blocking.** + +Template-aware checking costs **2.2× the compiler time** (16.3s vs 7.4s) and **1.7× wall clock** +(21.2s vs 12.7s) on the largest application. The TypeScript arm already breaches SC-005's 10s +budget at the tail; the template arm puts the worst case at over 20s. Measured against what +ADR-0013 bought — frontend merge time cut from ~45 min to ~15 min — that is not a cost to add +before the optimisations in §5 are done. + +The recommendation is **not** that templates are unsuitable. The mechanism works, the filter works, +and the four applications with no template gate at all are where the most user-facing code lives. +It is that the template arm should ship **after** the TypeScript arm, once the dependency-closure +cost is addressed — or immediately as a **non-blocking, advisory** run, which costs a reader +nothing and starts producing the data. + +### Three silent-failure bugs the spike exposed + +All three were invisible in the same way: the run reported a plausible result and a plausible exit +code. None would have been found by checking that the harness ran without error — only by checking +*which configuration it chose* and *what range it compared*. + +1. **Entry-point configs are invisible to file-list matching.** `apps/dotcms-ui/tsconfig.app.json` + declares `"files": ["src/main.ts", "src/polyfills.ts"]`. Its resolved file list is two entries — + every component arrives through the import graph. Selecting "the configuration whose resolved + list contains this file" therefore never picks it, and the app's sources and templates fell + through to `tsconfig.editor.json`, an IDE-only config Nx generates that carries **no** + `angularCompilerOptions`. Files were still checked, which is precisely why it hid. + +2. **Colocation makes the spec config look like the build config.** Angular puts + `x.component.ts`, `x.component.html` and `x.component.spec.ts` in one directory, so a rule of + "the configuration that owns TypeScript in this directory" matches `tsconfig.spec.json` as + readily as the build config — and the spec config has no Angular settings either. + +3. **A tree diff instead of a merge-base diff.** The harness compared `base..head` — two trees — + where a pull request means `base...head`, everything since the two diverged. The issue's own + acceptance criteria specify three dots; I used two. Invisible while a branch is fresh, wrong once + it is stale: every file the BASE modified is reported as changed, and the author is blamed for + violations someone else merged. Found by running the documented quickstart command against this + very branch, which reported **50 findings**, essentially none of them its own. After the fix: + PASS, 0 targets — correct, since this branch adds only `.mjs` files. The report now cites the + merge base rather than the base tip, which is also what makes a re-run months later reproduce + the same numbers. + +Selection now ranks candidates (`app` > `lib` > `json` > `spec` > `editor`) instead of taking the +first match, and an IDE-only configuration is ignored outright whenever a real build configuration +exists. Both rules are pinned by tests that reproduce the `apps/dotcms-ui` shape specifically — a +lib-shaped fixture passed by accident, because alphabetical ordering happened to put the right +answer first. + +--- + +## 8. Corrections to the issue's premises + +| Issue says | Verified | +|---|---| +| "PR #37262" | #37262 is an **issue**. The pull request is **#37264**, merge commit `788795e915` | +| "3 strict errors" | **5** under the repo convention (2 under bare `--strict`) | +| "`src/index.ts` (TS4111)" ×1 | **two** TS4111, at lines 294 and 515 | +| — | plus a TS7030 in `src/utils/index.ts:41` the issue did not list | +| TS4111 is a strict error | It is **not** — `noPropertyAccessFromIndexSignature` is outside `--strict` | + +The last row is the load-bearing one: it is why the flag-set decision could be settled with +evidence before the harness existed. + +--- + +## 9. Edge cases (SC-006) + +All ten exercised. None crashed; none skipped silently. + +| # | Edge case | Observed | +|---|---|---| +| 1 | Pull request with no TypeScript at all | exit 0, 0 targets, **132 ms** — no project graph read, no compiler started | +| 2 | Deleted and renamed files in the diff | 28 files resolved from a real merge, statuses `M`/`A`; deletions excluded, renames at their new path | +| 3 | Shared config touched (`tsconfig.base.json`, `nx.json`) | **1 target of 57 projects** — no fan-out, exactly as FR-010 requires | +| 4 | Project with no `tsconfig.lib.json` (apps, entry-point configs) | Resolved to `tsconfig.app.json` via ranked selection — and this is where two silent-failure bugs were found (§7) | +| 5 | Changed file matching no project | Reported as unmapped with a reason, never dropped | +| 6 | Shallow checkout / `merge_group` | Base ref fetched; an unresolvable base **throws** rather than reporting an empty diff | +| 7 | One file claimed by two configs | Both targets produced, diagnostics deduplicated by file/line/code — hit for real on `src/utils/index.ts` (TS7030) | +| 8 | Inline template | Diagnostic attributed to the component source, not to a nonexistent template path | +| 9 | Template-only pull request | Still resolves a project and checks it (§7); the config-selection work exists because of this case | +| 10 | Framework upgrade adding diagnostics | `extendedDiagnostics` deliberately excluded — promoting a whole category to errors lets a future minor fail pull requests for code they did not change (FR-018) | + +Edge case 6 deserves emphasis: an unresolvable base ref is treated as a **harness failure (exit 2)**, +never as a clean run. A gate that reports "no changes" because it could not find its base would +pass every pull request in CI while looking perfectly healthy. + +--- + +## 10. Recommendation — go / no-go on blocking merges (SC-008) + +### **GO**, for the TypeScript arm, with one precondition. + +| Criterion | Result | +|---|---| +| Detects real debt | 11 of 11 findings real, on lines their pull requests wrote | +| False positives | **0** of 11, after excluding three infrastructure diagnostic codes | +| Discards dependency noise | 217 of 219 on a representative portlet — 99.1 % | +| Needs no config change | Confirmed; every version-controlled file byte-identical after full corpus runs | +| Adds no dependency | Confirmed | +| **Runtime** | **8.4–9.4 s average, 12 s at the tail — SC-005's 10 s budget breached on 2 of 5 cases** | + +Precision is not the problem — it is better than the spec asked for. **Runtime is the only thing +standing between this and a day-one blocking gate**, and it is a solved kind of problem: the cost +is entirely dependency-closure recompilation (2 736 and 2 064 diagnostics computed and discarded on +the two slow cases). Untried optimisations, in the order I would try them: reuse one program across +a project's configurations, skip projects whose configuration already declares everything the flag +set forces, and cache the dependency closure between targets. + +**Recommended posture:** + +1. **Ship non-blocking first.** Same invocation, reporting only. Costs no one a merge, and produces + the data to set a realistic budget. +2. **Optimise the closure cost**, then flip to blocking. If the tail lands under ~10 s, blocking is + justified against ADR-0013's cost model; if it does not, blocking is not worth what ADR-0013 bought. +3. **Configuration:** `--flags strict --granularity line --scope core-web`. +4. **Templates: no-go for now** (§7). Ship advisory alongside, or defer to the follow-up. + +**Fallback if blocking proves untenable:** keep it non-blocking and surface findings as pull-request +annotations. Even advisory, it is the only thing type-checking 54 of 57 projects. + +### The argument that changed during the spike + +The issue framed this as a ratchet against new debt in non-strict libraries. It is that — but the +larger finding is that **`strict: true` does not mean anything compiles it**: the `typecheck` target +exists on 3 of 57 projects. Both pre-registered "clean" pull requests contained real type errors, in +a project declaring the strictest configuration in the workspace. The gate's value is bigger than +the issue assumed, and it does not depend on PR #37198 landing. + +--- + +### Operational note on the test suite + +93 tests, all passing — but only with `--test-concurrency=1`. `node --test` parallelises files by +default, and each of these builds real TypeScript or Angular programs; under that pressure one +acceptance case intermittently timed out and reported a failure that did not reproduce in +isolation. Serial run: 93/93 in ~120s. Recorded rather than papered over, since an intermittently +red suite is one people stop running. + +--- + +## 11. Timebox (SC-009) + +The issue set 4 hours; the template arm was expected to add ~2. **Both were exceeded**, and the +overrun is worth recording because of where it went — not into the mechanism, which worked early, +but into four things the plan did not anticipate: + +1. **Configuration selection.** Selecting by resolved file list rather than filename convention + (research D-003) turned out to be load-bearing twice over: it is why the anchor case's spec-file + violations were found at all, and it is where both silent-failure bugs in §7 lived. +2. **Adjudicating every finding by hand** (SC-003). This is what turned an apparent + false-positive rate of 1.0 into a measured 0 of 11, and what surfaced the TS2307 class. +3. **The pre-registration rule being refuted**, which produced the spike's most valuable finding + and was not on anyone's list. +4. **Two rounds of test correction** — including one test that passed for the wrong reason and had + to be rewritten against the real `apps/dotcms-ui` shape before it would fail. + +None of that is waste; a 4-hour version would have reported the mechanism works and missed every +one of these. But the estimate was wrong and the write-up says so. + +--- + +## 12. Follow-up + +**Recommendation: build it.** The follow-up task covers: + +- The durable script, promoted from `core-web/tools/scripts/strict-gate/`. +- The closure-cost optimisations in §10, with a measured tail before any blocking flip. +- The CI hook in `core-web/pom.xml` under the `-Pvalidate` profile, beside the existing + `lint-test` / `format-test` executions. `cicd_comp_test-phase.yml` already fetches `origin/main`, + and `.github/filters.yaml`'s `frontend` filter already gates the job on `core-web/**` — **no + workflow change is needed**. +- The local hook in `core-web/lint-staged.config.mjs`. +- **Non-blocking first**, blocking only once the tail is measured under budget. +- Templates advisory or deferred (§7). + +Two items this spike deliberately did not touch: + +- **`devEngines` for Node provisioning.** Verified working with pnpm 12.1.0, but it writes a + runtime entry into `pnpm-lock.yaml` and would sit alongside `core-web/.nvmrc`, which three CI + workflows read — `^22.0.0` resolves to 22.23.2 while `.nvmrc` pins 22.22.3, so the two drift on + day one. Its own change, pinned exactly, retiring `.nvmrc` and updating those workflows. +- **The `strict-max` ratchet.** Measured (§6) and recorded; not proposed. + +## 13. Still open + +- **US3** — full flag-set × granularity matrix, and the whole-file adoption cost. +- **US4** — Angular template strictness: cost, and go/no-go. +- **SC-008** — the day-one blocking recommendation. Detection and precision now support it; + runtime does not yet. +- **#37086** (`libs/sdk/angular`: `strict: true`, none of the extras) — the intermediate tier, + deliberately excluded from the corpus so it could not contaminate the false-positive denominator. From 81ec367ac6d8772e83e6e03c6698be7ed7742650 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Mon, 7 Sep 2026 20:01:51 -0400 Subject: [PATCH 4/8] docs(37401): record convergence findings and fix data-model drift - Documents accepted deviations (SC-005 budget miss, SC-002 criterion unachievable) and four spec additions from /speckit-converge review - Corrects data-model.md to match the shipped code: adds "infrastructure" Diagnostic.origin and renames/adds strict/null-checks/strict-max flagSet values, per the append-only convergence process --- .../data-model.md | 9 +++- .../findings.md | 50 ++++++++++++++++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md b/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md index 1ff762c0b71..db0f9d4e65a 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/data-model.md @@ -75,7 +75,7 @@ One reported error from either compiler, before or after filtering. | `column` | integer | 1-based. | | `code` | string | Compiler diagnostic code. | | `message` | string | Single line; nested explanatory chains are flattened. | -| `origin` | `"changed"` \| `"dependency"` \| `"untouched"` | Why it survived or was discarded. | +| `origin` | `"changed"` \| `"dependency"` \| `"untouched"` \| `"infrastructure"` | Why it survived or was discarded. | | `layer` | `"source"` \| `"template"` | Which arm produced it; lets the report count them apart (FR-015). | **Rules** @@ -83,6 +83,11 @@ One reported error from either compiler, before or after filtering. - `origin` is assigned by the filter and is the field the whole spike turns on: `"changed"` survives; `"dependency"` (a file from another project) and `"untouched"` (a file in this project the pull request did not touch) are discarded but **counted** (FR-004). +- `"infrastructure"` marks a diagnostic that is never a strictness violation whatever the flags — + `TS2307` (cannot find module), `TS2688`, `TS6053`. Added after adjudication found one such + diagnostic reported on a pre-registered clean pull request; it appears under plain `tsc` too, so + a strictness gate reporting it is crying wolf. Discarded and counted like any other, never + silently dropped. - Under line-level granularity, a diagnostic in a changed file whose line falls outside every `changedLines` range is discarded as `"untouched"`. @@ -95,7 +100,7 @@ The harness's output, one per invocation. Schema: [`contracts/report.schema.json | Field | Type | Notes | |---|---|---| | `base` / `head` | string | The resolved commit SHAs actually compared. | -| `flagSet` | `"strict"` \| `"nullChecks"` | Which candidate flag set ran (FR-007). | +| `flagSet` | `"strict"` \| `"null-checks"` \| `"strict-max"` | Which candidate flag set ran (FR-007). `strict` is the repo convention (8 + 4); `strict-max` adds `noUncheckedIndexedAccess` and `exactOptionalPropertyTypes` and is measured for a future ratchet, never the blocking set. | | `granularity` | `"file"` \| `"line"` | Which candidate granularity ran (FR-008). | | `targets` | array of `ProjectTarget` | Including each one's selected mode. | | `unmapped` | array of `UnmappedFile` | | diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md index f127c42f0bd..3046da83964 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md @@ -410,7 +410,55 @@ Two items this spike deliberately did not touch: day one. Its own change, pinned exactly, retiring `.nvmrc` and updating those workflows. - **The `strict-max` ratchet.** Measured (§6) and recorded; not proposed. -## 13. Still open +## 13. Accepted deviations and scope additions + +`/speckit-converge` compared the built code against the approved spec. Nothing in the specified +scope was missing, but eight gaps surfaced. All are resolved below — six by conscious acceptance, +two by correcting the artifact. + +### Deviations accepted, not fixed + +**SC-005 — the 10s budget is not met (deferred to #37448).** Two of five corpus cases run at +11.4s and 12.0s. The three untried optimisations are named in §5 and §10 and are scope for the +follow-up task, which explicitly gates the blocking flip on a measured tail under budget. Fixing +it here would mean optimising before the decision to build the production gate has been taken. + +**SC-002 — the criterion as written is unachievable in this workspace, and that is the finding.** +It asks for zero findings across at least three pull requests carrying no strict debt. Only two +structurally-clean pull requests exist across 42, and both report findings — every one adjudicated +real (§3), because declaring `strict: true` does not mean anything compiles the project (§4). The +precision guarantee that replaces it is measured **per finding, not per case: 0 false positives of +11**, and it is pinned by tests that assert no finding is an infrastructure diagnostic and that +every finding sits on a line its pull request wrote. + +### Additions that outran the approved spec + +Four behaviours were built that no functional requirement authorises. Each is defensible and each +is kept — but the spec was **approved on PR 1 before they existed**, so they are recorded here +rather than back-annotated into `spec.md`. Per the two-PR flow, spec changes after sign-off need +re-approval; silently editing an approved spec to match what was built inverts the point of the +gate. + +| Addition | Why it exists | Beyond | +|---|---|---| +| **Infrastructure-code exclusion** (`TS2307`, `TS2688`, `TS6053` in `lib/filter.mjs`) | Adjudication found one such diagnostic reported on a clean pull request; it appears under plain `tsc` too. Without the exclusion the false-positive rate would have been 1 in 12 | FR-004 | +| **Output formatters** (`lib/format.mjs`) — four formats, per-code fix hints, GitHub annotations, job summary | Requested during implementation. Coding agents read CI output and act on it; the text and github formats lead with the scope rule so an agent does not refactor an entire legacy file | FR-006, which asks only for file, line and code | +| **`--scope core-web`** | Requested during implementation. Makes a backend-only pull request a 132 ms no-op. FR-010 governs project fan-out, a different concern | — | +| **Third flag set `strict-max`** | Measured so a future ratchet arrives with its cost already known rather than blind. Never the blocking set | FR-007, which specifies two | + +### Artifacts corrected + +**`data-model.md`** now lists `infrastructure` among the `Diagnostic.origin` values and +`strict` / `null-checks` / `strict-max` for `RunReport.flagSet`, matching +`contracts/report.schema.json` and `lib/filter.mjs`. It had drifted while the schema and the code +moved together. + +**`quickstart.md`** now documents `--format` and `--scope`, which the recommended invocation uses +and the validation guide did not name. + +--- + +## 14. Still open - **US3** — full flag-set × granularity matrix, and the whole-file adoption cost. - **US4** — Angular template strictness: cost, and go/no-go. From b3ea4106a01e6cf9a667701dee889c9479481952 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 8 Sep 2026 08:56:07 -0400 Subject: [PATCH 5/8] docs(37401): disambiguate project counts and refresh findings status - spec.md: "22 of 55 project configs" now reads "22 of 55 TypeScript project configs", noting the workspace has 56 Nx projects and one (libs/dotcms-scss) ships no tsconfig.json. Both counts were correct but the same phrase carried two meanings, which read as a numeric inconsistency against "all 56 projects" further down. - findings.md: the status header and section 14 still listed US3 and US4 as pending, but section 6 is the US3 decision matrix and section 7 the US4 template arm, both complete. Section 14 now lists what is genuinely deferred (SC-005 runtime, the blocking flip, templates). Co-Authored-By: Claude Opus 5 (1M context) --- .../findings.md | 19 ++++++++++++++----- .../spec.md | 3 ++- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md index 3046da83964..026f2afafd4 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md @@ -1,7 +1,9 @@ # Findings: diff-scoped strict typecheck gate **Issue**: dotCMS/core#37401 · **Spec**: [spec.md](./spec.md) · **Date**: 2026-09-07 -**Status**: in progress — User Stories 1 and 2 complete; US3 (decision matrix) and US4 (templates) pending. +**Status**: complete — all four user stories reported. US1/US2 (mechanism, corpus, adjudication), +US3 (decision matrix, §6), US4 (template arm, §7). SC-005's runtime budget is the one criterion +not met, accepted as a deviation and deferred to #37448 (§13). Every number below was produced by `core-web/tools/scripts/strict-gate/`, replaying real merged pull requests. Nothing here is estimated. @@ -460,9 +462,16 @@ and the validation guide did not name. ## 14. Still open -- **US3** — full flag-set × granularity matrix, and the whole-file adoption cost. -- **US4** — Angular template strictness: cost, and go/no-go. -- **SC-008** — the day-one blocking recommendation. Detection and precision now support it; - runtime does not yet. +Every user story in the spec is reported. What remains is work this spike deliberately did not do, +carried into the follow-up (§12) rather than left unanswered here. + +- **SC-005 — the runtime budget.** 11.4s and 12.0s at the tail against a 10s budget. The three + untried optimisations are named in §5 and §10; deferred to #37448, which gates the blocking flip + on a measured tail. Accepted as a deviation in §13, not an open question. +- **SC-008 — the blocking flip, not the recommendation.** The recommendation is settled in §10: + **GO for the TypeScript arm, non-blocking first.** Detection and precision support day-one + blocking; runtime does not, so the flip waits on the line above. +- **Templates — advisory or deferred.** §7 measures the cost and recommends NO-GO for day-one + blocking; which of the two postures ships is a call for the follow-up. - **#37086** (`libs/sdk/angular`: `strict: true`, none of the extras) — the intermediate tier, deliberately excluded from the corpus so it could not contaminate the false-positive denominator. diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/spec.md b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md index 7a97990f552..f53da01b2fe 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/spec.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/spec.md @@ -15,7 +15,8 @@ ## Problem Statement *(mandatory)* The `core-web` workspace is only partly strict: the shared TypeScript baseline turns strict -mode **off**, and 22 of 55 project configs opt back in locally. The workspace-wide migration +mode **off**, and 22 of 55 TypeScript project configs opt back in locally (the workspace has +56 Nx projects; one of them ships no `tsconfig.json`). The workspace-wide migration (PR #37198, 1455 files) is waiting on full-team QA and is not imminent. While it waits, **new non-strict code keeps landing on `main`**. Every sync from `main` into From 7feb5098e6da7599f1bedc171875a19effef2e45 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 8 Sep 2026 09:13:12 -0400 Subject: [PATCH 6/8] fix(37401): reject unknown gate options instead of degrading silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the spike harness found three silent degradations — each one produced a plausible result and a plausible exit code, which is the failure mode section 7 of findings.md was written about. Correctness - An unknown --granularity fell through the `=== 'line'` test in filter.mjs and behaved as whole-file, while the report still echoed the name it was given. Section 6 measures whole-file at 83% inherited findings, and a plural typo ("lines") was enough to trigger it. - check-ng resolved an unknown flag set to `strict` where check-ts threw on the same input, so a --templates run could measure one flag set and report another. Both paths now share resolveFlagSet. - run.mjs validated --format but not --flags or --granularity; all three are now checked against closed sets at the CLI edge. - The merge-base fallback in changed-files.mjs reinstates the two-tree comparison its own comment warns against. It still degrades, but now warns on stderr and reports baseResolution so the numbers can be distrusted rather than believed. - readProjects stranded its temp directory whenever `nx graph` threw. - Dropped an unreachable `?? siblings[0]` in config-select, and guarded the filter's dirname fallback against a path with no separator. Four regression tests cover the two silent fallbacks. Simplification - Extracted parseConfigFile into resolve-tools: both call sites passed byte-identical arguments including the diagnostic swallow. - Named the ignored-diagnostic sum the two formatters each spelled out. - Hoisted the pure predicates and the ranking table out of selectConfigs, which closes over none of them (130 lines to 95). Tests: 97/97 with --test-concurrency=1 (93 before, 4 added). Behaviour is unchanged for every valid invocation. Co-Authored-By: Claude Opus 5 (1M context) --- .../tools/scripts/strict-gate/filter.test.mjs | 28 ++++++ .../scripts/strict-gate/lib/changed-files.mjs | 28 ++++-- .../scripts/strict-gate/lib/check-ng.mjs | 4 +- .../scripts/strict-gate/lib/check-ts.mjs | 25 ++++-- .../scripts/strict-gate/lib/config-select.mjs | 86 ++++++++++--------- .../tools/scripts/strict-gate/lib/filter.mjs | 18 +++- .../tools/scripts/strict-gate/lib/format.mjs | 16 +++- .../scripts/strict-gate/lib/project-map.mjs | 27 +++--- .../scripts/strict-gate/lib/resolve-tools.mjs | 18 ++++ core-web/tools/scripts/strict-gate/run.mjs | 30 +++++-- .../strict-gate/strict-override.test.mjs | 29 ++++++- 11 files changed, 231 insertions(+), 78 deletions(-) diff --git a/core-web/tools/scripts/strict-gate/filter.test.mjs b/core-web/tools/scripts/strict-gate/filter.test.mjs index 2a719a906c0..363773fc50e 100644 --- a/core-web/tools/scripts/strict-gate/filter.test.mjs +++ b/core-web/tools/scripts/strict-gate/filter.test.mjs @@ -256,3 +256,31 @@ test('whole-file granularity is a strict superset of line granularity', () => { const fileKeys = new Set(byFile.map((f) => `${f.file}:${f.line}`)); for (const f of byLine) assert.ok(fileKeys.has(`${f.file}:${f.line}`)); }); + +/** + * Regression: an unrecognised granularity used to fall through the `=== 'line'` test and behave as + * whole-file — reporting pre-existing debt on untouched lines while the report still echoed the + * name it was given. A plural typo was enough. It must fail by name instead. + */ +test('an unknown granularity is rejected rather than treated as whole-file', () => { + for (const granularity of ['lines', 'Line', 'per-line', '']) { + assert.throws( + () => + filterDiagnostics({ + diagnostics: [diag('libs/mine/src/a.ts', 99, 'TS2345')], + changedFiles: changed, + granularity + }), + /unknown granularity/, + `granularity '${granularity}' should be rejected` + ); + } +}); + +test('the two supported granularities are still accepted', () => { + for (const granularity of ['file', 'line']) { + assert.doesNotThrow(() => + filterDiagnostics({ diagnostics: [], changedFiles: changed, granularity }) + ); + } +}); diff --git a/core-web/tools/scripts/strict-gate/lib/changed-files.mjs b/core-web/tools/scripts/strict-gate/lib/changed-files.mjs index 2b79fa8d3eb..61448acf596 100644 --- a/core-web/tools/scripts/strict-gate/lib/changed-files.mjs +++ b/core-web/tools/scripts/strict-gate/lib/changed-files.mjs @@ -49,7 +49,12 @@ export async function ensureBaseRef({ repoDir, base }) { ); } -/** Added/modified line spans, 1-based inclusive, from a zero-context diff. */ +/** + * Added/modified line spans, 1-based inclusive, from a zero-context diff. + * + * `base` must already be the merge base — `resolveChangedFiles` resolves it before calling here. + * Passing a branch name would compare two trees and attribute the base's changes to this diff. + */ export async function changedLinesFor({ repoDir, base, head, file }) { const { stdout } = await git([ '-C', repoDir, 'diff', '--unified=0', '--no-color', `${base}..${head}`, '--', file @@ -59,7 +64,8 @@ export async function changedLinesFor({ repoDir, base, head, file }) { /** * @param {{ repoDir: string, base: string, head?: string }} options - * @returns {Promise<{ files: object[], base: string, head: string }>} + * @returns {Promise<{ files: object[], base: string, head: string, + * baseResolution: 'merge-base'|'base-tip' }>} */ export async function resolveChangedFiles({ repoDir, base, head = 'HEAD' }) { await ensureBaseRef({ repoDir, base }); @@ -73,9 +79,19 @@ export async function resolveChangedFiles({ repoDir, base, head = 'HEAD' }) { // blames the author for violations someone else merged. Resolving the merge base up front means // the report also CITES the point of divergence, which is what makes a re-run reproducible. const mergeBase = await git(['-C', repoDir, 'merge-base', base, headSha], { allowFailure: true }); - const baseSha = mergeBase.exitCode === 0 && mergeBase.stdout.trim() - ? mergeBase.stdout.trim() - : await sha(base); + const resolvedMergeBase = mergeBase.exitCode === 0 ? mergeBase.stdout.trim() : ''; + + // Falling back to the tip of base reinstates the very two-tree comparison the note above warns + // about, so it says so out loud. Silent degradation here is what produced the run that reported + // 50 findings, essentially none of them the branch's own (§7). + if (!resolvedMergeBase) { + process.stderr.write( + `strict-gate: warning — no merge base between '${base}' and head; comparing against the ` + + `tip of '${base}' instead. Findings may include changes the base introduced.\n` + ); + } + const baseSha = resolvedMergeBase || (await sha(base)); + const baseResolution = resolvedMergeBase ? 'merge-base' : 'base-tip'; // -M so a rename is reported at its new path; ACMR so deletions never appear — there is // nothing to typecheck in a file that no longer exists at head. @@ -98,5 +114,5 @@ export async function resolveChangedFiles({ repoDir, base, head = 'HEAD' }) { }); } - return { files, base: baseSha, head: headSha }; + return { files, base: baseSha, head: headSha, baseResolution }; } diff --git a/core-web/tools/scripts/strict-gate/lib/check-ng.mjs b/core-web/tools/scripts/strict-gate/lib/check-ng.mjs index 9ca68765726..b02077f2272 100644 --- a/core-web/tools/scripts/strict-gate/lib/check-ng.mjs +++ b/core-web/tools/scripts/strict-gate/lib/check-ng.mjs @@ -11,7 +11,7 @@ */ import path from 'node:path'; import { loadAngularCompiler, loadTypeScript } from './resolve-tools.mjs'; -import { FLAG_SETS } from './check-ts.mjs'; +import { resolveFlagSet } from './check-ts.mjs'; /** * The four settings the workspace already treats as its Angular convention: 30 project configs @@ -64,7 +64,7 @@ export async function checkAngularTemplates({ configPath, flagSet = 'strict', fo const ts = await loadTypeScript(); const overrides = { - ...(FLAG_SETS[flagSet] ?? FLAG_SETS.strict), + ...resolveFlagSet(flagSet), ...(forceTemplates ? ANGULAR_STRICT : {}), noEmit: true }; diff --git a/core-web/tools/scripts/strict-gate/lib/check-ts.mjs b/core-web/tools/scripts/strict-gate/lib/check-ts.mjs index 63a52171e25..61eb7038f4c 100644 --- a/core-web/tools/scripts/strict-gate/lib/check-ts.mjs +++ b/core-web/tools/scripts/strict-gate/lib/check-ts.mjs @@ -5,7 +5,7 @@ * central promise that the working tree is byte-identical afterwards (SC-010). */ import path from 'node:path'; -import { loadTypeScript } from './resolve-tools.mjs'; +import { loadTypeScript, parseConfigFile } from './resolve-tools.mjs'; /** * `--strict` is an umbrella over eight flags and does NOT include the four below. Verified against @@ -38,6 +38,21 @@ export const FLAG_SETS = { } }; +/** + * The single gate on flag-set names. Both checkers route through it so an unknown name can never + * resolve to a default: the run would then measure one flag set while the report named another, + * and the number would be wrong in a way nothing surfaces. + */ +export function resolveFlagSet(flagSet) { + const overrides = FLAG_SETS[flagSet]; + if (!overrides) { + throw new Error( + `unknown flag set '${flagSet}' — one of ${Object.keys(FLAG_SETS).join(', ')}` + ); + } + return overrides; +} + export function toDiagnostic(ts, diagnostic, layer = 'source') { const file = diagnostic.file; const { line, character } = file && diagnostic.start !== undefined @@ -59,13 +74,9 @@ export function toDiagnostic(ts, diagnostic, layer = 'source') { */ export async function checkTypeScript({ configPath, flagSet = 'strict' }) { const ts = await loadTypeScript(); - const overrides = FLAG_SETS[flagSet]; - if (!overrides) throw new Error(`unknown flag set '${flagSet}'`); + const overrides = resolveFlagSet(flagSet); - const parsed = ts.getParsedCommandLineOfConfigFile(configPath, {}, { - ...ts.sys, - onUnRecoverableConfigFileDiagnostic: () => {} - }); + const parsed = await parseConfigFile(configPath); if (!parsed) throw new Error(`cannot parse ${configPath}`); const program = ts.createProgram({ diff --git a/core-web/tools/scripts/strict-gate/lib/config-select.mjs b/core-web/tools/scripts/strict-gate/lib/config-select.mjs index 402e12b7c6e..f467af2520f 100644 --- a/core-web/tools/scripts/strict-gate/lib/config-select.mjs +++ b/core-web/tools/scripts/strict-gate/lib/config-select.mjs @@ -8,53 +8,61 @@ */ import fs from 'node:fs/promises'; import path from 'node:path'; -import { loadTypeScript } from './resolve-tools.mjs'; +import { parseConfigFile } from './resolve-tools.mjs'; + +const TSCONFIG_NAME = /^tsconfig\..*\.json$|^tsconfig\.json$/; + +/** + * `tsconfig.editor.json` is generated by Nx for IDE language services. It globs the whole project, + * so it will happily claim any file — and it carries no `angularCompilerOptions`, which makes + * template strictness unreachable for anything it swallows. A last resort, never a stand-in for + * the build config. + */ +const isIdeOnly = (configPath) => path.basename(configPath) === 'tsconfig.editor.json'; + +/** + * Preference order among configs that could host a file none of them names outright. + * + * The spec config is ranked LAST on purpose. Angular colocates `x.component.ts`, + * `x.component.html` and `x.component.spec.ts`, so any "owns TypeScript in this directory" + * rule matches the spec config as readily as the build config — and the spec config carries no + * `angularCompilerOptions`, so a template routed there goes unchecked behind a reported target + * and a PASS. + */ +const CONFIG_RANK = [ + 'tsconfig.app.json', + 'tsconfig.lib.json', + 'tsconfig.json', + 'tsconfig.spec.json', + 'tsconfig.editor.json' +]; + +/** Unknown config names rank alongside the spec config: plausible host, never a preferred one. */ +const rankOf = (configPath) => { + const index = CONFIG_RANK.indexOf(path.basename(configPath)); + return index === -1 ? CONFIG_RANK.indexOf('tsconfig.spec.json') : index; +}; + +/** + * Templates need a different rule from sources: a tsconfig's resolved file list contains only + * TypeScript, so a `.html` is never in it. Matching sources by file list and then dropping + * templates would make a template-only pull request resolve zero configs and pass silently — + * which reads exactly like "nothing to check". + */ +const isTemplate = (f) => path.extname(f) === '.html'; /** * @param {{ workspaceDir: string, project: {name:string,root:string}, files: string[], repoDir?: string }} input * @returns {Promise<{project:string,root:string,configPath:string,files:string[]}[]>} */ export async function selectConfigs({ workspaceDir, project, files, repoDir = workspaceDir }) { - const ts = await loadTypeScript(); const projectDir = path.resolve(repoDir, project.root); const candidates = (await fs.readdir(projectDir)) - .filter((name) => /^tsconfig\..*\.json$|^tsconfig\.json$/.test(name)) + .filter((name) => TSCONFIG_NAME.test(name)) .map((name) => path.join(projectDir, name)) .sort(); - // `tsconfig.editor.json` is generated by Nx for IDE language services. It globs the whole - // project, so it will happily claim any file — and it carries no `angularCompilerOptions`, - // which makes template strictness unreachable for anything it swallows. It is a last resort, - // never a stand-in for the build config. - const isIdeOnly = (configPath) => path.basename(configPath) === 'tsconfig.editor.json'; - - /** - * Preference order among configs that could host a file none of them names outright. - * - * The spec config is ranked LAST on purpose. Angular colocates `x.component.ts`, - * `x.component.html` and `x.component.spec.ts`, so any "owns TypeScript in this directory" - * rule matches the spec config as readily as the build config — and the spec config carries no - * `angularCompilerOptions`, so a template routed there goes unchecked behind a reported target - * and a PASS. - */ - const CONFIG_RANK = [ - 'tsconfig.app.json', - 'tsconfig.lib.json', - 'tsconfig.json', - 'tsconfig.spec.json', - 'tsconfig.editor.json' - ]; - const rankOf = (configPath) => { - const index = CONFIG_RANK.indexOf(path.basename(configPath)); - return index === -1 ? CONFIG_RANK.indexOf('tsconfig.spec.json') : index; - }; - - // Templates need a different rule: a tsconfig's resolved file list contains only TypeScript, - // so a .html is never in it. Matching sources by file list and then dropping templates would - // make a template-only pull request resolve zero configs and pass silently — which reads - // exactly like "nothing to check". - const isTemplate = (f) => path.extname(f) === '.html'; const sources = files.filter((f) => !isTemplate(f)); const templates = files.filter(isTemplate); @@ -62,10 +70,7 @@ export async function selectConfigs({ workspaceDir, project, files, repoDir = wo const parsedConfigs = []; for (const configPath of candidates) { - const parsed = ts.getParsedCommandLineOfConfigFile(configPath, {}, { - ...ts.sys, - onUnRecoverableConfigFileDiagnostic: () => {} - }); + const parsed = await parseConfigFile(configPath); // A references-only config resolves to zero files and excludes itself with no special-casing. if (!parsed || parsed.fileNames.length === 0) continue; parsedConfigs.push({ configPath, fileNames: parsed.fileNames.map((f) => path.resolve(f)) }); @@ -115,8 +120,9 @@ export async function selectConfigs({ workspaceDir, project, files, repoDir = wo return siblings[0]; } + // `siblings` is a subset of `eligible`, so if it had an entry so does `ranked`. const ranked = [...eligible].sort(byRank); - return ranked[0] ?? siblings[0] ?? null; + return ranked[0] ?? null; }; // Sources an entry-point config owns transitively but never names. diff --git a/core-web/tools/scripts/strict-gate/lib/filter.mjs b/core-web/tools/scripts/strict-gate/lib/filter.mjs index f537c2313d3..3247823076e 100644 --- a/core-web/tools/scripts/strict-gate/lib/filter.mjs +++ b/core-web/tools/scripts/strict-gate/lib/filter.mjs @@ -27,7 +27,17 @@ export const INFRASTRUCTURE_CODES = new Set([ * projectRoots?: string[] * }} input */ +export const GRANULARITIES = new Set(['file', 'line']); + export function filterDiagnostics({ diagnostics, changedFiles, granularity = 'file', projectRoots }) { + // Checked rather than defaulted. An unrecognised value used to fall through the `=== 'line'` + // test and behave as whole-file, which §6 measures at 83% inherited findings — while the report + // still echoed the name that was asked for. A plural typo was enough to trigger it. + if (!GRANULARITIES.has(granularity)) { + throw new Error( + `unknown granularity '${granularity}' — one of ${[...GRANULARITIES].join(', ')}` + ); + } const changed = new Map(changedFiles.map((f) => [f.path, f])); // Distinguishing "another project's file" from "an untouched file of this project" needs to @@ -36,7 +46,13 @@ export function filterDiagnostics({ diagnostics, changedFiles, granularity = 'fi const owned = projectRoots?.length ? (file) => projectRoots.some((r) => file === r || file.startsWith(`${r}/`)) : (() => { - const dirs = new Set(changedFiles.map((f) => f.path.slice(0, f.path.lastIndexOf('/')))); + // A repo-relative path always has a slash; guarding anyway keeps a path that + // somehow does not from producing an empty prefix that matches everything. + const dirs = new Set( + changedFiles + .map((f) => f.path.slice(0, f.path.lastIndexOf('/'))) + .filter((d) => d.length > 0) + ); return (file) => [...dirs].some((d) => file.startsWith(`${d}/`)); })(); diff --git a/core-web/tools/scripts/strict-gate/lib/format.mjs b/core-web/tools/scripts/strict-gate/lib/format.mjs index 936e03576a9..14d3e044097 100644 --- a/core-web/tools/scripts/strict-gate/lib/format.mjs +++ b/core-web/tools/scripts/strict-gate/lib/format.mjs @@ -34,6 +34,14 @@ const scopeRule = (granularity) => ? 'Only lines this pull request ADDED OR MODIFIED are checked. Pre-existing problems on untouched lines are deliberately ignored.' : 'Every line of a changed file is checked, including pre-existing problems on lines this pull request did not touch.'; +/** + * Diagnostics the gate deliberately did not report — dependency code plus untouched lines. + * Infrastructure discards are excluded: they are not debt anyone is being forgiven, they are + * diagnostics that were never strictness violations to begin with. + */ +const ignoredCount = (report) => + report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + function groupByFile(findings) { const byFile = new Map(); for (const f of findings) { @@ -47,7 +55,7 @@ function groupByFile(findings) { /** Plain text — the default, and what an agent reading raw CI logs gets. */ export function formatText(report) { const lines = []; - const total = report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + const total = ignoredCount(report); if (report.exitCode === 0) { lines.push('strict-gate: PASS — no new strict-mode violations in this diff.'); @@ -63,9 +71,9 @@ export function formatText(report) { lines.push(`strict-gate: FAIL — ${report.findings.length} new strict-mode violation(s) introduced by this diff.`); lines.push(''); lines.push('WHY THIS FAILS'); - lines.push(` main is not strict yet, so these files compile today. This gate checks the code`); + lines.push(' main is not strict yet, so these files compile today. This gate checks the code'); lines.push(` THIS pull request writes against ${FLAG_SET_LABEL[report.flagSet] ?? report.flagSet},`); - lines.push(` so new code stops adding to the debt the strict-mode migration has to clear.`); + lines.push(' so new code stops adding to the debt the strict-mode migration has to clear.'); lines.push(''); lines.push('SCOPE — READ BEFORE FIXING'); lines.push(` ${scopeRule(report.granularity)}`); @@ -108,7 +116,7 @@ export function formatGithub(report) { /** Markdown for the job summary — what a human opening the run sees first. */ export function formatMarkdown(report) { - const total = report.discarded.byOrigin.dependency + report.discarded.byOrigin.untouched; + const total = ignoredCount(report); if (report.exitCode === 0) { return [ '## ✅ strict-gate: pass', diff --git a/core-web/tools/scripts/strict-gate/lib/project-map.mjs b/core-web/tools/scripts/strict-gate/lib/project-map.mjs index 8120a501be3..94087371d24 100644 --- a/core-web/tools/scripts/strict-gate/lib/project-map.mjs +++ b/core-web/tools/scripts/strict-gate/lib/project-map.mjs @@ -46,18 +46,21 @@ export function mapFilesToProjects({ projects, files }) { * @returns {Promise<{name:string,root:string}[]>} roots relative to `repoDir`. */ export async function readProjects({ workspaceDir, repoDir }) { - const out = path.join(await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-graph-')), 'graph.json'); - await run('node', [resolveBin('nx'), 'graph', '--file', out], { cwd: workspaceDir }); + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'strict-gate-graph-')); + const out = path.join(dir, 'graph.json'); + try { + await run('node', [resolveBin('nx'), 'graph', '--file', out], { cwd: workspaceDir }); - const graph = JSON.parse(await fs.readFile(out, 'utf8')); - const nodes = graph.graph?.nodes ?? graph.nodes ?? {}; - const prefix = path.relative(repoDir, workspaceDir); + const graph = JSON.parse(await fs.readFile(out, 'utf8')); + const nodes = graph.graph?.nodes ?? graph.nodes ?? {}; + const prefix = path.relative(repoDir, workspaceDir); - const projects = Object.entries(nodes) - .map(([name, node]) => ({ name, root: node?.data?.root })) - .filter((p) => typeof p.root === 'string' && p.root.length > 0) - .map((p) => ({ name: p.name, root: prefix ? path.join(prefix, p.root) : p.root })); - - await fs.rm(path.dirname(out), { recursive: true, force: true }); - return projects; + return Object.entries(nodes) + .map(([name, node]) => ({ name, root: node?.data?.root })) + .filter((p) => typeof p.root === 'string' && p.root.length > 0) + .map((p) => ({ name: p.name, root: prefix ? path.join(prefix, p.root) : p.root })); + } finally { + // Ran on the throwing path too: `nx graph` failing used to strand the directory. + await fs.rm(dir, { recursive: true, force: true }); + } } diff --git a/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs b/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs index b9306b6f91c..790dbf44bc6 100644 --- a/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs +++ b/core-web/tools/scripts/strict-gate/lib/resolve-tools.mjs @@ -69,6 +69,24 @@ export function resolveBin(specifier, binName = specifier) { return path.resolve(path.dirname(pkgPath), entry); } +/** + * Parses a tsconfig the way both checking paths need it. + * + * The unrecoverable-diagnostic hook is a no-op on purpose: a malformed or unreadable config must + * not abort the run with a raw TypeScript diagnostic. Callers decide what a failed parse means — + * `check-ts` throws because it was asked to check that exact config, while `config-select` skips + * the candidate because it is only surveying which configs exist. + * + * @returns {import('typescript').ParsedCommandLine | undefined} + */ +export async function parseConfigFile(configPath) { + const ts = await loadTypeScript(); + return ts.getParsedCommandLineOfConfigFile(configPath, {}, { + ...ts.sys, + onUnRecoverableConfigFileDiagnostic: () => {} + }); +} + /** * Reports what the harness is actually running against. Recorded in the report so a measurement * can always be traced back to the toolchain that produced it. diff --git a/core-web/tools/scripts/strict-gate/run.mjs b/core-web/tools/scripts/strict-gate/run.mjs index 386c46e4b3c..e9b9bb76250 100644 --- a/core-web/tools/scripts/strict-gate/run.mjs +++ b/core-web/tools/scripts/strict-gate/run.mjs @@ -9,10 +9,10 @@ import { workspaceRoot } from './lib/resolve-tools.mjs'; import { resolveChangedFiles } from './lib/changed-files.mjs'; import { mapFilesToProjects, readProjects } from './lib/project-map.mjs'; import { selectConfigs } from './lib/config-select.mjs'; -import { checkTypeScript } from './lib/check-ts.mjs'; +import { checkTypeScript, FLAG_SETS } from './lib/check-ts.mjs'; import { checkAngularTemplates } from './lib/check-ng.mjs'; import { selectMode } from './lib/mode-select.mjs'; -import { filterDiagnostics } from './lib/filter.mjs'; +import { filterDiagnostics, GRANULARITIES } from './lib/filter.mjs'; import { buildReport } from './lib/report.mjs'; import { FORMATTERS } from './lib/format.mjs'; @@ -78,7 +78,7 @@ export async function collectDiagnostics({ // report as having done so, because a silent fallback means unchecked templates behind // a PASS. const decision = await selectMode({ configPath: config.configPath, templates }); - const started = performance.now(); + const checkStarted = performance.now(); const { diagnostics: raw } = decision.mode === 'template-aware' @@ -89,7 +89,7 @@ export async function collectDiagnostics({ flagSet }); - const elapsed = performance.now() - started; + const elapsed = performance.now() - checkStarted; if (decision.mode === 'template-aware') templateAwareMs += elapsed; else typescriptMs += elapsed; @@ -176,12 +176,32 @@ function parseArgs(argv) { return options; } +/** + * Every enumerated option is checked against a closed set here, at the edge. + * + * `--format` was already guarded; `--flags` and `--granularity` were not, and the two failed + * differently. An unknown flag set reached the checkers, where the TypeScript path threw but the + * template path defaulted to `strict` — measuring one thing and reporting another. An unknown + * granularity reached the filter and behaved as whole-file, which §6 measures at 83% inherited + * findings, while the report still echoed the name it was given. Both are now rejected by name. + */ +function validateOptions({ format, flagSet, granularity }) { + const oneOf = (label, value, allowed) => { + if (value !== undefined && !allowed.includes(value)) { + throw new Error(`unknown ${label} '${value}' — one of ${allowed.join(', ')}`); + } + }; + oneOf('format', format, Object.keys(FORMATTERS)); + oneOf('flag set', flagSet, Object.keys(FLAG_SETS)); + oneOf('granularity', granularity, [...GRANULARITIES]); +} + async function main(argv) { const { report: reportPath, format = 'text', ...options } = parseArgs(argv); if (!options.base) throw new Error('--base is required'); + validateOptions({ format, flagSet: options.flagSet, granularity: options.granularity }); const render = FORMATTERS[format]; - if (!render) throw new Error(`unknown format '${format}' — one of ${Object.keys(FORMATTERS).join(', ')}`); const report = await runGate(options); diff --git a/core-web/tools/scripts/strict-gate/strict-override.test.mjs b/core-web/tools/scripts/strict-gate/strict-override.test.mjs index 883f58713a5..21f2d071a6a 100644 --- a/core-web/tools/scripts/strict-gate/strict-override.test.mjs +++ b/core-web/tools/scripts/strict-gate/strict-override.test.mjs @@ -10,7 +10,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs/promises'; import path from 'node:path'; import { makeWorkspace } from './fixtures/make-workspace.mjs'; -import { checkTypeScript } from './lib/check-ts.mjs'; +import { checkTypeScript, resolveFlagSet, FLAG_SETS } from './lib/check-ts.mjs'; import { checkAngularTemplates } from './lib/check-ng.mjs'; import { makeNgProject } from './fixtures/make-ng-project.mjs'; @@ -199,3 +199,30 @@ test('template-aware checking leaves every configuration file byte-identical', a assert.equal(await fs.readFile(configPath, 'utf8'), before.lib); assert.equal(await fs.readFile(rootConfig, 'utf8'), before.root, 'strictTemplates:false must still say false'); }); + +/** + * Regression: `checkAngularTemplates` used to resolve an unknown flag set to `strict` while + * `checkTypeScript` threw on the same input. The template run would then measure one flag set and + * the report would name another — a wrong number with nothing to surface it. Both paths now share + * `resolveFlagSet`, so both reject. + */ +test('an unknown flag set is rejected, and identically on both checking paths', async () => { + assert.throws(() => resolveFlagSet('typo'), /unknown flag set/); + assert.throws(() => resolveFlagSet(undefined), /unknown flag set/); + + await assert.rejects( + () => checkTypeScript({ configPath: 'unused.json', flagSet: 'typo' }), + /unknown flag set/ + ); + await assert.rejects( + () => checkAngularTemplates({ configPath: 'unused.json', flagSet: 'typo' }), + /unknown flag set/ + ); +}); + +test('every named flag set resolves to a non-empty option object', () => { + for (const name of Object.keys(FLAG_SETS)) { + const options = resolveFlagSet(name); + assert.ok(Object.keys(options).length > 0, `${name} should force at least one option`); + } +}); From 5090028f9e399a40246e6d41491ca2f6fb2ecf93 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 8 Sep 2026 09:49:46 -0400 Subject: [PATCH 7/8] docs(37401): document how to retire the gate when #37198 merges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate is scaffolding with an expiry date: it only has a job while core-web/tsconfig.base.json is non-strict. Nothing recorded when or how to remove it, so it would have outlived its reason by default. Adds DECOMMISSION.md with the trigger (#37198 merged AND the baseline actually strict — verify both), the full inventory of what comes out (31 harness files, 5 tracked spec files, the gitignored spec artifacts, and the pom.xml/lint-staged hooks that exist only if #37448 promoted the gate), the removal commands, and verification that leaves nothing behind. Two things the procedure records because they are easy to get wrong: - #37198 makes the baseline strict but adds no mechanism that RUNS a type-check — it touches no nx.json, project.json, pom.xml or workflow. Only 5 of 56 projects have a typecheck target, all inferred by @nx/vite/plugin and none declared. Deleting on the trigger alone leaves 51 projects strict on paper with nothing compiling them, so the procedure calls for replace-then-delete. - findings.md must be archived onto #37401 before the directory goes: it is the only record of the measured false-positive rate, the 83% whole-file inheritance cost and the template-arm cost. Linked from the harness README banner and findings.md §12 so it is found from either direction. Co-Authored-By: Claude Opus 5 (1M context) --- core-web/tools/scripts/strict-gate/README.md | 12 ++ .../DECOMMISSION.md | 197 ++++++++++++++++++ .../findings.md | 5 + 3 files changed, 214 insertions(+) create mode 100644 specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md diff --git a/core-web/tools/scripts/strict-gate/README.md b/core-web/tools/scripts/strict-gate/README.md index d213ea27422..081fed1fe1c 100644 --- a/core-web/tools/scripts/strict-gate/README.md +++ b/core-web/tools/scripts/strict-gate/README.md @@ -1,5 +1,11 @@ # strict-gate — spike harness (issue #37401) +> **Temporary by design — delete this when #37198 merges.** +> This gate exists only while `core-web/tsconfig.base.json` is non-strict. Once the +> workspace-wide strict migration lands, every file in this directory comes out. +> 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: > Can a diff-scoped strict typecheck block new non-strict TypeScript from landing on `main`, @@ -73,3 +79,9 @@ Full measurements, adjudication of every finding, and the go/no-go: 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. + +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 +[`DECOMMISSION.md`](../../../../specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md). +Note the precondition in §2: #37198 makes the baseline strict but adds nothing that *runs* a +type-check, so removal should follow a replacement, not precede one. diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md b/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md new file mode 100644 index 00000000000..9eb84678bca --- /dev/null +++ b/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md @@ -0,0 +1,197 @@ +# Decommissioning the strict-gate + +**This gate is scaffolding with an expiry date.** It exists to stop new non-strict TypeScript +landing on `main` *while* the workspace-wide strict migration waits for QA. When that migration +merges, the reason for the scaffolding is gone and every artifact listed here comes out. + +This file is the removal procedure. It is written to be executable by someone — or something — +with no memory of why the gate was built. + +**Issue**: dotCMS/core#37401 · **Trigger**: dotCMS/core#37198 · **Follow-up**: dotCMS/core#37448 + +--- + +## 1. The trigger + +Remove the gate when **PR #37198 (`35932-enable-strict-mode-v3`, epic #35932) is merged to `main`** +and `core-web/tsconfig.base.json` on `main` carries `"strict": true`. + +Confirm both, do not assume either: + +```bash +gh pr view 37198 --repo dotCMS/core --json state,mergedAt --jq '{state, mergedAt}' +git fetch origin main +git show origin/main:core-web/tsconfig.base.json | grep -A1 '"strict"' +``` + +`state: MERGED` **and** `"strict": true` in the baseline. If the PR merged but the baseline is +still `false`, the migration was split or reverted — stop and find out which before deleting +anything. + +## 2. Precondition — read this before deleting + +Removing the gate on the trigger alone reopens a hole the spike discovered by accident. +`findings.md` §4 is the relevant finding, and it is counter-intuitive: + +> **Declaring `strict: true` does not mean anything compiles it.** + +Verified again while writing this file: + +| Fact | Value | +|---|---| +| Nx projects in `core-web` | 56 | +| Projects with a `typecheck` target | **5** — `edit-content-bridge`, `sdk-experiments`, `sdk-analytics`, `sdk-vue`, and the `core-web` root | +| …declared in a `project.json` | **0** — all five are inferred by `@nx/vite/plugin` | +| Files #37198 changes | 77 `.ts`, 13 `.html`, 8 `.json`, 1 `.prettierignore`, 1 `.md` | +| Does #37198 touch `nx.json`, any `project.json`, `pom.xml` or a workflow? | **No** | + +> `findings.md` §4 reports "3 of 57" for this. The spike measured it earlier; re-running +> `pnpm nx show projects --with-target typecheck` while writing this file returns five. The +> discrepancy does not change the argument — verify the current number yourself with the command +> in the next block rather than trusting either figure. + +So #37198 makes the configuration strict and fixes the existing violations — but it adds no +mechanism that *runs* a type-check. Lint does not type-check. After the merge, **51 of 56 projects** +are strict on paper with nothing in CI compiling them, and type errors can accumulate again from +the next pull request onward. + +**Before deleting, verify something else type-checks the workspace:** + +```bash +# Expect substantially more than the 5 above, or a CI step running tsc across the workspace. +cd core-web && NX_NO_CLOUD=true pnpm nx show projects --with-target typecheck +grep -rn "typecheck\|tsc --noEmit" pom.xml ../.github/workflows/ | grep -v node_modules +``` + +If nothing covers it, the honest sequence is **replace, then delete** — not delete and hope. +Deleting first is still a valid choice, but make it knowingly and say so in the removal PR. + +## 3. What comes out + +Everything below was created for this gate and has no other consumer. Verified: nothing outside +these two directories references `strict-gate` — not `core-web/pom.xml`, not `nx.json`, not +`lint-staged.config.mjs`, not any workflow, not `docs/`, not `.cursor/rules/`. + +### 3.1 The harness — 31 tracked files + +``` +core-web/tools/scripts/strict-gate/ +├── run.mjs replay.mjs corpus.mjs writeup.check.mjs README.md +├── lib/ 13 modules +├── fixtures/ 3 builders +└── *.test.mjs 9 suites +``` + +### 3.2 The spec directory — 5 tracked files + +``` +specs/37401-diff-scoped-strict-typecheck-gate/ +├── spec.md findings.md data-model.md DECOMMISSION.md (this file) +└── contracts/cli.md contracts/report.schema.json +``` + +Plus these, present locally but **gitignored** (`specs/*/plan.md` etc.) — they disappear with the +directory and are in no commit: + +``` +plan.md research.md tasks.md quickstart.md checklists/ +``` + +### 3.3 Only if the follow-up (#37448) promoted the gate + +The spike wires the harness into **nothing**. If #37448 shipped the production gate first, these +exist and must come out too — check each before assuming it does not: + +| Location | What to remove | +|---|---| +| `core-web/pom.xml` | the `` with `strict-gate`, next to `lint-test` / `format-test` in the `generate-resources` phase | +| `core-web/lint-staged.config.mjs` | any `strict-gate` entry in the `**/*.{ts,js,mjs,...}` task list | +| `.github/workflows/` | only if a step was added; §12 of `findings.md` records that none was needed | +| wherever the durable script landed | the promoted copy, if it moved out of `tools/scripts/` | + +## 4. What to keep + +**Archive `findings.md` before deleting it.** It is the only record of measurements that cost more +than the spike's timebox (§11) and that justify decisions outliving the gate: the 0-of-11 +false-positive rate, the 83 % whole-file inheritance cost, the 2.2× template-checking cost, and +the `typecheck`-coverage finding in §2 above. Losing it means re-running the spike to answer the +same questions. + +```bash +gh issue comment 37401 --repo dotCMS/core \ + --body-file specs/37401-diff-scoped-strict-typecheck-gate/findings.md +``` + +The issue outlives the directory. Do this **before** step 5, not after. + +## 5. The removal + +```bash +git switch -c "removal/37401-retire-strict-gate" origin/main +git rm -r core-web/tools/scripts/strict-gate +git rm -r specs/37401-diff-scoped-strict-typecheck-gate +``` + +Commit message — say what made it removable, so the history explains itself: + +``` +chore(37401): retire the diff-scoped strict typecheck gate + +#37198 merged and core-web/tsconfig.base.json is now strict: true, so the +diff-scoped gate has no remaining job: new code is held to the workspace +baseline like every other line. + +Removes the spike harness (core-web/tools/scripts/strict-gate/) and its +spec directory. findings.md is archived on #37401 — it holds the measured +false-positive rate, the granularity cost, and the typecheck-coverage +finding, none of which are reproduced by anything left in the repo. + +Closes #37401 +``` + +## 6. Verify nothing is left + +Every command must come back empty. Run them from the repo root. + +```bash +# 1. No file references the gate. The only expected hit is unrelated: +# dotcms-postman/.../historical-event.json matches on the id 3740143, not on #37401. +grep -rIl "strict-gate\|37401" . \ + --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=dist --exclude-dir=.nx + +# 2. Both directories are gone, from the worktree and from the index. +ls core-web/tools/scripts/strict-gate specs/37401-diff-scoped-strict-typecheck-gate 2>&1 +git ls-files core-web/tools/scripts/strict-gate specs/37401-diff-scoped-strict-typecheck-gate + +# 3. The build is unaffected — it never referenced the gate, and this proves it still does not. +cd core-web && NX_NO_CLOUD=true pnpm nx format:check --all +``` + +Then let CI confirm it: the removal touches no build input, so a green `PR Test / Frontend Unit +Tests` and `PR Build / Initial Artifact Build` is the whole verification story. + +## 7. Issues to settle + +| Issue | Action | +|---|---| +| **#37401** | Close. Archive `findings.md` on it first (§4). | +| **#37448** | Close as obsolete **if** the gate was never promoted. If it was, the follow-up's own work is what §3.3 removes — close it with a note pointing at the removal PR. | +| **#37086** | Independent of the gate (`libs/sdk/angular`, the intermediate tier). Leave open. | +| **#35930** | `TODO(#35930)`, the four apps with `strictTemplates: false`. **Not addressed by #37198** — the template arm was a no-go (§7). Leave open. | + +--- + +## Why the gate does not simply become permanent + +Worth recording, because it is the obvious counter-argument and it was considered. + +`findings.md` §4 makes a real case that the gate's value outlives the migration: it is the only +thing type-checking the 51 projects nothing else compiles, before and after #37198. But a +*diff-scoped* gate is the wrong shape for that job. Its entire design — discarding 99.1 % of +diagnostics, forgiving untouched lines, forcing flags the config does not declare — exists to be +useful while the baseline is **non-strict**. Once the baseline is strict, the right tool is an +ordinary workspace-wide `typecheck` target that compiles each project under its own configuration, +with no diff filter and nothing forced in memory. + +Keeping the diff-scoped gate to fill that gap means maintaining a filter that no longer filters +anything meaningful. Replace it (§2), do not repurpose it. diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md index 026f2afafd4..26569253e55 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/findings.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/findings.md @@ -392,6 +392,11 @@ one of these. But the estimate was wrong and the write-up says so. ## 12. Follow-up +**This gate is scaffolding with an expiry date.** Whatever the follow-up builds is removed when +#37198 merges and the workspace baseline turns strict — the removal procedure, the full inventory, +and the precondition that #37198 adds no mechanism which actually runs a type-check are in +[DECOMMISSION.md](./DECOMMISSION.md). + **Recommendation: build it.** The follow-up task covers: - The durable script, promoted from `core-web/tools/scripts/strict-gate/`. From 9260444b387b1dffee680fe514fff03e84ad9a79 Mon Sep 17 00:00:00 2001 From: Nicolas Molina Monroy Date: Tue, 8 Sep 2026 10:01:23 -0400 Subject: [PATCH 8/8] docs(37401): sharpen the decommission precondition with measured coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The precondition said only that few projects have a typecheck target, which invites the reasonable objection that the build already type-checks. It does — the section now answers that with the coverage numbers instead of leaving it implied. - nx build runs a real ngc/tsc, but only 18 of 57 projects have a build target, and tsconfig.lib.json excludes src/**/*.spec.ts by design. - 8 of the 11 findings the spike adjudicated were in .spec.ts (73%), so the build's blind spot is exactly where this gate earned its keep. - 5 of 51 tsconfig.spec.json set "strict": false themselves, which a strict baseline does not reach; #37198 fixes 1 of those 5. Adds the constraint a replacement must satisfy — include the specs and override the opted-out spec configs, or it reproduces the same blind spot — and records that adding a typecheck to #37198 is the intended fix. Corrects two figures this file inherited from findings.md §4: the typecheck-target count (3, now 5) and the claim that nothing type-checks 54 of 57 projects, which overstates it since the build does cover 18. Flags ts-jest's diagnostics behaviour as unverified rather than assuming it either way. Co-Authored-By: Claude Opus 5 (1M context) --- .../DECOMMISSION.md | 57 ++++++++++++++----- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md b/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md index 9eb84678bca..6f26463c4e3 100644 --- a/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md +++ b/specs/37401-diff-scoped-strict-typecheck-gate/DECOMMISSION.md @@ -35,37 +35,64 @@ Removing the gate on the trigger alone reopens a hole the spike discovered by ac > **Declaring `strict: true` does not mean anything compiles it.** -Verified again while writing this file: +**"But the build type-checks it" is the obvious objection, and it is half true.** `nx build` runs a +real `ngc`/`tsc`, so what it compiles is genuinely checked. It just does not compile most of this +workspace, and it never compiles the half where the spike found its violations: | Fact | Value | |---|---| -| Nx projects in `core-web` | 56 | -| Projects with a `typecheck` target | **5** — `edit-content-bridge`, `sdk-experiments`, `sdk-analytics`, `sdk-vue`, and the `core-web` root | -| …declared in a `project.json` | **0** — all five are inferred by `@nx/vite/plugin` | +| Nx projects in `core-web` | 57 | +| …with a `build` target | **18** — the other 39 have nothing that compiles them | +| …with a `typecheck` target | **5**, all inferred by `@nx/vite/plugin`, **0** declared in a `project.json` | +| Does the build see `.spec.ts`? | **No** — `tsconfig.lib.json` carries `exclude: ["src/**/*.spec.ts", …]` | +| Where the spike's findings lived | **8 of 11 in `.spec.ts`** (§3) — 73 %, in files the build excludes by design | +| `tsconfig.spec.json` files that set `"strict": false` themselves | **5 of 51** — a strict baseline does not reach them | | Files #37198 changes | 77 `.ts`, 13 `.html`, 8 `.json`, 1 `.prettierignore`, 1 `.md` | | Does #37198 touch `nx.json`, any `project.json`, `pom.xml` or a workflow? | **No** | +| …how many `tsconfig.spec.json` does it fix? | **1** (`apps/dotcms-block-editor`), leaving the other 4 opted out | -> `findings.md` §4 reports "3 of 57" for this. The spike measured it earlier; re-running -> `pnpm nx show projects --with-target typecheck` while writing this file returns five. The -> discrepancy does not change the argument — verify the current number yourself with the command -> in the next block rather than trusting either figure. +So #37198 makes the configuration strict and fixes the existing violations, but adds no mechanism +that *runs* a type-check over what the build skips. Lint does not type-check. The gap that outlives +the merge is **the 39 projects with no build plus every `.spec.ts` in the workspace** — which is +where 73 % of what this gate caught was living. -So #37198 makes the configuration strict and fixes the existing violations — but it adds no -mechanism that *runs* a type-check. Lint does not type-check. After the merge, **51 of 56 projects** -are strict on paper with nothing in CI compiling them, and type errors can accumulate again from -the next pull request onward. +> Two figures here supersede earlier ones. `findings.md` §4 says "3 of 57" projects have a +> `typecheck` target; re-running the command below returns 5. §4 also frames the gap as "nothing +> type-checks 54 of 57 projects", which overstates it — the build does cover 18. Re-measure rather +> than trusting any of these numbers; they move. + +**Not verified, and it changes the size of the gap:** whether `ts-jest` reports type errors during +`nx test` or only transpiles. With `jest-preset-angular` 17 it should type-check, which would cover +the specs of the 46 projects whose `tsconfig.spec.json` does not opt out — but this was never +confirmed. Settle it by putting a deliberate type error in a spec and running that project's tests. **Before deleting, verify something else type-checks the workspace:** ```bash -# Expect substantially more than the 5 above, or a CI step running tsc across the workspace. -cd core-web && NX_NO_CLOUD=true pnpm nx show projects --with-target typecheck +cd core-web +# Coverage today. A replacement should close the gap between these two. +NX_NO_CLOUD=true pnpm nx show projects --with-target typecheck +NX_NO_CLOUD=true pnpm nx show projects --with-target build +NX_NO_CLOUD=true pnpm nx show projects | tr ',' '\n' | wc -l + +# Does anything run tsc in the build pipeline? grep -rn "typecheck\|tsc --noEmit" pom.xml ../.github/workflows/ | grep -v node_modules + +# Do the spec configs still opt out of strict? +grep -rl '"strict"[[:space:]]*:[[:space:]]*false' apps libs --include='tsconfig.spec.json' ``` +A replacement only closes the gap if it **includes the specs** and **overrides the spec configs +that set `strict: false`**. A `typecheck` target that runs the build configuration reproduces the +exact blind spot this gate was built to cover. + If nothing covers it, the honest sequence is **replace, then delete** — not delete and hope. Deleting first is still a valid choice, but make it knowingly and say so in the removal PR. +**Status at the time of writing (2026-09-08):** adding a `typecheck` to #37198 itself is the +intended replacement, being handled on that pull request. If it landed, this precondition is +already satisfied — confirm with the commands above rather than assuming, then delete freely. + ## 3. What comes out Everything below was created for this gate and has no other consumer. Verified: nothing outside @@ -186,7 +213,7 @@ Tests` and `PR Build / Initial Artifact Build` is the whole verification story. Worth recording, because it is the obvious counter-argument and it was considered. `findings.md` §4 makes a real case that the gate's value outlives the migration: it is the only -thing type-checking the 51 projects nothing else compiles, before and after #37198. But a +thing checking the 39 projects with no build, and every `.spec.ts`, before and after #37198. But a *diff-scoped* gate is the wrong shape for that job. Its entire design — discarding 99.1 % of diagnostics, forgiving untouched lines, forcing flags the config does not declare — exists to be useful while the baseline is **non-strict**. Once the baseline is strict, the right tool is an