From 4d9576ab5384e0e61d4afb074a00f0f476ac72a0 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Mon, 24 Aug 2026 16:58:17 +0200 Subject: [PATCH 01/21] feat(cockpit): add D3 read-only dashboard host --- .../architecture/D3-cockpit-dashboard-host.md | 129 ++++++++ package.json | 1 + src/cockpit-host/escape.ts | 30 ++ src/cockpit-host/fixtures/stage-a.ts | 179 ++++++++++ src/cockpit-host/render.ts | 313 ++++++++++++++++++ src/cockpit-host/server.ts | 126 +++++++ src/cockpit-host/styles.ts | 107 ++++++ tests/cockpit-host/purity.test.ts | 74 +++++ tests/cockpit-host/render.test.ts | 88 +++++ tests/cockpit-host/server.test.ts | 102 ++++++ 10 files changed, 1149 insertions(+) create mode 100644 docs/architecture/D3-cockpit-dashboard-host.md create mode 100644 src/cockpit-host/escape.ts create mode 100644 src/cockpit-host/fixtures/stage-a.ts create mode 100644 src/cockpit-host/render.ts create mode 100644 src/cockpit-host/server.ts create mode 100644 src/cockpit-host/styles.ts create mode 100644 tests/cockpit-host/purity.test.ts create mode 100644 tests/cockpit-host/render.test.ts create mode 100644 tests/cockpit-host/server.test.ts diff --git a/docs/architecture/D3-cockpit-dashboard-host.md b/docs/architecture/D3-cockpit-dashboard-host.md new file mode 100644 index 0000000..f78acc7 --- /dev/null +++ b/docs/architecture/D3-cockpit-dashboard-host.md @@ -0,0 +1,129 @@ +# Cockpit Read-Only Dashboard Host (Cockpit D3) + +Status: V1 defaults, Stage A. Superseded only by an explicit architecture decision. + +## Scope + +D3 is the first *visible* Cockpit surface: a local HTTP host that renders one +deterministic fixture snapshot, validated through the existing D1/D2 contracts, +as a read-only browser page. + + Stage-A fixture (unknown) + -> readCockpitSnapshot() (D1: hostile-input validation) + -> projectCockpitEvidenceFreshness() (D2: freshness projection) + -> server-side HTML (every value escaped) + -> 127.0.0.1 GET-only browser view + +D3 is **presentation and observability only**. It is not authority, not policy, +not a collector, not persistence, and not an agent-control interface. + +## Why the host lives outside `src/cockpit/` + +D1 and D2 are pure: `tests/cockpit/architecture-invariants.test.ts` fails the +build if any file in `src/cockpit/` references `node:fs`, `node:http`, +`child_process`, `fetch(`, `process.`, Git, or an import outside the domain +kernel. A dashboard host needs `node:http`. Placing it in `src/cockpit/` would +break that invariant, so D3 is a **separate sibling module**, `src/cockpit-host/`, +that *imports* the pure Cockpit boundary and never modifies it. The host keeps +its own narrower purity invariant (`tests/cockpit-host/purity.test.ts`): no +subprocess, no environment access, no Git, and imports only Node builtins, +itself, or `../cockpit/`. + +## Ingestion boundary + +- **D1 is the only hostile-input normalizer.** The Stage-A fixture is typed + `unknown` and passes through `readCockpitSnapshot()` before any value is + rendered. If validation fails, the host **fails closed** — it throws and + refuses to serve rather than falling back to raw fixture data. +- **D2 is the only freshness projection.** Freshness state/reason/counts come + verbatim from `projectCockpitEvidenceFreshness()`; the host never re-derives + `CURRENT`/`STALE`/`INVALID`. +- The renderer accepts a validated `CockpitSnapshot`, never `unknown`, so a raw + caller object can never reach the page unvalidated. + +## Authority: none + +D3 has zero authority and no mutation surface of any kind: + +- **No repository write** — no filesystem write to the repo, no Git. +- **No GitHub write** — no adapter imported, no network egress. +- **No agent invocation, ExecutionPermit, or merge capability** — C1 authority + and the provider adapters are not imported and are unreachable. + +The absence is structural: there is no field, route, or imported function +through which a mutation could flow. The V1 read-only boundary is preserved and +**human merge authority remains external**. A reviewer recommendation is +rendered as a *claim*, never as permission. + +## Honest gap panels + +The dashboard shows only what the current read models project: + +- **Tree SHA — not projected.** D1 carries the observed HEAD only; there is no + tree SHA field. The host renders a capability notice, never an invented value. +- **Autoflow — not projected yet.** `WorkflowState` (status, revision, sequence, + invocations, human gate) has no Cockpit projection. Rendering a fixture + `WorkflowState` would manufacture orchestration state, so the Autoflow panel + shows an honest "not projected yet" notice. A real Autoflow view requires a + future **pure Cockpit D4 projection**; D3 does not import `WorkflowState`, + `workflow-transitions`, or `applyWorkflowEvent`. + +## HTTP security boundary + +- **Loopback only** — binds the literal `127.0.0.1`, never `0.0.0.0`, `::`, or a + resolvable hostname. +- **GET-only** — any other method returns `405 Method Not Allowed` with + `Allow: GET`; unknown routes return `404`. No mutation route, no request body, + no cookie, no session. +- **Strict headers on every response** — + `Content-Security-Policy: default-src 'none'; style-src 'self'; script-src 'none'; …` + and `X-Content-Type-Options: nosniff`. The page ships no client-side + JavaScript and no inline `style` attribute, so the policy needs no + `'unsafe-inline'` and no script source. +- **Untrusted text is always escaped** — every dynamic value (reviewer/finding + prose included) is HTML-entity-escaped before it enters the markup. No + `innerHTML`, no `document.write`, no inline event handler. Adversarial tests + assert that `', + }, + ], + evidence: [ + { + evidenceId: 'ev-ci-001', + kind: 'ci-result', + source: 'github', + commitSha: HEAD_SHA, + reference: 'gh-actions/run/1024', + observedAt: '2026-08-24T11:40:00.000Z', + }, + { + evidenceId: 'ev-review-002', + kind: 'code-review', + source: 'agent', + commitSha: HEAD_SHA, + reference: 'claude/review/pr-42', + observedAt: '2026-08-24T11:45:00.000Z', + }, + { + evidenceId: 'ev-sec-003', + kind: 'security-review', + source: 'local-verification', + commitSha: OLD_SHA, + reference: 'local/security-scan/pr-42', + observedAt: '2026-08-23T09:10:00.000Z', + }, + { + evidenceId: 'ev-test-004', + kind: 'test-result', + source: 'local-verification', + commitSha: HEAD_SHA, + reference: 'vitest/run/883', + observedAt: '2026-08-24T11:50:00.000Z', + }, + { + evidenceId: 'ev-human-005', + kind: 'human-decision', + source: 'human', + commitSha: HEAD_SHA, + reference: 'maintainer/gate/pr-42', + observedAt: '2026-08-24T11:55:00.000Z', + }, + { + evidenceId: 'ev-repo-006', + kind: 'repository-state', + source: 'github', + commitSha: OLD_SHA, + reference: 'gh/tree/dead0000', + observedAt: '2026-08-23T08:00:00.000Z', + }, + ], + findings: [ + { + findingId: 'f-001', + pullRequestId: 'pr-42', + reviewedCommitSha: OLD_SHA, + provider: 'claude', + reviewerId: 'claude-review-bot', + severity: 'blocking', + classification: 'security', + status: 'open', + // Hostile title + message: must render as inert escaped text. + title: " SQL injection in login handler", + message: + '"> Use parameterized queries & escape < > characters before rendering.', + filePath: 'src/auth/login.ts', + disposition: 'future-layer-obligation', + advisoryFreshness: 'STALE', + }, + { + findingId: 'f-002', + pullRequestId: 'pr-42', + reviewedCommitSha: HEAD_SHA, + provider: 'codex', + reviewerId: 'codex-review-bot', + severity: 'major', + classification: 'correctness', + status: 'open', + title: 'Off-by-one in session expiry comparison', + message: 'The expiry check uses <= where < is intended; sessions live one tick too long.', + filePath: 'src/auth/session.ts', + disposition: 'maintenance-observation', + advisoryFreshness: 'CURRENT', + }, + { + findingId: 'f-003', + pullRequestId: 'pr-43', + reviewedCommitSha: HEAD_SHA, + provider: 'claude', + reviewerId: 'claude-review-bot', + severity: 'minor', + classification: 'maintainability', + status: 'resolved', + title: 'Extract duplicated header-building logic', + message: 'Two handlers build the same response headers; factor into one helper.', + filePath: 'src/http/headers.ts', + disposition: 'optional-cleanup', + advisoryFreshness: null, + }, + { + findingId: 'f-004', + pullRequestId: 'pr-43', + reviewedCommitSha: HEAD_SHA, + provider: 'coderabbit', + reviewerId: 'coderabbit-bot', + severity: 'info', + classification: 'performance', + status: 'unknown', + title: 'Consider memoizing the freshness projection', + message: 'For large snapshots the projection could be cached per observed HEAD.', + filePath: null, + disposition: 'deferred', + advisoryFreshness: null, + }, + ], + repairJobs: [ + { + jobId: 'repair-001', + parentPullRequestId: 'pr-42', + findingId: 'f-001', + repairBranch: 'refs/heads/repair/f-001-login-injection', + repairAgentId: 'claude-repair-agent', + independentValidatorId: 'codex-independent-validator', + }, + ], +}; diff --git a/src/cockpit-host/render.ts b/src/cockpit-host/render.ts new file mode 100644 index 0000000..da02232 --- /dev/null +++ b/src/cockpit-host/render.ts @@ -0,0 +1,313 @@ +/** + * Cockpit D3 — pure server-side HTML rendering. + * + * Turns a **D1-validated** {@link CockpitSnapshot} and its **D2** evidence + * freshness projection into a single self-contained HTML string. This module is + * pure and total: no I/O, no network, no process access, no clock, no mutation. + * It imports only the Cockpit read-model boundary and the local escaper. + * + * Every dynamic value is passed through {@link escapeHtml} before it enters the + * markup. The output contains no `"); + expect(html).not.toContain(''); + expect(html).not.toContain(''); + // They must appear in escaped form instead. + expect(html).toContain('<script>alert('xss-title')</script>'); + expect(html).toContain('<img src=x onerror=alert(1)>'); + }); + + it('does not dump the raw fixture object as JSON', () => { + // A raw JSON.stringify of the fixture would contain quoted property keys. + expect(html).not.toContain('"findingId":'); + expect(html).not.toContain('"repositoryId":'); + }); + + it('presents the authority legend without offering any action control', () => { + expect(html).toContain('Authority legend'); + expect(html).toContain('Human-only action'); + // No interactive authority surface exists anywhere in the markup. + expect(html).not.toContain(' { + expect(html).not.toContain(' { + expect(html).toContain('state-CURRENT'); + expect(html).toContain('state-STALE'); + }); +}); diff --git a/tests/cockpit-host/server.test.ts b/tests/cockpit-host/server.test.ts new file mode 100644 index 0000000..130d107 --- /dev/null +++ b/tests/cockpit-host/server.test.ts @@ -0,0 +1,102 @@ +import type { AddressInfo } from 'node:net'; + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { createCockpitServer, HOST, PORT } from '../../src/cockpit-host/server.js'; + +let base = ''; +let boundAddress = ''; +const server = createCockpitServer(); + +beforeAll(async () => { + await new Promise((resolve) => { + // Listen on an ephemeral port on loopback to avoid clashing with the fixed port. + server.listen(0, '127.0.0.1', resolve); + }); + const address = server.address(); + if (address === null || typeof address === 'string') { + throw new Error('expected an AddressInfo from server.address()'); + } + const info: AddressInfo = address; + boundAddress = info.address; + base = `http://127.0.0.1:${String(info.port)}`; +}); + +afterAll(async () => { + await new Promise((resolve, reject) => { + server.close((error) => { + if (error === undefined) { + resolve(); + } else { + reject(error); + } + }); + }); +}); + +describe('Cockpit D3 host constants', () => { + it('binds the literal loopback address', () => { + expect(HOST).toBe('127.0.0.1'); + expect(boundAddress).toBe('127.0.0.1'); + }); + + it('declares a fixed non-privileged port', () => { + expect(PORT).toBeGreaterThanOrEqual(1024); + }); +}); + +describe('Cockpit D3 routes', () => { + it('GET / returns 200 HTML identifying read-only Stage-A data', async () => { + const response = await fetch(`${base}/`); + expect(response.status).toBe(200); + expect(response.headers.get('content-type')).toBe('text/html; charset=utf-8'); + const body = await response.text(); + expect(body).toContain('READ ONLY'); + expect(body).toContain('STAGE A'); + expect(body).toContain('Not projected yet'); + expect(body).toContain('Tree SHA'); + }); + + it('GET /styles.css returns 200 CSS', async () => { + const response = await fetch(`${base}/styles.css`); + expect(response.status).toBe(200); + expect(response.headers.get('content-type')).toBe('text/css; charset=utf-8'); + const body = await response.text(); + expect(body).toContain('body'); + }); + + it('POST / returns 405 with an Allow: GET header (no mutation route)', async () => { + const response = await fetch(`${base}/`, { method: 'POST' }); + expect(response.status).toBe(405); + expect(response.headers.get('allow')).toBe('GET'); + }); + + it('rejects every non-GET method with 405', async () => { + for (const method of ['PUT', 'DELETE', 'PATCH']) { + const response = await fetch(`${base}/`, { method }); + expect(response.status).toBe(405); + } + }); + + it('unknown route returns 404', async () => { + const response = await fetch(`${base}/does-not-exist`); + expect(response.status).toBe(404); + }); +}); + +describe('Cockpit D3 security headers', () => { + it('sends a strict CSP that blocks scripts and permits only the local stylesheet', async () => { + const response = await fetch(`${base}/`); + const csp = response.headers.get('content-security-policy') ?? ''; + expect(csp).toContain("default-src 'none'"); + expect(csp).toContain("script-src 'none'"); + expect(csp).toContain("style-src 'self'"); + }); + + it('sends X-Content-Type-Options: nosniff on every response', async () => { + const rootResponse = await fetch(`${base}/`); + expect(rootResponse.headers.get('x-content-type-options')).toBe('nosniff'); + const missingResponse = await fetch(`${base}/nope`); + expect(missingResponse.headers.get('x-content-type-options')).toBe('nosniff'); + }); +}); From 46530c70f67c1eae3ea02875acc7e0051599bae9 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Mon, 24 Aug 2026 18:21:20 +0200 Subject: [PATCH 02/21] fix(cockpit): show finding review bindings --- src/cockpit-host/render.ts | 4 +- tests/cockpit-host/render.test.ts | 83 +++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/cockpit-host/render.ts b/src/cockpit-host/render.ts index da02232..78fd228 100644 --- a/src/cockpit-host/render.ts +++ b/src/cockpit-host/render.ts @@ -186,6 +186,8 @@ function findingsSection(findings: readonly CockpitFindingReadModel[]): string { return ` ${text(finding.findingId)} + ${text(finding.pullRequestId)} + ${text(finding.reviewedCommitSha)} ${text(finding.severity)} ${text(finding.classification)} ${text(finding.status)} @@ -202,7 +204,7 @@ function findingsSection(findings: readonly CockpitFindingReadModel[]): string {

Findings claim

A finding is a reviewer/agent claim. Advisory freshness is a recomputable echo (derived judgment), never authority.

- + ${rows}
IDSeverityClassificationStatusDispositionTitleMessageFileAdvisory freshness
IDPRReviewed commitSeverityClassificationStatusDispositionTitleMessageFileAdvisory freshness
`; diff --git a/tests/cockpit-host/render.test.ts b/tests/cockpit-host/render.test.ts index b6bb8bf..b28db1e 100644 --- a/tests/cockpit-host/render.test.ts +++ b/tests/cockpit-host/render.test.ts @@ -86,3 +86,86 @@ describe('Cockpit D3 rendered page', () => { expect(html).toContain('state-STALE'); }); }); + +describe('Cockpit D3 finding binding (D3-CODEX-F1)', () => { + const html = buildDashboardHtml(); + + // Fixture SHAs. Each finding is reviewed against exactly one of these. + const HEAD_SHA = 'c0ffee00c0ffee00c0ffee00c0ffee00c0ffee00'; + const OLD_SHA = 'dead0000dead0000dead0000dead0000dead0000'; + + // Isolate the Findings
so assertions are scoped to it, not to the + // whole page: the PR ids and reviewed-commit SHAs also legitimately appear in + // the Pull requests, Evidence, and Repair-jobs sections, so a page-wide + // `toContain` would pass even if the Findings table still omitted the binding. + const headingAt = html.indexOf('

Findings'); + const sectionStart = html.lastIndexOf('
', headingAt); + const sectionEnd = html.indexOf('
', headingAt); + const findingsSection = html.slice(sectionStart, sectionEnd); + + // The header row, and each finding's , scoped within the Findings section. + const headerRow = findingsSection.slice( + findingsSection.indexOf(''), + findingsSection.indexOf(''), + ); + const rowFor = (findingId: string): string => { + const rows = findingsSection.split(''); + const row = rows.find((candidate) => candidate.includes(`>${findingId}<`)); + expect(row, `row for ${findingId}`).toBeDefined(); + return row as string; + }; + + it('labels PR, Reviewed commit, and Advisory freshness as distinct columns', () => { + expect(headerRow).toContain('PR'); + expect(headerRow).toContain('Reviewed commit'); + expect(headerRow).toContain('Advisory freshness'); + // "Reviewed commit" is the load-bearing label: it must not collapse to a + // bare "Commit", which would blur it against the repository Observed HEAD + // SHA and the PR Head SHA shown elsewhere on the page. + expect(headerRow).not.toContain('Commit'); + }); + + it('binds f-001 to pr-42 reviewed against the older (STALE) commit', () => { + const row = rowFor('f-001'); + expect(row).toContain('pr-42'); + expect(row).toContain(OLD_SHA); + }); + + it('binds f-002 to pr-42 reviewed against the observed HEAD commit', () => { + const row = rowFor('f-002'); + expect(row).toContain('pr-42'); + expect(row).toContain(HEAD_SHA); + }); + + it('binds an f-003/pr-43 finding to its PR and reviewed commit', () => { + const row = rowFor('f-003'); + expect(row).toContain('pr-43'); + expect(row).toContain(HEAD_SHA); + }); + + it('keeps two same-PR findings distinguishable by reviewed commit', () => { + // f-001 and f-002 are both pr-42 but were reviewed against different + // commits; the reviewed-commit binding, not advisory freshness, is what + // tells them apart. + expect(rowFor('f-001')).toContain(OLD_SHA); + expect(rowFor('f-002')).toContain(HEAD_SHA); + expect(rowFor('f-001')).not.toContain(HEAD_SHA); + }); + + it('keeps advisory freshness separate from reviewed-commit identity', () => { + // f-003 carries no advisory claim yet still shows its reviewed commit: + // freshness is not a substitute for commit identity. + const row = rowFor('f-003'); + expect(row).toContain(HEAD_SHA); + expect(row).toContain('no claim'); + }); + + it('renders both binding values as inert escaped text', () => { + // pullRequestId / reviewedCommitSha go through the same escaper as every + // other value; no raw markup or interactive control is introduced. + expect(findingsSection).not.toContain(' Date: Mon, 24 Aug 2026 20:09:19 +0200 Subject: [PATCH 03/21] test(cockpit): harden D3 import scanner --- tests/cockpit-host/purity.test.ts | 95 +++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 6 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index 4508f1b..ee9f6ab 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -23,6 +23,47 @@ function hostSources(): readonly { readonly file: string; readonly text: string .map((name) => ({ file: name, text: readFileSync(join(hostDir, name), 'utf8') })); } +/** + * Extract every module specifier from TypeScript/NodeNext ESM source. + * + * The import-discipline checks below judge *specifiers*, so a specifier this + * helper fails to surface is silently exempt from the boundary. It therefore + * covers all the static-graph import forms this project can use, in either + * quote style, so no forbidden dependency can hide behind syntax: + * + * - static: `import x from '...'` / `import x from "..."` + * (including multi-line `import type { ... } from '...'`) + * - side-effect: `import '...'` / `import "..."` + * - dynamic: `import('...')` / `import("...")` + * - re-export: `export { x } from '...'` / `export * from "..."` + * + * `import.meta.url` is deliberately not matched: the `import` keyword must be + * followed by whitespace (static/side-effect) or `(` (dynamic), and `.` is + * neither. This is a bounded lexical scan, not a parser — no AST dependency is + * introduced, and, like the original scanner, it does not exclude specifiers + * that appear inside comments. + */ +function extractModuleSpecifiers(source: string): readonly string[] { + const patterns: readonly RegExp[] = [ + // static (`import x from 'S'`) and side-effect (`import 'S'`) imports. + /\bimport\s+(?:[^'"]*?\bfrom\s+)?['"]([^'"]+)['"]/g, + // dynamic imports: `import('S')`. + /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g, + // re-export bindings: `export { x } from 'S'`, `export * from 'S'`. + /\bexport\b[^'"]*?\bfrom\s+['"]([^'"]+)['"]/g, + ]; + const specifiers: string[] = []; + for (const pattern of patterns) { + for (const match of source.matchAll(pattern)) { + const specifier = match[1]; + if (specifier !== undefined) { + specifiers.push(specifier); + } + } + } + return specifiers; +} + describe('D3 host has no mutation, subprocess, secret, or Git capability', () => { it('references no subprocess, environment, or Git operation', () => { const forbidden: readonly RegExp[] = [ @@ -45,12 +86,9 @@ describe('D3 host has no mutation, subprocess, secret, or Git capability', () => }); describe('D3 host import discipline', () => { - const importSpecifiers = /from\s+'([^']+)'/g; - it('imports only node builtins, itself, or the Cockpit boundary', () => { for (const { file, text } of hostSources()) { - for (const match of text.matchAll(importSpecifiers)) { - const specifier = match[1] ?? ''; + for (const specifier of extractModuleSpecifiers(text)) { const allowed = specifier.startsWith('node:') || specifier.startsWith('./') || @@ -62,8 +100,7 @@ describe('D3 host import discipline', () => { it('never imports an adapter, transport, or authority module (nor the domain kernel directly)', () => { for (const { file, text } of hostSources()) { - for (const match of text.matchAll(importSpecifiers)) { - const specifier = match[1] ?? ''; + for (const specifier of extractModuleSpecifiers(text)) { expect( /adapter|transport|authorization|repair-job|permit|\.\.\/domain\//i.test(specifier), `${file} imports forbidden module: ${specifier}`, @@ -72,3 +109,49 @@ describe('D3 host import discipline', () => { } }); }); + +describe('D3 host import scanner recognizes every supported ESM form (D3-CR-F1)', () => { + // A forbidden domain/adapter import must be surfaced no matter which valid + // import syntax hides it — otherwise the discipline checks above are blind to + // it. Each fixture below is a single valid TypeScript/NodeNext ESM statement. + const forbiddenForms: readonly { readonly form: string; readonly source: string }[] = [ + { form: 'single-quoted static from', source: `import x from '../domain/foo.js';` }, + { form: 'double-quoted static from', source: `import x from "../domain/foo.js";` }, + { form: 'single-quoted side-effect', source: `import '../adapters/foo.js';` }, + { form: 'double-quoted side-effect', source: `import "../adapters/foo.js";` }, + { form: 'single-quoted dynamic', source: `const m = import('../domain/foo.js');` }, + { form: 'double-quoted dynamic', source: `const m = import("../domain/foo.js");` }, + { form: 're-export from', source: `export { y } from "../domain/foo.js";` }, + ]; + + for (const { form, source } of forbiddenForms) { + it(`extracts the forbidden specifier from a ${form} import`, () => { + const specifiers = extractModuleSpecifiers(source); + const forbidden = specifiers.filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + expect(forbidden.length, `no specifier extracted from: ${source}`).toBeGreaterThan(0); + }); + } + + it('extracts allowed node builtin, local, and Cockpit-boundary specifiers', () => { + expect(extractModuleSpecifiers(`import http from 'node:http';`)).toContain('node:http'); + expect(extractModuleSpecifiers(`import { a } from "./local.js";`)).toContain('./local.js'); + expect( + extractModuleSpecifiers(`import { readCockpitSnapshot } from '../cockpit/index.js';`), + ).toContain('../cockpit/index.js'); + }); + + it('extracts a multi-line `import type { ... } from` specifier', () => { + const source = [ + 'import type {', + ' CockpitSnapshot,', + ' CockpitFindingReadModel,', + "} from '../cockpit/index.js';", + ].join('\n'); + expect(extractModuleSpecifiers(source)).toContain('../cockpit/index.js'); + }); + + it('does not treat `import.meta.url` as a module specifier', () => { + const source = `const isEntry = import.meta.url === pathToFileURL(entry).href;`; + expect(extractModuleSpecifiers(source)).toEqual([]); + }); +}); From 7627ff996af8c1e06477b69d0ec8fbb11317822f Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Tue, 25 Aug 2026 21:23:40 +0200 Subject: [PATCH 04/21] test(cockpit): close D3 scanner residual gaps --- tests/cockpit-host/purity.test.ts | 76 ++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index ee9f6ab..b693251 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -34,23 +34,36 @@ function hostSources(): readonly { readonly file: string; readonly text: string * - static: `import x from '...'` / `import x from "..."` * (including multi-line `import type { ... } from '...'`) * - side-effect: `import '...'` / `import "..."` - * - dynamic: `import('...')` / `import("...")` + * - dynamic: `import('...')` / `import("...")`, with or without a second + * options argument (`import('...', { with: { type: 'json' } })`) * - re-export: `export { x } from '...'` / `export * from "..."` * + * A block comment between tokens does not hide the specifier — not even a block + * comment that itself contains quote characters (a quoted-comment token + * separator between `import` and `from`, or a comment just inside `import(`). + * The token region between keywords consumes a whole block comment as a single + * unit instead of stopping at the first quote inside it. (Ordinary unquoted + * comment separators were already handled and remain so.) + * * `import.meta.url` is deliberately not matched: the `import` keyword must be * followed by whitespace (static/side-effect) or `(` (dynamic), and `.` is * neither. This is a bounded lexical scan, not a parser — no AST dependency is - * introduced, and, like the original scanner, it does not exclude specifiers - * that appear inside comments. + * introduced. */ function extractModuleSpecifiers(source: string): readonly string[] { + // A run of source between two keywords that may legally hold whole block + // comments (which can contain quote characters) or any other non-quote text. + // Consuming a `/* ... */` as one unit is what lets a quoted comment sit + // between `import` and `from` without the scanner mistaking the comment's + // quote for the specifier delimiter. const patterns: readonly RegExp[] = [ // static (`import x from 'S'`) and side-effect (`import 'S'`) imports. - /\bimport\s+(?:[^'"]*?\bfrom\s+)?['"]([^'"]+)['"]/g, - // dynamic imports: `import('S')`. - /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g, + /\bimport\s+(?:(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+)?['"]([^'"]+)['"]/g, + // dynamic imports: `import('S')`, an optional leading block comment, and an + // optional second options argument (closing `)` or a comma introduces it). + /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]([^'"]+)['"]\s*[,)]/g, // re-export bindings: `export { x } from 'S'`, `export * from 'S'`. - /\bexport\b[^'"]*?\bfrom\s+['"]([^'"]+)['"]/g, + /\bexport\b(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+['"]([^'"]+)['"]/g, ]; const specifiers: string[] = []; for (const pattern of patterns) { @@ -155,3 +168,52 @@ describe('D3 host import scanner recognizes every supported ESM form (D3-CR-F1)' expect(extractModuleSpecifiers(source)).toEqual([]); }); }); + +describe('D3 host import scanner covers dynamic-options and block-comment forms (D3-CR-F2/F3)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // D3-CR-F2: a dynamic import that carries a second options argument still + // surfaces its specifier. Before this fix the scanner required `)` right after + // the closing quote, so the comma-led options form extracted nothing and the + // forbidden dependency slipped past both discipline checks. + it('extracts the specifier from a dynamic import with an import-attributes options object', () => { + expect(forbiddenIn(`import('../domain/foo.js', { with: { type: 'json' } })`).length).toBeGreaterThan(0); + }); + + it('extracts the specifier from a dynamic import with a bundler-style options object', () => { + expect(forbiddenIn(`import('../domain/foo.js', { webpackChunkName: 'foo' })`).length).toBeGreaterThan(0); + }); + + // D3-CR-F3 (narrow, independently reproduced cases only). + it('extracts the specifier when a block comment sits inside the dynamic import', () => { + expect(forbiddenIn(`import(/* note */ '../domain/foo.js')`).length).toBeGreaterThan(0); + }); + + it('extracts the specifier across a quoted-comment token separator', () => { + // The block comment contains a quote character; the scanner must consume the + // whole comment as a unit rather than treating that inner quote as the + // specifier delimiter. + expect(forbiddenIn(`import /* 'note' */ x from '../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + // Preservation: the ordinary unquoted comment separator was never broken and + // must keep working (guards against over-narrowing the fix). The broad claim + // that ordinary comment separators evade the scanner was NOT REPRODUCIBLE. + it('still extracts across an ordinary unquoted comment separator', () => { + expect(forbiddenIn(`import /* note */ x from '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(forbiddenIn(`export /* note */ { x } from '../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + // Preservation: a single-argument dynamic import and allowed specifiers are + // unaffected, and import.meta.url is still ignored. + it('preserves single-argument dynamic, allowed, and import.meta behaviour', () => { + expect(extractModuleSpecifiers(`import('../domain/foo.js');`)).toContain('../domain/foo.js'); + expect(extractModuleSpecifiers(`import http from 'node:http';`)).toContain('node:http'); + expect(extractModuleSpecifiers(`import { a } from "./local.js";`)).toContain('./local.js'); + expect(extractModuleSpecifiers(`import { r } from '../cockpit/index.js';`)).toContain( + '../cockpit/index.js', + ); + expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); + }); +}); From 654ded06752fc41f03bfa6f9c0dfb81e86e1da7f Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Wed, 26 Aug 2026 07:17:53 +0200 Subject: [PATCH 05/21] test(cockpit): close D3 scanner comment gaps --- tests/cockpit-host/purity.test.ts | 72 ++++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index b693251..a845c32 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -43,7 +43,10 @@ function hostSources(): readonly { readonly file: string; readonly text: string * separator between `import` and `from`, or a comment just inside `import(`). * The token region between keywords consumes a whole block comment as a single * unit instead of stopping at the first quote inside it. (Ordinary unquoted - * comment separators were already handled and remain so.) + * comment separators were already handled and remain so.) A block comment is + * also tolerated in two further positions: immediately before a bare + * side-effect specifier (between `import` and the string), and after a + * dynamic-import specifier, before the options comma or the closing paren. * * `import.meta.url` is deliberately not matched: the `import` keyword must be * followed by whitespace (static/side-effect) or `(` (dynamic), and `.` is @@ -57,11 +60,14 @@ function extractModuleSpecifiers(source: string): readonly string[] { // between `import` and `from` without the scanner mistaking the comment's // quote for the specifier delimiter. const patterns: readonly RegExp[] = [ - // static (`import x from 'S'`) and side-effect (`import 'S'`) imports. - /\bimport\s+(?:(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+)?['"]([^'"]+)['"]/g, - // dynamic imports: `import('S')`, an optional leading block comment, and an - // optional second options argument (closing `)` or a comma introduces it). - /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]([^'"]+)['"]\s*[,)]/g, + // static (`import x from 'S'`) and side-effect (`import 'S'`) imports. The + // trailing `(?:/*...*/\s*)*` also lets block comments sit right before a + // bare side-effect specifier, where there is no `from` to consume them. + /\bimport\s+(?:(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+)?(?:\/\*[\s\S]*?\*\/\s*)*['"]([^'"]+)['"]/g, + // dynamic imports: `import('S')`, an optional leading block comment, an + // optional block comment after the specifier, and an optional second + // options argument (closing `)` or a comma introduces it). + /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]([^'"]+)['"](?:\s|\/\*[\s\S]*?\*\/)*[,)]/g, // re-export bindings: `export { x } from 'S'`, `export * from 'S'`. /\bexport\b(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+['"]([^'"]+)['"]/g, ]; @@ -217,3 +223,57 @@ describe('D3 host import scanner covers dynamic-options and block-comment forms expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); }); }); + +describe('D3 host import scanner covers boundary block-comment positions (D3-CR-F4/F5)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // D3-CR-F4: a bare side-effect import has no `from`, so a block comment + // between `import` and the specifier previously fell through every branch and + // the forbidden dependency was not surfaced. + it('extracts a side-effect specifier preceded by an unquoted block comment', () => { + expect(forbiddenIn(`import /* note */ '../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + it('extracts a side-effect specifier preceded by a quoted block comment', () => { + expect(forbiddenIn(`import /* "note" */ '../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + it('extracts a double-quoted side-effect specifier preceded by a block comment', () => { + expect(forbiddenIn(`import /* note */ "../adapters/foo.js";`).length).toBeGreaterThan(0); + }); + + // D3-CR-F5: a block comment after the dynamic-import specifier, before the + // options comma or the closing paren, previously blocked the match because + // only whitespace was allowed in that position. + it('extracts a dynamic specifier with a trailing block comment before the options object', () => { + expect( + forbiddenIn(`import('../domain/foo.js' /* note */, { with: { type: 'json' } })`).length, + ).toBeGreaterThan(0); + }); + + it('extracts a double-quoted dynamic specifier with a trailing block comment before options', () => { + expect( + forbiddenIn(`import("../domain/foo.js" /* note */, { with: { type: "json" } })`).length, + ).toBeGreaterThan(0); + }); + + it('extracts a dynamic specifier with a trailing block comment before the closing paren', () => { + expect(forbiddenIn(`import('../domain/foo.js' /* note */)`).length).toBeGreaterThan(0); + }); + + // Preservation: the earlier boundary-comment forms and allowed/import.meta + // behaviour are unaffected by widening these two positions. + it('preserves prior comment forms, allowed imports, and import.meta exclusion', () => { + expect(forbiddenIn(`import(/* note */ '../domain/foo.js')`).length).toBeGreaterThan(0); + expect( + forbiddenIn(`import('../domain/foo.js', { with: { type: 'json' } })`).length, + ).toBeGreaterThan(0); + expect(forbiddenIn(`import /* 'note' */ x from '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(extractModuleSpecifiers(`import http from 'node:http';`)).toContain('node:http'); + expect(extractModuleSpecifiers(`import { r } from '../cockpit/index.js';`)).toContain( + '../cockpit/index.js', + ); + expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); + }); +}); From 24efb0a7b5e553e7e025fed748f65c037f5ddf13 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Wed, 26 Aug 2026 12:35:20 +0200 Subject: [PATCH 06/21] test(cockpit): close D3 re-export comment gap --- tests/cockpit-host/purity.test.ts | 94 +++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 5 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index a845c32..23c38fb 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -44,9 +44,10 @@ function hostSources(): readonly { readonly file: string; readonly text: string * The token region between keywords consumes a whole block comment as a single * unit instead of stopping at the first quote inside it. (Ordinary unquoted * comment separators were already handled and remain so.) A block comment is - * also tolerated in two further positions: immediately before a bare - * side-effect specifier (between `import` and the string), and after a - * dynamic-import specifier, before the options comma or the closing paren. + * also tolerated in three further positions: immediately before a bare + * side-effect specifier (between `import` and the string), after a + * dynamic-import specifier, before the options comma or the closing paren, + * and between a re-export's `from` and its module string. * * `import.meta.url` is deliberately not matched: the `import` keyword must be * followed by whitespace (static/side-effect) or `(` (dynamic), and `.` is @@ -68,8 +69,9 @@ function extractModuleSpecifiers(source: string): readonly string[] { // optional block comment after the specifier, and an optional second // options argument (closing `)` or a comma introduces it). /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]([^'"]+)['"](?:\s|\/\*[\s\S]*?\*\/)*[,)]/g, - // re-export bindings: `export { x } from 'S'`, `export * from 'S'`. - /\bexport\b(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+['"]([^'"]+)['"]/g, + // re-export bindings: `export { x } from 'S'`, `export * from 'S'`, with + // optional block comments between `from` and the module string. + /\bexport\b(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+(?:\/\*[\s\S]*?\*\/\s*)*['"]([^'"]+)['"]/g, ]; const specifiers: string[] = []; for (const pattern of patterns) { @@ -277,3 +279,85 @@ describe('D3 host import scanner covers boundary block-comment positions (D3-CR- expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); }); }); + +describe('D3 host import scanner covers post-`from` re-export comments (D3-CR-F6)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // D3-CR-F6: a block comment between a re-export's `from` and its module string + // blocked the match, because the pattern required the quote right after + // `from`. The static-import pattern already tolerated that position, so only + // the re-export form was blind and the forbidden dependency slipped past both + // discipline checks. + it('extracts a re-export specifier preceded by an unquoted block comment', () => { + expect(forbiddenIn(`export { x } from /* note */ '../domain/foo.js';`).length).toBeGreaterThan( + 0, + ); + }); + + it('extracts a re-export specifier preceded by a quoted block comment', () => { + // The comment holds a quote character, so it must be consumed as a whole + // unit rather than mistaken for the specifier delimiter. + expect( + forbiddenIn(`export { x } from /* "note" */ '../domain/foo.js';`).length, + ).toBeGreaterThan(0); + }); + + it('extracts a double-quoted re-export specifier preceded by a block comment', () => { + expect(forbiddenIn(`export { x } from /* note */ "../domain/foo.js";`).length).toBeGreaterThan( + 0, + ); + }); + + it('extracts an export-star specifier preceded by a block comment', () => { + expect(forbiddenIn(`export * from /* note */ '../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + it('extracts a double-quoted export-star specifier preceded by a block comment', () => { + expect(forbiddenIn(`export * from /* note */ "../adapters/foo.js";`).length).toBeGreaterThan(0); + }); + + it('extracts an `export type` specifier preceded by a block comment', () => { + expect( + forbiddenIn(`export type { T } from /* note */ '../domain/foo.js';`).length, + ).toBeGreaterThan(0); + }); + + it('extracts a re-export specifier across a multi-line block comment', () => { + const source = ['export { x } from /* multi', " line note */ '../domain/foo.js';"].join('\n'); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + // Preservation: the comment-free re-export, every earlier comment position, + // allowed specifiers, and the import.meta.url exclusion are unaffected by + // widening this one position. + it('preserves prior re-export, import, dynamic, allowed, and import.meta behaviour', () => { + expect(forbiddenIn(`export { y } from "../domain/foo.js";`).length).toBeGreaterThan(0); + expect(forbiddenIn(`export /* note */ { x } from '../domain/foo.js';`).length).toBeGreaterThan( + 0, + ); + expect(forbiddenIn(`import /* note */ '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import /* 'note' */ x from '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import(/* note */ '../domain/foo.js')`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import('../domain/foo.js' /* note */)`).length).toBeGreaterThan(0); + expect( + forbiddenIn(`import('../domain/foo.js', { with: { type: 'json' } })`).length, + ).toBeGreaterThan(0); + expect(extractModuleSpecifiers(`import http from 'node:http';`)).toContain('node:http'); + expect(extractModuleSpecifiers(`import { a } from "./local.js";`)).toContain('./local.js'); + expect(extractModuleSpecifiers(`import { r } from '../cockpit/index.js';`)).toContain( + '../cockpit/index.js', + ); + expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); + }); + + // The widened position must not double-count a specifier, and one match must + // not swallow the statement that follows it. + it('extracts each re-export specifier once, without capturing across statements', () => { + const source = [ + "export { a } from /* note */ './local.js';", + "export { b } from '../cockpit/index.js';", + ].join('\n'); + expect(extractModuleSpecifiers(source)).toEqual(['./local.js', '../cockpit/index.js']); + }); +}); From a36e4453e71c111033f0e3295120a67c04523bac Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Wed, 26 Aug 2026 22:41:45 +0200 Subject: [PATCH 07/21] test(cockpit): consolidate ESM scanner invariants --- tests/cockpit-host/purity.test.ts | 515 +++++++++++++++++++++++++++--- 1 file changed, 474 insertions(+), 41 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index 23c38fb..515f36e 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -23,62 +23,299 @@ function hostSources(): readonly { readonly file: string; readonly text: string .map((name) => ({ file: name, text: readFileSync(join(hostDir, name), 'utf8') })); } +/** + * A single ESM lexical token. Comments and whitespace are trivia and are never + * emitted; strings (including template literals) are emitted whole so their + * contents can never be mistaken for code. `num`/`regex` carry no value — they + * exist only so a following `/` is disambiguated between division and a regex + * literal, and so a quote inside a regex body is never read as a string. + */ +type EsmToken = + | { readonly t: 'str'; readonly v: string; readonly template: boolean } + | { readonly t: 'id'; readonly v: string } + | { readonly t: 'punct'; readonly v: string } + | { readonly t: 'num' } + | { readonly t: 'regex' }; + +const isIdentifierStart = (c: string): boolean => /[A-Za-z_$]/.test(c); +const isIdentifierPart = (c: string): boolean => /[A-Za-z0-9_$]/.test(c); + +// Keywords after which a `/` begins a regex literal rather than division. Any +// other identifier (a value, including `from`/`fromValues`) means division. +const REGEX_CONTEXT_KEYWORDS: ReadonlySet = new Set([ + 'return', + 'typeof', + 'instanceof', + 'in', + 'of', + 'new', + 'delete', + 'void', + 'do', + 'else', + 'case', + 'yield', + 'await', + 'throw', +]); + +/** + * Tokenize TypeScript/NodeNext ESM source with a single linear, comment- and + * string-aware pass — a tiny state machine over four conceptual states (CODE, + * LINE_COMMENT, BLOCK_COMMENT, STRING). Each character is consumed exactly once + * and every inner scan advances monotonically, so the pass is O(n): there is no + * regex backtracking and therefore no catastrophic (ReDoS) blow-up on + * comment-heavy input. + * + * Trivia (whitespace, line comments, and block comments) is dropped while + * statement structure is preserved. A `//` or a block-comment opener occurring + * *inside* a string or a regex literal is ordinary text, never a comment. Because comments + * are gone before any specifier is read, no comment-contained `from` can ever + * fabricate a dependency (D3-CX-F8), and no comment position can hide a real one + * (D3-CR-F4/F5/F6/F7 and line comments inside a re-export clause). + */ +function tokenizeEsm(source: string): readonly EsmToken[] { + const tokens: EsmToken[] = []; + const length = source.length; + let index = 0; + let previous: EsmToken | null = null; + + const regexCanFollow = (): boolean => { + if (previous === null) return true; + switch (previous.t) { + case 'id': + return REGEX_CONTEXT_KEYWORDS.has(previous.v); + case 'num': + case 'str': + case 'regex': + return false; + case 'punct': + return previous.v !== ')' && previous.v !== ']'; + } + }; + const emit = (token: EsmToken): void => { + tokens.push(token); + previous = token; + }; + + while (index < length) { + const c = source.charAt(index); + + // insignificant whitespace + if (c === ' ' || c === '\t' || c === '\r' || c === '\n') { + index += 1; + continue; + } + // line comment — trivia, consumed to end of line (the newline stays) + if (c === '/' && source.charAt(index + 1) === '/') { + index += 2; + while (index < length && source.charAt(index) !== '\n') index += 1; + continue; + } + // block comment — trivia, consumed whole as one unit (may span lines) + if (c === '/' && source.charAt(index + 1) === '*') { + index += 2; + while (index < length && !(source.charAt(index) === '*' && source.charAt(index + 1) === '/')) { + index += 1; + } + index += 2; + continue; + } + // single- or double-quoted string — one atomic token; `//`, `/*`, and the + // word `from` inside it are ordinary text and cannot start a comment or a + // re-export clause. + if (c === "'" || c === '"') { + const quote = c; + let cursor = index + 1; + let value = ''; + while (cursor < length) { + const d = source.charAt(cursor); + if (d === '\\') { + value += source.charAt(cursor + 1); + cursor += 2; + continue; + } + if (d === quote) { + cursor += 1; + break; + } + if (d === '\n') break; // an unterminated single/double string ends at the line + value += d; + cursor += 1; + } + emit({ t: 'str', v: value, template: false }); + index = cursor; + continue; + } + // template literal — also an atomic string token, but flagged: only a plain + // `'`/`"` string is a literal module specifier, so a template (`import(`...`)` + // with substitutions) is never surfaced, exactly as before. + if (c === '`') { + let cursor = index + 1; + let value = ''; + while (cursor < length) { + const d = source.charAt(cursor); + if (d === '\\') { + value += source.charAt(cursor + 1); + cursor += 2; + continue; + } + if (d === '`') { + cursor += 1; + break; + } + value += d; + cursor += 1; + } + emit({ t: 'str', v: value, template: true }); + index = cursor; + continue; + } + // regex literal — only where a regex may legally begin, so `a / b` division + // is not mistaken for one. Its body (which may hold quotes, `//`, or `from`) + // is opaque and yields no specifier. + if (c === '/' && regexCanFollow()) { + let cursor = index + 1; + let inClass = false; + let closed = false; + while (cursor < length) { + const d = source.charAt(cursor); + if (d === '\\') { + cursor += 2; + continue; + } + if (d === '\n') break; + if (d === '[') inClass = true; + else if (d === ']') inClass = false; + else if (d === '/' && !inClass) { + cursor += 1; + closed = true; + break; + } + cursor += 1; + } + if (closed) { + while (cursor < length && isIdentifierPart(source.charAt(cursor))) cursor += 1; // flags + emit({ t: 'regex' }); + index = cursor; + continue; + } + // not a terminated regex — fall through and treat `/` as punctuation + } + // identifier / keyword + if (isIdentifierStart(c)) { + let cursor = index + 1; + while (cursor < length && isIdentifierPart(source.charAt(cursor))) cursor += 1; + emit({ t: 'id', v: source.slice(index, cursor) }); + index = cursor; + continue; + } + // numeric literal (coarse — only needs to block a following `/` regex) + if (c >= '0' && c <= '9') { + let cursor = index + 1; + while (cursor < length && /[0-9a-fA-FxXbBoOeE._]/.test(source.charAt(cursor))) cursor += 1; + emit({ t: 'num' }); + index = cursor; + continue; + } + // any other single character is punctuation / operator + emit({ t: 'punct', v: c }); + index += 1; + } + return tokens; +} + /** * Extract every module specifier from TypeScript/NodeNext ESM source. * * The import-discipline checks below judge *specifiers*, so a specifier this - * helper fails to surface is silently exempt from the boundary. It therefore - * covers all the static-graph import forms this project can use, in either - * quote style, so no forbidden dependency can hide behind syntax: + * helper fails to surface is silently exempt from the boundary, and any string + * it wrongly surfaces would fabricate a phantom dependency. It therefore covers + * every static-graph import/re-export form this project can use, in either quote + * style, with comments and quoted export names treated as trivia: * * - static: `import x from '...'` / `import x from "..."` * (including multi-line `import type { ... } from '...'`) * - side-effect: `import '...'` / `import "..."` * - dynamic: `import('...')` / `import("...")`, with or without a second * options argument (`import('...', { with: { type: 'json' } })`) - * - re-export: `export { x } from '...'` / `export * from "..."` + * - re-export: `export { x } from '...'` / `export * from "..."`, + * including quoted export names (`export { "x" as y } from …`) * - * A block comment between tokens does not hide the specifier — not even a block - * comment that itself contains quote characters (a quoted-comment token - * separator between `import` and `from`, or a comment just inside `import(`). - * The token region between keywords consumes a whole block comment as a single - * unit instead of stopping at the first quote inside it. (Ordinary unquoted - * comment separators were already handled and remain so.) A block comment is - * also tolerated in three further positions: immediately before a bare - * side-effect specifier (between `import` and the string), after a - * dynamic-import specifier, before the options comma or the closing paren, - * and between a re-export's `from` and its module string. + * It runs over the token stream from {@link tokenizeEsm}, not the raw text, so + * comments in any position are already gone and strings are atomic. That single + * mechanism replaces the earlier trio of hand-tuned regexes and closes the whole + * scanner family at once: block/line comments as trivia (D3-CR-F4..F7), quoted + * export names, no comment-contained `from` fabricating a dependency (D3-CX-F8), + * no cross-statement capture, no duplicate extraction, and linear-time scanning + * with no catastrophic backtracking. * - * `import.meta.url` is deliberately not matched: the `import` keyword must be - * followed by whitespace (static/side-effect) or `(` (dynamic), and `.` is - * neither. This is a bounded lexical scan, not a parser — no AST dependency is - * introduced. + * The specifier of a static import or a re-export is the string that follows the + * `from` keyword (so a quoted export name before `from` is skipped); a + * side-effect import has no `from`, so its specifier is its first string. A + * scan is bounded by the statement (`;`, or the next `import`/`export`), so a + * following statement can never be captured. `import.meta.*` is excluded: the + * `import` keyword there is followed by `.`, which starts neither a static + * import nor a `(` dynamic import. This is a bounded lexical scan, not a + * parser — no AST dependency is introduced. */ function extractModuleSpecifiers(source: string): readonly string[] { - // A run of source between two keywords that may legally hold whole block - // comments (which can contain quote characters) or any other non-quote text. - // Consuming a `/* ... */` as one unit is what lets a quoted comment sit - // between `import` and `from` without the scanner mistaking the comment's - // quote for the specifier delimiter. - const patterns: readonly RegExp[] = [ - // static (`import x from 'S'`) and side-effect (`import 'S'`) imports. The - // trailing `(?:/*...*/\s*)*` also lets block comments sit right before a - // bare side-effect specifier, where there is no `from` to consume them. - /\bimport\s+(?:(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+)?(?:\/\*[\s\S]*?\*\/\s*)*['"]([^'"]+)['"]/g, - // dynamic imports: `import('S')`, an optional leading block comment, an - // optional block comment after the specifier, and an optional second - // options argument (closing `)` or a comma introduces it). - /\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)?['"]([^'"]+)['"](?:\s|\/\*[\s\S]*?\*\/)*[,)]/g, - // re-export bindings: `export { x } from 'S'`, `export * from 'S'`, with - // optional block comments between `from` and the module string. - /\bexport\b(?:\/\*[\s\S]*?\*\/|[^'"])*?\bfrom\s+(?:\/\*[\s\S]*?\*\/\s*)*['"]([^'"]+)['"]/g, - ]; + const tokens = tokenizeEsm(source); const specifiers: string[] = []; - for (const pattern of patterns) { - for (const match of source.matchAll(pattern)) { - const specifier = match[1]; - if (specifier !== undefined) { - specifiers.push(specifier); + const isStatementBoundary = (tok: EsmToken): boolean => + (tok.t === 'punct' && tok.v === ';') || + (tok.t === 'id' && (tok.v === 'import' || tok.v === 'export')); + + for (let i = 0; i < tokens.length; i += 1) { + const token = tokens[i]; + if (token === undefined || token.t !== 'id') continue; + + if (token.v === 'import') { + const next = tokens[i + 1]; + // `import.meta.*` — not a module specifier. + if (next?.t === 'punct' && next.v === '.') continue; + // dynamic `import( 'S' … )` — the specifier, if a literal, is the first + // token inside the parens. + if (next?.t === 'punct' && next.v === '(') { + const arg = tokens[i + 2]; + if (arg?.t === 'str' && !arg.template) specifiers.push(arg.v); + continue; + } + // static (`import … from 'S'`) or side-effect (`import 'S'`). + let fromSpecifier: string | null = null; + let firstString: string | null = null; + for (let j = i + 1; j < tokens.length; j += 1) { + const scan = tokens[j]; + if (scan === undefined) break; + if (j > i + 1 && isStatementBoundary(scan)) break; + if (scan.t === 'str' && !scan.template && firstString === null) firstString = scan.v; + const after = tokens[j + 1]; + if (scan.t === 'id' && scan.v === 'from' && after?.t === 'str' && !after.template) { + fromSpecifier = after.v; + break; + } + } + const specifier = fromSpecifier ?? firstString; + if (specifier !== null) specifiers.push(specifier); + continue; + } + + if (token.v === 'export') { + // re-export: the specifier is the string immediately after the `from` + // keyword. Requiring a string right after `from` distinguishes the clause + // keyword from an identifier that merely starts with `from`, and from a + // `from` used as an exported binding name. The scan stops at `;` or the + // next statement, so no following statement is captured. + for (let j = i + 1; j < tokens.length; j += 1) { + const scan = tokens[j]; + if (scan === undefined) break; + if (scan.t === 'punct' && scan.v === ';') break; + if (scan.t === 'id' && (scan.v === 'import' || scan.v === 'export')) break; + const after = tokens[j + 1]; + if (scan.t === 'id' && scan.v === 'from' && after?.t === 'str' && !after.template) { + specifiers.push(after.v); + break; + } } } } @@ -361,3 +598,199 @@ describe('D3 host import scanner covers post-`from` re-export comments (D3-CR-F6 expect(extractModuleSpecifiers(source)).toEqual(['./local.js', '../cockpit/index.js']); }); }); + +describe('D3 host import scanner is comment/string-aware across the whole family (D3-CR-F7/D3-CX-F8 consolidated)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // --- Comment adjacent to / abutting `from` (D3-CR-F7) --- + it('extracts a named re-export with a block comment abutting `from`', () => { + expect(forbiddenIn(`export { x } from/* note */'../domain/foo.js';`).length).toBeGreaterThan(0); + }); + + it('extracts star/type/double-quoted re-exports with a comment abutting `from`', () => { + expect(forbiddenIn(`export * from/* note */'../domain/foo.js';`).length).toBeGreaterThan(0); + expect( + forbiddenIn(`export type { T } from/* note */'../domain/foo.js';`).length, + ).toBeGreaterThan(0); + expect(forbiddenIn(`export * from/* note */"../adapters/foo.js";`).length).toBeGreaterThan(0); + }); + + it('extracts a specifier across a multi-line block comment abutting `from`', () => { + const source = ['export { x } from/* multi', " line note */'../domain/foo.js';"].join('\n'); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + it('tolerates multiple consecutive comments after `from`', () => { + expect(forbiddenIn(`export { x } from/* a *//* b */'../domain/foo.js';`).length).toBeGreaterThan( + 0, + ); + }); + + // --- Previously-open holes now closed --- + // A: a line comment INSIDE a real re-export clause (before the real `from`). + it('extracts a re-export whose clause contains a line comment (hole A)', () => { + const source = ['export {', ' x, // note', ' y', "} from '../domain/foo.js';"].join('\n'); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + // B: a line comment between the export clause and the real `from`. + it('extracts a re-export with a line comment before `from` (hole B)', () => { + const source = ["export { x } // note", "from '../domain/foo.js';"].join('\n'); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + // C: a valid string ModuleExportName in the clause must not be read as the + // specifier, and must not block reaching the real specifier after `from`. + it('extracts a re-export with a quoted export name, not the quoted name (hole C)', () => { + expect(extractModuleSpecifiers(`export { "foo" as bar } from '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers(`export { x as "foo" } from "../adapters/foo.js";`)).toEqual([ + '../adapters/foo.js', + ]); + }); + + it('extracts a static import that uses a quoted import name', () => { + expect(extractModuleSpecifiers(`import { "foo" as bar } from '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + }); + + it('extracts a re-export whose binding is literally named `from`', () => { + expect(extractModuleSpecifiers(`export { from } from '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + }); + + // --- Comment-contained `from` must NOT fabricate a dependency (D3-CX-F8) --- + const falsePositiveFixtures: readonly { readonly form: string; readonly source: string }[] = [ + { + form: 'line comment, `from` abutting the quote', + source: `export const safe = true; // docs: from'../domain/example.js'`, + }, + { + form: 'line comment, whitespace before the quote', + source: `export const safe = true; // docs: from '../domain/example.js'`, + }, + { + form: 'block comment, `from` abutting the quote', + source: `export const safe = true; /* docs: from'../domain/example.js' */`, + }, + { + form: 'multi-line block comment', + source: ['export const safe = true;', '/* docs:', " from'../domain/example.js'", '*/'].join( + '\n', + ), + }, + { + form: 'ASI (no semicolon), trailing block comment', + source: `export const x = true /* docs: from'../domain/example.js' */`, + }, + { + form: 'ASI (no semicolon), trailing line comment', + source: ['export const x = true', "// docs: from'../domain/example.js'"].join('\n'), + }, + { + form: 'a `from`-bearing string value, not a re-export', + source: `export const doc = "from '../domain/example.js'";`, + }, + ]; + + for (const { form, source } of falsePositiveFixtures) { + it(`does not extract a comment- or string-contained module (${form})`, () => { + expect(extractModuleSpecifiers(source)).not.toContain('../domain/example.js'); + expect(forbiddenIn(source)).toEqual([]); + }); + } + + it('still consumes a prefix comment holding quotes or the word `from` as trivia', () => { + expect(extractModuleSpecifiers(`export /* 'note' */ { x } from '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers(`export /* from 'x' */ { a } from '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + }); + + // --- Identifier safety: only the exact `from` keyword introduces a clause --- + it('does not treat an identifier beginning with `from` as the clause keyword', () => { + expect(extractModuleSpecifiers(`export const fromValues = 1;`)).toEqual([]); + expect(extractModuleSpecifiers(`export const from_foo = true;`)).toEqual([]); + expect(extractModuleSpecifiers(`export { y } fromX '../domain/foo.js';`)).toEqual([]); + expect(extractModuleSpecifiers(`export { y } from1 '../domain/foo.js';`)).toEqual([]); + expect(extractModuleSpecifiers(`const a = Array.from(xs);`)).toEqual([]); + expect(extractModuleSpecifiers(`const b = Object.fromEntries(e);`)).toEqual([]); + }); + + // --- Boundaries: no cross-statement capture, no duplicates --- + it('extracts each specifier once across mixed comment-heavy statements', () => { + const source = [ + "export { a } from/* note */'./local.js';", + "export const doc = true; // from'../domain/example.js'", + "export { b } from '../cockpit/index.js';", + ].join('\n'); + expect(extractModuleSpecifiers(source)).toEqual(['./local.js', '../cockpit/index.js']); + }); + + it('extracts mixed import and re-export forms in order, once each', () => { + const source = [ + `import http from 'node:http';`, + `export { a } from './local.js';`, + `const d = import('../domain/foo.js');`, + ].join('\n'); + expect(extractModuleSpecifiers(source)).toEqual([ + 'node:http', + './local.js', + '../domain/foo.js', + ]); + }); + + // --- Slash / token cases: division, regex literals, comment markers in strings --- + it('is not confused by division, regex literals, or comment markers inside strings', () => { + expect(extractModuleSpecifiers(`const q = a / b; const r = 1 / 2;`)).toEqual([]); + expect(extractModuleSpecifiers(`const re = /['"]/g; const s = text.replace(/from/g, 'x');`)).toEqual( + [], + ); + expect(extractModuleSpecifiers(`const u = 'node:http//x'; const v = "a//b";`)).toEqual([]); + expect(extractModuleSpecifiers(`const u = 'a/*b*/c';`)).toEqual([]); + expect(extractModuleSpecifiers(`const u = 'https://example.com/from/x';`)).toEqual([]); + }); + + it('does not read a specifier out of a template literal', () => { + const template = ['const t = `', `import x from '../domain/x.js'`, '`;'].join(''); + expect(extractModuleSpecifiers(template)).toEqual([]); + }); + + // --- Preservation of every earlier form under the new mechanism --- + it('preserves static, side-effect, dynamic, allowed, and import.meta behaviour', () => { + expect(forbiddenIn(`import /* note */ '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import /* 'note' */ x from '../domain/foo.js';`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import(/* note */ '../domain/foo.js')`).length).toBeGreaterThan(0); + expect(forbiddenIn(`import('../domain/foo.js' /* note */)`).length).toBeGreaterThan(0); + expect( + extractModuleSpecifiers(`import('../domain/foo.js', { with: { type: 'json' } })`), + ).toContain('../domain/foo.js'); + expect(extractModuleSpecifiers(`import http from 'node:http';`)).toContain('node:http'); + expect(extractModuleSpecifiers(`import { a } from "./local.js";`)).toContain('./local.js'); + expect(extractModuleSpecifiers(`import { r } from '../cockpit/index.js';`)).toContain( + '../cockpit/index.js', + ); + expect(extractModuleSpecifiers(`const isEntry = import.meta.url === x;`)).toEqual([]); + }); + + // --- Liveness: comment-heavy legal input scans in linear time --- + // The prior regex family exhibited catastrophic backtracking here (seconds for + // ~20 comments). The tokenizer is single-pass, so a much larger input resolves + // instantly; a regression to backtracking would blow vitest's per-test timeout. + it('scans comment-heavy legal input in bounded, linear time', () => { + const heavyImport = `import ${'/* c */'.repeat(400)} '../domain/foo.js';`; + const heavyClause = `export {\n${' a, // note\n'.repeat(400)}} from '../domain/foo.js';`; + const start = performance.now(); + expect(extractModuleSpecifiers(heavyImport)).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers(heavyClause)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); From 2cf9e14c7f302f4d0542c1bf05da3d6818a7e2ab Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Thu, 27 Aug 2026 09:48:14 +0200 Subject: [PATCH 08/21] test(cockpit): close template scanner review gaps --- tests/cockpit-host/purity.test.ts | 378 +++++++++++++++++++++++++++--- 1 file changed, 351 insertions(+), 27 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index 515f36e..6ee4c84 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -25,13 +25,18 @@ function hostSources(): readonly { readonly file: string; readonly text: string /** * A single ESM lexical token. Comments and whitespace are trivia and are never - * emitted; strings (including template literals) are emitted whole so their - * contents can never be mistaken for code. `num`/`regex` carry no value — they - * exist only so a following `/` is disambiguated between division and a regex - * literal, and so a quote inside a regex body is never read as a string. + * emitted; a `'`/`"` string is emitted whole so its contents can never be + * mistaken for code. A template literal's text is likewise opaque, but its + * `${ ... }` substitutions are executable code and are tokenized inline, so an + * `import` hidden in a substitution is still seen. `template` marks a string that + * came from backticks; `hasSubstitution` marks a template that carried a `${ }` + * (so it is a computed value, not a fixed module specifier). `num`/`regex` carry + * no value — they exist only so a following `/` is disambiguated between division + * and a regex literal, and so a quote inside a regex body is never read as a + * string. */ type EsmToken = - | { readonly t: 'str'; readonly v: string; readonly template: boolean } + | { readonly t: 'str'; readonly v: string; readonly template: boolean; readonly hasSubstitution: boolean } | { readonly t: 'id'; readonly v: string } | { readonly t: 'punct'; readonly v: string } | { readonly t: 'num' } @@ -80,6 +85,11 @@ function tokenizeEsm(source: string): readonly EsmToken[] { let index = 0; let previous: EsmToken | null = null; + // Brace-depth stack for the currently open `${ ... }` template substitutions. + // Empty means ordinary code; a top value of 0 means the next unmatched `}` + // closes the current substitution and resumes the enclosing template's text. + const substitutionStack: number[] = []; + const regexCanFollow = (): boolean => { if (previous === null) return true; switch (previous.t) { @@ -98,6 +108,36 @@ function tokenizeEsm(source: string): readonly EsmToken[] { previous = token; }; + // Scan a template literal's *text* run starting at `from` (the character just + // past an opening backtick or past a substitution's closing `}`). Returns + // where scanning stopped, whether a `${` substitution was opened there, and + // the literal text consumed. `\`` and `\${` escapes are honoured so they never + // open a substitution or end the template. Each character is read once, so the + // scan is linear. + const scanTemplateText = ( + from: number, + ): { readonly end: number; readonly opened: boolean; readonly literal: string } => { + let cursor = from; + let literal = ''; + while (cursor < length) { + const d = source.charAt(cursor); + if (d === '\\') { + literal += source.charAt(cursor + 1); + cursor += 2; + continue; + } + if (d === '`') { + return { end: cursor + 1, opened: false, literal }; + } + if (d === '$' && source.charAt(cursor + 1) === '{') { + return { end: cursor + 2, opened: true, literal }; + } + literal += d; + cursor += 1; + } + return { end: cursor, opened: false, literal }; + }; + while (index < length) { const c = source.charAt(index); @@ -143,32 +183,67 @@ function tokenizeEsm(source: string): readonly EsmToken[] { value += d; cursor += 1; } - emit({ t: 'str', v: value, template: false }); + emit({ t: 'str', v: value, template: false, hasSubstitution: false }); index = cursor; continue; } - // template literal — also an atomic string token, but flagged: only a plain - // `'`/`"` string is a literal module specifier, so a template (`import(`...`)` - // with substitutions) is never surfaced, exactly as before. + // template literal — its text is opaque, but any `${ ... }` substitution is + // executable code and is tokenized inline (via the main loop, guarded by the + // substitution-brace stack), so a dynamic import hidden inside a substitution + // is still surfaced (D3-CR-C1). A substitution-free template is a valid fixed + // dynamic-import specifier (D3-CR-R1); a template that *has* substitutions is + // flagged so it is never taken as a fixed specifier. if (c === '`') { - let cursor = index + 1; - let value = ''; - while (cursor < length) { - const d = source.charAt(cursor); - if (d === '\\') { - value += source.charAt(cursor + 1); - cursor += 2; - continue; - } - if (d === '`') { - cursor += 1; - break; + const run = scanTemplateText(index + 1); + index = run.end; + if (run.opened) { + emit({ t: 'str', v: run.literal, template: true, hasSubstitution: true }); + substitutionStack.push(0); + // A `${ ... }` substitution begins a fresh JavaScript expression, so its + // first executable token is at expression-start: a leading `/` is a regex + // literal, not division. `emit` above set `previous` to the template + // prefix `str`, which would wrongly make regexCanFollow() report a value + // context; clear it so expression-start (regex allowed) holds (D3-CR-S1). + previous = null; + } else { + emit({ t: 'str', v: run.literal, template: true, hasSubstitution: false }); + } + continue; + } + // `{` / `}` inside an active substitution: track depth so a `}` that closes + // the `${ ... }` resumes the enclosing template's text instead of being read + // as code. Braces that belong to nested objects/blocks within the + // substitution stay ordinary punctuation. + if (substitutionStack.length > 0 && c === '{') { + const top = substitutionStack.length - 1; + substitutionStack[top] = (substitutionStack[top] ?? 0) + 1; + emit({ t: 'punct', v: '{' }); + index += 1; + continue; + } + if (substitutionStack.length > 0 && c === '}') { + const top = substitutionStack.length - 1; + const depth = substitutionStack[top] ?? 0; + if (depth === 0) { + substitutionStack.pop(); + const run = scanTemplateText(index + 1); + index = run.end; + if (run.opened) { + substitutionStack.push(0); + // another substitution opens immediately (`} … ${`); it too begins a + // fresh expression, so restore expression-start rather than inheriting + // the just-closed substitution's last token (D3-CR-S1). + previous = null; + } else { + // the enclosing template is now fully closed and is a value, so a + // following `/` is division, not the start of a regex. + previous = { t: 'str', v: '', template: true, hasSubstitution: false }; } - value += d; - cursor += 1; + continue; } - emit({ t: 'str', v: value, template: true }); - index = cursor; + substitutionStack[top] = depth - 1; + emit({ t: 'punct', v: '}' }); + index += 1; continue; } // regex literal — only where a regex may legally begin, so `a / b` division @@ -271,14 +346,21 @@ function extractModuleSpecifiers(source: string): readonly string[] { if (token === undefined || token.t !== 'id') continue; if (token.v === 'import') { + const prev = tokens[i - 1]; const next = tokens[i + 1]; + // `obj.import` / `obj.import(...)` — a member access, not an ESM import. + if (prev?.t === 'punct' && prev.v === '.') continue; // `import.meta.*` — not a module specifier. if (next?.t === 'punct' && next.v === '.') continue; + // `{ import: ... }` — a property keyed `import`, not an import statement. + if (next?.t === 'punct' && next.v === ':') continue; // dynamic `import( 'S' … )` — the specifier, if a literal, is the first - // token inside the parens. + // token inside the parens. A plain string or a substitution-free template + // is a fixed specifier; a template that carries `${ }` substitutions is a + // computed value and is not surfaced. if (next?.t === 'punct' && next.v === '(') { const arg = tokens[i + 2]; - if (arg?.t === 'str' && !arg.template) specifiers.push(arg.v); + if (arg?.t === 'str' && !arg.hasSubstitution) specifiers.push(arg.v); continue; } // static (`import … from 'S'`) or side-effect (`import 'S'`). @@ -794,3 +876,245 @@ describe('D3 host import scanner is comment/string-aware across the whole family expect(performance.now() - start).toBeLessThan(1000); }); }); + +describe('D3 host import scanner handles template literals and import context (D3-CR-C1/C3/R1)', () => { + // A backtick built without a template literal, so the source fixtures below can + // embed real backticks and `${ }` sequences as plain text. + const BT = '`'; + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // --- C1: executable code inside a `${ }` substitution is still scanned --- + it('surfaces a dynamic import hidden inside a template substitution (C1)', () => { + const source = 'const text = ' + BT + "${import('../domain/foo.js')}" + BT + ';'; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + it('surfaces a dynamic import inside a nested template substitution (C1)', () => { + const source = 'const t = ' + BT + '${ f(' + BT + '${import("../domain/foo.js")}' + BT + ') }' + BT + ';'; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + it('does not fabricate a specifier from a substitution that holds no import (C1 safety)', () => { + const source = 'const t = ' + BT + '${ compute(x) + y }' + BT + ';'; + expect(extractModuleSpecifiers(source)).toEqual([]); + }); + + // --- R1: a substitution-free template is a valid fixed dynamic specifier --- + it('surfaces a substitution-free template dynamic import specifier (R1)', () => { + const source = 'import(' + BT + '../domain/foo.js' + BT + ');'; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + it('surfaces a substitution-free template dynamic import with options (R1)', () => { + const source = 'import(' + BT + '../domain/foo.js' + BT + ", { with: { type: 'json' } });"; + expect(extractModuleSpecifiers(source)).toContain('../domain/foo.js'); + }); + + it('does NOT surface a template dynamic import that carries a substitution (R1 negative)', () => { + const source = 'import(' + BT + '../domain/${name}.js' + BT + ');'; + expect(extractModuleSpecifiers(source)).toEqual([]); + }); + + it('does NOT accept a template after `from` or as a bare specifier (syntax-invalid forms)', () => { + // `import x from ` template ` ` and `import ` template ` ` are not legal ESM; + // the scanner must not surface them. + expect(extractModuleSpecifiers('import x from ' + BT + '../domain/foo.js' + BT + ';')).toEqual([]); + expect(extractModuleSpecifiers('import ' + BT + '../domain/foo.js' + BT + ';')).toEqual([]); + }); + + // --- C3: `import` only counts in a genuine import context --- + it('does not treat a property keyed `import` as an import (C3)', () => { + expect(extractModuleSpecifiers("const config = { import: '../domain/foo.js' };")).toEqual([]); + expect(extractModuleSpecifiers("const config = { import : '../domain/foo.js' };")).toEqual([]); + }); + + it('does not treat a method named `import` as an import (C3)', () => { + expect(extractModuleSpecifiers('const obj = { import() { return 1; } };')).toEqual([]); + expect( + extractModuleSpecifiers("const obj = { import() { return '../domain/foo.js'; } };"), + ).toEqual([]); + }); + + it('does not treat member access `obj.import(...)` as a dynamic import (C3)', () => { + expect(extractModuleSpecifiers("obj.import('../domain/foo.js');")).toEqual([]); + expect(extractModuleSpecifiers('const x = obj.import;')).toEqual([]); + }); + + it('does not treat a quoted `"import"` key or `import`-prefixed identifier as an import (C3)', () => { + expect(extractModuleSpecifiers('const o = { "import": \'../domain/foo.js\' };')).toEqual([]); + expect(extractModuleSpecifiers("const importX = '../domain/foo.js';")).toEqual([]); + expect(extractModuleSpecifiers("const reimport = '../domain/foo.js';")).toEqual([]); + }); + + // --- Adversarial template tokenizer state --- + it('keeps tokenizer state correct across template escapes, comments, strings, and regex', () => { + const F = '../domain/foo.js'; + const cases: readonly string[] = [ + 'const t = ' + BT + 'a\\' + BT + 'b' + BT + "; import '" + F + "';", // escaped backtick + 'const t = ' + BT + '\\${import("x")}' + BT + "; import '" + F + "';", // escaped ${ is text + 'const t = ' + BT + '${ {a:1} }' + BT + "; import '" + F + "';", // object braces in subst + 'const t = ' + BT + '${ "}" + import(\'' + F + '\') }' + BT + ';', // string holding } in subst + 'const t = ' + BT + '${ /* } */ import(\'' + F + '\') }' + BT + ';', // comment holding } in subst + 'const t = ' + BT + '${ /[}]/g.test(x) }' + BT + "; import '" + F + "';", // regex holding } in subst + ]; + // Every case links exactly one real forbidden import (the trailing/inner one). + for (const source of cases) { + expect(forbiddenIn(source).length).toBeGreaterThan(0); + } + // Escaped-`${` and object-brace cases must not themselves fabricate a module. + expect(extractModuleSpecifiers('const t = ' + BT + '\\${import("../domain/x.js")}' + BT + ';')).toEqual( + [], + ); + }); + + it('scans substitution-heavy and deeply-nested templates in bounded, linear time', () => { + const many = 'const t = ' + BT + '${x}'.repeat(600) + BT + "; import '../domain/foo.js';"; + let nested = "import('../domain/foo.js')"; + for (let k = 0; k < 600; k += 1) nested = BT + '${' + nested + '}' + BT; + const nestedSource = 'const t = ' + nested + ';'; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers(nestedSource)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); + +describe('D3 host import scanner treats a `${` substitution as expression-start (D3-CR-S1)', () => { + // A `${ ... }` substitution begins a fresh JavaScript expression. The prior + // template-substitution tokenizer left `previous` pointing at the template + // prefix `str`, so regexCanFollow() reported a *value* context and a leading + // `/` inside the substitution was mis-tokenized as division. A quote in the + // resulting "regex-as-division" text then opened a spurious string that either + // swallowed a following real import (false negative) or exposed a fake one + // buried in the regex body (false positive). The fix resets `previous` to + // expression-start whenever a substitution opens (both `\`${` and `}…${`). + const BT = '`'; + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // --- S1-A: a regex whose class holds a quote must not swallow a later import. + // Single-line form is load-bearing: with the defect the spurious string runs + // to the real import's quote and the specifier is lost (returns []). + it('surfaces a real import after a `${ /[\']/ }` regex on the same line (S1-A)', () => { + const source = 'const t = ' + BT + "${ /[']/.test(x) }" + BT + "; import '../domain/secret.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/secret.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + it('surfaces a real import after a `${ /[\']/ }` regex on the next line (S1-A)', () => { + const source = ['const t = ' + BT + "${ /[']/.test(x) }" + BT + ';', "import '../domain/secret.js';"].join( + '\n', + ); + expect(extractModuleSpecifiers(source)).toEqual(['../domain/secret.js']); + }); + + // --- S1-B: a fake `import('...')` buried inside a regex body must NOT surface. + it('does not fabricate a module from an import call inside a `${ /.../ }` regex body (S1-B)', () => { + const source = 'const t = ' + BT + "${ /import('../domain/evil.js')/ }" + BT + ';'; + expect(extractModuleSpecifiers(source)).toEqual([]); + expect(forbiddenIn(source)).toEqual([]); + }); + + // --- S1-C: a leading regex followed by a ternary must not hide a later import. + it('surfaces a real import after a `${ /\\s+/ ? .. : .. }` ternary regex (S1-C)', () => { + const source = 'const t = ' + BT + '${ /\\s+/.test(x) ? "a" : "b" }' + BT + "; import '../domain/z.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/z.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + // --- Regex-vs-division context matrix. Each `${` independently begins an + // expression, so a leading `/` is a regex; after a value-producing token a `/` + // is division. A trailing real import proves no spurious string swallowed it. + it('reads a `/` at expression-start inside `${` as a regex literal', () => { + const withImport = (subst: string): string => + 'const t = ' + BT + subst + BT + "; import '../domain/z.js';"; + // leading / parenthesised / unary / ternary / logical / assignment regex + expect(extractModuleSpecifiers(withImport('${ /abc/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ /a\\/b/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ /[\'"]/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ (/abc/).test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ !/abc/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ x ? /a/ : /b/ }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ x && /a/.test(y) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ (r = /a/) }'))).toEqual(['../domain/z.js']); + }); + + it('reads a `/` after a value-producing token inside `${` as division', () => { + // No import is present, so a mis-read regex (which would eat to the next `/`) + // could only ADD a phantom; each of these must stay empty. + expect(extractModuleSpecifiers('const t = ' + BT + '${ x / 2 }' + BT + ';')).toEqual([]); + expect(extractModuleSpecifiers('const t = ' + BT + '${ 4 / 2 }' + BT + ';')).toEqual([]); + expect(extractModuleSpecifiers('const t = ' + BT + '${ fn() / 2 }' + BT + ';')).toEqual([]); + expect(extractModuleSpecifiers('const t = ' + BT + '${ arr[0] / 2 }' + BT + ';')).toEqual([]); + expect(extractModuleSpecifiers('const t = ' + BT + '${ ({ x: 1 }).x / 2 }' + BT + ';')).toEqual([]); + }); + + // --- Each `${` in a multi-substitution template independently resets context. + it('gives every substitution its own expression-start (regex then division)', () => { + const a = 'const t = ' + BT + '${ /a/.test(x) }-${ /b/.test(y) }' + BT + "; import '../domain/z.js';"; + expect(extractModuleSpecifiers(a)).toEqual(['../domain/z.js']); + // second substitution is a division context and must not fabricate a module + const b = 'const t = ' + BT + '${ /a/.test(x) }-${ y / 2 }' + BT + ';'; + expect(extractModuleSpecifiers(b)).toEqual([]); + }); + + // --- A nested template restores expression-start for its own substitution and + // then correctly resumes division in the outer expression on return. + it('resets and restores context correctly across nested template substitutions', () => { + const source = + 'const t = ' + BT + '${ ' + BT + 'x ${ /a/.test(p) }' + BT + ' + q / 2 }' + BT + "; import '../domain/z.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/z.js']); + }); + + // --- A regex body may hold quotes, `//`, `}`, or a `${` sequence without + // desyncing the substitution depth or fabricating a fake dependency. + it('keeps substitution depth intact when a regex body holds quotes, comments, or `${`', () => { + const withImport = (subst: string): string => + 'const t = ' + BT + subst + BT + "; import '../domain/z.js';"; + expect(extractModuleSpecifiers(withImport('${ /a"b/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport("${ /a'b/.test(x) }"))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ /a\\/\\/b/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ /a${b}/.test(x) }'))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(withImport('${ /[}]/g.test(x) }'))).toEqual(['../domain/z.js']); + // fake import/export text living inside a regex body must never surface + expect( + extractModuleSpecifiers('const t = ' + BT + "${ /from '..\\/domain\\/evil.js'/.test(x) }" + BT + ';'), + ).toEqual([]); + expect( + extractModuleSpecifiers( + 'const t = ' + BT + "${ /export y from '..\\/domain\\/e.js'/.test(x) }" + BT + ';', + ), + ).toEqual([]); + }); + + // --- Preservation: C1/C3/R1 remain fixed under the expression-start change. + it('preserves C1 substitution imports, R1 template specifiers, and C3 non-imports', () => { + expect(extractModuleSpecifiers('const text = ' + BT + "${import('../domain/foo.js')}" + BT + ';')).toEqual([ + '../domain/foo.js', + ]); + expect( + extractModuleSpecifiers('const t = ' + BT + '${ f(' + BT + '${import("../domain/foo.js")}' + BT + ') }' + BT + ';'), + ).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers('import(' + BT + '../domain/foo.js' + BT + ');')).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers('import(' + BT + '../domain/${name}.js' + BT + ');')).toEqual([]); + expect(extractModuleSpecifiers("const config = { import: '../domain/foo.js' };")).toEqual([]); + }); + + // --- Liveness: many regex-leading substitutions and nested templates scan in + // bounded linear time; a regression to rescanning would blow the timeout. + it('scans many regex-leading substitutions in bounded, linear time', () => { + const many = + 'const t = ' + BT + '${ /a/.test(x) }'.repeat(600) + BT + "; import '../domain/foo.js';"; + let nested = "import('../domain/foo.js')"; + for (let k = 0; k < 600; k += 1) nested = BT + '${ /q/.test(z) || ' + nested + ' }' + BT; + const nestedSource = 'const t = ' + nested + ';'; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers(nestedSource)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); From aed7376404b2aa1a59376c6e4af0c1b69d812869 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Thu, 27 Aug 2026 12:20:56 +0200 Subject: [PATCH 09/21] test(cockpit): harden control-context scanning --- tests/cockpit-host/purity.test.ts | 304 +++++++++++++++++++++++++++++- 1 file changed, 300 insertions(+), 4 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index 6ee4c84..e47519f 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -38,7 +38,7 @@ function hostSources(): readonly { readonly file: string; readonly text: string type EsmToken = | { readonly t: 'str'; readonly v: string; readonly template: boolean; readonly hasSubstitution: boolean } | { readonly t: 'id'; readonly v: string } - | { readonly t: 'punct'; readonly v: string } + | { readonly t: 'punct'; readonly v: string; readonly controlHeader?: boolean } | { readonly t: 'num' } | { readonly t: 'regex' }; @@ -64,6 +64,21 @@ const REGEX_CONTEXT_KEYWORDS: ReadonlySet = new Set([ 'throw', ]); +// Keywords whose parenthesised head is a *control-flow header* — `if (…)`, +// `while (…)`, `for (…)`, `with (…)`. The `)` that closes such a header is +// followed by a statement, whose first token may legally be a regex literal +// (`if (ok) /re/.test(x);`). This is unlike a value-producing `)` (`fn()`, +// `(x)`), after which a `/` is division. The distinction is carried on the +// closing `)` token via `controlHeader` so `regexCanFollow()` classifies the +// next `/` correctly (D3-CR-B); it is orthogonal to the object-literal `}` +// division case (C2), which is deliberately left unchanged. +const CONTROL_HEADER_KEYWORDS: ReadonlySet = new Set([ + 'if', + 'while', + 'for', + 'with', +]); + /** * Tokenize TypeScript/NodeNext ESM source with a single linear, comment- and * string-aware pass — a tiny state machine over four conceptual states (CODE, @@ -90,6 +105,12 @@ function tokenizeEsm(source: string): readonly EsmToken[] { // closes the current substitution and resumes the enclosing template's text. const substitutionStack: number[] = []; + // Parenthesis-context stack: each open `(` pushes whether it began a + // control-flow header (`if`/`while`/`for`/`with`). The matching `)` pops it and + // records the flag on the emitted token, so a `/` right after the `)` is + // classified as a regex (control header) or division (value paren). + const parenStack: boolean[] = []; + const regexCanFollow = (): boolean => { if (previous === null) return true; switch (previous.t) { @@ -100,9 +121,35 @@ function tokenizeEsm(source: string): readonly EsmToken[] { case 'regex': return false; case 'punct': - return previous.v !== ')' && previous.v !== ']'; + // A `)` closing a control-flow header (`if (…)`) may be followed by a + // regex; a value-producing `)` (`fn()`, `(x)`) means division. `]` is + // always a value (index/array), so a following `/` is division. + if (previous.v === ')') return previous.controlHeader === true; + return previous.v !== ']'; } }; + // Whether the token before an opening `(` is a control-flow keyword — read + // inside this helper (not inline in the main loop) so `previous` keeps its + // declared `EsmToken | null` type and narrows over the full token union, as in + // `regexCanFollow`. + const previousOpensControlHeader = (): boolean => { + if (previous === null || previous.t !== 'id' || !CONTROL_HEADER_KEYWORDS.has(previous.v)) { + return false; + } + // A control keyword spelled as a *member name* — `obj.for(…)`, `Symbol.for(…)`, + // `obj.if(…)` — is a value-producing method call, not a control-flow header: + // its closing `)` must stay a value paren so a following `/` is division, not + // a regex (which would swallow a later real import — D3-CR-B2). The keyword + // heads a control statement only when it is *bare*, i.e. not immediately + // preceded by a `.` member-access punctuator. `previous` is the just-emitted + // keyword (`tokens[len-1]`), so `tokens[len-2]` is the token before it; a + // leading `.` (including the `.` of `?.`) marks member access. Computed access + // (`obj['for'](…)`) already fails the `previous.t === 'id'` check above, since + // the token before `(` is then `]`. + const beforePrevious = tokens[tokens.length - 2]; + if (beforePrevious?.t === 'punct' && beforePrevious.v === '.') return false; + return true; + }; const emit = (token: EsmToken): void => { tokens.push(token); previous = token; @@ -293,6 +340,22 @@ function tokenizeEsm(source: string): readonly EsmToken[] { index = cursor; continue; } + // `(` / `)` — tracked so a control-flow header's closing `)` is distinguished + // from a value-producing `)` when the next `/` is classified. Any `(`/`)` + // reaching here is genuine code punctuation: those inside strings, comments, + // regex bodies, and template text were already consumed above. + if (c === '(') { + parenStack.push(previousOpensControlHeader()); + emit({ t: 'punct', v: '(' }); + index += 1; + continue; + } + if (c === ')') { + const controlHeader = parenStack.pop() ?? false; + emit({ t: 'punct', v: ')', controlHeader }); + index += 1; + continue; + } // any other single character is punctuation / operator emit({ t: 'punct', v: c }); index += 1; @@ -352,8 +415,14 @@ function extractModuleSpecifiers(source: string): readonly string[] { if (prev?.t === 'punct' && prev.v === '.') continue; // `import.meta.*` — not a module specifier. if (next?.t === 'punct' && next.v === '.') continue; - // `{ import: ... }` — a property keyed `import`, not an import statement. - if (next?.t === 'punct' && next.v === ':') continue; + // A member named `import` — `{ import: ... }` (object property keyed + // `import`) or `class C { import = … }` (a class field whose name is the + // reserved word `import`). Neither is an ESM import: `:` introduces the + // property value and `=` the field initializer, so the following string is + // data, not a module specifier (D3-CR-C3/A). A real import is never + // immediately followed by `:` or `=` (`import x = require(…)` puts an + // identifier after `import`, not `=`). + if (next?.t === 'punct' && (next.v === ':' || next.v === '=')) continue; // dynamic `import( 'S' … )` — the specifier, if a literal, is the first // token inside the parens. A plain string or a substitution-free template // is a fixed specifier; a template that carries `${ }` substitutions is a @@ -1118,3 +1187,230 @@ describe('D3 host import scanner treats a `${` substitution as expression-start expect(performance.now() - start).toBeLessThan(1000); }); }); + +describe('D3 host import scanner rejects a class field named `import` (D3-CR-A)', () => { + // `import` is a reserved word, but reserved words are legal member names, so a + // class field may be named `import`. Its `=` initializer is data, not a module + // specifier — the scanner must not fabricate a dependency from it. Reported + // independently by both Codex and CodeRabbit as the same false positive. + it('does not treat a one-line class field `import = …` as an import', () => { + expect(extractModuleSpecifiers("class Config { import = '../domain/foo.js'; }")).toEqual([]); + }); + + it('does not treat a multi-line class field `import = …` as an import', () => { + const source = ['class Config {', " import = '../domain/foo.js';", '}'].join('\n'); + expect(extractModuleSpecifiers(source)).toEqual([]); + }); + + it('does not treat an `import` field beside other fields as an import', () => { + const source = ['class Config {', " import = '../domain/foo.js';", ' other = 1;', '}'].join( + '\n', + ); + expect(extractModuleSpecifiers(source)).toEqual([]); + }); + + it('does not treat a typed or static class field named `import` as an import', () => { + // `import: string = …` is caught by the `:` guard; `static import = …` still + // lands on the `=` guard, since the field-name token is `import`. + expect(extractModuleSpecifiers("class C { import: string = '../domain/foo.js'; }")).toEqual([]); + expect(extractModuleSpecifiers("class C { static import = '../domain/foo.js'; }")).toEqual([]); + }); + + // Preservation: the `=` guard must not blind the scanner to genuine imports, + // which never place `=` immediately after the `import` keyword. + it('still surfaces every genuine import form under the `=` guard', () => { + expect(extractModuleSpecifiers("import '../domain/foo.js';")).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers("import x from '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("import { x } from '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("import type { T } from '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("import('../domain/foo.js');")).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("import('../domain/foo.js', { with: { type: 'json' } });"), + ).toContain('../domain/foo.js'); + }); + + // Preservation: the other `import`-member exclusions are unaffected. + it('keeps excluding property/method/member/prefixed `import` forms', () => { + expect(extractModuleSpecifiers("const o = { import: '../domain/foo.js' };")).toEqual([]); + expect( + extractModuleSpecifiers("const o = { import() { return '../domain/foo.js'; } };"), + ).toEqual([]); + expect(extractModuleSpecifiers("obj.import('../domain/foo.js');")).toEqual([]); + expect(extractModuleSpecifiers('const o = { "import": ' + "'../domain/foo.js' };")).toEqual([]); + expect(extractModuleSpecifiers("const importX = '../domain/foo.js';")).toEqual([]); + expect(extractModuleSpecifiers('const isEntry = import.meta.url === x;')).toEqual([]); + }); +}); + +describe('D3 host import scanner classifies `/` after a control-flow header as a regex (D3-CR-B)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // The `)` that closes `if (…)` / `while (…)` / `for (…)` / `with (…)` is a + // control-flow header, whose statement body may start with a regex literal + // (`if (ok) /re/.test(x);`). Before this fix that `)` was read as a + // value-producing close, so `regexCanFollow()` returned false and the `/` was + // treated as division. A quote inside the regex could then open a spurious + // string (swallowing a following real import — false negative), and an + // `import('…')` inside the regex body could be tokenized as code (fabricating + // a dependency — false positive). This is distinct from the object-literal + // `}` division case (C2), which is left unchanged. + + // False negative: a regex whose class holds a quote must not swallow a later + // import (single-line form is load-bearing). + it('surfaces a real import after a control-flow-header regex on the same line (B false negative)', () => { + const source = "if (ok) /[']/.test(value); import '../domain/foo.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + it('surfaces a real import after a control-flow-header regex on the next line (B false negative)', () => { + const source = ["if (ok) /[']/.test(value);", "import '../domain/foo.js';"].join('\n'); + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + // False positive: an `import('…')` buried in a regex body after a control + // header must NOT surface. + it('does not fabricate a module from an import call inside a control-flow-header regex (B false positive)', () => { + expect(extractModuleSpecifiers("if (ok) /import('../domain/evil.js')/.test(value);")).toEqual( + [], + ); + expect(extractModuleSpecifiers("if (ok) /import('evil')/.test(value);")).toEqual([]); + }); + + // Context matrix: a regex is recognised after every control-flow header, so a + // trailing real import is always surfaced (proving no spurious string ran on). + it('recognizes a regex after if/while/for/with headers and preserves the trailing import', () => { + const withImport = (stmt: string): string => stmt + " import '../domain/z.js';"; + for (const stmt of [ + "if (ok) /abc/.test(x);", + "if (ok) /[']/.test(x);", + 'if (ok) /["]/.test(x);', + "while (ok) /[']/.test(x);", + "for (; ok;) /[']/.test(x);", + "for (let i = 0; i < n; i += 1) /[']/.test(x);", + "if (a && b) /[']/.test(x);", + "if (f(x)) /[']/.test(x);", // nested value paren inside the control header + "if ((a)) /[']/.test(x);", + ]) { + expect(extractModuleSpecifiers(withImport(stmt))).toEqual(['../domain/z.js']); + expect(extractModuleSpecifiers(stmt + "\nimport '../domain/z.js';")).toEqual([ + '../domain/z.js', + ]); + } + }); + + // Preservation: a `/` after a value-producing `)` or `]` stays division, so no + // phantom module is fabricated and a following real import is still surfaced. + it('keeps division after value-producing parens and brackets', () => { + expect(extractModuleSpecifiers('const r = fn() / 2;')).toEqual([]); + expect(extractModuleSpecifiers('const r = (x) / 2;')).toEqual([]); + expect(extractModuleSpecifiers('const r = arr[0] / 2;')).toEqual([]); + expect(extractModuleSpecifiers('const r = (a + b) / c;')).toEqual([]); + expect(extractModuleSpecifiers('function f() { return (x) / 2; }')).toEqual([]); + expect(extractModuleSpecifiers("const r = fn() / 2;\nimport '../domain/z.js';")).toEqual([ + '../domain/z.js', + ]); + }); + + // The object-literal `}` division case (C2) is deliberately untouched: the + // same-line form still swallows and the next-line form still surfaces, exactly + // as before this fix. + it('leaves the object-literal `}` division case (C2) unchanged', () => { + expect(extractModuleSpecifiers("const ratio = {} / value; import '../domain/foo.js';")).toEqual( + [], + ); + expect( + extractModuleSpecifiers("const ratio = {} / value;\nimport '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + }); + + // Member-guard (D3-CR-B2): a control keyword spelled as a member/property name + // (`Symbol.for(…)`, `obj.if(…)`) opens a value-producing call, not a control + // header, so its `)` stays a value paren and a following `/` is division. Before + // this guard, such a `)` was stamped `controlHeader`, the `/` began a regex, and + // the regex swallowed the later real import (false negative — dependency skipped). + it('does not treat a control keyword used as a member name as a control header', () => { + for (const call of [ + "Symbol.for('x')", + 'obj.for(x)', + 'obj.if(x)', + 'a.while(y)', + 'a.with(y)', + 'foo.bar.for(x)', + 'ns.Symbol.for(x)', + ]) { + const source = call + " / 2; import '../domain/foo.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + // next-line form too — the trailing import must survive regardless of layout + expect(extractModuleSpecifiers(call + " / 2;\nimport '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + } + }); + + // Member-guard, computed / optional / prefixed forms must likewise never be + // read as control headers (none should even reach the keyword-member check, but + // pin the behaviour so a future tokenizer change can't silently regress it). + it('keeps division after computed, optional, and control-word-prefixed member calls', () => { + for (const call of [ + "obj['for'](x)", + 'obj["if"](x)', + 'obj?.for(x)', + 'obj?.if(x)', + 'beforeThing(x)', + 'format(x)', + 'different(x)', + 'whileX(x)', + 'ifX(x)', + ]) { + expect(extractModuleSpecifiers(call + " / 2; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + } + }); + + // Preservation of the genuine fix: a *bare* control-flow header still allows a + // following regex, so the false-positive and false-negative B cases stay fixed. + it('still treats a bare control-flow header regex correctly after the member guard', () => { + expect(extractModuleSpecifiers("if (ok) /import('../domain/evil.js')/.test(value);")).toEqual( + [], + ); + expect( + extractModuleSpecifiers("if (ok) /[']/.test(value); import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("while (ok) /[']/.test(value); import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("for (; ok;) /[']/.test(value); import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + }); + + // No stale marker leak: a member call named like a control keyword nested inside + // a genuine control header must not corrupt the header's own `)` classification. + it('keeps a genuine header regex working when it wraps a control-word member call', () => { + expect( + extractModuleSpecifiers("if (Symbol.for('x')) /[']/.test(value); import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + // …and a member call after the header body still divides, not regexes. + expect( + extractModuleSpecifiers("if (ok) { obj.for(x) / 2; } import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + }); + + // Liveness: many control-flow-header regex statements scan in bounded linear + // time; a regression to rescanning would blow vitest's per-test timeout. + it('scans many control-flow-header regex statements in bounded, linear time', () => { + const many = "if (ok) /[']/.test(x);\n".repeat(2000) + "import '../domain/foo.js';"; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); From 2a3ab337d04e7318bfbd88c9b62a7ece68473598 Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Thu, 27 Aug 2026 13:09:50 +0200 Subject: [PATCH 10/21] test(cockpit): handle postfix and for-await scanner contexts --- tests/cockpit-host/purity.test.ts | 175 +++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 3 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index e47519f..cc76b85 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -111,6 +111,23 @@ function tokenizeEsm(source: string): readonly EsmToken[] { // classified as a regex (control header) or division (value paren). const parenStack: boolean[] = []; + // Whether a `/` immediately after `tok` is division — i.e. `tok` ends a value/ + // expression — rather than a regex opener. Mirrors `regexCanFollow`'s per-token + // logic; used for the single look-back the postfix-`!` disambiguation needs. + const endsValue = (tok: EsmToken | undefined): boolean => { + if (tok === undefined) return false; + switch (tok.t) { + case 'id': + return !REGEX_CONTEXT_KEYWORDS.has(tok.v); + case 'num': + case 'str': + case 'regex': + return true; + case 'punct': + if (tok.v === ')') return tok.controlHeader !== true; + return tok.v === ']' || tok.v === '++' || tok.v === '--'; + } + }; const regexCanFollow = (): boolean => { if (previous === null) return true; switch (previous.t) { @@ -125,7 +142,19 @@ function tokenizeEsm(source: string): readonly EsmToken[] { // regex; a value-producing `)` (`fn()`, `(x)`) means division. `]` is // always a value (index/array), so a following `/` is division. if (previous.v === ')') return previous.controlHeader === true; - return previous.v !== ']'; + if (previous.v === ']') return false; + // Postfix `++`/`--` (emitted as one token, maximal-munch, below) end a + // value, so a following `/` is division. A prefix `++x`/`--x` never puts + // the operator immediately before a `/` (the operand does), so this only + // ever fires for the postfix form. Missing this let a fake regex swallow a + // later real import (D3-CR postfix). + if (previous.v === '++' || previous.v === '--') return false; + // `!` is either the TS non-null assertion (`x!` — a value, so `/` is + // division) or logical-not (`!/re/` — a prefix operator, so `/` opens a + // regex). It is the non-null form exactly when it follows a value-ending + // token; otherwise it is logical-not and a regex may follow. + if (previous.v === '!') return !endsValue(tokens[tokens.length - 2]); + return true; } }; // Whether the token before an opening `(` is a control-flow keyword — read @@ -133,9 +162,25 @@ function tokenizeEsm(source: string): readonly EsmToken[] { // declared `EsmToken | null` type and narrows over the full token union, as in // `regexCanFollow`. const previousOpensControlHeader = (): boolean => { - if (previous === null || previous.t !== 'id' || !CONTROL_HEADER_KEYWORDS.has(previous.v)) { - return false; + if (previous === null || previous.t !== 'id') return false; + // `for await (…)` — the async-iteration header. The token before `(` is the + // `await` keyword, itself preceded by a *bare* `for`; that parenthesised head + // is a control-flow header exactly like `for (…)`, so its closing `)` may be + // followed by a regex. `await` anywhere else (`await fn()`, `obj.await(…)`, a + // bare `await (expr)`) is a value/member call, not a header, so it must not + // open one (D3-CR for-await). The `for` must be bare — not a member name + // (`obj.for await` is not valid, but pin the `.` guard for symmetry with the + // keyword-member check below). + if (previous.v === 'await') { + const beforeAwait = tokens[tokens.length - 2]; + const beforeFor = tokens[tokens.length - 3]; + return ( + beforeAwait?.t === 'id' && + beforeAwait.v === 'for' && + !(beforeFor?.t === 'punct' && beforeFor.v === '.') + ); } + if (!CONTROL_HEADER_KEYWORDS.has(previous.v)) return false; // A control keyword spelled as a *member name* — `obj.for(…)`, `Symbol.for(…)`, // `obj.if(…)` — is a value-producing method call, not a control-flow header: // its closing `)` must stay a value paren so a following `/` is division, not @@ -356,6 +401,15 @@ function tokenizeEsm(source: string): readonly EsmToken[] { index += 1; continue; } + // `++` / `--` — combined into one token (maximal munch, as JS lexes), so a + // following `/` is classified as division rather than a regex opener. These + // are value contexts: postfix on the preceding operand, or prefix on the + // following one. Any other `+`/`-` (binary, unary, `+=`) stays single. + if ((c === '+' || c === '-') && source.charAt(index + 1) === c) { + emit({ t: 'punct', v: c + c }); + index += 2; + continue; + } // any other single character is punctuation / operator emit({ t: 'punct', v: c }); index += 1; @@ -1414,3 +1468,118 @@ describe('D3 host import scanner classifies `/` after a control-flow header as a expect(performance.now() - start).toBeLessThan(1000); }); }); + +describe('D3 host import scanner classifies `/` after a postfix operator as division (D3-CR postfix)', () => { + // `x++` / `x--` / TS non-null `x!` end a value, so the following `/` is + // division. Before this fix the tokenizer saw the bare trailing operator and + // `regexCanFollow()` opened a regex; a quote inside that fake regex ran on and + // swallowed a later real import (false negative). `++`/`--` are now emitted as + // one maximal-munch token, and `!` is disambiguated from logical-not by the + // token it follows. + it('surfaces a real import after `++`/`--`/`!` postfix division', () => { + expect(extractModuleSpecifiers("let x = 0; const r = x++ / 2; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("let x = 0; const r = x-- / 2; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("const r = x! / 2; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + }); + + it('surfaces a real import after postfix on member/index/call targets', () => { + for (const lhs of ['arr[i]++', 'obj.value--', 'fn()!', 'arr[i]!']) { + expect(extractModuleSpecifiers(`const r = ${lhs} / 2;\nimport '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + } + }); + + it('never fabricates a module from the fake regex a postfix `/` used to open', () => { + expect(extractModuleSpecifiers("let x = 0; const r = x++ / 2;")).toEqual([]); + expect(extractModuleSpecifiers("const r = x! / 2;")).toEqual([]); + }); + + // Preservation: a *prefix* `++x`/`--x` puts the operand (not the operator) + // immediately before the `/`, so division is already correct there; and a + // genuine regex after a real prefix operator / operator position must still be + // recognised (logical-not `!/re/`, binary `+ /re/`, `return /re/`). + it('keeps prefix increment as division and prefix/operator regex as regex', () => { + expect(extractModuleSpecifiers("let x = 0; const r = ++x / 2;\nimport '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("let x = 0; const r = --x / 2;\nimport '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("if (!/[']/.test(v)) {} import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("const r = a + /[']/.source; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers("function f() { return /[']/.test(v); } import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + }); + + it('scans many postfix-division statements in bounded, linear time', () => { + const many = "let x = 0; const r = x++ / 2;\n".repeat(2000) + "import '../domain/foo.js';"; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); + +describe('D3 host import scanner recognizes `for await (…)` as a control header (D3-CR for-await)', () => { + // `for await (…)` is the async-iteration header: the token before `(` is + // `await`, not `for`, so the base control-header check missed it and its `)` + // was read as a value paren — a following regex became division, its quote ran + // on, and a later real import was swallowed (false negative); an `import('…')` + // inside that regex body was tokenized as code (false positive). The header is + // now recognised only for the exact bare `for` + `await` + `(` sequence. + it('surfaces a real import after a `for await` header regex', () => { + const source = "async function f() { for await (const y of xs) /[']/.test(y); }\nimport '../domain/foo.js';"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + }); + + it('does not fabricate a module from an import call inside a `for await` header regex', () => { + expect( + extractModuleSpecifiers("async function f() { for await (const y of xs) /import('../domain/evil.js')/.test(y); }"), + ).toEqual([]); + }); + + // Preservation: `await` in any non-header position is a value/member call, not a + // control header, so a following `/` stays division and a later import surfaces. + it('keeps `await` value/member forms as value contexts, not headers', () => { + expect( + extractModuleSpecifiers("async function f() { const r = await fn() / 2; import '../domain/foo.js'; }"), + ).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers("const r = obj.await(x) / 2;\nimport '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect( + extractModuleSpecifiers("async function f() { const r = await (y) / 2; import '../domain/foo.js'; }"), + ).toEqual(['../domain/foo.js']); + }); + + // Preservation: a plain `for (…)` and the other headers keep working, and a + // keyword-member (`obj.for`) nested in a `for await` condition still divides. + it('keeps plain `for`/`if`/`while` headers and nested keyword-member division working', () => { + expect(extractModuleSpecifiers("for (let i = 0; i < n; i += 1) /[']/.test(x); import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect( + extractModuleSpecifiers("async function f() { for await (const y of obj.for(x)) /[']/.test(y); }\nimport '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + }); + + it('scans many `for await` header regex statements in bounded, linear time', () => { + const many = + "async function f() { for await (const y of xs) /[']/.test(y); }\n".repeat(2000) + + "import '../domain/foo.js';"; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); From 68df198ad9a3155131a43d3214990e8987fe721e Mon Sep 17 00:00:00 2001 From: LogicDuke Date: Thu, 27 Aug 2026 15:22:35 +0200 Subject: [PATCH 11/21] test(cockpit): preserve restricted-statement regex context --- tests/cockpit-host/purity.test.ts | 389 +++++++++++++++++++++++++++++- 1 file changed, 385 insertions(+), 4 deletions(-) diff --git a/tests/cockpit-host/purity.test.ts b/tests/cockpit-host/purity.test.ts index cc76b85..2fbaee5 100644 --- a/tests/cockpit-host/purity.test.ts +++ b/tests/cockpit-host/purity.test.ts @@ -37,7 +37,7 @@ function hostSources(): readonly { readonly file: string; readonly text: string */ type EsmToken = | { readonly t: 'str'; readonly v: string; readonly template: boolean; readonly hasSubstitution: boolean } - | { readonly t: 'id'; readonly v: string } + | { readonly t: 'id'; readonly v: string; readonly restrictedLabel?: boolean } | { readonly t: 'punct'; readonly v: string; readonly controlHeader?: boolean } | { readonly t: 'num' } | { readonly t: 'regex' }; @@ -64,6 +64,29 @@ const REGEX_CONTEXT_KEYWORDS: ReadonlySet = new Set([ 'throw', ]); +// Restricted-production statement keywords: `break`/`continue`. Their grammar has +// a `[no LineTerminator here]` before the optional label, so a newline after the +// bare keyword triggers ASI and closes the statement — the next line then begins a +// fresh statement whose first token may be a regex literal (`break \n /re/.test(x)`). +// A `/` can therefore never be division after a *bare* `break`/`continue` (a +// same-line `break /…` is a syntax error — a `/` is not a legal label), so a +// following `/` is always a regex opener there. This is unlike REGEX_CONTEXT_KEYWORDS +// because `break`/`continue` are *statements*, not expression operators, and — being +// reserved words that are still legal member names — they must stay value contexts +// when spelled as a member (`obj.break / 2` is division), which `regexCanFollow` +// enforces with a `.`-member guard (D3-CR-BREAK-CONTINUE-ASI). Kept separate from +// REGEX_CONTEXT_KEYWORDS so `endsValue` still treats them as value-ending, which is +// correct for the only reachable `!` case, the non-null member form `obj.break!`. +// The statement also carries one *optional* label (`break outer`); the label id is +// marked `restrictedLabel` at tokenize time (bare keyword, no intervening +// LineTerminator) so a `/` after it is likewise a regex opener — a newline between +// the keyword and the id triggers ASI, making the id a fresh statement where a +// following `/` stays division (D3-CR-BREAK-CONTINUE-LABEL-ASI). +const RESTRICTED_STATEMENT_KEYWORDS: ReadonlySet = new Set([ + 'break', + 'continue', +]); + // Keywords whose parenthesised head is a *control-flow header* — `if (…)`, // `while (…)`, `for (…)`, `with (…)`. The `)` that closes such a header is // followed by a statement, whose first token may legally be a regex literal @@ -100,6 +123,14 @@ function tokenizeEsm(source: string): readonly EsmToken[] { let index = 0; let previous: EsmToken | null = null; + // Whether a LineTerminator has appeared in the trivia (whitespace or a block + // comment's interior) since the last emitted token. Used only to tell a + // `break`/`continue` *label* — same line as the keyword — from a fresh statement + // begun after an ASI-closing newline (D3-CR-BREAK-CONTINUE-LABEL-ASI). Reset to + // false whenever a token is emitted, so it always reflects the gap before the + // *next* token. + let pendingNewline = false; + // Brace-depth stack for the currently open `${ ... }` template substitutions. // Empty means ordinary code; a top value of 0 means the next unmatched `}` // closes the current substitution and resumes the enclosing template's text. @@ -132,7 +163,28 @@ function tokenizeEsm(source: string): readonly EsmToken[] { if (previous === null) return true; switch (previous.t) { case 'id': - return REGEX_CONTEXT_KEYWORDS.has(previous.v); + if (REGEX_CONTEXT_KEYWORDS.has(previous.v)) return true; + // A *bare* `break`/`continue` closes its statement (ASI at the newline), + // so a following `/` opens a regex, not division — a fake regex after it + // otherwise swallowed a later real import (false negative) or fabricated a + // dependency from an `import('…')` in the regex body (false positive). When + // spelled as a member name (`obj.break`, `obj?.continue`), it is a value and + // a following `/` is division, so guard on a preceding `.` (D3-CR-BREAK- + // CONTINUE-ASI). `previous` is the keyword (`tokens[len-1]`), so + // `tokens[len-2]` is the token before it; a leading `.` marks member access. + if (RESTRICTED_STATEMENT_KEYWORDS.has(previous.v)) { + const before = tokens[tokens.length - 2]; + return !(before?.t === 'punct' && before.v === '.'); + } + // A *label* completing a `break`/`continue` statement (`break outer`, + // `continue outer`) also ends a restricted statement, so a following `/` + // opens a regex, not division. The label id was marked `restrictedLabel` + // when it was emitted directly after a bare keyword with no intervening + // LineTerminator; a newline there instead begins a fresh statement (an + // ordinary id) whose following `/` stays division (D3-CR-BREAK-CONTINUE- + // LABEL-ASI). + if (previous.restrictedLabel === true) return true; + return false; case 'num': case 'str': case 'regex': @@ -195,9 +247,23 @@ function tokenizeEsm(source: string): readonly EsmToken[] { if (beforePrevious?.t === 'punct' && beforePrevious.v === '.') return false; return true; }; + // Whether an identifier scanned *now* is the optional label of a just-emitted + // bare `break`/`continue` (`break outer`). True only when the keyword is the + // immediately previous token, is bare (not a member — guard the token before it), + // and no LineTerminator separated them (`pendingNewline`). Read inside this arrow + // so `previous` narrows over its declared `EsmToken | null` type, as in + // `regexCanFollow`/`previousOpensControlHeader` (D3-CR-BREAK-CONTINUE-LABEL-ASI). + const previousIsBareRestrictedKeyword = (): boolean => { + if (pendingNewline) return false; + if (previous === null || previous.t !== 'id') return false; + if (!RESTRICTED_STATEMENT_KEYWORDS.has(previous.v)) return false; + const beforeKeyword = tokens[tokens.length - 2]; + return !(beforeKeyword?.t === 'punct' && beforeKeyword.v === '.'); + }; const emit = (token: EsmToken): void => { tokens.push(token); previous = token; + pendingNewline = false; }; // Scan a template literal's *text* run starting at `from` (the character just @@ -235,6 +301,7 @@ function tokenizeEsm(source: string): readonly EsmToken[] { // insignificant whitespace if (c === ' ' || c === '\t' || c === '\r' || c === '\n') { + if (c === '\n' || c === '\r') pendingNewline = true; index += 1; continue; } @@ -244,10 +311,15 @@ function tokenizeEsm(source: string): readonly EsmToken[] { while (index < length && source.charAt(index) !== '\n') index += 1; continue; } - // block comment — trivia, consumed whole as one unit (may span lines) + // block comment — trivia, consumed whole as one unit (may span lines). A + // LineTerminator *inside* the comment still counts as one between the tokens it + // separates, so it is recorded like ordinary-whitespace newline for the + // restricted-statement label check (`break /*\n*/ outer` is ASI, not a label). if (c === '/' && source.charAt(index + 1) === '*') { index += 2; while (index < length && !(source.charAt(index) === '*' && source.charAt(index + 1) === '/')) { + const inner = source.charAt(index); + if (inner === '\n' || inner === '\r') pendingNewline = true; index += 1; } index += 2; @@ -373,7 +445,17 @@ function tokenizeEsm(source: string): readonly EsmToken[] { if (isIdentifierStart(c)) { let cursor = index + 1; while (cursor < length && isIdentifierPart(source.charAt(cursor))) cursor += 1; - emit({ t: 'id', v: source.slice(index, cursor) }); + // A bare `break`/`continue` immediately followed — with no intervening + // LineTerminator — by this identifier makes it the statement's optional + // *label* (`break outer`). Mark it so a `/` after the label opens a regex + // (the restricted statement is complete), mirroring the bare-keyword case. + // `pendingNewline` guards the `[no LineTerminator here]` restriction: a + // newline between the keyword and the id triggers ASI, so the id is a fresh + // statement, not a label. The keyword must be *bare*, not a member + // (`obj.break`), so guard on the token before it (`tokens[len-2]`, since + // `previous`/`tokens[len-1]` is the keyword) — D3-CR-BREAK-CONTINUE-LABEL-ASI. + const restrictedLabel = previousIsBareRestrictedKeyword(); + emit({ t: 'id', v: source.slice(index, cursor), restrictedLabel }); index = cursor; continue; } @@ -1583,3 +1665,302 @@ describe('D3 host import scanner recognizes `for await (…)` as a control heade expect(performance.now() - start).toBeLessThan(1000); }); }); + +describe('D3 host import scanner classifies `/` after a restricted-statement keyword as a regex (D3-CR-BREAK-CONTINUE-ASI)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // `break`/`continue` are restricted productions — a `[no LineTerminator here]` + // precedes the optional label — so a newline after the bare keyword triggers ASI + // and the next line begins a fresh statement whose first token may be a regex + // literal. Before this fix the keyword was read as an ordinary value-ending + // identifier, so `regexCanFollow()` returned false and the leading `/` was treated + // as division. A quote inside the regex then opened a spurious string that could + // swallow a following real import (false negative), and an `import('…')` in the + // regex body could be tokenized as code (false positive). The `/` is now a regex + // opener after a *bare* `break`/`continue`; a `.`-member form stays division. + + // False positive: an `import('…')` buried in a regex body after a bare + // `break`/`continue` + newline must NOT surface (this was the reproduced defect — + // the exact newline fixtures A/B returned the right set only accidentally, because + // a single-quoted string dies at the line end, but C/D fabricated `evil`). + it('does not fabricate a module from an import call in a regex after `break` + newline', () => { + const source = ['while (ok) {', ' break', " /import('../domain/evil.js')/.test(x);", '}'].join( + '\n', + ); + expect(extractModuleSpecifiers(source)).toEqual([]); + expect(forbiddenIn(source)).toEqual([]); + }); + + it('does not fabricate a module from an import call in a regex after `continue` + newline', () => { + const source = ['while (ok) {', ' continue', " /import('../domain/evil.js')/.test(x);", '}'].join( + '\n', + ); + expect(extractModuleSpecifiers(source)).toEqual([]); + expect(forbiddenIn(source)).toEqual([]); + }); + + // False negative: a quote-bearing regex after the keyword must not swallow a + // later real import. The exact Codex newline fixtures (import on its own line) + // pass either way — the load-bearing form places the import on the regex's line, + // where the old spurious string ran straight through it. + it('surfaces a real import after a `break`-newline quote-bearing regex (same line)', () => { + const source = "while (ok) { break\n/[']/.test(x); import '../domain/foo.js'; }"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + it('surfaces a real import after a `continue`-newline quote-bearing regex (same line)', () => { + const source = "while (ok) { continue\n/[']/.test(x); import '../domain/foo.js'; }"; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + expect(forbiddenIn(source).length).toBeGreaterThan(0); + }); + + // The exact Codex fixtures (A/B): import on a separate line — must stay correct. + it('surfaces a real import on a separate line after break/continue + newline regex', () => { + for (const kw of ['break', 'continue']) { + const source = ['while (ok) {', ` ${kw}`, " /[']/.test(x);", '}', "import '../domain/foo.js';"].join( + '\n', + ); + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + } + }); + + // Regex-vs-division context matrix after the keyword: a leading `/` is a regex, + // so a trailing real import always survives (no spurious string ran on). + it('recognizes a regex after break/continue across newline, CRLF, and comment separators', () => { + const bodies: readonly string[] = [ + 'break\n/[\']/.test(x);', + 'continue\n/[\']/.test(x);', + 'break\r\n/[\']/.test(x);', + 'continue\r\n/[\']/.test(x);', + 'break /* c */\n/[\']/.test(x);', + 'continue /* c */\n/[\']/.test(x);', + 'break // c\n/[\']/.test(x);', + 'continue // c\n/[\']/.test(x);', + ]; + for (const body of bodies) { + const source = `while (ok) { ${body} }\nimport '../domain/foo.js';`; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + } + }); + + // A regex body holding an `import('…')` must not fabricate a dependency across any + // of those same separators. + it('never fabricates a module from an import-bearing regex across separators', () => { + const bodies: readonly string[] = [ + "break\n/import('../domain/evil.js')/.test(x);", + "continue\n/import('../domain/evil.js')/.test(x);", + "break\r\n/import('../domain/evil.js')/.test(x);", + "continue /* c */\n/import('../domain/evil.js')/.test(x);", + "break // c\n/import('../domain/evil.js')/.test(x);", + ]; + for (const body of bodies) { + expect(extractModuleSpecifiers(`while (ok) { ${body} }`)).toEqual([]); + } + }); + + // Member guard: a control/restricted keyword spelled as a *member name* is a + // value, so a following `/` is division, not a regex — otherwise the fake regex + // would swallow the later real import (false negative). Covers `.` and `?.`. + it('keeps division after a `break`/`continue` used as a member name', () => { + for (const access of ['obj.break', 'obj.continue', 'obj?.break', 'obj?.continue']) { + const source = `const r = ${access} / 2; import '../domain/foo.js';`; + expect(extractModuleSpecifiers(source)).toEqual(['../domain/foo.js']); + expect(extractModuleSpecifiers(`const r = ${access} / 2;\nimport '../domain/foo.js';`)).toEqual([ + '../domain/foo.js', + ]); + } + }); + + it('fabricates no module from a bare `obj.break` / `obj.continue` division', () => { + expect(extractModuleSpecifiers('const r = obj.break / 2;')).toEqual([]); + expect(extractModuleSpecifiers('const r = obj.continue / 2;')).toEqual([]); + }); + + // The `!` lookback must still read `obj.break!` as a non-null assertion (value), + // so the following `/` is division — `break`/`continue` stay value-ending in + // `endsValue`, unlike REGEX_CONTEXT_KEYWORDS. + it('keeps division after a non-null member assertion `obj.break!`', () => { + expect(extractModuleSpecifiers("const r = obj.break! / 2; import '../domain/foo.js';")).toEqual([ + '../domain/foo.js', + ]); + expect(extractModuleSpecifiers('const r = obj.break! / 2;')).toEqual([]); + }); + + // Preservation: a labelled `break`/`continue`, an explicit-semicolon form, and the + // untouched `return`/`throw` restricted keywords all keep working. + it('preserves labelled, explicit-semicolon, and return/throw forms', () => { + expect( + extractModuleSpecifiers("outer: while (ok) { break outer; } import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("outer: while (ok) { continue outer; } import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("while (ok) { break;\n/[']/.test(x); } import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + expect( + extractModuleSpecifiers("function f() { return\n/[']/.test(v); } import '../domain/foo.js';"), + ).toEqual(['../domain/foo.js']); + }); + + // Liveness: many restricted-statement regex lines scan in bounded linear time; a + // regression to rescanning would blow vitest's per-test timeout. + it('scans many `break`-newline regex statements in bounded, linear time', () => { + const many = "while (ok) { break\n/[']/.test(x); }\n".repeat(2000) + "import '../domain/foo.js';"; + const start = performance.now(); + expect(extractModuleSpecifiers(many)).toEqual(['../domain/foo.js']); + expect(performance.now() - start).toBeLessThan(1000); + }); +}); + +describe('D3 host import scanner classifies `/` after a labelled restricted statement as a regex (D3-CR-BREAK-CONTINUE-LABEL-ASI)', () => { + const forbiddenIn = (source: string): readonly string[] => + extractModuleSpecifiers(source).filter((s) => /\.\.\/(?:domain|adapters)\//.test(s)); + + // `break outer` / `continue outer` carry the statement's optional label on the + // *same* line as the keyword; the statement is then complete, so a `/` beginning + // the next line is a regex opener, not division. The token immediately before that + // `/` is the *label* id (not the keyword), so the bare-keyword guard alone missed + // it: before this fix `regexCanFollow()` read the label as an ordinary value and + // classified the `/` as division. An `import('…')` in the regex body then + // fabricated a dependency (false positive) and a quote-bearing regex could swallow + // a following same-line import (false negative). The label is now marked + // `restrictedLabel` when it directly follows a bare `break`/`continue` with no + // intervening LineTerminator; a newline in between is ASI, leaving the id an + // ordinary fresh statement whose `/` stays division. + + // False positive: an `import('…')` in a regex body after `break

`; diff --git a/tests/cockpit-host/render.test.ts b/tests/cockpit-host/render.test.ts index b28db1e..5bddc76 100644 --- a/tests/cockpit-host/render.test.ts +++ b/tests/cockpit-host/render.test.ts @@ -115,9 +115,11 @@ describe('Cockpit D3 finding binding (D3-CODEX-F1)', () => { return row as string; }; - it('labels PR, Reviewed commit, and Advisory freshness as distinct columns', () => { + it('labels finding identity and reviewer attribution as distinct columns', () => { expect(headerRow).toContain('PR'); expect(headerRow).toContain('Reviewed commit'); + expect(headerRow).toContain('Provider'); + expect(headerRow).toContain('Reviewer'); expect(headerRow).toContain('Advisory freshness'); // "Reviewed commit" is the load-bearing label: it must not collapse to a // bare "Commit", which would blur it against the repository Observed HEAD @@ -143,6 +145,20 @@ describe('Cockpit D3 finding binding (D3-CODEX-F1)', () => { expect(row).toContain(HEAD_SHA); }); + it('attributes claims from different providers to their exact reviewers', () => { + const claudeRow = rowFor('f-001'); + expect(claudeRow).toContain('claude'); + expect(claudeRow).toContain('claude-review-bot'); + + const codexRow = rowFor('f-002'); + expect(codexRow).toContain('codex'); + expect(codexRow).toContain('codex-review-bot'); + + const coderabbitRow = rowFor('f-004'); + expect(coderabbitRow).toContain('coderabbit'); + expect(coderabbitRow).toContain('coderabbit-bot'); + }); + it('keeps two same-PR findings distinguishable by reviewed commit', () => { // f-001 and f-002 are both pr-42 but were reviewed against different // commits; the reviewed-commit binding, not advisory freshness, is what