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

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.