feat(adapter-store): add extend config hook for consumer-defined store methods

[jasperboerhof]

Implements Option 2, hardened per the #119 decision: a generic extend capability-injection hook on fs-adapter-store, where the sole ingest path is a response-backed retrieveInto — not the raw mutator tier.

What

Consumers pass extend?: (cap: ExtendCapabilities) => X to createAdapterStoreModule. The returned store is typed StoreModuleForAdapter<T, E, N> & X and carries the consumer's custom methods. The capability handed in is retrieveInto(endpoint, options?) — a GET that upserts the validated response into the store. The raw setById/deleteById never leave the factory through this door, so a non-HTTP write path (extend: (cap) => ({save: cap.setById}), or a wrapper around it) is structurally unexpressible, not policed by a runtime guard. The motivating kendo case stays a one-liner:

const usersStore = createAdapterStoreModule<User, Adapted<User>, NewAdapted<User>, {retrieveBySlug: (slug: string) => Promise<void>}>({
    domainName: 'users',
    adapter: resourceAdapter,
    httpService: http, storageService: storage, loadingService: loading,
    extend: ({retrieveInto}) => ({
        retrieveBySlug: (slug) => retrieveInto(`users/${slug}`),
    }),
});
await usersStore.retrieveBySlug('alice'); // fully typed, no cast

Why retrieveInto and not raw setById: extend's return value IS the public store surface, so a raw mutator could be re-exported as a non-HTTP write path that no runtime guard can distinguish from a legitimate fetch-then-set. Routing the only ingest through HTTP designs that out. (broadcast, by contrast, keeps validated raw handlers — its non-HTTP path is irreducible; extend's isn't. See #128.)

retrieveInto validates every response item (single or array) via isStorableItem — a non-object / non-integer-id item throws ExtendPayloadError rather than corrupting the keyspace.

Additive / backward-compatible

X extends object = {} — {} is the identity element for the & X intersection (A & {} ≡ A). An omitted extend is identical to prior behaviour. setById/deleteById are never reachable from the extend argument (asserted by a structural-impossibility test).

Version cascade (pre-1.0 discipline)

• fs-adapter-store 0.1.7 → 0.3.0 (rebased onto feat(adapter-store): validate broadcast payloads before they touch state #128, which took 0.2.0; new ExtendPayloadError export + behaviour change on malformed extend writes).
• fs-cached-adapter-store widens @script-development/fs-adapter-store to ^0.1.0 || ^0.2.0 || ^0.3.0 in both peerDependencies and devDependencies; own version → 0.2.2. No extend hook is added there — it intentionally narrows its surface.
• package-lock.json regenerated via npm install; verified every node_modules/@script-development/* resolves to the workspace (symlink → packages/*), no nested registry copies.

Rebased onto #128. The earlier esbuild override was lifted to (now-merged) chore PR #126, and this branch is rebased onto a green main that also carries #128 (broadcast payload validation). The npm audit gate is green. extend now mirrors #128's broadcast hardening — see the latest PR comment for the validating-wrapper change.

Verification (all 8 CI gates run locally, green)

Gate	Result
audit	clean (rebased onto green main; esbuild override merged via #126)
format:check	clean
lint (oxlint)	clean
build (tsdown)	complete
typecheck	clean — proves the no-cast type-safety contract
lint:pkg (publint + attw)	PASS — 11 packages + root
test:coverage	100% (533 tests, 21 files)
test:mutation (Stryker)	100% on adapter-store (0 survived — extend + broadcast validating wrappers, guards, and error call-sites all exercised)

New tests in the describe('extend (capability injection)') block: (1) extend called once at construction with a retrieveInto-only capability — and setById/deleteById asserted unreachable from the argument (the structural-impossibility guarantee); (2) extend-returned methods exposed and callable without a cast; (3) retrieveBySlug drives the store via retrieveInto; (4) retrieveInto validation matrix (non-object / missing-id / non-numeric / NaN / non-integer-float, single and array) throwing ExtendPayloadError without corrupting state; (5) options forwarding to getRequest.

Refs #119

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying fs-packages with    Cloudflare Pages

Latest commit:	556fd80
Status:	✅  Deploy successful!
Preview URL:	https://4ab097cf.fs-packages.pages.dev
Branch Preview URL:	https://feat-adapter-store-extend-ho.fs-packages.pages.dev

View logs

[Goosterhof]

⚠️ Concerns

0 blockers · 1 concern · 0 nits · 1 praise · 0 inline
CI: required check job is red — caps this below APPROVE.

Adds a generic extend capability-injection hook to createAdapterStoreModule, modelled on the existing broadcast slot, so consumers can attach store-level methods (e.g. kendo's retrieveBySlug) that close over the internal setById/deleteById tier with no as unknown as cast. The type-level mechanism is correct — X extends object = {}, & X identity for omitted extend, extend(storeModule) at packages/adapter-store/src/adapter-store.ts:94 references the legitimately-defined internal storeModule (:54), and the base rename is faithful. Backward-compatible, no public-surface change. The substance of the diff is clean.

Cross-file findings

The verification table is contradicted by CI, and the "green main" framing has gone stale. The PR body's table asserts "all 8 CI gates run locally" with the audit row green ("0 production vulns; the 3 dev-only vitepress highs are pre-existing on green main"). In CI the check job is red: the bare npm audit step (.github/workflows/ci.yml:18 — no --omit=dev) exited 1, which short-circuited the job — format:check, lint, build, validate:dist, typecheck, lint:pkg, test:coverage, test:mutation all show skipped in the run, so 7 of the 8 gates never executed in CI. The local run predates the failure.

Root cause is not this PR's code. main's check passed at 2026-06-12T17:11; this PR's ran at 2026-06-13T11:42; the esbuild advisory (GHSA-gv7w-rqvm-qjhr, Deno RCE) was published in that window and lands on a pre-existing transitive vitepress → vite → esbuild dev dependency that main's lockfile also carries. So the vuln is genuinely dev-only and not introduced here — but the gate is a hard, non---omit=dev npm audit, so it is now red on every branch including main's next push, and it will block merge until the audit posture is addressed (either bump vitepress, or scope the gate to production with npm audit --omit=dev). The body should stop claiming "green main"; main is one push away from the same red.

Two paths: (1) rebase onto a main that fixes the audit gate (separate chore — out of scope for this feature), or (2) fold an npm audit --omit=dev / --audit-level scoping change into this PR. Either way, don't merge over a red check on the assumption it's cosmetic — the skip means the real gates (typecheck, coverage, mutation) are unverified in CI, even though they passed locally.

Praise: the extend design earns its place — it generalizes the existing broadcast trust model into one narrow construction-time door rather than exposing setById on the public surface, and the test at tests/adapter-store.spec.ts asserts the no-cast contract by calling the extended method without a cast (the call site is the type assertion). That is the right way to test a type-safety claim.

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

[jasperboerhof]

Engineer · feedback claimed

• PR: feat(adapter-store): add extend config hook for consumer-defined store methods #120
• Branch: feat/adapter-store-extend-hook
• Scope: single MINOR (extend can shadow built-in store methods) — constrain X to forbid base keys (compile error)
• Started: 2026-06-13T13:26:29Z

[jasperboerhof]

Engineer · update

• PR: feat(adapter-store): add extend config hook for consumer-defined store methods #120
• Addressed: 1 MINOR (review-loop, restated across 2 threads) — extend key colliding with a built-in store method could silently shadow it via the {...base, ...extended} spread
• Fix: type-only — new ExtendShape<T,E,N,X> constraint maps any base-colliding key to never, so a collision is now a compile error; legit NEW keys (pingCount, retrieveBySlug) still compile. Runtime spread unchanged (coverage + mutation surface untouched). extend? JSDoc tightened; @ts-expect-error negative test added as the regression lock.
• Files: packages/adapter-store/src/{types.ts,adapter-store.ts} + tests/adapter-store.spec.ts (3 files, +55/-3)
• Commit: 2e82918
• Gates (worktree root): build PASS · typecheck PASS · test:coverage PASS (500 tests, 100%) · lint PASS · format:check PASS. test:mutation skipped (no runtime change).
• Replies: posted fix-evidence in both threads (PRRT_kwDOR0pEu86JUq50, PRRT_kwDOR0pEu86JUuD1)
• Gating caveat for the reviewer: this package's tsconfig.json has include: ['src'], so npm run typecheck (CI) does NOT type-check tests/. The @ts-expect-error lock is therefore documentary, not CI-enforced. Making it a true CI gate needs a one-line tsconfig include add (tests) — outside this fix's 3-file scope; flagging rather than silently expanding scope.

[jasperboerhof]

Engineer · feedback claimed

• PR: feat(adapter-store): add extend config hook for consumer-defined store methods #120
• Finding: 🔴 MAJOR our-badge (review-loop) — extend collision guard bypassed on 3-type-arg call
• Branch: feat/adapter-store-extend-hook
• Started: 2026-06-13T14:02:22Z

[jasperboerhof]

Engineer · update

• PR: feat(adapter-store): add extend config hook for consumer-defined store methods #120
• Addressed: 🔴 MAJOR our-badge (review-loop) — extend collision guard bypassed on the idiomatic 3-type-arg call → built-in silently shadowed at runtime
• Fix: construction-time runtime guard in adapter-store.ts throws new named ExtendKeyCollisionError on any base-key collision (holds on EVERY call form incl. <T,E,N> where X defaults to {}); Object.keys(base) (own enumerable) derives the base-key set — DRY, no hardcode. Kept ExtendShape compile-time constraint as the bonus on the explicit/inferred-X path.
• Honesty: JSDoc (types.ts) + docs/packages/adapter-store.md rewritten — collision throws at construction always, additionally a compile error when X is supplied/inferred. Stopped implying an unconditional compile error.
• Files (7, scope-locked): errors.ts, index.ts (barrel), adapter-store.ts (guard), types.ts (JSDoc), docs, adapter-store.spec.ts (+runtime-guard test, inferred path; wrapped existing feat(storage): add fs-storage package #4 collision test in toThrow), errors.spec.ts (+error-class test)
• Commit: 7e33b17 (was 2e82918)
• Gates: build pass · typecheck pass · test:coverage 100% (503 tests) · test:mutation adapter-store 100% (≥90 threshold; throw path mutants all killed) · lint pass · format:check pass
• Untouched: esbuild override, package versions, all unrelated files

[Goosterhof]

Verdict: REQUEST_CHANGES — the blocker here is governance, not code. The implementation is genuinely clean (careful F-bounded typing, a real runtime backstop behind the compile-time guard, honest tests, mutation-tested). On a private app I'd wave it through. This is the Armory, and a permanent generic API surface on a package every territory installs has to clear a higher bar. Three things stop it.

1. The design fork in #119 was never decided — this PR decides it unilaterally

#119 lays out four options and closes, verbatim: "Which direction fits the package best? Happy to PR whichever option we agree on." It explicitly flags the load-bearing question — "store-level custom methods become a thing. Whether that's a direction the package wants is the main question here." There are zero comments on #119; no agreement was recorded. This PR ships Option 2 — the most expansive of the four — as a finished thing. That inverts the order of operations for a shared package: a doctrine-level decision got made by the implementation rather than before it.

And Option 2 isn't the obvious pick. The motivating need is one method (retrieveBySlug) for one consumer (kendo). Against that, Option 1 (retrieveByKey(key: string)) is a first-class sibling of retrieveById — Laravel route-model binding with string keys is a framework concept with a legitimate claim to live in the package, it's narrow, typed, and adds no escape hatch. Option 2's selling point is "future-proof, no further package changes" — but that's the argument behind every escape hatch ever regretted, and the Armory's value is its constraint.

Please settle Option 1 vs Option 2 on #119 with the Commander before this merges. That's the actual open question, and #119 asked for exactly that decision. Everything below is conditional on Option 2 surviving that debate.

2. Unrelated scope is bundled in, and the PR body misdescribes the audit state

The diff carries an esbuild 0.27.7 → 0.28.1 override + full lockfile regeneration. The commit explains it (clears dev-only advisories via vitepress→vite→esbuild). The PR body never mentions it — and worse, the verification table says "audit | 0 production vulns (the 3 dev-only vitepress highs are pre-existing on green main)", framing those highs as left untouched while the PR actually fixes them. That row is stale and self-contradictory. Independent of merit, an audit fix that un-reds main on its own should be its own PR, not hidden inside a feature change with a body that misstates the audit. Split it out and correct the row. (Inline notes anchor the remaining points.)

[jasperboerhof]

§1 (design fork): Settled. Option 2 was agreed with the Commander over Mattermost; #119's open question is closed. I'll record that on #119.

#3 (the extend: (sm) => ({save: setById}) re-export thread): These two asks are mutually exclusive, and the PR has deliberately chosen its side.

The premise of extend is parity with broadcast's construction-time trust tier. broadcast and adapter are both handed the raw setById/deleteById — broadcast.subscribe({onUpdate: setById, onDelete: deleteById}) passes the bare mutators straight into consumer-controlled code. broadcast is "safe" only because it's a closed contract: its handlers are consumed internally, never returned. A consumer authoring a broadcast object can already stash setById and bypass HTTP today — the package doesn't prevent that, it just keeps it off the public surface.

extend is the same trust tier, open rather than closed. You can't have it both ways:

• Keep extend aligned with broadcast (hand over the raw mutator) → a renamed re-export is expressible, and no guard can stop it ({save: (i) => setById(i)} defeats any identity/type check).
• Guarantee "setById un-re-exportable in any kind" → extend must not receive the raw mutator → it stops being broadcast's model and becomes a bespoke, narrower capability that diverges from the rest of the store.

We're keeping the store intact: extend stays the same trust model as broadcast/adapter, by design. The marginal risk over broadcast is public exposure, not a new capability — a footgun, not a hole. Addressed where it belongs: the JSDoc and docs now state the open-vs-closed asymmetry explicitly, warn that a renamed re-export publishes the bare setter, and direct extend methods to wrap an HTTP fetch (as retrieveBySlug does). The collision guard stays for accidental name-shadowing.

§2 (esbuild override): Agreed — lifting the override + lockfile into a standalone chore PR so main goes green independently, and correcting the audit row in this PR's body.

#4 (collision guard keys on the current built-in set): Fair — adding a built-in later is a breaking change for extend-consumers. Adding a doc line; the mechanism stays.

[Goosterhof]

Design direction settled on #119 — see #119 (comment).

Short version: we're taking Option 2, hardened. The extend hook ships, but it receives a response-backed ingest primitive (retrieveInto, generalizing to requestInto) instead of the raw {setById, deleteById} tier — so extend: (sm) => ({ save: sm.setById }) (and its wrapper variant, which a runtime guard can't catch) becomes structurally unexpressible. The collision guard, version cascade, docs, and test structure here mostly carry over; the change is the capability object passed to extend. And please lift the esbuild override into its own PR. Full rationale on #119.

[Goosterhof]

Thanks for the thorough reply — and agreed on §2 (esbuild split) and #4 (doc line). On the re-export thread we're not aligned yet, and one factual correction up front: the Mattermost agreement settled Option 2 over Option 1 (hook vs. first-class method). It did not cover the raw-mutator-vs-constrained-capability sub-question — that's the part still open, and it's what #119's "Option 2, hardened" decision lands on. So this isn't re-litigating a closed call; it's the one piece that was never decided.

You're right about one thing, and I'll say it plainly: the bypass-HTTP capability already exists via broadcast — a consumer-authored subscribe receives the bare setById/deleteById and can stash them. (The adapter precedent is weaker — in practice adapter: resourceAdapter is package-shipped, not consumer code — but broadcast is a clean example.) So extend is not opening a brand-new hole. Granted.

But "same trust tier, so keep the raw mutator" is a false dilemma. broadcast (closed — handlers consumed internally, never returned) and extend (open — its return is the public surface) already differ on the one axis that matters: exposure. When the exposure model differs, the raw tier that is safe for a closed contract is precisely what's unsafe for an open one. Handing extend a capability matched to its exposure semantics isn't "bespoke divergence from the store" — it's correctness. "Consistency with broadcast" is surface-level when the two already diverge where it counts.

And the blast radius isn't symmetric. With broadcast, a leaked mutator is reachable only by code the author deliberately wires elsewhere. With extend, {save: setById} sits on the public store object every component in the app holds. Same footgun, app-wide barrel. "Footgun not hole" undersells that.

The concrete question that decides this: name one legitimate extend use case that a response-backed ingest primitive can't express.

extend?: (cap: ExtendCapabilities<T>) => X;
type ExtendCapabilities<T extends Item> = {
    retrieveInto: (endpoint: string, options?: AxiosRequestConfig) => Promise<void>;
    // generalizes to requestInto({method, url, data}, map?) for write-verb actions
};

extend: ({retrieveInto}) => ({
    retrieveBySlug: (slug) => retrieveInto(`${domainName}/${slug}`),
})

setById/deleteById never leave the factory, so {save: setById} and its wrapper variant become unexpressible — not guarded-against, structurally impossible. Custom endpoints, any verb, cross-store coordination, derived methods all stay expressible (extend still closes over the consumer's whole module scope). The only thing this removes versus raw setById is writing store state with no server response behind it — which is exactly the abuse. If there's a legitimate case it blocks, I'll genuinely reconsider; "narrower" in the abstract isn't a cost until it names one.

Holding REQUEST_CHANGES on this point. The collision guard stays useful regardless (accidental shadowing), and your esbuild + #4 concessions are already wins — happy to fast-track the esbuild PR.

[Goosterhof]

Esbuild split is up as #127 (off latest main, audit now 0 vulns). Once it merges, the override + lockfile churn drops out of this PR on rebase, and the audit row in the body can be corrected to reflect that the dev-only highs are resolved, not pre-existing. Leaves #120 to be purely the extend design.

[Goosterhof]

Concrete follow-up on the "same trust tier as broadcast" point — we hardened broadcast itself in #128, and doing so makes the asymmetry precise rather than rhetorical.

#128 wraps the broadcast handlers in validation (reject non-object / non-numeric-id payloads before they touch state) and stops handing the bare setById/deleteById out of the factory. But note what it could not do: it cannot make the bypass-HTTP capability un-acquirable. broadcast's entire reason to exist is to apply server-pushed events without an HTTP round-trip, so any "push an item into the store" handler is irreducibly a non-HTTP write path. A consumer who wants to wrap and re-expose it still can. We documented that as a discipline ("don't re-export the handlers") because it's the best available — the capability can't be removed without removing the feature.

That's exactly the difference with extend:

• broadcast accepts the raw-mutator tier because its purpose requires the non-HTTP path. The leak and the feature are the same mechanism.
• extend does not require the non-HTTP path — extend methods should fetch. So handing extend the raw tier imports broadcast's irreducible risk for no functional reason, and unlike broadcast it can be designed out (route through a server-backed ingest primitive).

So "parity with broadcast" actually argues for constraining extend, not against it: we accept the irreducible risk where the feature demands it (broadcast) and decline to import it where it doesn't (extend). The standing question still stands — name one legitimate extend use case retrieveInto/requestInto can't express. If there's none, the raw tier buys only the abuse.

(#128 is independent of this PR and merge-ready on its own; heads-up that it and #120 both target fs-adapter-store 0.2.0, so whichever lands first, the other rebases.)

[Goosterhof]

🚫 Blockers

Round 2 (round 1: COMMENT prsweep:6c9f535; an interim CHANGES_REQUESTED raised the #119-design and esbuild-scope blockers).
1 blocker · 0 concerns · 0 nits · 2 praise · 1 inline
CI: required check is red — stale (esbuild advisory, fixed by #126 which merged 10:00Z); a rebase clears it. mergeStateStatus: DIRTY / CONFLICTING confirms the branch predates the #126 merge.

Round-2 tally — the interim blockers are resolved: esbuild override lifted to #126 (merged 2026-06-15T10:00:23Z) and reverted here (61e6b3da); the audit row is corrected; the ally MAJOR 3-arg-bypass got a real runtime backstop (7e33b179); trust-model wording and the forward-compat hazard are now documented honestly (7ab4904e). The collision-guard work and tests are genuinely good. But the round-2 base moved: the #119 design question this PR was blocked on has been decided, and the decision changes the contract this PR ships.

The implementation is clean — F-bounded ExtendShape, runtime backstop behind the compile guard, mutation-tested. It does not matter, because it implements the wrong contract.

Cross-file findings

Blocker — the #119 decision is "Option 2 hardened", and this PR ships un-hardened Option 2. The Commander's decision on #119 (2026-06-15T09:01, two hours before this head) is explicit: extend ships, but it must receive a response-backed ingest primitive (ExtendCapabilities<T> with retrieveInto), not the raw setById/deleteById tier. The decision spells out exactly why a runtime guard is insufficient and names this PR by number:

• The hazard isn't {save: sm.setById} (a guard catches reference identity) — it's the one-line wrapper {save: (item) => sm.setById(item)}, which is a new function indistinguishable at runtime from a legitimate fetch-then-set. Quoting #119: "A guard is real protection against accidents and theater against intent."
• The decided fix is structural, not policed: extend?: (cap: ExtendCapabilities<T>) => X where the only ingest path is retrieveInto(endpoint, options?) => Promise<void>. setById/deleteById never leave the factory, so the raw-mutator re-export becomes unexpressible, not guarded-against.
• This PR's head still does the opposite — extend?: (storeModule: AdapterStoreModule<T>) => X hands the full internal module (anchored inline at packages/adapter-store/src/types.ts:113), and adapter-store.ts:102 invokes it with the raw storeModule. The motivating retrieveBySlug itself calls bare setById in this PR's docs and tests — exactly the surface the decision removes.

Regression-against-decision, not against prior code. #119 step 1: "Rework extend to pass ExtendCapabilities<T> (start with retrieveInto)... Widen the store's Pick<HttpService, ...> only as far as the shipped primitives require." Steps 2–3 confirm the collision guard, version cascade, docs, and test structure carry over — so this is a rework of the capability surface, not a rewrite. The author's own thread reply (3412235728) argued for keeping the raw tier and documenting the footgun; the #119 decision overrides that choice on ISO 27001 / NEN 7510 grounds (the HTTP path is where authz/audit live for emmie / ublgenie / codebook).

Fix: implement #119 as written — ExtendCapabilities<T> with retrieveInto as the sole ingest path; keep the collision guard, cascade, and test scaffold. Add a test asserting setById/deleteById are not reachable from the extend callback argument (the structural-impossibility assertion the decision is buying).

Praise: two real wins survive the rework. The @ts-expect-error-locked compile guard plus the construction-time ExtendKeyCollisionError backstop is the right belt-and-suspenders, and the forward-compat note (a new built-in is a breaking change for extend-consumers) is an honest call-out most authors miss. The four-option framing in #119 is what made the decision possible — that analysis carries weight.

Findings (detail inline)

• packages/adapter-store/src/types.ts:113 — extend signature passes the raw module, not ExtendCapabilities<T> (the #119-decided contract).

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

[jasperboerhof]

Rebased onto #128 + extend now mirrors the broadcast hardening

Force-pushed a rebase onto main (which now carries #128, the broadcast payload-validation work). Two substantive changes beyond a mechanical rebase:

1. Version cascade (the 0.2.0 collision resolved in #128's favour)

• fs-adapter-store 0.2.0 → 0.3.0 (feat(adapter-store): validate broadcast payloads before they touch state #128 took 0.2.0; new ExtendPayloadError export + behaviour change on malformed extend writes).
• fs-cached-adapter-store widens the fs-adapter-store peer/dev range to add || ^0.3.0, patch-bumps 0.2.1 → 0.2.2. Lockfile regenerated — workspace links intact, 0 registry copies.

2. extend now receives validating wrappers, not raw mutators — mirroring #128

#128 moved broadcast off the raw setById/deleteById tier onto validating wrappers. That left extend as the only construction hook still handed the bare mutators, with the identical keyspace-corruption vector ({id: NaN} → state.value[NaN] → "NaN" key, undeletable). The "extend = the same trust tier as broadcast" justification was about to become false.

So extend is brought to parity:

• extend gets validating wrappers: setById requires an object with an integer id, deleteById an integer id; a malformed payload throws the new ExtendPayloadError instead of mutating.
• The integer check is lifted into a shared isStorableItem predicate used by both the broadcast wrappers (DRY with feat(adapter-store): validate broadcast payloads before they touch state #128's inline check) and the extend wrappers. Raw mutators never leave the factory through either door — only adapter (trusted, package-internal) keeps the raw tier.
• Docs/JSDoc corrected: extend is validating wrappers like broadcast, not "the raw tier." The residual open-vs-closed asymmetry stays documented — a re-exported wrapper still permits non-HTTP writes of well-formed data, so validation stops corruption, not deliberate re-export; the footgun note remains.

Gates (all 8 green locally): build · typecheck · lint · format · lint:pkg (11 pkgs + root) · coverage 100% (533 tests) · mutation 100% on adapter-store (incl. the new guards; also killed 2 pre-existing broadcast throw-literal survivors from #128 by asserting the error message at the call site).

[Goosterhof]

🚫 Blockers

Round 3 (round 1: COMMENT prsweep:6c9f535; interim CHANGES_REQUESTED raised #119-design + esbuild-scope; round 2: prsweep:61e6b3da raised the #119-decision blocker).
1 blocker · 0 concerns · 0 nits · 1 praise · 1 inline
CI: required check is red — mergeStateStatus: BEHIND, branch trails main; the esbuild advisory was cleared by #126, a rebase re-runs green. Stale, not this PR's code.

Round-3 tally — one new commit since round 2 (ef2cd045, the feat!: validate extend payloads hardening). It is real hardening — setById/deleteById wrappers now throw ExtendPayloadError on a malformed payload, mutation-tested. But it hardens the wrong contract. The round-2 blocker is carried, unaddressed: this head still ships the raw-mutator extend door the #119 decision rejected. Adding payload validation to the raw tier is not the same as removing the raw tier.

Cross-file findings

Blocker (carried from round 2) — head still ships un-hardened Option 2, not the #119-decided ExtendCapabilities<T>. Verified directly against refs/pull/120/head:

• packages/adapter-store/src/types.ts:127 — extend?: (storeModule: AdapterStoreModule<T>) => X. Still the full internal module.
• packages/adapter-store/src/adapter-store.ts:152 — extend(extendStoreModule), where extendStoreModule (:137) exposes setById + deleteById to the callback.
• The #119 decision (2026-06-15T09:01) names this PR and is explicit: extend?: (cap: ExtendCapabilities<T>) => X where the only ingest path is retrieveInto(endpoint, options?) => Promise<void>. setById/deleteById never leave the factory, so the raw-mutator re-export becomes unexpressible, not policed.

What ef2cd045 changed is orthogonal to the blocker. The hazard the decision removes is {save: (item) => sm.setById(item)} — a new function indistinguishable at runtime from a legitimate fetch-then-set. Payload validation does not touch that: a well-formed payload passes the new wrapper and reaches state through a non-HTTP write path all the same. Quoting #119: "A guard is real protection against accidents and theater against intent." The decision buys structural impossibility; this head still buys a guard.

Fix is unchanged from round 2: implement #119 as written — ExtendCapabilities<T> with retrieveInto as the sole ingest path; widen the store's Pick<HttpService, ...> only as far as retrieveInto requires; keep the collision guard, version cascade, and test scaffold (the decision confirms they carry over). Add a test asserting setById/deleteById are not reachable from the extend callback argument — that structural-impossibility assertion is the whole point of the rework.

Praise: the payload-validation hardening in ef2cd045 is well-built on its own terms — the shared isStorableItem predicate (adapter-store.ts:51) factored out of the duplicated broadcast check, the integer-keyspace rationale captured in the comment, and the it.each table covering NaN / Infinity / non-integer-float / non-object all exercised. None of it is wasted; most of it survives the rework onto retrieveInto.

Findings (detail inline)

• packages/adapter-store/src/types.ts:127 — extend still passes the raw AdapterStoreModule<T>, not the #119-decided ExtendCapabilities<T>.

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

[Goosterhof]

Jasper — the rework is well-built and most of it stays. But it ships the wrong contract, and the contract isn't open anymore.

I decided this on #119 (09:01): extend ships as Option 2 hardened — it receives ExtendCapabilities<T> with retrieveInto as the sole ingest path. setById/deleteById do not leave the factory. The point is that extend: ({setById}) => ({save: setById}) becomes unexpressible, not documented-against. Your own JSDoc states the leak still works for well-formed data — that's exactly the line the decision removes.

Why this isn't negotiable: for emmie / ublgenie / codebook the HTTP path is where authz and audit live. "Document the footgun" is the weakest rung of our enforcement ladder; a shared package gets the control pushed to structural, not parked in a doc comment. Validation closes corruption; it does nothing about a deliberate non-HTTP write path.

"Parity with broadcast" doesn't carry it: broadcast is a closed contract (handlers never returned), extend's return is the public surface. Same validation, different trust tier — that asymmetry is the whole reason #128 validated broadcast but didn't try to design its leak out, and the reason extend's leak can be designed out.

None of your work is wasted. isStorableItem, ExtendPayloadError, the collision guard, the version cascade, the tests — they all move inside retrieveInto (which does the fetch, then a validated setById internally). It's a rework of the capability surface, not a rewrite; #119 step 1 spells it out.

Concretely: extend?: (cap: ExtendCapabilities<T>) => X, retrieveInto(endpoint, options?) => Promise<void> as the only door, widen the store's Pick<HttpService,...> only as far as that needs, and add a test asserting setById/deleteById are unreachable from the extend argument. If there's a real use case retrieveInto can't express, name it and we revisit — that question's been open three rounds.

[jasperboerhof]

@Goosterhof — done in d07b27f. Implemented #119 as written: extend receives ExtendCapabilities with retrieveInto(endpoint, options?) as the sole ingest path; the raw setById/deleteById no longer leave the factory through the extend door, so {save: cap.setById} (and the wrapper variant) is structurally unexpressible rather than guarded.

• retrieveInto GETs the endpoint and upserts the validated response (single item or array) via the internal setById; each item must be an object with an integer id or ExtendPayloadError is thrown.
• Store Pick<HttpService,...> unchanged — retrieveInto only needs getRequest. Options type derived via Parameters<HttpService['getRequest']>[1] (no direct axios import).
• Added the structural-impossibility test: setById/deleteById are not reachable from the extend argument.
• Carried over as you predicted: isStorableItem, ExtendPayloadError (reworded for the retrieveInto context), the collision guard, the 0.3.0 version cascade, and the test scaffold.

No use case that retrieveInto can't express has come up — the motivating kendo retrieveBySlug is (slug) => retrieveInto(${domainName}/${slug}). If a write-verb case lands, that's the requestInto follow-up #119 anticipated; not building it ahead of need.

All 8 gates green locally: build · typecheck · lint · format · lint:pkg · coverage 100% (531 tests) · mutation 100% (adapter-store, 0 survived).

[Goosterhof]

✅ Approved — ships the decided contract

Clearing my standing CHANGES_REQUESTED. d07b27f implements #119 as written, and the adoption is clean.

Resolved:

• extend?: (cap: ExtendCapabilities) => X, and ExtendCapabilities exposes only retrieveInto. setById/deleteById are not reachable from the callback, so {save: cap.setById} (and the wrapper variant) is unexpressible, not guarded — the structural property the decision was buying.
• The structural-impossibility test is there ('setById' in cap / 'deleteById' in cap both false) — that assertion is the whole point, good that it's pinned.
• retrieveInto GETs, validates each item (isStorableItem, integer-id), upserts via the internal setById, single-or-array, ExtendPayloadError on a malformed response. Pick<HttpService> unchanged; options typed via Parameters<...>, no direct axios import.
• Carried over exactly as anticipated: isStorableItem, the collision guard, the 0.3.0 cascade, the test scaffold. None of the earlier hardening was wasted — it moved inside the ingest boundary.

The use-case question that ran three rounds is settled: retrieveBySlug is (slug) => retrieveInto(...) over the domain endpoint, and the write-verb requestInto is correctly deferred until a real case lands rather than built ahead of need.

One non-blocker before merge: the branch is BEHIND main (a Dependabot happy-dom bump merged after it) — MERGEABLE, but rebase to current so CI runs on the merge result.

Verified locally on d07b27f: build, typecheck, lint, format, audit all green; check green on CI. Good work turning this around.

[jasperboerhof]

Review Loop · 8/10 · PASS — 🟡 1

fs-packages #120 · AC anchor: none — no Kendo issue / plan dir / PR AC section (acceptance pass skipped); reviewed against fs-packages root CLAUDE.md + consumer-contract discipline · head 556fd80fd4

Tip

1 finding(s) posted inline — see the review comments on the changed lines.

Action

merge-ready

[jasperboerhof]

🟡 MINOR — retrieveInto array ingest is non-atomic — valid items before a malformed one are committed before the throw

The validate-and-upsert loop calls setById on each valid item as it goes, so for a partly-malformed array (e.g. [{id:1},{id:NaN}]) item 1 is already persisted when item 2 throws ExtendPayloadError.

Fix: Either (a) validate all items first, then upsert (pre-pass items.every(isStorableItem) before any setById) to make the array ingest all-or-nothing, or (b) soften the docs/test name to "rejects the malformed item" and add an assertion documenting that earlier valid items are committed. Pick one so code, docs, and test agree.

Evidence and reasoning

The docs and the array-rejection test name both promise "without corrupting state", but the array path leaves the earlier valid items in the store.

adapter-store.ts:144-149 — for (const item of items) { if (!isStorableItem(item)) throw new ExtendPayloadError(...); setById(item); }. setById (line 148) mutates state + persists per item, before the next item is validated. The single-item path is fine — a lone bad item never lands, and the single-item tests do assert store.getAll.value === []. But the array test rejects a retrieveInto array response if any item is malformed, without corrupting state (adapter-store.spec.ts:561) only asserts .rejects.toThrow(ExtendPayloadError) — it does NOT assert the store is empty / item 1 was rolled back, so the partial-upsert behaviour is unverified and the test name overclaims. docs/packages/adapter-store.md:39 makes the same "rather than corrupting state" claim. The keyspace invariant (NaN/non-integer id never lands) IS upheld — only valid items are committed — so this is prose/test drift + non-atomicity, not data corruption; hence MINOR.