fix(#643): replace regex comment stripping with parser-backed architecture source contracts

[BorisTyshkevich]

What & why

tests/unit/side-panel-source-contract.test.ts and tests/unit/surface-lifecycle-arch.test.ts
both preprocessed TypeScript with the same unsound 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.

Following the same lesson already applied to production architecture checks in #630/#642
(don't patch one more regex — move to the real TypeScript-parser-backed machinery), both
suites now call two new named analyzers in build/lib/check-legacy-owners.mjs
(findSidePanelSourceContractViolations/findSurfaceLifecycleSourceContractViolations),
sharing the same 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. A new .d.mts plain-data declaration
boundary keeps compiler internals (SourceFile/Node/SyntaxKind) from ever crossing
into the strict-.ts test files.

Several rules deliberately gained precision along the way (documented in each rule's own
test comments and the CHANGELOG): 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) for the actual protected
value; the history/sidePanel.value comparison rules now support both operand orders;
currentWorkspace = null ?? fallback is now deliberately treated as clean.

The plan-author/review loop (ChatGPT-author, Fable/high-review) hit its 5-pass cap without
a final approval — every prior round's findings were accepted and incorporated, and the
final round's 4 residual findings were minor/fixable precision issues. The human owner
reviewed them directly and approved folding them in as mandatory implementation
requirements rather than spending a 6th review round; all four are implemented and tested
(see commit ce96363's message and the CHANGELOG entry). An internal Sonnet review pass
then found one real test-quality gap (most side-panel sabotage assertions only checked
violation count, not the exact .rule code, which could mask a rule-code mislabeling
bug for files dispatching multiple distinct codes) — fixed in the second commit 4e0ae49.

No src/** production code, dependency, or runtime behavior changed.

Closes #643

Checklist

• npm test passes (the per-file coverage gate is non-negotiable)
• Tests added/updated in the same change as the code
• npm run build succeeds (single-file dist/sql.html)
• Layers kept honest — n/a, this is a build-tooling/test-only change
• No new runtime dependency
• README / CHANGELOG.md ([Unreleased]) updated
• Reconciled affected tracked work — no roadmap/ADR impact; issue Replace unsound regex comment stripping in two architecture source-contract tests #643 itself is closed by this PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 4e0ae49d06840304903b20d2d3f45de2ee1d2f6d

Major issues

1. surface-protected-declaration no longer enforces the retired contract's const requirement.

   • Risk score: 84/100
   • tests/unit/surface-lifecycle-arch.test.ts on the base branch required const disposeShell, const disposeCurrentSurface, const committedWorkspaceSignal, and const mainSurfaceSignal inside the coordinator, and no such const declaration outside it.
   • The new protectedDeclarationViolations() accepts any VariableDeclaration with one of those names and marks it present based on the VariableDeclaration node's range. Therefore changing an in-region const disposeShell = ... to let disposeShell = ... (or var) now reads as clean even though the retired test went red. It also means a marker inserted between the const keyword and the identifier can make the narrower declaration node look inside although the old source contract did not see a complete const <name> declaration inside.
   • Fix: require the matching declaration's VariableDeclarationList to be const (using the TS AST flags/predicate), and use the declaration-list/statement range for coordinator placement so the const keyword cannot sit across the marker. Add synthetic sabotage for let/var replacements and for a declaration straddling the begin marker. Expected rule: surface-protected-declaration.

2. The new type-alias walker drops literals in generic constraints that the retired test rejected.

   • Risk score: 78/100
   • The retired typeAliasStatements regex captured the whole alias statement for ordinary generic clauses without a default and then rejected protected quoted IDs anywhere in that statement. So type Probe<T extends 'databases'> = T; fails the old source contract.
   • sidePanelsTypeAliasViolations() now walks only node.type, not node.typeParameters, so that same manual panel-ID allowlist in a generic constraint passes. This conflicts with the test's stated invariant that side-panels.ts declares no type alias containing a literal panel-ID string.
   • Fix: inspect the alias's type-parameter constraints/defaults as well as the RHS (or walk the complete TypeAliasDeclaration syntax subtree while keeping comments/trivia excluded). Add a sabotage case such as type Probe<T extends 'databases'> = T; and pin side-panels-type-alias.

Focused checks

• The withParsedSource compatibility wrapper retains the existing callback shape and try/finally parser cleanup; on this exact head, CI also passed check:arch, check:types, check-boundaries-dynamic-imports, clickhouse-http-package-policy, and dashboard-boundaries, so I found no separate Epic: extract the Fetch-native ClickHouse client into a reusable package #630/check-boundaries generic RULES do not fail closed on computed dynamic imports #642 lifetime/regression issue.
• collectOrderingScopes() matches the retired (?:=>|\))\s*\{ behavior for the reviewed control-flow table: function-like blocks plus if then-block, for/for-in/for-of/while/with, switch, and bound catch, while excluding else/do/try/finally/binding-less catch/bare blocks as independent scopes.
• build/lib/check-legacy-owners.d.mts exposes only strings/numbers/plain source-entry/options/violation DTOs; no compiler AST types cross the strict-TS boundary.
• Exact-head CI is green: surface-lifecycle-arch.test.ts (51 tests), side-panel-source-contract.test.ts (47 tests), and the full root suite (7,326 tests) passed. I could not independently execute a local checkout because this review environment's direct git clone path could not resolve github.com; the canonical PR API and exact-head CI logs were available and were used instead.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 4e0ae49d06840304903b20d2d3f45de2ee1d2f6d
Reviewed head: 94c4c0105dc6e9b87b849b3c5c93c264fce13c89

Pass-1 findings

1. RESOLVED — protected coordinator declarations now retain the const contract and keyword-aware boundary placement.

   • protectedDeclarationViolations() now requires the containing VariableDeclarationList to carry NodeFlags.Const before a protected name can count as an in-region declaration.
   • Coordinator placement now starts at the declaration-list start (the const/let/var keyword), so a keyword-before-BEGIN / binding-after-BEGIN declaration is a straddle rather than silently reading as inside.
   • The surface suite adds table-driven let/var sabotage for all four protected names plus the keyword/binding marker-straddle case.

2. RESOLVED — type-alias generic constraints/defaults are now inspected.

   • sidePanelsTypeAliasViolations() still scans the alias RHS and now also walks each typeParameter, covering both extends constraints and defaults.
   • The side-panel suite pins side-panels-type-alias for both type Probe<T extends 'databases'> = T; and type Probe<T = 'library'> = T;.

Focused re-review

• withParsedSources / withParsedSource: no regression found. The one-entry wrapper still invokes the existing callback with its SourceFile while the API is live, propagates parse/callback failures, and closes the native API in finally. The pre-existing Epic: extract the Fetch-native ClickHouse client into a reusable package #630/check-boundaries generic RULES do not fail closed on computed dynamic imports #642 mirror suites are green on this PR head.
• Ordering scopes: no mismatch found. The implementation preserves the retired textual opener's independent scopes for block-bodied function-like nodes and parenthesized if/for/for-in/for-of/while/with/switch/bound-catch, while not inventing independent else/do/try/finally/binding-less-catch/bare-block scopes. Nested descendants remain visible to enclosing scopes, preserving first-write/first-retire semantics.
• Assertion strength: no remaining count-only masking issue found where a different rule code could satisfy the assertion. Surface tests either filter/pin the relevant rule before counting or explicitly test for that rule; the newly added side-panel cases pin side-panels-type-alias.
• Strict-TS boundary: build/lib/check-legacy-owners.d.mts remains plain-data only; no SourceFile, compiler Node, SyntaxKind, or other compiler object crosses into the .ts tests.

Full updated PR / validation

The PR still changes the same five files; the new head is exactly one commit ahead of pass 1 and that commit changes only build/lib/check-legacy-owners.mjs plus the two architecture suites. I found no new actionable regression in the unchanged portions of the complete PR.

PR CI for this head's merge commit is green: check:arch, check:types, both source-contract suites (surface-lifecycle-arch.test.ts: 60 tests; side-panel-source-contract.test.ts: 49 tests), the #642/package-policy mirrors, the full root suite (7,337 tests), and the build all passed. I could not independently run a local checkout because this review environment's direct GitHub clone path cannot resolve github.com; I used the canonical PR/commit data and exact-head PR CI instead.

No new actionable findings.

VERDICT: SHIP