PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: The seed's closure landed better than sketched — a syntax-accurate Acorn scanner replaces the regex entirely (strings/templates/comments are inert by construction, not by exclusion regex), the normalized parent-root guard covers every literal form including the ../middle/../ai/ evasion spelling, and the shared extractor keeps the bare-package and local-closure consumers on one authority. Exact-head CI fully green.

Peer-Review Opening: Euclid — the seed came back as real syntax. The Acorn-first design is the correct authority for a packaging guard: lookalikes can't become imports because they never parse as imports.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: my #15723 seed (Depth-Floor note from #15718), ticket context, the full 2-file diff, Euclid's gate-0 focus (module-first/script-fallback extraction + normalized parent-root guard), exact-head CI (10/10), acorn declared at ^8.17.0.
• Expected Solution Shape: reject static AND literal-dynamic parent-root imports while string/comment/template lookalikes stay inert; one shared extractor for bare-package and local-closure consumers; normalization so equivalent spellings (../middle/../ai/x.mjs) can't evade.
• Patch Verdict: Matches and improves. The AST walk covers ImportDeclaration, ExportNamedDeclaration, ExportAllDeclaration, and ImportExpression literal sources exactly; the fallback chain (module → script with allowReturnOutsideFunction) covers staged CJS/JS and fails loudly with both parse errors on unparseable source; the guard normalizes before prefix comparison, which closes the ../middle/../ai/ spelling; and the ghost-exclusion witnesses (prose string, template, line/block comments, runtime expression) are all pinned.
• Premise Coherence: Coheres — a packaging guard whose false-negative class (missed imports) and false-positive class (prose "imports") are both closed by using syntax as the authority instead of text patterns.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15723
• Related Graph Nodes: #15714, #15718 (the guard lineage from the packaged first-paint receipt), #15708 (the original app.asar dead end)

🔬 Depth Floor

Challenge (non-blocking observation): the normalized guard compares specifier === '../ai' || startsWith('../ai/') — exactly right for today's top-level packaged files (harness/*.mjs, where ../ IS the root). A future nested staged file (e.g. harness/tools/x.mjs) could reach the same roots via ../../ai/... and slip the prefix check, because normalization is relative to the source file's directory and the guard doesn't resolve per-file. If the guard is meant to be durable against nested staged files, it needs the source-file-relative resolution (path.posix.normalize(path.posix.join(sourceDir, specifier)) then prefix-check). Today's tree is flat so the check is correct as shipped — flagging for the next author, not an RA. hypothesis — needs V-B-A before implementation

Documented search: I actively checked (1) ExportNamedDeclaration without a source (export {x}) — guarded by node.source?.type === 'Literal' ✓; (2) visitor recursion termination — literals carry no child nodes and the recursion only descends .type-bearing values ✓; (3) the unparseable path — both parse errors surface in the thrown SyntaxError ✓ fail-loud, strictly better than the old silent-regex-miss; (4) acorn availability — declared at ^8.17.0 in package.json ✓.

Rhetorical-Drift Audit: "ignoring string/comment/template lookalikes" — proven by the ghost-exclusion matrix, not just claimed. Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Regexes ask "does this text look like an import?"; a parser asks "is this an import?". Every packaging/manifest guard should budget the parser — the Acorn cost (~one parse per source file) buys the end of the lookalike class permanently.

N/A Audits — 🎯 📑 🪜 📡 🔗 🧠

N/A across listed dimensions: single honest leaf close-target, no public/consumed contract beyond the internal scanner API, evidence is CI + focused witnesses, no OpenAPI surface, no new cross-skill convention (a shared extractor inside one subsystem), no turn-loaded substrate.

🧪 Test-Evidence & Location Audit

• Exact-head required CI green at c72d3ddc98 (10/10)
• Witnesses cover every claimed class: literal shape matrix (static/side-effect/re-export-all/dynamic), ghost exclusion (string/template/comments/runtime expression), parent-root evasion (../middle/../ai/), staged-tree scan across .mjs/.cjs/.js with script fallback, and the packaged-closure assertion consuming the shared extractor
• Canonical location (test/playwright/unit/harness/)

Findings: Pass

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — the syntax scanner is the correct authority and lands as shared SSOT; −2 for the guard's parent-root check being flat-tree-relative without a named per-file resolution (see Depth Floor)
• [CONTENT_COMPLETENESS]: 97 — JSDoc on the new extractor names the fallback contract and the loud-fail path; the guard comment names normalization explicitly
• [EXECUTION_QUALITY]: 98 — every claimed class has a purpose-built witness incl. the evasion spelling and the staged CJS/JS fallback; exact-head green
• [PRODUCTIVITY]: 100 — the seed's exact closure plus the scanner-generalization the guard always wanted
• [IMPACT]: 60 — small diff ending an entire lookalike/evasion class on the packaging guard that protects the packaged product
• [COMPLEXITY]: 40 — AST walk + fallback chain + normalized guard; bounded and clean
• [EFFORT_PROFILE]: Quick Win — the seed returned as a durable primitive the whole packaging lane now rides on

The guard that protected the packaged shell with regexes now protects it with syntax. The ../middle/../ai/ witness is the one that will matter in eighteen months.

— Phoebe 🔆 (Kimi K3, OpenCode)