From f4ee8da267d7d9bae95c507a382c45a5023dfb40 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:57:04 +0200 Subject: [PATCH 1/2] fix(ci): harden PR-size exception governance --- README.md | 8 +- config/pr-size-exceptions.json | 3 +- scripts/check-pr-size.d.mts | 3 +- scripts/check-pr-size.mjs | 192 ++++++++++++++++--------- tests/unit/tooling/checkPrSize.test.ts | 141 +++++++++++++++++- 5 files changed, 265 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 120b70599..c55242b9b 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ IndexedDB v8 PWA v3.0 i18n 19 locales — 2942 keys - 7512+ tests / 602 files + 7516+ tests / 602 files Codecov Coverage License MIT CI Status @@ -511,7 +511,7 @@ The Settings → AI panel shows a live GPU status badge with adapter details and | **Document Export** | docx + jszip | Word-compatible `.docx` generation (lazy-loaded) | | **PWA** | Service Worker + Web App Manifest v3 | Offline support, installability, Workbox chunking | | **i18n** | Custom React Context (`I18nContext.tsx`) | 2942 keys × 19 locales (de/en/es/fr/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta); EN fallback; `localStorage` persistence | -| **Testing** | Vitest 4.x (7512+ tests / 602 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | +| **Testing** | Vitest 4.x (7516+ tests / 602 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | | **Code Quality** | Biome (lint + format) + TypeScript 7 (tsgo) strict | `--error-on-warnings` in CI; zero `any` policy | | **Visualization** | Force-directed graph | Interactive character relationship network | | **Desktop** | Tauri v2 | Cross-platform installer; auto-updater via `latest.json` | @@ -549,7 +549,7 @@ WorldScript-Studio/ │ ├── sw.js # PWA Service Worker │ └── manifest.json # PWA Web App Manifest v3 ├── tests/ -│ ├── unit/ # Vitest unit tests (7512+ tests, 602 files) — count spans tests/, components/, packages/*/tests/, not just this folder +│ ├── unit/ # Vitest unit tests (7516+ tests, 602 files) — count spans tests/, components/, packages/*/tests/, not just this folder │ │ ├── ai/ # aiSmallModules, aiCoreFallbackPaths │ │ └── settings/ # WebLlmPanel, AiSections │ └── e2e/ # Playwright specs + helpers.ts @@ -714,7 +714,7 @@ The main pipeline is [`.github/workflows/ci.yml`](.github/workflows/ci.yml). Opt Raw bundle-budget ceilings (KB per uncompressed asset): entry **2500 KB**, vendor **6200 KB**, other JavaScript **2500 KB**, and WASM **30000 KB**. **Current test metrics (2026-09-06, source-synchronized; CI remains authoritative for pass/fail):** -- **7512+ unit tests** across **602 test files** — CI is authoritative for pass/fail +- **7516+ unit tests** across **602 test files** — CI is authoritative for pass/fail - Coverage thresholds: lines ≥ 80 · branches ≥ 66 · functions ≥ 72 · statements ≥ 78 — enforced in CI (see Codecov badge for live metrics) - i18n: **2942 keys × 19 locales** (en/de/fr/es/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta) diff --git a/config/pr-size-exceptions.json b/config/pr-size-exceptions.json index 550df329a..84afbe1ad 100644 --- a/config/pr-size-exceptions.json +++ b/config/pr-size-exceptions.json @@ -75,6 +75,7 @@ }, { "id": "pr-596-532-e2e-startup-determinism", + "status": "historical", "repository": "qnbs/WorldScript-Studio", "prNumber": 596, "baseRef": "main", @@ -174,7 +175,7 @@ "tests/unit/storage/idbCore.test.ts", "tests/unit/storage/idbResetGate.test.ts" ], - "reason": "PR #583 (issue #532 E2E startup/navigation nondeterminism) grew through iterative review convergence into a full async, generation/epoch-based IDB reset-quiescence contract covering every long-lived connection in the app (9 service modules plus the shared gate itself). The gate fails closed — beginIdbReset() rejects if any registered closer fails, after every closer has still had its chance to run, so wipeAllAppData() aborts before any database deletion on an unproven teardown — and a closer registered while a reset is draining joins that same awaited barrier instead of racing ahead of it as fire-and-forget. A centralized beginIdbOpenAdmission()/isIdbOpenStillValid() pair closes a further gap: an open that STARTS after a reset already bumped the generation would otherwise still match at completion, so every opener now also refuses to start a fresh open while a reset is in progress. Every reset closer also invalidates its own module's pending open flight (not just the cached database), so the first post-reset caller starts a genuinely fresh flight instead of reusing one already doomed to a generation-mismatch rejection. deleteAllIndexedDBDatabases() uses Promise.allSettled so a fast-rejecting deletion cannot release the gate while another deletion is still outstanding, and only targets database names it can prove it owns (exact KNOWN_DB_NAMES plus the worldscript-localfirst- prefix), never assuming ownership just because native enumeration returned a name. Also closes several stale-open-completion single-flight races via an identity token (proForgeHistoryStore, loraAdapterService, sceneRevisionService, logSinks, crossProjectIndexService, proForgeMemoryBank, DeadLetterQueue), a synchronous indexedDB.open() throw that could permanently memoize a rejected open promise (DeadLetterQueue), and a test-reset helper that swapped the fake IndexedDB factory without first releasing its own cached connection (loraAdapterService). persistProjectDoc() returns a distinct-identity transient NOOP when a reset is in progress (never the shared NOOP_PERSISTENCE singleton), so a local-first handle cached during an active reset is not mistaken for an intentional NOOP and reused indefinitely after the reset ends. This entry was fully recomputed (not carried forward) after #583 was actually rebased onto current main, which by then already carried PR #592's independent factory-reset persistence-admission/drain-coordinator fix (issues #591/#593) — the two PRs modify overlapping files (app/listenerMiddleware.ts, services/factoryResetService.ts, services/crossProjectIndexService.ts, tests/unit/factoryResetService.test.ts, tests/unit/listenerMiddleware.test.ts, tests/e2e/onboarding-entry-precondition.spec.ts) and were reconciled by layering both mechanisms: #592's isFactoryResetInProgress()/coordinator-draining gate (blocks new Redux-listener-level writes, drains in-flight ones) runs first inside wipeAllAppData(), then this PR's beginIdbReset() force-closes every other long-lived IDB connection the coordinators do not track. The rebase also revealed that PR #590 (merged earlier, unrelated) had already independently shipped the same locale-independent Settings/mobile-\"More\"-button navigation fix this PR originally introduced via components/SettingsView.tsx, components/settings/SettingsModals.tsx, components/settings/DataSection.tsx, components/Sidebar.tsx, and tests/e2e/helpers.ts — parallel convergent evolution left this PR's own changes to those five files fully superseded (zero net diff against current main), so they are correctly absent from allowedPaths below; every remaining path was independently re-verified as a genuine, currently non-empty diff. Final measured diff against current main: 65 governed files (84 incl. generated locale bundles), 1611 meaningful lines, 14 commits — exact measured ceilings freshly computed via check-pr-size.mjs itself, no speculative headroom. This recompute also found and fixed a latent bug in check-pr-size.mjs itself, exposed for the first time by an exception whose own ceiling (65/1611/14) legitimately exceeds TIERS.absolute (30/3000/15): when exceptionWithinLimits was true, evaluatePrSize() fell through to selectSeverity() against the fixed absolute tier instead of treating the exception's own ceiling as authoritative, so a fully-satisfied exception with a wide-enough scope would still report blocking:true — neither #539 (maxFiles:30, at the absolute tier's own boundary) nor #564 (maxFiles:3, well under it) had ever exercised this path. Fixed to short-circuit on exception.entry directly; a regression test covers a wide exception ceiling exceeding the fixed tier. Review-thread state was not re-verified as part of this recompute and must be checked live against the current PR before merge, independent of this file-scope/line-ceiling exception. allowedPaths lists all 84 changed paths, not just the 65 governed ones: check-pr-size.mjs's path-scope match (pathScopeMatch) requires every path in the raw, unfiltered diff to be present in allowedPaths for the exception to apply at all, even though the separate governed-file-count and meaningful-line-count checks (maxFiles, maxNonExemptMeaningfulLines) already exclude generated locale bundles via isGovernanceExcluded() — omitting the 19 public/locales/*/bundle.json paths here would make the exception silently fail to apply. The 17 sidebar.json files show only a trailing-newline diff: they lacked a final newline on main already, and Biome's format-on-commit hook re-adds one the moment any of them is staged for any reason in this PR — content is otherwise byte-identical to main, and this could not be avoided without skipping the pre-commit hook. No file outside this exact list is admitted; the ceiling is a one-time allowance for this PR's actual, fully-reviewed, freshly-rebased diff, not a new target. Identity note: PR #583 could not be reopened after its branch (fix/532-e2e-startup-determinism) was force-pushed during this same recompute -- GitHub permanently blocks reopening a closed PR once its head branch has been force-pushed or recreated. PR #596 was opened from the identical branch/commit as #583's successor with no further content change, so prNumber was updated from 583 to 596 to match (landed via #597). #596's own review then surfaced three further genuine, narrowly-scoped gaps in the already-touched files (app/listenerMiddleware.ts, services/ai/aiInferenceCacheService.ts, services/storage/idbResetGate.ts, plus their test files and a README metrics sync) -- a symmetric encryption-disable NOOP-discard case, an openPromise identity-clear matching the sibling services' established pattern, and a concurrent-beginIdbReset() barrier-join fix -- fixed in place by amending #596's own last commit (preserving the exact 14-commit count) rather than adding a 15th. This raised maxNonExemptMeaningfulLines from 1611 to 1741 (measured exactly via check-pr-size.mjs against the rebased branch); maxFiles (65) and maxCommits (14) are unchanged, and no path outside the existing allowedPaths list was touched, so that list and the recompute methodology below still apply verbatim. A second review wave (CodeAnt, CodeRabbit, cubic independently) then surfaced further genuine, narrowly-scoped gaps across docPersistence.ts (closer unregistered before its own teardown settled; a rejected destroy() was swallowed before the reset gate ever saw it), idbResetGate.ts (a closer registering after the drain loop already emptied was never invoked at all; a closer that itself registers another mid-drain could be double-invoked via the live Set iteration), idbCore.ts (concurrent initDB() callers before the first open resolved could each orphan a connection -- fixed with the same single-flight pattern the sibling services already use), factoryResetService.ts (deleteDatabase() rejected immediately on onblocked even though the same request can still reach a real onsuccess, settling before the true outcome was known -- now bounded by a timeout instead), sceneRevisionService.ts (missing the identity check loraAdapterService already had), plus a locale grammar fix, a dead data-testid removal, and several test-only wording/mock-leak fixes -- fixed as a new 15th commit (not amended, since this is a materially separate review round from the prior wave) plus a README metrics sync folded into the same commit. components/settings/FactoryResetDangerZone.tsx's data-testid removal reverted that file to byte-identical with main, so it drops out of the diff entirely; tests/unit/storage/idbCore.test.ts is a new regression-test file for the idbCore.ts fix. maxFiles stays 65 (one dropped, one added); maxCommits rises 14 -> 15; maxNonExemptMeaningfulLines rises 1741 -> 1900 (a same-review-cycle coderabbit follow-up on the idbCore.ts fix -- a live-handle guard missing alongside the single-flight one -- was folded into the same 15th commit rather than adding a 16th), all measured exactly via check-pr-size.mjs against the rebased branch. A subsequent codecov/patch failure (67.20% vs a 74.99% target) was root-caused by downloading and directly inspecting the actual CI-generated lcov.info artifact rather than trusting the Codecov dashboard alone: the gap was real (Codecov correctly counts partial-branch-coverage lines as not-fully-covered), not stale data, and traced to reset-in-progress-rejection/generation-mismatch/onversionchange/onerror branches left untested across most of the reset-gate's service modules by this PR's own earlier commits. Closed with test-only additions (no production code changed) as a new 16th commit: services/storage/idbCore.ts, services/crossProjectIndexService.ts, services/diagnostics/logSinks.ts, services/proForge/proForgeHistoryStore.ts, services/proForge/proForgeMemoryBank.ts, and packages/worker-bus/src/deadLetterQueue.ts. maxFiles rises 65 -> 70 (5 new test files), maxCommits rises 15 -> 16, maxNonExemptMeaningfulLines rises 1900 -> 2199 (a same-wave coderabbit follow-up replaced a fixed-microtask-count synchronization in two logSinks.test.ts reset-race tests with an observable indexedDB.open()/beginIdbOpenAdmission() spy, per the coding-guideline against tests coupled to unmocked timing internals -- net +6 lines), all measured exactly via check-pr-size.mjs against the rebased branch." + "reason": "One-time PR #596 exception for reviewed issue #532 startup-determinism convergence; see PR #596 and issue #625." } ] } diff --git a/scripts/check-pr-size.d.mts b/scripts/check-pr-size.d.mts index ec999688e..8fb5d95f8 100644 --- a/scripts/check-pr-size.d.mts +++ b/scripts/check-pr-size.d.mts @@ -40,6 +40,7 @@ export interface SupplementalLineAllowance { export interface PrSizeException { id: string; + status?: 'active' | 'historical'; repository: string; prNumber: number; baseRef: string; @@ -72,7 +73,7 @@ export interface SizeTierLimits { commits: number; } -export type SizeTier = 'ok' | 'target' | 'hard' | 'docsGovernance' | 'absolute'; +export type SizeTier = 'ok' | 'target' | 'hard' | 'docsGovernance' | 'absolute' | 'exception'; export interface SizeSeverity { tier: SizeTier; diff --git a/scripts/check-pr-size.mjs b/scripts/check-pr-size.mjs index 99a63337b..4a55276b6 100644 --- a/scripts/check-pr-size.mjs +++ b/scripts/check-pr-size.mjs @@ -14,6 +14,13 @@ const TIERS = { const EXCEPTION_REGISTRY_PATH = 'config/pr-size-exceptions.json'; const REQUIRED_EXCEPTION_SCHEMA_VERSION = 1; +const MAX_EXCEPTION_REASON_LENGTH = 500; +const GOVERNANCE_CONTROL_PATHS = new Set([ + EXCEPTION_REGISTRY_PATH, + '.github/workflows/ci.yml', + 'scripts/check-pr-size.d.mts', + 'scripts/check-pr-size.mjs', +]); function runGit(args, dependencies = {}) { const spawn = dependencies.spawnSync ?? spawnSync; @@ -162,6 +169,116 @@ function isSafeRegistryPath(value) { ); } +function isGovernanceControlPath(path) { + return GOVERNANCE_CONTROL_PATHS.has(path); +} + +function validateExceptionCeilings(entry, index) { + const status = entry.status ?? 'active'; + if (status !== 'active' && status !== 'historical') { + throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid status`); + } + const ceilings = [ + ['maxFiles', TIERS.absolute.files], + ['maxCommits', TIERS.absolute.commits], + ['maxNonExemptMeaningfulLines', TIERS.absolute.lines], + ]; + for (const [field, absoluteLimit] of ceilings) { + if (!isValidPositiveInteger(entry[field])) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid ${field}`, + ); + } + if (status === 'active' && entry[field] > absoluteLimit) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} ${field} exceeds the absolute ceiling of ${absoluteLimit}`, + ); + } + } +} + +function validateExceptionPaths(entry, index) { + if (!Array.isArray(entry.allowedPaths) || entry.allowedPaths.length === 0) { + throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} needs allowedPaths`); + } + for (const path of entry.allowedPaths) { + if (!isSafeRegistryPath(path)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid allowed path`, + ); + } + if (isGovernanceControlPath(path)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} cannot allow governance control path ${path}`, + ); + } + } +} + +function validateSupplementalAllowances(entry, index) { + if (!Array.isArray(entry.supplementalLineAllowances)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} needs supplementalLineAllowances`, + ); + } + const supplementalPaths = new Set(); + for (const allowance of entry.supplementalLineAllowances) { + if ( + !allowance || + !isSafeRegistryPath(allowance.path) || + !isValidPositiveInteger(allowance.maxMeaningfulLines) + ) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid supplemental allowance`, + ); + } + if (supplementalPaths.has(allowance.path)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has a duplicate supplemental path ${allowance.path}`, + ); + } + if (!entry.allowedPaths.includes(allowance.path)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} supplemental path ${allowance.path} is not in allowedPaths`, + ); + } + supplementalPaths.add(allowance.path); + } +} + +function validateExceptionEntry(entry, index, ids) { + if (!entry || typeof entry !== 'object') { + throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} is not an object`); + } + for (const field of ['id', 'repository', 'baseRef', 'headRef', 'reason']) { + if (typeof entry[field] !== 'string' || entry[field].length === 0) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid ${field}`, + ); + } + } + if (ids.has(entry.id)) { + throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: duplicate exception id ${entry.id}`); + } + ids.add(entry.id); + if (!isValidPositiveInteger(entry.prNumber) || entry.prNumber < 1) { + throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid prNumber`); + } + validateExceptionCeilings(entry, index); + if ( + entry.reason.length > MAX_EXCEPTION_REASON_LENGTH || + entry.reason.includes('\n') || + entry.reason.includes('\r') + ) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} needs a short single-line reason (max ${MAX_EXCEPTION_REASON_LENGTH} characters)`, + ); + } + validateExceptionPaths(entry, index); + validateSupplementalAllowances(entry, index); + return entry; +} + function validateExceptionRegistry(value) { if ( !value || @@ -177,74 +294,7 @@ function validateExceptionRegistry(value) { } const ids = new Set(); - return value.exceptions.map((entry, index) => { - if (!entry || typeof entry !== 'object') { - throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} is not an object`); - } - const requiredStrings = ['id', 'repository', 'baseRef', 'headRef', 'reason']; - for (const field of requiredStrings) { - if (typeof entry[field] !== 'string' || entry[field].length === 0) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid ${field}`, - ); - } - } - if (ids.has(entry.id)) { - throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: duplicate exception id ${entry.id}`); - } - ids.add(entry.id); - if (!isValidPositiveInteger(entry.prNumber) || entry.prNumber < 1) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid prNumber`, - ); - } - for (const field of ['maxFiles', 'maxCommits', 'maxNonExemptMeaningfulLines']) { - if (!isValidPositiveInteger(entry[field])) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid ${field}`, - ); - } - } - if (!Array.isArray(entry.allowedPaths) || entry.allowedPaths.length === 0) { - throw new Error(`invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} needs allowedPaths`); - } - for (const path of entry.allowedPaths) { - if (!isSafeRegistryPath(path)) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid allowed path`, - ); - } - } - if (!Array.isArray(entry.supplementalLineAllowances)) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} needs supplementalLineAllowances`, - ); - } - const supplementalPaths = new Set(); - for (const allowance of entry.supplementalLineAllowances) { - if ( - !allowance || - !isSafeRegistryPath(allowance.path) || - !isValidPositiveInteger(allowance.maxMeaningfulLines) - ) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has invalid supplemental allowance`, - ); - } - if (supplementalPaths.has(allowance.path)) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} has a duplicate supplemental path ${allowance.path}`, - ); - } - if (!entry.allowedPaths.includes(allowance.path)) { - throw new Error( - `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} supplemental path ${allowance.path} is not in allowedPaths`, - ); - } - supplementalPaths.add(allowance.path); - } - return entry; - }); + return value.exceptions.map((entry, index) => validateExceptionEntry(entry, index, ids)); } function getPullRequestIdentity(dependencies = {}) { @@ -317,6 +367,7 @@ function resolveException(base, head, dependencies = {}) { return { applied: false, identityMatch: false, pathScopeMatch: false, baseGoverned: true }; const matches = registry.filter( (entry) => + (entry.status ?? 'active') === 'active' && entry.repository === identity.repository && entry.prNumber === identity.prNumber && entry.baseRef === identity.baseRef && @@ -329,7 +380,10 @@ function resolveException(base, head, dependencies = {}) { const changedPaths = getChangedPaths(base, head, dependencies); if (changedPaths === null) throw new Error('could not resolve changed paths for PR-size exception scope'); - const pathScopeMatch = changedPaths.every((path) => entry.allowedPaths.includes(path)); + // QNBS-v3: an exception cannot authorize the PR to rewrite the authority that governs it. + const pathScopeMatch = + !changedPaths.some(isGovernanceControlPath) && + changedPaths.every((path) => entry.allowedPaths.includes(path)); return { applied: pathScopeMatch, identityMatch: true, diff --git a/tests/unit/tooling/checkPrSize.test.ts b/tests/unit/tooling/checkPrSize.test.ts index 3db7bad19..e2b3318dc 100644 --- a/tests/unit/tooling/checkPrSize.test.ts +++ b/tests/unit/tooling/checkPrSize.test.ts @@ -543,6 +543,67 @@ describe('evaluatePrSize', () => { expect(result.severity?.blocking).toBe(true); }); + it.each([ + 'config/pr-size-exceptions.json', + 'scripts/check-pr-size.mjs', + 'scripts/check-pr-size.d.mts', + '.github/workflows/ci.yml', + ])('does not let an exception authorize governance-control path %s', (path) => { + const rows: NumstatRow[] = [{ path, added: 3001, removed: 0 }]; + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows, + changedPaths: [path], + registry: { schemaVersion: 1, exceptions: [exception] }, + }), + ); + expect(result.exception).toMatchObject({ + applied: false, + identityMatch: true, + pathScopeMatch: false, + }); + expect(result.severity?.blocking).toBe(true); + }); + + it('rejects a base registry that allowlists a governance-control path', () => { + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows: [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }], + registry: { + schemaVersion: 1, + exceptions: [ + { + ...exception, + allowedPaths: [...exception.allowedPaths, 'config/pr-size-exceptions.json'], + }, + ], + }, + }), + ); + expect(result.ok).toBe(false); + expect(result.error).toContain('cannot allow governance control path'); + }); + + it('rejects an exception rationale that is not short and single-line', () => { + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows: [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }], + registry: { + schemaVersion: 1, + exceptions: [{ ...exception, reason: 'x'.repeat(501) }], + }, + }), + ); + expect(result.ok).toBe(false); + expect(result.error).toContain('short single-line reason'); + }); + it('rejects rename-style scope smuggling through the no-renames path list', () => { const rows: NumstatRow[] = [{ path: 'graphify-out/GRAPH_REPORT.md', added: 10, removed: 10 }]; const result = evaluatePrSize( @@ -679,18 +740,84 @@ describe('evaluatePrSize', () => { expect(tooManyCommits.severity?.blocking).toBe(true); }); - // QNBS-v3: an exception's whole purpose is authorizing a PR past TIERS.absolute (30/3000/15) -- a ceiling wide enough to matter must not then get re-checked against that same fixed tier and blocked anyway. - it('does not block a PR within a wide exception ceiling that exceeds the fixed absolute tier', () => { - const rows: NumstatRow[] = Array.from({ length: 40 }, (_, i) => ({ + it.each([ + ['maxFiles', 31], + ['maxCommits', 16], + ['maxNonExemptMeaningfulLines', 3001], + ])('fails closed when an exception %s exceeds the absolute ceiling', (field, value) => { + const invalidException = { ...exception, [field]: value }; + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows: [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }], + registry: { schemaVersion: 1, exceptions: [invalidException] }, + }), + ); + expect(result.ok).toBe(false); + expect(result.error).toContain(`${field} exceeds the absolute ceiling`); + }); + + it('does not apply a historical exception with preserved legacy ceilings', () => { + const rows: NumstatRow[] = [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }]; + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows, + registry: { + schemaVersion: 1, + exceptions: [ + { + ...exception, + status: 'historical', + maxFiles: 70, + maxCommits: 16, + }, + ], + }, + }), + ); + expect(result.exception?.applied).toBe(false); + expect(result.severity?.tier).toBe('ok'); + }); + + it('rejects an ordinary diff over the absolute file and commit ceilings without an exception', () => { + const rows: NumstatRow[] = Array.from({ length: 31 }, (_, i) => ({ + path: `scripts/tool-${i}.mjs`, + added: 1, + removed: 0, + })); + const result = evaluatePrSize('base', 'head', { + ...exceptionDependencies({ rows, registry: { schemaVersion: 1, exceptions: [] } }), + env: {}, + }); + expect(result.severity?.blocking).toBe(true); + expect(result.severity?.tier).toBe('absolute'); + + const commitsOver = evaluatePrSize('base', 'head', { + ...exceptionDependencies({ + rows: [{ path: 'scripts/tool.mjs', added: 1, removed: 0 }], + registry: { schemaVersion: 1, exceptions: [] }, + commitCount: 16, + }), + env: {}, + }); + expect(commitsOver.severity?.blocking).toBe(true); + expect(commitsOver.severity?.tier).toBe('absolute'); + }); + + it('passes a narrow base-authorized exception at the absolute boundary', () => { + const rows: NumstatRow[] = Array.from({ length: 30 }, (_, i) => ({ path: `scripts/tool-${i}.mjs`, added: 100, removed: 0, })); const wide = { ...exception, - maxFiles: 65, - maxCommits: 20, - maxNonExemptMeaningfulLines: 4000, + maxFiles: 30, + maxCommits: 15, + maxNonExemptMeaningfulLines: 3000, supplementalLineAllowances: [], allowedPaths: rows.map((row) => row.path), }; @@ -700,7 +827,7 @@ describe('evaluatePrSize', () => { exceptionDependencies({ rows, changedPaths: rows.map((row) => row.path), - commitCount: 18, + commitCount: 15, registry: { schemaVersion: 1, exceptions: [wide] }, }), ); From d708ce4fb9639c17a8b0c1f4bfe706dea7a41619 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:27:05 +0200 Subject: [PATCH 2/2] fix(ci): close PR-size governance control gaps --- config/pr-size-exceptions.json | 2 +- scripts/check-pr-size.mjs | 19 +++++++++- tests/unit/tooling/checkPrSize.test.ts | 49 ++++++++++++++++++++++---- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/config/pr-size-exceptions.json b/config/pr-size-exceptions.json index 84afbe1ad..c1c2ddce9 100644 --- a/config/pr-size-exceptions.json +++ b/config/pr-size-exceptions.json @@ -175,7 +175,7 @@ "tests/unit/storage/idbCore.test.ts", "tests/unit/storage/idbResetGate.test.ts" ], - "reason": "One-time PR #596 exception for reviewed issue #532 startup-determinism convergence; see PR #596 and issue #625." + "reason": "One-time PR #596 exception for reviewed issue #532 startup-determinism convergence; see PR #596 and issue #532." } ] } diff --git a/scripts/check-pr-size.mjs b/scripts/check-pr-size.mjs index 4a55276b6..592db1f70 100644 --- a/scripts/check-pr-size.mjs +++ b/scripts/check-pr-size.mjs @@ -18,9 +18,17 @@ const MAX_EXCEPTION_REASON_LENGTH = 500; const GOVERNANCE_CONTROL_PATHS = new Set([ EXCEPTION_REGISTRY_PATH, '.github/workflows/ci.yml', + 'scripts/ci-prepush-classifier.mjs', 'scripts/check-pr-size.d.mts', 'scripts/check-pr-size.mjs', ]); +// QNBS-v3: supplemental ceilings are only for explicitly named non-executable artifacts. +const SUPPLEMENTAL_ARTIFACT_PATTERNS = [ + /^docs\/.+\.(?:md|mdx)$/i, + /^(?:graphify-out|\.codegraph)\/.+\.(?:json|md|mdx|html)$/i, + /(?:^|\/)(?:generated|reports?)\/[^/]+\.(?:json|md|mdx|html)$/i, + /(?:^|\/)[^/]+\.(?:generated|report)\.(?:json|md|mdx|html)$/i, +]; function runGit(args, dependencies = {}) { const spawn = dependencies.spawnSync ?? spawnSync; @@ -173,6 +181,10 @@ function isGovernanceControlPath(path) { return GOVERNANCE_CONTROL_PATHS.has(path); } +function isSupplementalArtifactPath(path) { + return SUPPLEMENTAL_ARTIFACT_PATTERNS.some((pattern) => pattern.test(path)); +} + function validateExceptionCeilings(entry, index) { const status = entry.status ?? 'active'; if (status !== 'active' && status !== 'historical') { @@ -242,6 +254,11 @@ function validateSupplementalAllowances(entry, index) { `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} supplemental path ${allowance.path} is not in allowedPaths`, ); } + if (!isSupplementalArtifactPath(allowance.path)) { + throw new Error( + `invalid ${EXCEPTION_REGISTRY_PATH}: exception ${index} supplemental path ${allowance.path} must be a recognized non-executable artifact`, + ); + } supplementalPaths.add(allowance.path); } } @@ -514,7 +531,7 @@ export function evaluatePrSize(base, head, dependencies = {}) { ); return count <= allowance.maxMeaningfulLines; })); - // QNBS-v3: an exception's own ceiling can legitimately exceed TIERS.absolute (that is the whole point of granting one) -- falling through to selectSeverity() here would re-check the raw counts against the fixed 30/3000/15 tier and block anyway, even though the PR-specific ceiling was satisfied. + // QNBS-v3: active exception ceilings remain bounded by the ordinary absolute tier. const severity = exception.entry ? { tier: exceptionWithinLimits ? 'exception' : 'absolute', diff --git a/tests/unit/tooling/checkPrSize.test.ts b/tests/unit/tooling/checkPrSize.test.ts index e2b3318dc..2c4034e67 100644 --- a/tests/unit/tooling/checkPrSize.test.ts +++ b/tests/unit/tooling/checkPrSize.test.ts @@ -543,8 +543,10 @@ describe('evaluatePrSize', () => { expect(result.severity?.blocking).toBe(true); }); + // QNBS-v3: keeps every governance-control path outside exception authority. it.each([ 'config/pr-size-exceptions.json', + 'scripts/ci-prepush-classifier.mjs', 'scripts/check-pr-size.mjs', 'scripts/check-pr-size.d.mts', '.github/workflows/ci.yml', @@ -567,7 +569,14 @@ describe('evaluatePrSize', () => { expect(result.severity?.blocking).toBe(true); }); - it('rejects a base registry that allowlists a governance-control path', () => { + // QNBS-v3: rejects registry entries that attempt to authorize governance controls. + it.each([ + 'config/pr-size-exceptions.json', + 'scripts/ci-prepush-classifier.mjs', + 'scripts/check-pr-size.d.mts', + 'scripts/check-pr-size.mjs', + '.github/workflows/ci.yml', + ])('rejects a base registry that allowlists governance-control path %s', (path) => { const result = evaluatePrSize( 'base', 'head', @@ -578,7 +587,7 @@ describe('evaluatePrSize', () => { exceptions: [ { ...exception, - allowedPaths: [...exception.allowedPaths, 'config/pr-size-exceptions.json'], + allowedPaths: [...exception.allowedPaths, path], }, ], }, @@ -588,6 +597,7 @@ describe('evaluatePrSize', () => { expect(result.error).toContain('cannot allow governance control path'); }); + // QNBS-v3: keeps exception rationale references short and auditable. it('rejects an exception rationale that is not short and single-line', () => { const result = evaluatePrSize( 'base', @@ -662,12 +672,14 @@ describe('evaluatePrSize', () => { }); it('reports duplicate supplemental paths separately', () => { - const rows: NumstatRow[] = [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }]; + const duplicatePath = 'docs/duplicate.md'; + const rows: NumstatRow[] = [{ path: duplicatePath, added: 10, removed: 0 }]; const duplicateAllowance = { ...exception, + allowedPaths: [...exception.allowedPaths, duplicatePath], supplementalLineAllowances: [ - { path: 'scripts/tool.mjs', maxMeaningfulLines: 10 }, - { path: 'scripts/tool.mjs', maxMeaningfulLines: 20 }, + { path: duplicatePath, maxMeaningfulLines: 10 }, + { path: duplicatePath, maxMeaningfulLines: 20 }, ], }; const result = evaluatePrSize( @@ -679,7 +691,7 @@ describe('evaluatePrSize', () => { }), ); expect(result.ok).toBe(false); - expect(result.error).toContain('duplicate supplemental path scripts/tool.mjs'); + expect(result.error).toContain(`duplicate supplemental path ${duplicatePath}`); expect(result.error).not.toContain('is not in allowedPaths'); }); @@ -704,6 +716,27 @@ describe('evaluatePrSize', () => { expect(result.error).not.toContain('duplicate supplemental path'); }); + // QNBS-v3: prevents executable files from escaping the ordinary line ceiling. + it('rejects a supplemental allowance for an executable path', () => { + const executablePath = 'services/security.ts'; + const executableAllowance = { + ...exception, + allowedPaths: [...exception.allowedPaths, executablePath], + supplementalLineAllowances: [{ path: executablePath, maxMeaningfulLines: 3000 }], + }; + const result = evaluatePrSize( + 'base', + 'head', + exceptionDependencies({ + rows: [{ path: executablePath, added: 10, removed: 0 }], + changedPaths: [executablePath], + registry: { schemaVersion: 1, exceptions: [executableAllowance] }, + }), + ); + expect(result.ok).toBe(false); + expect(result.error).toContain('must be a recognized non-executable artifact'); + }); + it('fails closed for duplicate matching identities', () => { const rows: NumstatRow[] = [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }]; const duplicate = { ...exception, id: 'second-test-exception' }; @@ -740,6 +773,7 @@ describe('evaluatePrSize', () => { expect(tooManyCommits.severity?.blocking).toBe(true); }); + // QNBS-v3: rejects active exception ceilings above the fixed absolute tier. it.each([ ['maxFiles', 31], ['maxCommits', 16], @@ -758,6 +792,7 @@ describe('evaluatePrSize', () => { expect(result.error).toContain(`${field} exceeds the absolute ceiling`); }); + // QNBS-v3: keeps historical authorization inert without rewriting its recorded limits. it('does not apply a historical exception with preserved legacy ceilings', () => { const rows: NumstatRow[] = [{ path: 'scripts/tool.mjs', added: 10, removed: 0 }]; const result = evaluatePrSize( @@ -782,6 +817,7 @@ describe('evaluatePrSize', () => { expect(result.severity?.tier).toBe('ok'); }); + // QNBS-v3: verifies ordinary diffs still fail closed beyond the absolute tier. it('rejects an ordinary diff over the absolute file and commit ceilings without an exception', () => { const rows: NumstatRow[] = Array.from({ length: 31 }, (_, i) => ({ path: `scripts/tool-${i}.mjs`, @@ -807,6 +843,7 @@ describe('evaluatePrSize', () => { expect(commitsOver.severity?.tier).toBe('absolute'); }); + // QNBS-v3: proves base authority can pass only within the exact absolute boundary. it('passes a narrow base-authorized exception at the absolute boundary', () => { const rows: NumstatRow[] = Array.from({ length: 30 }, (_, i) => ({ path: `scripts/tool-${i}.mjs`,