refactor(sphere)!: remove the remaining process-global state — one Sphere per network, disposable, concurrent-safe

[MastaP]

Closes #766. Folds in #769 item 2 and #770 item 5. Surfaced #771.

Continues #767 (per-instance TokenRegistry), which closed item 1 inside the SDK.

What was wrong

Sphere held a process-global static instance. Every create/load/import overwrote it, so with two Spheres alive getInstance() and isInitialized() described whichever was constructed last — and after a second Sphere was created and destroyed, getInstance() returned null while the first was alive and serving money.

Worse, clear() and import() keyed off that static rather than the storage they were handed. Sphere.import({ storage: B }) destroyed a live wallet on storage A: identity nulled, payments throwing NOT_INITIALIZED, providers disconnected, and every sphere.on() handler dropped — with no event and no error. The app simply went deaf.

What changed

Sphere.instance is gone. Replaced by a private Map<string, Set<Sphere>> used only by clear()/import(), keyed by backing-store identity — a new optional backingStoreId on the StorageProvider port.

Object identity was the first attempt and was wrong: two provider objects can address the same store (two IndexedDBStorageProviders sharing dbName+prefix, two FileStorageProviders resolving to one path), so clear() through one emptied the KV while a Sphere registered under its twin stayed isReady with its mnemonic gone. Class identity is wrong the other way — StorageProvider.id is a class constant ('file-storage'). Providers omitting the member fall back to object identity, so custom implementations keep today's behaviour. Because string keys are not weak, unregisterLive deletes an entry when its Set empties, and that cleanup has its own test and probe.

Registration happens at exactly the three publication points, preserving #767's invariant that a half-built Sphere is never reachable. Still private — it is not a liveness API.

BREAKING: Sphere.getInstance(), Sphere.isInitialized() and the root export getSphere are removed (126 → 125 exports). The consumer gate found zero users across all 29 sibling repos — every fleet getInstance is TokenRegistry's, every getSphere is repo-local, every isInitialized is a consumer's own field. Replacement: the instance the entry point already returns, plus sphere.isReady.

They were removed rather than deprecated because the defect survives deprecation: a note does not stop a wrong answer being consumed.

clear()/import() are storage-scoped. They destroy only Spheres built on the storage they were given. The exists(storage) disjunct is preserved — that is the storage-wipe contract sphere and Boxy-Run actually depend on, so no consumer moves.

importFromLegacyFile returned the wrong object. It reached for getInstance()! because importFromJSON discarded the Sphere it built. Threading it out (additive, non-breaking) fixes the cause; an interleaved init used to make that path return someone else's Sphere.

tracked_addresses no longer loses addresses. Every persist wrote the instance's whole snapshot, so A switchToAddress(1) → disk [0,1], then B switchToAddress(2) → disk [0,2], with A's entry erased while A's in-memory view still showed it. Now read-merge-write, serialized per provider; union by index, greater updatedAt wins hidden.

Deliberately not network-scoped, contrary to the issue: the payload is {index, hidden, createdAt, updatedAt} and deriveDirectAddress takes no network, so index n is byte-identical everywhere — this belongs on the shared side of "shared seed, isolated operational state". And it is a lost update, not a scoping problem: it reproduces with both Spheres on testnet2. Renaming would have orphaned every deployed wallet's address list for nothing.

The logger stays process-global, deliberately. 370 call sites across 27 files, most in providers constructed before any Sphere exists and shared between them by design (ConnectClient has no owning Sphere at all). Threading a handle would break every provider constructor to gain per-instance control of a third of the messages, and the worst outcome of this particular global is noisy stdout — unlike the registry global, which produced a 10^8 balance error. Fixed the real bug instead: if (options.debug) made the flag one-way, so no second init could quieten it. createNodeProviders had the mirror-image bug (?? false silently disabled a flag the consumer had set) and now matches createBrowserProviders.

Folded in

• Background work outlives its owner's teardown — five confirmed sites, one missing primitive #770.5 — TokenRegistry.resetInstance() now calls the instance dispose() instead of only stopAutoRefresh(), which left disposed unset, the generation unchanged and the in-flight fetch running, so a load past its entry guard re-armed the interval on an instance getInstance() could no longer return.
• checkNetworkHealth reports healthy gateways as unhealthy; Sphere.init drops verification/debug; three docs claims the code does not honour #769.2 — init now forwards verification to create/load. It was silently dropped, so opting into the worker pool at the documented entry point gave you the sequential verifier. (The issue also claimed debug was lost; it is not — init configures the logger itself.)

Verification

Every gate by exit code: lint / typecheck / typecheck:tests / build / test:run = 0. 2075 tests / 121 files. 61/61 mutation probes KILLED.

Both new test suites were falsified, and two-sidedly:

• sphere-instance-scoping.test.ts — restoring the old last-constructed-wins static reds the unrelated Sphere survives half; making liveOn return nothing reds the own Sphere is still destroyed half; returning every Sphere regardless of storage reds all three. Construction order in those tests is load-bearing: A is built last, so it is exactly the instance the deleted static pointed at — otherwise the first test would pass against the old code.
• tracked-addresses-concurrent.test.ts — written before the fix and confirmed red against unfixed code (disk [0,2] where [0,1,2] was expected), then red twice more on reverting to the wholesale write.

Real guards were kept with the mechanism changed rather than the assertion: the clear()-destroys-a-live-Sphere test seeds the private map instead of the static.

One implementation correction worth recording

The first tracked_addresses parse dropped unusable rows. That broke an existing round-trip pin and would have silently deleted a real user's address if any stored row were ever odd — worse than the bug being fixed. It now repairs instead: only a row with no finite index is dropped, and missing timestamps read as 0 so a timeless observation loses every conflict rather than winning on a fabricated one.

Not fixed here

• The exported singleton-bound registry readers (getDecimals, getSymbol, normalizeCoinId, TokenRegistry.getInstance) still resolve the process-global. refactor(registry): a Sphere owns its token registry, and destroy() disposes it #767 fixed sphere.payments.assets(), but the frontend's useTopUp/useAssets read those exports directly, so the 10^8 decimals bug is still live at the consumer boundary. Removing them is a genuine breaking change requiring a coordinated frontend PR — unlike everything above, which broke nobody.
• Two FileStorageProvider objects on one dataDir clobber the entire wallet file, money journals included #771 — two separate FileStorageProvider objects over one dataDir clobber the entire wallet file, money journals included. Larger than this issue and filed separately.
• Background work outlives its owner's teardown — five confirmed sites, one missing primitive #770 items 1-4 — fixed here after all; see the overnight batch below. The ordering trap held: the worker-pool hang (770.4) landed before the mailbox-drain fix (770.2), or a silent leak would have become a permanent hang.

---

Added after the first review rounds (overnight batch)

The PR grew past #766 on instruction: "make sure this pr fixes all outstanding issues with
mainnet stuff and global state." Everything below is that sweep. Each fix was falsified —
the guard reverted, the test observed failing, the guard restored — and the observed failure
is recorded in the commit message rather than asserted in the abstract.

#770 items 1–4 — teardown leaving live work behind

Landed in the mandated order (4 → 2 → 1 ‖ 3): tracking the mailbox drain before fixing the
worker-pool hang would have converted a silent leak into a permanent hang of
facade.stop().

• 1 — setIdentity orphaned the previous NostrClient. It moved this.nostrClient
  before connecting, so a failed connect left the old client's sockets, ping intervals and
  auto-reconnect chain running and unreachable — disconnect() only reaches the field —
  and since setIdentity never touches status, the caller's next retry orphaned another.
  The field now moves only after a successful connect. Not "tear down both": the mux shares
  that client, so that would kill its socket over a transient relay timeout. Folds in the
  same-class un-cleared deadline timer in connect().
• 4 — engine.dispose() mid-verify never settled the batch. The base SDK's
  WorkerPool.dispose() only terminate()s; a dispatched task resolves solely from
  worker.onmessage, so verify hung forever. Reachable via setOracleApiKey →
  setEngine. The cancellation rejects, and that is the money-critical part rather than
  a style choice: Receive.screen() turns a falsy verdict into a permanent
  rejectAck(entry, 'invalid'), so resolving { ok: false } would have destroyed a valid
  incoming token because an api-key change landed mid-drain. Only affects consumers opting
  into verification.createWorker.
• 2 — the mailbox poll drain was untracked. Receive.start() spawned its 30 s poll with
  a bare void drainOnce(), so stop() could return while a drain was still doing
  wallet-api I/O, verification, scoped-KV writes and transfer:incoming emission. The wake
  path was accidentally covered — but only by subscriber ordering.
• 3 — a switchToAddress racing destroy() re-armed the wallet. Liveness was checked
  once, then awaited ~8 times, and _initialized is cleared last so it could not mark
  "teardown has begun". Two live vectors: the transport mux was rebuilt and reconnected
  (destroy() leaves it null, which is exactly the build condition), and the switch's
  stop/start pair — queued behind destroy()'s own stop — started a whole new vertical for
  an owner whose destroy() had already resolved. A destroyed latch set as destroy()'s
  first statement gates both. A refused switch also no longer persists the index it never
  finished moving to, which sent the next boot to the wrong address.

Two of the added guards are defence in depth and not individually falsifiable — an
earlier guard throws first on every path reaching them. Their comments say so, and they are
deliberately not probed, rather than shipping probes that would report SURVIVED forever.

#769.1 — checkNetworkHealth could not tell healthy from unhealthy

It POSTed get_round_number with empty params and keyed the verdict off response.ok. The
gateway is a routing layer and refuses any call carrying neither stateId nor shardId
with HTTP 400 before it looks at the method — so the check you would reach for to gate a
mainnet cutover reported every live gateway unhealthy. It now sends get_block_height with
a 32-byte stateId and reads the JSON-RPC body: the status code cannot answer this in
either direction, since a healthy gateway answers a routing mistake with a 400 plus a body
and JSON-RPC puts application errors inside a 200.

The e2e that should have caught this asserted typeof healthy === 'boolean', which passes
whether the network is up or down. It now asserts healthy, with the error text inside the
expectation so a genuine outage names itself. Verified live against testnet2 and mainnet
(block heights ~40.9k / ~268k); reverting the request shape fails it against both.

#769.3 — documentation the code does not honour

Four CLAUDE.md claims corrected against source: there is no durable receive seen-set
(dedup runs against an in-memory heldStates map seeded from the inventory view, backed by
the server-side history dedupKey), and coin symbols are not registry-resolved on the
money path (mint() rejects non-hex, send() byte-compares, requests.create passes
through) — three sites showed coinId: 'UCT'. That last one is not cosmetic: it is why a
registry finding was initially mis-scoped as presentation-only during the #767 review.

tests/aggregator/ gained the INVALID_TRUSTBASE vacuity guard CLAUDE.md already asserted
as fact — grep -rn INVALID_TRUSTBASE tests/ previously returned zero. A token the real
aggregator-go certified must be refused when the trust base carries a wrong root key, with
node ids left intact so the quorum rule finds each node and rejects it on its key rather
than through "No root node defined" (asserted absent). Two layers, because either alone is
dishonest: the engine-level call is exactly what Receive.screen() makes but can only say
refused; the trace-level assertion names the cause, on a reconstruction that is
self-checking (the correct trust base must come back OK through the same context). It does
not exercise quorum — the compose stack is single-root-node, so mainnet's 4-node /
threshold-3 shape remains unexercised anywhere in the repo.

A related weak test was replaced: the factory case for "the trust base is the single source
of the network id" asserted only that construction succeeded, which an engine ignoring the
trust base entirely would also satisfy. It now decodes a real minted token offline to read
the derived id.

#773 — the seven doc items inside this PR's blast radius

Only those; the other ~25 are pre-existing rot for a separate PR. The one that matters:
docs/INTEGRATION.md published the StorageProvider interface with saveTrackedAddresses
and no contract, so a custom provider written from that doc clobbers addresses — the
exact #766 data-loss bug this PR fixes. It now carries the write contract, the uint32 rule,
and a worked implementation that was extracted, type-checked under --strict and executed.

Still not fixed here

• The exported singleton-bound registry readers. Unchanged from above, and now scoped:
  the consumer gate across all 31 sibling repos found that nine of the ten free readers have
  zero consumers, and the tenth (coinIdsMatch) has one. The blocker is the frontend's
  16 getInstance() sites plus 5 lines each in agentic-chatbot and sphere-infra. The
  complete SDK edit list and frontend migration are on Remove the process-global state: one Sphere per network, disposable, and concurrent instances must not interfere #766, ready to execute as a paired PR.
• Two FileStorageProvider objects on one dataDir clobber the entire wallet file, money journals included #771 — unchanged.
• Money-critical: a pending delivery is stranded on intent completion, and a trust-base rotation freezes the fleet #768 — both halves decided (discussed with codex, every claim re-verified). Item 1 is
  blocked on a wallet-api delivery outbox, filed as unicity-sphere/wallet-api#135; item 2's
  cheap classification patch is unblocked but is a money-path change that wants its own PR.
  Neither belongs in a lifecycle PR.

Review round 5 (Copilot) — six findings, all valid, all fixed

Two were introduced by the fixes above, which is the useful kind of review.

• The client swap could resurrect a client after teardown. Moving
  this.nostrClient to after the connect (item 1) closed the orphan leak and opened this:
  a disconnect() during the pending connect nulls the field and returns, and the
  resolving swap then installs a live client plus subscriptions owned by nobody. There is
  now an ownership re-check before any field moves, and it rejects — with the identity
  staged, a failed swap applies neither client nor identity, so resolving would tell the
  caller it took.
• The verifier's cancellation leaked one reaction per verification. Promise.race
  subscribes to every input and does not detach when another wins, so racing against one
  long-lived promise pinned a reaction per finished verify until dispose() — retention
  growing with every token received, inside the class added in item 4 to fix a teardown
  bug. Now per-call, and deliberately not the reviewer's deferred-plus-race shape, which
  allocates three promises per call where this allocates the one already owed to the
  caller — which also dissolves the unhandled-rejection question rather than managing it.
• setIdentity applied the identity before the client — see Background work outlives its owner's teardown — five confirmed sites, one missing primitive #770 item 6 above; found
  independently from the diff.
• readBlockNumber accepted any string or number, so {blockNumber: "error"} read as
  healthy: the same too-permissive reading as the bug checkNetworkHealth reports healthy gateways as unhealthy; Sphere.init drops verification/debug; three docs claims the code does not honour #769.1 fixes, one layer down.
• The uint32 index rule held only on read. Now enforced on write and — more
  importantly — at _deriveAddressInternal, the one point every index reaches derivation
  through. The storage layer alone is too late: ensureAddressTracked mutates the
  in-memory registry before persisting. The reviewer also asked for a guard at
  switchToAddress; declined, because derivation refuses first on every reachable path,
  so it would be exactly the non-falsifiable guard whose probe was deleted this round.
• Sphere.clear() could wipe a store under an in-flight init, which then published a
  Sphere reporting isReady over nothing. Fixed with a per-store clear generation rather
  than the suggested serialization: import() clears internally, so a per-store mutex
  round-trips through itself, and a clear() blocked behind a slow bring-up is a worse
  outage than a loud refusal.

Review rounds 6-7 (Copilot + local codex)

Four more findings, three of them real, and the two most serious showed this PR's own fix
was incomplete.

• Identity state split per tsup bundle. tsup.shared.js defines 12 configs, each
  splitting: false, each emitting esm+cjs — so Sphere._liveByStorage and the clear
  generation were per-bundle. A Sphere built through sphere-sdk was invisible to a
  clear() through sphere-sdk/core, which wiped the store and left it isReady over an
  emptied KV: Remove the process-global state: one Sphere per network, disposable, and concurrent instances must not interfere #766 again, across the entry-point boundary. LocalStorageProvider had
  the same shape from a module-local tag counter, and its read-merge-write serializer with
  it. All of it moved to a versioned globalThis cell, the pattern core/logger.ts
  already uses. The object-key allocator had to move too — sharing the registry while
  leaving the counter split would have been worse than the bug, putting two unrelated
  wallets in one bucket.
• destroy() landing INSIDE module bring-up. The switch guards are checks before an
  await, so a teardown starting mid-bring-up had its module-clearing loop run first and the
  continuation then registered a fully-built set — its own engine, hence its own worker
  pool — on a Sphere whose destroy() had returned.
• checkNetworkHealth dropped its deadline before reading the body. fetch settles on
  the headers, so a gateway stalling mid-response hung the probe forever.
• Not accepted as written: a reentrancy hazard in the verifier. WorkerTokenVerifier.verify
  awaits the genesis rule before the pool fan-out, so a reentrant dispose() cannot land
  before the cancellation is registered — I wrote the proposed test and it passes with the
  proposed guard removed. Kept the two-line guard as insurance against an SDK reordering,
  labelled in code and test as unreachable today and deliberately unprobed.
• Also not accepted: sharing FileStorageProvider's tracked-address chain across
  provider objects. The premise — that backingStoreId made that scenario supported — is
  wrong; it scopes teardown, and the concurrent-write half is open as Two FileStorageProvider objects on one dataDir clobber the entire wallet file, money journals included #771. I implemented
  it, then reverted: that provider rewrites the whole file from a per-object cache, so a
  sibling's unrelated write still rolls the registry back, and serializing the tracked
  path alone would have made the skipped contract case pass while leaving the provider
  unsafe.

The mutation run earned its keep

A full run left three survivors, and two were guards added in these rounds silently
shadowing older ones: the reentrancy re-check also rejects a post-dispose verify(), and
the post-bring-up discard undoes whatever the bring-up built — so in both cases deleting
the older guard stopped changing any observable outcome. The guards are right and stay;
the tests were sharpened to assert what each guard uniquely does (the torn-down verifier is
never entered; the bring-up is never started). The third survivor was a badly-aimed
probe of mine that swapped a shared WeakMap while leaving the shared counter, so it could
never fail.

Verification

Every gate by exit code: lint / typecheck / typecheck:tests / build / test:run = 0.
2198 tests / 129 files. 116/116 mutation probes KILLED.

One probe was removed rather than fixed, and the reason is worth stating: the
ensureAlive() before _transport.setIdentity is shadowed by the index-persist guard
added later in the same commit, which throws first on every path reaching it. Its probe
SURVIVED a full run. The guard stays (it still covers the await between them) and is
annotated as defence in depth; the probe went, because one that nothing can kill is worse
than none. Two other guards in that method are annotated the same way and were never
probed.

[Copilot]

🟡 Changes recommended

Malformed tracked-address indices can alias derived addresses, while several central regressions and migration documentation remain insufficiently guarded.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes Sphere’s global singleton lifecycle while improving concurrent storage handling and teardown safety.

Changes:

• Scopes live Sphere instances by storage and removes singleton APIs.
• Merges concurrent tracked-address updates.
• Fixes verification forwarding, logger configuration, registry disposal, and legacy import returns.

File summaries

File	Description
core/Sphere.ts	Reworks lifecycle, imports, logging, verification, and address timestamps.
index.ts	Removes getSphere export.
registry/TokenRegistry.ts	Fully disposes reset singleton.
storage/tracked-addresses.ts	Adds parsing and merge helpers.
storage/storage-provider.ts	Defines merge-on-save contract.
impl/nodejs/storage/FileStorageProvider.ts	Implements serialized address merging.
impl/nodejs/index.ts	Preserves explicitly configured logging.
impl/browser/storage/LocalStorageProvider.ts	Implements serialized address merging.
impl/browser/storage/IndexedDBStorageProvider.ts	Implements serialized address merging.
tests/mutation/probes.json	Adds tracked-address merge probe.
tests/integration/sphere-instance-scoping.test.ts	Tests storage-scoped lifecycle behavior.
tests/integration/tracked-addresses-concurrent.test.ts	Tests concurrent address persistence.
tests/integration/sphere-payments-v2-wiring.test.ts	Updates publication assertions.
tests/integration/wallet-clear.test.ts	Removes singleton resets.
tests/integration/tracked-addresses.test.ts	Removes singleton resets.
tests/integration/nametag-overwrite-guard.test.ts	Removes singleton resets.
tests/integration/nametag-normalization.test.ts	Removes singleton resets.
tests/unit/core/Sphere.status.test.ts	Tracks instances locally for teardown.
tests/unit/core/Sphere.registerNametag.test.ts	Removes singleton resets.
tests/unit/core/Sphere.network-delegation.test.ts	Tracks instances locally for teardown.
tests/unit/core/Sphere.destroy-secrets.test.ts	Tracks instances locally for teardown.
tests/unit/core/Sphere.clear.test.ts	Adapts clear tests to storage-scoped registry.

Review details

Suppressed comments (2)

core/Sphere.ts:714

• The new-wallet init() branch also needs a Sphere-level regression proving verification reaches the created engine. Testing only the engine factory would still pass if this forwarding line were removed, recreating the documented sequential-verifier fallback for newly created wallets.

      verification: options.verification,

core/Sphere.ts:1999

• The return type now exposes the imported Sphere, but the public JSDoc still says the method returns only success and an optional mnemonic. Document sphere so generated API docs describe the new additive contract.

  }): Promise<{ success: boolean; sphere?: Sphere; mnemonic?: string; error?: string }> {

• Files reviewed: 22/22 changed files
• Comments generated: 7
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[MastaP]

Fixed in c1b05e0f — new tests/unit/core/Sphere.init-verification.test.ts, covering both branches.

You were right that this was undeletable-in-name-only: the existing worker tests build the engine directly and never go through Sphere.init, so either forwarding line could have gone with the suite green.

The falsification discriminates, which is the part I care about: deleting the load() forwarding reds only the load test, deleting the create() forwarding reds only the create test, other cases green in both runs. The discriminator is which verifier owns engine.verify — a probe token makes the pool verifier reject while the stubbed Token.verify resolves. Two control cases are kept so a rejection cannot be mistaken for an unrelated failure. Two mutation probes added.

[MastaP]

Fixed in c1b05e0f. Correct — _liveByStorage maps to a Set, but every test had one Sphere per storage and the unit test seeded a single mock, so an implementation tracking only one instance per provider would have passed.

Added a case with two live Spheres on the same provider asserting clear() destroys both. Falsified by narrowing registerLive to set(storage, new Set([sphere])) — reds only that test, expected true to be false, other six green.

[MastaP]

Fixed in c1b05e0f. There was no test of either method, which is how the wrong-object bug survived in the first place.

Added an interleaving regression: another Sphere stays alive on different storage while the sphere-wallet-JSON path runs, asserting the returned Sphere is the one imported into the supplied storage.

Three separate falsifications, each reddening its own case: discarding the Sphere in importFromJSON's mnemonic branch reds two tests; the master-key branch reds one; dropping sphere: result.sphere in importFromLegacyFile reds one. All with expected undefined to be defined. Also documented the additive sphere field on the @returns, per your suppressed comment.

[MastaP]

Fixed in c1b05e0f. Added a static-path regression with fake timers and an in-flight fetch.

The interesting part is what turned out to be load-bearing. Reverting to stopAutoRefresh() reds the test — but when I removed the isDisposed/interval assertions to check the abort independently, it still red with expected +0 to be 1: the fetch was never aborted. The interval assertion alone does not red, which is exactly your point — the old code cleared the timer and did nothing else, so a test asserting only the timer would have passed against it.

[MastaP]

Fixed in c1b05e0f. Added tests/unit/storage/contracts/tracked-addresses.contract.ts and ran it against all three providers, following the existing tests/unit/payments-v2/contracts/ idiom. Includes the failed-write recovery case you asked for.

Nine falsifications, three per rule, and each reds only the provider it was applied to:

• mergeTrackedAddresses(onDisk, entries) → entries: each provider reds its own four cases, other eight green.
• serialization removed: reds only serializes concurrent calls, expected [0,3] to deeply equal [0,1,2,3].
• rejection-swallow removed: reds only the recovery case — the second write inherits the first's rejection.

That last one is the case I would not have written without the prompt, and it is the one that matters operationally.

[MastaP]

Fixed in c1b05e0f, and this was the one real correctness bug in the review.

Number.isFinite(1.5) is true, and deriveKeyAtPath parseInt()s the path segment (core/crypto.ts:232), so a stored row with index 1.5 derives index 1's keys — a malformed row silently impersonating a real address. Negatives reached derivation too. Now requires a non-negative integer.

Worth recording why this was easy to get wrong: the parse deliberately follows repair-over-drop, because dropping an odd row deletes one of the user's addresses. That is right for every field except the index — an unusable index is not a recoverable address, and keeping it aliases one that is. The rationale now lives on the saveTrackedAddresses port docstring, where the next implementer looks.

Tested and falsified: restoring Number.isFinite reds three cases ([0,1,1.5] vs [0,1], [-1,0] vs [0], and the mixed set). One vacuity caught while writing it — a stored NaN cannot round-trip, since JSON.stringify writes null, so a NaN-only assertion would have passed either way. Folded into a rejection set carrying 1.5, -1, Infinity, a string and a missing index, so every test in the file reds under the revert.

[MastaP]

Fixed in c1b05e0f. That section was mine, written in #767 and never revisited when this PR removed the APIs it describes — so the migration guide was actively directing consumers at Sphere.getInstance()/isInitialized() and telling them clear()/import() are storage-blind.

Rewritten: the removals with their replacement (hold the instance the entry point returns; sphere.isReady), the new storage-scoped clear()/import() behaviour, and what genuinely remains — two FileStorageProvider objects over one dataDir, now tracked as #771.

[Copilot]

🟡 Changes recommended

The parser accepts out-of-range BIP32 indices, and the breaking lifecycle documentation remains contradictory.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

impl/nodejs/index.ts:177

• This changes omitted debug from “reset to false” to “preserve the consumer’s current setting,” but the Node provider tests do not cover either omitted or explicit-false behavior. Add a regression that pre-enables the logger, verifies createNodeProviders without debug preserves it, and verifies debug: false disables it.

• Files reviewed: 28/28 changed files
• Comments generated: 3
• Review effort level: Balanced

[MastaP]

Fixed — new tests/unit/core/Sphere.debug-logging.test.ts, 11 tests, discriminating per entry point: reverting init reds three, create/load/import one each, no cross-talk. Four probes added.

Fair hit, and pointed: this is the same coverage gap you flagged last round, in code I wrote to satisfy that round.

Writing it turned up something I had wrong. I had told my reviewer that #769's debug finding was mistaken because init configures the logger itself. The forwarding really is missing — init does not pass debug to the create/load it dispatches to. It is harmless today only because init configures the global first and !== undefined makes the later calls no-ops. Under a ?? false rewrite, Sphere.init({ debug: true }) would end up off, because create would then run configure({ debug: undefined ?? false }) after init had set it true. So the guard is load-bearing for a second reason beyond the one-way bug, and there is now a test pinning that exact sequence.

[MastaP]

Fixed. Correct, and it shows my previous fix caught only one end of the range.

Last round I rejected fractional and negative indices because deriveKeyAtPath parseInt()s the path segment, so 1.5 derives index 1's keys. But deriveChildKey serializes the child number as index.toString(16).padStart(8, '0') (core/crypto.ts:166-177) — above 0xffffffff, toString(16) yields more than 8 hex digits and padStart does not truncate, so the derivation silently emits extra bytes. Same defect, opposite end. Now bounded to a uint32.

Boundary cases added, and each clause is independently load-bearing: removing <= 0xffffffff reds only the over-range case; >= 0 only the negatives; Number.isInteger only the fractionals. 0xffffffff, 0x80000000 (the hardened threshold) and 0 stay green under every mutant, so the ceiling never swallows a legal index. Two probes added.

[MastaP]

Fixed. Both locations now give one account.

The migration guide had the removed statics sitting under "Not fixed by this release" — I edited that section to describe the removal but left it under the old heading, so it read as the opposite of what it said. It now has an "Also removed" section for the lifecycle globals, with only the #771 FileStorageProvider limitation left under "Not fixed".

CHANGELOG [Unreleased] described only #767's registry work; there was no entry for this PR at all. Added five: the BREAKING removal with the export count and the reason it was not deprecated, the clear()/import() scoping, the tracked-address merge and uint32 bound, the debug/verification fixes, and #770.5.

[Copilot]

🟡 Changes recommended

Backing-store identity, shared-provider teardown, and cross-provider browser writes remain concurrency-unsafe.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (4) — in code that hasn't changed since the last review.

core/Sphere.ts:2189

• Using wall-clock time directly can make an explicit hide/unhide silently lose to the same row already on disk. If the clock moved backwards—or the stored row has a future updatedAt—this value is smaller, so mergeTrackedAddresses restores the old hidden value even though this method emits the change event. Advance from both the clock and the entry's prior version.

This issue also appears on line 2680 of the same file.
impl/browser/storage/IndexedDBStorageProvider.ts:249

• trackedWrites serializes only this provider object. Two IndexedDB providers using the same default database each have an independent chain, and the get and set here run in separate transactions; concurrent saves can both read [0] and then overwrite each other with [0,1] / [0,2]. Put the read and merged write in one IndexedDB read-write transaction (which serializes across connections), and add a two-provider contract case.
  impl/browser/storage/LocalStorageProvider.ts:177
• This mutex is per provider object, not per backing Storage. Two LocalStorage providers over the same window.localStorage have independent chains; because await this.get(...) yields, both saves can read the old registry before either writes, and the last set drops the other address. Coordinate by backing storage/prefix (including the multi-context case) and test two provider instances sharing one Storage object.
  storage/storage-provider.ts:75
• The contract documents only “non-negative integer,” but the shared parser now rejects values above 0xffffffff and the changelog calls the field a uint32. Custom StorageProvider implementations following this interface can therefore still admit the off-standard indices this PR intends to exclude. State the upper bound here as part of the normative contract.

core/Sphere.ts:2680

• This bulk visibility update has the same backward-clock failure as setAddressHidden: a future/prior updatedAt already on disk beats Date.now(), so the newly selected visibility is not persisted. Make the timestamp monotonic relative to the tracked entry.

        (tracked as { hidden: boolean; updatedAt: number }).updatedAt = Date.now();

core/Sphere.ts:1317

• The Set explicitly permits multiple live Spheres to share one provider, but destroying either Sphere still unconditionally calls this._storage.disconnect() (core/Sphere.ts:3489). For IndexedDB and LocalStorage, the remaining Sphere stays isReady but its next storage operation fails ensureConnected(). Shared providers need ownership/reference counting so only the last registered Sphere disconnects storage (while clear() can still destroy all of them).

    if (!live) {
      live = new Set();
      Sphere._liveByStorage.set(sphere._storage, live);
    }
    live.add(sphere);

core/Sphere.ts:2000

• The public README still demonstrates const { success, mnemonic, error } = await Sphere.importFromJSON(...) (README.md:684) and discards the returned instance. With getInstance() removed, a consumer following that example has no handle to use or destroy the live Sphere this method created. Update that example to retain sphere and explain it is present on success.

  static async importFromJSON(options: Omit<SphereImportOptions, 'mnemonic' | 'masterKey' | 'chainCode' | 'derivationPath' | 'basePath' | 'derivationMode'> & {
    jsonContent: string;
    password?: string;
  }): Promise<{ success: boolean; sphere?: Sphere; mnemonic?: string; error?: string }> {

• Files reviewed: 30/30 changed files
• Comments generated: 1
• Review effort level: Balanced

[MastaP]

Fixed in 3a4a0d54. You are right, and this was a regression I introduced in this PR — worse than the behaviour it replaced. The old last-constructed-wins static at least destroyed some Sphere in that scenario; object keying destroyed none, leaving the twin isReady with its mnemonic and journals gone.

The port now carries an optional readonly backingStoreId identifying the store — not the object, and not the class, since id is a class constant ('file-storage') and that is exactly the wrong granularity, as you said. Scheme-namespaced so unrelated stores cannot collide:

file:<resolved absolute path>
indexeddb:<enc dbName>:<enc prefix>
localstorage:<Storage object tag>:<enc prefix>

Omitting it falls back to object identity, so custom providers keep today's behaviour. _liveByStorage becomes Map<string, Set<Sphere>>, with the entry deleted when its Set empties — string keys are not weak, so that cleanup is now load-bearing and has its own test and probe.

Two things worth recording:

Your case had a mirror image that the naive fix would have created. LocalStorageProvider.getStorageSafe() mints a fresh in-memory Storage per provider under SSR, so keying on prefix alone would have merged genuinely unrelated stores into one — the same aliasing in the opposite direction. Hence the WeakMap<Storage, string> object tag.

One falsification ran against real pre-fix code, not a mutant. An accidental checkout put the new tests against the WeakMap implementation and the twin Sphere survived exactly as you described. Separately, setting backingStoreId = 'file:' — the class-constant mistake the new docstring warns against — reds four integration tests as well as the unit ones: cross-wallet kills return immediately.

Eleven falsifications in total, probes 77 → 80, all KILLED. docs/INTEGRATION.md publishes the StorageProvider interface custom implementers code against and now carries the member; the migration guide's "Not fixed" section separates the halves — teardown aliasing fixed here, the whole-file write clobber still open as #771.

[Copilot]

🟡 Changes recommended

Browser providers still lose tracked-address updates across provider objects sharing one backing store, and invalid public address indices remain accepted.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

storage/tracked-addresses.ts:11

• This validates only rows read from storage, while public writers still accept invalid indices: switchToAddress() rejects only index < 0, and ensureAddressTracked() derives and persists fractional or greater-than-uint32 values. Thus switchToAddress(1.5) can derive index 1's key, report success, and write an entry that this parser silently drops on reload. Apply the same uint32 guard before every derivation/persist path, ideally centrally in ensureAddressTracked().

• Files reviewed: 32/32 changed files
• Comments generated: 5
• Review effort level: Balanced

[MastaP]

Fixed. Confirmed: only backingStoreId was resolved — this.dataDir and this.filePath kept whatever relative form the caller passed, so a later process.chdir() would have this provider reading and writing a different file while still reporting the id computed from the old cwd. clear() would then empty one store and destroy the liveness bucket of another, which is the exact failure mode round 3 was about, reached by a different route.

Both are now resolved once at construction, and backingStoreId is derived from the already-absolute filePath rather than resolving independently — so the identifier and the file it names cannot diverge.

[MastaP]

Fixed — description updated. You were right that it described the first attempt rather than the merged code: WeakMap<StorageProvider, Set<Sphere>> keyed by object identity and "bounded by provider lifetime" is now a Map<string, Set<Sphere>> keyed by backingStoreId, with explicit empty-set deletion precisely because string keys are not weak.

Worth keeping visible rather than quietly rewriting, so the description now records both: object identity was the first attempt, why it was wrong (your round-3 finding), and why class identity is wrong in the other direction.

[MastaP]

Fixed. The contract said "NON-NEGATIVE INTEGER" while the implementation and changelog require a uint32 — so a custom provider following the port text would have retained values above 0xffffffff.

The docstring now states the ceiling and why it exists: deriveChildKey pads the child number to 8 hex digits, so a larger value emits extra bytes and derives off-standard. Both ends of the range are now given in the one place a custom implementer reads.

[Copilot]

🟡 Changes recommended

Store-level initialization and multi-Sphere concurrency can still overwrite wallet or payment-recovery state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

core/global-cell.ts:69

• cellBag() accepts any object stored under the public global key, but this property read is not protected. If another page script placed a Proxy there whose get trap throws, importing any module that calls sharedCell() throws, contrary to this helper's stated rule that hostile values are refused rather than crashing. Guard the cell lookup and fall back through put() just as writes and validator calls already do.

core/Sphere.ts:1419

• The generation check still has a publication window while clear() is active. An init that starts after the entry bump records that new generation; if it finishes before clearStore() reaches its finally, this comparison succeeds, the Sphere is registered after liveOn() was snapshotted, and the subsequent wipe leaves it ready over an empty store. The added test delays publication until after clear() returns, so it does not cover this ordering. Track an in-progress clear count per store (including concurrent clears) and refuse publication whenever it is nonzero, in addition to comparing generations.

    if (Sphere.clearGenerationOf(sphere._storage) !== clearGeneration) {

core/network-health.ts:336

• responseTimeMs is captured before the newly awaited response body. A gateway can deliver headers immediately and take seconds to stream the JSON, yet this reports near-zero latency (including the body-timeout path), so health telemetry and thresholds are inaccurate. Compute the elapsed time after response.json() settles.

    const responseTimeMs = Date.now() - startTime;

    const body: unknown = await response.json().catch(() => null);

• Files reviewed: 54/54 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The broad lifecycle, storage-concurrency, teardown, and money-path impact warrants final human validation despite extensive regression coverage.

Review details

• Files reviewed: 54/54 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Initialization, clearing, shared payment-store writes, and failed transport connections still contain concurrency or resource-lifecycle failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

core/Sphere.ts:1295

• The two generation bumps still leave a publication window. If an init starts after the entry bump, it records that generation; while clearStore() is paused before storage.clear(), that init can finish and publishLive() sees the same generation, so it registers successfully. The later wipe and exit bump do not re-check or destroy the newly registered Sphere, leaving it isReady over an empty store. Track an active clear per backing-store key (or otherwise make publication fail for the entire clear interval), and add a case that starts and publishes the init before the parked wipe is released.

    // Bumped on ENTRY and again on exit, so an init that publishes anywhere in this
    // window is refused too — not only one that publishes after the wipe (#772). The
    // snapshot below is taken before the wipe, so a Sphere that registers between the
    // two is neither destroyed here nor able to keep its data.

core/Sphere.ts:1436

• Registering multiple live Spheres for one backing store makes the payments stores concurrently writable without shared coordination. Each composition creates a fresh ScopedKV (core/payments-v2-wiring.ts:306-310), while ListStore serializes read-modify-write only by ScopedKV object identity (modules/payments-v2/machine/journal.ts:60-63,129-139). Two Spheres on the same address can therefore both read the same delivery journal/backstop and overwrite one another, potentially dropping a certified-but-undelivered blob. Either reject a second live Sphere for the same store or key all money-store coordination by backing store + scoped key before treating this Set as supported.

    let live = Sphere._liveByStorage.get(key);
    if (!live) {
      live = new Set();
      Sphere._liveByStorage.set(key, live);
    }
    live.add(sphere);

core/Sphere.ts:936

• The clear generation does not close the existing create/create TOCTOU. Two concurrent Sphere.init({ autoGenerate: true }) calls on one empty backing store both capture the same generation, both can observe exists() === false, and both then write different mnemonics and publish successfully; the stored mnemonic belongs to whichever write landed last while the other live Sphere serves a different identity. Reserve/serialize wallet creation per backing-store key before the existence check and key write, and add a two-concurrent-init regression.

    // #772: recorded BEFORE the first storage read/write and re-checked at publication —
    // a clear() cannot see this init to destroy it, so it would otherwise wipe the keys
    // written below out from under a Sphere that goes on to report itself ready.
    const clearGeneration = Sphere.clearGenerationOf(options.storage);

    // Check if wallet already exists
    if (await Sphere.exists(options.storage)) {
      throw new SphereError('Wallet already exists. Use Sphere.load() or Sphere.clear() first.', 'ALREADY_INITIALIZED');

• Files reviewed: 54/54 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[MastaP]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4440913cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Align IndexedDB lifecycle scope with database-wide clears

When two providers use the same dbName with different prefixes, this ID places their Spheres in separate lifecycle buckets, but IndexedDBStorageProvider.clear() without an argument calls idbClear() and empties the entire kv object store. Clearing one prefixed wallet therefore deletes the other wallet's data without destroying its Sphere, leaving it isReady over an erased store. Either restrict clear() to the configured prefix or make the backing-store ID cover the whole database.

Useful? React with 👍 / 👎.

[MastaP]

Valid, and the sharper framing is that this is #766 re-entering along a dimension the original fix did not consider. Fixed in f6e1a84.

Confirmed: clear() with no prefix takes the idbClear() path (IndexedDBStorageProvider.ts:182-213) and empties the entire kv object store — every prefix in it — while backingStoreId included the prefix. So two prefixed wallets in one database sat in separate liveness buckets while sharing one erasure fate, and clearing either wiped the other's data and left its Sphere isReady over an emptied store. Exactly the failure this PR exists to remove.

I took your second option — make the id cover the database — rather than scoping clear() to the prefix. clear() is documented and used as a whole-wallet wipe (Sphere.clear passes no prefix deliberately, so the mnemonic goes with it), and narrowing a destructive operation is the riskier half of the pair. Aligning the id is fail-safe in the right direction: the bucket becomes wider than strictly necessary, and nobody is left live over erased data. The invariant is now stated in the code — backingStoreId names the unit of erasure — which is the thing that was implicit and therefore driftable.

Two existing tests pinned the wrong invariant (differs on the database name and on the key prefix independently, cannot be forged across the dbName/prefix boundary) and are rewritten rather than deleted, with the reason. Falsified by restoring the prefixed expression; probe idb-store-id-splits-on-prefix.

[chatgpt-codex-connector]

Canonicalize aliases before deriving the file store ID

When the same wallet directory is reached through a real path and a symlink, path.resolve() leaves those aliases distinct even though both providers read and overwrite the same wallet.json. Their backingStoreId values consequently differ, so Sphere.clear() through one alias misses the live Sphere registered through the other alias, wipes its file, and leaves that Sphere reporting ready. Derive the identity from a canonical filesystem path rather than only the lexically resolved path.

Useful? React with 👍 / 👎.

[MastaP]

Correct — fixed in f6e1a84.

path.resolve is purely lexical, so a dataDir reached through a symlink and the same directory reached directly gave different ids for one wallet.json, and Sphere.clear() through one alias missed the Sphere registered through the other.

The wrinkle worth recording is that realpathSync alone does not work here: it throws when the path does not exist, and a fresh dataDir legitimately does not yet — a provider built before connect() created it would then disagree with one built after, reintroducing the split it was meant to close. So the canonicalisation walks up to the deepest existing ancestor, passes that through realpathSync, and appends the not-yet-created tail lexically. Both providers agree whether or not the directory exists, as long as the symlinked part of the path does — which is the case that causes the aliasing in the first place.

Three tests: the symlink alias (skipped rather than failed where symlink privilege is unavailable), genuinely different directories still separating, and before/after directory creation agreeing. Falsified by restoring the lexical form; probe file-store-id-not-canonical, and the existing file-backing-store-id-is-a-class-constant probe was re-pointed rather than dropped when its find string moved.

[MastaP]

@codex review

Both findings are fixed in f6e1a847 — please look at the fix itself, since it changes an identity that the whole PR depends on:

• backingStoreId for IndexedDB is now the database alone (prefix dropped), on the invariant that the id must name the unit of erasure — clear() with no prefix empties the whole object store. I chose widening the id over narrowing clear(); if that trade is wrong, the narrowing is the other half of your suggestion.
• the file provider canonicalises through the deepest existing ancestor (realpathSync throws on a fresh dataDir, which would have re-split the id across connect()).

Worth a specific look: whether widening the IndexedDB bucket now over-destroys anywhere — Sphere.clear() and Sphere.import() destroy every live Sphere in the bucket, so two prefixed wallets in one database now take each other down. My reasoning is that they already shared an erasure fate so this is fail-safe, but it is a behaviour change and I would rather have it challenged.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6e1a847cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Avoid treating sibling prefixes as an existing import target

When a Sphere is live under prefix q_ and Sphere.import() is called for an unused prefix p_ in the same database, this database-wide ID makes Sphere.liveOn(pStorage) return the q_ Sphere. The needsClear check in core/Sphere.ts therefore calls the unprefixed clear(), destroying the live wallet and deleting all of its data even though Sphere.exists(pStorage) is false and the import could safely write into the empty prefix. Keep database-wide teardown for an actual erase, but do not use that widened bucket to decide whether the target prefix has a wallet that needs erasing.

Useful? React with 👍 / 👎.

[MastaP]

You were right to push on this — it is the exact overreach I asked to be challenged, and I had not seen it. Fixed in ddd56c25.

Confirmed at core/Sphere.ts: needsClear = Sphere.liveOn(storage).length > 0 || await Sphere.exists(storage). With the widened bucket, liveOn(pStorage) returns the q_ Sphere, so an import into an empty prefix took the unprefixed clear() path, wiped the database and destroyed a live wallet nobody asked to touch — while Sphere.exists(pStorage) was false the whole time.

The distinction your comment draws is the one I had missed: the bucket names the unit of erasure, which is deliberately wider than the unit of existence. Correct for teardown-on-erase; wrong as an input to "is there something here to erase". So the decision is now the target's own wallet, plus a _storage === identity check that keeps the case the bucket was covering — a Sphere built on this very provider object, whose data may already be gone. When a clear does happen it still tears down the whole bucket, which is right.

Testing it needed a real IndexedDB: it is the only shape where the bucket is wider than existence (two providers on one file are one bucket and one existence). The scoping suite gains a fake-indexeddb case asserting the sibling stays live and keeps its data. Falsified by restoring the bucket-based condition; probe import-clears-on-the-widened-bucket.

[chatgpt-codex-connector]

Preserve the final symlink when identifying atomic file writes

When filePath itself is a symlink to another wallet file, canonicalPath() resolves it to the target and gives both providers the same lifecycle bucket, but save() writes filePath + '.tmp' and renames that temporary file over filePath; the rename replaces the symlink rather than modifying its target. Clearing through the symlink consequently destroys the Sphere for the target path even though the target wallet remains intact and the two paths diverge. Canonicalize the containing directory while retaining the final directory entry, rather than resolving the final symlink.

Useful? React with 👍 / 👎.

[MastaP]

Correct, and a genuinely subtle one — fixed in ddd56c25.

The asymmetry is exactly as you describe: a directory symlink is a true alias, because a rename inside it lands on the same file; a final-component symlink is not, because save() writes ${filePath}.tmp and renames it over filePath, replacing the link instead of writing through it. So the two paths agree until the first save and diverge permanently after it — and sharing a bucket would have let a clear through the link destroy the target's Sphere while the target wallet stayed intact. Worse than the aliasing I was fixing, since the data survives to make the stale Sphere look correct.

canonicalPath now applies to dataDir only and the final entry is kept verbatim. Two tests, opposite directions: a symlinked directory still shares an id, and two paths differing only in a final-component link do not. Falsified by restoring the whole-path canonicalisation; probe file-store-id-resolves-the-final-symlink.

Both rounds of yours landed on the same mechanism, which is worth saying plainly: the first found that the id disagreed with what clear() erases, and the second that I then let that widened id leak into decisions it had no business informing.