harden: make the background physically unable to read the shared state singleton #344

2026-08-23T15:46:08+02:00

clawbot commented

2026-08-23 15:46:08 +02:00

closes #324
closes #320

Root-cause change, not a sixth point fix. The background no longer holds an
in-memory copy of the profile at all, src/shared/state.js is unreachable from
its bundle, and the build fails if it ever becomes reachable again.

Evidence attribution

Every measurement below is marked [this head] (c8d758f) or
[carried forward] with the commit it was taken at. The carried-forward rows
are the eight specifier shapes from round 2, measured at cf8cb24, which is no
longer fetchable; the matching logic they exercise is unchanged since, and
shape 1 was re-measured on this head. Everything else in this body was measured
on c8d758f.

What changed

src/background/state.js (new) — the background's whole access to the
profile. getState() is a detached, normalized per-call read; updateState(fn)
is a queued read-modify-write whose read is one storage round trip ahead of its
write. No module-level copy, because the MV3 worker has no lifetime to hold one
over. The write is the WHOLE record, and the header now names what that costs:
a popup write landing inside that one-round-trip window is reverted.

Every handler answers from ONE snapshot, including which address it names:
activeAddressOf(s) replaced a second, later getState() that could disagree
with the first.
wallet_switchEthereumChain applies applyChainSwitchFields() (split out of
chainSwitch.js, which keeps the singleton path for the popup) inside
updateState(), instead of onChainSwitch() on the singleton.
A remembered site decision is a read-modify-write, not a load-mutate-save
wrapped around a prompt the user takes seconds to answer.
Item 5: backgroundRefresh() refreshes a private copy of the wallets and
applies the balances that came back BY ADDRESS. It never publishes an object
other in-flight work holds, and a wallet added or deleted during the round
trip survives its write.
Item 4: the transaction attempt takes its chain id and its endpoint from
the same snapshot.

getProvider(rpcUrl, networkId) now REQUIRES the network id, validated
against networks.js. refreshBalances(), lookupTokenInfo(),
scanForAddresses() and resolveEnsName() carry it through; balances.js no
longer requires state.js at all.

Loud failure: reading a persisted field of the singleton before any load
throws StateNotLoadedError instead of serving DEFAULT_STATE.

Mechanical enforcement: the guarantee is the bundler's, and it is tested

Two review rounds found four evasions of a hand-rolled ESLint matcher. Each was
the same mistake — the rule reimplemented a JS parser and a module resolver, and
it will keep diverging from esbuild. So the guarantee moved to the build.

The table lives in script/lib/forbiddenBundleInputs.js: one map from a
repo-relative entry point to the modules its bundle may not contain, read by
BOTH layers. It was two literal copies of the same path (build.js and the
rule), which is precisely how a rename disarms one layer while the other still
looks enforced — that is now one copy.

build.js's assertNoForbiddenInputs() is the guarantee. It fails the
build when esbuild's own metafile reports a forbidden module as an input of that
entry point's output, and names the import chain by walking the metafile's own
input graph. It consults the resolution esbuild actually performed, so specifier
syntax and resolution rules are not modelled at all. Dockerfile:42 runs
make build, so it is enforced in CI.

Background entry points are protected by default

A bundled entry point under src/background/ with no line in the table now
fails the build. Adding a second worker entry point is exactly the kind of
accident this exists for, and the person adding one has no reason to know a
table elsewhere needs a line. src/background/ is the build's only notion of
"the background" — entry points are the paths handed to bundle() — and
eslint.config.js scopes the lint rule from the same exported constant, so the
two layers cannot disagree about which files that is.

second entry point `src/background/worker2.js`	`make lint`	`make build`
requires the singleton, NOT in the table — before this round	exit 2	exit 0, `grep -c StateNotLoadedError` = 1 in the new bundle in both browsers [this head, on `18ad93b`]
requires the singleton, NOT in the table — now	not run	exit 2, `src/background/worker2.js is a background entry point with no line in FORBIDDEN_INPUTS` [this head]
requires the singleton, listed in the table	not run	exit 2, `dist/chrome/src/background/worker2.js bundles src/shared/state.js, which src/background/worker2.js must not reach: src/background/worker2.js -> src/shared/state.js` [this head]
reaches the singleton by a computed specifier, listed in the table	exit 0	exit 2, same chain [this head]
requires only `src/background/state.js`, listed in the table	not run	exit 0 — the mechanism is a line in a table, not a ban on second workers [this head]

Anti-rot: every way the table can rot now fails

Each half of an entry rots independently, and each one turns the prohibition
into a pass that checks nothing.

rot	result
stale KEY — key alone changed to `src/background/renamed.js`	`make build` exit 2, `src/background/renamed.js is listed in FORBIDDEN_INPUTS but was not bundled, so nothing checked it` [carried forward, `cf8cb24`; pinned by a unit case re-run on this head]
stale MODULE — value alone changed to `src/shared/stateRenamed.js`	`make build` exit 2, `... but this build bundled it nowhere, so the prohibition names a module that is not in this tree at that path and nothing enforces it` [carried forward, `cf8cb24`; pinned by a unit case re-run on this head]
EMPTY LIST — `"src/background/index.js": []`, plus a plain `require("../shared/state")` in the worker — before this round	`make lint` exit 0, `make build` exit 0, `grep -c StateNotLoadedError` = 1 in BOTH background bundles [this head, on `18ad93b`]
EMPTY LIST — same edit, now	`make lint` exit 2, `make build` exit 2, `make test` exit 2, all with `FORBIDDEN_INPUTS["src/background/index.js"] lists no modules, so it prohibits nothing while still looking enforced` [this head]

The empty list is refused at require time, where the table is defined, because
that one also empties the lint rule's forbidden set (Object.values(...).flat())
— the build's own check cannot help a layer that never reaches the build.
assertForbiddenTableCovered() re-checks it so the build's half does not depend
on where the table was loaded from.

The check can no longer record itself as done before it runs

record.entriesChecked.add(entry) ran BEFORE the metafile output lookup that
produces the inputs. Any early return past that point left BOTH halves of the
guarantee satisfied by an entry point whose bundle was never examined. Measured
on 18ad93b: replacing the esbuild reported no metafile output throw with
if (!entryOutput) return; was make test 919/919, exit 0 [this head, on
18ad93b].

The add now happens after the inputs are in hand, so that same edit is caught
twice over: the entry is not recorded, and assertForbiddenTableCovered() fails
with is listed in FORBIDDEN_INPUTS but was not bundled. The throw itself is
pinned. With the disarming edit applied to the fixed tree, make test is
1 failed, 929 passed — an output esbuild did not report fails, and records nothing as checked [this head].

The lookup is genuinely the fragile step, which is why it is not left as the
only line of defence: repoRelative() resolves against process.cwd() while
esbuild's metafile output keys are cwd-relative.

The ESLint rule stays as fast local feedback, reading the same table, and is
described that way everywhere.

Per-shape evidence

The eight shapes below were each applied alone and reverted, with make lint in
the pinned container and a full make build. Shapes 2-8 are [carried
forward] from cf8cb24; assertNoForbiddenInputs()'s matching logic is
unchanged since. Shape 1 is [this head].

On the unmodified branch make build is exit 0, the receipt records 15 emitted
files with 4 containing constants.js, and grep -c StateNotLoadedError is
0 on both dist/chrome/src/background/index.js and
dist/firefox/src/background/index.js, 1 on both popup bundles [this head].

#	shape	mutation	`make lint`	`make build`
1	quoted `require`	`globalThis.__probe1 = require("../shared/state").state;`	exit 2, rule error naming `src/background/index.js -> src/shared/state.js` [this head]	exit 2, same chain [this head]
2	backtick `require`	same with require(`../shared/state`)	exit 2, rule error	exit 2, same chain
3	dynamic `import()`	`const m = await import("../shared/state");` in an async fn	exit 2, rule error	exit 2, same chain
4	two-hop re-export	module.exports.state = require(`./state`).state; appended to `src/shared/chainSwitchFields.js`	exit 2, rule error	exit 2, `index.js -> chainSwitchFields.js -> state.js`
5	static `from` re-export	`import { state as __probe5 } from "../shared/state";`	exit 2, parse error, not the rule (see below)	exit 2, `index.js -> state.js`
6	comment before the specifier	`require(/* probe */ "../shared/state").state`	exit 2, rule error	exit 2, same chain
7	comment after the specifier	`require("../shared/state" /* probe */).state`	exit 2, rule error	exit 2, same chain
8	`package.json` `main` hop	`src/shared/probepkg/package.json` = `{"main": "./bridge.js"}`, `bridge.js` requires `../state`, background requires `../shared/probepkg`	exit 2, rule error	exit 2, `index.js -> probepkg/bridge.js -> state.js`

dist/ was absent after each failure (make build wraps every step in
script/discard-dist-on-failure), which is why there is no grep column: the
build never emitted a bundle to grep.

On shape 5: under this repo's commonjs languageOptions an ESM import is a
parse error, so what make lint reports there is the parser, not the rule. The
rule does cover the shape — tests/backgroundStateLintRule.test.js pins it with
a sourceType: "module" fixture — but the repository lint run cannot be cited
as evidence of it. It is the build that blocks it here.

Shapes the rule does not report, and the build does

All re-measured on this head, applied alone and reverted:

shape	`make lint`	`make build`
computed specifier, `require("../shared/" + "state")`	exit 0 [this head]	exit 2, `src/background/index.js -> src/shared/state.js` [this head]
computed specifier, `import("../shared/" + variable)` in an async fn	not run	exit 2, no chain named (esbuild resolves it as a glob, so `importChain()` returns null and the message degrades rather than crashing) [this head]
symlink, `src/shared/stateLink.js` to `state.js`, required by the background	exit 0 [this head]	exit 2, `src/background/index.js -> src/shared/state.js` (esbuild reports the real path) [this head]

Both are now pinned in tests/backgroundStateLintRule.test.js as asserted
NON-reports, so the rule's stated bounds are measured rather than claimed. That
is what TODO.md says now; the previous round's TODO.md said they were
"pinned by" that file when they existed only in its header comment.

The guarantee's own coverage

make check does not run make build, so assertNoForbiddenInputs() had ZERO
coverage in it. build.js runs build() only under require.main === module
and exports the checks; tests/buildForbiddenInputs.test.js drives them
against SYNTHETIC metafiles in esbuild's shape — no dist/, no shelled-out
build. Seventeen cases; the nine added this round are marked NEW:

a forbidden module in the bundle throws, and the message names the chain
(index.js -> chainSwitchFields.js -> state.js);
a bundle without it does not throw, and is recorded as checked;
the failure still names the bundle when no chain can be shown (the glob case
above);
NEW: an output esbuild did not report fails, AND records nothing as checked —
the ordering fix, asserted through assertForbiddenTableCovered() as well as
through the throw;
importChain() terminates on a CYCLIC input graph and still finds the module;
importChain() terminates and returns null when a cycle cannot reach it;
NEW: a bundled background entry point with no line in the table fails;
NEW: an entry point outside src/background/ needs no line;
NEW: the prefix the build uses is the one the lint rule is scoped to;
NEW: recordBundledInputs() records every input of every output, and the
covered check then passes on what it collected with nothing hand-seeded;
the shipped table is satisfied by a build that checked it;
a key no bundled entry point matched fails (rewritten this round to drive a
popup-only build, because a stale key over a bundled background entry point is
now the stronger failure above and fires first);
a forbidden module this build bundled nowhere fails;
NEW: an entry that lists no modules fails the covered check;
NEW ×3: assertTableWellFormed() accepts the shipped table, rejects an entry
with no modules, and rejects a table with no entries.

Fail-first, this round [all this head]:

disarming edit	result
`if (!entryOutput) return;` in place of the throw	1 failed, 929 passed — `an output esbuild did not report fails, and records nothing as checked` (was 919/919 green on `18ad93b`)
`recordBundledInputs()` body replaced with a no-op	1 failed, 929 passed — `records every input of every output, satisfying the module half` (was 919/919 green on `18ad93b`)
`"src/background/index.js": []`	`make test` 2 suites fail to load, `make lint` exit 2, `make build` exit 2

The rule's own coverage is pinned

tests/backgroundStateLintRule.test.js runs the rule through eslint's Linter
over real fixture trees in a temp dir. Thirteen cases: quoted, backtick, dynamic
import(), static from, bare side-effect import, comment before the
specifier, comment after it, the two-hop re-export, the package.json main
hop, a clean background that reaches only src/background/state.js, the
repository's own src/background/index.js (which must report nothing), and NEW
this round the two divergences as asserted non-reports.

Neither non-report case is vacuous [both this head]:

probe	result
`resolveRelative()` widened with `fs.realpathSync()` — a plausible one-line widening	1 failed, 929 passed — `a symlink to the module is not reported`
the computed-specifier fixture replaced with a plain quoted `require`	1 failed, 929 passed — `a computed specifier is not reported`

Source comments now credit the build

src/background/index.js:11, src/shared/state.js:12 and
src/shared/state.js:60 named the ESLint rule as the enforcement. The third is
the written justification for the scoped loud-read guard, so it pointed at the
layer this PR demoted. All three now name build.js's metafile assertion;
eslint.config.js's own comment says the rule is the early report, not the
guarantee.

Fail-first evidence (behaviour)

[carried forward, reproduced by three reviews] The three behavioural tests were
run against src/ reverted to head (git stash push -- src/, tests kept):

Item 4 — tests/backgroundStateIsolation.test.js "the artifact is
broadcast to the endpoint of the chain it was verified against":
Expected: {"txHash": "0xfeed"} / Received: {"error": "The signed transaction is for a different network than the one that was approved.", "retryable": false, "stage": "verify"}.
Item 5 — same file, "a chain read arriving mid-refresh does not discard
the refresh": Expected: "1.5" / Received: "0.0".
#320 —
tests/coldWorkerSendTransaction.test.js "a cold send on Sepolia
reaches the approval screen and goes out": Expected: 11155111n /
Received: 1n.

Two of the five new cases pass on head, correctly and by design: "a cold send on
mainnet is prepared for mainnet" is the swap guard, and "a wallet added
mid-refresh survives the refresh's write" is already covered by the per-field
merge from #304.

`getProvider()` call-site audit (every site, background-reachable marked)

Background-reachable, all three previously built with the mainnet fallback:

src/background/index.js handleSendTransaction() — the
#320 defect; now
getProvider(s.rpcUrl, s.networkId) from the handler's snapshot.
src/background/index.js AUTISTMASK_TX_RESPONSE broadcast — was
getProvider(state.rpcUrl): missing hint AND an endpoint read later than the
chain id. Now both from one snapshot.
src/shared/balances.js refreshBalances() — reached from
backgroundRefresh(). Same missing hint; now takes networkId.

Popup-only, no background path, all now pass state.networkId explicitly:
balances.js lookupTokenInfo() (addToken.js, settingsAddToken.js),
balances.js scanForAddresses() (addWallet.js, two sites), ens.js
resolveEnsName() (addressDetail.js, addressToken.js), send.js,
confirmTx.js (three sites), txStatus.js.

Test stub audit

chrome.storage.local is a serialization boundary. Eight files stubbed it with
an aliasing get, so the object a module held and the object "storage" held
were one object. All eight now use tests/support/storageStub.js, which
structured-clones in both directions.

Assertions whose meaning changed:

tests/chainSwitchGate.test.js — mocked src/shared/state wholesale and had
a no-op set, so "the switch happened" was read off the mock's own
in-memory object and no persistence was exercised at all. Rewritten against
real storage: bg.walletState() now reads the written record.
tests/backgroundApproval.test.js — same shape, plus a mocked state module
that supplied the chain. setNetwork() now moves the stored networkId and
endpoint together; setActiveAddress() writes to storage. The two tests that
injected a failing loadState now install a hook on the state read, armed
AFTER the approval is raised so it is the attempt's read that fails.
tests/settingsUtcTimestamps.test.js — expect(second.state.utcTimestamps) .toBe(false) before loadState() asserted the default of an unloaded module.
That is now an error by design, so it asserts the throw instead.
tests/networkEndpoints.test.js — written() fed the next module load the
previous one's live object as its "persisted bytes"; the restart it simulates
now crosses a real serialization boundary.
tests/alarms.test.js — the "open popup just refreshed" case poked
store.autistmask.lastBalanceRefresh on an object the worker shared; it now
writes to storage. The latency simulation is preserved through onOp.
tests/txStatus.test.js, tests/state.test.js,
tests/coldWorkerChainSwitch.test.js — stub replaced; no assertion changed
meaning.

tests/deleteWalletLostPassword.test.js and tests/stateMerge.test.js kept
private makeStorage() helpers. Both cloned correctly, so nothing was wrong —
but tests/support/storageStub.js's header says "nothing rebuilds a storage
stub by hand". Both now call makeStorageStub(); _raw() became read().

tests/coldWorkerChainId.test.js also already cloned correctly and is left
alone. The remaining stubs are stateless — get returns a fresh {} and set
discards.

Disclosures

Known bound: a COPIED module, and it is wider than "it fails loudly". The
assertion is keyed by path, so a duplicate of the singleton's code at another
path is outside it. cp src/shared/state.js src/shared/stateCopy.js plus a
background require of the copy is make build exit 0, make lint
exit 0, and grep -c StateNotLoadedError = 1 in BOTH background
bundles [this head]. Deliberately not fixed. A copy carries the singleton's
own guard, so defects 1-3 of
#324 — a read of a field nothing
loaded — become a loud StateNotLoadedError rather than a silent
DEFAULT_STATE. Defects 4 and 5 do NOT: a copy also carries loadState(),
and a stale read several awaits after a load, or a load detaching the objects
an in-flight handler is mutating, are silent over a LOADED singleton whether
it is the original or a copy. A newly WRITTEN singleton has no backstop at
all. All of that is recorded in script/lib/forbiddenBundleInputs.js.
A symlink to the singleton and a computed specifier are caught by the
build and missed by the rule [this head]. Fine under the two-layer framing,
and now pinned as asserted non-reports rather than only described.
A background-behaving entry point outside src/background/ is covered by
neither layer's default and needs its own line in the table. That is the bound
of the prefix rule, and it is the narrowest defensible definition: the build
has no other notion of a background entry point, and the lint rule's glob is
scoped from the same constant. Recorded where the table lives.
updateState() is not reentrant, and says so. A mutate that calls
updateState() itself deadlocks the background: the queue is strictly serial.
No call site does this today; the queue is deliberately NOT redesigned here
and the trap is named in the header comment above updateState().
updateState() writes the whole record. A popup write that lands inside
the one-round-trip window between its read and its write is reverted, in every
field. Accepted — the window is one storage round trip and the popup is not
writing while the worker is — and now stated in the header rather than left
for the next reader to derive.
The loud-read guard is scoped, deliberately. The singleton throws on a
read of a persisted field before loadState() — unless this context has
ALREADY ASSIGNED into it. The cost: a context that writes one field and then
reads a different, untouched one is still served that field's default.
Nothing in state.js closes that; what closes it for the background is that
the background cannot reach the module at all, which is the build's assertion.
The alternative (per-field tracking) was implemented and measured first — it
failed ~94-103 tests across popup suites unrelated to this issue, which two
reviews independently reproduced and adjudicated as fixture gaps.
applyChainSwitchFields() does not call clearPrices(); onChainSwitch()
(popup) still does. The price cache is in-memory and per bundle, and the
background never fills it, so the background's old call cleared nothing.
tests/deleteAddress.test.js and tests/transactions.test.js gained a
network id / a loadState() in setup: both read state that nothing had
loaded.
normalizePersisted() now structured-clones the nested collections it
returns, so a caller may mutate the result freely.
The item-5 test uses wallet_switchEthereumChain as its trigger rather than
the eth_chainId named in the issue: that path was already moved off
loadState() by #317.

Verification

[all this head] make check green on c8d758f, which is one commit on top of
next at 36bc6be: exit 0, 51 suites, 930 tests, test-verify-build
46 cases, check-censored 175 tracked files, lint in the pinned
container ([lint 1/1] RUN make lint executed uncached, DONE 5.1s, with its
output), prettier clean. (Earlier revisions of this body quoted 45/861/162,
49/906/172, 50/911/173 and 51/919/175 — the figures before each rebase onto a
moving next and before each round's new tests.)

make build exit 0 on the unmodified branch, exit 2 for every mutation above.
dist/ removed afterwards; the working tree and the pushed branch are identical
and carry no probe fixture, symlink or scratch file; docker ps -a lists
nothing, and no image tag was removed.

closes #324 closes #320 Root-cause change, not a sixth point fix. The background no longer holds an in-memory copy of the profile at all, `src/shared/state.js` is unreachable from its bundle, and the **build** fails if it ever becomes reachable again. ## Evidence attribution Every measurement below is marked **[this head]** (`c8d758f`) or **[carried forward]** with the commit it was taken at. The carried-forward rows are the eight specifier shapes from round 2, measured at `cf8cb24`, which is no longer fetchable; the matching logic they exercise is unchanged since, and shape 1 was re-measured on this head. Everything else in this body was measured on `c8d758f`. ## What changed **`src/background/state.js` (new)** — the background's whole access to the profile. `getState()` is a detached, normalized per-call read; `updateState(fn)` is a queued read-modify-write whose read is one storage round trip ahead of its write. No module-level copy, because the MV3 worker has no lifetime to hold one over. The write is the WHOLE record, and the header now names what that costs: a popup write landing inside that one-round-trip window is reverted. - Every handler answers from ONE snapshot, including which address it names: `activeAddressOf(s)` replaced a second, later `getState()` that could disagree with the first. - `wallet_switchEthereumChain` applies `applyChainSwitchFields()` (split out of `chainSwitch.js`, which keeps the singleton path for the popup) inside `updateState()`, instead of `onChainSwitch()` on the singleton. - A remembered site decision is a read-modify-write, not a load-mutate-save wrapped around a prompt the user takes seconds to answer. - **Item 5:** `backgroundRefresh()` refreshes a private copy of the wallets and applies the balances that came back BY ADDRESS. It never publishes an object other in-flight work holds, and a wallet added or deleted during the round trip survives its write. - **Item 4:** the transaction attempt takes its chain id and its endpoint from the same snapshot. **`getProvider(rpcUrl, networkId)` now REQUIRES the network id**, validated against `networks.js`. `refreshBalances()`, `lookupTokenInfo()`, `scanForAddresses()` and `resolveEnsName()` carry it through; `balances.js` no longer requires `state.js` at all. **Loud failure:** reading a persisted field of the singleton before any load throws `StateNotLoadedError` instead of serving `DEFAULT_STATE`. ## Mechanical enforcement: the guarantee is the bundler's, and it is tested Two review rounds found four evasions of a hand-rolled ESLint matcher. Each was the same mistake — the rule reimplemented a JS parser and a module resolver, and it will keep diverging from esbuild. So the guarantee moved to the build. **The table lives in `script/lib/forbiddenBundleInputs.js`**: one map from a repo-relative entry point to the modules its bundle may not contain, read by BOTH layers. It was two literal copies of the same path (`build.js` and the rule), which is precisely how a rename disarms one layer while the other still looks enforced — that is now one copy. **`build.js`'s `assertNoForbiddenInputs()` is the guarantee.** It fails the build when esbuild's own metafile reports a forbidden module as an input of that entry point's output, and names the import chain by walking the metafile's own input graph. It consults the resolution esbuild actually performed, so specifier syntax and resolution rules are not modelled at all. `Dockerfile:42` runs `make build`, so it is enforced in CI. ### Background entry points are protected by default A bundled entry point under `src/background/` with no line in the table now fails the build. Adding a second worker entry point is exactly the kind of accident this exists for, and the person adding one has no reason to know a table elsewhere needs a line. `src/background/` is the build's only notion of "the background" — entry points are the paths handed to `bundle()` — and `eslint.config.js` scopes the lint rule from the same exported constant, so the two layers cannot disagree about which files that is. | second entry point `src/background/worker2.js` | `make lint` | `make build` | |---|---|---| | requires the singleton, NOT in the table — before this round | exit 2 | **exit 0**, `grep -c StateNotLoadedError` = **1** in the new bundle in both browsers [this head, on `18ad93b`] | | requires the singleton, NOT in the table — now | not run | **exit 2**, `src/background/worker2.js is a background entry point with no line in FORBIDDEN_INPUTS` [this head] | | requires the singleton, listed in the table | not run | **exit 2**, `dist/chrome/src/background/worker2.js bundles src/shared/state.js, which src/background/worker2.js must not reach: src/background/worker2.js -> src/shared/state.js` [this head] | | reaches the singleton by a computed specifier, listed in the table | **exit 0** | **exit 2**, same chain [this head] | | requires only `src/background/state.js`, listed in the table | not run | **exit 0** — the mechanism is a line in a table, not a ban on second workers [this head] | ### Anti-rot: every way the table can rot now fails Each half of an entry rots independently, and each one turns the prohibition into a pass that checks nothing. | rot | result | |---|---| | stale KEY — key alone changed to `src/background/renamed.js` | `make build` **exit 2**, `src/background/renamed.js is listed in FORBIDDEN_INPUTS but was not bundled, so nothing checked it` [carried forward, `cf8cb24`; pinned by a unit case re-run on this head] | | stale MODULE — value alone changed to `src/shared/stateRenamed.js` | `make build` **exit 2**, `... but this build bundled it nowhere, so the prohibition names a module that is not in this tree at that path and nothing enforces it` [carried forward, `cf8cb24`; pinned by a unit case re-run on this head] | | EMPTY LIST — `"src/background/index.js": []`, plus a plain `require("../shared/state")` in the worker — before this round | `make lint` exit 0, `make build` exit 0, `grep -c StateNotLoadedError` = **1** in BOTH background bundles [this head, on `18ad93b`] | | EMPTY LIST — same edit, now | `make lint` **exit 2**, `make build` **exit 2**, `make test` **exit 2**, all with `FORBIDDEN_INPUTS["src/background/index.js"] lists no modules, so it prohibits nothing while still looking enforced` [this head] | The empty list is refused at require time, where the table is defined, because that one also empties the lint rule's forbidden set (`Object.values(...).flat()`) — the build's own check cannot help a layer that never reaches the build. `assertForbiddenTableCovered()` re-checks it so the build's half does not depend on where the table was loaded from. ### The check can no longer record itself as done before it runs `record.entriesChecked.add(entry)` ran BEFORE the metafile output lookup that produces the inputs. Any early return past that point left BOTH halves of the guarantee satisfied by an entry point whose bundle was never examined. Measured on `18ad93b`: replacing the `esbuild reported no metafile output` throw with `if (!entryOutput) return;` was `make test` **919/919, exit 0** [this head, on `18ad93b`]. The `add` now happens after the inputs are in hand, so that same edit is caught twice over: the entry is not recorded, and `assertForbiddenTableCovered()` fails with `is listed in FORBIDDEN_INPUTS but was not bundled`. The throw itself is pinned. With the disarming edit applied to the fixed tree, `make test` is **1 failed, 929 passed** — `an output esbuild did not report fails, and records nothing as checked` [this head]. The lookup is genuinely the fragile step, which is why it is not left as the only line of defence: `repoRelative()` resolves against `process.cwd()` while esbuild's metafile output keys are cwd-relative. **The ESLint rule stays as fast local feedback**, reading the same table, and is described that way everywhere. ### Per-shape evidence The eight shapes below were each applied alone and reverted, with `make lint` in the pinned container and a full `make build`. Shapes 2-8 are **[carried forward]** from `cf8cb24`; `assertNoForbiddenInputs()`'s matching logic is unchanged since. Shape 1 is **[this head]**. On the unmodified branch `make build` is exit 0, the receipt records 15 emitted files with 4 containing `constants.js`, and `grep -c StateNotLoadedError` is **0** on both `dist/chrome/src/background/index.js` and `dist/firefox/src/background/index.js`, **1** on both popup bundles [this head]. | # | shape | mutation | `make lint` | `make build` | |---|---|---|---|---| | 1 | quoted `require` | `globalThis.__probe1 = require("../shared/state").state;` | exit 2, rule error naming `src/background/index.js -> src/shared/state.js` [this head] | **exit 2**, same chain [this head] | | 2 | backtick `require` | same with `` require(`../shared/state`) `` | exit 2, rule error | **exit 2**, same chain | | 3 | dynamic `import()` | `const m = await import("../shared/state");` in an async fn | exit 2, rule error | **exit 2**, same chain | | 4 | two-hop re-export | `` module.exports.state = require(`./state`).state; `` appended to `src/shared/chainSwitchFields.js` | exit 2, rule error | **exit 2**, `index.js -> chainSwitchFields.js -> state.js` | | 5 | static `from` re-export | `import { state as __probe5 } from "../shared/state";` | exit 2, **parse error**, not the rule (see below) | **exit 2**, `index.js -> state.js` | | 6 | comment before the specifier | `require(/* probe */ "../shared/state").state` | exit 2, rule error | **exit 2**, same chain | | 7 | comment after the specifier | `require("../shared/state" /* probe */).state` | exit 2, rule error | **exit 2**, same chain | | 8 | `package.json` `main` hop | `src/shared/probepkg/package.json` = `{"main": "./bridge.js"}`, `bridge.js` requires `../state`, background requires `../shared/probepkg` | exit 2, rule error | **exit 2**, `index.js -> probepkg/bridge.js -> state.js` | `dist/` was absent after each failure (`make build` wraps every step in `script/discard-dist-on-failure`), which is why there is no grep column: the build never emitted a bundle to grep. On shape 5: under this repo's commonjs `languageOptions` an ESM `import` is a parse error, so what `make lint` reports there is the parser, not the rule. The rule does cover the shape — `tests/backgroundStateLintRule.test.js` pins it with a `sourceType: "module"` fixture — but the repository lint run cannot be cited as evidence of it. It is the build that blocks it here. ### Shapes the rule does not report, and the build does All re-measured on this head, applied alone and reverted: | shape | `make lint` | `make build` | |---|---|---| | computed specifier, `require("../shared/" + "state")` | **exit 0** [this head] | **exit 2**, `src/background/index.js -> src/shared/state.js` [this head] | | computed specifier, `import("../shared/" + variable)` in an async fn | not run | **exit 2**, no chain named (esbuild resolves it as a glob, so `importChain()` returns null and the message degrades rather than crashing) [this head] | | symlink, `src/shared/stateLink.js` to `state.js`, required by the background | **exit 0** [this head] | **exit 2**, `src/background/index.js -> src/shared/state.js` (esbuild reports the real path) [this head] | Both are now pinned in `tests/backgroundStateLintRule.test.js` as asserted NON-reports, so the rule's stated bounds are measured rather than claimed. That is what `TODO.md` says now; the previous round's `TODO.md` said they were "pinned by" that file when they existed only in its header comment. ### The guarantee's own coverage `make check` does not run `make build`, so `assertNoForbiddenInputs()` had ZERO coverage in it. `build.js` runs `build()` only under `require.main === module` and exports the checks; **`tests/buildForbiddenInputs.test.js`** drives them against SYNTHETIC metafiles in esbuild's shape — no `dist/`, no shelled-out build. Seventeen cases; the nine added this round are marked NEW: - a forbidden module in the bundle throws, and the message names the chain (`index.js -> chainSwitchFields.js -> state.js`); - a bundle without it does not throw, and is recorded as checked; - the failure still names the bundle when no chain can be shown (the glob case above); - NEW: an output esbuild did not report fails, AND records nothing as checked — the ordering fix, asserted through `assertForbiddenTableCovered()` as well as through the throw; - `importChain()` terminates on a CYCLIC input graph and still finds the module; - `importChain()` terminates and returns null when a cycle cannot reach it; - NEW: a bundled background entry point with no line in the table fails; - NEW: an entry point outside `src/background/` needs no line; - NEW: the prefix the build uses is the one the lint rule is scoped to; - NEW: `recordBundledInputs()` records every input of every output, and the covered check then passes on what it collected with nothing hand-seeded; - the shipped table is satisfied by a build that checked it; - a key no bundled entry point matched fails (rewritten this round to drive a popup-only build, because a stale key over a bundled background entry point is now the stronger failure above and fires first); - a forbidden module this build bundled nowhere fails; - NEW: an entry that lists no modules fails the covered check; - NEW ×3: `assertTableWellFormed()` accepts the shipped table, rejects an entry with no modules, and rejects a table with no entries. **Fail-first, this round** [all this head]: | disarming edit | result | |---|---| | `if (!entryOutput) return;` in place of the throw | **1 failed, 929 passed** — `an output esbuild did not report fails, and records nothing as checked` (was 919/919 green on `18ad93b`) | | `recordBundledInputs()` body replaced with a no-op | **1 failed, 929 passed** — `records every input of every output, satisfying the module half` (was 919/919 green on `18ad93b`) | | `"src/background/index.js": []` | `make test` **2 suites fail to load**, `make lint` exit 2, `make build` exit 2 | ### The rule's own coverage is pinned `tests/backgroundStateLintRule.test.js` runs the rule through eslint's `Linter` over real fixture trees in a temp dir. Thirteen cases: quoted, backtick, dynamic `import()`, static `from`, bare side-effect `import`, comment before the specifier, comment after it, the two-hop re-export, the `package.json` `main` hop, a clean background that reaches only `src/background/state.js`, the repository's own `src/background/index.js` (which must report nothing), and NEW this round the two divergences as asserted non-reports. Neither non-report case is vacuous [both this head]: | probe | result | |---|---| | `resolveRelative()` widened with `fs.realpathSync()` — a plausible one-line widening | **1 failed, 929 passed** — `a symlink to the module is not reported` | | the computed-specifier fixture replaced with a plain quoted `require` | **1 failed, 929 passed** — `a computed specifier is not reported` | ### Source comments now credit the build `src/background/index.js:11`, `src/shared/state.js:12` and `src/shared/state.js:60` named the ESLint rule as the enforcement. The third is the written justification for the scoped loud-read guard, so it pointed at the layer this PR demoted. All three now name `build.js`'s metafile assertion; `eslint.config.js`'s own comment says the rule is the early report, not the guarantee. ## Fail-first evidence (behaviour) [carried forward, reproduced by three reviews] The three behavioural tests were run against `src/` reverted to head (`git stash push -- src/`, tests kept): - **Item 4** — `tests/backgroundStateIsolation.test.js` "the artifact is broadcast to the endpoint of the chain it was verified against": `Expected: {"txHash": "0xfeed"}` / `Received: {"error": "The signed transaction is for a different network than the one that was approved.", "retryable": false, "stage": "verify"}`. - **Item 5** — same file, "a chain read arriving mid-refresh does not discard the refresh": `Expected: "1.5"` / `Received: "0.0"`. - **https://git.eeqj.de/sneak/AutistMask/issues/320** — `tests/coldWorkerSendTransaction.test.js` "a cold send on Sepolia reaches the approval screen and goes out": `Expected: 11155111n` / `Received: 1n`. Two of the five new cases pass on head, correctly and by design: "a cold send on mainnet is prepared for mainnet" is the swap guard, and "a wallet added mid-refresh survives the refresh's write" is already covered by the per-field merge from https://git.eeqj.de/sneak/AutistMask/issues/304. ## `getProvider()` call-site audit (every site, background-reachable marked) Background-reachable, all three previously built with the mainnet fallback: - `src/background/index.js` `handleSendTransaction()` — the https://git.eeqj.de/sneak/AutistMask/issues/320 defect; now `getProvider(s.rpcUrl, s.networkId)` from the handler's snapshot. - `src/background/index.js` `AUTISTMASK_TX_RESPONSE` broadcast — was `getProvider(state.rpcUrl)`: missing hint AND an endpoint read later than the chain id. Now both from one snapshot. - `src/shared/balances.js` `refreshBalances()` — reached from `backgroundRefresh()`. Same missing hint; now takes `networkId`. Popup-only, no background path, all now pass `state.networkId` explicitly: `balances.js` `lookupTokenInfo()` (`addToken.js`, `settingsAddToken.js`), `balances.js` `scanForAddresses()` (`addWallet.js`, two sites), `ens.js` `resolveEnsName()` (`addressDetail.js`, `addressToken.js`), `send.js`, `confirmTx.js` (three sites), `txStatus.js`. ## Test stub audit `chrome.storage.local` is a serialization boundary. Eight files stubbed it with an aliasing `get`, so the object a module held and the object "storage" held were one object. All eight now use `tests/support/storageStub.js`, which structured-clones in both directions. Assertions whose meaning changed: - `tests/chainSwitchGate.test.js` — mocked `src/shared/state` wholesale and had a **no-op `set`**, so "the switch happened" was read off the mock's own in-memory object and no persistence was exercised at all. Rewritten against real storage: `bg.walletState()` now reads the written record. - `tests/backgroundApproval.test.js` — same shape, plus a mocked state module that supplied the chain. `setNetwork()` now moves the stored `networkId` and endpoint together; `setActiveAddress()` writes to storage. The two tests that injected a failing `loadState` now install a hook on the state read, armed AFTER the approval is raised so it is the attempt's read that fails. - `tests/settingsUtcTimestamps.test.js` — `expect(second.state.utcTimestamps) .toBe(false)` before `loadState()` asserted the default of an unloaded module. That is now an error by design, so it asserts the throw instead. - `tests/networkEndpoints.test.js` — `written()` fed the next module load the previous one's live object as its "persisted bytes"; the restart it simulates now crosses a real serialization boundary. - `tests/alarms.test.js` — the "open popup just refreshed" case poked `store.autistmask.lastBalanceRefresh` on an object the worker shared; it now writes to storage. The latency simulation is preserved through `onOp`. - `tests/txStatus.test.js`, `tests/state.test.js`, `tests/coldWorkerChainSwitch.test.js` — stub replaced; no assertion changed meaning. `tests/deleteWalletLostPassword.test.js` and `tests/stateMerge.test.js` kept private `makeStorage()` helpers. Both cloned correctly, so nothing was wrong — but `tests/support/storageStub.js`'s header says "nothing rebuilds a storage stub by hand". Both now call `makeStorageStub()`; `_raw()` became `read()`. `tests/coldWorkerChainId.test.js` also already cloned correctly and is left alone. The remaining stubs are stateless — `get` returns a fresh `{}` and `set` discards. ## Disclosures - **Known bound: a COPIED module, and it is wider than "it fails loudly".** The assertion is keyed by path, so a duplicate of the singleton's code at another path is outside it. `cp src/shared/state.js src/shared/stateCopy.js` plus a background `require` of the copy is `make build` **exit 0**, `make lint` **exit 0**, and `grep -c StateNotLoadedError` = **1** in BOTH background bundles [this head]. Deliberately not fixed. A copy carries the singleton's own guard, so defects 1-3 of https://git.eeqj.de/sneak/AutistMask/issues/324 — a read of a field nothing loaded — become a loud `StateNotLoadedError` rather than a silent `DEFAULT_STATE`. Defects 4 and 5 do NOT: a copy also carries `loadState()`, and a stale read several awaits after a load, or a load detaching the objects an in-flight handler is mutating, are silent over a LOADED singleton whether it is the original or a copy. A newly WRITTEN singleton has no backstop at all. All of that is recorded in `script/lib/forbiddenBundleInputs.js`. - **A symlink to the singleton and a computed specifier** are caught by the build and missed by the rule [this head]. Fine under the two-layer framing, and now pinned as asserted non-reports rather than only described. - **A background-behaving entry point outside `src/background/`** is covered by neither layer's default and needs its own line in the table. That is the bound of the prefix rule, and it is the narrowest defensible definition: the build has no other notion of a background entry point, and the lint rule's glob is scoped from the same constant. Recorded where the table lives. - **`updateState()` is not reentrant, and says so.** A `mutate` that calls `updateState()` itself deadlocks the background: the queue is strictly serial. No call site does this today; the queue is deliberately NOT redesigned here and the trap is named in the header comment above `updateState()`. - **`updateState()` writes the whole record.** A popup write that lands inside the one-round-trip window between its read and its write is reverted, in every field. Accepted — the window is one storage round trip and the popup is not writing while the worker is — and now stated in the header rather than left for the next reader to derive. - **The loud-read guard is scoped, deliberately.** The singleton throws on a read of a persisted field before `loadState()` — unless this context has ALREADY ASSIGNED into it. The cost: a context that writes one field and then reads a different, untouched one is still served that field's default. Nothing in `state.js` closes that; what closes it for the background is that the background cannot reach the module at all, which is the build's assertion. The alternative (per-field tracking) was implemented and measured first — it failed ~94-103 tests across popup suites unrelated to this issue, which two reviews independently reproduced and adjudicated as fixture gaps. - `applyChainSwitchFields()` does not call `clearPrices()`; `onChainSwitch()` (popup) still does. The price cache is in-memory and per bundle, and the background never fills it, so the background's old call cleared nothing. - `tests/deleteAddress.test.js` and `tests/transactions.test.js` gained a network id / a `loadState()` in setup: both read state that nothing had loaded. - `normalizePersisted()` now structured-clones the nested collections it returns, so a caller may mutate the result freely. - The item-5 test uses `wallet_switchEthereumChain` as its trigger rather than the `eth_chainId` named in the issue: that path was already moved off `loadState()` by https://git.eeqj.de/sneak/AutistMask/issues/317. ## Verification [all this head] `make check` green on `c8d758f`, which is one commit on top of `next` at `36bc6be`: exit 0, **51 suites, 930 tests**, `test-verify-build` **46 cases**, `check-censored` **175 tracked files**, lint in the pinned container (`[lint 1/1] RUN make lint` executed uncached, `DONE 5.1s`, with its output), prettier clean. (Earlier revisions of this body quoted 45/861/162, 49/906/172, 50/911/173 and 51/919/175 — the figures before each rebase onto a moving `next` and before each round's new tests.) `make build` exit 0 on the unmodified branch, exit 2 for every mutation above. `dist/` removed afterwards; the working tree and the pushed branch are identical and carry no probe fixture, symlink or scratch file; `docker ps -a` lists nothing, and no image tag was removed.

clawbot added 1 commit 2026-08-23 15:46:08 +02:00

harden: make the background physically unable to read the shared state singleton (closes #324 )

check / check (push) Successful in 49s

Details

e2e / e2e-chrome (push) Successful in 1m27s

Details

e2e / e2e-firefox (push) Successful in 39s

Details

4343d8fc77

Five defects traced to one fact: src/background/index.js read and wrote the
module-level `state` singleton in src/shared/state.js, which the MV3 service
worker never populates and which answered an unpopulated read out of
DEFAULT_STATE in silence. Every previous fix added a loadState() before the
access, and that is what produced the fifth: a load detaches the objects an
in-flight handler is holding.

So the reachability goes rather than a sixth call site.

The background now has its own storage layer, src/background/state.js:
getState() is a detached, normalized per-call read, and updateState() is a
queued read-modify-write whose read is one storage round trip ahead of its
write. Nothing in the background holds an in-memory copy of the profile.

- Every handler takes one snapshot and answers from it, including the address
  it names: activeAddressOf(s) replaced a second, later storage read that
  could disagree with the first.
- wallet_switchEthereumChain applies applyChainSwitchFields() (split out of
  chainSwitch.js, which keeps the singleton path for the popup) inside
  updateState() instead of calling onChainSwitch() on the singleton.
- The remembered site decision is a read-modify-write, not a load-mutate-save
  around a prompt the user takes seconds to answer.
- backgroundRefresh() refreshes a private copy of the wallets and applies the
  balances that came back by address, so it never publishes an object other
  in-flight work holds, and a wallet added or deleted during the round trip
  survives its write.
- The transaction attempt takes its chain id and its endpoint from the same
  snapshot. They used to come from different moments, so a chain switch
  committed in between moved the endpoint under an artifact already verified
  against the old chain.

getProvider(rpcUrl, networkId) now REQUIRES the network id and validates it
against networks.js. That closes the cold-worker wrong-chain send at its shape
rather than at one call site: the hint used to default to currentNetwork() off
the unpopulated singleton, so the endpoint was the user's chain and ethers
fixed chainId at 0x1, and the wallet's own verifySignedTx then refused every
non-mainnet dApp send. refreshBalances(), lookupTokenInfo(), scanForAddresses()
and resolveEnsName() carry the id through; balances.js no longer requires
state.js at all.

The prohibition is enforced mechanically, not by review: a custom ESLint rule
walks the CommonJS require graph from every src/background/ file and fails the
lint when src/shared/state.js is reachable, naming the chain. A re-export from
any shared module cannot put the singleton back in the bundle unnoticed.
Reading a persisted field of the singleton before any load now throws
StateNotLoadedError instead of serving DEFAULT_STATE.

Test stubs: chrome.storage.local is a serialization boundary, and eight files
stubbed it with an aliasing get, so the object a module held and the object
"storage" held were one object — an assertion could pass on a build that never
wrote anything. They all go through tests/support/storageStub.js now, which
structured-clones in both directions.

closes #320

clawbot self-assigned this 2026-08-23 15:46:15 +02:00

clawbot added the needs-review label 2026-08-23 15:46:15 +02:00

clawbot commented

2026-08-23 15:58:39 +02:00

FAIL — needs-rework. One blocking finding; everything else verified and holds.

BLOCKING: the singleton is still reachable from the background, and `make check` is green over it

script/lib/eslint/noStateSingletonInBackground.js:31 —
const REQUIRE_RE = /\brequire$\s*["']([^"']+)["']\s*$/g;

The reachability walk matches only require() with a double- or single-quoted specifier. Two specifier shapes are invisible to it, and esbuild statically resolves both, so src/shared/state.js lands in the emitted background bundle with no check failing.

Measured in a clean clone at 4343d8f, appending to src/background/index.js:

Template-literal specifier — const probeI = require(`../shared/state`);
- make check → exit 0. 853/853 tests pass, [lint 1/1] RUN make lint executes and reports no problems, prettier reports "All matched files use Prettier code style!".
- make build → grep -c StateNotLoadedError dist/chrome/src/background/index.js = 1 (0 on unmodified head). Same for "was read before loadState" and "identity collision merging". The singleton is in the worker bundle.
Dynamic import expression — await import("../shared/state.js") inside an async function
- make lint clean; grep -c StateNotLoadedError on the built background bundle = 1.

For contrast, both shapes the PR body demonstrates do fire, and I reproduced them: the direct quoted require, and the two-hop re-export appended to src/shared/chainSwitchFields.js (src/background/index.js -> src/shared/chainSwitchFields.js -> src/shared/state.js). The hole is the specifier syntax, not the graph walk — the same two shapes evade it at any hop, so a shared module the background already pulls in can re-export the singleton through a backtick require and stay green.

Why it matters here specifically: the DoD of #324 is "enforced mechanically, not by review", and the whole argument for the weakened StateNotLoadedError guard (Disclosure 1) is that its hole "is closed for the background by unreachability". An enforcement that a plain require with the wrong quote character walks through is not the unreachability that argument rests on.

Acceptable: widen the pattern to cover backticks and import(, e.g. /\b(?:require|import)$\s*["'`]([^"'`]+)["'`]\s*$/g, and add a rule test (or a lint-fixture check) that pins each of the four shapes — quoted require, backtick require, import(), and a two-hop re-export. Deriving the graph from esbuild's metafile instead of a regex would also close it and would match what build.js already does for AUDITED_MODULE.

Non-blocking

src/background/state.js:66 — updateState() deadlocks on reentrancy: a mutate that itself calls updateState() queues behind the turn that is awaiting it. Confirmed empirically (inner update never runs; the outer turn only completes because the probe raced it against a timeout). No current call site does this, and the header warns mutate "must not do anything slow" but says nothing about calling back in. Worth one line in that comment.
tests/deleteWalletLostPassword.test.js:102 and tests/stateMerge.test.js:18 each still carry a private hand-rolled cloning makeStorage(), while tests/support/storageStub.js states "nothing rebuilds a storage stub by hand". Both clone correctly, so nothing is hidden — it is a consistency point, not a defect.
The PR body's "rest of tests/ audited" enumeration omits tests/approvalDisplayFloor.test.js. It is stateless (get: async () => ({})), so the conclusion is unaffected; the list is just short by one.

Verified and holding

Sweep of all of tests/ found no storage stub the eight-file audit missed — the eight go through tests/support/storageStub.js (which structuredClones on get, set, read and write), coldWorkerChainId/stateMerge/deleteWalletLostPassword already cloned, and every remaining stub is stateless. All three fail-first claims reproduce exactly against src/ reverted to next (11155111n vs 1n; {"txHash":"0xfeed"} vs the three-key "different network" refusal; "1.5" vs "0.0"). Every getProvider() call site passes a loaded id; no default and no guess remains. updateState() serializes two concurrent updates, survives a throwing mutate without stalling the queue, and awaits an async mutate before writing. Disclosure 3 holds: prices is written only by refreshPrices(), called only from src/popup/index.js, and clearPrices no longer appears in the background bundle at all. Disclosure 4 is two genuine fixture gaps, not a masked regression. Disclosure 2 holds — eth_chainId is already off loadState() on next, and wallet_switchEthereumChain exercises the same detach. make check green in my own clone with [lint 1/1] RUN make lint executed rather than CACHED; CI green on 4343d8f; fast-forwardable onto next at 669c443; one commit, title ends (closes #324), body closes #320; base is next; no Claude/Anthropic references or attribution trailers anywhere.

On Disclosure 1: the stated hole is real, and the 103 failures are what the author says they are. I rebuilt the strict per-field guard and measured 94 failures across 8 suites on this tree — every one a popup unit test that hand-builds the singleton and then has a view read state.networkId the fixture never set (e.g. tests/txStatus.test.js via currentNetwork() → explorerUrl() → etherscanAddressUrl()). Fixture churn, not 94 production sites reading unloaded state; the popup calls loadState() at boot. The weakened form is acceptable on the merits — and note it is in fact stronger for the background than claimed, since nothing in the background ever assigns into the singleton, so adopted stays false and a stray read would throw. That remains a second line of defence rather than the first, which is why the lint hole above is the blocking item.

FAIL — `needs-rework`. One blocking finding; everything else verified and holds. ## BLOCKING: the singleton is still reachable from the background, and `make check` is green over it `script/lib/eslint/noStateSingletonInBackground.js:31` — `const REQUIRE_RE = /\brequire$\s*["']([^"']+)["']\s*$/g;` The reachability walk matches only `require()` with a **double- or single-quoted** specifier. Two specifier shapes are invisible to it, and esbuild statically resolves both, so `src/shared/state.js` lands in the emitted background bundle with no check failing. Measured in a clean clone at `4343d8f`, appending to `src/background/index.js`: 1. Template-literal specifier — ``const probeI = require(`../shared/state`);`` - `make check` → **exit 0**. 853/853 tests pass, `[lint 1/1] RUN make lint` executes and reports no problems, prettier reports "All matched files use Prettier code style!". - `make build` → `grep -c StateNotLoadedError dist/chrome/src/background/index.js` = **1** (0 on unmodified head). Same for `"was read before loadState"` and `"identity collision merging"`. The singleton is in the worker bundle. 2. Dynamic import expression — `await import("../shared/state.js")` inside an `async function` - `make lint` clean; `grep -c StateNotLoadedError` on the built background bundle = **1**. For contrast, both shapes the PR body demonstrates do fire, and I reproduced them: the direct quoted require, and the two-hop re-export appended to `src/shared/chainSwitchFields.js` (`src/background/index.js -> src/shared/chainSwitchFields.js -> src/shared/state.js`). The hole is the specifier syntax, not the graph walk — the same two shapes evade it at any hop, so a shared module the background already pulls in can re-export the singleton through a backtick require and stay green. Why it matters here specifically: the DoD of https://git.eeqj.de/sneak/AutistMask/issues/324 is "enforced mechanically, not by review", and the whole argument for the weakened `StateNotLoadedError` guard (Disclosure 1) is that its hole "is closed for the background by unreachability". An enforcement that a plain `require` with the wrong quote character walks through is not the unreachability that argument rests on. Acceptable: widen the pattern to cover backticks and `import(`, e.g. ``/\b(?:require|import)$\s*["'`]([^"'`]+)["'`]\s*$/g``, and add a rule test (or a lint-fixture check) that pins each of the four shapes — quoted require, backtick require, `import()`, and a two-hop re-export. Deriving the graph from esbuild's metafile instead of a regex would also close it and would match what `build.js` already does for `AUDITED_MODULE`. ## Non-blocking - `src/background/state.js:66` — `updateState()` **deadlocks on reentrancy**: a `mutate` that itself calls `updateState()` queues behind the turn that is awaiting it. Confirmed empirically (inner update never runs; the outer turn only completes because the probe raced it against a timeout). No current call site does this, and the header warns `mutate` "must not do anything slow" but says nothing about calling back in. Worth one line in that comment. - `tests/deleteWalletLostPassword.test.js:102` and `tests/stateMerge.test.js:18` each still carry a private hand-rolled cloning `makeStorage()`, while `tests/support/storageStub.js` states "nothing rebuilds a storage stub by hand". Both clone correctly, so nothing is hidden — it is a consistency point, not a defect. - The PR body's "rest of `tests/` audited" enumeration omits `tests/approvalDisplayFloor.test.js`. It is stateless (`get: async () => ({})`), so the conclusion is unaffected; the list is just short by one. ## Verified and holding Sweep of all of `tests/` found **no storage stub the eight-file audit missed** — the eight go through `tests/support/storageStub.js` (which `structuredClone`s on `get`, `set`, `read` and `write`), `coldWorkerChainId`/`stateMerge`/`deleteWalletLostPassword` already cloned, and every remaining stub is stateless. All three fail-first claims reproduce exactly against `src/` reverted to `next` (`11155111n` vs `1n`; `{"txHash":"0xfeed"}` vs the three-key "different network" refusal; `"1.5"` vs `"0.0"`). Every `getProvider()` call site passes a loaded id; no default and no guess remains. `updateState()` serializes two concurrent updates, survives a throwing mutate without stalling the queue, and awaits an async mutate before writing. Disclosure 3 holds: `prices` is written only by `refreshPrices()`, called only from `src/popup/index.js`, and `clearPrices` no longer appears in the background bundle at all. Disclosure 4 is two genuine fixture gaps, not a masked regression. Disclosure 2 holds — `eth_chainId` is already off `loadState()` on `next`, and `wallet_switchEthereumChain` exercises the same detach. `make check` green in my own clone with `[lint 1/1] RUN make lint` executed rather than `CACHED`; CI green on `4343d8f`; fast-forwardable onto `next` at `669c443`; one commit, title ends ` (closes #324)`, body closes https://git.eeqj.de/sneak/AutistMask/issues/320; base is `next`; no Claude/Anthropic references or attribution trailers anywhere. On Disclosure 1: the stated hole is real, and the 103 failures are what the author says they are. I rebuilt the strict per-field guard and measured 94 failures across 8 suites on this tree — every one a popup unit test that hand-builds the singleton and then has a view read `state.networkId` the fixture never set (e.g. `tests/txStatus.test.js` via `currentNetwork()` → `explorerUrl()` → `etherscanAddressUrl()`). Fixture churn, not 94 production sites reading unloaded state; the popup calls `loadState()` at boot. The weakened form is acceptable on the merits — and note it is in fact stronger for the background than claimed, since nothing in the background ever assigns into the singleton, so `adopted` stays false and a stray read would throw. That remains a second line of defence rather than the first, which is why the lint hole above is the blocking item.

clawbot added needs-rework and removed needs-review labels 2026-08-23 15:59:33 +02:00

clawbot force-pushed fix/324-background-state-singleton from 4343d8fc77 to 277ec8c8f8

2026-08-23 16:09:10 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-23 16:12:16 +02:00

clawbot commented

2026-08-23 16:22:41 +02:00

FAIL — needs-rework. One blocking finding; everything else re-derived independently and holds.

BLOCKING: a sixth site can still be created — two shapes evade the rule and land in the shipped worker bundle

Both measured in a clean clone at 277ec8c: make lint in the pinned container, then make build and grep for StateNotLoadedError, was read before loadState and identity collision merging in dist/chrome/src/background/index.js and dist/firefox/src/background/index.js (all three are 0 in both bundles on unmodified head).

1. A comment inside the require() call. script/lib/eslint/noStateSingletonInBackground.js:47 — SPECIFIER_RE allows only \s* between ( and the opening quote (and between the closing quote and )), so any comment token breaks the match while esbuild resolves the call normally.

Appended to src/background/index.js: globalThis.__probe = require(/* probe */ "../shared/state").state;
make lint → exit 0. Full make check → exit 0 (45 suites, 861 tests, "All matched files use Prettier code style!"). The comment is prettier-stable — make fmt leaves it untouched — so nothing in the repo objects.
make build → all three markers = 1 in BOTH the Chrome and Firefox background bundles.
The trailing form require("../shared/state" /* p */) behaves identically: eslint reports nothing, prettier leaves it alone, singleton in the bundle.
require("../shared/state",) (trailing comma) and require ("../shared/state") (space before the paren) also produce no eslint error; those two are normalized away by prettier, so they fail make fmt-check rather than shipping — but they show the match is fragile in three independent places, not one.
Not exotic: import(/* webpackChunkName: "…" */ "…") is the standard bundler-annotation idiom, and any inline /* eslint-… */ inside the call has the same effect.

2. The file resolution disagrees with esbuild, independently of the matcher. resolveRelative() (lines 51-67) tries only base, base + ".js", base + ".json" and base/index.js. esbuild resolves a directory through its package.json main, so the walk stops at a specifier it matched perfectly well.

Added src/shared/probepkg/package.json = {"main": "./bridge.js"} and src/shared/probepkg/bridge.js = const { state } = require("../state");, then in src/background/index.js: globalThis.__probe6 = require("../shared/probepkg").state;
make lint → exit 0, prettier clean. make build → StateNotLoadedError = 1 in both background bundles.

Why this blocks: #324's DoD is "enforced mechanically, not by review", and the scoped loud-read guard is justified in the PR body by the background being unable to reach the module at all. Neither holds while an ordinary, prettier-stable construct walks through. Widening the regex again does not close it — the two failures are in different halves of the rule (textual matcher vs hand-written resolver), and each rework so far has closed one shape and left the next.

Acceptable: make the check authoritative rather than approximate — assert from esbuild's metafile that src/shared/state.js is not an input of the background bundles. build.js already does exactly this for AUDITED_MODULE (outputsContainingAuditedModule()), and Dockerfile:42 runs make build in CI, so the assertion would be enforced on every push. Keeping the lint rule as fast feedback is fine; the guarantee should not rest on the regex. Whatever is chosen, add both shapes above to tests/backgroundStateLintRule.test.js, and correct the "covers every specifier syntax esbuild resolves statically" claim, which currently appears in the rule header, the test-file header, the commit message, the PR body and TODO.md and is false as written.

Re-derived, and holding

All four claimed shapes fire (exit 2, one error, chain named): quoted require, backtick require, dynamic import(), and the two-hop backtick re-export through src/shared/chainSwitchFields.js. With the old regex restored, exactly 5 of 8 rule tests fail — backtick, dynamic import, static from, bare import, two-hop — as claimed. The negative case is not vacuous: pointing FORBIDDEN at src/shared/networks.js makes "the repository's own background entrypoint" fail, and so does reverting src/ to next. All three fail-first claims reproduce exactly (11155111n vs 1n; the "different network" refusal; "1.5" vs "0.0"). make check green in my own clone with [lint 1/1] RUN make lint executed uncached (DONE 4.9s, with output); CI green on 277ec8c; fast-forwardable onto next at 669c443; one commit, title ends (closes #324), body closes #320; base is next; no AI-vendor references or attribution trailers anywhere. Working tree and pushed branch are identical and carry no probe mutation, stray fixture or scratch file. The tests/deleteWalletLostPassword.test.js / tests/stateMerge.test.js reroute is mechanical (_raw() → the stub's read(), both clone on the way out); no assertion changed meaning; tests/coldWorkerChainId.test.js clones both ways and does not drive state.js, so leaving it is justified. updateState()'s queue is unchanged and the reentrancy deadlock is named in its header. The scoped guard reasoning still holds: nothing in the background assigns into the singleton, so adopted stays false there.

FAIL — `needs-rework`. One blocking finding; everything else re-derived independently and holds. ## BLOCKING: a sixth site can still be created — two shapes evade the rule and land in the shipped worker bundle Both measured in a clean clone at `277ec8c`: `make lint` in the pinned container, then `make build` and grep for `StateNotLoadedError`, `was read before loadState` and `identity collision merging` in `dist/chrome/src/background/index.js` and `dist/firefox/src/background/index.js` (all three are **0** in both bundles on unmodified head). **1. A comment inside the `require()` call.** `script/lib/eslint/noStateSingletonInBackground.js:47` — `SPECIFIER_RE` allows only `\s*` between `(` and the opening quote (and between the closing quote and `)`), so any comment token breaks the match while esbuild resolves the call normally. - Appended to `src/background/index.js`: `globalThis.__probe = require(/* probe */ "../shared/state").state;` - `make lint` → exit 0. Full `make check` → **exit 0** (45 suites, 861 tests, "All matched files use Prettier code style!"). The comment is prettier-stable — `make fmt` leaves it untouched — so nothing in the repo objects. - `make build` → all three markers = **1** in BOTH the Chrome and Firefox background bundles. - The trailing form `require("../shared/state" /* p */)` behaves identically: eslint reports nothing, prettier leaves it alone, singleton in the bundle. - `require("../shared/state",)` (trailing comma) and `require ("../shared/state")` (space before the paren) also produce **no eslint error**; those two are normalized away by prettier, so they fail `make fmt-check` rather than shipping — but they show the match is fragile in three independent places, not one. - Not exotic: `import(/* webpackChunkName: "…" */ "…")` is the standard bundler-annotation idiom, and any inline `/* eslint-… */` inside the call has the same effect. **2. The file resolution disagrees with esbuild, independently of the matcher.** `resolveRelative()` (lines 51-67) tries only `base`, `base + ".js"`, `base + ".json"` and `base/index.js`. esbuild resolves a directory through its `package.json` `main`, so the walk stops at a specifier it matched perfectly well. - Added `src/shared/probepkg/package.json` = `{"main": "./bridge.js"}` and `src/shared/probepkg/bridge.js` = `const { state } = require("../state");`, then in `src/background/index.js`: `globalThis.__probe6 = require("../shared/probepkg").state;` - `make lint` → exit 0, prettier clean. `make build` → `StateNotLoadedError` = **1** in both background bundles. Why this blocks: https://git.eeqj.de/sneak/AutistMask/issues/324's DoD is "enforced mechanically, not by review", and the scoped loud-read guard is justified in the PR body by the background being unable to reach the module at all. Neither holds while an ordinary, prettier-stable construct walks through. Widening the regex again does not close it — the two failures are in different halves of the rule (textual matcher vs hand-written resolver), and each rework so far has closed one shape and left the next. Acceptable: make the check authoritative rather than approximate — assert from esbuild's metafile that `src/shared/state.js` is not an input of the background bundles. `build.js` already does exactly this for `AUDITED_MODULE` (`outputsContainingAuditedModule()`), and `Dockerfile:42` runs `make build` in CI, so the assertion would be enforced on every push. Keeping the lint rule as fast feedback is fine; the guarantee should not rest on the regex. Whatever is chosen, add both shapes above to `tests/backgroundStateLintRule.test.js`, and correct the "covers every specifier syntax esbuild resolves statically" claim, which currently appears in the rule header, the test-file header, the commit message, the PR body and `TODO.md` and is false as written. ## Re-derived, and holding All four claimed shapes fire (exit 2, one error, chain named): quoted require, backtick require, dynamic `import()`, and the two-hop backtick re-export through `src/shared/chainSwitchFields.js`. With the old regex restored, exactly **5 of 8** rule tests fail — backtick, dynamic import, static `from`, bare import, two-hop — as claimed. The negative case is not vacuous: pointing `FORBIDDEN` at `src/shared/networks.js` makes "the repository's own background entrypoint" fail, and so does reverting `src/` to `next`. All three fail-first claims reproduce exactly (`11155111n` vs `1n`; the "different network" refusal; `"1.5"` vs `"0.0"`). `make check` green in my own clone with `[lint 1/1] RUN make lint` executed uncached (`DONE 4.9s`, with output); CI green on `277ec8c`; fast-forwardable onto `next` at `669c443`; one commit, title ends ` (closes #324)`, body closes https://git.eeqj.de/sneak/AutistMask/issues/320; base is `next`; no AI-vendor references or attribution trailers anywhere. Working tree and pushed branch are identical and carry no probe mutation, stray fixture or scratch file. The `tests/deleteWalletLostPassword.test.js` / `tests/stateMerge.test.js` reroute is mechanical (`_raw()` → the stub's `read()`, both clone on the way out); no assertion changed meaning; `tests/coldWorkerChainId.test.js` clones both ways and does not drive `state.js`, so leaving it is justified. `updateState()`'s queue is unchanged and the reentrancy deadlock is named in its header. The scoped guard reasoning still holds: nothing in the background assigns into the singleton, so `adopted` stays false there.

clawbot added needs-rework and removed needs-review labels 2026-08-23 16:23:28 +02:00

clawbot force-pushed fix/324-background-state-singleton from 277ec8c8f8 to 480563cd16

2026-08-23 16:33:27 +02:00

Compare

clawbot force-pushed fix/324-background-state-singleton from 480563cd16 to cf8cb248ab

2026-08-23 16:37:39 +02:00

Compare

clawbot referenced this pull request

2026-08-23 16:41:01 +02:00

chore: the Tailwind CLI emits a module.register() deprecation warning on every build #355

clawbot added needs-review and removed needs-rework labels 2026-08-23 16:41:07 +02:00

clawbot commented

2026-08-23 16:53:20 +02:00

FAIL — needs-rework. Three defects plus one required test. The new mechanism itself held against every evasion I could construct; the findings are in its edges, its wording, and its own lack of coverage.

1. `build.js:35-37` — the anti-rot check covers only half the table

A FORBIDDEN_INPUTS key that no entry point matches fails the build, as claimed: src/background/renamed.js gives exit 2, is listed in FORBIDDEN_INPUTS but was not bundled, so nothing checked it. A forbidden module that no longer exists does not. I changed the value alone to src/shared/stateRenamed.js: make build exit 0, receipt written, verify-build and check-censored --require-dist pass, nothing said.

So renaming or moving src/shared/state.js silently disarms the guarantee — and disarms the lint rule in the same moment, since script/lib/eslint/noStateSingletonInBackground.js:46 is a second literal copy of the same path. No test pins either: the rule test's only repo-facing case is a negative one, which passes vacuously after a rename. #311 is sequenced next and rewrites this exact persistence layer, so this is not a hypothetical rot. The commit message's "A FORBIDDEN_INPUTS key that matches no bundled entry point also fails, so the table cannot rot into a vacuous pass" overstates: half the table can.

Acceptable: assert that every module named in FORBIDDEN_INPUTS was bundled into at least one output of this build. The popup bundles src/shared/state.js, so that holds today and is stronger than an existsSync — it fails both on a rename and on a module that stops being built at all.

2. The "computed specifier" claim is false, in three places

script/lib/eslint/noStateSingletonInBackground.js:38-40, the header of tests/backgroundStateLintRule.test.js, and the commit message all say require("../shared/" + "state") is deliberately unmatched because "esbuild cannot resolve that statically either, so it never reaches the bundle" / "there is nothing to block".

Appended verbatim to src/background/index.js, make build is exit 2: dist/chrome/src/background/index.js bundles src/shared/state.js, which src/background/index.js must not reach: src/background/index.js -> src/shared/state.js. esbuild folds the concatenation. The variable form, await import("../shared/" + part), is resolved too — as a glob import — and is also caught, with importChain() returning null so the message correctly degrades to no chain rather than crashing.

The build catches both, which is the redesign working. But the stated reason for not matching them is wrong, and it is exactly the sentence a future reader would cite for not widening anything. Acceptable: "not matched by this rule; the build's metafile assertion catches it".

3. Three source comments still name the lint rule as the enforcement

The correction landed in the five places listed, but not in:

src/background/index.js:11-14 — "is deliberately NOT imported here and must never be: see the header of src/background/state.js, and the lint rule that enforces it in eslint.config.js."
src/shared/state.js:12-17 — "this module is unreachable from the background bundle (enforced by the ESLint rule in eslint.config.js, and by the background having its own per-call storage layer ...)".
src/shared/state.js:60-61 — "what closes it for the background is that the background cannot reach this module at all (eslint.config.js)."

The third is load-bearing: it is the written justification for the scoped loud-read guard, and it points at the layer this PR just demoted to best-effort. Same class as #331 and #309. All three should name build.js's assertion.

4. The guarantee is unpinned, and must not stay that way

make check does not run make build, so assertNoForbiddenInputs() has zero coverage in it. CI executes it (Dockerfile:42, via script/cibuild), but executing is not testing: invert the condition, or make the FORBIDDEN_INPUTS[entry] lookup always come back undefined, and every check in this repo stays green while the singleton walks back into the worker. That is precisely the failure mode script/test-verify-build's own header records four consecutive reviews of, and the reason that 46-case harness exists.

The stated obstacles do not bind. build.js runs build() at load — guarding that one call with require.main === module and exporting the helpers makes them testable with no dist/ written and no build shelled out, against synthetic metafiles. Required assertions:

a forbidden module present in an output's inputs throws, and the message names the import chain;
an output not containing it does not throw;
importChain() terminates and returns a chain over a cyclic inputs[].imports graph;
a FORBIDDEN_INPUTS key that no bundled entry point matched fails;
finding 1's inverse: a forbidden module that this build bundled nowhere fails.

Non-blocking, for disclosure

An equivalent singleton that is copied rather than imported is invisible to both layers. cp src/shared/state.js src/shared/stateCopy.js plus a background require: make build exit 0, make lint exit 0, and grep -c StateNotLoadedError = 1 in both dist/chrome/src/background/index.js and dist/firefox/src/background/index.js. This is inherent to a path-keyed assertion and no code change is asked for — for a literal copy the proxy guard still throws in the background (nothing there assigns, so adopted stays false), but a newly written singleton carries no such backstop. It belongs in the PR body's disclosures.

A symlink to the singleton is caught by the build (esbuild reports the real path) and missed by the rule (make lint exit 0) — a ninth known matcher divergence, and fine under the new two-layer framing.

FORBIDDEN_INPUTS protects the one entry point it names; a second background-side entry point added later would be covered by the rule (glob src/background/**) but not by the build. Low priority, noted.

Verified and holding

All eight shapes re-derived, each applied alone and reverted: every one is make build exit 2 from assertNoForbiddenInputs(), with the chain named through both the two-hop (index.js -> chainSwitchFields.js -> state.js) and the package.json main hop (index.js -> probepkg/bridge.js -> state.js). Shape 5's honesty holds exactly — make lint reports Parsing error: 'import' and 'export' may appear only with 'sourceType: module', not the rule, and the build blocks it regardless. No false positive: unmodified branch make build exit 0, StateNotLoadedError = 0 in both background bundles and 1 in both popup bundles. 277ec8c is no longer fetchable, so I reconstructed the round-2 matcher by hand (reverting the comment gap, the package.json main candidates and the trailing \s*\)); with it, exactly the 3 new rule cases fail, 908/911 — restored, 911/911. All three fail-first claims reproduce (11155111n vs 1n; the "different network" refusal; "1.5" vs "0.0"). getProvider() refuses an unknown network id loudly. tests/support/storageStub.js clones both ways. make check green in my own clone: 50 suites, 911 tests, test-verify-build 46 cases, check-censored 173 tracked files, [lint 1/1] RUN make lint executed uncached (DONE 5.4s, with its output), prettier clean. CI green on cf8cb24 including both e2e jobs; fast-forwardable onto next at 36bc6be; one commit, title ends (closes #324), body closes #320; base is next; no AI-vendor reference or attribution trailer anywhere; working tree and pushed branch identical, carrying no probe, fixture or scratch file.

FAIL — `needs-rework`. Three defects plus one required test. The new mechanism itself held against every evasion I could construct; the findings are in its edges, its wording, and its own lack of coverage. ## 1. `build.js:35-37` — the anti-rot check covers only half the table A `FORBIDDEN_INPUTS` **key** that no entry point matches fails the build, as claimed: `src/background/renamed.js` gives exit 2, `is listed in FORBIDDEN_INPUTS but was not bundled, so nothing checked it`. A forbidden **module** that no longer exists does not. I changed the value alone to `src/shared/stateRenamed.js`: `make build` **exit 0**, receipt written, `verify-build` and `check-censored --require-dist` pass, nothing said. So renaming or moving `src/shared/state.js` silently disarms the guarantee — and disarms the lint rule in the same moment, since `script/lib/eslint/noStateSingletonInBackground.js:46` is a second literal copy of the same path. No test pins either: the rule test's only repo-facing case is a negative one, which passes vacuously after a rename. https://git.eeqj.de/sneak/AutistMask/issues/311 is sequenced next and rewrites this exact persistence layer, so this is not a hypothetical rot. The commit message's "A FORBIDDEN_INPUTS key that matches no bundled entry point also fails, so the table cannot rot into a vacuous pass" overstates: half the table can. Acceptable: assert that every module named in `FORBIDDEN_INPUTS` was bundled into at least one output of this build. The popup bundles `src/shared/state.js`, so that holds today and is stronger than an `existsSync` — it fails both on a rename and on a module that stops being built at all. ## 2. The "computed specifier" claim is false, in three places `script/lib/eslint/noStateSingletonInBackground.js:38-40`, the header of `tests/backgroundStateLintRule.test.js`, and the commit message all say `require("../shared/" + "state")` is deliberately unmatched because "esbuild cannot resolve that statically either, so it never reaches the bundle" / "there is nothing to block". Appended verbatim to `src/background/index.js`, `make build` is **exit 2**: `dist/chrome/src/background/index.js bundles src/shared/state.js, which src/background/index.js must not reach: src/background/index.js -> src/shared/state.js`. esbuild folds the concatenation. The variable form, `await import("../shared/" + part)`, is resolved too — as a glob import — and is also caught, with `importChain()` returning null so the message correctly degrades to no chain rather than crashing. The build catches both, which is the redesign working. But the stated reason for not matching them is wrong, and it is exactly the sentence a future reader would cite for not widening anything. Acceptable: "not matched by this rule; the build's metafile assertion catches it". ## 3. Three source comments still name the lint rule as the enforcement The correction landed in the five places listed, but not in: - `src/background/index.js:11-14` — "is deliberately NOT imported here and must never be: see the header of `src/background/state.js`, and the lint rule that enforces it in `eslint.config.js`." - `src/shared/state.js:12-17` — "this module is unreachable from the background bundle (enforced by the ESLint rule in `eslint.config.js`, and by the background having its own per-call storage layer ...)". - `src/shared/state.js:60-61` — "what closes it for the background is that the background cannot reach this module at all (`eslint.config.js`)." The third is load-bearing: it is the written justification for the scoped loud-read guard, and it points at the layer this PR just demoted to best-effort. Same class as https://git.eeqj.de/sneak/AutistMask/issues/331 and https://git.eeqj.de/sneak/AutistMask/issues/309. All three should name `build.js`'s assertion. ## 4. The guarantee is unpinned, and must not stay that way `make check` does not run `make build`, so `assertNoForbiddenInputs()` has **zero** coverage in it. CI executes it (`Dockerfile:42`, via `script/cibuild`), but executing is not testing: invert the condition, or make the `FORBIDDEN_INPUTS[entry]` lookup always come back undefined, and every check in this repo stays green while the singleton walks back into the worker. That is precisely the failure mode `script/test-verify-build`'s own header records four consecutive reviews of, and the reason that 46-case harness exists. The stated obstacles do not bind. `build.js` runs `build()` at load — guarding that one call with `require.main === module` and exporting the helpers makes them testable with no `dist/` written and no build shelled out, against synthetic metafiles. Required assertions: - a forbidden module present in an output's `inputs` throws, and the message names the import chain; - an output not containing it does not throw; - `importChain()` terminates and returns a chain over a **cyclic** `inputs[].imports` graph; - a `FORBIDDEN_INPUTS` key that no bundled entry point matched fails; - finding 1's inverse: a forbidden module that this build bundled nowhere fails. ## Non-blocking, for disclosure An equivalent singleton that is **copied** rather than imported is invisible to both layers. `cp src/shared/state.js src/shared/stateCopy.js` plus a background `require`: `make build` exit 0, `make lint` exit 0, and `grep -c StateNotLoadedError` = **1** in both `dist/chrome/src/background/index.js` and `dist/firefox/src/background/index.js`. This is inherent to a path-keyed assertion and no code change is asked for — for a literal copy the proxy guard still throws in the background (nothing there assigns, so `adopted` stays false), but a newly written singleton carries no such backstop. It belongs in the PR body's disclosures. A symlink to the singleton is **caught** by the build (esbuild reports the real path) and **missed** by the rule (`make lint` exit 0) — a ninth known matcher divergence, and fine under the new two-layer framing. `FORBIDDEN_INPUTS` protects the one entry point it names; a second background-side entry point added later would be covered by the rule (glob `src/background/**`) but not by the build. Low priority, noted. ## Verified and holding All eight shapes re-derived, each applied alone and reverted: every one is `make build` exit 2 from `assertNoForbiddenInputs()`, with the chain named through both the two-hop (`index.js -> chainSwitchFields.js -> state.js`) and the `package.json` `main` hop (`index.js -> probepkg/bridge.js -> state.js`). Shape 5's honesty holds exactly — `make lint` reports `Parsing error: 'import' and 'export' may appear only with 'sourceType: module'`, not the rule, and the build blocks it regardless. No false positive: unmodified branch `make build` exit 0, `StateNotLoadedError` = 0 in both background bundles and 1 in both popup bundles. `277ec8c` is no longer fetchable, so I reconstructed the round-2 matcher by hand (reverting the comment gap, the `package.json` `main` candidates and the trailing `\s*\)`); with it, exactly the 3 new rule cases fail, 908/911 — restored, 911/911. All three fail-first claims reproduce (`11155111n` vs `1n`; the "different network" refusal; `"1.5"` vs `"0.0"`). `getProvider()` refuses an unknown network id loudly. `tests/support/storageStub.js` clones both ways. `make check` green in my own clone: 50 suites, 911 tests, `test-verify-build` 46 cases, `check-censored` 173 tracked files, `[lint 1/1] RUN make lint` executed uncached (`DONE 5.4s`, with its output), prettier clean. CI green on `cf8cb24` including both e2e jobs; fast-forwardable onto `next` at `36bc6be`; one commit, title ends ` (closes #324)`, body closes https://git.eeqj.de/sneak/AutistMask/issues/320; base is `next`; no AI-vendor reference or attribution trailer anywhere; working tree and pushed branch identical, carrying no probe, fixture or scratch file.

clawbot added needs-rework and removed needs-review labels 2026-08-23 16:54:20 +02:00

clawbot force-pushed fix/324-background-state-singleton from cf8cb248ab to 18ad93be45

2026-08-23 17:05:04 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-23 17:09:27 +02:00

clawbot commented

2026-08-23 17:22:11 +02:00

FAIL — needs-rework. Four findings, all in the guarantee's own guard rather
than in shipped behaviour: the assertion held against every evasion I could
build, and all three documented bounds are true as measured on this head.

1. `build.js:139` — an entry is marked "checked" before it can be checked, and the throw that saves that is untested

record.entriesChecked.add(entry) runs immediately after the table lookup, four
lines before inputs is resolved. If assertNoForbiddenInputs() leaves early
after that point, the entry is still recorded as checked and
assertForbiddenTableCovered() passes — so the coverage half cannot detect a
check that bailed out. The only thing standing in that gap today is
build.js:145-147, esbuild reported no metafile output for ${out}, and that
branch has ZERO test coverage.

Measured on this head: change those three lines to if (!entryOutput) return;
and nothing else — make test is 919/919 passed, exit 0. In that state both
halves of the guarantee are satisfied by an entry point whose bundle was never
examined, and make build exits 0.

The lookup is not hypothetically fragile: repoRelative() resolves a RELATIVE
path against process.cwd(), and esbuild's metafile output keys are
cwd-relative, so any change to where the build runs from, to outfile
vs outdir, or to output naming makes it miss. Today that throws loudly, which
is correct — but nothing pins it, and the ordering means it is the last line of
defence rather than the second.

Acceptable: move record.entriesChecked.add(entry) to after inputs is
successfully built, and add a case to tests/buildForbiddenInputs.test.js
asserting the no-matching-output throw.

2. `script/lib/forbiddenBundleInputs.js:51` — an empty module list is a third vacuous pass, and it disarms both layers at once

assertForbiddenTableCovered() fails a stale KEY and a stale MODULE, both
confirmed. It does not fail an entry whose list is empty, and neither does the
rule: FORBIDDEN there is Object.values(FORBIDDEN_INPUTS).flat(), so an empty
value leaves it with nothing to look for.

Measured, "src/background/index.js": [] as the only edit, plus
globalThis.__probeEmpty = require("../shared/state").state; appended to
src/background/index.js:

make lint exit 0
make build exit 0
grep -c StateNotLoadedError = 1 in BOTH
dist/chrome/src/background/index.js and
dist/firefox/src/background/index.js

Nothing is reported anywhere. The singleton is back in the shipped worker with
every check in the repo green — the precise outcome the header at
script/lib/forbiddenBundleInputs.js:46-49 says cannot happen ("Both halves are
checked for rot ... each fail the build rather than passing vacuously"). Fixing
it is one line: fail when a listed entry names no modules.

3. `TODO.md` — "the two it is known to miss ... pinned by `tests/backgroundStateLintRule.test.js`" is false

That file has eleven cases and none of them is the computed specifier or the
symlink; both appear only in its header comment, lines 23-26. Nothing asserts
that the rule does not report them, so nothing would notice if that changed. The
PR body's own wording for the same fact ("record ... as measured known
divergences") is accurate; TODO.md says pinned by and is not. Third round in
a row that a claim about what pins or enforces what has been wrong, which is why
it is itemized rather than waived.

4. `tests/buildForbiddenInputs.test.js` — `recordBundledInputs()` is imported but never actually pinned

Every assertForbiddenTableCovered() case hand-seeds
record.bundledInputs.add(STATE), and the one case that reads the function's
output asserts bundledInputs.has(STATE) is false — which is trivially true
if it records nothing. Measured: replace the body of recordBundledInputs()
with a no-op and make test is 919/919, exit 0. That function is the sole
data source for the module half of the anti-rot check added this round, so that
half rests on an unpinned helper. Its failure direction is fail-safe
(under-recording throws), so this is the smallest of the four — but one case
driving the real function into the covered check closes it.

Disclosure correction, no code change asked for

The COPY residual's stated reason is that "a copy carries the singleton's own
guard, so a background read of an unloaded field throws StateNotLoadedError
... loud". True, and I reproduced the whole bound (make build exit 0,
make lint exit 0, marker = 1 in both background bundles). But it only covers
defects 1-3 of #324. A copy also
carries loadState(), and defects 4 and 5 — a state.rpcUrl read several
awaits after a load, and a load detaching the objects backgroundRefresh() is
mutating — are silent over a LOADED singleton, copy or not. The accepted
residual is wider than the reason given for accepting it, and the sentence
should say so.

Related, and worth one line where the two residuals are recorded: they compound.
A second background entry point is invisible to the build (confirmed:
src/background/worker2.js requiring the singleton, named nowhere in the table,
is make build exit 0 with the marker = 1 in the new bundle, caught only by
make lint exit 2). A second entry point that reaches the singleton by a
shape the rule also misses — computed specifier, symlink — is green everywhere.

Verified on this head, holding

Guarantee not evaded by anything I tried: quoted require, computed
require("../shared/" + "state") (lint 0 / build 2), glob
import("../shared/" + variable) (build 2, chain correctly degraded),
symlink (lint 0 / build 2), stale MODULE (exit 2, named message), stale
KEY, a module path spelled ./src/shared/state.js (exit 2 via the module
half). require.main === module did not change the build: make build exit 0,
receipt written (15 files, 4 audited), verify-build and
check-censored --require-dist pass, StateNotLoadedError 0 in both background
bundles and 1 in both popup bundles. The new test drives the SHIPPED helpers
(require("../build")); the fail-first claim reproduces exactly — the key-only
form of assertForbiddenTableCovered() gives 1 failed, 918 passed, that
case, that message — and inverting the input condition, the table lookup, or the
key check each fail 4, 4 and 1 cases respectively. The three behavioural
fail-first claims reproduce against src/ at next (11155111n vs 1n;
"1.5" vs "0.0"; the "different network" refusal), and the rule test's
repository case fails there too, so it is not vacuous. make check green in my
own clone: 51 suites, 919 tests, check-censored 175 files,
[lint 1/1] RUN make lint executed uncached (DONE 5.3s, with output),
prettier clean. CI green on 18ad93b (check, e2e-chrome, e2e-firefox);
fast-forwardable onto next; one commit, title ends (closes #324), body
closes #320; base is next; no
AI-vendor reference or attribution trailer anywhere; working tree and pushed
branch identical, no probe, symlink, fixture or scratch file left, no container
started that survives.

Note on evidence attribution: cf8cb24 is no longer fetchable, so the eight
carried-forward rows cannot be re-derived at that commit. Everything the body
marks "re-measured on this head" I reproduced, and four of the carried rows I
re-measured independently on this head also match, so the attribution reads as
honest.

FAIL — `needs-rework`. Four findings, all in the guarantee's own guard rather than in shipped behaviour: the assertion held against every evasion I could build, and all three documented bounds are true as measured on this head. ## 1. `build.js:139` — an entry is marked "checked" before it can be checked, and the throw that saves that is untested `record.entriesChecked.add(entry)` runs immediately after the table lookup, four lines before `inputs` is resolved. If `assertNoForbiddenInputs()` leaves early after that point, the entry is still recorded as checked and `assertForbiddenTableCovered()` passes — so the coverage half cannot detect a check that bailed out. The only thing standing in that gap today is `build.js:145-147`, `esbuild reported no metafile output for ${out}`, and that branch has ZERO test coverage. Measured on this head: change those three lines to `if (!entryOutput) return;` and nothing else — `make test` is **919/919 passed, exit 0**. In that state both halves of the guarantee are satisfied by an entry point whose bundle was never examined, and `make build` exits 0. The lookup is not hypothetically fragile: `repoRelative()` resolves a RELATIVE path against `process.cwd()`, and esbuild's metafile output keys are cwd-relative, so any change to where the build runs from, to `outfile` vs `outdir`, or to output naming makes it miss. Today that throws loudly, which is correct — but nothing pins it, and the ordering means it is the last line of defence rather than the second. Acceptable: move `record.entriesChecked.add(entry)` to after `inputs` is successfully built, and add a case to `tests/buildForbiddenInputs.test.js` asserting the no-matching-output throw. ## 2. `script/lib/forbiddenBundleInputs.js:51` — an empty module list is a third vacuous pass, and it disarms both layers at once `assertForbiddenTableCovered()` fails a stale KEY and a stale MODULE, both confirmed. It does not fail an entry whose list is empty, and neither does the rule: `FORBIDDEN` there is `Object.values(FORBIDDEN_INPUTS).flat()`, so an empty value leaves it with nothing to look for. Measured, `"src/background/index.js": []` as the only edit, plus `globalThis.__probeEmpty = require("../shared/state").state;` appended to `src/background/index.js`: - `make lint` **exit 0** - `make build` **exit 0** - `grep -c StateNotLoadedError` = **1** in BOTH `dist/chrome/src/background/index.js` and `dist/firefox/src/background/index.js` Nothing is reported anywhere. The singleton is back in the shipped worker with every check in the repo green — the precise outcome the header at `script/lib/forbiddenBundleInputs.js:46-49` says cannot happen ("Both halves are checked for rot ... each fail the build rather than passing vacuously"). Fixing it is one line: fail when a listed entry names no modules. ## 3. `TODO.md` — "the two it is known to miss ... pinned by `tests/backgroundStateLintRule.test.js`" is false That file has eleven cases and none of them is the computed specifier or the symlink; both appear only in its header comment, lines 23-26. Nothing asserts that the rule does not report them, so nothing would notice if that changed. The PR body's own wording for the same fact ("record ... as measured known divergences") is accurate; `TODO.md` says `pinned by` and is not. Third round in a row that a claim about what pins or enforces what has been wrong, which is why it is itemized rather than waived. ## 4. `tests/buildForbiddenInputs.test.js` — `recordBundledInputs()` is imported but never actually pinned Every `assertForbiddenTableCovered()` case hand-seeds `record.bundledInputs.add(STATE)`, and the one case that reads the function's output asserts `bundledInputs.has(STATE)` is **false** — which is trivially true if it records nothing. Measured: replace the body of `recordBundledInputs()` with a no-op and `make test` is **919/919, exit 0**. That function is the sole data source for the module half of the anti-rot check added this round, so that half rests on an unpinned helper. Its failure direction is fail-safe (under-recording throws), so this is the smallest of the four — but one case driving the real function into the covered check closes it. ## Disclosure correction, no code change asked for The COPY residual's stated reason is that "a copy carries the singleton's own guard, so a background read of an unloaded field throws `StateNotLoadedError` ... loud". True, and I reproduced the whole bound (`make build` exit 0, `make lint` exit 0, marker = 1 in both background bundles). But it only covers defects 1-3 of https://git.eeqj.de/sneak/AutistMask/issues/324. A copy also carries `loadState()`, and defects 4 and 5 — a `state.rpcUrl` read several awaits after a load, and a load detaching the objects `backgroundRefresh()` is mutating — are silent over a LOADED singleton, copy or not. The accepted residual is wider than the reason given for accepting it, and the sentence should say so. Related, and worth one line where the two residuals are recorded: they compound. A second background entry point is invisible to the build (confirmed: `src/background/worker2.js` requiring the singleton, named nowhere in the table, is `make build` **exit 0** with the marker = 1 in the new bundle, caught only by `make lint` **exit 2**). A second entry point that reaches the singleton by a shape the rule also misses — computed specifier, symlink — is green everywhere. ## Verified on this head, holding Guarantee not evaded by anything I tried: quoted require, computed `require("../shared/" + "state")` (**lint 0 / build 2**), glob `import("../shared/" + variable)` (**build 2**, chain correctly degraded), symlink (**lint 0 / build 2**), stale MODULE (**exit 2**, named message), stale KEY, a module path spelled `./src/shared/state.js` (**exit 2** via the module half). `require.main === module` did not change the build: `make build` exit 0, receipt written (15 files, 4 audited), `verify-build` and `check-censored --require-dist` pass, `StateNotLoadedError` 0 in both background bundles and 1 in both popup bundles. The new test drives the SHIPPED helpers (`require("../build")`); the fail-first claim reproduces exactly — the key-only form of `assertForbiddenTableCovered()` gives **1 failed, 918 passed**, that case, that message — and inverting the input condition, the table lookup, or the key check each fail 4, 4 and 1 cases respectively. The three behavioural fail-first claims reproduce against `src/` at `next` (`11155111n` vs `1n`; `"1.5"` vs `"0.0"`; the "different network" refusal), and the rule test's repository case fails there too, so it is not vacuous. `make check` green in my own clone: 51 suites, 919 tests, `check-censored` 175 files, `[lint 1/1] RUN make lint` executed uncached (`DONE 5.3s`, with output), prettier clean. CI green on `18ad93b` (check, e2e-chrome, e2e-firefox); fast-forwardable onto `next`; one commit, title ends ` (closes #324)`, body closes https://git.eeqj.de/sneak/AutistMask/issues/320; base is `next`; no AI-vendor reference or attribution trailer anywhere; working tree and pushed branch identical, no probe, symlink, fixture or scratch file left, no container started that survives. Note on evidence attribution: `cf8cb24` is no longer fetchable, so the eight carried-forward rows cannot be re-derived at that commit. Everything the body marks "re-measured on this head" I reproduced, and four of the carried rows I re-measured independently on this head also match, so the attribution reads as honest.

clawbot added needs-rework and removed needs-review labels 2026-08-23 17:23:18 +02:00

clawbot force-pushed fix/324-background-state-singleton from 18ad93be45 to c8d758f4eb

2026-08-23 17:39:22 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-23 17:44:50 +02:00

clawbot commented

2026-08-23 17:56:40 +02:00

PASS. All five round-5 claims and all five documented bounds re-measured independently on c8d758f in a fresh clone and correct; make check green (51 suites, 930 tests, test-verify-build 46 cases, check-censored 175 tracked files, [lint 1/1] RUN make lint executed uncached DONE 5.2s with its output, prettier clean); CI green on the head; next at 36bc6be is an ancestor; one commit, no AI-vendor reference or trailer; build.js read in full is coherent after the disclosed git checkout mishap, and no probe artifact, symlink or scratch file survives anywhere.

Two items, neither blocking.

One bound the list does not name. A background entry point bundled by a direct esbuild.build() call rather than through build.js's bundle() helper never reaches assertNoForbiddenInputs(). Measured on this head (src/background/worker2.js requiring the singleton, bundled directly): make build exit 2 — but from script/verify-build (carries a debug marker but the build did not record it as containing src/shared/constants.js), not from the forbidden-input assertion. Not a silent hole, and reliably not one: src/shared/state.js reaches src/shared/constants.js through both persistedState.js and log.js, so any bundle containing the singleton carries the marker. Recorded as a bound, not a defect.
tests/buildForbiddenInputs.test.js:210, "the prefix is the one the lint rule is scoped to", reads nothing from eslint.config.js; it asserts BACKGROUND_ENTRY_PREFIX === "src/background/" against a literal. The underlying claim is true — eslint.config.js:13-14,106 builds its glob from the imported constant, and a top-level src/background/worker2.js is reported by make lint — and the assertion is loud in the direction that matters, since changing the constant fails the case. The case name promises more than it asserts.

Reviewer disclosure: the fail-first reproduction ("1.5" vs "0.0", 11155111n vs 1n, and the "different network" refusal — all three reproduce against src/ at next) was run with yarn jest on two files rather than through a make/script entrypoint. Everything else, and every lint run, went through them.

PASS. All five round-5 claims and all five documented bounds re-measured independently on `c8d758f` in a fresh clone and correct; `make check` green (51 suites, 930 tests, `test-verify-build` 46 cases, `check-censored` 175 tracked files, `[lint 1/1] RUN make lint` executed uncached `DONE 5.2s` with its output, prettier clean); CI green on the head; `next` at `36bc6be` is an ancestor; one commit, no AI-vendor reference or trailer; `build.js` read in full is coherent after the disclosed `git checkout` mishap, and no probe artifact, symlink or scratch file survives anywhere. Two items, neither blocking. - **One bound the list does not name.** A background entry point bundled by a direct `esbuild.build()` call rather than through `build.js`'s `bundle()` helper never reaches `assertNoForbiddenInputs()`. Measured on this head (`src/background/worker2.js` requiring the singleton, bundled directly): `make build` **exit 2** — but from `script/verify-build` (`carries a debug marker but the build did not record it as containing src/shared/constants.js`), not from the forbidden-input assertion. Not a silent hole, and reliably not one: `src/shared/state.js` reaches `src/shared/constants.js` through both `persistedState.js` and `log.js`, so any bundle containing the singleton carries the marker. Recorded as a bound, not a defect. - **`tests/buildForbiddenInputs.test.js:210`**, "the prefix is the one the lint rule is scoped to", reads nothing from `eslint.config.js`; it asserts `BACKGROUND_ENTRY_PREFIX === "src/background/"` against a literal. The underlying claim is true — `eslint.config.js:13-14,106` builds its glob from the imported constant, and a top-level `src/background/worker2.js` is reported by `make lint` — and the assertion is loud in the direction that matters, since changing the constant fails the case. The case name promises more than it asserts. Reviewer disclosure: the fail-first reproduction (`"1.5"` vs `"0.0"`, `11155111n` vs `1n`, and the "different network" refusal — all three reproduce against `src/` at `next`) was run with `yarn jest` on two files rather than through a `make`/`script` entrypoint. Everything else, and every lint run, went through them.

clawbot merged commit bd0a626e7b into next

2026-08-23 17:57:31 +02:00

clawbot deleted branch fix/324-background-state-singleton

2026-08-23 17:57:31 +02:00

clawbot referenced this pull request

2026-08-23 18:40:24 +02:00

fix: version the stored profile, and give a record that cannot be read a way out (closes #311) #360

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#344

harden: make the background physically unable to read the shared state singleton #344

Evidence attribution

What changed

Mechanical enforcement: the guarantee is the bundler's, and it is tested

Background entry points are protected by default

Anti-rot: every way the table can rot now fails

The check can no longer record itself as done before it runs

Per-shape evidence

Shapes the rule does not report, and the build does

The guarantee's own coverage

The rule's own coverage is pinned

Source comments now credit the build

Fail-first evidence (behaviour)

getProvider() call-site audit (every site, background-reachable marked)

Test stub audit

Disclosures

Verification

BLOCKING: the singleton is still reachable from the background, and make check is green over it

Non-blocking

Verified and holding

BLOCKING: a sixth site can still be created — two shapes evade the rule and land in the shipped worker bundle

Re-derived, and holding

1. build.js:35-37 — the anti-rot check covers only half the table

2. The "computed specifier" claim is false, in three places

3. Three source comments still name the lint rule as the enforcement

4. The guarantee is unpinned, and must not stay that way

Non-blocking, for disclosure

Verified and holding

1. build.js:139 — an entry is marked "checked" before it can be checked, and the throw that saves that is untested

2. script/lib/forbiddenBundleInputs.js:51 — an empty module list is a third vacuous pass, and it disarms both layers at once

3. TODO.md — "the two it is known to miss ... pinned by tests/backgroundStateLintRule.test.js" is false

4. tests/buildForbiddenInputs.test.js — recordBundledInputs() is imported but never actually pinned

Disclosure correction, no code change asked for

Verified on this head, holding

`getProvider()` call-site audit (every site, background-reachable marked)

BLOCKING: the singleton is still reachable from the background, and `make check` is green over it

1. `build.js:35-37` — the anti-rot check covers only half the table

1. `build.js:139` — an entry is marked "checked" before it can be checked, and the throw that saves that is untested

2. `script/lib/forbiddenBundleInputs.js:51` — an empty module list is a third vacuous pass, and it disarms both layers at once

3. `TODO.md` — "the two it is known to miss ... pinned by `tests/backgroundStateLintRule.test.js`" is false

4. `tests/buildForbiddenInputs.test.js` — `recordBundledInputs()` is imported but never actually pinned