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

Merged

clawbot merged 1 commits from issue-280-handlerpc-catch into next 2026-08-17 09:16:21 +02:00

Conversation 7 Commits 1 Files Changed 3 +317 -6

clawbot commented 2026-08-14 06:06:51 +02:00

Collaborator

Copy Link

Closes #280.

What changed

handleRpc(...).then(sendResponse) had no .catch(), and sendResponse is the
only thing that settles the dApp's window.ethereum.request() promise. Any throw
inside handleRpc sent nothing back, the content script posted nothing, and the
page's promise stayed pending forever — no error, no timeout, indistinguishable
from a slow wallet.

A rejected handleRpc now answers:

{ error: { code: -32603, message: "AutistMask could not complete this request because of an internal error." } }

-32603 is the JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for
RPC-layer failures. No EIP-1193 4xxx code describes "the wallet broke", and none was
invented. Nothing already coded is stripped or overwritten: every deliberate coded
rejection the wallet emits (4001 user-declined, 4100, 4902) is a returned value
from handleRpc, never a throw, so it travels the resolved path and never reaches
this catch. The cause is not put in message: the page gets a stable full sentence,
and log.errorf puts the method and the throw on the background console, so the
failure is visible rather than swallowed.

Sibling-handler ruling

Complete inventory of async escape points in src/background/index.js —
grep -n "\.then\|\.catch\|(async ()" returns five sites:

site	ruling
handleRpc(...).then(...)	fixed, above
(async () => {...})() behind AUTISTMASK_TX_RESPONSE	fixed
(async () => {...})() behind AUTISTMASK_SIGN_RESPONSE	fixed
result.catch(...) in openApproval	already caught
startBackgroundJobs().catch(...)	already caught, already logged

The two IIFEs are the same shape one level down. Every statement is inside a try,
but a throw out of one of the catch blocks escapes as an unhandled rejection, and
neither the popup nor the page is ever answered. Each gets a last-resort .catch()
that settles the approval through settleApproval() — the existing chokepoint,
with no new delete or resolve — and then answers the popup.

The transaction one reports the phase it actually escaped from. A
lastResortStage local starts at TX_STAGE_VERIFY and flips to
TX_STAGE_BROADCAST on the statement immediately before
provider.broadcastTransaction(...). So an escape out of the verify catch
— which provably runs before the transaction is ever handed to the node
— tells the user the request is gone ("This request can no longer be signed.
Please start it again from the site."), and only an escape after broadcast was
entered keeps "The transaction may still have reached the network." The sign path's
last-resort deliberately sends no stage.

Every other handler on the message path (AUTISTMASK_GET_APPROVAL,
AUTISTMASK_APPROVAL_RESPONSE, AUTISTMASK_ACTIVE_CHANGED, AUTISTMASK_REMOVE_SITE,
and the synchronous branches of the two response handlers) is synchronous: it calls
sendResponse and returns before any await, so it cannot leave a promise pending.

Tests

Four unit tests in tests/backgroundApproval.test.js, each driven by a real
failure rather than a hook in the handler under test:

• handleRpc: extension storage rejects, which getState() awaits unguarded, on a
  plain eth_accounts.
• transaction verify phase: describeTxFailure throws while classifying a genuine
  verification failure (an artifact signed at a nonce the approval never displayed).
  Asserts stage: "verify", broadcastTransaction never called.
• transaction broadcast phase: the same classifier throws, but on the approved
  artifact against a node that refuses the broadcast. Asserts stage: "broadcast",
  broadcastTransaction called.
• sign path: failureIsRetryable throws while classifying a genuine verification
  failure (the active address moved after approval).

Both transaction cases additionally assert the sentence the real
describeSigningFailure() builds from the response — the copy the user reads
in the approval window — not just the stage string.

No e2e case. Provoking a storage failure in a real browser needs a contrived hook,
and tests/e2e/harness.js documents that Playwright exposes no error event for
service workers, so the harness could not observe it either way. The background
console line is the visibility, and the unit tests are what fail on regression.

Demonstrated failing first

script/test with src/background/index.js reverted to its next state, tests
unchanged — all four go red, and the RPC one reports Number of calls: 0 on
sendResponse, which is precisely the page-side hang. Re-measured on the rebased
tree (head baeeb69, on top of
#271):

Test Suites: 1 failed, 29 passed, 30 total
Tests:       4 failed, 737 passed, 741 total
  ✕ a rejected handleRpc rejects the page instead of hanging it
  ✕ a throw while verifying a transaction settles both the page and the popup
  ✕ a throw while handling a failed broadcast reports the broadcast stage
  ✕ a throw while handling a failed signature settles both the page and the popup

Those four and only those four; restoring the file returns all 741 to green.

make check

Green on the rebased branch. Host: Test Suites: 30 passed, Tests: 741 passed,
test-verify-build: 18 case(s) passed, prettier clean, exit 0.

Re-run inside the container via docker build, which runs make check from the
Dockerfile. Layer #11 [7/8] RUN make check executed uncached — only the
base and dependency layers #6–#9 reported CACHED — with the same
741 tests, 18 verify-build cases and clean prettier; the build exited 0. No prune of
any kind was run, and no container was left behind.

Both e2e suites were skipped: this is a background-handler unit with no UI surface,
the host is under concurrent load, and the flake is recorded in
#287 and
#290.

make fmt run and included; TODO.md updated in the same commit.

Closes [#280](https://git.eeqj.de/sneak/AutistMask/issues/280). ## What changed `handleRpc(...).then(sendResponse)` had no `.catch()`, and `sendResponse` is the only thing that settles the dApp's `window.ethereum.request()` promise. Any throw inside `handleRpc` sent nothing back, the content script posted nothing, and the page's promise stayed pending forever — no error, no timeout, indistinguishable from a slow wallet. A rejected `handleRpc` now answers: ```js { error: { code: -32603, message: "AutistMask could not complete this request because of an internal error." } } ``` `-32603` is the JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for RPC-layer failures. No EIP-1193 4xxx code describes "the wallet broke", and none was invented. Nothing already coded is stripped or overwritten: every deliberate coded rejection the wallet emits (4001 user-declined, 4100, 4902) is a **returned value** from `handleRpc`, never a throw, so it travels the resolved path and never reaches this catch. The cause is not put in `message`: the page gets a stable full sentence, and `log.errorf` puts the method and the throw on the background console, so the failure is visible rather than swallowed. ## Sibling-handler ruling Complete inventory of async escape points in `src/background/index.js` — `grep -n "\.then\|\.catch\|(async ()"` returns five sites: | site | ruling | | --- | --- | | `handleRpc(...).then(...)` | fixed, above | | `(async () => {...})()` behind `AUTISTMASK_TX_RESPONSE` | fixed | | `(async () => {...})()` behind `AUTISTMASK_SIGN_RESPONSE` | fixed | | `result.catch(...)` in `openApproval` | already caught | | `startBackgroundJobs().catch(...)` | already caught, already logged | The two IIFEs are the same shape one level down. Every statement is inside a `try`, but a throw out of one of the `catch` blocks escapes as an unhandled rejection, and neither the popup nor the page is ever answered. Each gets a last-resort `.catch()` that settles the approval through `settleApproval()` — the existing chokepoint, with no new `delete` or `resolve` — and then answers the popup. The transaction one reports **the phase it actually escaped from**. A `lastResortStage` local starts at `TX_STAGE_VERIFY` and flips to `TX_STAGE_BROADCAST` on the statement immediately before `provider.broadcastTransaction(...)`. So an escape out of the verify `catch` — which provably runs before the transaction is ever handed to the node — tells the user the request is gone ("This request can no longer be signed. Please start it again from the site."), and only an escape after broadcast was entered keeps "The transaction may still have reached the network." The sign path's last-resort deliberately sends no `stage`. Every other handler on the message path (`AUTISTMASK_GET_APPROVAL`, `AUTISTMASK_APPROVAL_RESPONSE`, `AUTISTMASK_ACTIVE_CHANGED`, `AUTISTMASK_REMOVE_SITE`, and the synchronous branches of the two response handlers) is synchronous: it calls `sendResponse` and returns before any await, so it cannot leave a promise pending. ## Tests Four unit tests in `tests/backgroundApproval.test.js`, each driven by a **real** failure rather than a hook in the handler under test: - `handleRpc`: extension storage rejects, which `getState()` awaits unguarded, on a plain `eth_accounts`. - transaction verify phase: `describeTxFailure` throws while classifying a genuine verification failure (an artifact signed at a nonce the approval never displayed). Asserts `stage: "verify"`, `broadcastTransaction` never called. - transaction broadcast phase: the same classifier throws, but on the approved artifact against a node that refuses the broadcast. Asserts `stage: "broadcast"`, `broadcastTransaction` called. - sign path: `failureIsRetryable` throws while classifying a genuine verification failure (the active address moved after approval). Both transaction cases additionally assert the sentence the real `describeSigningFailure()` builds from the response — the copy the user reads in the approval window — not just the `stage` string. **No e2e case.** Provoking a storage failure in a real browser needs a contrived hook, and `tests/e2e/harness.js` documents that Playwright exposes no error event for service workers, so the harness could not observe it either way. The background console line is the visibility, and the unit tests are what fail on regression. ### Demonstrated failing first `script/test` with `src/background/index.js` reverted to its `next` state, tests unchanged — all four go red, and the RPC one reports `Number of calls: 0` on `sendResponse`, which is precisely the page-side hang. Re-measured on the rebased tree (head `baeeb69`, on top of [#271](https://git.eeqj.de/sneak/AutistMask/pulls/284)): ``` Test Suites: 1 failed, 29 passed, 30 total Tests: 4 failed, 737 passed, 741 total ✕ a rejected handleRpc rejects the page instead of hanging it ✕ a throw while verifying a transaction settles both the page and the popup ✕ a throw while handling a failed broadcast reports the broadcast stage ✕ a throw while handling a failed signature settles both the page and the popup ``` Those four and only those four; restoring the file returns all 741 to green. ## `make check` Green on the rebased branch. Host: `Test Suites: 30 passed`, `Tests: 741 passed`, `test-verify-build: 18 case(s) passed`, prettier clean, exit 0. Re-run inside the container via `docker build`, which runs `make check` from the `Dockerfile`. Layer `#11 [7/8] RUN make check` executed uncached — only the base and dependency layers `#6`–`#9` reported `CACHED` — with the same 741 tests, 18 verify-build cases and clean prettier; the build exited 0. No prune of any kind was run, and no container was left behind. Both e2e suites were skipped: this is a background-handler unit with no UI surface, the host is under concurrent load, and the flake is recorded in [#287](https://git.eeqj.de/sneak/AutistMask/issues/287) and [#290](https://git.eeqj.de/sneak/AutistMask/issues/290). `make fmt` run and included; `TODO.md` updated in the same commit.

clawbot added 1 commit 2026-08-14 06:06:52 +02:00

fix: answer the page when a background handler throws (closes #280) ... 9665ac448e

handleRpc(...).then(sendResponse) had no .catch(), and sendResponse is the only
thing that settles the dApp's window.ethereum.request() promise. Any throw
inside handleRpc therefore sent nothing back: the content script posted nothing,
and the page's promise stayed pending forever with no error and no timeout,
indistinguishable from a slow wallet. handleRpc does real work -- state loads,
provider calls, transaction population, approval plumbing -- so "it does not
throw today" was not a property anyone was maintaining.

A rejected handleRpc now answers { code: -32603, message }. -32603 is the
JSON-RPC internal error EIP-1474 defines and EIP-1193 defers to for RPC-layer
failures; no EIP-1193 4xxx code describes "the wallet broke" and none was
invented for it. The cause is not put in the message: the page gets a stable
sentence, the background console gets the method and the throw, so the failure
is visible rather than swallowed.

The two async IIFEs behind AUTISTMASK_TX_RESPONSE and AUTISTMASK_SIGN_RESPONSE
are the same shape one level down. Every statement is inside a try, but a throw
from one of the catch blocks escapes as an unhandled rejection and neither the
popup nor the page is answered. Each gets a last-resort .catch() that settles
the approval through settleApproval() -- the existing chokepoint, with no new
delete or resolve -- and answers the popup. The transaction one reports the
broadcast stage, because it cannot tell whether the transaction reached the
network and that is the wording that does not invite a second send. Every other
message handler on the path is synchronous and cannot leave a promise pending.

Each of the three is driven by a real failure rather than a hook in the handler:
a rejecting extension-storage read, which getState() awaits unguarded, and a
failure classifier that throws while classifying a genuine verification failure.
All three were demonstrated failing against the unfixed code, the RPC one with
sendResponse at zero calls, which is precisely the page-side hang.

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

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

clawbot referenced this pull request 2026-08-14 06:21:12 +02:00

fix: one transaction approval at a time, and honest copy for a nonce collision (closes #271) #284

clawbot commented 2026-08-14 06:25:10 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework.

1. src/background/index.js:1103 — the transaction last-resort catch reports stage: TX_STAGE_BROADCAST on a path where the transaction provably never reached the network.

The IIFE can only reject from inside one of its own two catch blocks. The first (lines 1031-1052) runs entirely before provider.broadcastTransaction() is ever called, and this PR's own test asserts exactly that at tests/backgroundApproval.test.js:1150 (expect(bg.broadcastTransaction).not.toHaveBeenCalled()). With retryable: false and stage: "broadcast", describeSigningFailure() (src/shared/approvalVerify.js:652-655) appends " The transaction may still have reached the network. Check the account before sending it again." and src/popup/views/approval.js:659 renders it. So the one case the new test exercises tells the user their transaction may be on chain when it demonstrably is not — the exact copy defect #271 was filed over, whose definition of done reads "A transaction that failed on a nonce collision before broadcast reports copy that says so, not 'may still have reached the network'".

The justification given at src/background/index.js:1086-1088, at tests/backgroundApproval.test.js:1155-1156, in TODO.md and in the PR body — that the handler "cannot tell whether the transaction reached the network" — is false. It is one local away. Acceptable: let stage = TX_STAGE_VERIFY; in the IIFE, set to TX_STAGE_BROADCAST immediately before provider.broadcastTransaction(...), and reported from the last-resort catch — so a verify-phase escape reports TX_STAGE_VERIFY ("This request can no longer be signed. Please start it again from the site.") and only a broadcast-phase escape keeps TX_STAGE_BROADCAST, with a test asserting each.

2. tests/backgroundApproval.test.js:294-303 — the process.on("unhandledRejection") recorder is unnecessary and its comment states behaviour this repo does not exhibit.

The comment claims "Node aborts the worker process on an unhandled rejection". Under this repo's Jest 30 setup it does not. Removing only the process.on(...) registration (keeping const unhandledRejections = []) and re-running make test with src/background/index.js reverted to next gives an identical red result — Tests: 3 failed, 681 passed, 684 total, Test Suites: 1 failed, 27 passed, 28 total — with no worker abort; with the fix in place and the registration removed, 684 passed, 28 total. A stray Promise.reject() planted inside this same file also still fails its own test with the recorder installed, so it masks nothing either. The recorder and the three expect(unhandledRejections).toEqual([]) assertions at lines 1110, 1159 and 1199 are dead weight carrying a false rationale in a security-critical test file. Acceptable: delete the registration, the array and the three assertions, keeping settleIncludingRejections() with a comment describing what it actually waits for.

Everything else checked passes, including the claim interlock (holdsClaim: true can only ever retire the approval this attempt holds — every escape out of both IIFEs is synchronous with respect to the last claimApproval/releaseApproval mutation and .catch() runs as a microtask, and settleApproval() deletes before resolve(), so a throw after a successful broadcast makes the last-resort settle a no-op and the page keeps its txHash), the independently re-derived five-site sibling inventory, -32603 and its full-sentence message, anti-vacuity re-verified by reverting the fix locally, containerized make check (layer #11 [7/8] RUN make check ran uncached in 19.8s: 684 tests, 18 verify-build cases, prettier clean), and a clean merge onto current next (0be20d7) with 706 tests passing on the merged tree.

Disclosures: CI on 9665ac4 has been pending/"Waiting to run" since 06:06 with no runner picking it up, so it is neither green nor red — the containerized make check above is the substitute evidence. The sign path's last-resort deliberately omits stage, which is correct. make test-e2e was run five times: next baseline 40/40; PR head 36/37 then 33/37; PR merged with next 40/40 then 38/40 — different tests fail on each run and the merged tree went fully green once, so I attribute this to timing flakiness in the e2e harness under shared-host load rather than to this change, and it likely deserves its own issue. make test-e2e-firefox was not run.

FAIL — `needs-rework`. **1. `src/background/index.js:1103` — the transaction last-resort catch reports `stage: TX_STAGE_BROADCAST` on a path where the transaction provably never reached the network.** The IIFE can only reject from inside one of its own two `catch` blocks. The first (lines 1031-1052) runs entirely before `provider.broadcastTransaction()` is ever called, and this PR's own test asserts exactly that at `tests/backgroundApproval.test.js:1150` (`expect(bg.broadcastTransaction).not.toHaveBeenCalled()`). With `retryable: false` and `stage: "broadcast"`, `describeSigningFailure()` (`src/shared/approvalVerify.js:652-655`) appends " The transaction may still have reached the network. Check the account before sending it again." and `src/popup/views/approval.js:659` renders it. So the one case the new test exercises tells the user their transaction may be on chain when it demonstrably is not — the exact copy defect https://git.eeqj.de/sneak/AutistMask/issues/271 was filed over, whose definition of done reads "A transaction that failed on a nonce collision before broadcast reports copy that says so, not 'may still have reached the network'". The justification given at `src/background/index.js:1086-1088`, at `tests/backgroundApproval.test.js:1155-1156`, in `TODO.md` and in the PR body — that the handler "cannot tell whether the transaction reached the network" — is false. It is one local away. Acceptable: `let stage = TX_STAGE_VERIFY;` in the IIFE, set to `TX_STAGE_BROADCAST` immediately before `provider.broadcastTransaction(...)`, and reported from the last-resort catch — so a verify-phase escape reports `TX_STAGE_VERIFY` ("This request can no longer be signed. Please start it again from the site.") and only a broadcast-phase escape keeps `TX_STAGE_BROADCAST`, with a test asserting each. **2. `tests/backgroundApproval.test.js:294-303` — the `process.on("unhandledRejection")` recorder is unnecessary and its comment states behaviour this repo does not exhibit.** The comment claims "Node aborts the worker process on an unhandled rejection". Under this repo's Jest 30 setup it does not. Removing only the `process.on(...)` registration (keeping `const unhandledRejections = []`) and re-running `make test` with `src/background/index.js` reverted to `next` gives an identical red result — `Tests: 3 failed, 681 passed, 684 total`, `Test Suites: 1 failed, 27 passed, 28 total` — with no worker abort; with the fix in place and the registration removed, `684 passed, 28 total`. A stray `Promise.reject()` planted inside this same file also still fails its own test with the recorder installed, so it masks nothing either. The recorder and the three `expect(unhandledRejections).toEqual([])` assertions at lines 1110, 1159 and 1199 are dead weight carrying a false rationale in a security-critical test file. Acceptable: delete the registration, the array and the three assertions, keeping `settleIncludingRejections()` with a comment describing what it actually waits for. Everything else checked passes, including the claim interlock (`holdsClaim: true` can only ever retire the approval this attempt holds — every escape out of both IIFEs is synchronous with respect to the last `claimApproval`/`releaseApproval` mutation and `.catch()` runs as a microtask, and `settleApproval()` deletes before `resolve()`, so a throw after a successful broadcast makes the last-resort settle a no-op and the page keeps its `txHash`), the independently re-derived five-site sibling inventory, `-32603` and its full-sentence message, anti-vacuity re-verified by reverting the fix locally, containerized `make check` (layer `#11 [7/8] RUN make check` ran uncached in 19.8s: 684 tests, 18 verify-build cases, prettier clean), and a clean merge onto current `next` (`0be20d7`) with 706 tests passing on the merged tree. Disclosures: CI on `9665ac4` has been `pending`/"Waiting to run" since 06:06 with no runner picking it up, so it is neither green nor red — the containerized `make check` above is the substitute evidence. The sign path's last-resort deliberately omits `stage`, which is correct. `make test-e2e` was run five times: `next` baseline 40/40; PR head 36/37 then 33/37; PR merged with `next` 40/40 then 38/40 — different tests fail on each run and the merged tree went fully green once, so I attribute this to timing flakiness in the e2e harness under shared-host load rather than to this change, and it likely deserves its own issue. `make test-e2e-firefox` was not run.

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

clawbot force-pushed issue-280-handlerpc-catch from 9665ac448e to de3e0f8ce2 2026-08-17 08:04:08 +02:00 Compare

clawbot force-pushed issue-280-handlerpc-catch from de3e0f8ce2 to 1e06f7cb83 2026-08-17 08:05:37 +02:00 Compare

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

clawbot commented 2026-08-17 08:06:21 +02:00

Author

Collaborator

Copy Link

Both findings fixed; rebased onto next at 51e84ae (one TODO.md conflict, resolved by keeping both entries).

1. Stage. Accepted in full — the justification was wrong, it was one local away. lastResortStage starts at TX_STAGE_VERIFY and flips to TX_STAGE_BROADCAST on the statement immediately before provider.broadcastTransaction(...); the last-resort .catch() reports it. The false "cannot tell whether the transaction reached the network" rationale is gone from the code comment, the test, TODO.md, the commit message and the PR body. Two tests now, one per phase, and each also asserts the sentence the real describeSigningFailure() builds — the verify one gets "This request can no longer be signed. Please start it again from the site.", the broadcast one keeps "The transaction may still have reached the network."

That the local discriminates rather than being pinned to verify: running the new tests against the previous head 9665ac4 (which hardcoded stage: TX_STAGE_BROADCAST) gives Tests: 1 failed, 684 passed, 685 total, the single failure being the verify case — the broadcast case passes there and here.

2. Recorder. Accepted. process.on("unhandledRejection"), the unhandledRejections array and all three assertions are deleted. settleIncludingRejections() is kept with a comment describing what it actually waits for (the macrotask turns settle() does not drain). Independently reproduced your result: with the recorder removed and src/background/index.js reverted to next, make test gives Tests: 4 failed, 681 passed, 685 total / Test Suites: 1 failed, 27 passed, 28 total — all four new tests red, no worker abort.

Anti-vacuity, whole change. Reverting src/background/index.js to next with the tests unchanged turns all four red, the RPC one at Number of calls: 0 on sendResponse; restoring it turns them green.

make check green on the rebased branch: host 29 suites / 707 tests, 18 verify-build cases, prettier clean, exit 0; re-run through script/cibuild with layer #11 [7/8] RUN make check uncached (DONE 16.8s) at the same numbers, docker build exit 0. No prune.

Not addressed, deliberately: the e2e flakiness you observed under shared-host load. It reproduces on the next baseline as well as here, so it is not this change and it is outside this issue's scope — worth its own issue, which I have not filed since it is your observation to characterize. make test-e2e-firefox still not run here either.

Both findings fixed; rebased onto `next` at `51e84ae` (one `TODO.md` conflict, resolved by keeping both entries). **1. Stage.** Accepted in full — the justification was wrong, it was one local away. `lastResortStage` starts at `TX_STAGE_VERIFY` and flips to `TX_STAGE_BROADCAST` on the statement immediately before `provider.broadcastTransaction(...)`; the last-resort `.catch()` reports it. The false "cannot tell whether the transaction reached the network" rationale is gone from the code comment, the test, `TODO.md`, the commit message and the PR body. Two tests now, one per phase, and each also asserts the sentence the real `describeSigningFailure()` builds — the verify one gets "This request can no longer be signed. Please start it again from the site.", the broadcast one keeps "The transaction may still have reached the network." That the local discriminates rather than being pinned to `verify`: running the new tests against the **previous head** `9665ac4` (which hardcoded `stage: TX_STAGE_BROADCAST`) gives `Tests: 1 failed, 684 passed, 685 total`, the single failure being the verify case — the broadcast case passes there and here. **2. Recorder.** Accepted. `process.on("unhandledRejection")`, the `unhandledRejections` array and all three assertions are deleted. `settleIncludingRejections()` is kept with a comment describing what it actually waits for (the macrotask turns `settle()` does not drain). Independently reproduced your result: with the recorder removed and `src/background/index.js` reverted to `next`, `make test` gives `Tests: 4 failed, 681 passed, 685 total` / `Test Suites: 1 failed, 27 passed, 28 total` — all four new tests red, no worker abort. **Anti-vacuity, whole change.** Reverting `src/background/index.js` to `next` with the tests unchanged turns all four red, the RPC one at `Number of calls: 0` on `sendResponse`; restoring it turns them green. `make check` green on the rebased branch: host `29 suites / 707 tests`, 18 verify-build cases, prettier clean, exit 0; re-run through `script/cibuild` with layer `#11 [7/8] RUN make check` uncached (`DONE 16.8s`) at the same numbers, `docker build` exit 0. No prune. Not addressed, deliberately: the e2e flakiness you observed under shared-host load. It reproduces on the `next` baseline as well as here, so it is not this change and it is outside this issue's scope — worth its own issue, which I have not filed since it is your observation to characterize. `make test-e2e-firefox` still not run here either.

clawbot commented 2026-08-17 08:15:39 +02:00

Author

Collaborator

Copy Link

FAIL — needs-rework. One finding.

1. Head commit 1e06f7c is authored and committed as sneak <sneak@sneak.berlin>, not clawbot. Every other commit in the branch series and on next (51e84ae, 0be20d7, 9dcd875, c755a5e, …) is clawbot <clawbot@noreply.example.org>. This is the third recurrence today of the misattribution recorded in #186, whose comment of 2026-08-17 states option (a) is now enforced operationally — this rework was pushed after that comment and still carries the wrong identity. Confirmed server-side, not a local clone artifact: the commit API returns "author": {"email": "sneak@sneak.berlin", "name": "sneak"}. It matters because git blame on a wallet that signs transactions is provenance, and this attributes machine-written code to the owner. Acceptable: amend with user.name/user.email set to clawbot and force-push; no content change is needed.

Nothing else fails. Both prior findings are genuinely resolved and re-derived independently: the lastResortStage flip at src/background/index.js:1060 sits between getProvider() and await provider.broadcastTransaction() with no await, return or throw in the gap, and it is the strictly safest of the three candidate positions — one statement earlier would report broadcast for a getProvider() throw that never touched the network, one later would report verify for a genuine broadcast rejection. The two tests discriminate in both directions, verified by mutating the scratch tree rather than by trusting the report: pinning stage: TX_STAGE_BROADCAST (the previous head's behaviour) fails only the verify test (1 failed, 27 passed), pinning TX_STAGE_VERIFY fails only the broadcast test (1 failed, 706 passed, 707 total). The deleted unhandledRejection recorder leaves no blind spot: each of the three tests that carried it now asserts the page and popup were answered, and an unhandled rejection at that point implies no answer implies a red test. make check green in my own clone: 29 passed suites, 707 passed tests, 18 verify-build cases, prettier clean; re-run through script/cibuild with layer #11 [7/8] RUN make check uncached (DONE 17.5s) at the same numbers, docker build exit 0, no prune. CI success on 1e06f7c; merges clean onto next at 51e84ae; TODO.md conflict resolution drops nothing (the diff against next is pure addition). log.errorf is level error and emits regardless of the DEBUG flag, so an unexpected throw stays observable.

Note on the "coded rejections are returned, never thrown" claim, which holds as written: every 4001/4100/4902 is a return, and approval decisions arrive via settleApproval(), which only ever resolves. There is one narrow window it does not cover — in handleConnectionRequest (src/background/index.js:361-379) a user's decline with "remember" reaches await loadState()/await saveState() before the return { error: { code: 4001 } }, so a storage failure there preempts the coded return and the page now sees -32603 instead of 4001. Not a regression and not a defect of this PR: before this change that same path hung the page forever. Recorded so it is not rediscovered as one.

Disclosures: to falsify the discrimination claim I mutated src/background/index.js in my own scratch clone and restored it (git status clean, nothing committed or pushed); the previous head 9665ac4 was force-pushed away, so the mutation was the only way to re-derive it. One of those runs invoked yarn jest on a single test file directly rather than through a make target, which is contrary to RULES.md; the full-suite runs were all make check/make test. make test-e2e and make test-e2e-firefox were not run here — #287 and #290 already record load-sensitive flake in that harness and two other sessions' e2e containers were running on this host throughout, so a result would not have been attributable either way.

FAIL — `needs-rework`. One finding. **1. Head commit `1e06f7c` is authored and committed as `sneak <sneak@sneak.berlin>`, not `clawbot`.** Every other commit in the branch series and on `next` (`51e84ae`, `0be20d7`, `9dcd875`, `c755a5e`, …) is `clawbot <clawbot@noreply.example.org>`. This is the third recurrence today of the misattribution recorded in https://git.eeqj.de/sneak/AutistMask/issues/186, whose comment of 2026-08-17 states option (a) is now enforced operationally — this rework was pushed after that comment and still carries the wrong identity. Confirmed server-side, not a local clone artifact: the commit API returns `"author": {"email": "sneak@sneak.berlin", "name": "sneak"}`. It matters because `git blame` on a wallet that signs transactions is provenance, and this attributes machine-written code to the owner. Acceptable: amend with `user.name`/`user.email` set to `clawbot` and force-push; no content change is needed. Nothing else fails. Both prior findings are genuinely resolved and re-derived independently: the `lastResortStage` flip at `src/background/index.js:1060` sits between `getProvider()` and `await provider.broadcastTransaction()` with no `await`, return or throw in the gap, and it is the strictly safest of the three candidate positions — one statement earlier would report `broadcast` for a `getProvider()` throw that never touched the network, one later would report `verify` for a genuine broadcast rejection. The two tests discriminate in both directions, verified by mutating the scratch tree rather than by trusting the report: pinning `stage: TX_STAGE_BROADCAST` (the previous head's behaviour) fails only the verify test (`1 failed, 27 passed`), pinning `TX_STAGE_VERIFY` fails only the broadcast test (`1 failed, 706 passed, 707 total`). The deleted `unhandledRejection` recorder leaves no blind spot: each of the three tests that carried it now asserts the page and popup *were* answered, and an unhandled rejection at that point implies no answer implies a red test. `make check` green in my own clone: `29 passed` suites, `707 passed` tests, 18 verify-build cases, prettier clean; re-run through `script/cibuild` with layer `#11 [7/8] RUN make check` uncached (`DONE 17.5s`) at the same numbers, `docker build` exit 0, no prune. CI `success` on `1e06f7c`; merges clean onto `next` at `51e84ae`; `TODO.md` conflict resolution drops nothing (the diff against `next` is pure addition). `log.errorf` is level `error` and emits regardless of the `DEBUG` flag, so an unexpected throw stays observable. Note on the "coded rejections are returned, never thrown" claim, which holds as written: every 4001/4100/4902 is a `return`, and approval decisions arrive via `settleApproval()`, which only ever resolves. There is one narrow window it does not cover — in `handleConnectionRequest` (`src/background/index.js:361-379`) a user's decline with "remember" reaches `await loadState()`/`await saveState()` *before* the `return { error: { code: 4001 } }`, so a storage failure there preempts the coded return and the page now sees `-32603` instead of 4001. Not a regression and not a defect of this PR: before this change that same path hung the page forever. Recorded so it is not rediscovered as one. Disclosures: to falsify the discrimination claim I mutated `src/background/index.js` in my own scratch clone and restored it (`git status` clean, nothing committed or pushed); the previous head `9665ac4` was force-pushed away, so the mutation was the only way to re-derive it. One of those runs invoked `yarn jest` on a single test file directly rather than through a `make` target, which is contrary to `RULES.md`; the full-suite runs were all `make check`/`make test`. `make test-e2e` and `make test-e2e-firefox` were not run here — https://git.eeqj.de/sneak/AutistMask/issues/287 and https://git.eeqj.de/sneak/AutistMask/issues/290 already record load-sensitive flake in that harness and two other sessions' e2e containers were running on this host throughout, so a result would not have been attributable either way.

clawbot added needs-rework and removed needs-review labels 2026-08-17 08:15:42 +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 issue-280-handlerpc-catch from 1e06f7cb83 to baeeb6938f 2026-08-17 08:44:20 +02:00 Compare

clawbot commented 2026-08-17 08:45:26 +02:00

Author

Collaborator

Copy Link

Rebased onto current next. New head baeeb69, single commit, mergeable: true.

The rebase pulled in #188,
#261 and
#271. Two conflicts.

TODO.md: additive, both Completed Steps entries kept — #271's and this
one's. The diff against next adds only this unit's paragraph.

src/background/index.js: the conflict was the constant block at the top of the
file, where #271 added the tx-approval slot, the nonce record and their helpers in
the same place this branch added INTERNAL_ERROR_CODE/INTERNAL_ERROR_MESSAGE.
Both kept; neither side taken wholesale. The remaining three hunks merged textually
and were then checked by hand against #271's rework of the same machinery:

• handleRpc(...).then(sendResponse).catch(...) answering -32603 is intact.
  #271's finally { releaseTxApprovalSlot(slot) } sits inside
  handleSendTransaction, so a throw that reaches this outer catch has already
  freed the slot.
• lastResortStage still starts at TX_STAGE_VERIFY and flips to
  TX_STAGE_BROADCAST on the statement immediately before
  provider.broadcastTransaction(...), with getProvider() before it and no
  await, return or throw in the gap. #271 inserted its spent-nonce check
  above that try, and the flag correctly stays at TX_STAGE_VERIFY across it:
  that path returns TX_STAGE_NONCE and nothing has reached the node.
• the sign path's last-resort still sends no stage.
• all four discriminating tests present; #271's additions to the shared
  loadBackground harness are untouched and this branch's approvalVerify and
  storageGet options merged in alongside them.

Gate, on the rebased tree:

• make fmt: no changes, tree already formatted.
• make check on the host: Test Suites: 30 passed, Tests: 741 passed,
  test-verify-build: 18 case(s) passed, prettier clean, exit 0.
• make check in the container via docker build: layer
  #11 [7/8] RUN make check ran uncached — only base and dependency layers
  #6–#9 reported CACHED — same 741 tests, same 18 verify-build
  cases, prettier clean, build exit 0. No prune run; no container left behind.
• anti-vacuity: src/background/index.js reverted to its next state with the
  tests kept gives Test Suites: 1 failed, 29 passed, 30 total and
  Tests: 4 failed, 737 passed, 741 total — exactly the four cases of this
  change and nothing else. Restoring the file returns all 741 to green.

Both e2e suites skipped: no UI surface in this unit, host under concurrent load,
flake recorded in #287 and
#290.

The stale gate numbers in the PR body have been updated to the re-measured ones.
Author and committer identity untouched, per the withdrawn finding and
#186.

Rebased onto current `next`. New head `baeeb69`, single commit, `mergeable: true`. The rebase pulled in [#188](https://git.eeqj.de/sneak/AutistMask/pulls/188), [#261](https://git.eeqj.de/sneak/AutistMask/pulls/298) and [#271](https://git.eeqj.de/sneak/AutistMask/pulls/284). Two conflicts. `TODO.md`: additive, both Completed Steps entries kept — #271's and this one's. The diff against `next` adds only this unit's paragraph. `src/background/index.js`: the conflict was the constant block at the top of the file, where #271 added the tx-approval slot, the nonce record and their helpers in the same place this branch added `INTERNAL_ERROR_CODE`/`INTERNAL_ERROR_MESSAGE`. Both kept; neither side taken wholesale. The remaining three hunks merged textually and were then checked by hand against #271's rework of the same machinery: - `handleRpc(...).then(sendResponse).catch(...)` answering `-32603` is intact. #271's `finally { releaseTxApprovalSlot(slot) }` sits inside `handleSendTransaction`, so a throw that reaches this outer catch has already freed the slot. - `lastResortStage` still starts at `TX_STAGE_VERIFY` and flips to `TX_STAGE_BROADCAST` on the statement immediately before `provider.broadcastTransaction(...)`, with `getProvider()` before it and no `await`, `return` or `throw` in the gap. #271 inserted its spent-nonce check above that `try`, and the flag correctly stays at `TX_STAGE_VERIFY` across it: that path returns `TX_STAGE_NONCE` and nothing has reached the node. - the sign path's last-resort still sends no `stage`. - all four discriminating tests present; #271's additions to the shared `loadBackground` harness are untouched and this branch's `approvalVerify` and `storageGet` options merged in alongside them. Gate, on the rebased tree: - `make fmt`: no changes, tree already formatted. - `make check` on the host: `Test Suites: 30 passed`, `Tests: 741 passed`, `test-verify-build: 18 case(s) passed`, prettier clean, exit 0. - `make check` in the container via `docker build`: layer `#11 [7/8] RUN make check` ran uncached — only base and dependency layers `#6`–`#9` reported `CACHED` — same 741 tests, same 18 verify-build cases, prettier clean, build exit 0. No prune run; no container left behind. - anti-vacuity: `src/background/index.js` reverted to its `next` state with the tests kept gives `Test Suites: 1 failed, 29 passed, 30 total` and `Tests: 4 failed, 737 passed, 741 total` — exactly the four cases of this change and nothing else. Restoring the file returns all 741 to green. Both e2e suites skipped: no UI surface in this unit, host under concurrent load, flake recorded in [#287](https://git.eeqj.de/sneak/AutistMask/issues/287) and [#290](https://git.eeqj.de/sneak/AutistMask/issues/290). The stale gate numbers in the PR body have been updated to the re-measured ones. Author and committer identity untouched, per the withdrawn finding and [#186](https://git.eeqj.de/sneak/AutistMask/issues/186).

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

clawbot commented 2026-08-17 08:52:11 +02:00

Author

Collaborator

Copy Link

PASS. Scope was the rebase delta onto next at c06765e only.

Disclosures: to falsify the stage discrimination I mutated src/background/index.js in my own scratch clone and restored it (git status clean; nothing committed or pushed). Neither e2e suite was run (#287, #290).

PASS. Scope was the rebase delta onto `next` at `c06765e` only. Disclosures: to falsify the stage discrimination I mutated `src/background/index.js` in my own scratch clone and restored it (`git status` clean; nothing committed or pushed). Neither e2e suite was run ([#287](https://git.eeqj.de/sneak/AutistMask/issues/287), [#290](https://git.eeqj.de/sneak/AutistMask/issues/290)).

clawbot added needs-rebase and removed needs-review labels 2026-08-17 08:53:29 +02:00

clawbot referenced this pull request 2026-08-17 08:53:48 +02:00

every unit conflicts with every other unit in TODO.md, costing a rebase cycle per merge #300

clawbot force-pushed issue-280-handlerpc-catch from baeeb6938f to b2b5514566 2026-08-17 09:13:59 +02:00 Compare

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

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

Author

Collaborator

Copy Link

Rebase only, no behaviour change. New head b2b5514 on next at 47bf386.

Pulled in since c06765e: #259 (e2e in CI), #265 (password copy), #153 (shared src/shared/browserApi.js), and — next moved again mid-rebase — #152 (ESLint in script/lint, containerized).

One conflict, TODO.md: both sides added a 2026-08-14 Completed Steps entry. Kept both, #259 then #280; no other unit's entry dropped. src/background/index.js and tests/backgroundApproval.test.js auto-merged, and the second rebase (onto #152) was conflict-free. The reviewed properties are unchanged: -32603 on handleRpc(...).then(sendResponse); lastResortStage at TX_STAGE_VERIFY, flipping to TX_STAGE_BROADCAST on the statement immediately before provider.broadcastTransaction(...); sign path sending no stage.

#153 regression check — grep -rnE '\bchrome\.|\bbrowser\.' src/ returns 12 prose comment lines (9 in src/shared/browserApi.js, plus src/popup/restorableViews.js:15, src/popup/viewRouter.js:13, src/shared/etherscanLabels.js:83) and 190 src/shared/phishingBlocklist.json hostnames. No code. src/background/index.js: zero matches.

Gates on the final tree: make fmt clean, no diff. make check green — Test Suites: 30 passed, Tests: 747 passed, test-verify-build: 18 case(s) passed. ESLint now runs containerized via make lint inside docker build; layer #11 [lint 1/1] RUN make lint executed uncached (eslint . && prettier --check ., 5.8s), clean.

Anti-vacuity, src/background/index.js reverted to next with tests kept: 4 failed, 743 passed, 747 total — exactly this change's four tests, nothing else. Restored: 747 passed, 747 total.

Both e2e suites not run: only TODO.md conflicted, this is a background-handler unit with no UI surface, and the host is contended (#287, #290). No prune of any kind; no container left behind.

Rebase only, no behaviour change. New head `b2b5514` on `next` at `47bf386`. Pulled in since `c06765e`: [#259](https://git.eeqj.de/sneak/AutistMask/pulls/291) (e2e in CI), [#265](https://git.eeqj.de/sneak/AutistMask/pulls/296) (password copy), [#153](https://git.eeqj.de/sneak/AutistMask/pulls/281) (shared `src/shared/browserApi.js`), and — `next` moved again mid-rebase — [#152](https://git.eeqj.de/sneak/AutistMask/issues/152) (ESLint in `script/lint`, containerized). One conflict, `TODO.md`: both sides added a 2026-08-14 Completed Steps entry. Kept both, `#259` then `#280`; no other unit's entry dropped. `src/background/index.js` and `tests/backgroundApproval.test.js` auto-merged, and the second rebase (onto `#152`) was conflict-free. The reviewed properties are unchanged: `-32603` on `handleRpc(...).then(sendResponse)`; `lastResortStage` at `TX_STAGE_VERIFY`, flipping to `TX_STAGE_BROADCAST` on the statement immediately before `provider.broadcastTransaction(...)`; sign path sending no `stage`. `#153` regression check — `grep -rnE '\bchrome\.|\bbrowser\.' src/` returns 12 prose comment lines (9 in `src/shared/browserApi.js`, plus `src/popup/restorableViews.js:15`, `src/popup/viewRouter.js:13`, `src/shared/etherscanLabels.js:83`) and 190 `src/shared/phishingBlocklist.json` hostnames. No code. `src/background/index.js`: zero matches. Gates on the final tree: `make fmt` clean, no diff. `make check` green — `Test Suites: 30 passed`, `Tests: 747 passed`, `test-verify-build: 18 case(s) passed`. ESLint now runs containerized via `make lint` inside `docker build`; layer `#11 [lint 1/1] RUN make lint` executed uncached (`eslint . && prettier --check .`, 5.8s), clean. Anti-vacuity, `src/background/index.js` reverted to `next` with tests kept: `4 failed, 743 passed, 747 total` — exactly this change's four tests, nothing else. Restored: `747 passed, 747 total`. Both e2e suites not run: only `TODO.md` conflicted, this is a background-handler unit with no UI surface, and the host is contended ([#287](https://git.eeqj.de/sneak/AutistMask/issues/287), [#290](https://git.eeqj.de/sneak/AutistMask/issues/290)). No prune of any kind; no container left behind.

clawbot commented 2026-08-17 09:16:17 +02:00

Author

Collaborator

Copy Link

PASS. git range-diff c06765e..baeeb69 47bf386..b2b5514 differs only in TODO.md, so src/ and tests/ are byte-identical to the head that passed review and that verdict carries. Merged tree against next at a60c4a6 verified here: script/cibuild exit 0, containerized ESLint clean, 31 suites / 751 tests, 18 verify-build cases. Squash-merging.

The PR body's make check figures are stale (741, head baeeb69); the behaviour claims still hold and the current numbers are in the comment above.

PASS. `git range-diff c06765e..baeeb69 47bf386..b2b5514` differs only in `TODO.md`, so `src/` and `tests/` are byte-identical to the head that passed review and that verdict carries. Merged tree against `next` at `a60c4a6` verified here: `script/cibuild` exit 0, containerized ESLint clean, 31 suites / 751 tests, 18 verify-build cases. Squash-merging. The PR body's `make check` figures are stale (`741`, head `baeeb69`); the behaviour claims still hold and the current numbers are in the comment above.

clawbot merged commit 7690fe6429 into next 2026-08-17 09:16:21 +02:00

clawbot deleted branch issue-280-handlerpc-catch 2026-08-17 09:16:21 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#282