From ce96363e67ccee218c14f351556b264dfca4a0a3 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 19:43:51 +0200 Subject: [PATCH 1/3] fix(#643): replace regex comment stripping with parser-backed architecture source contracts tests/unit/side-panel-source-contract.test.ts and tests/unit/surface-lifecycle-arch.test.ts preprocessed source with a two-pass regex comment stripper before applying their own textual assertions. The block-comment pass ran before line-comment removal, so a `/*`-shaped substring sitting inside a real `//` comment could make the block pass consume real code through the next genuine `*/`, hiding a real violation before either suite's assertions ever ran. Both suites now call two new named analyzers in build/lib/check-legacy-owners.mjs (findSidePanelSourceContractViolations / findSurfaceLifecycleSourceContractViolations), backed by the same real- TypeScript-parser infrastructure the #630/#642 checks already use. The module's single-source `withParsedSource` is refactored into a thin wrapper over a new internal `withParsedSources` batch primitive, so the surface scan shares one native parser process across the whole `src/**` tree (measured ~250-310ms for 221 files) instead of spawning one per file. A new build/lib/check-legacy-owners.d.mts gives the two strict-.ts test files a plain-data declaration boundary with no SourceFile/Node/SyntaxKind crossing it. Several rules deliberately gained precision along the way: the exact-value panel-id/label checks no longer false-positive on a longer literal merely containing a protected id, while gaining multi-quote-style coverage (including type-position literals, e.g. `type Pref = 'library'`) for the actual protected value; the history/sidePanel.value comparison rules support both operand orders; the surface ordering scopes now recognize return-annotated function declarations (a real gap in the retired textual opener) while explicitly preserving its accidental treatment of parenthesized control-flow blocks as independent ordering scopes; and `currentWorkspace = null ?? fallback` is now deliberately treated as clean. Verified the real src/** tree produces zero findings under the new analyzers, and manually confirmed both analyzers go red against temporarily-sabotaged real production files (workbench-session.ts, app.ts), then restored the originals from saved bytes. No production src/** code, dependency, or runtime behavior changed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- CHANGELOG.md | 30 + build/lib/check-legacy-owners.d.mts | 97 ++ build/lib/check-legacy-owners.mjs | 911 +++++++++++++++++- tests/unit/side-panel-source-contract.test.ts | 378 ++++++-- tests/unit/surface-lifecycle-arch.test.ts | 501 +++++++--- 5 files changed, 1668 insertions(+), 249 deletions(-) create mode 100644 build/lib/check-legacy-owners.d.mts diff --git a/CHANGELOG.md b/CHANGELOG.md index 800f1a48..c1256e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -466,6 +466,36 @@ auto-generated per-PR notes; this file is the curated, human-readable history. template, concatenation, conditional, or otherwise) is an unconditional violation. No product/runtime behavior changed — this hardens the architecture gate's own soundness, not the policy it enforces. +- **#643: `tests/unit/side-panel-source-contract.test.ts` and + `tests/unit/surface-lifecycle-arch.test.ts` now use real TypeScript syntax + instead of unsafe comment preprocessing.** Both suites used to run a + two-pass regex comment stripper (block comments removed before line + comments) ahead of their own textual assertions — unsound in the direction + that matters most for an architecture guard: a `/*`-shaped substring + sitting inside a real `//` comment could make the block-comment pass + consume real code through the next genuine `*/`, hiding a violation before + either suite's assertions ever ran. Both now call two new named analyzers + in `build/lib/check-legacy-owners.mjs` (`findSidePanelSourceContractViolations`/ + `findSurfaceLifecycleSourceContractViolations`, backed by the same shared + real-TypeScript-parser infrastructure #630/#642 already use, extended with + an internal `withParsedSources` batch primitive so the whole-tree surface + scan shares one native parser process rather than spawning one per file) + and a new `.d.mts` plain-data declaration boundary. Several rules + deliberately gained precision along the way (documented in each rule's own + test comments): the exact-value panel-id/label checks (app-preferences.ts, + state.ts, app-shell.ts panel ids) no longer false-positive on a longer + literal merely containing a protected id (`"pick 'library' now"` stays + clean) while gaining multi-quote-style coverage for the actual protected + value, including a TYPE-position literal (`type Pref = 'library'`); the + `history`/`sidePanel.value` comparison rules now support both operand + orders; the surface suite's ordering scopes now also recognize + return-annotated function declarations (a real gap in the retired textual + opener) while explicitly preserving its accidental treatment of + parenthesized control-flow blocks (`if`/`for`/`while`/`switch`/`catch (e)`) + as independent ordering scopes; and `currentWorkspace = null ?? fallback` + is now deliberately treated as clean (a `??` introduces real + conditional/fallback semantics a bare null-equivalent write does not have). + No production `src/**` code, dependency, or runtime behavior changed. ## [0.7.3] - 2026-08-06 diff --git a/build/lib/check-legacy-owners.d.mts b/build/lib/check-legacy-owners.d.mts new file mode 100644 index 00000000..249e5215 --- /dev/null +++ b/build/lib/check-legacy-owners.d.mts @@ -0,0 +1,97 @@ +// Issue #643 — the strict-`.ts` declaration boundary over +// `check-legacy-owners.mjs`'s two source-contract analyzers +// (`findSidePanelSourceContractViolations` / +// `findSurfaceLifecycleSourceContractViolations`), consumed by +// `tests/unit/side-panel-source-contract.test.ts` and +// `tests/unit/surface-lifecycle-arch.test.ts`. Deliberately declares ONLY +// plain-data APIs — no `SourceFile`, no compiler `Node`, no `SyntaxKind`, no +// untyped callback parameter ever crosses this boundary, so a strict `.ts` +// caller never needs `any`/`@ts-ignore`/`@ts-expect-error` to consume it. +// `npm run check:types` proves declaration resolution and caller +// conformance; it does NOT prove this file accurately models the runtime +// `.mjs` shape — the two test files' own synthetic-source assertions are +// what actually exercise the real exports and validate the returned DTOs. +// +// This file intentionally does not declare every export the `.mjs` module +// has (the #630/#642 legacy-owner/package helpers, `manifestDependencyFields` +// et al.) — only the #643 side-panel/surface-lifecycle surface strict `.ts` +// callers need. Every other consumer of this module stays plain `.js`/`.mjs` +// (checkJs:false), so this declaration file never needs to describe them. + +/** The #587 AC5 side-panel source-contract rule codes + * `findSidePanelSourceContractViolations` may report. */ +export type SidePanelRule = + | 'workbench-sidepanel-mention' + | 'workbench-history-compare' + | 'app-preferences-panel-id' + | 'state-panel-label' + | 'app-side-panel-comparison' + | 'app-shell-panel-def' + | 'app-shell-panel-id' + | 'app-shell-host-accessor' + | 'side-panels-type-alias'; + +/** The #590 invariant (k) surface-lifecycle source-contract rule codes + * `findSurfaceLifecycleSourceContractViolations` may report. */ +export type SurfaceLifecycleRule = + | 'surface-protected-declaration' + | 'surface-teardown-call' + | 'surface-signal-write' + | 'surface-current-workspace-null' + | 'surface-retirement-ordering'; + +/** One reported source-contract violation — a plain, JSON-serializable DTO. + * `pos` is the offending AST node's own `getStart(sourceFile)` (or `0` for a + * whole-file "the required construct is entirely absent" finding, which + * names no single node): a stable, deterministic identity, never a + * line/column and never required in a user-facing diagnostic. */ +export interface SourceContractViolation { + readonly rule: SidePanelRule | SurfaceLifecycleRule; + readonly filename: string; + readonly pos: number; + readonly detail: string; +} + +/** + * The #587 AC5 side-panel source contract, real-TypeScript-parser-backed. + * `filename` selects which (if any) rule group applies; a `filename` this + * function does not recognize returns `[]` without parsing `source` at all. + * Callers may pass either a real guarded file's current contents (with its + * real repo-relative `filename`) or synthetic probe source under the SAME + * `filename` to exercise that file's specific rule(s) in isolation. + */ +export function findSidePanelSourceContractViolations( + source: string, + filename: string, +): SourceContractViolation[]; + +/** One (filename, raw source) entry in a surface-lifecycle batch — `source` + * is the file's complete, unmodified text (comments included; nothing is + * stripped or reconstructed before parsing). */ +export interface SurfaceLifecycleSourceEntry { + readonly filename: string; + readonly source: string; +} + +/** `appFile` must be one of `sources`' own `filename` values. + * `coordinatorStart`/`coordinatorEnd` are the raw byte offsets of the + * `#590-COORDINATOR-BEGIN`/`#590-COORDINATOR-END` marker comments in + * `appFile`'s OWN raw source (the caller locates them there directly — the + * markers are themselves `//` comments, so they intentionally stay outside + * this function's AST-based analysis). */ +export interface SurfaceLifecycleOptions { + readonly appFile: string; + readonly coordinatorStart: number; + readonly coordinatorEnd: number; +} + +/** + * The #590 invariant (k) surface-lifecycle source contract, real- + * TypeScript-parser-backed, over one shared parser batch for the complete + * `sources` set (never one parse per file). Throws if `options.appFile` is + * not among `sources`' filenames. + */ +export function findSurfaceLifecycleSourceContractViolations( + sources: readonly SurfaceLifecycleSourceEntry[], + options: SurfaceLifecycleOptions, +): SourceContractViolation[]; diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index c5a7e1ff..011a4ea5 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -22,19 +22,28 @@ // build/test tooling only and must never be imported by `src/**` or // `packages/clickhouse-http/**` runtime code. // -// Scope stays deliberately narrow (this is NOT a generic static-analysis -// framework): exactly the former production owners of moved -// progress-stream/exception-parsing/quoting primitives, and exactly the -// identifier/property names each phase moved into `@altinity/clickhouse-http` -// (plus, for Rule D, exactly which of the package's OWN export names are -// pure-language vs. transport/protocol). An AST walk flags any Identifier -// with a moved name — a declaration, an import/export specifier, a member -// reference — and any string-literal property/member name (`{ "streamLines": -// … }`), so a second implementation and a forwarding wrapper both fail. -// Intentionally obfuscated constructs (computed strings, dynamically built -// property names) are outside this check's threat model. Comments and JSDoc -// are trivia to the parser, so prose narrating the move can never -// false-positive. +// Scope (post-#643): a shared, narrowly-scoped real-TypeScript-parser +// architecture-source utility module — NOT a generic static-analysis +// framework. It originated with #630's legacy-owner/package-boundary checks +// (below) and, since #643, also hosts two further explicit, named +// source-contract analyzers: `findSidePanelSourceContractViolations` (the +// #587 side-panel registry contract) and +// `findSurfaceLifecycleSourceContractViolations` (the #590 surface-retirement +// coordinator contract). Every exported analyzer in this module owns one +// named, bounded architecture contract; none of them generalizes into a +// vocabulary any caller can extend ad hoc. The paragraph below — computed +// strings/dynamically built property names sitting outside this check's +// threat model — describes `findNamedIdentifierViolations`'s own contract +// specifically (the #630 owner/name-list checks and the thin wrappers over +// it), not a blanket statement about every analyzer this module exports: the +// #643 analyzers' own doc comments state their own (different, narrower or +// broader as appropriate) obfuscation boundaries. +// +// An AST walk flags any Identifier with a moved name — a declaration, an +// import/export specifier, a member reference — and any string-literal +// property/member name (`{ "streamLines": … }`), so a second implementation +// and a forwarding wrapper both fail. Comments and JSDoc are trivia to the +// parser, so prose narrating the move can never false-positive. // // Issue #630 Phase 5 — generalized into a small shared AST utility (plan // §8.3): the Phase 3 owner/name-list check below is now a thin wrapper over @@ -144,32 +153,68 @@ export const PHASE8_NARROW_RULE_D_EXCEPTIONS = Object.freeze({ // ── Shared real-parser plumbing ───────────────────────────────────────────── -// Parse `source` (claiming to be the repo-relative `filename`) with the real -// TypeScript parser and hand back its root AST node. Always used inside a -// try/finally that calls `api.close()` — the native child process must -// always be reaped, on every return path including a thrown parse failure. -function withParsedSource(source, filename, fn) { - // The virtual path keeps the real basename so the parser applies the right - // grammar for the file's extension (.ts here; never .tsx among any of the - // owner files or import-usage callers below). - const virtualPath = `/legacy-owner-check/${path.posix.basename(filename)}`; - const api = new API({ fs: createVirtualFileSystem({ [virtualPath]: source }) }); +// Issue #643 — one real parser session per CALL, not per FILE. Every prior +// caller of `withParsedSource` (the #630/#642 helpers above/below) parses +// exactly one source at a time, so spawning one native `tsc` child process +// per call was never wasteful for THEM. #643's own surface-lifecycle +// analyzer instead needs one violation pass over the ENTIRE scanned +// `src/**` tree (a hundred-plus files) — one process per file there would +// multiply this module's own documented startup cost by the file count, the +// exact CI-timeout-pressure shape `mightReferencePackage`'s/ +// `mightReferenceForbiddenRelativeDir`'s own doc comments already warn +// about for a *parse* (not just a pre-filter) granularity. `withParsedSources` +// is the batch primitive both cases now share: one `API`/one virtual +// filesystem for an arbitrary number of (source, filename) entries, all +// recovered from the SAME parsed snapshot. `withParsedSource` becomes a +// one-entry compatibility wrapper over it — every existing #630/#642 caller +// keeps its original 3-argument call shape and `fn(sourceFile)` callback +// unchanged. +// +// Virtual paths are the entry's own repo-relative path (forward-slash, +// preserving its real extension so the parser selects the right grammar), +// rooted under `/legacy-owner-check/` — collision-proof by construction: two +// distinct real (or synthetic-but-caller-distinct) repo-relative paths can +// never collide, unlike the single-file wrapper's now-removed +// basename-only scheme, which relied on there only ever being one entry to +// namespace at all. +function withParsedSources(entries, fn) { + const virtualPaths = new Map(); // filename -> virtualPath + const files = {}; + for (const { source, filename } of entries) { + const virtualPath = path.posix.join('/legacy-owner-check', filename); + virtualPaths.set(filename, virtualPath); + files[virtualPath] = source; + } + const api = new API({ fs: createVirtualFileSystem(files) }); try { - const snapshot = api.updateSnapshot({ openFiles: [virtualPath] }); - const sourceFile = snapshot - .getDefaultProjectForFile(virtualPath) - ?.program.getSourceFile(virtualPath); - if (!sourceFile) { - // Fail loud, never silently-clean: an unparseable probe must not read - // as "no violations". - throw new Error(`check-legacy-owners: could not parse ${filename}`); + const snapshot = api.updateSnapshot({ openFiles: [...virtualPaths.values()] }); + const sourceFiles = new Map(); // filename -> SourceFile + for (const [filename, virtualPath] of virtualPaths) { + const sourceFile = snapshot + .getDefaultProjectForFile(virtualPath) + ?.program.getSourceFile(virtualPath); + if (!sourceFile) { + // Fail loud, never silently-clean: an unparseable probe must not read + // as "no violations". + throw new Error(`check-legacy-owners: could not parse ${filename}`); + } + sourceFiles.set(filename, sourceFile); } - return fn(sourceFile); + return fn(sourceFiles); } finally { - api.close(); // always reap the native child process + api.close(); // always reap the native child process, on every return path } } +// Parse `source` (claiming to be the repo-relative `filename`) with the real +// TypeScript parser and hand back its root AST node — the original #630 +// single-source entry point, now a thin one-entry wrapper over +// `withParsedSources` above. Every existing caller's behavior (including the +// thrown-on-unparseable-source contract) is unchanged. +function withParsedSource(source, filename, fn) { + return withParsedSources([{ source, filename }], (sourceFiles) => fn(sourceFiles.get(filename))); +} + /** * Parse `source` and return which of `movedNames` it declares or references * — a declaration, an import/export specifier, a member reference, or a @@ -1085,3 +1130,803 @@ export function lockHasPackage(lock, specifier) { export function retiredClientSpikeScriptNames(scripts) { return Object.keys(scripts ?? {}).filter((s) => s === 'check:client-spike:evidence' || s.startsWith('test:client-spike')); } + +// ── Issue #643 — parser-backed side-panel / surface-lifecycle source +// contracts ─────────────────────────────────────────────────────────────── +// +// `tests/unit/side-panel-source-contract.test.ts` and +// `tests/unit/surface-lifecycle-arch.test.ts` used to preprocess source with +// a hand-rolled two-pass regex comment stripper +// (`/\*[\s\S]*?\*\//g` then `/(^|[^:"'`])\/\/.*$/gm`) before applying their +// own textual assertions. That stripper is unsound in the direction that +// matters most for an architecture GUARD: the block-comment pass runs +// BEFORE line-comment removal, so a `/*`-shaped substring sitting inside a +// real `//` comment (e.g. `// documentation mentioning src/core/**`) can +// make the block pass consume every real line of code up to the next +// genuine `*/`, deleting a real violation before either test's assertions +// ever see it — exactly backwards for a check whose entire job is to catch +// code a reviewer might miss. Below replaces both stripping/scanning +// implementations with real-TypeScript-parser-backed analyzers, sharing the +// same `withParsedSources`/`withParsedSource` plumbing the #630/#642 checks +// above already use — comments, strings, template literals, and +// regex-vs-division are resolved by the actual grammar, so none of the +// stripper's lexical-bypass shapes is even representable in the AST these +// analyzers walk. +// +// `findSidePanelSourceContractViolations` and +// `findSurfaceLifecycleSourceContractViolations` are the two public +// entrypoints (mirroring the `SidePanelRule`/`SurfaceLifecycleRule` unions +// `build/lib/check-legacy-owners.d.mts` declares); every other export in +// this section is an internal building block composed differently by each +// rule — per this module's own stated policy, they are NOT collapsed into +// one generic vocabulary matcher, because the six side-panel rule groups and +// the five surface-lifecycle rule groups each have genuinely different +// literal/structural semantics (exact-value literal comparison vs. broad +// contiguous-substring detection vs. structural chain/call-shape matching). + +/** Every AST node kind whose `.text` is a real decoded source string this + * module's broad-substring/exact-value checks may safely inspect. This is a + * DELIBERATE allowlist, not "every node with a `.text` field": `SourceFile` + * itself also carries a `.text` property (the file's entire raw content, + * comments included) — checking it unconditionally would silently + * reintroduce exactly the "matches inside a comment" defect this whole + * migration exists to close, since it sits above and outside the parser's + * own trivia/AST distinction. Restricting to these leaf literal/identifier + * kinds means every match found this way is provably a real code token, not + * raw file text. */ +const TEXTUAL_LEAF_KINDS = new Set([ + SyntaxKind.Identifier, + SyntaxKind.PrivateIdentifier, + SyntaxKind.StringLiteral, + SyntaxKind.NoSubstitutionTemplateLiteral, + SyntaxKind.RegularExpressionLiteral, + SyntaxKind.TemplateHead, + SyntaxKind.TemplateMiddle, + SyntaxKind.TemplateTail, +]); + +/** The two literal kinds a "complete parser value equals X" exact-match rule + * ever accepts — a plain string literal or a no-substitution template + * literal (`` `library` ``), matching every sibling exact-value check + * elsewhere in this module (e.g. `findNamedIdentifierViolations`'s own + * quoted-property-name arm). A substitution template (`` `${x}` ``) is + * never one node with one decoded `.text` — it has no single "complete + * value" a parser can hand back — so it is correctly never eligible here. */ +const EXACT_LITERAL_KINDS = new Set([SyntaxKind.StringLiteral, SyntaxKind.NoSubstitutionTemplateLiteral]); + +/** Depth-first pre-order walk of `root` and every descendant, calling + * `visit(node)` once per node (including `root` itself). Deliberately + * unconditional — `visit`'s return value never prunes recursion — because + * every #643 rule below wants "the whole subtree", never "the whole + * subtree except nodes underneath the first match": e.g. the ordering + * rule's own nested-scope requirement (a retirement call visible to BOTH + * its own enclosing scope and every scope that contains it) depends on + * this never stopping early. */ +function walkTree(root, visit) { + const step = (node) => { + visit(node); + node.forEachChild(step); + }; + step(root); +} + +/** True when `node` is one of `EXACT_LITERAL_KINDS` and its complete decoded + * value is exactly one of `targets` (a `Set`). Because this checks + * the AST NODE KIND, not the node's syntactic position, it identically + * matches an expression-position literal (`const id = "library"`) and a + * type-position one (`type Pref = 'library'`, a `StringLiteral` sitting + * inside a `LiteralTypeNode`) — a plain unscoped tree walk reaches both, so + * none of the exact-value rules below need (or have) a separate + * type-position branch: restricting a walk to "expression-context nodes + * only" is precisely the narrowing this helper's callers must avoid. */ +function exactLiteralMatch(node, targets) { + return !!node && EXACT_LITERAL_KINDS.has(node.kind) && targets.has(node.text); +} + +/** True when `node` is a plain (non-private) identifier whose complete text + * is exactly one of `targets`. Matches a declaration, a reference, a + * property-access `.name`, or a destructuring binding's `.name` identically + * — they are all the same AST node kind to this check, by construction of + * a blanket tree walk. */ +function exactIdentifierMatch(node, targets) { + return !!node + && (node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.PrivateIdentifier) + && targets.has(node.text); +} + +/** + * The last (innermost-to-outermost, i.e. rightmost-in-source) `count` + * identifier names of a property-access chain ending at `expr` — e.g. for + * `app.shell.sidePanel.value`, `terminalNames(expr, 2)` returns + * `['sidePanel', 'value']` regardless of how many segments precede them. + * Stops (returning fewer than `count` names) the moment the chain hits + * anything other than a `PropertyAccessExpression` or a terminal + * `Identifier`/`PrivateIdentifier` — an `ElementAccessExpression`, a call, a + * parenthesized expression, etc. — so a computed/dynamic segment anywhere in + * the chain correctly makes the match fail rather than guessing past it. + * Shared by every #643 rule that cares only about a chain's OWN terminal + * segments, not its full length or receiver (member-terminal matching): + * teardown calls, the private-signal `.value` write rule, and the + * `app.ts` side-panel comparison rule. + * + * @param {object} expr + * @param {number} count + * @returns {string[]} + */ +function terminalNames(expr, count) { + const names = []; + let current = expr; + while (current && names.length < count) { + if (current.kind === SyntaxKind.PropertyAccessExpression) { + names.unshift(current.name.text); + current = current.expression; + } else if (current.kind === SyntaxKind.Identifier || current.kind === SyntaxKind.PrivateIdentifier) { + names.unshift(current.text); + current = null; + } else { + current = null; + } + } + return names; +} + +/** Unwrap every transparent cast/assertion wrapper the plan names around a + * `currentWorkspace = null` RHS — `ParenthesizedExpression`, `AsExpression` + * (`null as never`), `SatisfiesExpression` (`null satisfies never`), + * `NonNullExpression` (`null!`), and `TypeAssertionExpression` + * (`null`) — returning the innermost expression a cast-bypassing + * write can never hide behind. Deliberately does NOT unwrap a + * `BinaryExpression` (`null ?? fallback`): that operator introduces genuine + * conditional/fallback semantics, so the actual invariant this rule + * enforces — "this property was set to a bare null-equivalent value" — no + * longer holds once a `??` is present, and treating it as transparent would + * be a correctness bug, not extra coverage. */ +function unwrapNullEquivalentWrappers(expr) { + let current = expr; + while (current) { + if ( + current.kind === SyntaxKind.ParenthesizedExpression + || current.kind === SyntaxKind.AsExpression + || current.kind === SyntaxKind.SatisfiesExpression + || current.kind === SyntaxKind.NonNullExpression + || current.kind === SyntaxKind.TypeAssertionExpression + ) { + current = current.expression; + continue; + } + break; + } + return current; +} + +/** One violation both #643 analyzers report — matches + * `build/lib/check-legacy-owners.d.mts`'s `SourceContractViolation` DTO + * exactly (a plain-data shape, no `SourceFile`/`Node` ever crosses this + * boundary). `pos` is the offending node's own `getStart(sourceFile)` (or + * `0` for a whole-file "required construct is entirely absent" finding, + * which names no single node) — a stable, deterministic identity, not a + * line/column. */ +function makeViolation(rule, filename, pos, detail) { + return { rule, filename, pos, detail }; +} + +// ── Side-panel source contract (#587 AC5 regression backstop) ────────────── + +const WORKBENCH_SESSION_FILE = 'src/ui/workbench/workbench-session.ts'; +const APP_PREFERENCES_FILE = 'src/application/app-preferences.ts'; +const STATE_FILE = 'src/state.ts'; +const APP_FILE = 'src/ui/app.ts'; +const APP_SHELL_FILE = 'src/ui/app-shell.ts'; +const SIDE_PANELS_CORE_FILE = 'src/core/side-panels.ts'; + +/** #276/#587 — `app-preferences.ts` may never hard-code one of the registry's + * own panel-id literals; its `sidePanel` preference stays typed as + * `SidePanelKey`, derived from the manifest. */ +export const SIDE_PANEL_APP_PREFERENCES_IDS = Object.freeze(['library', 'databases', 'dashboards']); +/** #587 — `state.ts` may never hard-code one of the registry's own display + * labels; labels belong to the registry, not the state model. */ +export const SIDE_PANEL_STATE_LABELS = Object.freeze(['Databases', 'Dashboards', 'Library', 'History']); +/** #587/#600 — `app-shell.ts` may never hard-code one of the registry's own + * panel ids as a literal. */ +export const SIDE_PANEL_APP_SHELL_IDS = Object.freeze(['databases', 'dashboards', 'library', 'history']); +/** #600 — `app-shell.ts` may never name one of the four concrete panel-def + * symbols the registry composes instead. */ +export const SIDE_PANEL_APP_SHELL_DEFS = Object.freeze([ + 'databasesPanelDef', 'dashboardsPanelDef', 'libraryPanelDef', 'historyPanelDef', +]); +/** #600 (round 2) — `app-shell.ts` may never name one of the two concrete + * upper-pane host accessors the registry's own `entries` should supply + * instead. */ +export const SIDE_PANEL_APP_SHELL_HOSTS = Object.freeze(['databasesHost', 'dashboardsHost']); +/** #587 — `side-panels.ts`'s own derived pane-id type aliases + * (`UpperPanelId`/`LowerPanelId`/etc.) may never contain a hand-written + * protected literal panel id — the whole point of deriving them from + * `SIDE_PANELS` is that adding a manifest row is the only thing that grows + * either union. */ +export const SIDE_PANEL_TYPE_ALIAS_IDS = Object.freeze(['databases', 'dashboards', 'library', 'history']); +/** #587 — `app.ts` may never directly string-compare `sidePanel.value`; it + * must address panels only through `app.shell.sidePanels`. */ +export const SIDE_PANEL_APP_COMPARISON_VALUES = Object.freeze(['saved', 'history', 'library']); + +/** #587 AC5 — `workbench-session.ts` must never spell the contiguous raw + * string `sidePanel` in real code: not as an identifier, a string/template + * literal, a regex literal, or a computed-element-access argument. This is + * DELIBERATELY a broad contiguous-substring check (not an exact-value + * check like the panel-id/label rules below) — it preserves today's + * `/sidePanel/` raw-regex contract, which flags ANY occurrence containing + * that spelling, e.g. `sidePanelAlias`, not only the bare word. Comments + * are trivia the parser never hands to `TEXTUAL_LEAF_KINDS`, so prose + * explaining the invariant can never false-positive. */ +function workbenchSidePanelMentionViolations(sourceFile, filename) { + const violations = []; + walkTree(sourceFile, (node) => { + if (TEXTUAL_LEAF_KINDS.has(node.kind) && typeof node.text === 'string' && node.text.includes('sidePanel')) { + violations.push(makeViolation( + 'workbench-sidepanel-mention', filename, node.getStart(sourceFile), + 'contiguous "sidePanel" spelling found in real code (identifier/string/template/regex/computed-access)', + )); + } + }); + return violations; +} + +/** + * A real strict-equality (`===`) `BinaryExpression` where one operand is an + * exact-value literal in `literalTargets` and the other satisfies + * `chainPredicate` — in EITHER operand order (`value === 'x'` and + * `'x' === value` both match), and under any quote style (a string vs. a + * no-substitution template literal decode to the identical `.text`, so + * quote-style support falls out of the AST representation for free — no + * separate quote-style branch is needed or present). Shared by the + * workbench `history`-comparison rule (`chainPredicate` always true — ANY + * other operand counts) and the `app.ts` `sidePanel.value` comparison rule + * (`chainPredicate` requires the terminal two-segment chain). This is a + * deliberate STRENGTHENING over today's one-directional, single-quote-only + * regexes (`/===\s*'history'/`, `/sidePanel\.value\s*===\s*'(saved|history| + * library)'/`) — not a preservation of an existing bidirectional/ + * multi-quote-style contract, since neither existed before. + * + * @param {object} sourceFile + * @param {string} filename + * @param {string} rule + * @param {readonly string[]} literalTargets + * @param {(expr: object) => boolean} chainPredicate + * @param {(literalText: string) => string} detailFor + * @returns {object[]} + */ +function strictEqualityLiteralViolations(sourceFile, filename, rule, literalTargets, chainPredicate, detailFor) { + const targets = new Set(literalTargets); + const violations = []; + walkTree(sourceFile, (node) => { + if (node.kind !== SyntaxKind.BinaryExpression || node.operatorToken.kind !== SyntaxKind.EqualsEqualsEqualsToken) return; + let literalSide = null; + let otherSide = null; + if (exactLiteralMatch(node.left, targets)) { + literalSide = node.left; + otherSide = node.right; + } else if (exactLiteralMatch(node.right, targets)) { + literalSide = node.right; + otherSide = node.left; + } else { + return; + } + if (!chainPredicate(otherSide)) return; + violations.push(makeViolation(rule, filename, node.getStart(sourceFile), detailFor(literalSide.text))); + }); + return violations; +} + +/** Every node in `EXACT_LITERAL_KINDS` whose complete value is exactly one of + * `targetsArr` is a violation of `rule` — the shared implementation behind + * the app-preferences/state/app-shell-panel-id exact-value rules (#643 + * mandatory addition 1: because this is an UNSCOPED tree walk, it matches a + * type-position literal, e.g. `type Pref = 'library'`, on exactly the same + * terms as an expression-position one, e.g. `const id = "library"` — see + * `exactLiteralMatch`'s own doc comment). Deliberately does NOT flag a + * longer literal merely CONTAINING one of `targetsArr` as a substring + * (`"pick 'library' now"` stays clean) — the precision change every one of + * these rules' plan sections documents relative to today's raw + * single-quoted substring regexes. */ +function exactLiteralRuleViolations(sourceFile, filename, rule, targetsArr) { + const targets = new Set(targetsArr); + const violations = []; + walkTree(sourceFile, (node) => { + if (exactLiteralMatch(node, targets)) { + violations.push(makeViolation(rule, filename, node.getStart(sourceFile), `protected literal value "${node.text}"`)); + } + }); + return violations; +} + +/** Every node that is either an exact-value identifier OR an exact-value + * literal spelling one of `targetsArr` is a violation of `rule` — the + * app-shell panel-DEFINITION rule ("a real identifier or parser-recognized + * literal token spelling the concrete symbol remains a violation"), unlike + * the literal-only exact-value rule above. */ +function exactIdentifierOrLiteralRuleViolations(sourceFile, filename, rule, targetsArr) { + const targets = new Set(targetsArr); + const violations = []; + walkTree(sourceFile, (node) => { + if (exactIdentifierMatch(node, targets) || exactLiteralMatch(node, targets)) { + violations.push(makeViolation(rule, filename, node.getStart(sourceFile), `concrete symbol "${node.text}" referenced in real code`)); + } + }); + return violations; +} + +/** `app-shell.ts`'s concrete-host rule — `databasesHost`/`dashboardsHost` + * must never be named. Combines three independent shapes into one rule + * (plan §"concrete hosts"): (1) an exact-value identifier — covers dot + * access (`host.databasesHost`), optional access (`host?.dashboardsHost`), + * and destructuring (`const { databasesHost } = hosts`), since all three + * are the SAME AST node kind (a plain `Identifier`) to an unscoped walk; + * (2) an `ElementAccessExpression` whose argument is an exact-value + * string/no-substitution-template literal — `host['dashboardsHost']` / + * `` host[`databasesHost`] ``, the two forms `findNamedIdentifierViolations` + * intentionally does not cover (this rule extends coverage for exactly + * these two names, without changing that shared helper globally — plan + * ruling); (3) preserving today's broad CONTIGUOUS `.databasesHost`/ + * `.dashboardsHost` literal-code substring behavior, but now scoped to an + * actual literal TOKEN rather than the whole raw file text — a string/ + * template literal whose value happens to contain the dotted spelling + * (e.g. prose mentioning `.databasesHost`) still trips this rule, exactly + * as today's substring regex would, while a comment saying the same thing + * does not (trivia). Dynamic construction (`host[prefix + 'Host']`) stays + * provably out of scope: it is neither an exact-value identifier nor an + * `ElementAccessExpression` with a literal argument, and no constant + * folding is attempted. */ +function appShellHostAccessorViolations(sourceFile, filename) { + const targets = new Set(SIDE_PANEL_APP_SHELL_HOSTS); + const violations = []; + walkTree(sourceFile, (node) => { + if (exactIdentifierMatch(node, targets)) { + violations.push(makeViolation( + 'app-shell-host-accessor', filename, node.getStart(sourceFile), + `concrete host accessor "${node.text}" named directly`, + )); + return; + } + if (node.kind === SyntaxKind.ElementAccessExpression && exactLiteralMatch(node.argumentExpression, targets)) { + violations.push(makeViolation( + 'app-shell-host-accessor', filename, node.getStart(sourceFile), + `concrete host accessor "${node.argumentExpression.text}" named via computed element access`, + )); + return; + } + if (TEXTUAL_LEAF_KINDS.has(node.kind) && typeof node.text === 'string') { + for (const name of targets) { + if (node.text.includes(`.${name}`)) { + violations.push(makeViolation( + 'app-shell-host-accessor', filename, node.getStart(sourceFile), + `literal token spells the contiguous accessor ".${name}"`, + )); + break; + } + } + } + }); + return violations; +} + +/** `side-panels.ts`'s type-alias rule: walk actual `TypeAliasDeclaration` + * nodes (never the retired type-alias-extraction regex). Requires at least + * one alias to exist at all (a total-removal regression — deleting every + * derived pane-id type alias — must not silently read as "zero violations + * found"), and flags any protected literal panel id sitting anywhere in an + * alias's OWN `.type` subtree — deliberately scoped to that subtree, not + * the whole file, because the file's real, authoritative `SIDE_PANELS` + * manifest array legitimately spells these exact literals (`{ id: + * 'databases', pane: 'upper' }`) outside any type alias, and must stay + * clean. A defaulted generic type parameter (`type Probe = + * T | 'databases';`) does not exempt the alias from either check — the walk + * finds the `TypeAliasDeclaration` node itself regardless of its type + * parameters, then walks its `.type` unconditionally. */ +function sidePanelsTypeAliasViolations(sourceFile, filename) { + const targets = new Set(SIDE_PANEL_TYPE_ALIAS_IDS); + const violations = []; + let aliasCount = 0; + walkTree(sourceFile, (node) => { + if (node.kind !== SyntaxKind.TypeAliasDeclaration) return; + aliasCount += 1; + const aliasName = node.name.text; + walkTree(node.type, (inner) => { + if (exactLiteralMatch(inner, targets)) { + violations.push(makeViolation( + 'side-panels-type-alias', filename, inner.getStart(sourceFile), + `type alias "${aliasName}" contains protected literal panel id "${inner.text}"`, + )); + } + }); + }); + if (aliasCount === 0) { + violations.push(makeViolation( + 'side-panels-type-alias', filename, 0, + 'no type alias declarations found at all — the derived pane-id unions must stay derived from the manifest', + )); + } + return violations; +} + +/** Terminal two-segment chain predicate for the `app.ts` comparison rule: + * the receiver expression's own last two identifier segments must be + * exactly `sidePanel`, then `value` — `sidePanel.value`, + * `app.shell.sidePanel.value`, etc. all match; a shorter or differently + * named chain does not. */ +function isSidePanelValueChain(expr) { + const names = terminalNames(expr, 2); + return names.length === 2 && names[0] === 'sidePanel' && names[1] === 'value'; +} + +const SIDE_PANEL_RULE_DISPATCH = Object.freeze({ + [WORKBENCH_SESSION_FILE]: (sourceFile, filename) => [ + ...workbenchSidePanelMentionViolations(sourceFile, filename), + ...strictEqualityLiteralViolations( + sourceFile, filename, 'workbench-history-compare', ['history'], () => true, + (value) => `strict equality against literal "${value}"`, + ), + ], + [APP_PREFERENCES_FILE]: (sourceFile, filename) => + exactLiteralRuleViolations(sourceFile, filename, 'app-preferences-panel-id', SIDE_PANEL_APP_PREFERENCES_IDS), + [STATE_FILE]: (sourceFile, filename) => + exactLiteralRuleViolations(sourceFile, filename, 'state-panel-label', SIDE_PANEL_STATE_LABELS), + [APP_FILE]: (sourceFile, filename) => strictEqualityLiteralViolations( + sourceFile, filename, 'app-side-panel-comparison', SIDE_PANEL_APP_COMPARISON_VALUES, isSidePanelValueChain, + (value) => `sidePanel.value strictly compared against literal "${value}"`, + ), + [APP_SHELL_FILE]: (sourceFile, filename) => [ + ...exactIdentifierOrLiteralRuleViolations(sourceFile, filename, 'app-shell-panel-def', SIDE_PANEL_APP_SHELL_DEFS), + ...exactLiteralRuleViolations(sourceFile, filename, 'app-shell-panel-id', SIDE_PANEL_APP_SHELL_IDS), + ...appShellHostAccessorViolations(sourceFile, filename), + ], + [SIDE_PANELS_CORE_FILE]: (sourceFile, filename) => sidePanelsTypeAliasViolations(sourceFile, filename), +}); + +/** + * Issue #643 — the #587 AC5 side-panel source contract, real-parser-backed. + * `filename` selects which (if any) of the six rule groups above apply — a + * file outside `SIDE_PANEL_RULE_DISPATCH`'s six keys returns `[]` without + * even being parsed, matching every other owner-scoped helper in this + * module (`findNamedIdentifierViolations` et al.). Different rule groups + * intentionally use different literal semantics (broad contiguous-substring + * detection vs. exact-value comparison vs. structural chain/identifier + * matching) — this dispatcher composes them, it does not collapse them into + * one generic vocabulary matcher. + * + * @param {string} source + * @param {string} filename repo-relative, forward-slash separated + * @returns {{rule: string, filename: string, pos: number, detail: string}[]} + */ +export function findSidePanelSourceContractViolations(source, filename) { + const dispatch = SIDE_PANEL_RULE_DISPATCH[filename]; + if (!dispatch) return []; + return withParsedSource(source, filename, (sourceFile) => dispatch(sourceFile, filename)); +} + +// ── Surface-lifecycle source contract (#590 invariant (k)) ────────────────── + +/** Classify `[start, end)` against `[coordinatorStart, coordinatorEnd)`: + * `'inside'` when it lies entirely within, `'outside'` when it lies + * entirely outside (on either side), and `'straddle'` for the one shape no + * compile-time mechanism can foreclose either — a range that crosses a + * marker boundary. A `'straddle'` is always treated as a violation by every + * caller below (never silently passed), matching the plan's own + * "deterministic boundary violation" wording. */ +function coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd) { + if (start >= coordinatorStart && end <= coordinatorEnd) return 'inside'; + if (end <= coordinatorStart || start >= coordinatorEnd) return 'outside'; + return 'straddle'; +} + +/** The four coordinator-owned declarations `app.ts` must declare EXACTLY + * inside the marked region — both directions enforced (a name missing from + * inside is flagged exactly like an occurrence found outside), mirroring + * the retired regex test's own two-sided + * `toMatch(inside)`/`not.toMatch(outside)` pair: a symbol that disappears + * from the file ENTIRELY must not silently read as "zero violations", the + * same reasoning `sidePanelsTypeAliasViolations`'s alias-count guard + * applies above. */ +const PROTECTED_DECLARATION_NAMES = Object.freeze([ + 'disposeShell', 'disposeCurrentSurface', 'committedWorkspaceSignal', 'mainSurfaceSignal', +]); + +function protectedDeclarationViolations(appSourceFile, appFile, coordinatorStart, coordinatorEnd) { + const violations = []; + const foundInside = new Set(); + walkTree(appSourceFile, (node) => { + if (node.kind !== SyntaxKind.VariableDeclaration || node.name.kind !== SyntaxKind.Identifier) return; + const name = node.name.text; + if (!PROTECTED_DECLARATION_NAMES.includes(name)) return; + const start = node.getStart(appSourceFile); + const end = node.getEnd(); + const placement = coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd); + if (placement === 'inside') { + foundInside.add(name); + } else { + violations.push(makeViolation( + 'surface-protected-declaration', appFile, start, + `"${name}" is declared ${placement} the coordinator region`, + )); + } + }); + for (const name of PROTECTED_DECLARATION_NAMES) { + if (!foundInside.has(name)) { + violations.push(makeViolation( + 'surface-protected-declaration', appFile, 0, + `"${name}" has no declaration inside the coordinator region`, + )); + } + } + return violations; +} + +/** `app.ts`'s teardown-call rule, outside the coordinator only. A real + * `CallExpression` violates when its callee's own terminal ONE segment is + * `disposeShell`/`disposeCurrentSurface` (bare or member-prefixed — + * `disposeShell()`, `owner.disposeShell()` both match, since + * `terminalNames` only inspects the LAST segment) or its terminal TWO + * segments are exactly `shell`, `dispose` (`shell.dispose()`, + * `app.shell.dispose()`, and — because optional-chained property access is + * the SAME `PropertyAccessExpression` AST kind, just with a + * `questionDotToken` set — `shell?.dispose()` too, with no separate + * branch needed). */ +const TEARDOWN_SINGLE_SEGMENT_NAMES = new Set(['disposeShell', 'disposeCurrentSurface']); + +function teardownCallViolations(appSourceFile, appFile, coordinatorStart, coordinatorEnd) { + const violations = []; + walkTree(appSourceFile, (node) => { + if (node.kind !== SyntaxKind.CallExpression) return; + const single = terminalNames(node.expression, 1); + const double = terminalNames(node.expression, 2); + const isSingleMatch = single.length === 1 && TEARDOWN_SINGLE_SEGMENT_NAMES.has(single[0]); + const isShellDisposeMatch = double.length === 2 && double[0] === 'shell' && double[1] === 'dispose'; + if (!isSingleMatch && !isShellDisposeMatch) return; + const start = node.getStart(appSourceFile); + const end = node.getEnd(); + const placement = coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd); + if (placement !== 'inside') { + violations.push(makeViolation( + 'surface-teardown-call', appFile, start, + `a teardown call sits ${placement} the coordinator region`, + )); + } + }); + return violations; +} + +/** The two private signal identifiers a plain `.value =` write may never + * target outside the coordinator, tree-wide (`app.ts` gets the coordinator + * exception; every other file never legally names either identifier at + * all, since they are not exported). */ +const SURFACE_SIGNAL_NAMES = Object.freeze(['committedWorkspaceSignal', 'mainSurfaceSignal']); + +function signalWriteViolations(sourceFile, filename, isAppFile, coordinatorStart, coordinatorEnd) { + const violations = []; + walkTree(sourceFile, (node) => { + if (node.kind !== SyntaxKind.BinaryExpression || node.operatorToken.kind !== SyntaxKind.EqualsToken) return; + const left = node.left; + if (left.kind !== SyntaxKind.PropertyAccessExpression || left.name.text !== 'value') return; + const owner = terminalNames(left.expression, 1); + if (owner.length !== 1 || !SURFACE_SIGNAL_NAMES.includes(owner[0])) return; + const start = node.getStart(sourceFile); + const end = node.getEnd(); + if (isAppFile && coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd) === 'inside') return; + violations.push(makeViolation( + 'surface-signal-write', filename, start, + `"${owner[0]}.value" is written outside the coordinator region`, + )); + }); + return violations; +} + +/** + * `currentWorkspace = null` (and every transparent cast/assertion wrapper + * around the `null`) outside the coordinator, tree-wide (`app.ts` gets the + * coordinator exception). The left side must be an actual property access + * ending in `.currentWorkspace` (a bare, receiver-less `currentWorkspace = + * null` was never in scope for today's `\.currentWorkspace` regex either, + * and stays out of scope here). `null ?? fallback` is a deliberate, + * documented exclusion — see `unwrapNullEquivalentWrappers`'s own doc + * comment for why `??` is not a transparent wrapper. + */ +function currentWorkspaceNullViolations(sourceFile, filename, isAppFile, coordinatorStart, coordinatorEnd) { + const violations = []; + walkTree(sourceFile, (node) => { + if (node.kind !== SyntaxKind.BinaryExpression || node.operatorToken.kind !== SyntaxKind.EqualsToken) return; + const left = node.left; + if (left.kind !== SyntaxKind.PropertyAccessExpression || left.name.text !== 'currentWorkspace') return; + const resolved = unwrapNullEquivalentWrappers(node.right); + if (!resolved || resolved.kind !== SyntaxKind.NullKeyword) return; + const start = node.getStart(sourceFile); + const end = node.getEnd(); + if (isAppFile && coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd) === 'inside') return; + violations.push(makeViolation( + 'surface-current-workspace-null', filename, start, + '"currentWorkspace" is assigned a null-equivalent value outside the coordinator region', + )); + }); + return violations; +} + +/** Every function-like AST kind whose `.body` may be a `Block` — including + * return-annotated declarations (`function f(): T { ... }`), which the + * retired textual opener (`/(?:=>|\))\s*\{/`) could never recognize because + * the return-type annotation's text sits between the parameter list's `)` + * and the body's `{`. Deliberately excludes concise (non-block) arrow + * bodies (`() => expr`) — there is no `Block` there to scope. */ +const FUNCTION_LIKE_KINDS = new Set([ + SyntaxKind.FunctionDeclaration, + SyntaxKind.FunctionExpression, + SyntaxKind.ArrowFunction, + SyntaxKind.MethodDeclaration, + SyntaxKind.GetAccessor, + SyntaxKind.SetAccessor, + SyntaxKind.Constructor, +]); + +/** + * Every "ordering scope" in `sourceFile` — a `Block`/`CaseBlock` node whose + * enclosing construct the plan's own ordering-scope table names: every + * block-bodied function-like node (see `FUNCTION_LIKE_KINDS`), PLUS exactly + * the parenthesized control-flow forms the retired `) {` textual opener + * happened to also treat as independent scopes — `if`'s then-block, a + * `for`/`for-in`/`for-of`/`while`/`with` body when it is itself a `Block`, + * a `switch`'s whole `CaseBlock` (one scope for every case together, not + * one per case — matching the opener's single `switch (...) {` match), and + * a `catch` block ONLY when it has the parenthesized binding form + * (`catch (e) { ... }`, never binding-less `catch { ... }`, which the old + * opener's `)` requirement also never matched). Deliberately does NOT add + * `else`/`do`/`try`/`finally`/binding-less-`catch`/a bare standalone block as + * independent scopes — the old opener never recognized those either, and + * they remain reachable (and checked) only through whichever enclosing + * scope from this list actually contains them. + * + * @param {object} sourceFile + * @returns {object[]} `Block`/`CaseBlock` nodes, one per ordering scope + */ +function collectOrderingScopes(sourceFile) { + const scopes = []; + walkTree(sourceFile, (node) => { + if (FUNCTION_LIKE_KINDS.has(node.kind) && node.body && node.body.kind === SyntaxKind.Block) { + scopes.push(node.body); + return; + } + if (node.kind === SyntaxKind.IfStatement && node.thenStatement && node.thenStatement.kind === SyntaxKind.Block) { + scopes.push(node.thenStatement); + return; + } + if ( + (node.kind === SyntaxKind.ForStatement + || node.kind === SyntaxKind.ForInStatement + || node.kind === SyntaxKind.ForOfStatement + || node.kind === SyntaxKind.WhileStatement + || node.kind === SyntaxKind.WithStatement) + && node.statement && node.statement.kind === SyntaxKind.Block + ) { + scopes.push(node.statement); + return; + } + if (node.kind === SyntaxKind.SwitchStatement) { + scopes.push(node.caseBlock); + return; + } + if (node.kind === SyntaxKind.CatchClause && node.variableDeclaration && node.block) { + scopes.push(node.block); + } + }); + return scopes; +} + +/** True for a plain `=` write whose left side is a property access ending in + * `.mainSurface` or `.currentWorkspace` — the ordering rule's OWN "protected + * write" shape, deliberately independent of (broader than in file scope, + * narrower in property-name scope than) the tree-wide null/signal rules + * above: this fires regardless of the RHS value, matching today's + * `/\.(?:mainSurface|currentWorkspace)\s*=(?!=)/` identifier-anchored + * regex. */ +function isProtectedOrderingWrite(node) { + return node.kind === SyntaxKind.BinaryExpression + && node.operatorToken.kind === SyntaxKind.EqualsToken + && node.left.kind === SyntaxKind.PropertyAccessExpression + && (node.left.name.text === 'mainSurface' || node.left.name.text === 'currentWorkspace'); +} + +const RETIRE_CALL_NAME_PATTERN = /^retireTo/; + +/** True for a real call whose callee's own terminal (last) identifier + * segment matches `retireTo*` — bare or member-prefixed, matching today's + * `/\bretireTo\w*\s*\(/` textual pattern's own breadth. */ +function isRetirementCall(node) { + if (node.kind !== SyntaxKind.CallExpression) return false; + const names = terminalNames(node.expression, 1); + return names.length === 1 && RETIRE_CALL_NAME_PATTERN.test(names[0]); +} + +/** The earliest `getStart(sourceFile)` position, among every descendant of + * `scopeNode` (scopeNode itself included) for which `predicate` is true — + * or `null` when none match. Deliberately walks the WHOLE subtree + * unconditionally (never stopping at a nested scope's own boundary), which + * is exactly how "nested descendants remain visible to their enclosing + * scope" (today's conservative lexical model) is preserved: a retirement + * call three functions deep still counts for every scope that contains it. */ +function firstMatchStart(scopeNode, sourceFile, predicate) { + let best = null; + walkTree(scopeNode, (node) => { + if (!predicate(node)) return; + const pos = node.getStart(sourceFile); + if (best === null || pos < best) best = pos; + }); + return best; +} + +/** + * The retirement-ordering rule, tree-wide, unconditional on the coordinator + * (the plan's own rule-scope matrix names no coordinator carve-out for + * ordering, matching the retired implementation, which applied + * `functionBodies`/its regex identically to every scanned file with no + * app.ts-specific branch at all). For every ordering scope + * (`collectOrderingScopes`), find the scope's own first protected write and + * first retirement call (both possibly satisfied by a nested descendant — + * see `firstMatchStart`) and flag the scope only when a write precedes a + * retirement call that also exists in that same scope; a scope with a write + * but no retirement call anywhere within it is clean (today's "retire before + * write" and "no retire at all" cases were never distinguished, and stay + * that way). + * + * @param {object} sourceFile + * @param {string} filename + * @returns {object[]} + */ +function retirementOrderingViolations(sourceFile, filename) { + const violations = []; + for (const scope of collectOrderingScopes(sourceFile)) { + const writeStart = firstMatchStart(scope, sourceFile, isProtectedOrderingWrite); + if (writeStart === null) continue; + const retireStart = firstMatchStart(scope, sourceFile, isRetirementCall); + if (retireStart !== null && writeStart < retireStart) { + violations.push(makeViolation( + 'surface-retirement-ordering', filename, writeStart, + `a mainSurface/currentWorkspace write at ${writeStart} precedes a retireTo*() call at ${retireStart} within one ordering scope`, + )); + } + } + return violations; +} + +/** + * Issue #643 — the #590 invariant (k) surface-lifecycle source contract, + * real-parser-backed, over the COMPLETE scanned `src/**` source set in one + * shared parser batch (`withParsedSources`, never one process per file — see + * that function's own doc comment for why this matters at this tree's + * file count). `sources` supplies every currently scanned file's raw text + * unchanged (comments included — the coordinator markers themselves are + * `//` line comments the caller locates in the SAME raw text before calling + * this, and their byte offsets align exactly with this function's AST node + * positions because neither side strips or reconstructs anything). + * `coordinatorStart`/`coordinatorEnd` are those two raw offsets in + * `appFile`'s own source; `appFile` must be one of the filenames in + * `sources`, or this throws (fail loud, matching every other "could not + * resolve the source I was asked to check" case in this module). + * + * @param {readonly {filename: string, source: string}[]} sources + * @param {{appFile: string, coordinatorStart: number, coordinatorEnd: number}} options + * @returns {{rule: string, filename: string, pos: number, detail: string}[]} + */ +export function findSurfaceLifecycleSourceContractViolations(sources, { appFile, coordinatorStart, coordinatorEnd }) { + return withParsedSources(sources, (sourceFiles) => { + const appSourceFile = sourceFiles.get(appFile); + if (!appSourceFile) { + throw new Error(`check-legacy-owners: ${appFile} was not found in the supplied surface-lifecycle source batch`); + } + const violations = [ + ...protectedDeclarationViolations(appSourceFile, appFile, coordinatorStart, coordinatorEnd), + ...teardownCallViolations(appSourceFile, appFile, coordinatorStart, coordinatorEnd), + ]; + for (const [filename, sourceFile] of sourceFiles) { + const isAppFile = filename === appFile; + violations.push(...signalWriteViolations(sourceFile, filename, isAppFile, coordinatorStart, coordinatorEnd)); + violations.push(...currentWorkspaceNullViolations(sourceFile, filename, isAppFile, coordinatorStart, coordinatorEnd)); + violations.push(...retirementOrderingViolations(sourceFile, filename)); + } + return violations; + }); +} diff --git a/tests/unit/side-panel-source-contract.test.ts b/tests/unit/side-panel-source-contract.test.ts index 83eb6999..8a499492 100644 --- a/tests/unit/side-panel-source-contract.test.ts +++ b/tests/unit/side-panel-source-contract.test.ts @@ -2,6 +2,8 @@ import { describe, it, expect } from 'vitest'; import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { dirname, join } from 'node:path'; +import { findSidePanelSourceContractViolations } from '../../build/lib/check-legacy-owners.mjs'; +import type { SourceContractViolation } from '../../build/lib/check-legacy-owners.mjs'; // #587 AC5 (per R2.10's falsifiability requirement — a fake-panel test alone // proves an injectable builder accepts injected data, not that adding a REAL @@ -9,15 +11,22 @@ import { dirname, join } from 'node:path'; // compile-time guarantee alone isn't falsifiable either). This is the third // leg: an executable, source-level check that no panel id/label comparison or // hard-coded tab-row vocabulary has crept back into the four files #587 AC5 -// names — it must go red the moment one does (see the sabotage check in the -// phase report: reintroducing `sidePanel` into `workbench-session.ts`, or a -// hard-coded 'Databases' label into `state.ts`, both fail this test). +// names — it must go red the moment one does (see the sabotage checks below: +// reintroducing `sidePanel` into `workbench-session.ts`, or a hard-coded +// 'Databases' label into `state.ts`, both fail this test). // -// Comments are stripped before matching (a best-effort block/line-comment -// regex, not a real parser) — every current mention of these strings in the -// four files is documentation ABOUT the invariant, not code enforcing a -// panel-specific branch, and this test must not flag its own explanatory -// comments as violations. +// #643 — this suite used to preprocess source with a two-pass regex comment +// stripper (`/\*[\s\S]*?\*\//g` then a line-comment regex) before matching. +// That order is unsound: a `/*`-shaped substring sitting inside a real `//` +// comment can make the block-comment pass consume real code through the +// NEXT genuine `*/`, hiding a real violation before this suite's assertions +// ever ran. Every check below instead calls +// `findSidePanelSourceContractViolations` (`build/lib/check-legacy-owners.mjs`, +// #643), which walks the REAL TypeScript AST the same shared parser +// infrastructure `tests/unit/clickhouse-http-package-policy.test.js` and +// `tests/unit/check-boundaries-dynamic-imports.test.js` already exercise — +// no second lexer, no comment/string/template/regex-literal ambiguity is +// even representable in what it inspects. // `new URL(...)` goes through happy-dom's own (non-Node) URL implementation // under this test environment, which rejects `file:` schemes — so this @@ -25,123 +34,280 @@ import { dirname, join } from 'node:path'; const here = dirname(fileURLToPath(import.meta.url)); // tests/unit const root = join(here, '..', '..'); // repo root -function stripComments(src: string): string { - return src - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/(^|[^:"'`])\/\/.*$/gm, '$1'); +function realSource(relativePath: string): string { + return readFileSync(join(root, relativePath), 'utf8'); } -function codeOf(relativePath: string): string { - return stripComments(readFileSync(join(root, relativePath), 'utf8')); +function violations(source: string, filename: string): SourceContractViolation[] { + return findSidePanelSourceContractViolations(source, filename); } -describe('#587 AC5 source contract: no panel id/label selection outside the registry', () => { - it('workbench-session.ts never mentions `sidePanel` in code — it does not know the concept exists', () => { - const code = codeOf('src/ui/workbench/workbench-session.ts'); - expect(code).not.toMatch(/sidePanel/); - // The specific regression #587 replaced: a direct panel-id string compare. - expect(code).not.toMatch(/===\s*'history'/); +const WORKBENCH_SESSION = 'src/ui/workbench/workbench-session.ts'; +const APP_PREFERENCES = 'src/application/app-preferences.ts'; +const STATE = 'src/state.ts'; +const APP = 'src/ui/app.ts'; +const APP_SHELL = 'src/ui/app-shell.ts'; +const SIDE_PANELS_CORE = 'src/core/side-panels.ts'; + +describe('#587 AC5 source contract: no panel id/label selection outside the registry (real files)', () => { + it('workbench-session.ts is clean: no `sidePanel` mention, no `=== \'history\'` comparison', () => { + expect(violations(realSource(WORKBENCH_SESSION), WORKBENCH_SESSION)).toEqual([]); }); - it('app-preferences.ts never hard-codes a panel id — its union is a TYPE, derived, never a literal comparison', () => { - const code = codeOf('src/application/app-preferences.ts'); - for (const id of ['library', 'databases', 'dashboards']) { - expect(code).not.toContain(`'${id}'`); - } + it('app-preferences.ts is clean: no hard-coded panel id — its union is a TYPE, derived, never a literal comparison', () => { + expect(violations(realSource(APP_PREFERENCES), APP_PREFERENCES)).toEqual([]); }); - it('state.ts never hard-codes a display label — labels belong to the registry, not the state model', () => { - const code = codeOf('src/state.ts'); - for (const label of ['Databases', 'Dashboards', 'Library', 'History']) { - expect(code).not.toContain(`'${label}'`); - } + it('state.ts is clean: no hard-coded display label — labels belong to the registry, not the state model', () => { + expect(violations(realSource(STATE), STATE)).toEqual([]); }); - it('app.ts never string-compares a lower-panel id directly — it addresses panels only through app.shell.sidePanels', () => { - const code = codeOf('src/ui/app.ts'); - expect(code).not.toMatch(/sidePanel\.value\s*===\s*'(saved|history|library)'/); + it('app.ts is clean: no direct lower-panel id string comparison — it addresses panels only through app.shell.sidePanels', () => { + expect(violations(realSource(APP), APP)).toEqual([]); }); // #600 review finding 1: `app-shell.ts` is the FOURTH file AC5 names - // outright ("adding a panel must not touch app-shell.ts") — and the three - // checks above never covered it, so the four concrete panel-def imports - // that used to sit right in this file's composition stayed green forever. - // `buildProductionSidePanelRegistry` (side-panel-registry.ts) is now the - // ONE place the four defs are listed; this must go red the moment a - // concrete panel-def import or a bare panel-id literal creeps back into - // `app-shell.ts`. - it('app-shell.ts names no concrete panel-def symbol or panel id — panel composition lives in the registry, not the shell', () => { - const code = codeOf('src/ui/app-shell.ts'); - for (const symbol of ['databasesPanelDef', 'dashboardsPanelDef', 'libraryPanelDef', 'historyPanelDef']) { - expect(code).not.toContain(symbol); - } - for (const id of ['databases', 'dashboards', 'library', 'history']) { - expect(code).not.toMatch(new RegExp(`['"]${id}['"]`)); - } - }); - - // #600 review finding 1 (round 2): the two checks above missed this — a - // concrete HOST ACCESSOR (`.databasesHost`/`.dashboardsHost`) is neither a - // `*PanelDef` symbol nor a quoted panel-id literal, so `app-shell.ts` could - // (and did) compose `schemaPane` by naming these two properties directly - // instead of deriving them from `registry.entries`, and neither check - // above caught it. This must go red the moment either accessor creeps back - // into this file's code (comments describing the invariant are stripped - // first, same as every other check in this suite). - it('app-shell.ts names no concrete upper-pane host accessor — the upper pane\'s hosts come from registry.entries, like the lower pane\'s', () => { - const code = codeOf('src/ui/app-shell.ts'); - for (const accessor of ['.databasesHost', '.dashboardsHost']) { - expect(code).not.toContain(accessor); - } + // outright ("adding a panel must not touch app-shell.ts"). + it('app-shell.ts is clean: no concrete panel-def symbol, no bare panel-id literal, no concrete host accessor', () => { + expect(violations(realSource(APP_SHELL), APP_SHELL)).toEqual([]); + }); + + it('side-panels.ts is clean: its derived pane id unions contain no hand-written literal panel id', () => { + expect(violations(realSource(SIDE_PANELS_CORE), SIDE_PANELS_CORE)).toEqual([]); }); }); // #587 finding 2/3 (PR #600 review, round 2): `tests/types/side-panels.test-d.ts` // pins coverage/disjointness of `UpperPanelId`/`LowerPanelId` against TODAY'S // manifest only — for the current four-row `SIDE_PANELS`, the derived unions -// and the old hand-written `Extract` -// literals produce IDENTICAL types, so that type-level test alone cannot +// and a hand-written `Extract` +// literal produce IDENTICAL types, so that type-level test alone cannot // detect a plain revert to hand-written literals with no accompanying -// manifest change. This is the source-level backstop for exactly that case. -describe('#587 source contract: side-panels.ts derives its pane id unions, never a literal allowlist', () => { - it('side-panels.ts declares no type alias containing a literal panel-id string — UpperPanelId/LowerPanelId must stay derived from the manifest', () => { - const code = codeOf('src/core/side-panels.ts'); - // Best-effort, not a real parser: matches each `type Name<...> = ...;` - // declaration in this file — INCLUDING a multi-line one, despite how that - // might look: `[^=]*`/`[^;]*` are negated character classes, and (unlike - // `.` without the `/s` flag) those DO match newlines, so a type alias - // whose `=`/body spans multiple lines is captured whole, not truncated at - // the first line break (verified against a literal multi-line sample - // before writing this comment — do not restate "multi-line slips past" - // without re-checking, since that claim was wrong once already here). - // - // What actually slips past unmatched: a generic parameter list carrying a - // DEFAULT type argument, e.g. `type Foo = ...;` — the - // optional `(?:<[^=]*>)?` group forbids `=` inside the angle brackets, so - // on a default-typed generic the group can't match either the `<...>` - // clause OR (falling back to its "absent" alternative) the identifier - // immediately followed by `<`, and the WHOLE statement fails to match. - // That drops it from `typeAliasStatements` entirely — not "matched but - // unflagged", but never inspected at all — so a literal panel id inside - // such an alias's body would go undetected. None of today's seven type - // aliases in this file declare a defaulted generic; catching that shape - // would need a real AST parse rather than a regex. - const typeAliasStatements = code.match(/\btype\s+[A-Za-z_]\w*(?:<[^=]*>)?\s*=\s*[^;]*;/g) ?? []; - expect(typeAliasStatements.length).toBeGreaterThan(0); // the pattern itself must still find something - const panelIds = ['databases', 'dashboards', 'library', 'history']; - for (const statement of typeAliasStatements) { - for (const id of panelIds) { - // All three quoting styles TypeScript allows for a string literal - // type, not single-quotes only — `export type UpperPanelId = - // Extract;` is an identical - // hand-written-allowlist regression that single-quote-only checking - // would miss outright (no lint/formatting script here enforces one - // quote style — see this repo's CLAUDE.md hard rule 4's dependency - // list; none of the seven is a linter). - expect(statement).not.toContain(`'${id}'`); - expect(statement).not.toContain(`"${id}"`); - expect(statement).not.toContain(`\`${id}\``); - } - } +// manifest change. This suite is the source-level backstop for exactly that +// case — `side-panels-type-alias` (below) requires at least one type alias +// to exist AND rejects a protected literal anywhere in an alias's own type +// subtree, including one hidden behind a defaulted generic parameter. + +describe('required lexical sabotage matrix (both analyzers share this shape)', () => { + // The exact reported reproduction: the retired two-pass stripper would + // genuinely have deleted the middle statement (a `/*`-shaped substring + // inside a `//` comment eating real code through the next `*/`). + it('a `//` comment containing a fake block-opener does not hide the real violation that follows', () => { + const source = [ + '// documentation mentioning src/core/**', + 'const sidePanelViolation = 1;', + '/* next real block comment */', + ].join('\n'); + const found = violations(source, WORKBENCH_SESSION); + expect(found).toHaveLength(1); + expect(found[0]!.rule).toBe('workbench-sidepanel-mention'); + }); + + it('a `//` comment mentioning a glob-like path does not hide the real violation that follows', () => { + const source = '// src/core/**\nconst sidePanelViolation = 1;\n'; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('a legal block comment does not hide the real violation that follows', () => { + const source = '/* a normal, legal block comment */\nconst sidePanelViolation = 1;\n'; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('comment-shaped text inside a string literal does not hide the real violation that follows', () => { + const source = "const s = 'comment-shaped /* text';\nconst sidePanelViolation = 1;\n"; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('comment-shaped text inside a template literal does not hide the real violation that follows', () => { + const source = 'const t = `comment-shaped /* text`;\nconst sidePanelViolation = 1;\n'; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('a parser-valid regex literal containing comment-shaped characters does not hide the real violation that follows', () => { + const source = 'const r = /a\\/\\*b/;\nconst sidePanelViolation = 1;\n'; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('a real forbidden construct immediately following a lexical trap is still caught', () => { + const source = '/*c*/const sidePanelViolation = 1;\n'; + expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('forbidden vocabulary appearing only in comments stays clean', () => { + const source = [ + '// this comment mentions sidePanel, src/core/**, and forbiddenArchitectureViolation', + '/* so does this block comment: sidePanel */', + 'const x = 1;', + ].join('\n'); + expect(violations(source, WORKBENCH_SESSION)).toEqual([]); + }); +}); + +describe('additional side-panel sabotage: workbench breadth', () => { + const cases: Array<[string, string]> = [ + ['identifier containing the raw spelling', 'const sidePanelThing = 1;'], + ['string literal', "const text = 'sidePanel';"], + ['no-substitution template literal', 'const tpl = `sidePanel`;'], + ['regex literal', 'const re = /sidePanel/;'], + ['computed string element access', "obj['sidePanel'];"], + ['computed template element access', 'obj[`sidePanel`];'], + ]; + for (const [label, code] of cases) { + it(`${label} is a violation`, () => { + expect(violations(code, WORKBENCH_SESSION)).toHaveLength(1); + }); + } + + it('comment-only equivalents of every shape above stay clean', () => { + const source = [ + '// const sidePanelThing = 1;', + "// const text = 'sidePanel';", + '// const tpl = `sidePanel`;', + '// const re = /sidePanel/;', + "// obj['sidePanel'];", + '// obj[`sidePanel`];', + 'const x = 1;', + ].join('\n'); + expect(violations(source, WORKBENCH_SESSION)).toEqual([]); + }); + + it('the history comparison rule supports both operand orders and every quote style', () => { + expect(violations("value === 'history';", WORKBENCH_SESSION)).toHaveLength(1); + expect(violations("'history' === value;", WORKBENCH_SESSION)).toHaveLength(1); + expect(violations('value === "history";', WORKBENCH_SESSION)).toHaveLength(1); + expect(violations('value === `history`;', WORKBENCH_SESSION)).toHaveLength(1); + }); + + it('a string literal that merely LOOKS like the history comparison (not a real equality) stays clean', () => { + const source = 'const note = "value === \'history\'";'; + expect(violations(source, WORKBENCH_SESSION)).toEqual([]); + }); +}); + +describe('additional side-panel sabotage: literal-value precision (app-preferences/state/app-shell ids)', () => { + it('app-preferences.ts: an exact protected id literal fails, in every quote style, but a longer literal merely containing one stays clean', () => { + expect(violations('const id = "library";', APP_PREFERENCES)).toHaveLength(1); + expect(violations('const id = `library`;', APP_PREFERENCES)).toHaveLength(1); + expect(violations("const note = \"pick 'library' now\";", APP_PREFERENCES)).toEqual([]); + }); + + it('state.ts: an exact protected label literal fails, but a longer literal merely containing one stays clean', () => { + expect(violations('const label = "History";', STATE)).toHaveLength(1); + expect(violations("const note2 = \"old 'History' label\";", STATE)).toEqual([]); + }); + + it('app-shell.ts panel ids: an exact protected id literal fails, but a longer literal merely containing one stays clean', () => { + expect(violations('const panel = `databases`;', APP_SHELL)).toHaveLength(1); + expect(violations("const note3 = \"pick 'databases'\";", APP_SHELL)).toEqual([]); + }); + + // #643 mandatory addition 1 (pass-5 finding): the CURRENT regex-based test + // scans the whole stripped file textually, so it already catches a + // hand-written literal TYPE union too (e.g. `type Pref = 'library'`) — + // exactly the #587 finding-2/3 regression shape. An implementation that + // only walked EXPRESSION-position AST nodes for these three rule groups + // would silently weaken that existing contract. These three cases pin that + // a type-position `StringLiteral`/`NoSubstitutionTemplateLiteral` (a + // `LiteralTypeNode`'s own literal) is caught on exactly the same terms as + // an expression-position one. + it('app-preferences.ts: a TYPE-position literal ("type Pref = \'library\';") still fails', () => { + expect(violations("type Pref = 'library';", APP_PREFERENCES)).toHaveLength(1); + }); + + it('state.ts: a TYPE-position literal ("type X = \'History\';") still fails', () => { + expect(violations("type X = 'History';", STATE)).toHaveLength(1); + }); + + it('app-shell.ts panel ids: a TYPE-position literal ("type X = \'databases\';") still fails', () => { + expect(violations("type X = 'databases';", APP_SHELL)).toHaveLength(1); + }); +}); + +describe('additional side-panel sabotage: app.ts comparison', () => { + it('supports the full receiver chain, both operand orders, and every quote style', () => { + expect(violations("app.shell.sidePanel.value === 'saved';", APP)).toHaveLength(1); + expect(violations("'saved' === app.shell.sidePanel.value;", APP)).toHaveLength(1); + expect(violations('sidePanel.value === "history";', APP)).toHaveLength(1); + expect(violations('sidePanel.value === `library`;', APP)).toHaveLength(1); + }); + + it('a string literal that merely LOOKS like the comparison (not a real equality) stays clean', () => { + const source = 'const note = "sidePanel.value === \'saved\'";'; + expect(violations(source, APP)).toEqual([]); + }); +}); + +describe('additional side-panel sabotage: panel defs and hosts (app-shell.ts)', () => { + it('a concrete panel-def identifier reference is a violation', () => { + expect(violations('const x = databasesPanelDef;', APP_SHELL)).toHaveLength(1); + }); + + it('a concrete panel-def spelling in a real literal token is a violation', () => { + expect(violations('const x = "dashboardsPanelDef";', APP_SHELL)).toHaveLength(1); + }); + + it('a comment naming a panel-def symbol stays clean', () => { + expect(violations('// libraryPanelDef used to live here\nconst x = 1;', APP_SHELL)).toEqual([]); + }); + + it('dot host access is a violation', () => { + expect(violations('host.databasesHost;', APP_SHELL)).toHaveLength(1); + }); + + it('optional host access is a violation', () => { + expect(violations('host?.dashboardsHost;', APP_SHELL)).toHaveLength(1); + }); + + it('destructuring a host name is a violation', () => { + expect(violations('const { databasesHost } = hosts;', APP_SHELL)).toHaveLength(1); + }); + + it('string element access naming a host is a violation', () => { + expect(violations("host['dashboardsHost'];", APP_SHELL)).toHaveLength(1); + }); + + it('template element access naming a host is a violation', () => { + expect(violations('host[`databasesHost`];', APP_SHELL)).toHaveLength(1); + }); + + it('a contiguous ".databasesHost" spelling inside a literal token is a violation (preserving today\'s broad substring behavior)', () => { + expect(violations('const msg = "call host.databasesHost please";', APP_SHELL)).toHaveLength(1); + }); + + it('a comment-only host spelling stays clean', () => { + expect(violations('// .databasesHost used to live here\nconst x = 1;', APP_SHELL)).toEqual([]); + }); + + it('dynamic construction of a host name is out of scope (no constant folding)', () => { + expect(violations("host[prefix + 'Host'];", APP_SHELL)).toEqual([]); + }); +}); + +describe('additional side-panel sabotage: side-panels.ts type aliases', () => { + it('a defaulted generic type parameter does not exempt the alias from the protected-literal check', () => { + const found = violations("type Probe = T | 'databases';", SIDE_PANELS_CORE); + expect(found).toHaveLength(1); + expect(found[0]!.rule).toBe('side-panels-type-alias'); + }); + + it('a plain protected literal in a type alias fails', () => { + expect(violations("type Probe = 'library';", SIDE_PANELS_CORE)).toHaveLength(1); + }); + + it('a file with zero type alias declarations at all is itself a violation (a total-removal regression must not read as clean)', () => { + const found = violations('const x = 1;', SIDE_PANELS_CORE); + expect(found).toHaveLength(1); + expect(found[0]!.rule).toBe('side-panels-type-alias'); + }); + + it('the manifest array itself (outside any type alias) legitimately spells the protected literals and stays clean', () => { + const source = "type Probe = string;\nexport const SIDE_PANELS = [{ id: 'databases', pane: 'upper' }];\n"; + expect(violations(source, SIDE_PANELS_CORE)).toEqual([]); + }); +}); + +describe('a filename outside the six guarded files is never parsed and never reports a violation', () => { + it('returns [] for an unrelated filename even with clearly-forbidden-looking source', () => { + expect(violations("const sidePanel = 'library';", 'src/core/unrelated.ts')).toEqual([]); }); }); diff --git a/tests/unit/surface-lifecycle-arch.test.ts b/tests/unit/surface-lifecycle-arch.test.ts index c1f92b1b..76239bc4 100644 --- a/tests/unit/surface-lifecycle-arch.test.ts +++ b/tests/unit/surface-lifecycle-arch.test.ts @@ -4,10 +4,11 @@ // narrowing): compile-time elimination is already covered by // `surface-accessor-contracts.test.ts`'s `@ts-expect-error` fixtures (the // asymmetric `currentWorkspace` setter, the narrowed structural ports); THIS -// test is the third, weakest layer — a hand-rolled regex scan over `src/**` -// production sources, the same idiom `build/check-boundaries.mjs` (mechanical -// dependency-direction checks) and `typography-contract.test.js` (reading -// `src/styles.css` directly) already use in this repo. It fails the build on: +// test is the third, weakest layer — a real-TypeScript-parser-backed scan +// over `src/**` production sources (#643; see below), the same idiom +// `build/check-boundaries.mjs` (mechanical dependency-direction checks) and +// `typography-contract.test.js` (reading `src/styles.css` directly) already +// use in this repo. It fails the build on: // (a) an out-of-coordinator `.value` write naming the private signal // identifiers (identifier-anchored, so an alias/non-literal // right-hand-side is caught too — pass-7 finding); @@ -18,20 +19,44 @@ // (d) any of those three declarations moving outside the marked region; // (e) the ADJACENCY hazard no compile-time mechanism can foreclose: a // `mainSurface`/`currentWorkspace` assignment lexically preceding a -// `retireTo*`/retirement-hook call in the SAME function body (the +// `retireTo*`/retirement-hook call in the SAME ordering scope (the // exported `retireTo*` ops are meant to be callable from outside the // coordinator by design, so this is the one shape compile scoping // cannot reject). // +// #643 — this suite used to preprocess source with a two-pass regex comment +// stripper before scanning, and located "function bodies" via a textual +// `/(?:=>|\))\s*\{/` opener plus manual brace-depth matching. Both were +// unsound/imprecise in ways that mattered for an architecture GUARD: the +// stripper could delete real code hidden behind a `/*`-shaped substring +// inside a `//` comment, and the textual opener both MISSED +// return-annotated function declarations (`function f(): T { ... }` — the +// return-type text sits between the parameter list's `)` and the body's +// `{`) and accidentally treated every parenthesized control-flow block +// (`if (...) {`, `for (...) {`, etc.) as an independent ordering scope. Every +// check below instead calls `findSurfaceLifecycleSourceContractViolations` +// (`build/lib/check-legacy-owners.mjs`, #643): a real TypeScript parse over +// one shared parser batch for the whole scanned tree, using explicit +// AST-recognized ordering scopes (every block-bodied function-like node — +// now INCLUDING return-annotated ones — plus the exact parenthesized +// control-flow forms the old opener happened to also match: `if`'s +// then-block, `for`/`for-in`/`for-of`/`while`/`with` bodies, a `switch`'s +// whole case block, and a `catch (e) { ... }` block). `else`/`do`/`try`/ +// `finally`/binding-less-`catch`/a bare standalone block are deliberately +// NOT independent scopes (the old opener never recognized them either); they +// remain visible through whichever enclosing scope contains them. +// // Stays `.js`-idiom-compatible (reads files via `node:fs`) but is `.ts`, // matching `tests/unit/side-panel-source-contract.test.ts`'s precedent (the // repo carries no `@types/node`; `tests/types/node-fs-url.d.ts` is the // minimal ambient shim both files share). -import { describe, expect, it } from 'vitest'; +import { beforeAll, describe, expect, it } from 'vitest'; import { readFileSync, readdirSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { dirname, join } from 'node:path'; +import { findSurfaceLifecycleSourceContractViolations } from '../../build/lib/check-legacy-owners.mjs'; +import type { SourceContractViolation, SurfaceLifecycleSourceEntry } from '../../build/lib/check-legacy-owners.mjs'; const here = dirname(fileURLToPath(import.meta.url)); // tests/unit const root = join(here, '..', '..'); // repo root @@ -41,12 +66,6 @@ const APP_TS = 'src/ui/app.ts'; const BEGIN_MARKER = '// #590-COORDINATOR-BEGIN'; const END_MARKER = '// #590-COORDINATOR-END'; -function stripComments(src: string): string { - return src - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/(^|[^:"'`])\/\/.*$/gm, '$1'); -} - function listSourceFiles(): string[] { return readdirSync(srcDir, { recursive: true }) .filter((rel) => /\.(ts|js)$/.test(rel)) @@ -57,24 +76,33 @@ function listSourceFiles(): string[] { } const files = listSourceFiles(); -// The markers ARE `//` line comments, so they must be located in the RAW -// (unstripped) text — `stripComments` runs per-slice AFTER the region split -// below, never on the whole file first (that would delete the markers -// before they could be found, silently collapsing the "region" to nothing -// and making every real disposeShell/disposeCurrentSurface call outside it -// look like a violation — caught the hard way while writing this test). -const rawSources = new Map( - files.map((relPath) => [relPath, readFileSync(join(root, relPath), 'utf8')]), -); -const fileSources = new Map( - [...rawSources].map(([relPath, raw]) => [relPath, stripComments(raw)]), -); - -const appTsRawFull = rawSources.get(APP_TS); -if (!appTsRawFull) throw new Error(`fixture assumption failed: ${APP_TS} not found by the source walk`); +const rawSources: SurfaceLifecycleSourceEntry[] = files.map((relPath) => ({ + filename: relPath, + source: readFileSync(join(root, relPath), 'utf8'), +})); + +const appTsRawFull = rawSources.find((entry) => entry.filename === APP_TS)?.source; +if (appTsRawFull == null) throw new Error(`fixture assumption failed: ${APP_TS} not found by the source walk`); + +// #643 "Measure the surface batch" — five fresh `vitest run` invocations of +// this exact file, instrumented with `performance.now()` around the +// `beforeAll` body below (instrumentation since removed), measured: +// 307.19ms / 273.43ms / 269.27ms / 244.54ms / 243.43ms — worst 307.19ms. +// 3x that is ~922ms, well under the plan's own 10000ms floor, so the floor +// (not the 3x figure) sets this timeout; the one real parser batch over the +// whole `src/**` tree (#643's own point — one process, not one per file) is +// nowhere near the 30000ms this repo already treats as a parser/batching +// performance defect (`tests/unit/clickhouse-http-package-policy.test.js`'s +// own `beforeAll`, by contrast, budgets 60000ms for FOUR real-tree scans). +const SURFACE_BATCH_TIMEOUT_MS = 10000; describe('#590 surface-lifecycle architecture (invariant (k))', () => { it('the coordinator markers exist exactly once each, in order, in src/ui/app.ts', () => { + // Coordinator marker uniqueness/order stays a RAW-text check over + // src/ui/app.ts (rule scope matrix) — the markers are themselves `//` + // comments that intentionally define the coordinator region for every + // AST-based rule below, so their own discovery deliberately never goes + // through the parser. const beginIndex = appTsRawFull.indexOf(BEGIN_MARKER); const endIndex = appTsRawFull.indexOf(END_MARKER); expect(beginIndex).toBeGreaterThan(-1); @@ -83,99 +111,352 @@ describe('#590 surface-lifecycle architecture (invariant (k))', () => { expect(appTsRawFull.indexOf(END_MARKER, endIndex + 1)).toBe(-1); }); - const beginIndex = appTsRawFull.indexOf(BEGIN_MARKER); - const endIndex = appTsRawFull.indexOf(END_MARKER); - // Stripped PER SLICE (comments inside the region — which explain the - // mechanism at length — must not themselves be scanned for forbidden - // tokens; a comment mentioning `disposeShell(` in prose is not a call). - const coordinatorRegion = stripComments(appTsRawFull.slice(beginIndex, endIndex)); - const outsideCoordinator = stripComments( - appTsRawFull.slice(0, beginIndex) + appTsRawFull.slice(endIndex + END_MARKER.length), - ); - - it('the three teardown primitives are declared INSIDE the coordinator region', () => { - expect(coordinatorRegion).toMatch(/\bconst\s+disposeShell\s*=/); - expect(coordinatorRegion).toMatch(/\bconst\s+disposeCurrentSurface\s*=/); - expect(coordinatorRegion).toMatch(/\bconst\s+committedWorkspaceSignal\s*[:=]/); - expect(coordinatorRegion).toMatch(/\bconst\s+mainSurfaceSignal\s*[:=]/); - // ...and NOT redeclared a second time outside it (a re-hoist would leave - // one copy outside even if the marked region also still has one). - expect(outsideCoordinator).not.toMatch(/\bconst\s+disposeShell\s*=/); - expect(outsideCoordinator).not.toMatch(/\bconst\s+disposeCurrentSurface\s*=/); - expect(outsideCoordinator).not.toMatch(/\bconst\s+committedWorkspaceSignal\s*[:=]/); - expect(outsideCoordinator).not.toMatch(/\bconst\s+mainSurfaceSignal\s*[:=]/); + let violations: SourceContractViolation[]; + + beforeAll(() => { + const beginIndex = appTsRawFull.indexOf(BEGIN_MARKER); + const endIndex = appTsRawFull.indexOf(END_MARKER); + violations = findSurfaceLifecycleSourceContractViolations(rawSources, { + appFile: APP_TS, + coordinatorStart: beginIndex, + coordinatorEnd: endIndex, + }); + }, SURFACE_BATCH_TIMEOUT_MS); + + it('the four coordinator-owned declarations are declared inside the coordinator region, and nowhere outside it', () => { + expect(violations.filter((v) => v.rule === 'surface-protected-declaration')).toEqual([]); }); it('no out-of-coordinator call to disposeShell(/disposeCurrentSurface(/shell.dispose( exists in app.ts', () => { - const forbidden = /\bdisposeShell\s*\(|\bdisposeCurrentSurface\s*\(|\bshell\s*\?\.\s*dispose\s*\(|\bshell\.dispose\s*\(/g; - const hits = outsideCoordinator.match(forbidden) ?? []; - expect(hits).toEqual([]); + expect(violations.filter((v) => v.rule === 'surface-teardown-call')).toEqual([]); }); it('no out-of-coordinator .value write names the private signal identifiers, in ANY src file', () => { - // Identifier-anchored (pass-7 finding): matches regardless of the - // right-hand expression, so an alias write - // (`const next = null; committedWorkspaceSignal.value = next;`) is - // caught the same as a literal one. Scoped to app.ts's own - // out-of-coordinator text for app.ts, and to the WHOLE file for every - // other source (the identifiers cannot legally appear there at all, - // since they are never exported). - const pattern = /\b(committedWorkspaceSignal|mainSurfaceSignal)\s*\.\s*value\s*=(?!=)/; - for (const [relPath, source] of fileSources) { - const haystack = relPath === APP_TS ? outsideCoordinator : source; - expect(haystack, `${relPath} must not write the private signal outside the coordinator`).not.toMatch(pattern); - } + expect(violations.filter((v) => v.rule === 'surface-signal-write')).toEqual([]); }); it('no out-of-coordinator `currentWorkspace = null` assignment exists in ANY src file', () => { - // A plain `.currentWorkspace = null` (not `===`/`!==`) — cast-bypassing - // writes (`as never`/`as any`) still match this token, which is exactly - // the defense-in-depth a pure `tsc` check cannot provide. - const pattern = /\.currentWorkspace\s*=\s*null\b(?!\s*[=!]=)/; - for (const [relPath, source] of fileSources) { - const haystack = relPath === APP_TS ? outsideCoordinator : source; - expect(haystack, `${relPath} must not assign currentWorkspace = null outside the coordinator`).not.toMatch(pattern); - } - }); - - // The adjacency hazard (pass-7 finding): no compile-time mechanism can - // foreclose a two-STATEMENT sequence when the second statement is an - // exported function meant to be callable from outside the coordinator. - // Scoped to one function body at a time (bracket-matched from each - // `=> {`/`) {` opener) — same-function-body precision, matching the - // plan's own stated residual-risk boundary (a split across HELPER - // functions is explicitly out of this rule's reach, per §1.9). - function functionBodies(source: string): string[] { - const bodies: string[] = []; - const opener = /(?:=>|\))\s*\{/g; - let match: RegExpExecArray | null; - while ((match = opener.exec(source))) { - const start = match.index + match[0].length - 1; // index of the '{' - let depth = 0; - let i = start; - for (; i < source.length; i += 1) { - if (source[i] === '{') depth += 1; - else if (source[i] === '}') { - depth -= 1; - if (depth === 0) break; - } - } - if (depth === 0) bodies.push(source.slice(start + 1, i)); - } - return bodies; + expect(violations.filter((v) => v.rule === 'surface-current-workspace-null')).toEqual([]); + }); + + it('no ordering scope writes mainSurface/currentWorkspace lexically before calling a retireTo*/retirement hook', () => { + expect(violations.filter((v) => v.rule === 'surface-retirement-ordering')).toEqual([]); + }); +}); + +// ── Synthetic characterization — every check below builds its OWN small +// batch, exercising the analyzer directly (never the real tree) ─────────── + +const CLEAN_COORDINATOR = [ + '// #590-COORDINATOR-BEGIN', + 'const disposeShell = () => {};', + 'const disposeCurrentSurface = () => {};', + 'const committedWorkspaceSignal = { value: null };', + 'const mainSurfaceSignal = { value: null };', + '// #590-COORDINATOR-END', +].join('\n'); + +function surfaceViolations( + appBody: string, + otherSources: SurfaceLifecycleSourceEntry[] = [], +): SourceContractViolation[] { + const appSource = `${appBody}\n${CLEAN_COORDINATOR}\n`; + const beginIndex = appSource.indexOf(BEGIN_MARKER); + const endIndex = appSource.indexOf(END_MARKER); + const sources: SurfaceLifecycleSourceEntry[] = [{ filename: APP_TS, source: appSource }, ...otherSources]; + return findSurfaceLifecycleSourceContractViolations(sources, { + appFile: APP_TS, + coordinatorStart: beginIndex, + coordinatorEnd: endIndex, + }); +} + +function rulesOf(violations: SourceContractViolation[]): string[] { + return violations.map((v) => v.rule); +} + +describe('required lexical sabotage matrix (both analyzers share this shape)', () => { + it('a `//` comment containing a fake block-opener does not hide the real teardown violation that follows', () => { + const body = [ + '// documentation mentioning src/core/**', + 'disposeShell();', + '/* next real block comment */', + ].join('\n'); + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('a `//` comment mentioning a glob-like path does not hide the real violation that follows', () => { + const body = '// src/core/**\ndisposeShell();'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('a legal block comment does not hide the real violation that follows', () => { + const body = '/* a normal, legal block comment */\ndisposeShell();'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('comment-shaped text inside a string literal does not hide the real violation that follows', () => { + const body = "const s = 'comment-shaped /* text';\ndisposeShell();"; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('comment-shaped text inside a template literal does not hide the real violation that follows', () => { + const body = 'const t = `comment-shaped /* text`;\ndisposeShell();'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('a parser-valid regex literal containing comment-shaped characters does not hide the real violation that follows', () => { + const body = 'const r = /a\\/\\*b/;\ndisposeShell();'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('a real forbidden construct immediately following a lexical trap is still caught', () => { + const body = '/*c*/disposeShell();'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-teardown-call'); + }); + + it('forbidden vocabulary appearing only in comments stays clean', () => { + const body = [ + '// this comment mentions disposeShell(, src/core/**, and forbiddenArchitectureViolation', + '/* so does this block comment: disposeCurrentSurface( */', + 'const x = 1;', + ].join('\n'); + expect(surfaceViolations(body)).toEqual([]); + }); +}); + +describe('additional surface sabotage: scope', () => { + it('a teardown call outside the coordinator in app.ts fails', () => { + expect(rulesOf(surfaceViolations('disposeShell();'))).toContain('surface-teardown-call'); + }); + + it('an equivalent teardown-shaped call in an UNRELATED file is not governed by the app-only teardown rule', () => { + const found = surfaceViolations('const x = 1;', [ + { filename: 'src/ui/unrelated.ts', source: 'function f() { disposeShell(); }\n' }, + ]); + expect(found.filter((v) => v.rule === 'surface-teardown-call')).toEqual([]); + }); + + it('a currentWorkspace = null write in an unrelated source file fails (the null rule is tree-wide)', () => { + const found = surfaceViolations('const x = 1;', [ + { filename: 'src/ui/unrelated.ts', source: 'function f() { target.currentWorkspace = null; }\n' }, + ]); + expect(rulesOf(found)).toContain('surface-current-workspace-null'); + }); + + it('a private signal .value write in an unrelated source file fails (the signal rule is tree-wide)', () => { + const found = surfaceViolations('const x = 1;', [ + { filename: 'src/ui/unrelated.ts', source: 'function f() { obj.mainSurfaceSignal.value = next; }\n' }, + ]); + expect(rulesOf(found)).toContain('surface-signal-write'); + }); + + it('an ordering violation in an unrelated source file fails (ordering is tree-wide, unconditional on the coordinator)', () => { + const found = surfaceViolations('const x = 1;', [ + { + filename: 'src/ui/unrelated.ts', + source: 'function f() { app.mainSurface = next; retireToLater(); }\n', + }, + ]); + expect(rulesOf(found)).toContain('surface-retirement-ordering'); + }); +}); + +describe('additional surface sabotage: member chains', () => { + it('obj.committedWorkspaceSignal.value = next; fails', () => { + expect(rulesOf(surfaceViolations('function f() { obj.committedWorkspaceSignal.value = next; }'))).toContain( + 'surface-signal-write', + ); + }); + + it('obj.mainSurfaceSignal.value = next; fails', () => { + expect(rulesOf(surfaceViolations('function f() { obj.mainSurfaceSignal.value = next; }'))).toContain( + 'surface-signal-write', + ); + }); + + it('app.shell.dispose(); fails (member-terminal shell.dispose match through a longer receiver chain)', () => { + expect(rulesOf(surfaceViolations('function f() { app.shell.dispose(); }'))).toContain('surface-teardown-call'); + }); + + it('owner.disposeShell(); fails (member-terminal disposeShell match)', () => { + expect(rulesOf(surfaceViolations('function f() { owner.disposeShell(); }'))).toContain('surface-teardown-call'); + }); +}); + +describe('additional surface sabotage: null wrappers (currentWorkspace = null)', () => { + const wrapped: Array<[string, string]> = [ + ['bare null', 'target.currentWorkspace = null;'], + ['as never', 'target.currentWorkspace = null as never;'], + ['as any', 'target.currentWorkspace = null as any;'], + ['non-null assertion', 'target.currentWorkspace = null!;'], + ['satisfies', 'target.currentWorkspace = null satisfies never;'], + ['parenthesized cast', 'target.currentWorkspace = (null as never);'], + ]; + for (const [label, stmt] of wrapped) { + it(`${label} still fails (a cast-bypassing write must not slip past this defense)`, () => { + expect(rulesOf(surfaceViolations(`function f() { ${stmt} }`))).toContain('surface-current-workspace-null'); + }); } - it('no function body writes mainSurface/currentWorkspace lexically before calling a retireTo*/retirement hook', () => { - const writePattern = /\.(?:mainSurface|currentWorkspace)\s*=(?!=)/; - const retirePattern = /\bretireTo\w*\s*\(/; - for (const [relPath, source] of fileSources) { - for (const body of functionBodies(source)) { - const writeIndex = body.search(writePattern); - if (writeIndex === -1) continue; - const retireIndex = body.search(retirePattern); - const violates = retireIndex !== -1 && writeIndex < retireIndex; - expect(violates, `${relPath} has a mainSurface/currentWorkspace write lexically before a retireTo*() call in one function body`).toBe(false); - } - } + // #643 mandatory addition 3 (pass-5 finding). Today's regex + // (`\.currentWorkspace\s*=\s*null\b(?!\s*[=!]=)`) would actually flag + // `x.currentWorkspace = null ?? y` too — the negative lookahead only + // excludes a following `==`/`!=`, not `??`. This is a DELIBERATE + // precision change, not a preservation: the real invariant this rule + // enforces is "this property was set to a bare null-equivalent value", + // and `??` introduces genuine conditional/fallback semantics that is not + // that — see `unwrapNullEquivalentWrappers`'s own doc comment in + // `build/lib/check-legacy-owners.mjs` for why `??` is not treated as a + // transparent wrapper the way `as`/`!`/`satisfies`/parens are. + it('### Deliberate precision change: `null ?? fallback` is intentionally treated as clean, not a violation', () => { + const found = surfaceViolations('function f() { target.currentWorkspace = null ?? fallback; }'); + expect(found.filter((v) => v.rule === 'surface-current-workspace-null')).toEqual([]); + }); +}); + +describe('additional surface sabotage: ordering', () => { + it('1. write before the first retire fails', () => { + const body = 'function probe() { app.mainSurface = next; retireToLater(); }'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-retirement-ordering'); + }); + + it('2. first retire, then write, then a later retire passes', () => { + const body = 'function probe() { retireToEarly(); app.mainSurface = next; retireToLater(); }'; + expect(surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering')).toEqual([]); + }); + + it('3. an outer write with the first retire only in a NESTED scope fails under the enclosing lexical scope', () => { + const body = 'function probe() { app.mainSurface = next; if (c) { retireToLater(); } }'; + expect(rulesOf(surfaceViolations(body))).toContain('surface-retirement-ordering'); + }); + + it('4. an early outer retire, then a write, then a later NESTED retire passes in the outer scope (the required control-block characterization)', () => { + // The outer function passes because firstRetire < firstWrite, but the + // independent `if` scope fails because ITS first write precedes ITS + // first retire — both facts are true of the SAME source at once. + const body = [ + 'function probe() {', + ' retireToEarly();', + ' if (condition) {', + ' app.mainSurface = next;', + ' retireToLater();', + ' }', + '}', + ].join('\n'); + const found = surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering'); + expect(found).toHaveLength(1); // only the inner `if` scope violates + }); + + it('5. a return-annotated function participates: retire, write, retire passes', () => { + const body = [ + 'export function typed(): void {', + ' retireToEarly();', + ' app.mainSurface = next;', + ' retireToLater();', + '}', + ].join('\n'); + expect(surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering')).toEqual([]); + }); + + it('5b. a return-annotated function participates: write, retire fails (the old textual opener would have missed this scope entirely)', () => { + const body = [ + 'export function typed(): void {', + ' app.mainSurface = next;', + ' retireToLater();', + '}', + ].join('\n'); + expect(rulesOf(surfaceViolations(body))).toContain('surface-retirement-ordering'); + }); + + // 6. Every preserved parenthesized control-block scope, table-driven — + // each must independently fail on its own local write-before-retire, even + // though the ENCLOSING function scope is clean (an early outer retire + // precedes the outer scope's own first write, which is nested). + const controlBlocks: Array<[string, string]> = [ + ['if', 'function probe() { retireToEarly(); if (c) { app.mainSurface = next; retireToLater(); } }'], + ['for', 'function probe() { retireToEarly(); for (let i = 0; i < 1; i++) { app.mainSurface = next; retireToLater(); } }'], + ['for-in', 'function probe() { retireToEarly(); for (const k in obj) { app.mainSurface = next; retireToLater(); } }'], + ['for-of', 'function probe() { retireToEarly(); for (const k of obj) { app.mainSurface = next; retireToLater(); } }'], + ['while', 'function probe() { retireToEarly(); while (c) { app.mainSurface = next; retireToLater(); } }'], + ['switch', 'function probe() { retireToEarly(); switch (v) { case 1: app.mainSurface = next; retireToLater(); break; } }'], + ['catch (e)', 'function probe() { retireToEarly(); try {} catch (e) { app.mainSurface = next; retireToLater(); } }'], + ]; + for (const [label, body] of controlBlocks) { + it(`6. ${label} is an independently checked ordering scope`, () => { + const found = surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering'); + expect(found).toHaveLength(1); // only the control-block scope violates; the outer function scope is clean + }); + } + + // Explicitly NOT independent scopes — the old textual opener never + // recognized these either, so widening to cover them would be a NEW + // enforcement the plan's non-goals forbid. + const nonScopes: Array<[string, string]> = [ + ['else', 'function probe() { retireToEarly(); if (c) {} else { app.mainSurface = next; retireToLater(); } }'], + ['do', 'function probe() { retireToEarly(); do { app.mainSurface = next; retireToLater(); } while (c); }'], + ['try', 'function probe() { retireToEarly(); try { app.mainSurface = next; retireToLater(); } catch {} }'], + ['binding-less catch', 'function probe() { retireToEarly(); try {} catch { app.mainSurface = next; retireToLater(); } }'], + ['bare block', 'function probe() { retireToEarly(); { app.mainSurface = next; retireToLater(); } }'], + ]; + for (const [label, body] of nonScopes) { + it(`6b. ${label} is NOT an independently checked ordering scope`, () => { + // The enclosing function scope's own first retire (retireToEarly) + // precedes its first write (nested inside the non-scope), so the + // whole thing is clean UNLESS the non-scope were wrongly treated as + // independent. + expect(surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering')).toEqual([]); + }); + } + + it('7. braces and a retireToX(-shaped call inside comments/strings/templates/regex create no phantom calls or scopes', () => { + const body = [ + 'function probe() {', + ' // if (x) { app.mainSurface = y; retireToPhantom(); }', + " const s = 'if (x) { app.mainSurface = y; retireToPhantom(); }';", + ' const t = `if (x) { app.mainSurface = y; retireToPhantom(); }`;', + ' const r = /retireTo\\w*\\(/;', + ' retireToEarly();', + ' app.mainSurface = next;', + '}', + ].join('\n'); + // The only REAL write (app.mainSurface) has no real retire after it — + // the phantom text must not manufacture either a call or a scope. + expect(surfaceViolations(body).filter((v) => v.rule === 'surface-retirement-ordering')).toEqual([]); + }); +}); + +describe('coordinator boundary classification', () => { + it('a node whose range straddles the BEGIN marker is a deterministic boundary violation, not silently accepted', () => { + // A multi-line declaration whose opening half sits before the marker and + // whose closing half sits after it — impossible to place wholly inside + // OR wholly outside. + const appSource = [ + 'const disposeShell = (', + '// #590-COORDINATOR-BEGIN', + ') => {};', + 'const disposeCurrentSurface = () => {};', + 'const committedWorkspaceSignal = { value: null };', + 'const mainSurfaceSignal = { value: null };', + '// #590-COORDINATOR-END', + ].join('\n'); + const beginIndex = appSource.indexOf(BEGIN_MARKER); + const endIndex = appSource.indexOf(END_MARKER); + const found = findSurfaceLifecycleSourceContractViolations( + [{ filename: APP_TS, source: appSource }], + { appFile: APP_TS, coordinatorStart: beginIndex, coordinatorEnd: endIndex }, + ); + expect(found.some((v) => v.rule === 'surface-protected-declaration')).toBe(true); + }); +}); + +describe('fail-loud contract', () => { + it('throws when appFile is not present in the supplied source batch', () => { + expect(() => + findSurfaceLifecycleSourceContractViolations( + [{ filename: 'src/ui/other.ts', source: 'const x = 1;\n' }], + { appFile: APP_TS, coordinatorStart: 0, coordinatorEnd: 0 }, + ), + ).toThrow(); }); }); From 4e0ae49d06840304903b20d2d3f45de2ee1d2f6d Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 20:11:19 +0200 Subject: [PATCH 2/3] fix(#643): pin exact rule codes in side-panel sabotage assertions Independent review found nearly every sabotage case in side-panel-source-contract.test.ts asserted only .toHaveLength(1)/ .toEqual([]), never which .rule code was returned. Several guarded files (app-shell.ts most notably, with app-shell-panel-def/ app-shell-panel-id/app-shell-host-accessor) dispatch to multiple distinct rule codes from the same call, so a bug that swapped two rule-code strings in the dispatch table would still pass. Added a rulesOf() helper mirroring surface-lifecycle-arch.test.ts's own precedent and tightened every under-specified assertion to pin the exact expected rule code(s) via a single rulesOf(...).toEqual([...]) assertion. No production logic changed; tightening revealed no latent rule-code bug (today's dispatch table is correct). --- tests/unit/side-panel-source-contract.test.ts | 78 +++++++++++-------- 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/tests/unit/side-panel-source-contract.test.ts b/tests/unit/side-panel-source-contract.test.ts index 8a499492..05db2f92 100644 --- a/tests/unit/side-panel-source-contract.test.ts +++ b/tests/unit/side-panel-source-contract.test.ts @@ -42,6 +42,20 @@ function violations(source: string, filename: string): SourceContractViolation[] return findSidePanelSourceContractViolations(source, filename); } +// #643 review follow-up: mirrors `surface-lifecycle-arch.test.ts`'s own +// `rulesOf` helper. Nearly every sabotage case below expects EXACTLY one +// violation, so `rulesOf(...).toEqual([...])` is the single assertion that +// pins both the count and the exact rule code(s) — no separate `.rule` +// assertion needed. This matters because several of the guarded files +// dispatch to MULTIPLE distinct rule codes from the same +// `findSidePanelSourceContractViolations` call (`app-shell.ts` alone can +// report `app-shell-panel-def`, `app-shell-panel-id`, or +// `app-shell-host-accessor`) — a bug that swapped two of those rule-code +// strings in the dispatch table would still pass a bare `.toHaveLength(1)`. +function rulesOf(vs: SourceContractViolation[]): string[] { + return vs.map((v) => v.rule); +} + const WORKBENCH_SESSION = 'src/ui/workbench/workbench-session.ts'; const APP_PREFERENCES = 'src/application/app-preferences.ts'; const STATE = 'src/state.ts'; @@ -105,32 +119,32 @@ describe('required lexical sabotage matrix (both analyzers share this shape)', ( it('a `//` comment mentioning a glob-like path does not hide the real violation that follows', () => { const source = '// src/core/**\nconst sidePanelViolation = 1;\n'; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('a legal block comment does not hide the real violation that follows', () => { const source = '/* a normal, legal block comment */\nconst sidePanelViolation = 1;\n'; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('comment-shaped text inside a string literal does not hide the real violation that follows', () => { const source = "const s = 'comment-shaped /* text';\nconst sidePanelViolation = 1;\n"; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('comment-shaped text inside a template literal does not hide the real violation that follows', () => { const source = 'const t = `comment-shaped /* text`;\nconst sidePanelViolation = 1;\n'; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('a parser-valid regex literal containing comment-shaped characters does not hide the real violation that follows', () => { const source = 'const r = /a\\/\\*b/;\nconst sidePanelViolation = 1;\n'; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('a real forbidden construct immediately following a lexical trap is still caught', () => { const source = '/*c*/const sidePanelViolation = 1;\n'; - expect(violations(source, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(source, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); it('forbidden vocabulary appearing only in comments stays clean', () => { @@ -154,7 +168,7 @@ describe('additional side-panel sabotage: workbench breadth', () => { ]; for (const [label, code] of cases) { it(`${label} is a violation`, () => { - expect(violations(code, WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations(code, WORKBENCH_SESSION))).toEqual(['workbench-sidepanel-mention']); }); } @@ -172,10 +186,10 @@ describe('additional side-panel sabotage: workbench breadth', () => { }); it('the history comparison rule supports both operand orders and every quote style', () => { - expect(violations("value === 'history';", WORKBENCH_SESSION)).toHaveLength(1); - expect(violations("'history' === value;", WORKBENCH_SESSION)).toHaveLength(1); - expect(violations('value === "history";', WORKBENCH_SESSION)).toHaveLength(1); - expect(violations('value === `history`;', WORKBENCH_SESSION)).toHaveLength(1); + expect(rulesOf(violations("value === 'history';", WORKBENCH_SESSION))).toEqual(['workbench-history-compare']); + expect(rulesOf(violations("'history' === value;", WORKBENCH_SESSION))).toEqual(['workbench-history-compare']); + expect(rulesOf(violations('value === "history";', WORKBENCH_SESSION))).toEqual(['workbench-history-compare']); + expect(rulesOf(violations('value === `history`;', WORKBENCH_SESSION))).toEqual(['workbench-history-compare']); }); it('a string literal that merely LOOKS like the history comparison (not a real equality) stays clean', () => { @@ -186,18 +200,18 @@ describe('additional side-panel sabotage: workbench breadth', () => { describe('additional side-panel sabotage: literal-value precision (app-preferences/state/app-shell ids)', () => { it('app-preferences.ts: an exact protected id literal fails, in every quote style, but a longer literal merely containing one stays clean', () => { - expect(violations('const id = "library";', APP_PREFERENCES)).toHaveLength(1); - expect(violations('const id = `library`;', APP_PREFERENCES)).toHaveLength(1); + expect(rulesOf(violations('const id = "library";', APP_PREFERENCES))).toEqual(['app-preferences-panel-id']); + expect(rulesOf(violations('const id = `library`;', APP_PREFERENCES))).toEqual(['app-preferences-panel-id']); expect(violations("const note = \"pick 'library' now\";", APP_PREFERENCES)).toEqual([]); }); it('state.ts: an exact protected label literal fails, but a longer literal merely containing one stays clean', () => { - expect(violations('const label = "History";', STATE)).toHaveLength(1); + expect(rulesOf(violations('const label = "History";', STATE))).toEqual(['state-panel-label']); expect(violations("const note2 = \"old 'History' label\";", STATE)).toEqual([]); }); it('app-shell.ts panel ids: an exact protected id literal fails, but a longer literal merely containing one stays clean', () => { - expect(violations('const panel = `databases`;', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('const panel = `databases`;', APP_SHELL))).toEqual(['app-shell-panel-id']); expect(violations("const note3 = \"pick 'databases'\";", APP_SHELL)).toEqual([]); }); @@ -211,24 +225,24 @@ describe('additional side-panel sabotage: literal-value precision (app-preferenc // `LiteralTypeNode`'s own literal) is caught on exactly the same terms as // an expression-position one. it('app-preferences.ts: a TYPE-position literal ("type Pref = \'library\';") still fails', () => { - expect(violations("type Pref = 'library';", APP_PREFERENCES)).toHaveLength(1); + expect(rulesOf(violations("type Pref = 'library';", APP_PREFERENCES))).toEqual(['app-preferences-panel-id']); }); it('state.ts: a TYPE-position literal ("type X = \'History\';") still fails', () => { - expect(violations("type X = 'History';", STATE)).toHaveLength(1); + expect(rulesOf(violations("type X = 'History';", STATE))).toEqual(['state-panel-label']); }); it('app-shell.ts panel ids: a TYPE-position literal ("type X = \'databases\';") still fails', () => { - expect(violations("type X = 'databases';", APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations("type X = 'databases';", APP_SHELL))).toEqual(['app-shell-panel-id']); }); }); describe('additional side-panel sabotage: app.ts comparison', () => { it('supports the full receiver chain, both operand orders, and every quote style', () => { - expect(violations("app.shell.sidePanel.value === 'saved';", APP)).toHaveLength(1); - expect(violations("'saved' === app.shell.sidePanel.value;", APP)).toHaveLength(1); - expect(violations('sidePanel.value === "history";', APP)).toHaveLength(1); - expect(violations('sidePanel.value === `library`;', APP)).toHaveLength(1); + expect(rulesOf(violations("app.shell.sidePanel.value === 'saved';", APP))).toEqual(['app-side-panel-comparison']); + expect(rulesOf(violations("'saved' === app.shell.sidePanel.value;", APP))).toEqual(['app-side-panel-comparison']); + expect(rulesOf(violations('sidePanel.value === "history";', APP))).toEqual(['app-side-panel-comparison']); + expect(rulesOf(violations('sidePanel.value === `library`;', APP))).toEqual(['app-side-panel-comparison']); }); it('a string literal that merely LOOKS like the comparison (not a real equality) stays clean', () => { @@ -239,11 +253,11 @@ describe('additional side-panel sabotage: app.ts comparison', () => { describe('additional side-panel sabotage: panel defs and hosts (app-shell.ts)', () => { it('a concrete panel-def identifier reference is a violation', () => { - expect(violations('const x = databasesPanelDef;', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('const x = databasesPanelDef;', APP_SHELL))).toEqual(['app-shell-panel-def']); }); it('a concrete panel-def spelling in a real literal token is a violation', () => { - expect(violations('const x = "dashboardsPanelDef";', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('const x = "dashboardsPanelDef";', APP_SHELL))).toEqual(['app-shell-panel-def']); }); it('a comment naming a panel-def symbol stays clean', () => { @@ -251,27 +265,29 @@ describe('additional side-panel sabotage: panel defs and hosts (app-shell.ts)', }); it('dot host access is a violation', () => { - expect(violations('host.databasesHost;', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('host.databasesHost;', APP_SHELL))).toEqual(['app-shell-host-accessor']); }); it('optional host access is a violation', () => { - expect(violations('host?.dashboardsHost;', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('host?.dashboardsHost;', APP_SHELL))).toEqual(['app-shell-host-accessor']); }); it('destructuring a host name is a violation', () => { - expect(violations('const { databasesHost } = hosts;', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('const { databasesHost } = hosts;', APP_SHELL))).toEqual(['app-shell-host-accessor']); }); it('string element access naming a host is a violation', () => { - expect(violations("host['dashboardsHost'];", APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations("host['dashboardsHost'];", APP_SHELL))).toEqual(['app-shell-host-accessor']); }); it('template element access naming a host is a violation', () => { - expect(violations('host[`databasesHost`];', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('host[`databasesHost`];', APP_SHELL))).toEqual(['app-shell-host-accessor']); }); it('a contiguous ".databasesHost" spelling inside a literal token is a violation (preserving today\'s broad substring behavior)', () => { - expect(violations('const msg = "call host.databasesHost please";', APP_SHELL)).toHaveLength(1); + expect(rulesOf(violations('const msg = "call host.databasesHost please";', APP_SHELL))).toEqual([ + 'app-shell-host-accessor', + ]); }); it('a comment-only host spelling stays clean', () => { @@ -291,7 +307,7 @@ describe('additional side-panel sabotage: side-panels.ts type aliases', () => { }); it('a plain protected literal in a type alias fails', () => { - expect(violations("type Probe = 'library';", SIDE_PANELS_CORE)).toHaveLength(1); + expect(rulesOf(violations("type Probe = 'library';", SIDE_PANELS_CORE))).toEqual(['side-panels-type-alias']); }); it('a file with zero type alias declarations at all is itself a violation (a total-removal regression must not read as clean)', () => { From 94c4c0105dc6e9b87b849b3c5c93c264fce13c89 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Mon, 10 Aug 2026 20:40:08 +0200 Subject: [PATCH 3/3] fix(#643): address review pass 1 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - protectedDeclarationViolations() now requires a const-flagged VariableDeclarationList, not any VariableDeclaration: a let/var rewrite of disposeShell/disposeCurrentSurface/committedWorkspaceSignal/ mainSurfaceSignal now fails the guard instead of passing undetected. - Its checked range now anchors at the declaration list's own start (the const/let/var keyword's position) instead of the VariableDeclaration node's start (the binding identifier) — a straddle whose keyword sits outside the coordinator and whose binding sits inside is now correctly classified as a straddle violation rather than misread as fully "inside". - sidePanelsTypeAliasViolations() now also walks each TypeAliasDeclaration's typeParameters, not just its .type RHS, so a protected literal confined to a generic constraint/default (`type Probe = T;`) is caught the same way a literal in the RHS already was. - Added table-driven let/var sabotage tests (all four protected names, both keywords), a keyword-before-marker/binding-after-marker straddle test, and rule-code-pinned extends-constraint/default-clause sabotage tests for the type-alias check. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- build/lib/check-legacy-owners.mjs | 78 +++++++++++++------ tests/unit/side-panel-source-contract.test.ts | 19 +++++ tests/unit/surface-lifecycle-arch.test.ts | 67 ++++++++++++++++ 3 files changed, 142 insertions(+), 22 deletions(-) diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index 011a4ea5..344b958e 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -60,7 +60,7 @@ import path from 'node:path'; import { API } from 'typescript/unstable/sync'; import { createVirtualFileSystem } from 'typescript/unstable/fs'; -import { SyntaxKind } from 'typescript/unstable/ast'; +import { NodeFlags, SyntaxKind } from 'typescript/unstable/ast'; import * as is from 'typescript/unstable/ast/is'; /** Every symbol Phase 3 moved out of the legacy owners into the package @@ -1513,23 +1513,24 @@ function appShellHostAccessorViolations(sourceFile, filename) { * one alias to exist at all (a total-removal regression — deleting every * derived pane-id type alias — must not silently read as "zero violations * found"), and flags any protected literal panel id sitting anywhere in an - * alias's OWN `.type` subtree — deliberately scoped to that subtree, not - * the whole file, because the file's real, authoritative `SIDE_PANELS` - * manifest array legitimately spells these exact literals (`{ id: - * 'databases', pane: 'upper' }`) outside any type alias, and must stay - * clean. A defaulted generic type parameter (`type Probe = - * T | 'databases';`) does not exempt the alias from either check — the walk - * finds the `TypeAliasDeclaration` node itself regardless of its type - * parameters, then walks its `.type` unconditionally. */ + * alias's `.type` subtree OR its `.typeParameters` subtree (each type + * parameter's own `extends`/default clause) — deliberately scoped to those + * two subtrees, not the whole file, because the file's real, authoritative + * `SIDE_PANELS` manifest array legitimately spells these exact literals + * (`{ id: 'databases', pane: 'upper' }`) outside any type alias, and must + * stay clean. A defaulted generic type parameter (`type Probe = T | 'databases';`) does not exempt the alias from either + * check, and neither does a literal sitting ONLY inside a type parameter's + * own constraint/default and never in `.type` at all (`type Probe = T;`) — pass-2 finding: the original walk covered `.type` + * but never `.typeParameters`, so a constraint-only literal was invisible. */ function sidePanelsTypeAliasViolations(sourceFile, filename) { const targets = new Set(SIDE_PANEL_TYPE_ALIAS_IDS); const violations = []; let aliasCount = 0; - walkTree(sourceFile, (node) => { - if (node.kind !== SyntaxKind.TypeAliasDeclaration) return; - aliasCount += 1; - const aliasName = node.name.text; - walkTree(node.type, (inner) => { + const scanSubtree = (root, aliasName) => { + if (!root) return; + walkTree(root, (inner) => { if (exactLiteralMatch(inner, targets)) { violations.push(makeViolation( 'side-panels-type-alias', filename, inner.getStart(sourceFile), @@ -1537,6 +1538,17 @@ function sidePanelsTypeAliasViolations(sourceFile, filename) { )); } }); + }; + walkTree(sourceFile, (node) => { + if (node.kind !== SyntaxKind.TypeAliasDeclaration) return; + aliasCount += 1; + const aliasName = node.name.text; + scanSubtree(node.type, aliasName); + if (node.typeParameters) { + for (const typeParam of node.typeParameters) { + scanSubtree(typeParam, aliasName); + } + } }); if (aliasCount === 0) { violations.push(makeViolation( @@ -1624,11 +1636,22 @@ function coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd) { * `toMatch(inside)`/`not.toMatch(outside)` pair: a symbol that disappears * from the file ENTIRELY must not silently read as "zero violations", the * same reasoning `sidePanelsTypeAliasViolations`'s alias-count guard - * applies above. */ + * applies above. Each must ALSO specifically be a `const` (pass-1 finding): + * the retired regex test asserted `toMatch(/\bconst\s+disposeShell\s*=/)` + * etc. — `const` was part of the invariant, not incidental — so a + * `let`/`var` rewrite of any of these four must fail exactly like a wrong + * location would. */ const PROTECTED_DECLARATION_NAMES = Object.freeze([ 'disposeShell', 'disposeCurrentSurface', 'committedWorkspaceSignal', 'mainSurfaceSignal', ]); +/** The non-`const` keyword an offending `VariableDeclarationList`'s flags + * spell — `NodeFlags.Let` set means `let`, otherwise (no block-scoped flag + * at all) it's `var`. Never called for a `const` list. */ +function nonConstDeclarationKeyword(declarationListFlags) { + return (declarationListFlags & NodeFlags.Let) !== 0 ? 'let' : 'var'; +} + function protectedDeclarationViolations(appSourceFile, appFile, coordinatorStart, coordinatorEnd) { const violations = []; const foundInside = new Set(); @@ -1636,17 +1659,28 @@ function protectedDeclarationViolations(appSourceFile, appFile, coordinatorStart if (node.kind !== SyntaxKind.VariableDeclaration || node.name.kind !== SyntaxKind.Identifier) return; const name = node.name.text; if (!PROTECTED_DECLARATION_NAMES.includes(name)) return; - const start = node.getStart(appSourceFile); + const declarationList = node.parent; + const hasDeclarationList = declarationList != null && declarationList.kind === SyntaxKind.VariableDeclarationList; + const isConst = hasDeclarationList && (declarationList.flags & NodeFlags.Const) !== 0; + // Anchor the checked range at the declaration list's OWN start — which + // IS the `const`/`let`/`var` keyword's position — never at the + // `VariableDeclaration` node's own start (the binding identifier, which + // begins strictly AFTER the keyword): otherwise a straddle whose keyword + // sits outside the coordinator and whose binding sits inside would + // misclassify as fully 'inside' (pass-1 finding). + const start = hasDeclarationList ? declarationList.getStart(appSourceFile) : node.getStart(appSourceFile); const end = node.getEnd(); const placement = coordinatorPlacement(start, end, coordinatorStart, coordinatorEnd); - if (placement === 'inside') { + if (placement === 'inside' && isConst) { foundInside.add(name); - } else { - violations.push(makeViolation( - 'surface-protected-declaration', appFile, start, - `"${name}" is declared ${placement} the coordinator region`, - )); + return; } + const detail = isConst + ? `"${name}" is declared ${placement} the coordinator region` + : `"${name}" must be declared "const", not "${ + hasDeclarationList ? nonConstDeclarationKeyword(declarationList.flags) : 'a non-declaration-list binding' + }"`; + violations.push(makeViolation('surface-protected-declaration', appFile, start, detail)); }); for (const name of PROTECTED_DECLARATION_NAMES) { if (!foundInside.has(name)) { diff --git a/tests/unit/side-panel-source-contract.test.ts b/tests/unit/side-panel-source-contract.test.ts index 05db2f92..1050c077 100644 --- a/tests/unit/side-panel-source-contract.test.ts +++ b/tests/unit/side-panel-source-contract.test.ts @@ -306,6 +306,25 @@ describe('additional side-panel sabotage: side-panels.ts type aliases', () => { expect(found[0]!.rule).toBe('side-panels-type-alias'); }); + // Pass-2 review finding: a protected literal confined ENTIRELY to a type + // parameter's own `extends` constraint (never appearing in the alias's + // `.type` RHS at all) functions as a hand-written panel-id allowlist + // bypass exactly like a literal in the RHS does — the guarded file itself + // uses this exact `

` shape in production + // (src/core/side-panels.ts's `PanelIdInPane`), so this is a realistic + // revert shape, not a contrived one. + it('a protected literal confined to a type parameter\'s own extends-constraint is not exempt either', () => { + const found = violations("type Probe = T;", SIDE_PANELS_CORE); + expect(found).toHaveLength(1); + expect(found[0]!.rule).toBe('side-panels-type-alias'); + }); + + it('a protected literal confined to a type parameter\'s own default clause is not exempt either', () => { + const found = violations("type Probe = T;", SIDE_PANELS_CORE); + expect(found).toHaveLength(1); + expect(found[0]!.rule).toBe('side-panels-type-alias'); + }); + it('a plain protected literal in a type alias fails', () => { expect(rulesOf(violations("type Probe = 'library';", SIDE_PANELS_CORE))).toEqual(['side-panels-type-alias']); }); diff --git a/tests/unit/surface-lifecycle-arch.test.ts b/tests/unit/surface-lifecycle-arch.test.ts index 76239bc4..89d8b222 100644 --- a/tests/unit/surface-lifecycle-arch.test.ts +++ b/tests/unit/surface-lifecycle-arch.test.ts @@ -448,6 +448,73 @@ describe('coordinator boundary classification', () => { ); expect(found.some((v) => v.rule === 'surface-protected-declaration')).toBe(true); }); + + // Pass-1 review finding: the ABOVE straddle test happens to keep the + // `VariableDeclaration` node's own start (the binding identifier, + // `disposeShell`) before the marker in both halves — the `const` keyword + // and the identifier are never on opposite sides of the marker there, so + // it never exercised the identifier-vs-keyword position gap. THIS test + // does: the `const` keyword sits before the marker, and the binding + // identifier plus the rest of the declaration sit after it — a real + // straddle (keyword outside, binding inside) that a check anchored at the + // identifier's own start would misclassify as fully 'inside' the + // coordinator (since the identifier's start is >= coordinatorStart and its + // end is <= coordinatorEnd), silently letting the sabotage through. + it('a declaration whose "const" keyword sits before the BEGIN marker and whose binding sits after it is a straddle, not "inside"', () => { + const appSource = [ + 'const', + '// #590-COORDINATOR-BEGIN', + 'disposeShell = () => {};', + 'const disposeCurrentSurface = () => {};', + 'const committedWorkspaceSignal = { value: null };', + 'const mainSurfaceSignal = { value: null };', + '// #590-COORDINATOR-END', + ].join('\n'); + const beginIndex = appSource.indexOf(BEGIN_MARKER); + const endIndex = appSource.indexOf(END_MARKER); + const found = findSurfaceLifecycleSourceContractViolations( + [{ filename: APP_TS, source: appSource }], + { appFile: APP_TS, coordinatorStart: beginIndex, coordinatorEnd: endIndex }, + ); + expect(found.some((v) => v.rule === 'surface-protected-declaration')).toBe(true); + }); +}); + +describe('coordinator declarations must specifically be `const` (pass-1 finding)', () => { + // The retired regex test asserted `toMatch(/\bconst\s+disposeShell\s*=/)` + // etc. for all four names — `const` was part of the invariant, and #643's + // parser port silently dropped that requirement. Table-driven over all + // four protected names and both non-const keywords: a `let`/`var` rewrite, + // still positioned exactly where the real `const` declaration was (fully + // inside the coordinator region), must still fail this guard. + const NAMES = ['disposeShell', 'disposeCurrentSurface', 'committedWorkspaceSignal', 'mainSurfaceSignal']; + const KEYWORDS = ['let', 'var'] as const; + + function coordinatorWith(sabotagedName: string, keyword: string): string { + const decls = [ + ['disposeShell', 'const disposeShell = () => {};'], + ['disposeCurrentSurface', 'const disposeCurrentSurface = () => {};'], + ['committedWorkspaceSignal', 'const committedWorkspaceSignal = { value: null };'], + ['mainSurfaceSignal', 'const mainSurfaceSignal = { value: null };'], + ] as const; + const lines = decls.map(([name, decl]) => (name === sabotagedName ? decl.replace('const', keyword) : decl)); + return ['// #590-COORDINATOR-BEGIN', ...lines, '// #590-COORDINATOR-END'].join('\n'); + } + + for (const name of NAMES) { + for (const keyword of KEYWORDS) { + it(`"${keyword} ${name}" inside the coordinator region still fails ("${name}" must be const)`, () => { + const appSource = coordinatorWith(name, keyword); + const beginIndex = appSource.indexOf(BEGIN_MARKER); + const endIndex = appSource.indexOf(END_MARKER); + const found = findSurfaceLifecycleSourceContractViolations( + [{ filename: APP_TS, source: appSource }], + { appFile: APP_TS, coordinatorStart: beginIndex, coordinatorEnd: endIndex }, + ); + expect(found.some((v) => v.rule === 'surface-protected-declaration')).toBe(true); + }); + } + } }); describe('fail-loud contract', () => {