What
tests/unit/side-panel-source-contract.test.ts:28-32 and
tests/unit/surface-lifecycle-arch.test.ts:44-48 each strip comments with two
sequential .replace() calls:
source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/(^|[^:"'`])\/\/.*$/gm, '$1')
This is unsound: a /*-shaped substring sitting inside an as-yet-unstripped //
line comment (e.g. a doc comment mentioning a glob like `src/core/**`) is read
by the first (block-comment) pass as a spurious opener, which then swallows
everything up to the next genuine */ — silently deleting real code in between and
potentially masking a sabotage probe or a real architecture violation the test exists
to catch.
Where
tests/unit/side-panel-source-contract.test.ts:28-32
tests/unit/surface-lifecycle-arch.test.ts:44-48
Why deferred
Discovered as a pre-existing, out-of-scope finding during the /ship 630 phase 3
high-risk pre-PR review (PR for issue #630 phase 3). That phase's own new
build/check-boundaries.mjs legacy-owner rule had the identical bug (introduced
fresh in that phase, not pre-existing) — caught and fixed in commit 9ff449e by
switching to a single alternation-based regex:
source.replace(/\/\*[\s\S]*?\*\/|\/\/.*/g, '')
That fix is scoped to the one rule/mirror pair Phase 3 added; it does not touch these
two unrelated, already-existing test files, so the same class of bug remains latent
in them. Not blocking Phase 3's PR since neither file was touched by that diff.
Suggested fix
Apply the same single-alternation-regex fix to both files' stripComments() helpers,
and consider whether other regex-based comment-strippers in the test tree share this
pattern (a repo-wide grep for .replace(/\/\* would find them).
What
tests/unit/side-panel-source-contract.test.ts:28-32andtests/unit/surface-lifecycle-arch.test.ts:44-48each strip comments with twosequential
.replace()calls:This is unsound: a
/*-shaped substring sitting inside an as-yet-unstripped//line comment (e.g. a doc comment mentioning a glob like
`src/core/**`) is readby the first (block-comment) pass as a spurious opener, which then swallows
everything up to the next genuine
*/— silently deleting real code in between andpotentially masking a sabotage probe or a real architecture violation the test exists
to catch.
Where
tests/unit/side-panel-source-contract.test.ts:28-32tests/unit/surface-lifecycle-arch.test.ts:44-48Why deferred
Discovered as a pre-existing, out-of-scope finding during the
/ship 630phase 3high-risk pre-PR review (PR for issue #630 phase 3). That phase's own new
build/check-boundaries.mjslegacy-owner rule had the identical bug (introducedfresh in that phase, not pre-existing) — caught and fixed in commit
9ff449ebyswitching to a single alternation-based regex:
That fix is scoped to the one rule/mirror pair Phase 3 added; it does not touch these
two unrelated, already-existing test files, so the same class of bug remains latent
in them. Not blocking Phase 3's PR since neither file was touched by that diff.
Suggested fix
Apply the same single-alternation-regex fix to both files'
stripComments()helpers,and consider whether other regex-based comment-strippers in the test tree share this
pattern (a repo-wide grep for
.replace(/\/\*would find them).