Replace unsound regex comment stripping in two architecture source-contract tests

[BorisTyshkevich]

Status

Triaged: confirmed architecture-test defect.

This is architecture hardening, not a known user-facing product regression. The affected tests are intended to fail closed when forbidden source shapes reappear, but their hand-rolled comment stripping can delete real code before the assertions inspect it.

Summary

Two source-level architecture tests currently preprocess TypeScript with the same two-pass regex comment stripper:

• tests/unit/side-panel-source-contract.test.ts
• tests/unit/surface-lifecycle-arch.test.ts

Both do effectively:

source
  .replace(/\/\*[\s\S]*?\*\//g, '')
  .replace(/(^|[^:"'`])\/\/.*$/gm, '$1');

This is unsound because the block-comment pass runs before line comments have been removed.

A /*-shaped substring inside a real // comment can therefore be treated as a block-comment opener. The first pass then consumes everything until the next genuine */, including real production code between those points. The architecture assertion sees the already-damaged source and can silently miss the violation it exists to detect.

Example shape:

// documentation mentioning `src/core/**`
forbiddenArchitectureViolation();
/* next real block comment */

The /** substring inside the line comment can act as the spurious opener during the first pass, causing forbiddenArchitectureViolation() to disappear from the string under test.

Affected code

tests/unit/side-panel-source-contract.test.ts

Its stripComments() helper is applied before assertions that forbid panel-specific identifiers, labels, host accessors, and other concrete composition knowledge from regrowing in architecture-sensitive files.

A false negative here weakens the executable contract introduced for the side-panel registry work.

tests/unit/surface-lifecycle-arch.test.ts

Its stripComments() helper is applied to production source slices before checking the surface-retirement coordinator invariants.

A false negative here can hide an out-of-region lifecycle write/call or another source-level violation that the test is specifically intended to catch.

Why the previously suggested regex fix is no longer sufficient

This issue was originally filed after the same two-pass defect was found in the #630 Phase 3 legacy-owner architecture rule. That specific rule was initially patched by changing the two replacements to one alternation-based regex:

source.replace(/\/\*[\s\S]*?\*\/|\/\/.*/g, '')

That fixes the exact // ... /* ordering bug because the line comment is consumed before a later comment-shaped substring on the same line can be interpreted independently.

However, subsequent #630 review found additional lexical bypasses in hand-rolled source scanners involving strings, templates, regex literals, and regex-vs-division ambiguity. The corresponding production architecture checks were therefore moved to the repository's real TypeScript-parser-backed machinery in build/lib/check-legacy-owners.mjs rather than continuing to accumulate regex patches.

These two tests should follow that lesson. Replacing one fragile regex with a slightly less fragile regex would fix the reported reproduction but would leave the tests dependent on another incomplete JavaScript/TypeScript lexer.

Intended fix

Remove the regex-based comment stripping from both architecture tests and make their source inspection parser-backed.

Preferred direction:

1. Parse the relevant TypeScript source using the existing real TypeScript parser infrastructure already used by repository architecture checks.
2. Inspect only syntactic code nodes/tokens relevant to each invariant, so comments and literal contents are not part of the search domain unless an invariant explicitly needs them.
3. Share the smallest reusable parser helper needed by these tests rather than adding another independent scanner.
4. Preserve the current test intent and failure diagnostics: this is a strengthening of lexical correctness, not a redesign of the architecture contracts.

If a full AST predicate for one assertion would be disproportionately complex, a parser-backed extraction of code ranges/tokens is still preferable to reconstructing JavaScript lexical rules with regexes.

Scope audit

A repository search for the same comment-stripping pattern found these two TypeScript architecture tests as the relevant remaining instances.

tests/unit/typography-contract.test.js also strips /* ... */ comments, but it operates on CSS and does not combine that pass with JavaScript // comment handling, so it is not the same defect.

Required sabotage tests

Add explicit regressions proving the source-contract checks cannot be bypassed by comment/literal syntax surrounding a real violation.

At minimum cover:

• // comment containing a /*-shaped substring before a forbidden code construct;
• // comment containing a glob/doc example such as src/core/**;
• normal /* ... */ comments surrounding legal code;
• string literal containing // and /* ... */ text;
• template literal containing comment-shaped text;
• regex literal containing // or /*-like text where syntactically legal;
• the real forbidden identifier/call immediately after those constructs still causes the architecture test to fail;
• comments containing the same forbidden vocabulary remain ignored.

The tests should exercise the architecture predicate/helper directly with synthetic source where practical, so the bypass is locked independently of today's production files.

Acceptance criteria

• tests/unit/side-panel-source-contract.test.ts no longer relies on regex comment stripping.
• tests/unit/surface-lifecycle-arch.test.ts no longer relies on regex comment stripping.
• Both tests use real TypeScript syntax information, directly or through a shared parser-backed helper.
• A /*-shaped substring inside a // comment cannot hide real code from either architecture test.
• Comment-shaped text inside strings, templates, or regex literals cannot corrupt the inspected code region.
• Forbidden architecture constructs in real code still fail the corresponding test.
• Forbidden vocabulary appearing only in comments remains ignored.
• No second hand-rolled JavaScript/TypeScript lexer is introduced.
• The existing architecture intent and diagnostics remain understandable and deterministic.
• npm run check:types, npm run check:arch, and npm test remain green.

Non-goals

• Rewriting unrelated architecture checks.
• Building a generic static-analysis framework.
• Changing the side-panel registry or surface-lifecycle architecture itself.
• Changing production runtime behavior.
• Reworking the CSS-only comment stripping in typography-contract.test.js unless an independent defect is found there.

History

Originally discovered as a pre-existing, out-of-scope finding during the #630 Phase 3 high-risk review.

Commit 9ff449e fixed the same immediate two-pass failure mode in the Phase 3 rule with a one-pass alternation regex. Later #630 phases then replaced the related hand-rolled lexical architecture scanner with real TypeScript parsing after further bypasses were found.

This issue now tracks applying that stronger, final lesson to the two remaining architecture source-contract tests.

[BorisTyshkevich]

Triage: confirmed, architecture-test soundness bug; not a user-facing runtime regression.

The reported two-pass failure mode is real in both current helpers. In particular, surface-lifecycle-arch.test.ts scans all src/**, and current source already contains line-comment prose such as src/core/**; a sabotage probe placed after such an opener and before the next genuine */ can be deleted by the block-comment pass before the // pass sees it.

The original suggested fix should not be the endpoint now. Since this issue was filed, the related #630 legacy-owner rule was hardened further: the repository retired its hand-rolled lexical scanner after repeated bypasses involving strings/templates/regex literals and now uses the real TypeScript parser in build/lib/check-legacy-owners.mjs. Replacing these two helpers with /\/\*[\s\S]*?\*\/|\/\/.*/g fixes this exact ordering bug but remains unsound for comment-shaped text inside strings/templates/regex literals.

Recommended scope:

• remove the hand-rolled stripComments() dependency from tests/unit/side-panel-source-contract.test.ts and tests/unit/surface-lifecycle-arch.test.ts;
• express the guarded invariants using the existing real-TypeScript-parser approach (shared helper where practical), rather than adding another lexical regex variant;
• keep the current invariant strength: these tests must still fail on the same real source-level violations they protect today;
• add sabotage coverage for at least the reported // ... /** trap plus comment-shaped text inside string/template/regex literals, so the fix proves false-negative resistance rather than only changing regex order;
• run the full architecture/unit/type/build gates.

Repo-wide search for the same .replace(/\/\* shape currently finds these two TS tests plus typography-contract.test.js; the latter strips CSS block comments only, so this specific //-before-block interaction does not apply there.

Priority recommendation: normal backlog / architecture hardening. The failure mode can mask future violations, so it should remain open, but there is no evidence of a current production behavior regression.

[BorisTyshkevich]

Ship log — #643

Phase	Status	PR
(unphased)	shipped	#671

Handoff

• Landed: build/lib/check-legacy-owners.mjs gains two new parser-backed analyzers
  (findSidePanelSourceContractViolations/findSurfaceLifecycleSourceContractViolations)
  plus an internal withParsedSources batch primitive (withParsedSource now delegates
  to it, unchanged for every 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 caller) and a new .d.mts plain-data
  declaration boundary. tests/unit/side-panel-source-contract.test.ts and
  tests/unit/surface-lifecycle-arch.test.ts no longer preprocess source with the unsound
  two-pass regex comment stripper.
• Decisions taken (not in the issue): 4 findings the plan-author loop raised on its
  final (5th, capped) pass — never folded into the plan text itself since the loop hit
  its cap right after — were reviewed directly by the human owner and approved as
  mandatory implementation requirements (type-position literals, a missing test file in
  migration verification, explicit precision-change labeling for null ?? fallback, and
  a preserve-vs-strengthen wording fix). All four confirmed implemented and tested by the
  coordinator independently before opening the PR.
• Deviations from the spec: none beyond the above.
• Deferred acceptance criteria: none.
• Follow-ups filed: none.
• Plan review: hit the 5-pass cap (needs_human) without a final Fable APPROVED
  verdict — every prior round's findings (1-4) were accepted and incorporated; the human
  owner approved treating pass 5's 4 residual findings as direct implementation
  requirements rather than spending a 6th ChatGPT round. Conversation
  https://chatgpt.com/c/6a79eef2-bae4-83eb-8d1f-4030bd404b7b.
• Review outcome: certified at head 94c4c0105dc6e9b87b849b3c5c93c264fce13c89 after 2
  code-review passes. One internal Sonnet review pass (pre-PR) found and fixed a
  rule-code test-pinning gap. Pass 1 of the ChatGPT PR review found and fixed two real
  regressions versus the retired regex checks: (1) the protected coordinator declarations
  (disposeShell/disposeCurrentSurface/committedWorkspaceSignal/mainSurfaceSignal)
  accepted any let/var rewrite, not just const, and anchored its range at the
  binding identifier rather than the keyword (weakening marker-straddle detection); (2)
  the protected panel-id type-alias scan only walked a type alias's .type RHS, missing
  a protected literal confined to a type parameter's own extends/default clause — a
  pattern the guarded file itself already uses in production
  (src/core/side-panels.ts's PanelIdInPane). Both independently re-verified live by
  the coordinator before pushing. Pass 2: SHIP, no further findings. Merged as c9aa033.