Skip to content

Tighten shell-guardrail 'provably clean' classification to fail-closed by default #673

Description

@BorisTyshkevich

Context

Filed while shipping #592 (PR #672 — mechanical shell-primitive architecture guards in
build/check-boundaries.mjs / build/lib/check-legacy-owners.mjs).

Two formal ChatGPT pr-mode review sessions (6 total passes) each found and the
coordinator verified/fixed real defects in the new shell-body-mount and
shell-capture-escape analyzers. The first session's 3 passes were all variants of one
root cause — a hand-rolled scope-resolution layer approximating JS/TS binding semantics
— fixed by a structural restructuring to real TypeScript-checker-based symbol
resolution (checker.getSymbolAtLocation), independently confirmed sound by a second
internal review.

The second session's 3 passes (after the restructuring) were a different, adjacent
problem: proving a captured JS value/identifier is never mutated or aliased after its
own declaration. Each pass closed one concrete shape and the next pass found another:

  • let/var reassignment of a capture-options identifier or a Document/Window alias
    (fixed for resolveCaptureFlag/resolveHandlerNode/the body resolver, then found
    missing in the Document/Window resolver itself in the very next pass).
  • Direct property mutation on a const-declared options object (opts.capture = true).
  • The SAME mutation via an alias of the original identifier (const a = opts; a.capture = true), and via a compound assignment operator (opts.capture ||= true).
  • An unresolved const ESC = 'Escape' alias for the Escape-comparison string literal.
  • A bracket-notation call spelling (document['addEventListener'](...)) never
    considered a candidate at all.

All of the above were fixed and are live on main as of PR #672's merge. But "does this
identifier's bound value ever change, through any alias, any operator, anywhere in
scope" is a full points-to/alias-analysis problem — genuinely open-ended. The
coordinator and the repo owner judged shipping the current state as the right call (real
adversarial code exploiting this is a much narrower threat than #592's actual concern —
an ordinary developer re-introducing a copy-pasted overlay lifecycle — and a mechanical
grep/AST-light architecture guard, per this repo's own stated design philosophy for
check-boundaries.mjs, was never going to fully close arbitrary adversarial JS), but the
gap is real and worth a deliberate follow-up decision rather than silent acceptance.

Proposal

Rather than continuing to chase individual mutation/aliasing shapes as they're found (the
pattern that produced 6 review passes), consider narrowing what the analyzer classifies
as "provably safe"
to an intentionally small, easily-exhaustible set of syntactic
shapes — e.g., a bare literal directly in the call, or a const binding with a literal
initializer that a cheap same-file check proves is never re-referenced by any OTHER
identifier and never has any property-mutation expression (<name>.<prop> = ... in any
form, including compound operators) anywhere in the enclosing file. Everything else
(aliases, mutation of any kind, unresolved identifiers) fails closed as
uncheckable-options/uncheckable-handler — an unconditional violation requiring a
human-reviewed, explicit allowlist entry — rather than the analyzer trying to prove a
negative about ever-more-exotic mutation shapes.

This flips the maintenance burden: today, each new bypass shape discovered requires a
resolver code change to "understand" it; under the proposal, only the (much smaller,
finite) set of provably-safe shapes needs code, and everything else is conservatively
rejected by construction — closer to this repo's existing check-boundaries.mjs
philosophy elsewhere (frozen exact allowlists, fail loud rather than fail permissive).

Files

  • build/lib/check-legacy-owners.mjsresolveCaptureFlag, resolveHandlerNode,
    resolveGlobalKind/classifyGlobalDeclaration, resolvesToDocumentBody,
    hasCapturePropertyMutation.
  • tests/unit/shell-guardrails-arch.test.ts — the sabotage matrix would need updating to
    match a narrower "provably safe" contract (many currently-passing-because-permissive
    fixtures would flip to expecting uncheckable-*).

Why deferred

Out of scope for #592 itself (enforcement-only, and the current state already ships
real, valuable protection against the actual regrowth pattern #586/#587 fixed); this is
a deliberate hardening decision for a human to schedule, not an emergency.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    inboxFiled mid-task; not yet triaged into the roadmap

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions