feat(adapter-store): validate broadcast payloads before they touch state

[Goosterhof]

The broadcast half of the trust-surface review on #120 / #119.

Problem

broadcast applies server-pushed events to the store without an HTTP round-trip — that non-HTTP path is the whole point. But the bare internal setById/deleteById were handed straight to the consumer-authored subscribe, and whatever the channel emits landed in frozen state unvalidated:

broadcast?.subscribe({onUpdate: setById, onDelete: deleteById});
// setById: state.value = {...state.value, [item.id]: Object.freeze(item)}  — no checks

A malformed event — {id: undefined}, a non-object, a non-numeric id — silently corrupts the store (entry keyed under "undefined"/"null"). For care-data / compliance territories, untrusted-channel input reaching state unvalidated is the concern.

Change

The store passes validating wrappers instead of the raw mutators:

• onUpdate requires an object with a numeric id
• onDelete requires a numeric id
• a bad payload throws the new BroadcastPayloadError rather than mutating

Two consequences beyond validation: the raw setById/deleteById no longer leave the factory (only wrappers do), and the closed-contract discipline (don't re-export the handlers onto your own public surface) is now documented in the type JSDoc and package docs. AdapterStoreBroadcast<T>'s public shape is unchanged — this is a non-breaking type surface; only the runtime behaviour on malformed payloads changes (was: silent corruption; now: throw).

Why not the same fix as extend (#120)?

extend's leak is closeable because extend methods should fetch — route them through HTTP. broadcast's leak is irreducible: its reason to exist is a non-HTTP write path, so no constraint removes the bypass capability without removing the feature. The available lever is payload validation, not capability constraint. (That asymmetry is itself why extend should be constrained where broadcast can't be.)

Design choice

On a malformed payload the handler throws (Commander's call) — a missing/non-numeric id is a contract violation, not a transient, and the item can't be stored correctly anyway. Consumers wiring a socket should guard their handler. (Alternative considered: skip + console.warn for realtime resilience — rejected as it silently drops malformed updates.)

Version cascade (pre-1.0)

• fs-adapter-store 0.1.7 → 0.2.0 (new public export BroadcastPayloadError + behaviour change).
• fs-cached-adapter-store widens @script-development/fs-adapter-store ^0.1.0 → ^0.1.0 || ^0.2.0 (peer + dev) and patch-bumps 0.2.0 → 0.2.1.
• Lockfile regenerated; every node_modules/@script-development/* resolves to the workspace, 0 registry copies.

Coordination with #120: both PRs target fs-adapter-store 0.2.0. Whichever merges first wins; the other rebases (the extend feature would then become 0.3.0).

Verification (local; vitest/coverage/mutation are CI-gated on this box)

Gate	Result
build (tsdown)	complete
typecheck (tsc)	clean
lint (oxlint)	clean
format:check (oxfmt)	clean
audit	⚠️ inherits main's 3 dev-only highs (esbuild via vitepress) — fixed independently by #127; clears on rebase once that merges

New tests: BroadcastPayloadError unit coverage, plus broadcast-integration tests asserting onUpdate/onDelete reject malformed payloads (one case per validation clause, incl. undefined to isolate the object-type guard for mutation testing) without corrupting state, and that well-formed payloads still pass through the wrapper.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying fs-packages with    Cloudflare Pages

Latest commit:	7e0484e
Status:	✅  Deploy successful!
Preview URL:	https://e36c21e8.fs-packages.pages.dev
Branch Preview URL:	https://fix-broadcast-payload-valida.fs-packages.pages.dev

View logs

[Goosterhof]

⚠️ Concerns

0 blockers · 2 concerns · 0 nits · 1 praise · 1 inline · round 1

This PR swaps the bare internal setById/deleteById handed to broadcast.subscribe for validating wrappers, so a malformed server-pushed payload throws the new BroadcastPayloadError instead of silently corrupting frozen state. The thesis is sound and the test matrix is thorough (one case per validation clause, including the undefined-isolates-object-guard case, plus the well-formed pass-through on both handlers and the errors.spec message assertions). The non-breaking type-surface claim checks out — AdapterStoreBroadcast<T>'s public shape is unchanged; only runtime behaviour on bad payloads shifts.

The body's scope claims are honest: the version cascade matches the diff (fs-adapter-store 0.1.7→0.2.0, fs-cached-adapter-store peer/dev widened to ^0.1.0 || ^0.2.0 + patch to 0.2.1), and the #120 coordination note is declared. No description-vs-diff mismatch.

Praise — the JSDoc + docs framing of broadcast as a closed contract ("don't re-export the handlers onto your own public surface") is the load-bearing call here. The irreducible-leak argument in the body (broadcast's reason to exist is a non-HTTP write path, so validation is the only available lever, unlike extend's closeable leak in #120) is the right distinction and worth documenting where you put it.

Cross-file findings

• Concern — check (npm audit) required gate is RED, unrelated to this diff. Run npm audit fails on 3 dev-only high advisories: esbuild 0.17.0–0.28.0 via vitepress→vite (run 27536816384). I verified this is inherited from main and not introduced by this PR's hunks, and that the fix (#127, chore(deps): override esbuild to ^0.28.1) is OPEN as of this review — so the body's "clears on rebase once that merges" is accurate. This caps the verdict at COMMENT (a red required check forbids approve-worthy) but is not a blocker against this PR's code. Merge order: land #127 (or #120, per your coordination note) first, rebase, gate goes green.

Findings (detail inline)

• Concern — packages/adapter-store/src/adapter-store.ts:64 — typeof === 'number' admits NaN / non-integer ids; the guard's own "can't corrupt state" promise has a residual hole.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[Goosterhof]

Rebased onto main now that #126 (the esbuild override) merged — the audit gate that was inherited-red is now green, and the lockfile carries both the esbuild override and this PR's version bumps (workspace links intact, 0 registry copies).

Round-1 concern addressed: the integer-id hole (NaN/Infinity/non-integer float passing typeof === 'number' yet corrupting the keyspace) is closed in b4dacc5 via Number.isInteger, with tests + wording updated. Both gates green locally (build, typecheck, lint, format).

[jasperboerhof]

Review Loop · 9/10 · PASS

fs-packages #128 · AC anchor: none (no issue/plan/PR-AC; reviewed against package conventions + consumer contract) · head b4dacc5b14

Tip

No findings — clean against the review checklist.

Action

merge-ready

[jasperboerhof]

Auto-approved — review-loop verdict PASS, CI green, no human blocker, no open MAJOR+ threads. See the verdict comment + inline notes.

[Goosterhof]

✅ Approve-worthy

0 blockers · 0 concerns · 0 nits · 1 praise · 0 inline · round 2

Round 1 (f8156264) raised 2 concerns — both cleared. The typeof === 'number' hole is closed: both guards now use Number.isInteger(...) at adapter-store.ts:67,73, and the test matrix gained {id: NaN} / {id: 1.5} on onUpdate and NaN / 1.5 on onDelete, each asserting toThrow(BroadcastPayloadError) with no state mutation. Error message, JSDoc, and docs were tightened "numeric" → "integer" to match the actual contract. The round-1 audit gate that capped the verdict at COMMENT has cleared — check is green (the rebased HEAD 7e0484e1 carries the esbuild override), and jasperboerhof has approved.

I re-walked the corruption trace against live HEAD: setById keys under [item.id] and deleteById filters via Number(key) !== id (adapter-store.ts:38,45), so a NaN id keyed as "NaN" would have been undeletable. Number.isInteger now rejects it before either mutator runs — the trace is closed.

Beyond-the-diff: the broadcast path has a single producer (createAdapterStoreModule), and the validating wrappers sit at that one chokepoint. No other broadcast consumer exists in the repo, and fs-cached-adapter-store wraps the same factory, so it inherits the validation. The new explicit-validation invariant holds on every consumer, conforming to ADR-0019's explicit-hydration line — untrusted channel input is validated before touching frozen state rather than mass-assigned blind.

Praise — the integer-vs-typeof number distinction is the load-bearing fix: the original guard would have admitted a class of numbers (NaN / Infinity / non-integer floats) that demonstrably corrupt the keyspace, and the new code documents why in the inline comment with the exact Number("NaN") !== NaN mechanism. That's the right depth for a guard whose stated contract is "a malformed broadcast cannot corrupt store state."

No remaining findings. Verdict is approve-worthy; the event is COMMENT only because the author is the war-room runner.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.