build(#592): lock in the #586/#587 shell primitive guardrails

[BorisTyshkevich]

What & why

Part of the ADR-0004 vanilla-shell investment track. Closes #592.

Once #586 (SurfaceLifecycle + docked inspectorHost) and #587 (side-panel registry)
landed, nothing mechanically stopped the six-copy-pasted-overlays problem #586 fixed
from regrowing. This PR adds three deterministic check:arch guards that lock in what
those two issues established:

1. shell-body-mount — a new Document.body.append/.appendChild call fails the
   build unless it matches an exact, reviewed baseline snapshot of sanctioned
   lifecycle/primitive scopes (11 scopes: the SurfaceLifecycle-backed cell-detail
   overlay in results.ts, dialog-shell.ts, both popover.ts primitive families,
   toast.ts, both detached-view.ts mount paths, menu.ts, shortcuts.ts's modal,
   and app.ts's export-progress surface + download anchor). The results.ts exception
   additionally requires its openSurfaceLifecycle(...) composition to stay in scope —
   losing that call while keeping the body mount fails.
2. shell-fixed-position — a new position: fixed selector in src/styles.css
   fails unless it matches the exact current selector/at-rule snapshot (14 declarations),
   via a hand-written comment/string/escape/brace-aware CSS lexical scanner (no CSS
   parser dependency).
3. shell-capture-escape — a new global capture-phase Escape keydown lifecycle
   fails unless it matches the canonical SurfaceLifecycle scope, one of 5 distinct
   pre-existing panel exceptions (dialog-shell.ts, popover.ts ×2, results.ts's Data
   Pane, explain-graph.ts ×2, menu.ts), or one of 3 non-panel gesture-cancellation
   exclusions (dashboard-tile-gestures.ts ×2, dashboard-chart-interaction.ts's
   beginSelection — Escape there cancels a chart-range selection, not a panel close).
   Non-Escape capture-phase keydown listeners (e.g. dashboard.ts's activity/
   nav-highlight trackers) are excluded structurally before any policy lookup, not via
   an exception fingerprint.

Rules 1 and 3 share one real-TypeScript-parser batch
(findShellGuardrailSourceContractViolations, build/lib/check-legacy-owners.mjs),
reusing this repo's existing withParsedSources/walkTree/SyntaxKind idiom — no new
parser dependency, and no unsound textual prefilter gating it.

Also closes the "Inherited from #586" item: an independent
tests/unit/resize-handle-thickness-contract.test.js proves src/ui/app-shell.ts's
HANDLE_PX and src/styles.css's .col-resize/.inspector-resize width cannot drift
apart unnoticed (chosen over a CSS-custom-property refactor, to keep this unit
enforcement-only).

Enforcement-only. No SurfaceLifecycle, side-panel registry, inspectorHost,
Escape ordering, gesture-cancellation behavior, or runtime UI/CSS composition changed —
verified via git diff --stat: only build/check-boundaries.mjs,
build/lib/check-legacy-owners.mjs/.d.mts, two new test files, and CHANGELOG.md
changed.

Delivery contract, invariant map, and decision rationale (why results.ts/
explain-graph.ts/menu.ts's existing Escape handlers stay allowlisted rather than
migrated, why the chart-selection exclusion is a separate category from panel-close
exceptions) were established through a ChatGPT-authored / Fable-reviewed plan (3 review
passes, all findings incorporated) — see the ship-log comment on #592 for the full
handoff.

Sabotage verification

Five real, temporary mutations against production files, each confirmed to fail
check:arch (or the new focused test) and then restored from exact original bytes:

1. A second body-mount in toast.ts → shell-body-mount violation.
2. A new position: fixed selector appended to styles.css → shell-fixed-position violation.
3. A second capture-Escape listener in dialog-shell.ts → shell-capture-escape violation.
4. A second listener beside dashboard-chart-interaction.ts's beginSelection exclusion
   (proving the non-panel exclusion is non-transitive) → violation.
5. HANDLE_PX changed to 8 while CSS stayed at 7px → the drift contract test failed.

Checklist

• npm test passes (the per-file coverage gate is non-negotiable)
• Tests added/updated in the same change as the code
• npm run build succeeds (single-file dist/sql.html)
• Layers kept honest: pure logic in src/core/, network in src/net/ (injected fetch), DOM in src/ui/
• No new runtime dependency
• CHANGELOG.md ([Unreleased]) updated
• N/A — no tracked roadmap/ADR reshaping; this issue's own acceptance criteria are fully claimed and met

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 36ef21c23350d55a22ce28f7e6bd7fe0538dc397

I reviewed the complete six-file PR against #592 and the supplied acceptance/invariant map. The production wiring is present: all three new architecture checks are called from build/check-boundaries.mjs; the PR changes only build/test/CHANGELOG files; and canonical CI for this head's merge ref ran npm test, including pretest -> check:arch -> check:types, with the new shell-guardrail and resize-handle tests passing. I could not run additional local mutants because this review environment could not resolve github.com for a clone, so the findings below are from the exact canonical head/patch plus CI logs.

P1 — Resolve aliases/handlers lexically; the current file-global maps can silently miss violations

build/lib/check-legacy-owners.mjs:2187-2214, :2222-2270, and :2310-2325 build whole-file maps keyed only by identifier text. They do not track lexical scope or shadowing. The later candidate pass therefore uses whichever same-named binding happened to survive / look nearest by source position, not necessarily the binding visible at the call site.

This creates direct false-negative shapes. For example, a rogue function a(doc: Document) { doc.body.appendChild(panel); } is no longer recognized as a Document-body mount if a later sibling declares function b(doc: Window) {} because the file-global doc entry ends as window. Similarly, a capture listener using local const options = {capture:true} can be treated as non-capture if another scope later binds options = false. resolveHandlerNode has the same issue: a parameter named onKey can be incorrectly resolved to an unrelated same-named function from another scope, defeating the intended fail-closed behavior.

Please resolve bindings at each use site through the lexical ancestor chain (or an equivalent TypeScript binding mechanism), and fail closed when the visible binding is ambiguous/unresolvable. Add sabotage fixtures with same-named parameters/locals in sibling and nested scopes for the Document target, capture-options alias, and handler alias.

P1 — The frozen snapshots are only upper-bound/membership allowlists, not exact snapshots

build/lib/check-legacy-owners.mjs:2541-2547 and :2693-2698 only report body/Escape occurrences beyond allowedCount; they never report a policy entry whose observed count falls below the frozen count. findShellFixedPositionViolations at :2906-2912 is even looser: it performs membership with .some(...) and has no declaration count at all.

That violates the supplied invariant requiring exact (filename, scope, count) matching. A second top-level .auth-host { position: fixed; } (or another duplicate of any approved selector/context) is a new fixed declaration but passes because the selector/at-rule pair is already in the policy. Removing an approved body/Escape/fixed occurrence also leaves stale permission that can be reintroduced later without a policy review.

Please compare the discovered baseline and policy as exact multisets in both directions. For CSS, include a count per (selector, at-rule) key. Add tests that duplicate an approved fixed selector and that omit one approved body, Escape, and fixed occurrence; each should fail until the snapshot is deliberately updated.

P1 — The HANDLE_PX drift test misses individual CSS overrides

tests/unit/resize-handle-thickness-contract.test.js:79-100 only extracts widths from a rule whose selector list contains both .col-resize and .inspector-resize, then compares that one shared width to HANDLE_PX. It never checks additional rules that affect either selector separately.

For example, this still passes the shipped contract while making the actual inspector handle 8px in normal CSS cascade order:

.col-resize, .inspector-resize { width: 7px; }
.inspector-resize { width: 8px; }

The same hole exists for a media-query or more-specific override. That means the inherited acceptance criterion — JS reserved thickness and CSS-declared handle thickness cannot drift unnoticed — is not mechanically guaranteed.

Please scan every width declaration capable of targeting .col-resize or .inspector-resize (or otherwise mechanically forbid overrides outside the single canonical shared declaration), and add sabotage cases for a later individual-selector override and a media-query/specificity override.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 36ef21c23350d55a22ce28f7e6bd7fe0538dc397

Reviewed head: 570e08940998c27c2585107b3c9cc3515dd0ac07

I compared the new head directly with the pass-1 SHA and re-reviewed the complete six-file PR against the pass-2 delivery contract. The fix commit is one commit ahead of the old review head. Canonical CI for this exact new head is green: npm test ran pretest -> check:schemas -> check:examples -> check:arch -> check:types; check-boundaries reported clean; tests/unit/shell-guardrails-arch.test.ts passed 88 tests; resize-handle-thickness-contract.test.js passed 17 tests; the root suite passed 7442 tests; build and e2e also succeeded. The PR still changes only build tooling, tests, and CHANGELOG — no runtime src/** implementation/CSS or dependency files. Local clone-based mutants were not feasible in this review environment because github.com DNS resolution is unavailable, so the adversarial findings below are based on the exact canonical head plus CI execution evidence.

Pass-1 reassessment

• File-global alias/handler resolution: improved, but not fully fixed; function-scope shadowing is covered, block scope is not.
• Exact snapshots: CSS duplicate/missing checks improved, and TS counts now check the reverse direction while a scope still exists, but whole-scope deletion still leaves stale permission.
• HANDLE_PX CSS override detection: exact standalone/media single-class overrides are now covered, but compound/more-specific selectors remain invisible.

P1 — The new “lexical” resolver still ignores block scope, so valid let/const shadowing can hide violations

scopeOwnerOf() defines a binding's owner as only the nearest FUNCTION_LIKE_KINDS ancestor (or module), and scopeChain() therefore walks function scopes only. buildGlobalAliasMap, buildCaptureAliasMap, and buildFunctionDeclMap all key their entries through that owner. That is not JavaScript/TypeScript lexical scoping for let/const.

A real body-mount bypass remains:

function rogue(doc: Document) {
  { const doc: Window = window; }
  doc.body.appendChild(panel);
}

At runtime/typecheck level the final doc is the Document parameter, but both declarations are stored under the same function-scope map entry; the block-local Window wins in the analyzer, so the append is no longer recognized as a Document-body candidate.

The same issue can make a capture listener disappear:

function rogue(doc: Document) {
  const onKey = (e) => { if (e.key === 'Escape') close(); };
  const opts = { capture: true };
  { const opts = false; }
  doc.addEventListener('keydown', onKey, opts);
}

and handler resolution has the analogous shape with an inner-block const onKey = () => {}: resolveHandlerNode picks the nearest preceding same-named function expression from the function-level bucket even when that binding is out of scope at the listener.

Please model block-scoped bindings (or use parser binding/symbol information where available), and add sabotage cases with same-named const/let bindings in nested blocks, not only sibling/nested functions. Also scope bodyAliasNames; it is still a single file-global Set<string> and can cross-contaminate unrelated scopes in the opposite direction.

P1 — Capture options with quoted/shorthand capture properties bypass the fail-closed rule

resolveObjectCaptureLiteral() recognizes only a PropertyAssignment whose property name is an Identifier named capture; if it sees no such property and no spread, it returns false (provably non-capture). Valid equivalent object-literal spellings are therefore misclassified as non-capture:

document.addEventListener('keydown', onKey, { 'capture': true });

const capture = true;
document.addEventListener('keydown', onKey, { capture });

The first uses a string-literal property name; the second is a ShorthandPropertyAssignment. Neither is recognized, so a real capture-phase Escape listener can be structurally dropped before policy lookup. A static computed ['capture'] spelling is the same class of issue.

Please recognize all statically-known capture property names and resolve safe shorthand constants, or return null for unhandled object-literal property shapes so the existing fail-closed path fires. Add quoted, shorthand, and static-computed sabotage fixtures.

P1 — TS “exact snapshots” still do not detect deletion of the entire approved scope/file

The new reverse passes in shellBodyMountViolations() and shellCaptureEscapeViolations() first compute declaredScopeKeys(sourceFile) and then explicitly skip a policy entry when !declaredScopes.has(key). This catches “scope still exists but occurrence count dropped” (the new tests exercise empty flashToast/openMenu bodies), but it does not catch the stronger stale-permission case where the approved function itself is deleted or renamed. If an entire policy file disappears, that policy is not visited at all.

That is still not an exact (filename, scope, count) snapshot: an obsolete policy row can survive after its production scope has gone away, then silently authorize a later reintroduction under the old fingerprint.

Please add a full-tree reverse-baseline pass (or a completeBaseline mode) that compares every TS policy entry against the complete scanned source set, analogous to the separate CSS missing-baseline helper. Keep partial synthetic fixtures forward-only if needed. Add tests that remove the whole approved function and, separately, the whole approved file from a complete synthetic/live source set.

P1 — The HANDLE_PX fix still misses compound/more-specific CSS selectors

extractSharedResizeWidthPx() now considers a rule only when a comma-split selector string is exactly .col-resize or exactly .inspector-resize (selectors.includes(...)). That closes the pass-1 standalone/media examples, but not specificity overrides such as:

.col-resize, .inspector-resize { width: 7px; }
.inspector-resize.dragging { width: 8px; }

or:

.shell .col-resize { width: 9px; }

Both can override the actual handle width while the contract still extracts only [7]. Compound resize selectors are already normal style in src/styles.css (for example .inspector-resize.dragging::before), so this is not an exotic grammar shape.

Please detect the resize class as a selector token within compound/descendant selectors (or mechanically forbid any width declaration targeting either class outside the one canonical grouped rule). Add .inspector-resize.dragging and ancestor-qualified sabotage cases, including a media-scoped compound selector.

P1 — shell-fixed-position fingerprints only the nearest at-rule, so nested context can change without review

scanFixedPositionDeclarations() walks outward from the rule but stops at the first enclosing at-rule and records only that one string. The approved mobile fingerprint therefore cannot distinguish:

@media (max-width: 768px) {
  .inspector-host { position: fixed; }
}

from:

@supports (display: grid) {
  @media (max-width: 768px) {
    .inspector-host { position: fixed; }
  }
}

Both record selector .inspector-host plus nearest at-rule @media (max-width: 768px), so the second passes even though its full at-rule context changed. The pass-2 contract explicitly calls out nested at-rule/media behavior as an adversarial probe for the exact selector+at-rule snapshot.

Please fingerprint the complete normalized enclosing at-rule chain (e.g. an array outermost -> innermost), not only the nearest frame. Add a nested @supports + approved @media sabotage and nested-media coverage.

The production wiring itself is correct: all three categories are reached from node build/check-boundaries.mjs, the reverse CSS helper is wired there too, the exceptions remain scope-path-specific rather than filename-wide, the results.ts lifecycle-composition check remains enforced, and there is still no textual prefilter gating the TypeScript parser batch.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 570e08940998c27c2585107b3c9cc3515dd0ac07

Reviewed head: ad4f71f74d92a8a6cfc216539649d52eb5961022

I compared the new head directly with the pass-2 SHA and re-reviewed the complete six-file PR against the pass-3 contract/invariant map. The new head is exactly one commit ahead of the prior review head. Canonical CI for this exact SHA is green (test, build, e2e, bundle, Docker smoke, CI gate). The PR still changes only CHANGELOG.md, build/check-boundaries.mjs, build/lib/check-legacy-owners.mjs/.d.mts, and the two new unit-test files; no runtime src/**, CSS, package, or dependency file changed. The real check-boundaries.mjs path invokes the batched TS guard and both CSS forward/reverse checks.

Pass-2 reassessment

• Nested block-local const shadowing: the exact reviewed if/bare-block cases are fixed, but the lexical model is still not JavaScript/TypeScript-correct for var declarations or for-header lexical bindings; see finding 1.
• Quoted/computed/shorthand capture properties: the three exact reviewed spellings are fixed. A neighboring fail-closed hole remains when an explicit capture member coexists with a spread or unresolved computed member; see finding 2.
• Whole-scope deletion leaving stale TS snapshot permission: unresolved; see finding 3.
• HANDLE_PX contract missing more-specific/compound selector overrides: unresolved; see finding 4.
• Nested fixed-position at-rule context: fixed; the scanner now fingerprints the complete outer-to-inner at-rule chain and has forward/reverse sabotage coverage.

P1 — The new lexical resolver still models scope by AST block, not by declaration semantics

scopeOwnerOf() now chooses the nearest Block/CaseBlock or function for every variable declaration. That fixes block-local const shadows, but it is wrong in two important directions:

1. var is function-scoped, not block-scoped. A valid body-mount bypass is:

function rogue(doc: Document) {
  { var d = doc; }
  d.body.appendChild(panel);
}

d is visible after the block at runtime, but buildGlobalAliasMap records it under the inner block, so the later receiver cannot resolve to Document and the mount is dropped before policy lookup.

2. let/const in a for initializer are scoped to the loop, but the VariableDeclaration is not an AST descendant of the loop body's Block. It is therefore recorded under the containing function-body block. This can hide a real outer alias after the loop:

function rogue(doc: Document) {
  for (const doc = window; false; ) {}
  doc.body.appendChild(panel);
}

The analyzer leaves the loop-local doc = window in the containing block's alias bucket, so the final doc can resolve as Window instead of the Document parameter.

Please make binding ownership declaration-aware: var to nearest function/module; let/const to the correct lexical environment including for/for-in/for-of headers (and equivalent declaration scopes). Add sabotage fixtures for both shapes. This also applies to the body-alias map, which uses the same owner helper.

P1 — Capture options still fail open when a known capture member is followed by a spread/unresolved override

resolveObjectCaptureLiteral() records hasSpread / an unresolved key, but if it has found any explicit captureValueNode it returns that value immediately and never considers those uncertainty flags. Object-literal property order matters, so this is a direct false negative:

const spread = { capture: true };
const onKey = (e) => { if (e.key === 'Escape') close(); };
document.addEventListener('keydown', onKey, { capture: false, ...spread });

At runtime the later spread makes capture === true; the analyzer resolves the explicit false and drops the listener as provably non-capture. The same class exists with a later computed key whose static name is unresolved but may evaluate to "capture".

Please preserve property order and only trust an explicit value when no later spread/unknown key can override it; the simpler safe rule is to return null whenever the object contains a spread or unresolved key. Add sabotage cases for { capture:false, ...{capture:true} } / an aliased spread and for an unknown computed key after explicit capture:false.

P1 — Exact TS snapshots still do not detect removal of the entire approved scope or file

The reverse body/capture passes still do:

if (!declaredScopes.has(key)) continue;

and findShellGuardrailSourceContractViolations() only invokes those checks for files that exist in the supplied source set. Therefore deleting/renaming the whole approved function — or deleting an approved file together with its callers — leaves the frozen policy entry untouched and produces no missing-baseline violation. That is still stale permission, contrary to the contract's exact (filename, scope, count) snapshot and the implementation's own statement that deleting an exception must shrink the table.

Please add a complete-tree reverse comparison at the batch level (or a separate complete-tree missing-baseline helper, analogous to CSS) that checks every policy row even when its file/scope vanished. Keep the current partial-fixture behavior only for synthetic unit helpers. Add whole-scope and whole-file deletion sabotage tests.

P1 — The HANDLE_PX contract still ignores compound/more-specific selectors

This file was unchanged by the pass-3 fix. extractSharedResizeWidthPx() still accepts a rule only when a comma-split selector is exactly .col-resize or .inspector-resize:

if (!rule.selectors.includes('.col-resize') && !rule.selectors.includes('.inspector-resize')) continue;

So this still passes while changing the rendered inspector handle width:

.col-resize, .inspector-resize { width: 7px; }
.inspector-resize.dragging { width: 8px; }

as does .shell .inspector-resize { width: 8px; } (including inside a media query). The inherited contract says JS reserved thickness and CSS-declared handle thickness cannot drift unnoticed, not merely that exact single-class selector rules cannot drift.

Please detect the resize class as a selector token within compound/descendant selectors, or mechanically forbid any other width declaration capable of targeting either class. Add compound/specificity sabotage cases.

P1 — shell-body-mount misses destructured Document.body aliases

bodyAliasMap only examines VariableDeclarations whose name is an Identifier, so a direct destructuring alias is invisible:

function rogue() {
  const { body } = document;
  body.appendChild(panel);
}

That is still an append to Document.body, but the call receiver is neither a direct .body access nor an entry in bodyAliasMap, so it produces no candidate. Renamed destructuring (const { body: host } = document; host.append(...)) has the same problem.

Please recognize statically clear object-binding aliases of Document.body (and add sabotage fixtures) so the body-mount guard cannot be bypassed by equivalent binding syntax.

P1 — The “escape-aware” CSS scanner does not decode CSS escapes in property/value identifiers

The scanner preserves a backslash escape pair verbatim in buf, then compares the raw property to position and the raw value to /^fixed...$/. CSS identifiers may spell those names with escapes, so a browser-valid fixed declaration such as:

.sabotage { \70osition: fixed; }
/* or */
.sabotage { position: \66ixed; }

is not recognized as position: fixed by the guard at all. This is a direct false negative in the contract's comment/string/escape/brace-aware lexer requirement.

Please decode identifier escapes for declaration property/value comparison (or conservatively flag an escaped property/value that could normalize to position/fixed) and add positive scanner/sabotage tests for escaped identifier spellings.

The pass-3 nested-at-rule fix itself is sound from the inspected paths, the results.ts lifecycle backing check remains present, exceptions remain filename+scope+count bounded rather than filename-wide, non-Escape capture keydowns are still filtered before policy lookup, and there is still no raw textual prefilter gating the TS parser batch.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 0c3c6d489132092ffc2cac014e7bfd6f41c8b61c
Base: c9aa0334c8ee75edebe5cf02d10ea8c00e1055f3

The checker-based binding refactor is a material improvement: it removes the hand-rolled lexical scope tables that caused the earlier sibling/block/var/loop shadowing failures. The complete PR still changes only the six expected build/test/CHANGELOG files, all three policy categories are on the real build/check-boundaries.mjs path, the allowlists are scope/count-specific rather than filename-wide, and the exact-head GitHub Actions CI run is green. I could not run the repository locally because the sandbox could not resolve github.com, so executable verification here is based on the canonical exact-head CI plus code-level adversarial analysis.

P1 — the TypeScript “exact snapshot” reverse check still skips whole-scope and whole-file deletion

Both shellBodyMountViolations and shellCaptureEscapeViolations do:

const declaredScopes = declaredScopeKeys(sourceFile);
...
if (!declaredScopes.has(key)) continue;

That only detects a count drop while the approved function/scope still exists. If flashToast/openMenu/another approved scope is renamed or deleted wholesale, the policy entry is deliberately skipped. If an approved file is removed entirely, findShellGuardrailSourceContractViolations never iterates that filename at all. This leaves stale permission in the frozen policy and contradicts the contract's exact (filename, scope, count) snapshot invariant.

The shipped “loses its ONLY occurrence” tests do not cover this: wrapScope(...) keeps the approved function declaration present and only empties its body.

Action: add a complete-tree reverse comparison at batch level (or an explicit completeBaseline mode used by production check:arch) that checks every policy filename/scope even when no corresponding source/scope exists. Keep partial-fixture behavior separate if synthetic tests need it. Add sabotage tests for (1) deleting the whole approved function and (2) omitting an approved file from a complete batch.

P1 — binding identity is now correct, but alias values are still resolved from stale declaration initializers

resolveCaptureFlag resolves an identifier to its VariableDeclaration and recursively evaluates only declNode.initializer. It does not account for later assignment or object mutation. Therefore valid code such as:

const onKey = (e) => { if (e.key === 'Escape') close(); };
let opts = { capture: false };
opts = { capture: true };
document.addEventListener('keydown', onKey, opts);

is a real capture-phase Escape listener, but the guard resolves opts back to { capture: false } and drops it as provably non-capture. The same problem exists with const opts = { capture: false }; opts.capture = true;, mutable handler aliases, and mutable body aliases. The TypeScript binder answers “which binding is this?”; it does not answer “what value reaches this use?”.

Action: never treat a mutable alias as provably clean from its declaration initializer alone. For capture options, the simplest sound behavior is to allow only immutable/provable forms (e.g. inline object literals and primitive const booleans) and return null/fail closed for mutable or object aliases unless writes are explicitly proven absent. Add reassignment and property-mutation sabotage tests. Apply the same principle anywhere body/handler classification follows a variable initializer.

P1 — Document/Window classification still depends on the type/property spelling, so ordinary aliases bypass both source guards

The new checker is used to resolve the binding, but classifyGlobalDeclaration then recognizes only a direct syntactic : Document / : Window type name. Thus:

type Doc = Document;
function rogue(doc: Doc) {
  doc.body.appendChild(panel);
}

resolves doc to the correct parameter, but typeNamesOf sees only Doc, so resolveGlobalKind returns null and the body mount is not a candidate. The same receiver classification gap can hide a capture-Escape listener.

There is also a narrower destructuring spelling gap: resolvesToDocumentBody only recognizes a BindingElement.propertyName when it is an Identifier, so valid const { 'body': host } = document; host.appendChild(panel); is missed (and the analogous quoted document/window property rename is missed by classifyGlobalDeclaration).

Action: resolve type aliases through the checker (or recursively inspect their resolved type-alias declaration) instead of keying only on the literal annotation text; reuse a static-property-name helper for binding-element property names so identifier/string/static-computed spellings are equivalent. Add both sabotage forms.

P1 — the Escape-semantic filter has a direct constant-alias bypass

containsEscapeSemantics only recognizes strict === / !== comparisons where the literal 'Escape' is directly one operand, plus a literal case 'Escape'. A resolved handler like this is classified clean and removed before policy lookup:

const ESC = 'Escape';
const onKey = (e) => { if (e.key === ESC) close(); };
document.addEventListener('keydown', onKey, true);

It is nevertheless exactly a global capture-phase Escape lifecycle. A loose e.key == 'Escape' is another simple valid spelling the current operator filter ignores.

Action: at minimum resolve local constant string aliases and the equality operators JavaScript permits. More robustly, make the “clean non-Escape” classification conservative: if key/code-dependent behavior cannot be proven non-Escape, fail closed rather than classifying it clean. Add sabotage tests for a local ESC constant (and loose equality if that syntax remains permitted by the repo).

P1 — the HANDLE_PX drift test silently ignores valid width overrides it cannot parse

extractSharedResizeWidthPx only counts declarations matching exactly a numeric <number>px; value:

/\bwidth\s*:\s*(-?\d+(?:\.\d+)?)px\s*;/g

So this override is ignored entirely:

.inspector-resize { width: 8px !important; }

The canonical 7px remains the only extracted value and the contract reports success even though the browser renders a different handle width. width: calc(8px) and width: var(--handle-width) have the same problem.

Action: detect every width declaration on a selector capable of targeting either resize handle. Parse a plain numeric px value when possible; any other value should make the contract ambiguous/uncheckable and fail instead of being skipped. Add !important, calc(...), and/or custom-property sabotage cases.

P2 — fixed-position fingerprints ignore enclosing nested style-rule context

The CSS scanner now records the full enclosing at-rule chain, but for style rules it records only frames[frames.length - 1].prelude. With valid CSS nesting:

.wrapper {
  .auth-host { position: fixed; }
}

the stored fingerprint is still selector .auth-host, atRule: null, which is indistinguishable from the approved top-level .auth-host row even though the effective selector/context changed. The reverse check also sees the approved fingerprint as present.

Action: include enclosing style-rule context in the fingerprint, or conservatively reject nested style-rule position: fixed declarations when no such nesting exists in the baseline. Add a sabotage test that wraps an approved root selector in an outer style rule.

Verified positives

• The real build/check-boundaries.mjs path invokes the shared TS batch, fixed-position forward check, and fixed-position reverse check.
• The body/capture policies are keyed by full scope path and occurrence count, not filename-wide permission.
• The results.ts body-mount exception still requires openSurfaceLifecycle(...) composition.
• There is no source.includes(...) prefilter gating the new TS analyzers.
• The checker refactor removes the earlier custom lexical-scope resolver class rather than adding another scope special case.
• The current full PR remains enforcement-only at the changed-file level: build tooling, declarations, tests, and CHANGELOG only; no src/** runtime/CSS file or dependency change.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 0c3c6d489132092ffc2cac014e7bfd6f41c8b61c

Reviewed head: 9b530eca4bee2e4ab5a622414d94a639836136aa

Base: c9aa0334c8ee75edebe5cf02d10ea8c00e1055f3

I compared the new head directly with the previous review SHA and re-reviewed the complete six-file PR against the supplied pass-2 contract. The new revision is one commit ahead. Canonical CI for this exact head is green: the test job ran npm test, whose pretest executed the real node build/check-boundaries.mjs; check-boundaries reported clean, tests/unit/shell-guardrails-arch.test.ts passed 140 tests, resize-handle-thickness-contract.test.js passed 30 tests, and the root suite passed 7,507 tests. Build/e2e/docker/size and the CI gate also succeeded. Local clone/focused mutants were not feasible in this review environment because github.com DNS resolution is unavailable.

The complete PR still changes only CHANGELOG.md, build/check-boundaries.mjs, build/lib/check-legacy-owners.{mjs,d.mts}, and the two unit-test files — no runtime src/** implementation/CSS or dependency files.

Pass-1 reassessment

• Whole-scope/file reverse snapshot: functionally fixed. findShellGuardrailMissingBaselineViolations now has strict complete-tree checks and shipped tests for an approved function deleted outright and an approved file missing from the batch.
• Mutable capture/handler aliases: the specific let/var reassignment cases are fixed by refusing to trust non-const aliases, but the broader value-flow issue is not fully fixed (details below).
• Type/property aliases: the reviewed local type Doc = Document chain and quoted/computed destructuring-key cases are fixed and covered by sabotage tests.
• Indirect Escape semantics: not fixed; containsEscapeSemantics is still direct-literal-only.
• Resize width non-literal values: !important, calc(...), var(...), etc. are now counted/fail closed, but a valid no-final-semicolon override still bypasses the extractor.
• Nested plain CSS style rules: the reviewed .wrapper { .auth-host { position: fixed } } shape is fixed, but a different valid CSS-nesting shape under a conditional group rule is still invisible.

P1 — Value-flow is still unsound for const option objects and mutable body aliases

The new isConstVariableDeclaration gate fixes reassignment of the binding, but const does not make an object immutable. resolveCaptureFlag still reduces a const options identifier to its declaration initializer forever, so this real capture listener is silently treated as non-capture:

const opts = { capture: false };
opts.capture = true;
const onKey = (e) => { if (e.key === 'Escape') close(); };
document.addEventListener('keydown', onKey, opts);

opts resolves to the same const declaration, resolveObjectCaptureLiteral reads the stale initializer { capture: false }, and captureEscapeCandidates drops the listener before policy lookup. There is no shipped sabotage for opts.capture = ....

The same original value-flow class also remains on the body-mount path because resolvesToDocumentBody still follows a VariableDeclaration's initializer without any const/write check. This valid shape is a false negative:

let body: HTMLElement = document.createElement('div');
body = document.body;
body.appendChild(panel);

At the append call the runtime value is document.body, but the analyzer resolves body back to the initial createElement(...) expression and reports no candidate.

Please either perform conservative write/use analysis or fail closed whenever an alias whose current value matters is mutable (including property writes on an aliased options object). Add sabotage cases for a const options object whose .capture is mutated and a body alias reassigned to document.body before .appendChild(...).

P1 — containsEscapeSemantics still misses const-aliased Escape values

This pass did not change the Escape-semantic recognizer: it still requires one side of a strict ===/!== comparison (or a switch case) to be the literal 'Escape' directly. Therefore a global capture-phase Escape lifecycle can still be structurally classified as clean and discarded before policy lookup:

const ESC = 'Escape';
const onKey = (e) => { if (e.key === ESC) close(); };
document.addEventListener('keydown', onKey, true);

The TypeScript checker is already available here; resolve simple const string aliases used in key/code comparisons (and, if supported code style permits them, the equivalent ==/!= forms), or conservatively fail closed on unresolved key/code comparisons in a global capture-keydown handler. Add sabotage coverage for the aliased-literal form above.

P1 — CSS nesting with a conditional group rule inside a style rule bypasses shell-fixed-position

The new nested flag catches an inner style rule below another style rule, but processDeclaration still immediately returns unless the innermost frame itself is kind === 'rule'. Valid CSS nesting allows conditional group rules inside a style rule, with declarations applying to the parent selector:

.rogue-shell {
  @media (min-width: 0px) {
    position: fixed;
  }
}

At position: fixed, the innermost frame is the @media frame, so the scanner returns before finding the enclosing .rogue-shell rule. This produces no fixed-position candidate at all, despite the contract explicitly calling out nested @media adversarial probing.

Please associate a declaration with the nearest enclosing style-rule frame even when one or more conditional at-rules sit between it and that rule, preserving the complete at-rule chain and treating the resulting nested selector context conservatively. Add sabotage coverage for style rule > @media/@supports > position: fixed (including deeper conditional nesting).

P1 — The HANDLE_PX CSS extractor still skips a valid final declaration with no semicolon

extractSharedResizeWidthPx now matches every width: value it can see, but its declaration regex still requires a terminating ;:

/\bwidth\s*:\s*([^;]+?)\s*;/g

CSS permits the final declaration in a rule to omit that semicolon. This therefore still passes the shipped contract while changing the rendered inspector handle width:

.col-resize, .inspector-resize { width: 7px; }
.inspector-resize { width: 8px }

The second rule is found by flatCssRules, but its width contributes no value, leaving the extracted list as [7]. Accept end-of-rule-body as a declaration terminator (or use a tiny declaration lexer) and add standalone/media sabotage cases without a final semicolon.

P2 — The strict reverse fix creates a second TypeScript parser batch in the real check:arch path

The production block now calls both findShellGuardrailSourceContractViolations(shellSources) and findShellGuardrailMissingBaselineViolations(shellSources). The first calls withParsedSources(...); the new strict reverse function then builds toParse and calls withParsedSources(...) again. That means the source guards no longer run in the single shared parser batch that this PR's own architecture contract/comments explicitly require.

Please fold strict reverse comparison into the same parsed sourceFiles/checkers batch as the forward pass — for example via a completeTree option used only by production/live-tree callers, while minimal synthetic fixtures keep the softened behavior — so exact reverse checking does not spawn a second parser process/batch. This also avoids duplicate missing-baseline diagnostics when a count drops inside a still-present approved scope.

The results.ts lifecycle companion check remains present, allowlists remain filename+scope+count bounded rather than filename-wide, non-Escape capture keydowns are still filtered before policy lookup, and I found no raw textual prefilter gating the TypeScript parser batch.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 9b530eca4bee2e4ab5a622414d94a639836136aa

Reviewed head: 5d5d166e372cb1ec4f8d4eb9d5d15eae9435bd8a

I compared the new head directly with the pass-2 SHA and re-reviewed the complete six-file PR against the pass-3 contract. The new head is one commit ahead. Canonical CI run 1531 is green for this exact head: npm test exercised pretest -> check:arch -> check:types, check-boundaries scanned 1893 files/10 rules clean, shell-guardrails-arch.test.ts passed 153 tests, and the resize-handle contract passed 31 tests. The overall PR still changes only CHANGELOG/build tooling/declarations/tests; no runtime src/**, CSS, or dependency file changed.

Pass-2 reassessment

The exact pass-2 repros are materially fixed: direct const options mutation, direct mutable body-alias reassignment, a const ESC = 'Escape' comparison, style-rule -> at-rule -> position: fixed, final CSS width without a semicolon, and the production double-parser-batch path all now have implementation/test coverage. The remaining blockers are neighboring equivalent value-flow/syntax paths plus one new false-positive regression.

P1 — shell-body-mount still misses a mutable Document alias reassigned before .body

laterAssignmentResolvesToDocumentBody() is only consulted after the append receiver itself resolves to a non-const variable declaration (for example body.appendChild(...)). A .body access takes a different path: resolvesToDocumentBody() immediately asks resolveGlobalKind(expr.expression), and resolveGlobalKind() classifies an untyped variable only from its original initializer. It never consults later whole-binding writes to that Document/Window alias.

This valid body mount is therefore still invisible:

function rogue() {
  let doc = window;
  doc = document;
  doc.body.appendChild(panel);
}

Please make mutable Document/Window alias resolution use the same conservative write-awareness as mutable body aliases (or fail closed when the current value cannot be proven), and add this exact sabotage case.

P1 — Capture-options mutation detection only catches direct = writes through the original identifier

hasCapturePropertyMutation() requires an EqualsToken, requires the property receiver to be an Identifier, and then requires that receiver's symbol to resolve directly to the original options VariableDeclaration. That closes opts.capture = true, but not a write through an alias to the same object:

const onKey = (e) => { if (e.key === 'Escape') close(); };
const opts = { capture: false };
const alias = opts;
alias.capture = true;
document.addEventListener('keydown', onKey, opts);

At runtime opts.capture is true; the analyzer still trusts opts's original { capture: false } literal and drops the listener as non-capture. Compound writes such as opts.capture ||= true are also skipped because they are not plain = binary assignments.

Please conservatively follow simple object aliases / writes to the same binding identity and cover assignment operators that can change capture, or fail closed once the object is aliased/escapes. Add alias-write and compound-assignment sabotage fixtures.

P1 — The new fail-closed Escape comparison rule mis-flags legitimate non-Escape capture listeners

classifyEscapeComparison() now returns ambiguous when the value compared to event.key/event.code cannot be reduced to a const string literal, and containsEscapeSemantics() deliberately treats ambiguous exactly like Escape.

That fixes the earlier false negative by introducing a direct violation of the pass-3 invariant that non-Escape capture-keydown listeners must not be mis-flagged. For example:

let expected = 'ArrowDown';
const onKey = (e) => { if (e.key === expected) noteInteraction(); };
document.addEventListener('keydown', onKey, true);

This is a non-Escape listener but is classified as an Escape lifecycle and rejected outside the policy table. The same happens with const expected = getConfiguredKey().

Please keep Escape recognition positive/provable rather than converting every unresolved key/code comparison into Escape semantics, or explicitly introduce a distinct uncheckable-semantics policy and revise the acceptance contract if conservative rejection is intended. Add a dynamic non-Escape characterization test; the current contract says it must stay clean.

P1 — shell-capture-escape still has a syntactic call-shape bypass

captureEscapeCandidates() accepts only PropertyAccessExpression callees whose name is exactly addEventListener. The equivalent static bracket spelling is never a candidate:

const onKey = (e) => { if (e.key === 'Escape') close(); };
document['addEventListener']('keydown', onKey, true);

That is a real global capture-phase Escape lifecycle but silently bypasses the rule. This is especially inconsistent with shell-body-mount, which deliberately recognizes static bracket spellings for .body and .appendChild.

Please recognize static ElementAccessExpression spellings of addEventListener (and add a sabotage fixture). A const alias for the 'keydown' event-name argument is the analogous string-value case and should either resolve or fail closed rather than disappear before policy lookup.

P1 — The independent resize-width contract remains bypassable with standard CSS identifier escapes

The resize test recognizes handle selectors and the width property with raw regular expressions; unlike the fixed-position scanner, it never decodes CSS identifier escapes. A browser-equivalent property spelling such as this is therefore omitted from cssValues:

.col-resize, .inspector-resize { width: 7px; }
.inspector-resize { \77idth: 8px; }

\77idth decodes to width, so the browser can render the inspector handle at 8px while the test still sees only the canonical 7px declaration and passes. That violates the inherited requirement that the JS/CSS thickness cannot drift unnoticed.

Please make the independent extractor escape-aware (it can keep an independent local decoder) or conservatively reject escaped property/class spellings in rules targeting either resize class, and add an escaped-width sabotage fixture.

P2 — Mutable body-alias cycles can recurse indefinitely inside the architecture check

The new laterAssignmentResolvesToDocumentBody() recursively calls resolvesToDocumentBody() on assignment RHS values but carries no visited-binding set. A valid alias cycle such as:

let a = document.createElement('div');
let b = a;
a = b;
a.appendChild(panel);

causes a -> later assignment b -> initializer a -> later assignment b -> ... in the analyzer even though this is not a Document-body mount. Add a seen set keyed by resolved declaration/symbol through the recursive body-resolution path and a non-mount cycle regression test so check:arch cannot stack-overflow on legal source.

The production wiring itself is now correct: body/capture forward + strict reverse checks run through one findShellGuardrailSourceContractViolations(..., { completeTree: true }) parser batch, fixed-position forward/reverse checks are both reachable from build/check-boundaries.mjs, policy entries remain scope/count-bound rather than filename-wide, and there is still no raw source.includes(...) gate on these analyzers.

VERDICT: REVISE

[BorisTyshkevich]

Coordinator note — merging without a formal VERDICT: SHIP certification, by explicit human decision.

Two formal ChatGPT pr-mode review sessions (6 total passes across c644ced0-... and
64bc5c7d-...) each hit their 3-pass cap without a clean pass. Every finding across both
sessions was accepted, independently verified against the real repository, and fixed —
none rejected, none left unresolved:

• Session 1 (pre-restructuring): 3 passes, all variants of one root cause — a hand-rolled
  scope-resolution layer approximating JS/TS binding semantics. Fixed by restructuring
  the resolver onto the real TypeScript checker (checker.getSymbolAtLocation), per
  569e089/ad4f71f/e76c8a7/0c3c6d4. Independently confirmed structurally sound by
  a second internal review (tested against 4 new shadow shapes the original tests never
  covered).
• Session 2 (post-restructuring): 3 more passes, a different and adjacent problem —
  proving a captured value/identifier is never mutated or aliased after declaration.
  Each pass closed one concrete shape (9b530ec/5d5d166/aa5a4e5); the next pass found
  another. This class of problem (full points-to/alias analysis) is open-ended by nature.

Current head aa5a4e5 is CI-green and gate-green (100/100/97/100 coverage, 7533 tests).
The repo owner reviewed this exact tradeoff and decided to merge now rather than spend a
third review session chasing further mutation/aliasing shapes — real adversarial code
exploiting the remaining gap is a narrower threat than #592's actual concern (an ordinary
developer re-copying the old overlay-lifecycle pattern), and a mechanical grep/AST-light
architecture guard was never expected to close every adversarial JS shape. Follow-up
proposal to tighten the "provably clean" classification to fail-closed-by-default filed
as #673.