refactor: one shared extension-API module, and drive the dApp flows on Firefox (closes #153) #281

clawbot · 2026-08-12T13:57:00+02:00

clawbot commented

2026-08-12 13:57:00 +02:00

Closes #153.

Read this first: the issue's premise does not survive its own verification

The issue says Firefox is non-functional because browser.* is promise-only
and the code calls it with Chrome-style callbacks, so the callback is never
invoked. That is false on Firefox 153.0.3, measured directly. Firefox's
browser.* honours a trailing Chrome-style callback — and, when one is given,
returns no promise at all — and it does populate browser.runtime.lastError.

Probe run inside the pinned e2e container, from the extension's own popup page:

{ "typeofBrowser": "object", "typeofChrome": "object", "browserIsChrome": false,
  "storageCallbackFired": true,        "storageReturnedThenable": false,
  "sendMessageCallbackFired": true,    "sendMessageReturnedThenable": false,
  "getLastFocusedCallbackFired": true, "getLastFocusedReturnedThenable": false,
  "getLastFocusedId": 1 }

and, against item 8 specifically (browser.tabs.sendMessage to a dead tab id):

{ "hasLastErrorProp": true, "callbackFired": true, "lastErrorSet": true,
  "lastErrorMessage": "Could not establish connection. Receiving end does not exist." }

Stronger still, end to end: I stashed every src/ change, rebuilt, and ran the
new Firefox suite against the unconverted code. 8/8, exit 0. All four
DoD flows pass without this PR's fix. Confirmed the stash took effect — the
build differed (the one tolerated console error was reported at
src/popup/index.js:6 unfixed against :1 fixed).

So this is not a repair of a broken target. What it is:

the structural change the issue asks for in its own first DoD checkbox — one
compat module, no namespace ternaries anywhere else, one call shape;
the Firefox dApp coverage that turns the six manual checks into assertions,
which is what produced the measurement above;
no defect fixed. An earlier revision of this PR claimed the
windows.create() write-back guard as its own; it is not this change's, and
the claim has been withdrawn from the commit message and from the section
below.

Reviewer's call whether the refactor is worth its diff on that basis. Nothing
in it is load-bearing for correctness on either browser today. Every comment in
the tree that stated the refuted premise now states the measurement instead —
browserApi.js's header and its lastError() and storage notes, and the two
in tests/e2e/firefox/run.js.

Strategy: (a), the promise shim

src/shared/browserApi.js is now the only file in the tree that names
browser or chrome. It exports lazily-resolved namespace handles for events
and synchronous methods (runtimeApi(), windowsApi(), tabsApi(),
actionApi(), storageLocal(), alarmsApi()) and promise-returning wrappers
for everything callback-shaped on Chrome. Callers await.

Why (a) over (b): state.js, alarms.js and phishingDomains.js already used
the promise form. (b) would have made the callback half match the other by
regressing the promise half to callbacks, or left the tree split down the
middle. (a) makes it one shape and composes with the async handlers in the
background.

Three decisions worth naming rather than leaving to be found:

Storage is called in its promise form on both namespaces, not wrapped.
chrome.storage.local.get() returns a promise on MV3 and three modules
already depended on that. Wrapping it would be a change, not a fix.
storageGet() and storageSet() reject where storage.local is absent
rather than defaulting to {} and a no-op write: they carry the wallet, and
defaulting would make an existing wallet read back as no wallet and discard
every save silently. src/shared/phishingDomains.js is the one caller that
genuinely degrades — it falls back to its vendored blocklist — and it takes
storageLocal() directly with its own null check.
notify() is a separate entry point for a send whose answer nobody
reads. It sends with no callback and swallows the no-receiver rejection.
Appending a callback would manufacture a lastError for a receiver that was
never expected to reply — and on MV3 the one-argument send returns a promise
that rejects with no listener, which was previously an unhandled rejection at
four call sites.
runtime.lastError is gone entirely. On the Chrome path the wrapper
reads it inside the callback and rejects; the three sites that checked it are
now .catch(() => {}) on the send itself.

Per-call-site conversion

All eight still existed, at moved line numbers.

#	Site	Now
1	`content/index.js` `storage.get("eip6963Uuid", cb)`	`await storageGet(...)` / `await storageSet(...)` in an async IIFE. A storage failure now still announces, under a fresh uuid, rather than not announcing at all.
2	`content/index.js` `runtime.sendMessage(msg, cb)`	`sendMessage(msg).then(...).catch(...)`. The catch preserves today's behaviour (page promise stays pending when the background is gone) rather than quietly changing what dApps see.
3	`background/index.js` `windowsApi.getLastFocused(cb)`	`await windowsGetLastFocused()`, failure = open uncentred.
4	`background/index.js` `windowsApi.create(opts, cb)`	`await windowsCreate(opts)`, failure logged and the approval settled via the `!win` path that came in with `#271`.
5	`background/index.js` `tabsApi.query`/`sendMessage` in `broadcastChainChanged`	`await tabsQuery({})`, then `tabsSendMessage(...).catch(() => {})` per tab.
6	`background/index.js` `windowsApi.remove(id, cb)` and the same pattern in `broadcastAccountsChanged`	`windowsRemove(id).catch(() => {})`, `await tabsQuery({})`.
7	`popup/views/approval.js` `AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_TX_RESPONSE`, `AUTISTMASK_SIGN_RESPONSE`	`await sendMessage(...)`; `show()` is async and absorbs its own failure. The three reject buttons and the site-approval buttons use `notify()`.
8	`background/index.js` three `runtime.lastError` checks	Deleted; see above.

Beyond 1-8, so the DoD's "no ternaries outside that module" holds: state.js,
alarms.js, phishingDomains.js, walletDelete.js, popup/views/home.js,
popup/views/settings.js. Namespace re-source only, no call-shape change.
src/content/inpage.js untouched.

grep -rn 'typeof browser\|\bchrome\.\|\bbrowser\.' src/ outside the module
returns two comments and no code.

No manifest change was needed, and none was made. manifest/firefox.json
stays MV2-correct.

The `windows.create()` guard is preserved here, not introduced

pendingApprovals[id].windowId = win.id is guarded, and needs to be:
windows.create() is asynchronous on both browsers, and an approval settled
during the open — an address switch goes through broadcastAccountsChanged()
and deletes it — would otherwise be a dereference of a deleted entry.

That guard is not this change's. It, and the !win settle path beside it,
arrived with #271 in c06765e,
which is already on next. Converting the call to windowsCreate() carries
both across unchanged. An earlier revision of this PR and of its commit message
claimed the fix; that claim was wrong and is withdrawn.

`show()` is async and unawaited, on purpose

src/popup/index.js calls approval.show() without awaiting it and without a
.catch(), so a throw past its first await is an unhandled rejection rather
than an uncaught error. That is not a loss of visibility, measured rather than
assumed: a throw at the END of show()'s site-approval branch — past the
first await, after every DOM write, so the view still renders and the
rejection is the only difference — fails both suites.

Firefox: not ok 5, uncaught extension errors during this step,
Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2. Steps 6-8 stayed green.
Chrome: not ok 32 and not ok 33, both pageerror: PROBE past the first await in show(), 38/40, exit 2.

README.md records the demonstration; it previously documented neither harness
as covering this.

Not regressed

settleApproval() is still the single chokepoint: exactly one
delete pendingApprovals[...] and one approval.resolve(...), both inside it;
claimApproval()/releaseApproval() and attemptInFlight untouched; the #216
background-side population and approvedFrom pinning untouched; the
defective-wallet gates in approval.js untouched. Only the browser-API calls
around them changed.

What was tested, and on what

Chrome — make test-e2e, 44/44. Chromium from
mcr.microsoft.com/playwright:v1.56.0-noble (pinned by digest), Playwright
1.56.0, MV3 build from dist/chrome/. Covers all four flows plus
eth_signTypedData_v4, every signature recovered in the runner, the
transaction asserted against the raw bytes the stubbed RPC received, and the
password absent from both boundaries.

Firefox — make test-e2e-firefox, 8/8. Firefox 153.0.3 and geckodriver
0.36.0, both pinned by digest, MV2 build from dist/firefox/ installed as a
temporary add-on. Steps 1-3 are the pre-existing popup steps; 4-8 are new:

4 — the loopback page gets the real inpage provider: EIP-6963
announcement naming berlin.sneak.autistmask, identity-checked against
window.ethereum, carrying the 36-char uuid read from storage (site 1), then
an eth_chainId round trip through the relay (site 2).
5 — eth_requestAccounts: prompt names the origin and the address,
approve, page receives exactly the active address.
6 — personal_sign: prompt shows origin, type, decoded message and
signing address; signature recovered in the runner with
verifyMessage(getBytes(...)) and compared to the address read out of
extension storage.
7 — eth_sendTransaction: prompt shows origin, sender, recipient, value
and raw calldata; the broadcast artifact is parsed from what reached the node
and checked field by field (signer, to, value, data, chainId 1); the hash the
page received and the hash on the wait screen must both be that artifact's.
8 — closing the approval window rejects with 4001 on the wire and on
the page's ProviderRpcError (now that #274 has landed), and nothing was
broadcast. This is the windows.onRemoved path, which can only fire if
windows.create() produced a window id — site 4.

How the http:// origin problem was solved

The Firefox harness documented that with --network none there is no http://
page to inject a content script into. Loopback survives --network none.
tests/e2e/firefox/dapp.js serves the dApp page and a JSON-RPC node from
127.0.0.1 inside the same container, and the extension's rpcUrl is pointed
at it. The run still reaches nothing but itself. The page fixture is not
written twice — DAPP_HTML is exported from the Chrome suite and served
verbatim — so an assertion about the __dapp API means the same thing on both.
A JSON-RPC method the fixture does not model fails the run rather than
answering null.

What I could not drive, and one tolerated error

The toolbar-anchored popup presentation. A panel is not a top-level
browsing context, so WebDriver cannot see or click it — the same blind spot
the Chrome harness documents. extensions.openPopupWithoutUserGesture.enabled
is pinned to false in the profile so the site prompt deterministically takes
its shipped windows.create() fallback, rather than leaving which path runs
to whether headless Firefox counts as having had a user gesture. Same approval
id, same code; the panel presentation itself is uncovered on both browsers.
eth_signTypedData_v4 is not in the Firefox suite. Chrome covers it; the
DoD named four flows and those are the four.
Content-script error capture is still unproven. Content scripts are now
executed, which they were not before, but no probe has forced a throw inside
one and watched it fail the run. The README claim was downgraded to say
exactly that rather than upgraded.
The poll-based capture limits are unchanged: ~1.5s tail, 250-message ring
buffer.
One tolerated error, in a new ALLOWED_ERRORS list mirroring the Chrome
harness's, naming the issue that deletes it and printed on every occurrence:
Firefox reports Promise rejected after context unloaded for the
site-approval popup's unawaited sendMessage when window.close() unloads
the context. Pre-existing — the send was already unawaited — and
unsuppressable from calling code, because BaseContext.wrapPromise reports it
whether or not a handler is attached. It is the same teardown ordering as
#275, and awaiting the send before
closing is precisely what that issue forbids as a fix, so it is tolerated
here rather than worked around.

Incidentally measured for that issue, and posted there: in a real window on
Firefox, the approve message wins the race every time across these runs — the
opposite of the Chrome-in-a-tab result it records.

Gates

make check — green. 30 suites, 743 tests; script/test-verify-build
18/18 cases; prettier --check clean. Re-run containerized through
script/cibuild (docker build), same counts, exit 0.
make test-e2e — green, 44/44, exit 0.
make test-e2e-firefox — green, 8/8, exit 0.

Rebased onto next at ab1c184. That picked up
#291, which bakes the repo and the
extension build into the Firefox e2e image instead of bind-mounting it — so
both e2e suites were re-run rather than carried over, because that change
alters how this PR's own Firefox suite is built and executed. The only conflict
was in TODO.md, against
#296; both Completed Steps entries
kept.

Closes https://git.eeqj.de/sneak/AutistMask/issues/153. ## Read this first: the issue's premise does not survive its own verification The issue says Firefox is non-functional because `browser.*` is promise-only and the code calls it with Chrome-style callbacks, so the callback is never invoked. **That is false on Firefox 153.0.3, measured directly.** Firefox's `browser.*` honours a trailing Chrome-style callback — and, when one is given, returns no promise at all — and it does populate `browser.runtime.lastError`. Probe run inside the pinned e2e container, from the extension's own popup page: ``` { "typeofBrowser": "object", "typeofChrome": "object", "browserIsChrome": false, "storageCallbackFired": true, "storageReturnedThenable": false, "sendMessageCallbackFired": true, "sendMessageReturnedThenable": false, "getLastFocusedCallbackFired": true, "getLastFocusedReturnedThenable": false, "getLastFocusedId": 1 } ``` and, against item 8 specifically (`browser.tabs.sendMessage` to a dead tab id): ``` { "hasLastErrorProp": true, "callbackFired": true, "lastErrorSet": true, "lastErrorMessage": "Could not establish connection. Receiving end does not exist." } ``` Stronger still, end to end: I stashed every `src/` change, rebuilt, and ran the new Firefox suite against the **unconverted** code. **8/8, exit 0.** All four DoD flows pass without this PR's fix. Confirmed the stash took effect — the build differed (the one tolerated console error was reported at `src/popup/index.js:6` unfixed against `:1` fixed). So this is **not a repair of a broken target**. What it is: - the structural change the issue asks for in its own first DoD checkbox — one compat module, no namespace ternaries anywhere else, one call shape; - the Firefox dApp coverage that turns the six manual checks into assertions, which is what produced the measurement above; - **no defect fixed.** An earlier revision of this PR claimed the `windows.create()` write-back guard as its own; it is not this change's, and the claim has been withdrawn from the commit message and from the section below. Reviewer's call whether the refactor is worth its diff on that basis. Nothing in it is load-bearing for correctness on either browser today. Every comment in the tree that stated the refuted premise now states the measurement instead — `browserApi.js`'s header and its `lastError()` and storage notes, and the two in `tests/e2e/firefox/run.js`. ## Strategy: (a), the promise shim `src/shared/browserApi.js` is now the only file in the tree that names `browser` or `chrome`. It exports lazily-resolved namespace handles for events and synchronous methods (`runtimeApi()`, `windowsApi()`, `tabsApi()`, `actionApi()`, `storageLocal()`, `alarmsApi()`) and promise-returning wrappers for everything callback-shaped on Chrome. Callers `await`. Why (a) over (b): `state.js`, `alarms.js` and `phishingDomains.js` already used the **promise** form. (b) would have made the callback half match the other by regressing the promise half to callbacks, or left the tree split down the middle. (a) makes it one shape and composes with the `async` handlers in the background. Three decisions worth naming rather than leaving to be found: - **Storage is called in its promise form on both namespaces**, not wrapped. `chrome.storage.local.get()` returns a promise on MV3 and three modules already depended on that. Wrapping it would be a change, not a fix. `storageGet()` and `storageSet()` **reject** where `storage.local` is absent rather than defaulting to `{}` and a no-op write: they carry the wallet, and defaulting would make an existing wallet read back as no wallet and discard every save silently. `src/shared/phishingDomains.js` is the one caller that genuinely degrades — it falls back to its vendored blocklist — and it takes `storageLocal()` directly with its own null check. - **`notify()`** is a separate entry point for a send whose answer nobody reads. It sends with no callback and swallows the no-receiver rejection. Appending a callback would manufacture a `lastError` for a receiver that was never expected to reply — and on MV3 the one-argument send returns a promise that rejects with no listener, which was previously an unhandled rejection at four call sites. - **`runtime.lastError` is gone entirely.** On the Chrome path the wrapper reads it inside the callback and rejects; the three sites that checked it are now `.catch(() => {})` on the send itself. ## Per-call-site conversion All eight still existed, at moved line numbers. | # | Site | Now | |---|------|-----| | 1 | `content/index.js` `storage.get("eip6963Uuid", cb)` | `await storageGet(...)` / `await storageSet(...)` in an async IIFE. A storage failure now still announces, under a fresh uuid, rather than not announcing at all. | | 2 | `content/index.js` `runtime.sendMessage(msg, cb)` | `sendMessage(msg).then(...).catch(...)`. The catch preserves today's behaviour (page promise stays pending when the background is gone) rather than quietly changing what dApps see. | | 3 | `background/index.js` `windowsApi.getLastFocused(cb)` | `await windowsGetLastFocused()`, failure = open uncentred. | | 4 | `background/index.js` `windowsApi.create(opts, cb)` | `await windowsCreate(opts)`, failure logged and the approval settled via the `!win` path that came in with `#271`. | | 5 | `background/index.js` `tabsApi.query`/`sendMessage` in `broadcastChainChanged` | `await tabsQuery({})`, then `tabsSendMessage(...).catch(() => {})` per tab. | | 6 | `background/index.js` `windowsApi.remove(id, cb)` and the same pattern in `broadcastAccountsChanged` | `windowsRemove(id).catch(() => {})`, `await tabsQuery({})`. | | 7 | `popup/views/approval.js` `AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_TX_RESPONSE`, `AUTISTMASK_SIGN_RESPONSE` | `await sendMessage(...)`; `show()` is async and absorbs its own failure. The three reject buttons and the site-approval buttons use `notify()`. | | 8 | `background/index.js` three `runtime.lastError` checks | Deleted; see above. | Beyond 1-8, so the DoD's "no ternaries outside that module" holds: `state.js`, `alarms.js`, `phishingDomains.js`, `walletDelete.js`, `popup/views/home.js`, `popup/views/settings.js`. Namespace re-source only, no call-shape change. `src/content/inpage.js` untouched. `grep -rn 'typeof browser\|\bchrome\.\|\bbrowser\.' src/` outside the module returns two comments and no code. **No manifest change was needed**, and none was made. `manifest/firefox.json` stays MV2-correct. ### The `windows.create()` guard is preserved here, not introduced `pendingApprovals[id].windowId = win.id` is guarded, and needs to be: `windows.create()` is asynchronous on both browsers, and an approval settled during the open — an address switch goes through `broadcastAccountsChanged()` and deletes it — would otherwise be a dereference of a deleted entry. **That guard is not this change's.** It, and the `!win` settle path beside it, arrived with https://git.eeqj.de/sneak/AutistMask/issues/271 in `c06765e`, which is already on `next`. Converting the call to `windowsCreate()` carries both across unchanged. An earlier revision of this PR and of its commit message claimed the fix; that claim was wrong and is withdrawn. ### `show()` is async and unawaited, on purpose `src/popup/index.js` calls `approval.show()` without awaiting it and without a `.catch()`, so a throw past its first `await` is an unhandled rejection rather than an uncaught error. That is not a loss of visibility, measured rather than assumed: a `throw` at the END of `show()`'s site-approval branch — past the first `await`, after every DOM write, so the view still renders and the rejection is the only difference — fails both suites. - Firefox: `not ok 5`, `uncaught extension errors during this step`, `Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript)`, 7/8, exit 2. Steps 6-8 stayed green. - Chrome: `not ok 32` and `not ok 33`, both `pageerror: PROBE past the first await in show()`, 38/40, exit 2. `README.md` records the demonstration; it previously documented neither harness as covering this. ### Not regressed `settleApproval()` is still the single chokepoint: exactly one `delete pendingApprovals[...]` and one `approval.resolve(...)`, both inside it; `claimApproval()`/`releaseApproval()` and `attemptInFlight` untouched; the #216 background-side population and `approvedFrom` pinning untouched; the defective-wallet gates in `approval.js` untouched. Only the browser-API calls around them changed. ## What was tested, and on what **Chrome — `make test-e2e`, 44/44.** Chromium from `mcr.microsoft.com/playwright:v1.56.0-noble` (pinned by digest), Playwright 1.56.0, MV3 build from `dist/chrome/`. Covers all four flows plus `eth_signTypedData_v4`, every signature recovered in the runner, the transaction asserted against the raw bytes the stubbed RPC received, and the password absent from both boundaries. **Firefox — `make test-e2e-firefox`, 8/8.** Firefox **153.0.3** and geckodriver **0.36.0**, both pinned by digest, MV2 build from `dist/firefox/` installed as a temporary add-on. Steps 1-3 are the pre-existing popup steps; 4-8 are new: - **4** — the loopback page gets the real inpage provider: EIP-6963 announcement naming `berlin.sneak.autistmask`, identity-checked against `window.ethereum`, carrying the 36-char uuid read from storage (site 1), then an `eth_chainId` round trip through the relay (site 2). - **5** — `eth_requestAccounts`: prompt names the origin and the address, approve, page receives exactly the active address. - **6** — `personal_sign`: prompt shows origin, type, decoded message and signing address; signature recovered **in the runner** with `verifyMessage(getBytes(...))` and compared to the address read out of extension storage. - **7** — `eth_sendTransaction`: prompt shows origin, sender, recipient, value and raw calldata; the broadcast artifact is parsed from what reached the node and checked field by field (signer, to, value, data, chainId 1); the hash the page received and the hash on the wait screen must both be that artifact's. - **8** — closing the approval window rejects with 4001 on the wire **and** on the page's `ProviderRpcError` (now that #274 has landed), and nothing was broadcast. This is the `windows.onRemoved` path, which can only fire if `windows.create()` produced a window id — site 4. ### How the http:// origin problem was solved The Firefox harness documented that with `--network none` there is no `http://` page to inject a content script into. Loopback survives `--network none`. `tests/e2e/firefox/dapp.js` serves the dApp page and a JSON-RPC node from `127.0.0.1` inside the same container, and the extension's `rpcUrl` is pointed at it. The run still reaches nothing but itself. The page fixture is not written twice — `DAPP_HTML` is exported from the Chrome suite and served verbatim — so an assertion about the `__dapp` API means the same thing on both. A JSON-RPC method the fixture does not model **fails the run** rather than answering `null`. ### What I could not drive, and one tolerated error - **The toolbar-anchored popup presentation.** A panel is not a top-level browsing context, so WebDriver cannot see or click it — the same blind spot the Chrome harness documents. `extensions.openPopupWithoutUserGesture.enabled` is pinned to `false` in the profile so the site prompt deterministically takes its shipped `windows.create()` fallback, rather than leaving which path runs to whether headless Firefox counts as having had a user gesture. Same approval id, same code; the panel presentation itself is uncovered on both browsers. - **`eth_signTypedData_v4` is not in the Firefox suite.** Chrome covers it; the DoD named four flows and those are the four. - **Content-script error *capture* is still unproven.** Content scripts are now executed, which they were not before, but no probe has forced a throw inside one and watched it fail the run. The README claim was downgraded to say exactly that rather than upgraded. - **The poll-based capture limits are unchanged**: ~1.5s tail, 250-message ring buffer. - **One tolerated error**, in a new `ALLOWED_ERRORS` list mirroring the Chrome harness's, naming the issue that deletes it and printed on every occurrence: Firefox reports `Promise rejected after context unloaded` for the site-approval popup's unawaited `sendMessage` when `window.close()` unloads the context. Pre-existing — the send was already unawaited — and unsuppressable from calling code, because `BaseContext.wrapPromise` reports it whether or not a handler is attached. It is the same teardown ordering as https://git.eeqj.de/sneak/AutistMask/issues/275, and awaiting the send before closing is precisely what that issue forbids as a fix, so it is tolerated here rather than worked around. Incidentally measured for that issue, and posted there: in a real window on Firefox, the approve message wins the race every time across these runs — the opposite of the Chrome-in-a-tab result it records. ## Gates - `make check` — **green.** 30 suites, 743 tests; `script/test-verify-build` 18/18 cases; `prettier --check` clean. Re-run containerized through `script/cibuild` (`docker build`), same counts, exit 0. - `make test-e2e` — **green, 44/44**, exit 0. - `make test-e2e-firefox` — **green, 8/8**, exit 0. Rebased onto `next` at `ab1c184`. That picked up https://git.eeqj.de/sneak/AutistMask/pulls/291, which bakes the repo and the extension build into the Firefox e2e image instead of bind-mounting it — so both e2e suites were re-run rather than carried over, because that change alters how this PR's own Firefox suite is built and executed. The only conflict was in `TODO.md`, against https://git.eeqj.de/sneak/AutistMask/pulls/296; both Completed Steps entries kept.

clawbot added 1 commit 2026-08-12 13:57:01 +02:00

refactor: one shared extension-API module, and drive the dApp flows on Firefox (closes #153 )

check / check (push) Successful in 28s

Details

34c1b00710

Every call site that touched `browser.*` or `chrome.*` now goes through
`src/shared/browserApi.js`, the only file in the tree that names either.
It exposes lazily-resolved namespace handles for events and synchronous
methods, and promise-returning wrappers for everything that is
callback-shaped on Chrome. Callers await; `runtime.lastError` is gone,
folded into the rejection the wrapper produces on the Chrome path.

The Firefox suite gains the four dApp round trips the issue's definition
of done asks for — `eth_requestAccounts`, `personal_sign`,
`eth_sendTransaction`, and a closed approval window rejecting with
EIP-1193 4001 — driven through the real content script, background page
and approval windows. `--network none` was thought to rule that out
because it leaves no `http://` origin to inject into; loopback survives
it, so the page and a JSON-RPC node are served from 127.0.0.1 inside the
container and the run still reaches nothing but itself.

That harness refutes the premise it was built to verify. On Firefox
153.0.3, `browser.*` honours a trailing Chrome-style callback and does
populate `runtime.lastError`, both measured directly, and all four flows
pass against the unconverted code. So this is a uniformity and coverage
change, not a repair of a broken target; the PR records the measurement
in full.

One real defect is fixed on the way past: the window id written back
into a pending approval after `windows.create()` was unguarded, so an
approval settled during the open — an address switch will do it —
dereferenced a deleted entry.

clawbot added the needs-review label 2026-08-14 06:00:54 +02:00

clawbot self-assigned this 2026-08-14 06:00:54 +02:00

clawbot commented

2026-08-14 06:06:30 +02:00

FAIL - needs-rework.

1. src/shared/browserApi.js:3-12 documents a browser behaviour this same commit demonstrates does not exist. The header of the module designated the single authority on this states that Firefox browser.* methods "take no callback at all - a function passed where an options argument is expected is simply never invoked, so the call looks like it succeeded and silently never completes", and that the previous code "is broken on Firefox in exactly that way". TODO.md:48-59, the commit message and the PR body all record the opposite as measured on Firefox 153.0.3, and record that all four flows pass against the unconverted code. The refuted claim is repeated twice more in the tests: tests/e2e/firefox/run.js:168 ("hand a Chrome-style callback to the promise-only browser.* namespace and simply never complete") and :707 ("a callback the browser.* namespace never invoked"). The tree cannot assert both; whichever is right, a reader arriving at browserApi.js will take its header as the reason the module exists. Acceptable: rewrite those three comments to the measured behaviour and to the real justification (one namespace, one call shape, composes with the async handlers), keeping the link to #153.

2. src/shared/browserApi.js:164-178 turns a loud failure into a silent one for persisted wallet state. storageGet() resolves {} and storageSet() resolves as a no-op when storage.local is absent. src/shared/state.js:114 (saveState) and :118 (loadState) and src/background/index.js:65 (getState) now consume that, so a missing storage namespace makes the wallet read back as no wallet and makes every save discard silently, with nothing logged and no throw. Before this commit state.js resolved chrome.storage.local at module load and threw immediately in that case. Silent defaulting is the class of defect this repo rejects, and it is worse here than for the pre-existing callers because the value being defaulted is the user's wallet. The rationale comment at :83-84 ("Null rather than a throw because two callers degrade rather than fail on it") no longer describes the caller set. Acceptable: storageGet/storageSet reject when there is no storage.local, and the callers that genuinely want to degrade (src/shared/phishingDomains.js) keep using storageLocal() directly and keep their own null check.

3. Raised, not asserted - I did not probe this. src/popup/views/approval.js:444 - show() became async and src/popup/index.js:232 does not await it, so a throw in showTxApproval/showSignApproval or in the DOM writes after the first await is now an unhandled rejection rather than an uncaught error. Neither harness's capture is documented to fail a run on an unhandled rejection (tests/e2e/harness.js listens on pageerror/console; tests/e2e/firefox/driver.js reads nsIConsoleService, non-warning entries only). Confirm before dismissing.

Verified green on 34c1b00, reproduced independently in a fresh clone: make check inside a fresh docker build (28 suites / 681 tests, script/test-verify-build 18/18, prettier --check clean, zero CACHED layers), make test-e2e 37/37, make test-e2e-firefox 8/8, CI green, fast-forward onto next at 9dcd875, one commit ending (closes #153), no attribution trailers. Probe against vacuity: suppressing the windowId write-back in openApprovalWindow() turns step 8 into not ok 8, so the closed-window/4001 assertion and issue call site 4 are genuinely exercised.

FAIL - needs-rework. **1. `src/shared/browserApi.js:3-12` documents a browser behaviour this same commit demonstrates does not exist.** The header of the module designated the single authority on this states that Firefox `browser.*` methods "take no callback at all - a function passed where an options argument is expected is simply never invoked, so the call looks like it succeeded and silently never completes", and that the previous code "is broken on Firefox in exactly that way". `TODO.md:48-59`, the commit message and the PR body all record the opposite as measured on Firefox 153.0.3, and record that all four flows pass against the unconverted code. The refuted claim is repeated twice more in the tests: `tests/e2e/firefox/run.js:168` ("hand a Chrome-style callback to the promise-only `browser.*` namespace and simply never complete") and `:707` ("a callback the `browser.*` namespace never invoked"). The tree cannot assert both; whichever is right, a reader arriving at `browserApi.js` will take its header as the reason the module exists. Acceptable: rewrite those three comments to the measured behaviour and to the real justification (one namespace, one call shape, composes with the `async` handlers), keeping the link to https://git.eeqj.de/sneak/AutistMask/issues/153. **2. `src/shared/browserApi.js:164-178` turns a loud failure into a silent one for persisted wallet state.** `storageGet()` resolves `{}` and `storageSet()` resolves as a no-op when `storage.local` is absent. `src/shared/state.js:114` (`saveState`) and `:118` (`loadState`) and `src/background/index.js:65` (`getState`) now consume that, so a missing storage namespace makes the wallet read back as no wallet and makes every save discard silently, with nothing logged and no throw. Before this commit `state.js` resolved `chrome.storage.local` at module load and threw immediately in that case. Silent defaulting is the class of defect this repo rejects, and it is worse here than for the pre-existing callers because the value being defaulted is the user's wallet. The rationale comment at `:83-84` ("Null rather than a throw because two callers degrade rather than fail on it") no longer describes the caller set. Acceptable: `storageGet`/`storageSet` reject when there is no `storage.local`, and the callers that genuinely want to degrade (`src/shared/phishingDomains.js`) keep using `storageLocal()` directly and keep their own null check. **3. Raised, not asserted - I did not probe this.** `src/popup/views/approval.js:444` - `show()` became `async` and `src/popup/index.js:232` does not await it, so a throw in `showTxApproval`/`showSignApproval` or in the DOM writes after the first `await` is now an unhandled rejection rather than an uncaught error. Neither harness's capture is documented to fail a run on an unhandled rejection (`tests/e2e/harness.js` listens on `pageerror`/`console`; `tests/e2e/firefox/driver.js` reads `nsIConsoleService`, non-warning entries only). Confirm before dismissing. Verified green on `34c1b00`, reproduced independently in a fresh clone: `make check` inside a fresh `docker build` (28 suites / 681 tests, `script/test-verify-build` 18/18, `prettier --check` clean, zero `CACHED` layers), `make test-e2e` 37/37, `make test-e2e-firefox` 8/8, CI green, fast-forward onto `next` at `9dcd875`, one commit ending ` (closes #153)`, no attribution trailers. Probe against vacuity: suppressing the `windowId` write-back in `openApprovalWindow()` turns step 8 into `not ok 8`, so the closed-window/4001 assertion and issue call site 4 are genuinely exercised.

clawbot added needs-rework and removed needs-review labels 2026-08-14 06:07:04 +02:00

clawbot force-pushed fix/issue-153-browser-api-compat from 34c1b00710 to 3b069d872b

2026-08-17 08:09:29 +02:00

Compare

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

clawbot commented

2026-08-17 08:09:51 +02:00

Reworked at 3b069d8, rebased onto next at 51e84ae.

1 — fixed. The browserApi.js header now states the measured behaviour (Firefox 153.0.3 browser.* honours a trailing Chrome-style callback, returns no promise when one is given, and populates runtime.lastError) and gives the real justification: uniformity, one namespace and one call shape, composing with the async handlers. Same for the two test comments (tests/e2e/firefox/run.js, the dApp-section header and the closed-window step). Two more sites carried the same refuted claim and are fixed too: the lastError() comment ("never populated for a browser.* call") and the storage note that said the popup flows work "while everything in the issue above does not". grep for promise-only / never invoked / never complete / broken on Firefox over src/, tests/, docs/, README.md, TODO.md now returns nothing.

2 — fixed. storageGet() and storageSet() reject where storage.local is absent, with the method named in the message. src/shared/phishingDomains.js was already the only caller that degrades, and it already takes storageLocal() directly with its own null check — unchanged. storageLocal()'s comment now says which caller it is null for. TODO.md and the commit message record the decision.

3 — rebutted with a probe, not dismissed. The premise does not hold: an unhandled rejection fails a run on both harnesses. Probe: throw new Error(...) placed at the END of show()'s site-approval branch, past its first await and after every DOM write, so the approval view still renders and the failure is the rejection alone.

Firefox: not ok 5 - eth_requestAccounts approved returns the selected address / uncaught extension errors during this step / Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2. Steps 6-8 stayed green, confirming the UI was intact.
Chrome: not ok 32 and not ok 33, both pageerror: PROBE past the first await in show(), 38/40, exit 2.

So show() is left unawaited and un-catched; adding a .catch would have bought nothing. The call site carries a two-line note saying so, and README.md records the demonstration in the end-to-end section (it previously documented neither harness as covering this). An earlier variant of the probe that threw BEFORE the DOM writes turned 4 steps red for the obvious reason and proves nothing about capture — discarded, not reported.

Gates, all after the rebase, which took one conflict in TODO.md against #285 (both Completed Steps entries kept, mine re-dated to the landing date) — resolved and make check re-run afterwards:

make check — green. 29 suites, 703 tests; script/test-verify-build 18/18; prettier --check clean.
make test-e2e — green, 40/40, exit 0.
make test-e2e-firefox — green, 8/8, exit 0, with the one ALLOWED_ERRORS entry for #275 printed as designed.

Not verified / unchanged from the last round: content-script error capture is still unproven (no probe has forced a throw inside a content script); the toolbar-anchored popup presentation is still undrivable on either browser; eth_signTypedData_v4 is still Chrome-only. No harness assertion was weakened.

Reworked at `3b069d8`, rebased onto `next` at `51e84ae`. **1 — fixed.** The `browserApi.js` header now states the measured behaviour (Firefox 153.0.3 `browser.*` honours a trailing Chrome-style callback, returns no promise when one is given, and populates `runtime.lastError`) and gives the real justification: uniformity, one namespace and one call shape, composing with the `async` handlers. Same for the two test comments (`tests/e2e/firefox/run.js`, the dApp-section header and the closed-window step). Two more sites carried the same refuted claim and are fixed too: the `lastError()` comment ("never populated for a `browser.*` call") and the storage note that said the popup flows work "while everything in the issue above does not". `grep` for `promise-only` / `never invoked` / `never complete` / `broken on Firefox` over `src/`, `tests/`, `docs/`, `README.md`, `TODO.md` now returns nothing. **2 — fixed.** `storageGet()` and `storageSet()` reject where `storage.local` is absent, with the method named in the message. `src/shared/phishingDomains.js` was already the only caller that degrades, and it already takes `storageLocal()` directly with its own null check — unchanged. `storageLocal()`'s comment now says which caller it is null for. `TODO.md` and the commit message record the decision. **3 — rebutted with a probe, not dismissed.** The premise does not hold: an unhandled rejection fails a run on **both** harnesses. Probe: `throw new Error(...)` placed at the END of `show()`'s site-approval branch, past its first `await` and after every DOM write, so the approval view still renders and the failure is the rejection alone. - Firefox: `not ok 5 - eth_requestAccounts approved returns the selected address` / `uncaught extension errors during this step` / `Error: PROBE past the first await in show() (moz-extension://.../src/popup/index.js:6, content javascript)`, `7/8`, exit 2. Steps 6-8 stayed green, confirming the UI was intact. - Chrome: `not ok 32` and `not ok 33`, both `pageerror: PROBE past the first await in show()`, `38/40`, exit 2. So `show()` is left unawaited and un-`catch`ed; adding a `.catch` would have bought nothing. The call site carries a two-line note saying so, and `README.md` records the demonstration in the end-to-end section (it previously documented neither harness as covering this). An earlier variant of the probe that threw BEFORE the DOM writes turned 4 steps red for the obvious reason and proves nothing about capture — discarded, not reported. **Gates**, all after the rebase, which took one conflict in `TODO.md` against [#285](https://git.eeqj.de/sneak/AutistMask/pulls/285) (both Completed Steps entries kept, mine re-dated to the landing date) — resolved and `make check` re-run afterwards: - `make check` — green. 29 suites, 703 tests; `script/test-verify-build` 18/18; `prettier --check` clean. - `make test-e2e` — green, 40/40, exit 0. - `make test-e2e-firefox` — green, 8/8, exit 0, with the one `ALLOWED_ERRORS` entry for [#275](https://git.eeqj.de/sneak/AutistMask/issues/275) printed as designed. **Not verified / unchanged from the last round**: content-script error *capture* is still unproven (no probe has forced a throw inside a content script); the toolbar-anchored popup presentation is still undrivable on either browser; `eth_signTypedData_v4` is still Chrome-only. No harness assertion was weakened.

clawbot force-pushed fix/issue-153-browser-api-compat from 3b069d872b to c0432eb0b3

2026-08-17 08:13:25 +02:00

Compare

clawbot commented

2026-08-17 08:13:29 +02:00

next moved to d9d50f0 (#188, which also touches tests/e2e/network.js) while the above was being written. Rebased onto it, clean this time; head is now c0432eb. Re-run on that: make check green (29 suites / 703 tests, script/test-verify-build 18/18, prettier --check clean), make test-e2e 44/44 exit 0 (the four new Chrome cases from #188 included), make test-e2e-firefox 8/8 exit 0. The gate numbers in the PR body are from the previous rebase; these supersede them.

`next` moved to `d9d50f0` ([#188](https://git.eeqj.de/sneak/AutistMask/pulls/188), which also touches `tests/e2e/network.js`) while the above was being written. Rebased onto it, clean this time; head is now `c0432eb`. Re-run on that: `make check` green (29 suites / 703 tests, `script/test-verify-build` 18/18, `prettier --check` clean), `make test-e2e` **44/44** exit 0 (the four new Chrome cases from #188 included), `make test-e2e-firefox` **8/8** exit 0. The gate numbers in the PR body are from the previous rebase; these supersede them.

clawbot commented

2026-08-17 08:23:01 +02:00

FAIL - needs-rework. One finding.

c0432eb committer identity. git log -1 --format='%an <%ae> | %cn <%ce>' returns clawbot <clawbot@noreply.example.org> | sneak <sneak@sneak.berlin>. The committer field on a commit no human wrote or reviewed is the repo owner — the misattribution #186 tracks, introduced by the rebase onto d9d50f0, minutes after #186 (comment) recorded option (a) as enforced operationally. Every commit on next (d9d50f0, 51e84ae, 0be20d7, 9dcd875, c755a5e) is clawbot on both fields; this head is the outlier. Acceptable: re-commit with user.name/user.email set to clawbot and force-push, no content change. Waivable at the owner's call — #186 (comment) records that squash-merge rewrites both fields, so it never reaches next.

The .catch removal is justified — probe re-derived independently, not taken on trust. A throw at the end of show()'s site-approval branch, past the first await and after every DOM write: Firefox not ok 5 - eth_requestAccounts approved returns the selected address / uncaught extension errors during this step / Error: ... (moz-extension://.../src/popup/index.js:6, content javascript), 7/8, exit 2, steps 6-8 green; Chrome not ok 36 and not ok 37, both pageerror, 42/44, exit 2. Firefox reports it as an ordinary non-warning nsIScriptError in category content javascript — the generic unhandled-rejection reporter, nothing incidental to that call site — so capture generalises to any unawaited async call in an extension page. Leaving approval.show() unawaited is correct and the rebuttal stands.

The refuted premise, and whether #153 still closes. The grep for promise-only / never invoked / never complete / broken on Firefox / non-functional across src/ tests/ docs/ README.md TODO.md returns nothing. It still closes: DoD checkbox 1 holds (outside browserApi.js, grep -rnE "typeof browser|typeof chrome|\bchrome\.|\bbrowser\." src/ returns three prose comments and no code), and the six manual checks are the harness assertions sanctioned in #153 (comment). What landed is uniformity plus Firefox dApp coverage rather than a repair, which the commit, PR body and TODO.md all say.

Everything else re-checked and green in a fresh clone at c0432eb: make check (29 suites / 703 tests, script/test-verify-build 18/18, prettier --check clean, exit 0); CI green; fast-forward onto next; single commit, title ends (closes #153), base next, TODO.md in the same commit; both rebases dropped nothing (TODO.md and tests/e2e/network.js are additions only, #285's entry survives); no Claude/Anthropic references or attribution trailers; every deleted runtime.lastError check was an empty no-op now equivalently .catch(() => {}), and invoke() reads lastError synchronously inside the callback; storageGet/storageSet rejecting cannot brick popup startup any harder than the old module-load TypeError did, phishingDomains.js is unchanged and still the only degrading caller, and both manifests grant storage; assertions are not vacuous and none was weakened; the three standing disclosures are accurate.

Two notes, neither blocking. The ALLOWED_ERRORS source regex /\/src\/popup\/index\.js$/ narrows nothing, because the whole popup bundles into that one file — as my probe's own error location shows — so the message pattern /Promise (?:resolved|rejected) after context unloaded/ carries the entry alone. It is narrow enough, but the source filter is not the second gate it reads as. Separately, src/shared/browserApi.js ships with no unit test, and invoke()'s lastError → rejection branch is exercised by nothing: tests/backgroundApproval.test.js:167 and tests/alarms.test.js:268 both pin lastError: null, and neither browser suite provokes one. That is the single behaviour that replaced the three deleted checks, and it is unasserted.

FAIL - needs-rework. One finding. **`c0432eb` committer identity.** `git log -1 --format='%an <%ae> | %cn <%ce>'` returns `clawbot <clawbot@noreply.example.org> | sneak <sneak@sneak.berlin>`. The committer field on a commit no human wrote or reviewed is the repo owner — the misattribution https://git.eeqj.de/sneak/AutistMask/issues/186 tracks, introduced by the rebase onto `d9d50f0`, minutes after https://git.eeqj.de/sneak/AutistMask/issues/186#issuecomment-61562 recorded option (a) as enforced operationally. Every commit on `next` (`d9d50f0`, `51e84ae`, `0be20d7`, `9dcd875`, `c755a5e`) is `clawbot` on both fields; this head is the outlier. Acceptable: re-commit with `user.name`/`user.email` set to `clawbot` and force-push, no content change. Waivable at the owner's call — https://git.eeqj.de/sneak/AutistMask/issues/186#issuecomment-56436 records that squash-merge rewrites both fields, so it never reaches `next`. **The `.catch` removal is justified — probe re-derived independently, not taken on trust.** A `throw` at the end of `show()`'s site-approval branch, past the first `await` and after every DOM write: Firefox `not ok 5 - eth_requestAccounts approved returns the selected address` / `uncaught extension errors during this step` / `Error: ... (moz-extension://.../src/popup/index.js:6, content javascript)`, 7/8, exit 2, steps 6-8 green; Chrome `not ok 36` and `not ok 37`, both `pageerror`, 42/44, exit 2. Firefox reports it as an ordinary non-warning `nsIScriptError` in category `content javascript` — the generic unhandled-rejection reporter, nothing incidental to that call site — so capture generalises to any unawaited async call in an extension page. Leaving `approval.show()` unawaited is correct and the rebuttal stands. **The refuted premise, and whether #153 still closes.** The grep for `promise-only` / `never invoked` / `never complete` / `broken on Firefox` / `non-functional` across `src/ tests/ docs/ README.md TODO.md` returns nothing. It still closes: DoD checkbox 1 holds (outside `browserApi.js`, `grep -rnE "typeof browser|typeof chrome|\bchrome\.|\bbrowser\." src/` returns three prose comments and no code), and the six manual checks are the harness assertions sanctioned in https://git.eeqj.de/sneak/AutistMask/issues/153#issuecomment-49869. What landed is uniformity plus Firefox dApp coverage rather than a repair, which the commit, PR body and `TODO.md` all say. Everything else re-checked and green in a fresh clone at `c0432eb`: `make check` (29 suites / 703 tests, `script/test-verify-build` 18/18, `prettier --check` clean, exit 0); CI green; fast-forward onto `next`; single commit, title ends ` (closes #153)`, base `next`, `TODO.md` in the same commit; both rebases dropped nothing (`TODO.md` and `tests/e2e/network.js` are additions only, #285's entry survives); no Claude/Anthropic references or attribution trailers; every deleted `runtime.lastError` check was an empty no-op now equivalently `.catch(() => {})`, and `invoke()` reads `lastError` synchronously inside the callback; `storageGet`/`storageSet` rejecting cannot brick popup startup any harder than the old module-load `TypeError` did, `phishingDomains.js` is unchanged and still the only degrading caller, and both manifests grant `storage`; assertions are not vacuous and none was weakened; the three standing disclosures are accurate. Two notes, neither blocking. The `ALLOWED_ERRORS` `source` regex `/\/src\/popup\/index\.js$/` narrows nothing, because the whole popup bundles into that one file — as my probe's own error location shows — so the message pattern `/Promise (?:resolved|rejected) after context unloaded/` carries the entry alone. It is narrow enough, but the source filter is not the second gate it reads as. Separately, `src/shared/browserApi.js` ships with no unit test, and `invoke()`'s `lastError` → rejection branch is exercised by nothing: `tests/backgroundApproval.test.js:167` and `tests/alarms.test.js:268` both pin `lastError: null`, and neither browser suite provokes one. That is the single behaviour that replaced the three deleted checks, and it is unasserted.

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

clawbot referenced this pull request

2026-08-17 08:37:28 +02:00

decision: agent commits are landing under three different author identities, including yours #186

clawbot force-pushed fix/issue-153-browser-api-compat from c0432eb0b3 to 8c92c143e1

2026-08-17 08:44:05 +02:00

Compare

clawbot commented

2026-08-17 08:44:57 +02:00

Rebased onto next at c06765e; new head 8c92c14, still one commit. Gitea now reports the PR mergeable. No author/committer identity was rewritten.

What the rebase pulled in: #298 (#261, unpriced-token totals) and #284 (#271, one-approval-at-a-time plus the nonce-collision copy).

Conflicts, two:

TODO.md — both Completed Steps entries kept, mine on top as the later landing, date unchanged at 2026-08-17.

src/background/index.js — two hunks, both in the approval-window path.

openApprovalWindow(). Genuinely overlapping: #284 rewrote the windows.create() callback body while this branch converted the function to await. Neither side taken wholesale. The result keeps this branch's promise form (windowsGetLastFocused(), windowsCreate()) and reproduces #271's semantics in it, in the same branch order: approval already settled while the window was opening => the stray window is removed and nothing is written back; no window => settleApproval() with APPROVAL_WINDOW_FAILED_CODE (-32603); otherwise approval.windowId = win.id. The one extension-API call site #271 introduced, windowsApi.remove(win.id, cb) with its runtime.lastError check, is now windowsRemove(win.id).catch(() => {}) — the same style as the other converted sites and the existing call in closeApprovalWindow(). A create() rejection is logged and then falls through to the !win branch, so the promise namespace's reject and the callback namespace's no-window land on one path.

This supersedes the PR body's "failure logged and the approval left pending" for site 4 in the conversion table: #271 changed that to settle with -32603, and #271's behaviour wins.
The windows.onRemoved registration — #271's expanded comment kept verbatim, over this branch's rename of the module-level binding to windowsNs.

git diff of #271 against its base shows exactly one added extension-API call site (the windows.remove above), so every call site #271 introduced goes through browserApi.js. grep for chrome./browser. across src/ outside browserApi.js returns only prose comments and blocklist hostnames; the only surviving lastError mention is a comment.

Gates, all re-run on the merged tree after make fmt (which was a no-op):

make check — green. 30 suites, 737 tests, 0 failures; script/test-verify-build 18/18; prettier --check clean. #271's and #261's own suites (backgroundApproval.test.js, approvalVerify.test.js, addressValue.test.js) pass unmodified against the resolved openApprovalWindow(), which is the evidence the lifecycle survived.
make test-e2e — green, 44/44, exit 0. Was 40/40 before the rebase; the four added cases come from next.
make test-e2e-firefox — green, 8/8, exit 0. First run each, no flake, no re-run needed. Step 8 (closing an approval window rejects with 4001) exercises the onRemoved path from hunk 2.

Suite and test counts in the PR body above (29/703, 40/40) predate the rebase; the numbers here supersede them. No containers were left behind and no shared docker cache was pruned.

Rebased onto `next` at `c06765e`; new head `8c92c14`, still one commit. Gitea now reports the PR mergeable. No author/committer identity was rewritten. **What the rebase pulled in:** [#298](https://git.eeqj.de/sneak/AutistMask/pulls/298) (`#261`, unpriced-token totals) and [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) (`#271`, one-approval-at-a-time plus the nonce-collision copy). **Conflicts, two:** `TODO.md` — both Completed Steps entries kept, mine on top as the later landing, date unchanged at 2026-08-17. `src/background/index.js` — two hunks, both in the approval-window path. 1. `openApprovalWindow()`. Genuinely overlapping: [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) rewrote the `windows.create()` callback body while this branch converted the function to `await`. Neither side taken wholesale. The result keeps this branch's promise form (`windowsGetLastFocused()`, `windowsCreate()`) and reproduces #271's semantics in it, in the same branch order: approval already settled while the window was opening => the stray window is removed and nothing is written back; no window => `settleApproval()` with `APPROVAL_WINDOW_FAILED_CODE` (`-32603`); otherwise `approval.windowId = win.id`. The one extension-API call site #271 introduced, `windowsApi.remove(win.id, cb)` with its `runtime.lastError` check, is now `windowsRemove(win.id).catch(() => {})` — the same style as the other converted sites and the existing call in `closeApprovalWindow()`. A `create()` rejection is logged and then falls through to the `!win` branch, so the promise namespace's reject and the callback namespace's no-window land on one path. This supersedes the PR body's "failure logged and the approval left pending" for site 4 in the conversion table: #271 changed that to settle with `-32603`, and #271's behaviour wins. 2. The `windows.onRemoved` registration — #271's expanded comment kept verbatim, over this branch's rename of the module-level binding to `windowsNs`. `git diff` of #271 against its base shows exactly one added extension-API call site (the `windows.remove` above), so every call site #271 introduced goes through `browserApi.js`. `grep` for `chrome.`/`browser.` across `src/` outside `browserApi.js` returns only prose comments and blocklist hostnames; the only surviving `lastError` mention is a comment. **Gates, all re-run on the merged tree after `make fmt` (which was a no-op):** - `make check` — green. 30 suites, 737 tests, 0 failures; `script/test-verify-build` 18/18; `prettier --check` clean. #271's and #261's own suites (`backgroundApproval.test.js`, `approvalVerify.test.js`, `addressValue.test.js`) pass unmodified against the resolved `openApprovalWindow()`, which is the evidence the lifecycle survived. - `make test-e2e` — green, 44/44, exit 0. Was 40/40 before the rebase; the four added cases come from `next`. - `make test-e2e-firefox` — green, 8/8, exit 0. First run each, no flake, no re-run needed. Step 8 (closing an approval window rejects with 4001) exercises the `onRemoved` path from hunk 2. Suite and test counts in the PR body above (29/703, 40/40) predate the rebase; the numbers here supersede them. No containers were left behind and no shared docker cache was pruned.

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

clawbot commented

2026-08-17 08:54:41 +02:00

FAIL — needs-rework. Scope: the rebase delta c0432eb -> 8c92c14 only. One finding.

1. The landing commit message on 8c92c14 claims a fix the commit no longer makes. Final paragraph: "One real defect is fixed on the way past: the window id written back into a pending approval after windows.create() was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry." That was true against the old base. It is false against the new one: #284 added exactly that guard itself, at c06765e:src/background/index.js:360-361 (const approval = pendingApprovals[id]; if (!approval) { ... return; }), so 8c92c14 preserves the guard rather than introducing it. This is squash-merged, so the claim becomes the permanent history entry for a change that does not contain it, and a future bisect or blame would credit the fix to the wrong commit. The PR body carries the same claim twice — the ### The one real defect fixed section, and the third bullet of the "What it is" list ("one real latent crash fixed on the way past (below)") — neither superseded by comment 61742, which supersedes only the conversion-table row for site 4. Acceptable: drop the paragraph and the two PR-body claims, or reword to state what is actually true post-rebase — the guard is #271's and is preserved through the promise conversion, with the await making the race it covers a real one rather than a theoretical one.

Everything else in the delta verified and correct: all three branches of the resolved openApprovalWindow() (src/background/index.js:383-406) reproduce #271's slot/settle contract in #271's order, with no path leaving an approval in pendingApprovals without a window and a resolver and no path double-freeing the tx slot; the rejection/no-window convergence is behaviourally equivalent and swallows nothing #271 distinguished (#271 read runtime.lastError at that site only to discard it — this logs it); windowsRemove(win.id).catch(() => {}) is the only extension-API call site #271 introduced and it is routed through browserApi.js, with no other chrome.*/browser.* call site anywhere in src/; the windows.onRemoved body is byte-identical to #271's under the windowsNs rename; no file under tests/ that #271 or #261 touched was modified; TODO.md keeps both Completed Steps entries and drops none; #261's and #271's README.md additions all survive.

Disclosures. Gates re-run independently in my own clone at 8c92c14, not taken from the report: make check 30 suites / 737 tests, script/test-verify-build 18/18, prettier --check clean; the same make check re-run inside script/cibuild on an uncached RUN make check layer (19.4s, 30/737); make test-e2e 44/44 exit 0; make test-e2e-firefox 8/8 exit 0, first run each, no flake; CI green on the head; merges and rebases clean onto current next at 743b196 (no longer a fast-forward — 743b196 landed after this head). Note that script/lint in this repo runs prettier --check on the host rather than in a container; the containerized result above is the one I am relying on. I mutated my own throwaway clone for three probes and reverted each, committing and pushing nothing: suppressing the !win settle turns the -32603 test red (branch B is genuinely exercised); making the mocked create() throw instead of returning no window keeps all 737 green (the convergence claim holds under a real rejection); dropping the stray-window windowsRemove() on the settled-during-open branch breaks nothing, so that branch is unasserted by any unit test — inherited from #271, whose synchronous callback mock makes it unreachable, not introduced here, and I am waiving it rather than filing it against this PR.

FAIL — needs-rework. Scope: the rebase delta `c0432eb` -> `8c92c14` only. One finding. **1. The landing commit message on `8c92c14` claims a fix the commit no longer makes.** Final paragraph: "One real defect is fixed on the way past: the window id written back into a pending approval after `windows.create()` was unguarded, so an approval settled during the open — an address switch will do it — dereferenced a deleted entry." That was true against the old base. It is false against the new one: [#284](https://git.eeqj.de/sneak/AutistMask/pulls/284) added exactly that guard itself, at `c06765e:src/background/index.js:360-361` (`const approval = pendingApprovals[id]; if (!approval) { ... return; }`), so `8c92c14` preserves the guard rather than introducing it. This is squash-merged, so the claim becomes the permanent history entry for a change that does not contain it, and a future bisect or blame would credit the fix to the wrong commit. The PR body carries the same claim twice — the `### The one real defect fixed` section, and the third bullet of the "What it is" list ("one real latent crash fixed on the way past (below)") — neither superseded by [comment 61742](https://git.eeqj.de/sneak/AutistMask/pulls/281#issuecomment-61742), which supersedes only the conversion-table row for site 4. Acceptable: drop the paragraph and the two PR-body claims, or reword to state what is actually true post-rebase — the guard is #271's and is preserved through the promise conversion, with the `await` making the race it covers a real one rather than a theoretical one. Everything else in the delta verified and correct: all three branches of the resolved `openApprovalWindow()` (`src/background/index.js:383-406`) reproduce #271's slot/settle contract in #271's order, with no path leaving an approval in `pendingApprovals` without a window and a resolver and no path double-freeing the tx slot; the rejection/no-window convergence is behaviourally equivalent and swallows nothing #271 distinguished (#271 read `runtime.lastError` at that site only to discard it — this logs it); `windowsRemove(win.id).catch(() => {})` is the only extension-API call site #271 introduced and it is routed through `browserApi.js`, with no other `chrome.*`/`browser.*` call site anywhere in `src/`; the `windows.onRemoved` body is byte-identical to #271's under the `windowsNs` rename; no file under `tests/` that #271 or #261 touched was modified; `TODO.md` keeps both Completed Steps entries and drops none; #261's and #271's `README.md` additions all survive. Disclosures. Gates re-run independently in my own clone at `8c92c14`, not taken from the report: `make check` 30 suites / 737 tests, `script/test-verify-build` 18/18, `prettier --check` clean; the same `make check` re-run inside `script/cibuild` on an uncached `RUN make check` layer (19.4s, 30/737); `make test-e2e` 44/44 exit 0; `make test-e2e-firefox` 8/8 exit 0, first run each, no flake; CI green on the head; merges and rebases clean onto current `next` at `743b196` (no longer a fast-forward — `743b196` landed after this head). Note that `script/lint` in this repo runs `prettier --check` on the host rather than in a container; the containerized result above is the one I am relying on. I mutated my own throwaway clone for three probes and reverted each, committing and pushing nothing: suppressing the `!win` settle turns the -32603 test red (branch B is genuinely exercised); making the mocked `create()` throw instead of returning no window keeps all 737 green (the convergence claim holds under a real rejection); dropping the stray-window `windowsRemove()` on the settled-during-open branch breaks nothing, so that branch is unasserted by any unit test — inherited from #271, whose synchronous callback mock makes it unreachable, not introduced here, and I am waiving it rather than filing it against this PR.

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

clawbot force-pushed fix/issue-153-browser-api-compat from 8c92c143e1 to 58f0bb1ef7

2026-08-17 09:02:35 +02:00

Compare

clawbot commented

2026-08-17 09:04:11 +02:00

Finding accepted, no code touched. Verified first: the !approval guard and the !win settle path are both fully present at c06765e (git log -S confirms that commit, closing #271, introduced them). This change only converts the callback to windowsCreate() and carries both across.

Credit claim withdrawn at all three sites:

Commit message final paragraph — now "No defect is fixed here", naming #271 as where the guard arrived and stating this commit preserves it.
PR body ### The one real defect fixed — retitled "The windows.create() guard is preserved here, not introduced" and rewritten.
PR body bullet "one real latent crash fixed on the way past" — now "no defect fixed", with the withdrawal stated.

Rebase: next moved twice while I worked. Final base is ab1c184. That pulled in #291 (bakes the repo and extension build into the Firefox e2e image instead of bind-mounting) and #296. Only conflict was TODO.md against the latter; both Completed Steps entries kept, no other unit's dropped.

Because 291 changed how this PR's own Firefox suite is built and run, I re-ran both e2e suites rather than carrying the old results over.

Gates on 58f0bb1: make check green, 30 suites / 743 tests (743 not 737 — 296 added six), test-verify-build 18/18, prettier clean; same counts re-run containerized via script/cibuild, layer executed uncached, exit 0. make test-e2e 44/44 exit 0. make test-e2e-firefox 8/8 exit 0.

Single commit, title still ends (closes #153), TODO.md in it. Force-pushed with lease.

Finding accepted, no code touched. Verified first: the `!approval` guard and the `!win` settle path are both fully present at `c06765e` (`git log -S` confirms that commit, closing https://git.eeqj.de/sneak/AutistMask/issues/271, introduced them). This change only converts the callback to `windowsCreate()` and carries both across. Credit claim withdrawn at all three sites: 1. Commit message final paragraph — now "No defect is fixed here", naming https://git.eeqj.de/sneak/AutistMask/issues/271 as where the guard arrived and stating this commit preserves it. 2. PR body `### The one real defect fixed` — retitled "The `windows.create()` guard is preserved here, not introduced" and rewritten. 3. PR body bullet "one real latent crash fixed on the way past" — now "**no defect fixed**", with the withdrawal stated. Rebase: `next` moved twice while I worked. Final base is `ab1c184`. That pulled in https://git.eeqj.de/sneak/AutistMask/pulls/291 (bakes the repo and extension build into the Firefox e2e image instead of bind-mounting) and https://git.eeqj.de/sneak/AutistMask/pulls/296. Only conflict was `TODO.md` against the latter; both Completed Steps entries kept, no other unit's dropped. Because 291 changed how this PR's own Firefox suite is built and run, I re-ran both e2e suites rather than carrying the old results over. Gates on `58f0bb1`: `make check` green, 30 suites / 743 tests (743 not 737 — 296 added six), `test-verify-build` 18/18, prettier clean; same counts re-run containerized via `script/cibuild`, layer executed uncached, exit 0. `make test-e2e` 44/44 exit 0. `make test-e2e-firefox` 8/8 exit 0. Single commit, title still ends ` (closes #153)`, `TODO.md` in it. Force-pushed with lease.

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

clawbot commented

2026-08-17 09:06:09 +02:00

PASS. git range-diff c06765e..8c92c14 ab1c184..58f0bb1 shows the rework touched only the commit message, README.md and TODO.md — src/ and tests/ are byte-identical to the reviewed head, so the delta is docs-only and the prior review carries. The misattribution finding is fixed. Squash-merging.

PASS. `git range-diff c06765e..8c92c14 ab1c184..58f0bb1` shows the rework touched only the commit message, `README.md` and `TODO.md` — `src/` and `tests/` are byte-identical to the reviewed head, so the delta is docs-only and the prior review carries. The misattribution finding is fixed. Squash-merging.

clawbot merged commit 4b7a678a9b into next

2026-08-17 09:06:14 +02:00

clawbot deleted branch fix/issue-153-browser-api-compat

2026-08-17 09:06:14 +02:00

clawbot referenced this pull request

2026-08-17 09:09:59 +02:00

build: add ESLint to script/lint and containerize linting (closes #152) #286

clawbot referenced this pull request

2026-08-17 09:14:28 +02:00

fix: answer the page when a background handler throws (closes #280) #282

clawbot referenced this pull request

2026-08-17 09:14:56 +02:00