sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

test: the defective-wallet UI gates have no coverage — all six can be deleted with the suite still green #254

New Issue
Open
opened 2026-08-12 10:42:40 +02:00 by clawbot · 0 comments
clawbot commented 2026-08-12 10:42:40 +02:00
Collaborator
Copy Link

Three findings from the independent review of #247, reported as non-blocking and filed rather than dropped. That PR has landed.

1. Six gates, zero regression signal

The reviewer removed all six defective-wallet UI gates and make test still reported 15 suites / 383 passed. The e2e suite has no defective-wallet case either. Only walletListHtml and the getSignerForAddress backstop carry any coverage.

The code is correct — the reviewer verified each gate by reading and confirmed via make test-e2e (13/13) that no touched view throws. But six guards that protect a user from a signing path that would otherwise throw mid-send, with nothing that fails when they are removed, will not survive contact with a future refactor.

This is the same class #152 addresses from the other direction: make check is prettier --check plus unit tests and cannot see a ReferenceError in a view handler, which is how #150 and #151 shipped.

2. A fourth getSignerForAddress caller is ungated

src/popup/views/confirmTx.js:423. Not a live defect — all three showView("send") call sites are gated, and its throw is caught into txStatus.showError — but it is the one path that reaches the signer without a gate in front of it, and it was not in the implementer's enumerated list.

3. Two comments state things that are not true

  • The walletDefects module comment says "the pre-#210 path". Commit 7a7f9c5 stored the xpub with no derivation, so a wallet written by that build would be falsely condemned at depth 0 and missed at depth 4. Unreachable — same-second fix, no tag contains it — but it should read "paths".
  • src/popup/views/addressDetail.js claims "There is no private key to export" for a defective wallet. Untrue: the key derives fine. The gate is still right (without it the catch reports "Wrong password."), but the stated reason is wrong.

Definition of done

  • Each of the six gates fails at least one test when removed, demonstrated by deleting them one at a time with captured output.
  • src/popup/views/confirmTx.js:423 is either gated or explicitly ruled out in the PR body with the reason.
  • Both comments state what is actually true.
  • TODO.md updated in the same commit.
  • make check passes.
Three findings from the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/247, reported as non-blocking and filed rather than dropped. That PR has landed. ## 1. Six gates, zero regression signal The reviewer removed **all six** defective-wallet UI gates and `make test` still reported 15 suites / 383 passed. The e2e suite has no defective-wallet case either. Only `walletListHtml` and the `getSignerForAddress` backstop carry any coverage. The code is correct — the reviewer verified each gate by reading and confirmed via `make test-e2e` (13/13) that no touched view throws. But six guards that protect a user from a signing path that would otherwise throw mid-send, with nothing that fails when they are removed, will not survive contact with a future refactor. This is the same class https://git.eeqj.de/sneak/AutistMask/issues/152 addresses from the other direction: `make check` is `prettier --check` plus unit tests and cannot see a `ReferenceError` in a view handler, which is how https://git.eeqj.de/sneak/AutistMask/issues/150 and https://git.eeqj.de/sneak/AutistMask/issues/151 shipped. ## 2. A fourth `getSignerForAddress` caller is ungated `src/popup/views/confirmTx.js:423`. Not a live defect — all three `showView("send")` call sites are gated, and its throw is caught into `txStatus.showError` — but it is the one path that reaches the signer without a gate in front of it, and it was not in the implementer's enumerated list. ## 3. Two comments state things that are not true - The `walletDefects` module comment says "the pre-#210 path". Commit `7a7f9c5` stored the xpub with **no** derivation, so a wallet written by that build would be falsely condemned at depth 0 and missed at depth 4. Unreachable — same-second fix, no tag contains it — but it should read "paths". - `src/popup/views/addressDetail.js` claims "There is no private key to export" for a defective wallet. Untrue: the key derives fine. The gate is still right (without it the `catch` reports "Wrong password."), but the stated reason is wrong. ## Definition of done - [ ] Each of the six gates fails at least one test when removed, demonstrated by deleting them one at a time with captured output. - [ ] `src/popup/views/confirmTx.js:423` is either gated or explicitly ruled out in the PR body with the reason. - [ ] Both comments state what is actually true. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#254
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?