From 18d1fb122285b21c59c08144fd14680c79fa2af0 Mon Sep 17 00:00:00 2001 From: Chris Troutner Date: Wed, 16 Sep 2026 21:34:14 -0700 Subject: [PATCH] Record multi-push encoding review and verification By architect. --- .../memo-multipush-encoding-summary.md | 164 ++++++++++++++++++ .../memo-multipush-encoding-verification.json | 46 +++++ 2 files changed, 210 insertions(+) create mode 100644 docs/reviews/memo-multipush-encoding-summary.md create mode 100644 docs/reviews/memo-multipush-encoding-verification.json diff --git a/docs/reviews/memo-multipush-encoding-summary.md b/docs/reviews/memo-multipush-encoding-summary.md new file mode 100644 index 0000000..f8b1811 --- /dev/null +++ b/docs/reviews/memo-multipush-encoding-summary.md @@ -0,0 +1,164 @@ +# memo-multipush-encoding — Architect Review + +Task: `memo-multipush-encoding` +Component: `psf-memo-client` +Base: `c018a01` (last merged architect review); inbound refactorer commit `8310b6a` + +## What was reviewed + +Inbound refactorer batch (priority 10), merged onto `swarmforge-architect` by +fast-forwarding to `8310b6a`. The linear chain reviewed: + +- **`6eddd9c`** — specifier: *Specify multi-push encoding for Memo multi-field + actions*. Adds `psf-memo-client/specs/memo-multipush-encoding.feature` (5 + scenario outlines) requiring reply, topic message, add-poll-option, and + poll-vote to broadcast the prefix plus fields as **3 separate OP_RETURN + pushes**, and create-poll as **4 pushes**. +- **`2e2db86`** — coder: *Implement multi-push encoding for Memo multi-field + actions*. Rewrites `hex.js` to return a pushes array instead of one combined + payload, adds the `memo-multipush` wallet adapter, switches the five action + services to pass field arrays, adds acceptance step handlers, and adds + unit/property tests. +- **`8310b6a`** — refactorer: *Refactor multi-push helpers and add adapter + property tests*. Shares `encodeUtf8` across `hex`/poll-create/topic-post, + drops the redundant `Uint8Array` branch in the push normalizer, renames the + txid-action builder parameter `buildPayload` → `buildPushes`, extracts the DB + `repairedFixture`, and adds property tests for the adapter. + +**Architect review commit: `fac01730c4`** — the browser-Buffer fix and the +regression test, the `encodeScript` test-helper extraction, the acceptance +assertion de-duplication, the `buffer` dependency, and the `mutate4javascript` +and soft `gherkin-mutator` manifests. The summary and verification record are +committed on top, so `git diff fac01730c4 HEAD` touches only `docs/`. The +record's `git_sha` is `fac01730c4`, the commit that contains the verified source +state. + +## Architectural findings and fixes applied + +The refactorer's structure is sound: `hex.js` and `utf8.js` are pure leaves, +the five action services are core modules with injected wallet/store adapters, +and `memo-multipush.js` confines the wallet wiring to a small adapter boundary. +Two real issues were found and fixed. + +1. **Browser runtime defect — the adapter read the Node global `Buffer`.** This + is the important finding. `memo-multipush.js` called `Buffer.from(...)` + directly, but the CRA 5 production bundle does **not** polyfill Node globals + and the externally loaded wallet script does not define `window.Buffer` + (verified: the built bundle had no `Buffer` assignment and the only other + `Buffer.from` came from axios behind a `typeof Blob` guard). Every real + browser reply, topic message, poll option, poll vote, and create-poll would + have thrown `Buffer is not defined` at runtime, while the Node test suite + passed. `@psf/bitcoincashjs-lib`'s `compile2` requires genuine `Buffer` + instances (`Buffer.isBuffer` + `.copy`), so a `Uint8Array` substitution is + not possible. Fix: import the browser-safe implementation + (`const { Buffer } = require('buffer')`) — the same module the bundled + bitcoin library uses — and declare `buffer` as a direct dependency so the + adapter does not depend on a dev-transitive package. Confirmed in the built + bundle: the adapter now compiles to `const{Buffer:r}=n(6382)` instead of a + free global. A regression test re-requires the module with `global.Buffer` + removed and asserts pushes still build. +2. **Mutation survivors in `attachMultiPushOpReturn`.** The first run killed 2 + of 4 sites. The `!wallet || wallet.__multiPushAttached` logical mutation + survived because nothing tested a falsy wallet, and `__multiPushAttached = + true` survived because the idempotence test only counted wallet calls (which + a harmless double-wrap does not change). Focused tests now assert a falsy + wallet is a no-op and that a second attach leaves the wrapper function + identity unchanged; the file re-runs 4/4. +3. **DRY — duplicated test/acceptance helpers.** The `encodeScript` Bitcoin + script double was copied verbatim between the new unit and property tests; + extracted to `test/support/script-encoding.js` (a test helper, outside + `test/unit/**` and `test/property/*.test.js`). The three new acceptance + handlers that assert a UTF-8 push (topic/text/question) were structurally + identical; extracted `assertUtf8Push`. Scoped DRY dropped the duplicate + blocks in the touched set from 132 to 129 and removed the new-handler + duplicates. +4. **UI/Core separation, dependency rule, information hiding.** `memo-multipush` + depends on nothing; `hex`/`utf8` are pure; the action services depend inward + on those leaves; no React/DOM/router/LevelDB/wallet-global leaks. The wire + order still has one definition (`txidToWireBytes`) and the txid+text push + shape one definition (`buildTxidTextPushes`). The adapter hides the + `bchjs.Script.encode2` swap behind `attachMultiPushOpReturn`, restoring it + synchronously inside the same tick (the property tests pin the expansion and + delegation contract). + +No further boundary change was required. + +## Verification results + +### Language mutation (`mutate4javascript`, `--mutate-all` when the wrapper +detected differential under-selection, `--max-workers 8`) + +| File | Sites | Killed | Survived | Uncovered | +|------|------:|-------:|---------:|----------:| +| `src/services/memo-multipush.js` | 4 | 4 | 0 | 0 | +| `src/services/hex.js` | 5 | 5 | 0 | 0 | +| `src/services/utf8.js` | 0 | 0 | 0 | 0 | +| `src/services/memo-reply.js` | 2 | 2 | 0 | 0 | +| `src/services/memo-topic-post.js` | 7 | 7 | 0 | 0 | +| `src/services/memo-poll-create.js` | 7 | 7 | 0 | 0 | +| `src/services/memo-poll-option.js` | 0 | 0 | 0 | 0 | +| `src/services/memo-poll-vote.js` | 0 | 0 | 0 | 0 | +| `src/services/memo-txid-action.js` | 4 | 4 | 0 | 0 | + +`utf8.js`, `memo-poll-option.js`, and `memo-poll-vote.js` were confirmed with +`--scan` as structural zeros (`Total mutation sites: 0`): they contain no +arithmetic/comparison/boolean/logical/`0<->1` sites, only string/array work and +base-class delegation. `async-load.js` is the browser/wallet adapter shell and +is excluded from tools that run the test suite, consistent with the standing +precedent for adapter shells. + +### DRY (`dry4javascript`, scoped to the touched production files, tests, and +adapters) + +129 duplicate blocks remain in the scoped set, all pre-existing pattern +boilerplate: `acceptance/lib/handlers.js` step-handler repetition (192 of the +line references), the established per-suite `makeWallet` test double (also +present across ~28 client test files), and parallel poll/topic/reply unit-test +bodies that exercise the parallel `MemoTxidAction` subclasses on purpose. The +only two task-local duplications (`encodeScript`, the three UTF-8 push +assertions) were extracted. The production-service duplicates +(`memo-poll-option`/`memo-poll-vote` config blocks, the two 2-field +constructors) are intentional parallel-config declarations and were left as-is +to preserve their per-action clarity. + +### CRAP / cyclomatic complexity (`crap4javascript`) + +All changed functions at or below CRAP 5.0 with ~100% coverage: +`hexToBytes` (CC 5, 100%, 5.0), `MemoPollCreate.create` (CC 4, 4.0), +`attachMultiPushOpReturn`/`broadcastMultiPush` (CC 3, 3.0), and the rest CC ≤ 3. +`MemoReply.reply` is 90% covered but CRAP 2.0. Well below the 8.0 threshold. + +### Soft Gherkin acceptance mutation (`gherkin-mutator --level soft`) + +**`memo-multipush-encoding.feature`: 20 executed, 8 killed, 12 survived, 0 +errors.** Every survivor is a single-character case mutation of a `text`, +`topic`, or `question` example value. Those values are used consistently on the +setup and assertion sides of their scenario (`...with the text ""` / +`...is the UTF-8 text ""`), so the mutant is an intrinsic equivalent: the +push bytes match whatever the example says. The point of the feature — the +separate-push shape (count), the little-endian wire txid, the poll type, and the +option count — is killed (all 8 non-text mutations, including the `option_count` +and txid mutations). No implementation change is warranted. The tool left its +manifest (empty `scenarios`, because every scenario has equivalents) in the +feature file; it is committed as tool-written. + +### Suite status + +Canonical record against review commit +`fac01730c4beaf5b412302f919e47871778f4d08`: + +- `swarmforge/scripts/verify.sh client --record + docs/reviews/memo-multipush-encoding-verification.json --task + memo-multipush-encoding` -> **pass (5/5)**: unit **425 pass / 0 fail**, + property **85 pass / 0 fail**, acceptance **all 31 suites passed**, lint + **ok**, build **ok**. + +## Handoffs sent + +- End-of-chain `git_handoff` to the specifier (task `memo-multipush-encoding`) + with the review commit `fac01730c4` so it can merge `swarmforge-architect` + into `master`. +- No coder/refactorer handoff: the review is a runtime fix, mutation hardening, + and local DRY with no follow-up work for those roles. + +By architect. diff --git a/docs/reviews/memo-multipush-encoding-verification.json b/docs/reviews/memo-multipush-encoding-verification.json new file mode 100644 index 0000000..a929f88 --- /dev/null +++ b/docs/reviews/memo-multipush-encoding-verification.json @@ -0,0 +1,46 @@ +{ + "schema_version": 1, + "task": "memo-multipush-encoding", + "component": "psf-memo-client", + "git_sha": "fac01730c4beaf5b412302f919e47871778f4d08", + "branch": "swarmforge-architect", + "timestamp": "2026-09-17T04:33:07.792Z", + "commands": [ + { + "name": "unit", + "command": "npm test", + "exit": 0, + "duration_ms": 8897, + "summary": "425 pass / 0 fail" + }, + { + "name": "property", + "command": "npm run test:property", + "exit": 0, + "duration_ms": 10258, + "summary": "85 pass / 0 fail" + }, + { + "name": "acceptance", + "command": "npm run test:acceptance", + "exit": 0, + "duration_ms": 14315, + "summary": "all 31 acceptance suites passed" + }, + { + "name": "lint", + "command": "npm run lint", + "exit": 0, + "duration_ms": 2278, + "summary": "ok" + }, + { + "name": "build", + "command": "npm run build", + "exit": 0, + "duration_ms": 60886, + "summary": "ok" + } + ], + "result": "pass" +}