Follow-up from the second review round on #501. Deferred there: both ADRs are pre-existing on main, and this is security-relevant architecture rather than prose.
Problem
ARCH-022 and ARCH-024 give conflicting accounts of how the two rule-file scanner entry points relate.
ARCH-024 states the scans are converged, in three places:
DO keep the first-party and imported scans converged (scanImportedRuleSource() delegates to scanRuleSource()) unless a genuinely imported-only concern justifies divergence
DON'T rely on scanImportedRuleSource() alone for imported rules — it is additive to scanRuleSource(), not a replacement
The first-party and imported scans are still converged (scanImportedRuleSource() delegates), so a new global block cannot be closed for one entry point and left open for the other
ARCH-022 describes the parser reuse differently, framing "typescript"/"javascript" as reusing the in-process meriyah parser already used by src/engine/rule-scanner.ts without stating how the two entry points share it.
Why it matters
This is the convergence property that stops a security block from being closed for first-party rule files and left open for imported ones. If the two ADRs describe the sharing differently, a future change can satisfy one reading and violate the other while both ADRs appear honoured — and writeImportedAdrs() is the point where an untrusted .rules.ts enters the project.
Suggested approach
- Read
src/engine/rule-scanner.ts and establish what scanRuleSource() and scanImportedRuleSource() actually do today — specifically whether the imported path re-parses independently or delegates.
- Make ARCH-024's "delegates to
scanRuleSource()" match reality exactly, or qualify it.
- Give ARCH-022 the corresponding statement about which parser helper both entry points use, so the two ADRs describe one architecture.
- Consider whether the convergence property is testable — an escape-regression test that runs the same payload through both entry points and asserts both block it would make the invariant enforceable rather than documentary.
tests/engine/rule-scanner-escapes.test.ts is the existing home for that style of test.
Point 4 matters most: ARCH-024's convergence claim is currently prose. Per this repo's own guidance on picking an enforcement layer, a behavioural property like "both entry points block the same payload" is provable only by executing the attack against each.
Review thread
#501 (comment) (thread on ARCH-022:42; also references ARCH-024:52)
Both texts verified pre-existing on origin/main.
Follow-up from the second review round on #501. Deferred there: both ADRs are pre-existing on
main, and this is security-relevant architecture rather than prose.Problem
ARCH-022 and ARCH-024 give conflicting accounts of how the two rule-file scanner entry points relate.
ARCH-024 states the scans are converged, in three places:
ARCH-022 describes the parser reuse differently, framing
"typescript"/"javascript"as reusing the in-processmeriyahparser already used bysrc/engine/rule-scanner.tswithout stating how the two entry points share it.Why it matters
This is the convergence property that stops a security block from being closed for first-party rule files and left open for imported ones. If the two ADRs describe the sharing differently, a future change can satisfy one reading and violate the other while both ADRs appear honoured — and
writeImportedAdrs()is the point where an untrusted.rules.tsenters the project.Suggested approach
src/engine/rule-scanner.tsand establish whatscanRuleSource()andscanImportedRuleSource()actually do today — specifically whether the imported path re-parses independently or delegates.scanRuleSource()" match reality exactly, or qualify it.tests/engine/rule-scanner-escapes.test.tsis the existing home for that style of test.Point 4 matters most: ARCH-024's convergence claim is currently prose. Per this repo's own guidance on picking an enforcement layer, a behavioural property like "both entry points block the same payload" is provable only by executing the attack against each.
Review thread
#501 (comment) (thread on
ARCH-022:42; also referencesARCH-024:52)Both texts verified pre-existing on
origin/main.