26 enhanced kv store key value experience

[corrideat]

Closes #26

KV-REVAMPED.md KV-REVAMPED.md
KV-REVAMPED.md contains the spec used (which this PR should conform to).

AI disclosure

GPT-5.5, Opus 4.7 and GLM 5.1 were used in generating code for this PR. GPT-5.5, Opus 4.7 & GLM 5.2 were used for reviews.

[corrideat]

/crush_fast

---

AI review started.

[taoeffect]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.2

Click to expand review

I'll review this PR thoroughly. Let me start by exploring the codebase structure and reading the key files.Now let me look at the constants, types, and errors files:Now let me look at the types file and the chelonia.ts kv/set and kv/get changes:Now let me look at the chelonia.ts kv/set implementation in detail:Let me look at the events.ts, index.ts and the chelonia.ts connect function in more detail:Now let me look at the pubsub index to check the PUBSUB_RECONNECTION_SUCCEEDED constant and the kvFilter:Let me search for some key patterns to understand the code better:Let me check a few more specific things:Now let me check one more specific area — the duplicated validation logic in update:Let me verify the type vs implementation mismatch for currentData:Now I have a thorough understanding of the changes. Let me compile my review.

---

Code Review: KV Slots API

1. 🔴 DRY violation: duplicated reducer-output validation in chelonia/kv/update

• Addressed
• Dismissed

The reducer-output validation logic (symbol check, null/undefined check, schema parse / assertJsonShape) is copy-pasted between the first-attempt path and the onconflict retry path. ~50 lines of near-identical code:

First attempt — src/kv.ts:2497-2549:

      let reducerOut: unknown
      try {
        reducerOut = reducer(seedValue)
      } catch (e) {
        throw new ChelErrorKvUpdateInvalid(
          `[chelonia/kv] update: ${contractID}::${key} reducer threw`,
          { cause: e }
        )
      }
      if (typeof reducerOut === 'symbol') {
        if (reducerOut === KV_NOOP) return undefined
        throw new ChelErrorKvUpdateInvalid(...)
      }
      if (reducerOut === null || reducerOut === undefined) {
        throw new ChelErrorKvUpdateInvalid(...)
      }
      let nextValue: JSONType = reducerOut as JSONType
      if (slot.schema) {
        try {
          nextValue = parseSyncSlotValue(slot, nextValue, `update ${contractID}::${key}`)
        } catch (e) { throw new ChelErrorKvValidation(...) }
      } else {
        try {
          nextValue = assertJsonShape(nextValue, `update ${contractID}::${key}`)
        } catch (e) { throw new ChelErrorKvValidation(...) }
      }

Onconflict retry — src/kv.ts:2617-2665:

        let retried: unknown
        try {
          retried = reducer(basis)
        } catch (e) {
          throw new ChelErrorKvUpdateInvalid(
            `[chelonia/kv] update: ${contractID}::${key} reducer threw on retry`,
            { cause: e }
          )
        }
        if (typeof retried === 'symbol') {
          if (retried === KV_NOOP) { throw new KvNoopAbort() }
          throw new ChelErrorKvUpdateInvalid(...)
        }
        if (retried === null || retried === undefined) {
          throw new ChelErrorKvUpdateInvalid(...)
        }
        let validated: JSONType = retried as JSONType
        if (slot.schema) {
          try {
            validated = parseSyncSlotValue(slot, retried, `update onconflict retry ${contractID}::${key}`)
          } catch (e) { throw new ChelErrorKvValidation(...) }
        } else {
          try {
            validated = assertJsonShape(validated, `update onconflict retry ${contractID}::${key}`)
          } catch (e) { throw new ChelErrorKvValidation(...) }
        }

Suggestion: Extract a shared helper:

function validateReducerOutput (
  slot: SlotDefinition,
  output: unknown,
  contractID: string,
  key: string,
  context: string
): { noop: true } | { noop: false; value: JSONType } {
  if (typeof output === 'symbol') {
    if (output === KV_NOOP) return { noop: true }
    throw new ChelErrorKvUpdateInvalid(
      `[chelonia/kv] update: ${contractID}::${key} reducer returned ` +
      `an unexpected symbol${context ? ` ${context}` : ''}; use KV_NOOP to abort`
    )
  }
  if (output === null || output === undefined) {
    throw new ChelErrorKvUpdateInvalid(
      `[chelonia/kv] update: ${contractID}::${key} reducer returned ` +
      `${String(output)}${context ? ` ${context}` : ''}; use chelonia/kv/clear or KV_NOOP instead`
    )
  }
  const where = `update${context ? ` ${context}` : ''} ${contractID}::${key}`
  const value = slot.schema
    ? parseSyncSlotValue(slot, output, where)
    : assertJsonShape(output, where)
  return { noop: false, value: value as JSONType }
}

Then both call sites become:

// First attempt
let reducerOut: unknown
try { reducerOut = reducer(seedValue) }
catch (e) { throw new ChelErrorKvUpdateInvalid(`... reducer threw`, { cause: e }) }
const result = validateReducerOutput(slot, reducerOut, contractID, key, '')
if (result.noop) return undefined
let nextValue = result.value

// Onconflict retry
let retried: unknown
try { retried = reducer(basis) }
catch (e) { throw new ChelErrorKvUpdateInvalid(`... reducer threw on retry`, { cause: e }) }
const retryResult = validateReducerOutput(slot, retried, contractID, key, 'on retry')
if (retryResult.noop) throw new KvNoopAbort()
nextValue = retryResult.value

Note: parseSyncSlotValue and assertJsonShape already throw ChelErrorKvValidation internally, so the try/catch wrappers around them in the current code are only for re-wrapping the message. If the where string is descriptive enough, the inner try/catch can be dropped entirely.

---

2. 🟡 Documentation bug: CHELONIA_KV_UPDATED event value for remote clear

• Addressed
• Dismissed

docs/api.md:369 states:

A cleared value emits value: undefined when discovered through a 404 / missing-key load, but emits the cloned default when received as a remote pubsub null frame

But the code emits value: undefined for both cases. In _handleRemote (src/kv.ts:2233-2240):

    // A remote clear (unwrapped === null) stores `undefined` as the
    // canonical 'non-init' mirror `value` (§4.3/§4.5), matching local
    // `clear` and the 404 branch; `onUpdate` below still receives the
    // resolved default via `nextValue`.
    const mirrorValue = unwrapped === null ? undefined : nextValue
    this.config.reactiveSet(entry, 'value', mirrorValue)
    this.config.reactiveSet(entry, 'etag', remoteEtag)
    // ...
    if (!silent) {
      sbp('okTurtles.events/emit', CHELONIA_KV_UPDATED, {
        // ...
        value: cloneForEmit(mirrorValue),   // ← undefined for remote null

onUpdate does receive the cloned default (cloneForEmit(nextValue) at src/kv.ts:2262), but the event payload carries undefined. The documentation conflates the two. This also contradicts the local clear path (src/kv.ts:3073), which emits value: undefined.

Suggestion: Fix the docs to match the code — the event value is always undefined for a cleared slot; only onUpdate receives the cloned default:

A cleared value always emits `value: undefined` in the event payload (whether
discovered via 404, remote pubsub `null` frame, or local `clear`); `onUpdate`
receives the cloned default. Both read back as the default through
`chelonia/kv/read`.

---

3. 🟡 Type mismatch: currentData is a throwing getter but typed as a plain value

• Addressed
• Dismissed

ChelKvOnConflictCallback in src/types.ts:665-673 types currentData as a plain property:

export type ChelKvOnConflictCallback = (args: {
  contractID: string;
  key: string;
  failedData?: JSONType;
  status: number;
  etag: string | null | undefined;
  currentData: JSONType | undefined;          // ← typed as plain value
  currentValue: ParsedEncryptedOrUnencryptedMessage<JSONType> | undefined;
}) => Promise<[JSONType, string | undefined] | false>;

But the implementation in src/chelonia.ts:2872-2874 passes it as a getter that can throw:

        get currentData () {
          return currentValue?.data
        },

currentValue.data is a lazy accessor (src/chelonia.ts:3125 / signedIncomingData) that forces decryption/verification and can throw ChelErrorDecryptionError or ChelErrorSignatureError. The update onconflict handler wraps access in try/catch (src/kv.ts:2564-2572), and clear does the same (src/kv.ts:2980-2984), but the type gives no indication that accessing currentData can throw. A future caller or refactor that reads args.currentData without a try/catch would introduce an unhandled exception.

Suggestion: Either change the implementation to eagerly compute currentData before calling onconflict (catching decryption failures and passing undefined), or change the type to reflect the throwing nature. The eager approach is safer and simpler:

      // In resolveData, before calling onconflict:
      let currentDataValue: JSONType | undefined
      try {
        currentDataValue = currentValue?.data
      } catch {
        currentDataValue = undefined
      }
      const result = await onconflict!({
        contractID,
        key,
        failedData: data,
        status: response.status,
        etag: headerEtag,
        currentData: currentDataValue,   // ← plain value, no getter
        currentValue
      })

This also makes the value accessible without re-invoking the lazy accessor on each read.

---

4. 🟡 Pre-existing typos in comments within the modified resolveData function

• Addressed
• Dismissed

The resolveData function in src/chelonia.ts was significantly modified by this PR. It contains three pre-existing comment typos (confirmed via git blame — from the initial commit, but now in heavily-modified code):

src/chelonia.ts:2799-2800:

      // All of these situations should trigger parsing the respinse and
      // conlict resolution

src/chelonia.ts:2919:

          // for subsequent iterations that require conflic resolution.

Suggestion:

• respinse → response
• conlict → conflict
• conflic resolution → conflict resolution

---

5. ⚪️ Redundant optional chaining after existence check in _loadSlotNow

• Addressed
• Dismissed

src/kv.ts:1707-1709:

      const entry = rootState._kv?.[contractID]?.[slot.key]
      if (!entry) return
      const previousValue = entry?.value

entry is guaranteed non-null after the if (!entry) return guard, so entry?.value can be entry.value. The ?. is harmless but could mask a logic error if the guard were ever removed.

Suggestion:

      const previousValue = entry.value

---

6. ⚪️ seedValue IIFE in chelonia/kv/update could be a named helper

• Addressed
• Dismissed

src/kv.ts:2470-2495 uses an IIFE to compute seedValue, which makes the control flow harder to follow than a named function:

      const seedValue: JSONType | undefined = (() => {
        if (reloadRan) {
          if (mirrorEntry?.value !== undefined) {
            return cloneDeep(mirrorEntry.value as JSONType)
          }
          return slot.resolvedDefault !== undefined
            ? cloneDeep(slot.resolvedDefault)
            : undefined
        }
        if (
          mirrorEntry &&
          mirrorEntry.status !== KV_LOAD_STATUS.ERROR &&
          mirrorEntry.value !== undefined
        ) {
          return cloneDeep(mirrorEntry.value as JSONType)
        }
        return slot.resolvedDefault !== undefined
          ? cloneDeep(slot.resolvedDefault)
          : undefined
      })()

Suggestion: Extract to a named helper for readability:

function computeSeedValue (
  slot: SlotDefinition,
  mirrorEntry: KvMirrorEntry | undefined,
  reloadRan: boolean
): JSONType | undefined {
  if (reloadRan || (
    mirrorEntry &&
    mirrorEntry.status !== KV_LOAD_STATUS.ERROR &&
    mirrorEntry.value !== undefined
  )) {
    if (mirrorEntry?.value !== undefined) {
      return cloneDeep(mirrorEntry.value as JSONType)
    }
  }
  return slot.resolvedDefault !== undefined
    ? cloneDeep(slot.resolvedDefault)
    : undefined
}

Then: const seedValue = computeSeedValue(slot, mirrorEntry, reloadRan)

---

Summary

The implementation is thorough and the inline documentation is extensive. The most actionable issue is #1 (DRY violation) — the duplicated validation logic is ~50 lines that must be kept in sync manually, and extracting a helper would eliminate the risk of them diverging. #2 and #3 are medium-priority issues where the docs/types don't match the runtime behavior, which could trip up consumers or future maintainers.

---

Review generated using glm-5.2 via Z.AI. Comment /review to re-run.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.2

Click to expand review

I'll start by examining the key files in this PR to provide a thorough review. Let me look at the new kv.ts file and related changes.Now let me look at the supporting files and key parts of the diff.Now let me look at key sections of chelonia.ts in detail to understand the kv/set changes:Let me check the index.ts exports and look for the ChelErrorKvReentrant import:Let me check how the test file imports things and verify the re-export situation:Let me check if the new test file is actually run:Let me verify the specific typos I noticed in the comments:Let me check the PUBSUB_RECONNECTION_SUCCEEDED export:Let me check the CONTRACTS_MODIFIED event payload shape:Let me verify the externalStateSetup integration with _kv:Let me check if ChelErrorKvReentrant is in the docs error table:Let me verify the export ambiguity issue by checking all KV constant import patterns:Now I have a thorough understanding of the changes. Let me write up the review.Now I have a complete picture of the changes. Here is my review:

---

Review: PR #26 — Enhanced KV Store Key-Value Experience

Issue 1 🔴

• Addressed
• Dismissed

Star-export ambiguity makes KV_NOOP and other public KV constants unimportable from the package root.

src/index.ts:20-21 does:

export * from './kv-constants.js'
export * from './kv.js'

And src/kv.ts:3504-3514 re-exports those same names:

export {
  KV_AUTO_LOAD,
  KV_DEFAULT_ENCRYPTION_KEY_NAME,
  KV_DEFAULT_SIGNING_KEY_NAME,
  KV_LOAD_STATUS,
  KV_NOOP,
  KV_UPDATE_REASON,
  KV_VALIDATION_REASON_REVALIDATE
} from './kv-constants.js'
export type { KvNoop } from './kv-constants.js'

Under the ES module spec (which TypeScript follows for export *), when the same name is star-exported from two different sources, the name is silently dropped — it is not exported from the module and no error is raised. This means all 7 constants plus the KvNoop type listed above become ambiguous and are absent from the package root. A consumer following the documented import (docs/api.md:393):

import { KV_NOOP } from '@chelonia/lib'

will get a compile error: Module '"@chelonia/lib"' has no exported member 'KV_NOOP'.

Fix — remove the redundant export * from './kv-constants.js' line from index.ts so kv.ts's curated re-exports are the sole source:

// src/index.ts — remove this line:
// export * from './kv-constants.js'

Internal-only constants (KV_ECHO_CID_MAX, KV_KEY_SEPARATOR, etc.) will simply no longer be importable from the package root, which is the correct behavior for implementation details. If any external consumer needs them, add a ./kv-constants subpath to the exports map.

---

Issue 2 🟡

• Addressed
• Dismissed

Three typos in comments inside the rewritten resolveData / retry-loop code in chelonia/kv/set.

src/chelonia.ts:2799-2800:

      // All of these situations should trigger parsing the respinse and
      // conlict resolution

"respinse" → "response", "conlict" → "conflict".

src/chelonia.ts:2919:

          // for subsequent iterations that require conflic resolution.

"conflic" → "conflict".

---

Issue 3 🟡

• Addressed
• Dismissed

ChelErrorKvReentrant and ChelErrorInvalidMessageHeight are missing from the error table in docs/api.md.

ChelErrorKvReentrant is a public error class (src/errors.ts:62, re-exported via index.ts → export * from './errors.js') thrown by chelonia/kv/update, chelonia/kv/clear, and chelonia/kv/sync. It is mentioned in the prose body (docs/api.md:279) but absent from the formal error table at docs/api.md:439-443.

ChelErrorInvalidMessageHeight (src/errors.ts:33) was introduced in this PR and replaces a generic Error throw in parseEncryptedOrUnencryptedMessage (src/chelonia.ts:3128). Callers that previously matched on the generic Error may now see a different .name. It is not documented anywhere.

Both should be added to the error table:

| `ChelErrorInvalidMessageHeight` | `parseEncryptedOrUnencryptedMessage` received a message whose height is outside `[0, currentHeight]`. |
| `ChelErrorKvReentrant` | A KV write selector (`update`/`clear`/`sync`) was called synchronously from within the same contract's `onUpdate` callback, which would deadlock the per-contract queue lane. |

---

Issue 4 🟡

• Addressed
• Dismissed

DRY violation: reducer-output validation logic is duplicated between the first-attempt path and the conflict-retry onconflict path in chelonia/kv/update.

The following validation sequence appears twice in src/kv.ts — once at kv.ts:2496-2549 (first attempt) and again at kv.ts:2617-2665 (conflict retry). The blocks are structurally identical: call reducer → check symbol → check null/undefined → schema.parse or assertJsonShape → wrap error.

This is ~50 lines of near-identical code, differing only in variable names, error message suffixes ("threw" vs "threw on retry"), and the KV_NOOP handling (return undefined vs throw new KvNoopAbort()).

Suggestion — extract a shared helper:

function validateReducerOutput (
  slot: SlotDefinition,
  contractID: string,
  key: string,
  output: unknown,
  context: 'first-attempt' | 'conflict-retry'
): { noop: true } | { noop: false; value: JSONType } {
  if (typeof output === 'symbol') {
    if (output === KV_NOOP) return { noop: true }
    throw new ChelErrorKvUpdateInvalid(
      `[chelonia/kv] update: ${contractID}::${key} reducer returned ` +
      `an unexpected symbol${context === 'conflict-retry' ? ' on retry' : ''}; use KV_NOOP to abort`
    )
  }
  if (output === null || output === undefined) {
    throw new ChelErrorKvUpdateInvalid(
      `[chelonia/kv] update: ${contractID}::${key} reducer returned ` +
      `${String(output)}${context === 'conflict-retry' ? ' on retry' : ''}; use chelonia/kv/clear or KV_NOOP instead`
    )
  }
  const label = context === 'conflict-retry'
    ? `update onconflict retry ${contractID}::${key}`
    : `update ${contractID}::${key}`
  const value = slot.schema
    ? parseSyncSlotValue(slot, output, label)
    : assertJsonShape(output, label)
  return { noop: false, value }
}

Then each call site becomes:

// First attempt
const result = validateReducerOutput(slot, contractID, key, reducerOut, 'first-attempt')
if (result.noop) return undefined
let nextValue = result.value

// Conflict retry
const result = validateReducerOutput(slot, contractID, key, retried, 'conflict-retry')
if (result.noop) throw new KvNoopAbort()
nextValue = result.value

---

Issue 5 🟡

• Addressed
• Dismissed

The basis-selection logic in chelonia/kv/update's onconflict callback duplicates the seed-value computation from the first-attempt path, with subtle differences in error handling.

src/kv.ts:2470-2495 (seed value) and src/kv.ts:2573-2615 (onconflict basis) both implement the same pattern: undefined → default, null → default, schema-backed → parseSyncSlotValue, schemaless → assertJsonShape. However, the seed path (kv.ts:2470-2495) does NOT clone before parsing (the mirror value was already cloned via cloneDeep), while the onconflict path (kv.ts:2586-2606) DOES cloneDeep the parsed result — to detach from the server's cached decode. These differences are intentional but fragile: a future maintainer changing one path could easily forget the other.

Suggestion — extract a shared resolveBasis(slot, rawValue, contractID, key, context) helper that both the seed path and the onconflict path call, with the clone parameter documented:

function resolveReducerBasis (
  slot: SlotDefinition,
  rawValue: JSONType | undefined | null,
  contractID: string,
  key: string,
  context: 'seed' | 'conflict',
  clone: boolean
): JSONType | undefined {
  if (rawValue === undefined || rawValue === null) {
    return slot.resolvedDefault !== undefined
      ? cloneDeep(slot.resolvedDefault)
      : undefined
  }
  const label = context === 'conflict'
    ? `update onconflict currentData ${contractID}::${key}`
    : `update seed ${contractID}::${key}`
  const parsed = slot.schema
    ? parseSyncSlotValue(slot, rawValue, label)
    : assertJsonShape(rawValue, label)
  return clone ? cloneDeep(parsed) : parsed
}

---

Issue 6 🟡

• Addressed
• Dismissed

ChelErrorKvValidation error table description is incomplete.

docs/api.md:442:

| `ChelErrorKvValidation` | Reducer output failed `schema.parse`. |

This error is thrown in seven distinct paths, not just reducer output validation:

• src/kv.ts:1720 — load-time schema validation failure
• src/kv.ts:1728 — load-time JSON-shape validation failure (schemaless slots)
• src/kv.ts:2507 — schema-less reducer output failing assertJsonShape (the else branch)
• src/kv.ts:2526 — reducer output failing schema.parse
• src/kv.ts:2567 — server currentData decode failure on conflict retry
• src/kv.ts:2594 — server currentData failing schema.parse on conflict retry
• src/kv.ts:3349 — re-validation failure in revalidateMirrorEntry

The description should be broadened to match:

| `ChelErrorKvValidation` | A slot value failed `schema.parse` or `assertJsonShape`. Thrown by `update` (reducer output or server `currentData` on conflict), `_loadSlotNow` (GET response decode/shape failure), and `revalidateMirrorEntry`. Companion event: `CHELONIA_KV_VALIDATION_ERROR` for load/remote/re-validate failures (local reducer-output failures reject the `update` promise instead). |

---

Issue 7 ⚪️

• Addressed
• Dismissed

No ./kv or ./kv-constants subpath in the exports map.

Unlike ./journal (which has its own subpath export), the KV module has no subpath. The AGENTS.md even references @chelonia/lib/journal as a precedent for tree-shaking, and docs/api.md lists src/kv.ts as the source for KV_NOOP, but there's no corresponding @chelonia/lib/kv import path. This is inconsistent with the journal module's export strategy. If Issue 1's ambiguity fix removes export * from './kv-constants.js' from index.ts, consumers who need internal constants would have no import path at all. Consider adding:

"./kv-constants": {
  "import": {
    "types": "./dist/esm/kv-constants.d.mts",
    "default": "./dist/esm/kv-constants.mjs"
  },
  "require": {
    "types": "./dist/cjs/kv-constants.d.cts",
    "default": "./dist/cjs/kv-constants.cjs"
  }
}

---

Issue 8 ⚪️

• Addressed
• Dismissed

Type mismatch: clear's onconflict callback declares currentData as optional, but ChelKvOnConflictCallback defines it as required.

src/kv.ts:2961-2964:

    const onconflict = async (conflictArgs: {
      currentData?: JSONType;
      etag?: string | null;
    }): Promise<[JSONType, string | undefined]> => {

But ChelKvOnConflictCallback in src/types.ts:665-673 declares:

  currentData: JSONType | undefined;

(required key, though the value may be undefined). The ? in the callback type is technically incorrect — chelonia/kv/set always provides currentData (as a getter). It doesn't cause a runtime issue because SBP bypasses TypeScript at call time, but it's misleading to readers. The etag?: is also narrower than the real type (string | null | undefined).

---

Issue 9 ⚪️

• Addressed
• Dismissed

CONTRACTS_MODIFIED listener first parameter typed as Set<string> but the event emits an Array.

src/chelonia.ts:1272:

      this.kvContractsModifiedListener = (
        _contracts: Set<string>,
        payload: { added: string[]; removed: string[] }
      ) => {

But all CONTRACTS_MODIFIED emissions pass Array.from(this.subscriptionSet) (e.g. src/internals.ts:699). The parameter is unused (prefixed _), so there's no runtime impact, but the type annotation should be string[] for accuracy:

        _contracts: string[],

---

Review generated using glm-5.2 via Z.AI. Comment /review to re-run.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.2

Click to expand review

I'll review this large KV slots PR thoroughly. Let me start by examining the key files.Now let me read the constants and types files in parallel:Now let me check the chelonia.ts KV-related changes (the kv/set, kv/get, and the connect/reset paths):Let me run the tests to verify the implementation works:Now let me investigate specific concerns more deeply. Let me check the composeSignals for a potential leak and verify the _loadSlotNow early return paths don't leave kvPendingLoads unbalanced:Now let me check a specific concern about the _handleRemote — the interaction between echo suppression and the rawKvHandler in chelonia.ts:Let me do a final targeted search for potential issues:Let me verify one final potential issue — the interaction between chelonia/kv/set's resolveData and body consumption, and verify the onconflict! non-null assertion is safe:Now let me compile the final review. Let me verify a few remaining details:Now I have a thorough understanding. Let me compile the review:## Code Review: KV Slots API (#26)

I've thoroughly reviewed the diff — kv.ts, kv-constants.ts, internal-errors.ts, chelonia.ts (kv/set/get/connect/reset), chelonia-utils.ts, types.ts, events.ts, pubsub/index.ts, both test files, and the doc updates. The design is solid and the edge-case reasoning in the comments is exceptional. Below are the issues I found, ordered by importance.

---

Issue 1 — 🟡 DRY: repeated validation-failure pattern in _handleRemote

• Addressed
• Dismissed

The "emit CHELONIA_KV_VALIDATION_ERROR + setSlotStatus(ERROR) + return" pattern is copy-pasted three times in _handleRemote (kv.ts:2227-2238, kv.ts:2260-2277, kv.ts:2287-2299):

// kv.ts:2227-2238 (decode failure)
sbp('okTurtles.events/emit', CHELONIA_KV_VALIDATION_ERROR, {
  contractID,
  contractType: slot.contractType,
  key,
  error: e,
  reason: KV_UPDATE_REASON.REMOTE
})
setSlotStatus(
  this, rootState, contractID, slot.contractType, key,
  KV_LOAD_STATUS.ERROR, normalizeError(e)
)
return Promise.resolve()

_loadSlotNow already centralizes this into its failLoadValidation helper (kv.ts:1648-1662). _handleRemote should do the same — a local failRemoteValidation(e) closure would eliminate the duplication and prevent the three call-sites from drifting:

const failRemoteValidation = (e: unknown): Promise<void> => {
  sbp('okTurtles.events/emit', CHELONIA_KV_VALIDATION_ERROR, {
    contractID, contractType: slot.contractType, key, error: e, reason: KV_UPDATE_REASON.REMOTE
  })
  setSlotStatus(this, rootState, contractID, slot.contractType, key,
    KV_LOAD_STATUS.ERROR, normalizeError(e))
  return Promise.resolve()
}

---

Issue 2 — ⚪️ DRY: currentData === undefined and currentData === null branches produce identical code

• Addressed
• Dismissed

In the update onconflict (kv.ts:2593-2601), both the undefined and null cases assign the exact same basis:

// kv.ts:2593-2601
if (currentData === undefined) {
  basis = slot.resolvedDefault !== undefined
    ? cloneDeep(slot.resolvedDefault)
    : undefined
} else {
  if (currentData === null) {
    basis = slot.resolvedDefault !== undefined
      ? cloneDeep(slot.resolvedDefault)
      : undefined
  } else if (slot.schema) {

These can be collapsed with == null (which catches both null and undefined):

if (currentData == null) {
  basis = slot.resolvedDefault !== undefined
    ? cloneDeep(slot.resolvedDefault)
    : undefined
} else if (slot.schema) {

The same slot.resolvedDefault !== undefined ? cloneDeep(slot.resolvedDefault) : undefined expression appears ~7 more times across kv.ts (lines 741-743, 800-802, 1800-1802, 2245-2247, 2594-2596, 3041-3043) and could be a tiny cloneDefault(slot) helper.

---

Issue 3 — ⚪️ Dead/redundant code in assertJsonShape

• Addressed
• Dismissed

In assertJsonShape (kv.ts:310-355), several branches are unreachable:

1. kv.ts:323 — t === 'undefined' is dead: value === undefined was already caught and thrown at line 317.
2. kv.ts:339 — if (value && typeof value === 'object') is always true at that point (null was thrown at 317, non-object thrown at 330, arrays returned at 335).
3. kv.ts:351 — the trailing throw is unreachable because every type is handled before reaching it.

Suggested simplification:

function assertJsonShape (input: unknown, where: string): JSONType {
  const visit = (value: unknown, path: string, depth: number): void => {
    if (depth > MAX_JSON_DEPTH) {
      throw new ChelErrorKvValidation(
        `[chelonia/kv] ${where}: ${path} exceeded max depth (possible circular reference)`
      )
    }
    if (value === null || value === undefined) {
      throw new ChelErrorKvValidation(
        `[chelonia/kv] ${where}: ${path} is the reserved sentinel ${String(value)}`
      )
    }
    const t = typeof value
    if (t === 'string' || t === 'boolean') return
    if (t === 'number') {
      if (Number.isFinite(value)) return
      throw new ChelErrorKvValidation(
        `[chelonia/kv] ${where}: ${path} has non-finite number ${String(value)}`
      )
    }
    if (t !== 'object') {
      throw new ChelErrorKvValidation(
        `[chelonia/kv] ${where}: ${path} has non-JSON type ${t}`
      )
    }
    if (Array.isArray(value)) {
      for (let i = 0; i < value.length; i++) visit(value[i], `${path}[${i}]`, depth + 1)
      return
    }
    const proto = Object.getPrototypeOf(value)
    if (proto !== Object.prototype && proto !== null) {
      throw new ChelErrorKvValidation(
        `[chelonia/kv] ${where}: ${path} has non-plain object prototype`
      )
    }
    for (const [k, v] of Object.entries(value as Record<string, unknown>)) {
      visit(v, `${path}.${k}`, depth + 1)
    }
  }
  visit(input, 'value', 0)
  return input as JSONType
}

---

Issue 4 — ⚪️ Outdated test-count comment

• Addressed
• Dismissed

kv.test.ts:1 says:

// KV slot API tests — 82 cases total: 27 from KV-REVAMPED.md §11.6
// plus 55 implementation-specific cases ...

The file now has 156 it(...) blocks (up to test 88e). The comment should be updated or changed to a non-counted form (e.g. "KV slot API tests covering KV-REVAMPED.md §11.6 and implementation-specific edge cases").

---

Issue 5 — ⚪️ Inaccurate "five KV maps" in _assertIndexConsistent comment

• Addressed
• Dismissed

kv.ts:993 states:

// Walks the five KV maps + `rootState._kv` and verifies:

The function actually walks kvSlotsByContractID, kvActiveFilters, kvFilterDirty (a Set, not a Map), and rootState._kv. It also uses kvSlots for lookups but does not iterate it. The count "five" is not accurate.

---

Issue 6 — ⚪️ Naming inconsistency: _recordEchoCIDForTest vs _test* prefix

• Addressed
• Dismissed

The other dev-only test selectors in kv.ts all use a _test prefix (kv.ts:961 _testSetNowMs, kv.ts:969 _testNowMs, kv.ts:974 _testSetFilterRetryMs). But kv.ts:980 uses a ...ForTest suffix:

'chelonia/kv/_recordEchoCIDForTest': function (...)

For consistency it should be _testRecordEchoCID so all test-only selectors share the same _test prefix and are easy to grep/filter.

---

Review generated using glm-5.2 via Z.AI. Comment /review to re-run.

[taoeffect]

Fable review ready!

Details

Code Review

Base: main
Head: pr (da28cd8)
Date: 2026-07-10
Model: Fable (Claude Fable 5)
Scope: KV API revamp (KV-REVAMPED.md) — src/kv.ts (new, 3496 lines), src/kv.test.ts (new, 156 tests), plus extensions to chelonia.ts, chelonia-utils.ts, types.ts, local-selectors/, pubsub/, events.ts, errors.ts, internals.ts, docs.

---

Verification performed

• Read the full KV-REVAMPED.md design document and checked the implementation against it (§4 API surface, §11.2 index invariant, §11.3 selector semantics, §11.5 filter coalescing, §4.9 self-echo suppression).
• npm test passes (kv.test.ts suite + 133 aggregate tests), npm run lint clean.
• npm run build re-run from scratch: zero drift against the committed dist/ output (the Build commit is complete; the subsequent merge of main brought no src/ changes).
• Two targeted call-site investigations: (a) pubsub kvFilter restoration across connect/reconnect, (b) all four CONTRACTS_MODIFIED emission sites vs. the new KV listeners.

Compatibility assessment (changes outside kv.ts)

Per request, breaking-change analysis was done first. No breaking changes found. Details of every observable change to pre-existing surface:

Change	Verdict
chelonia/kv/set resolves to { etag: string | null } (was void); accepts signal, forwards maxAttempts	Additive. All existing callers ignore the return value.
chelonia/kv/set conflict exhaustion now throws ChelErrorKvMaxAttempts (internal class) instead of plain Error	Same message string ('kv/set conflict setting KV value'); only .name/prototype differ. Documented in internal-errors.ts. Callers matching on message are unaffected.
chelonia/kv/set recovery GET on body-less 409/412 (chelonia.ts, recoveryGetAttempted)	New behavior, strictly better for direct callers: onconflict now receives a recovered etag/currentData instead of undefined, bounded to one GET per call (locked in by chelonia-kv-set.test.ts).
Conflict with no onconflict handler now throws before the 0–1500 ms random delay instead of after	Same error, faster failure. Not observable except in timing.
chelonia/kv/get returns ChelKvGetResult (parsed message + lazily attached etag)	Additive; defineProperty on the (unfrozen) result object, laziness of the data accessor preserved.
chelonia/kv/queuedSet forwards signal/maxAttempts, returns { etag }	Additive.
ChelKvOnConflictCallback return type widened to [JSONType, string | undefined] | false	Type-level only. The runtime if (!result) return false falsy-abort path already existed on main; the widened type documents pre-existing behavior (the "behaviour change" note in types.ts slightly overstates this — see issue 6).
NOTIFICATION_TYPE.KV message handler restructured (chelonia.ts:1170+)	Verified: a consumer-provided raw KV handler is still invoked with [key, parsed] inside the same per-contract queueInvocation lane; the slot layer runs in addition. When neither a raw handler nor the kv module exists, the handler returns before parsing. No regression.
parseEncryptedOrUnencryptedMessage throws ChelErrorInvalidMessageHeight (was plain Error)	Name change only; exported and documented in docs/api.md.
chelonia/externalStateSetup now uses sbp(stateSelector) in the CONTRACTS_MODIFIED handler (was hardcoded state/vuex/state)	Bug fix. Behavior changes only for consumers that passed a stateSelector different from 'state/vuex/state' — previously their contract projections silently went to Vuex. Worth a line in release notes.
chelonia/externalStateSetup returns a teardown function	Additive (previously returned undefined).
chelonia/reset: abort moved before postCleanupFn, KV writes drained via _waitInFlight	In-flight fetches are now aborted earlier (before postCleanupFn instead of after). All contract waits still precede it; low risk.
chelonia/contract/fullState gains optional key param and kvState/kvEntry result fields	Additive.
chelonia/defineContract throws if contract.kv is present and the kv module isn't loaded	Only affects new-style contracts; correct fail-fast.
createKvMessage gains optional cid; NOTIFICATION_TYPE.KV msg type gains cid?	Additive. Old servers that emit frames without cid degrade gracefully (authoritative-GET path, documented in docs/api.md).
index.ts import reordering (chelonia.ts last, because it locks the chelonia SBP domain)	Consistent with the pre-existing constraint that already applied to ./journal, ./files, etc. Deep-importing ./chelonia before ./kv silently drops the kv selector registration (SBP warns) — pre-existing pattern, now explicitly documented in index.test.ts.

Cross-cutting wiring verified specifically:

• Filter restoration across connect/reconnect: chelonia/connect pre-seeds client.kvFilter synchronously before the socket can open, and the pubsub client reads kvFilter lazily at SUB-send time (with a KV_FILTER re-send on SUB-ack mismatch), so filters survive first connect and every reconnect. The direct-map-mutation idiom at chelonia.ts:1244 is only safe because the socket is guaranteed not-yet-open there (it does not transmit) — fine as used, but do not reuse that idiom against a connected client.
• CONTRACTS_MODIFIED: all four emit sites pass Array.from(this.subscriptionSet) (matches the new listener typing), mutate subscriptionSet before emitting (the design's ordering claim holds), and the direct _cleanupContractRuntime call in removeImmediately plus the listener-driven call are correctly idempotent.
• Self-echo suppression / conflict-marker state machine (_handleRemote): traced both arrival orderings (echo-first and competing-frame-first), the demote-don't-delete rule, TTL expiry, and the eviction cap; no mirror-regression path found. Suppression is recorded before the abort/staleness checks in update/clear, so a committed write's echo can never re-validate against a replaced slot.
• Lane serialization: mirror reads, reducers, kv/set, _handleRemote, and onUpdate all run inside the per-contract queueInvocation lane; _waitInFlight's three-source drain (index, echo CIDs, pending-write counters) covers the queued-but-not-started window.

The issues found follow, in priority order.

---

1. 🟡 Filter flush enters an infinite 2-second retry loop if slots reconcile before chelonia/connect

• Addressed
• Dismissed

chelonia/kv/setFilter dereferences this.pubsub unguarded (src/chelonia.ts:3023-3025):

'chelonia/kv/setFilter': function (this: CheloniaContext, contractID: string, filter?: string[]) {
  this.pubsub.setKvFilter(contractID, filter)
},

this.pubsub is only assigned inside chelonia/connect (src/chelonia.ts:1086). Before this PR that was harmless — only consumers called setFilter, and they did so after connecting. Now the slot layer calls it autonomously: defineSlot reconciles against already-synced contracts and queues a filter flush (src/kv.ts:1398-1408 → queueFilterFlush → flushDirtyFilters). If an app syncs contracts (HTTP-only, works without a websocket) and registers slots before calling chelonia/connect — or never connects at all (SSR / CLI / tooling contexts) — the flush throws TypeError: Cannot read properties of undefined (reading 'setKvFilter'), which is caught at src/kv.ts:918-928 and routed to scheduleFilterRetry:

} catch (e) {
  console.warn(`[chelonia/kv] setFilter flush failed for ${cID}`, e)
  ...
  scheduleFilterRetry(ctx, cID)
}

scheduleFilterRetry (src/kv.ts:942-956) re-queues the contract and re-flushes after KV_FILTER_RETRY_MS (2 s), fails identically, and re-enters itself — an unbounded retry loop that (a) spams one console.warn per contract every 2 seconds and (b) keeps a live setTimeout chain that prevents a Node process from ever exiting. Nothing distinguishes this permanent precondition failure from the transient server errors the retry was designed for.

It self-heals only if chelonia/connect is eventually called (the retry then succeeds), which is why tests (which stub setFilter) and Group Income (which connects at boot) never hit it.

Suggested fix — treat "no pubsub yet" as "nothing to flush" rather than a failure, since chelonia/connect already re-applies the complete filter set from kvActiveFilters (src/chelonia.ts:1242-1246):

// in flushDirtyFilters, before the setFilter call (or at loop entry):
if (!ctx.pubsub) {
  // chelonia/connect re-seeds client.kvFilter from kvActiveFilters,
  // so pre-connect flushes are safely dropped.
  ctx.kvFilterDirty.clear()
  return
}

Alternatively guard inside chelonia/kv/setFilter itself (if (!this.pubsub) return). Also consider .unref?.() on kvFilterRetryTimer so a genuinely-retrying timer cannot pin a Node process open.

2. 🟡 docs/api.md contradicts the implemented 'error'-status seeding behavior of chelonia/kv/update

• Addressed
• Dismissed

docs/api.md (added in this PR, "rejection taxonomy" section) states:

When a slot is in 'error' status, update seeds the reducer the same way read does: from the declared default, not from the retained mirror value. The mirror may still hold the last valid value and etag after a validation failure, but the reducer does not observe that retained value until a successful load, sync, remote update, or local write transitions the slot out of 'error'.

This describes an earlier design. The implementation (and AGENTS.md's "Data-loss-guard reload (status quiet)" section) do the opposite for the retained-value case: chelonia/kv/update on an 'error' slot that still holds a value performs a silent authoritative reload and then seeds the reducer from the mirror — the live server value on reload success, or the retained value on reload failure (src/kv.ts:2492-2555, computeSeedValue at src/kv.ts:365-381):

if (mirrorEntry && mirrorEntry.status === KV_LOAD_STATUS.ERROR && mirrorEntry.value !== undefined) {
  reloadRan = true
  ... // silent _loadSlotNow; on failure, seed from retained value
}
const seedValue = computeSeedValue(slot, mirrorEntry, reloadRan)

Only the 'error'-with-no-retained-value case seeds from the default. The implemented behavior is the right one (the doc's described behavior is exactly the silent-data-loss scenario the reload guard exists to prevent), so the fix is to rewrite this api.md paragraph to match AGENTS.md — e.g. "When a slot is in 'error' status but still holds a retained value, update first performs one silent authoritative reload and seeds the reducer from the refreshed (or retained) mirror value; only an 'error' slot with no retained value seeds from the declared default."

While editing docs: AGENTS.md's config table says autoLoad: 'on-demand' "waits for read/sync" — chelonia/kv/read never triggers a fetch (it synchronously returns the default); only sync or a successful update materializes the value. docs/api.md's selector table has the same implication for read. Both should say "waits for sync (or a successful update)".

3. ⚪️ Released contracts leave a stale empty kvFilter entry on the pubsub client, silencing raw-API KV frames after re-sync

• Addressed
• Dismissed

chelonia/kv/_cleanupContractRuntime (src/kv.ts:1910-1936) queues an empty-filter flush before dropping per-contract state, so the flush emits setFilter(cID, []). On the release path, chelonia/pubsub/update unsubscribes the channel synchronously (during the CONTRACTS_MODIFIED emit) before the flush's microtask runs, so setKvFilter (src/pubsub/index.ts:977-1000) takes the not-subscribed branch: it does not send a frame but it does record client.kvFilter.set(cID, []) — and an empty array means "deliver no KV keys". That map entry is never removed.

If the same contract is later re-synced, the SUB payload carries the stale kvFilter: [] (pubPayloadFactory, src/pubsub/index.ts:272-275). When active slots reconcile, the subsequent flush corrects it (via the SUB-ack mismatch path), so the slot API is unaffected. But if no slot matches the re-synced contract anymore — slot definitions removed via defineContract replacement, or every match returns false — the stale [] persists and the server delivers no KV frames for that contract, whereas a never-slotted contract receives all of them. A raw-API consumer (the legacy NOTIFICATION_TYPE.KV callback) would silently observe nothing.

The "Filter ownership: don't mix slots with raw setFilter" section of docs/api.md partially covers this, but the ownership claim there is per-contract-with-declared-slots, while this leak outlives the slots. Suggested fix: on the release path, remove the client-side filter entry instead of setting it empty — e.g. have _cleanupContractRuntime skip the flush-and-empty dance when the contract is leaving the subscription set entirely, and instead call chelonia/kv/setFilter(cID) (the documented remove-filters form) or delete from client.kvFilter directly, restoring the "no filter = all keys" default for a future unslotted re-subscribe.

4. ⚪️ update/clear aborted after the server write commits leave the mirror stale indefinitely (by design, but undocumented for consumers)

• Addressed
• Dismissed

When the caller's signal aborts in the window after chelonia/kv/set resolves, update records the echo CID first and then rejects with AbortError (src/kv.ts:2726-2733; same pattern in clear at src/kv.ts:3016-3031). The write has committed server-side, its pubsub echo is deliberately suppressed, and the §4.2 abort contract ("mirror unchanged, no event") is honored — with the consequence that this client's mirror (value and etag) stays stale until the next remote frame, local write (whose stale ifMatch then round-trips through a 412), or explicit sync. Other clients see the new value; the aborting client does not, potentially forever on a quiet slot.

The in-code rationale (src/kv.ts:2711-2725) is sound and I found no better alternative that keeps the abort contract, but neither AGENTS.md's abort discussion nor docs/api.md's taxonomy row ("AbortError — mirror is unchanged; no event fires") tells the consumer that the write may nonetheless have been persisted and the mirror may diverge from the server afterwards. Since aborting near the commit boundary is precisely when a caller most needs to know this, add a sentence to both: "an abort that lands after the server accepted the write leaves the write committed; the mirror intentionally does not reflect it until the next load/remote/write — call chelonia/kv/sync after aborting if you need the mirror reconciled."

5. ⚪️ Dead branch in assertJsonShape

• Addressed
• Dismissed

src/kv.ts:317-323:

if (value === null || value === undefined) {
  throw new ChelErrorKvValidation(...)
}
const t = typeof value
if (t === 'string' || t === 'boolean' || t === 'undefined') return

t === 'undefined' is unreachable — value === undefined already threw four lines above. Harmless, but the dead condition reads as if undefined were accepted, which is the opposite of the invariant this function enforces. Drop || t === 'undefined'.

6. ⚪️ types.ts doc comment overstates the onconflict falsy-abort as a behavior change

• Addressed
• Dismissed

The new doc block on ChelKvOnConflictCallback (src/types.ts, "NOTE: this is a behaviour change vs. pre-KV-revamp consumers, who could only return a tuple… will now silently no-op") is inaccurate: the runtime if (!result) return false abort path already existed on main (it is unchanged context in this diff, resolveData in src/chelonia.ts). Only the type was narrowed before; a falsy return already silently aborted the write pre-revamp rather than "historically threw downstream". The same claim is repeated in docs/api.md's migration notes ("Pre-revamp consumers that accidentally returned a non-tuple falsy value would have crashed downstream; they now silently no-op"). Both should say the type now documents pre-existing runtime behavior — otherwise a direct caller auditing per these notes will hunt for a runtime change that never happened.

---

Notes (no action required)

• src/kv.ts overall: the implementation is unusually defensive and internally consistent — staleness guards on every await boundary, the §11.2 index invariant enforced by a dev-mode checker, non-destructive mirror reads to avoid orphaned records, deep-clone discipline on every value crossing the mirror boundary, and a sound synchronous-window re-entrancy guard for onUpdate. The conflict-marker demote-don't-delete logic in _handleRemote correctly handles both echo orderings.
• The design doc's §11.6 asks for _assertIndexConsistent in every test's afterEach; the suite instead calls it at targeted points (5 call sites). Acceptable, but a global afterEach would be cheap insurance.
• chelonia/kv/get still treats HTTP 410 as an error while chelonia/kv/set treats 404/410 alike as "no data" — a pre-existing asymmetry this PR neither introduced nor fixed; a released-then-GC'd contract's slot load will surface 'error' rather than 'non-init'.
• The cid field on pubsub KV frames requires a matching server-side change to be emitted; until servers send it, every remote frame on an etag-bearing slot takes the authoritative-GET path (one extra GET per frame). This is documented in docs/api.md and degrades gracefully, but is worth remembering when sequencing the server deploy.

[corrideat]

Ready