Tighten shell-guardrail 'provably clean' classification to fail-closed by default
Nobody has claimed this yet.
Assessment
- Difficulty
- 5/5
- Estimated time
- Over a week
- Newbie friendliness
- 35/100
- Issue type
- Refactor
- Clarity
- Mostly clear
- Activity status
- Quiet
- Tech stack
- typescript
- Domain
- build-system, testing-qa, tooling
Research direction
Start in build/lib/check-legacy-owners.mjs by reading resolveCaptureFlag, resolveHandlerNode, resolveGlobalKind/classifyGlobalDeclaration, resolvesToDocumentBody, and hasCapturePropertyMutation. Then inspect tests/unit/shell-guardrails-arch.test.ts and its sabotage matrix. Done means the analyzer accepts only the deliberately narrow provably-safe shapes and classifies aliases, mutations, and unresolved identifiers as uncheckable-* with updated test expectations.
Written by the indexing model from the issue text.
Description
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/varreassignment of a capture-options identifier or a Document/Window alias
(fixed forresolveCaptureFlag/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.mjs—resolveCaptureFlag,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 expectinguncheckable-*).
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.
- Dominant language
- TypeScript
- Stars
- 8
- Forks
- 2
- Avg merge
- 1h 34m
- Merged PRs (30d)
- 6
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from Altinity/altinity-sql-browser
-
inbox
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
Altinity/altinity-sql-browser#605 ·
-
inbox
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Altinity/altinity-sql-browser#509 ·
-
inbox
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Altinity/altinity-sql-browser#489 ·
-
flamegraph Openenhancement
Difficulty 5/5 Over a week Newbie friendliness 25/100
Altinity/altinity-sql-browser#684 ·
-
bug
Difficulty 4/5 3-5 days Newbie friendliness 68/100
Altinity/altinity-sql-browser#680 · 2 comments ·
All issues in Altinity/altinity-sql-browser
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
safetrustcr/dApp-SafeTrust#426 ·
-
area:workflow bug ready-for-agent
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
fil-donadoni/tolaria#4409 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
Fission-AI/OpenSpec#1960 ·
-
Add dependabot Open
Difficulty 2/5 1-3 hours Newbie friendliness 70/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
corsairdev/corsair#1764 ·