[corrideat]

This PR addresses some of the feedback in okTurtles/group-income#3137.

In particular, okTurtles/group-income#3137 introduces a new file, frontend/controller/actions/kv-guard.js, to address some limitations in the KV API here in @chelonia/lib regarding slots (for example, telling apart those that are empty from those that are uninitialized).

Those workarounds don't belong in Group Income, as we can expect that other applications will run into exactly the same limitations. This PR should make those guards redundant by directly addressing those concerns in the KV API.

File in question

export const isHeightAheadError = (e: ?Object): boolean => {
  // Chelonia may rewrap the original error, so walk the cause chain instead of
  // only checking the top-level error (e.g. `chelonia/kv/*` wraps decode
  // failures in `ChelErrorKvValidation`). Matching on `name` alone is enough
  // and is realm-safe: `ChelErrorGenerator` hard-codes the name on every
  // instance, so the test keeps working when the error crosses a bundle
  // boundary, where `instanceof` fails because the class identity differs.
  for (let cur = e, i = 0; cur && i < 5; cur = cur.cause, i++) {
    if (cur.name === 'ChelErrorInvalidMessageHeight') return true
  }
  return false
}

export const isKvConflictError = (e: ?Object): boolean => e?.name === 'ChelErrorKvConflict'

const HEIGHT_CHECK_INTERVAL = 10000
const lastHeightCheckByContractID: Map<string, number> = new Map()

// Cheap when the local contract is already current: `chelonia/contract/sync`
// only issues a `latestHEADinfo` request in that case. Throttled per contract
// (and the timestamp is taken before the await, so failures are throttled too)
// so that a burst of writes doesn't turn into a burst of syncs.
export const ensureContractHeightCurrent = async (contractID: string, force: boolean = false): Promise<void> => {
  const now = Date.now()
  if (!force && now - (lastHeightCheckByContractID.get(contractID) ?? 0) < HEIGHT_CHECK_INTERVAL) return
  lastHeightCheckByContractID.set(contractID, now)
  await sbp('chelonia/contract/sync', contractID)
}

export const withKvHeightRetry = async (contractID: string, key: string, write: () => Promise<any>): Promise<any> => {
  try {
    return await write()
  } catch (e) {
    if (!isHeightAheadError(e) && !isKvConflictError(e)) throw e
    console.warn(`[kv-guard.js] '${key}' write on ${contractID} raced a newer server value; syncing before retrying`, e)
    // A failed recovery sync must neither swallow `e` nor cancel the retry: on a
    // plain KV conflict the retried write can still succeed without it, and the
    // caller would otherwise see the sync error instead of the write error.
    await ensureContractHeightCurrent(contractID, true).catch((syncError) => {
      console.warn(`[kv-guard.js] failed to sync ${contractID} before retrying '${key}'`, syncError)
    })
    await sbp('chelonia/kv/sync', contractID, key).catch((syncError) => {
      console.warn(`[kv-guard.js] failed to re-sync '${key}' before retrying`, syncError)
    })
    return write()
  }
}

// Pre-flight sync plus retry.
//
// Which of the two helpers a given write uses follows one rule:
//
//   `withKvGuard` (pre-flight on) is for writes driven only by UI interaction
//   or by a timer, where an extra `latestHEADinfo` round trip is invisible.
//
//   `withKvHeightRetry` (pre-flight off) is for writes reachable from a
//   contract `sideEffect` or from a `MESSAGE_RECEIVE_RAW` handler. Those run on
//   the chatroom's queue lane; the sync targets the identity contract, so it
//   cannot deadlock against the lane being held, but adding a network round
//   trip there would stall chatroom event processing for every member.
//
// The one timer-driven exception is `updateLastLoggedIn`, which fans out over
// every active group and usually turns out to be a `KV_NOOP`; it passes
// `preflight: false` selectively (see `needsHeightPreflight` in group-kv.js).
// A failed pre-flight is not fatal: the write is attempted anyway.
export const withKvGuard = async (
  contractID: string,
  key: string,
  write: () => Promise<any>,
  { preflight = true }: { preflight?: boolean } = {}
): Promise<any> => {
  if (preflight) {
    await ensureContractHeightCurrent(contractID).catch((e) => {
      console.warn(`[kv-guard.js] pre-write sync of ${contractID} failed; writing '${key}' anyway`, e)
    })
  }
  return withKvHeightRetry(contractID, key, write)
}

AI disclosure: Used Opus 5.5

[corrideat]

/review

---

AI review started.

The review job timed out.

[taoeffect]

The review job timed out.

Looks like this will need a local review with code-review via Opus 5.5. instead. Exceeds the timeout here with GLM-5.3.

[corrideat]

Opus 5.5 Review

Code Review

Base: main
Head: kv-improvements
Date: 2026-10-14
Model: claude-opus-5-5

---

Unreviewed files

The worker for this slice failed (a tool timeout), so these files were not reviewed:

• src/sync-contract.test.ts (added): slice slice-14, whole file
• src/test-utils.ts (modified): slice slice-14, whole file (includes until, within and the simulated KV server makeKvServer)

---

1. 🟡 After a recovery-sync timeout, the 30 s bound stops applying once the height has been reached

• Addressed
• Dismissed

KV_HEIGHT_RECOVERY_TIMEOUT_MS (src/kv-height.ts:47-50) is the only thing that stops a KV write from stalling the caller's lane forever when the write is awaited on another contract's lane. Group Income's case is identity KV written from a chatroom sideEffect or MESSAGE_RECEIVE_RAW handler. When the timeout fires, fail resolves whenever the contract has reached the required height (src/kv-height.ts:468-477, 483-486):

const reached = required !== undefined && isHeightReached(ctx, contractID, required)
...
if (reached) resolve()

withHeightRecovery (src/kv-height.ts:599-610) then runs the attempt again. The attempt goes through chelonia/queueInvocation, which first queues a noop on the contract's internal lane (src/chelonia.ts:1603-1605). That puts it behind the recovery sync that just timed out and is still holding the lane. The code says so itself (src/kv-height.ts:438-439, and src/kv.ts:4105-4114 for single-key sync). From then on the operation waits for that sync with no deadline.

"The height was reached but the sync is still running" is the typical state of a slow sync: it has applied events up to required and is stuck on a later one. Here is how that becomes a permanent deadlock:

1. A sideEffect of chatroom C holds C's lane and awaits an update on identity contract I.
2. I's recovery sync applies events past required, then blocks in one of I's sideEffects that waits on C's lane (for example a retain or sync of C).
3. After 30 s the timeout sees reached and resolves. The retry queues behind I's blocked sync, which waits on C, which waits on the retry.

If the timeout rejected, C's sideEffect would fail after 30 s and both lanes would drain. Since the retry can't run before the sync finishes anyway, the timeout path should always give up. Keep "reached anyway, retry" only on the sync-failure path, where the lane has already been released:

onTimeout: ({ reject }) => {
  console.warn(
    `[chelonia/kv] contract sync for ${contractID} did not finish within ${recoveryTimeoutMs} ms; ` +
    'giving up on height recovery (a retry would queue behind it)'
  )
  reject(error)
}

Single-key sync's tail (src/kv.ts:4112-4114) needs the same change, and the test that pins the current behaviour (src/kv-height.test.ts:497-513) has to be updated.

2. 🟡 Wrapped height errors are still the app's job, and the migration guide contradicts itself

• Addressed
• Dismissed

kv-guard.js's isHeightAheadError walks .cause. The branch fixes the library-side reason for that walk: heights are now classified before the lazy decode (classifyKvValue, src/chelonia.ts:3509-3528), and _loadSlotNow rethrows ChelErrorKvHeightAhead unwrapped (src/kv.ts:2733-2760). Everything else still only looks at the top-level error:

• The predicates. isKvHeightAhead, isKvHeightMalformed, kvHeightAheadCause and isKvConflict only check e.name on the error itself (src/errors.ts:95-133 via hasErrorName). src/errors.test.ts:132-143 asserts that a wrapped error does not match.
• The public recovery selector. chelonia/kv/withHeightRecovery recovers only when isKvHeightAhead(e) holds (src/kv-height.ts:606). An app attempt that rethrows new Error(..., { cause: e }) (Group Income's action wrappers do this) gets no recovery and no warning. recoverContractHeight also loses requiredHeight (src/kv-height.ts:465), so its "already reached, skip the sync" shortcut never applies.
• The docs disagree with each other:
  • docs/kv.md:375-377 says to replace ChelErrorInvalidMessageHeight checks "(with or without a walk along .cause) with isKvHeightAhead(e)".
  • The table at docs/kv.md:436 maps isHeightAheadError(e) one-to-one onto isKvHeightAhead(e).
  • docs/kv.md:445-458 then tells apps that wrap errors to copy a findCause loop.
  • AGENTS.md:237-238 says not to compare e.name yourself.

Following the first two instructions mechanically silently loses detection above any app wrapper. To make the guard fully redundant, ship the walk: export a bounded, cycle-safe finder next to the predicates, use it in withHeightRecovery, and point the docs at it.

// src/error-names.ts
export function findErrorByName<N extends string> (
  e: unknown, ...names: N[]
): (Error & { name: N }) | undefined {
  const seen = new Set<unknown>()
  for (let cur = e, depth = 0; cur && typeof cur === 'object' && depth < 8; depth++) {
    if (seen.has(cur)) return undefined
    seen.add(cur)
    if (hasErrorName(cur, ...names)) return cur
    cur = (cur as { cause?: unknown }).cause
  }
  return undefined
}

// src/errors.ts
export const findKvHeightAhead = (e: unknown) => findErrorByName(e, 'ChelErrorKvHeightAhead')
export const findKvConflict = (e: unknown) =>
  findErrorByName(e, 'ChelErrorKvConflict', 'ChelErrorKvMaxAttempts')

// src/kv-height.ts (withHeightRecovery): rethrow the caller's own `e`
const ahead = findKvHeightAhead(e)
if (!ahead || mode === 'reject' || recoveries >= maxRecoveries) throw e
...
await recoverContractHeight(ctx, contractID, e, {
  callerSignal: signal, sessionSignal, requiredHeight: kvHeightAheadCause(ahead)?.requiredHeight
})

Then:

• remove "(with or without a walk along .cause)" from docs/kv.md:375-376;
• replace the findCause snippet with the exported helper;
• turn the negative test into a positive one for the finder.

3. 🟡 queuedSet and withHeightRecovery awaited from a same-contract onUpdate hang forever, with no error

• Addressed
• Dismissed

update, clear, single-key sync and whenSettled reject a synchronous call from an onUpdate of the same contract with ChelErrorKvReentrant (assertNotReentrant, src/kv.ts:1549-1564). chelonia/kv/queuedSet (src/chelonia-utils.ts:59-121) and chelonia/kv/withHeightRecovery (src/kv.ts:2003-2017) don't. Both queue on the lane that safeOnUpdateGuarded is holding, so an awaited call never settles. The docs admit the gap (docs/kv.md:322-324; src/chelonia-utils.ts:40-42).

This is the only permanent lane stall in the KV API that has no timeout and isn't detected, and the PR exists to remove exactly that kind of app-side rule. Both selectors have the context they need. queuedSet is an arrow function, so turn it into a function (this: CheloniaContext, …), then export assertNotReentrant and extend its operation union:

// src/chelonia-utils.ts
'chelonia/kv/queuedSet': function (this: CheloniaContext, { contractID, key, /* … */ }) {
  try {
    assertNotReentrant(this, contractID, key, 'queuedSet')
  } catch (e) {
    return Promise.reject(e)
  }
  return sbp('chelonia/kv/withHeightRecovery', contractID, /* … */)
}

// src/kv.ts, 'chelonia/kv/withHeightRecovery'
if ((this.kvOnUpdateActive.get(contractID) ?? 0) > 0) {
  return Promise.reject(new ChelErrorKvReentrant(
    `[chelonia/kv] ${label ?? 'withHeightRecovery'}: called synchronously from an onUpdate ` +
    `callback of ${contractID}; awaiting it there would deadlock the contract lane`
  ))
}

Related: the comment above assertNotReentrant (src/kv.ts:1540-1548) still says "a KV write selector", although it now also guards whenSettled. Its message (:1557-1561) recommends queueMicrotask(() => sbp(...whenSettled...)), which still deadlocks if the callback awaits the result. Add "and don't await it from the callback".

4. 🟡 The docs say the recovery sync runs "outside its lanes" and is aborted by signal; neither is true, and the same-contract hang isn't mentioned

• Addressed
• Dismissed

Several places say the recovery syncs the contract "outside its lanes" (or "outside the queue lane") and that the sync is "aborted by chelonia/reset and signal":

• docs/api.md:197
• docs/kv.md:363, 408-410
• src/kv.ts:1990-1992
• src/chelonia-utils.ts:29
• src/types.ts:344

What the code does:

• The sync runs on a lane. startRecoverySync queues it on the contract's internal lane (src/kv-height.ts:402-413). Only the caller's wait holds no lane, which is how src/kv-height.ts:13-15 correctly puts it.
• signal doesn't stop the sync. It only ends this caller's wait (withDeadline in recoverContractHeight); the shared sync keeps running.

This matters for the deadlock requirement. Readers of those rows will conclude that withHeightRecovery is safe to await from a sideEffect, and it isn't:

• A queued attempt awaited from one of contractID's own sideEffects never runs.
• A raw-message handler for the same contract runs inside chelonia/queueInvocation (src/chelonia.ts:1211, 1266), so it holds the public lane. Every update, clear, queuedSet or withHeightRecovery it awaits for that contract also never settles.

docs/kv.md:317-325 lists the selectors that never settle there but leaves out withHeightRecovery. The note on update (src/kv.ts:3508-3511) covers sideEffects only. Suggested wording for the selector row:

While the attempt rejects with `ChelErrorKvHeightAhead`, it waits (holding no lane) for a
sync of the contract that runs on the contract's internal lane (one sync shared by
concurrent callers; `signal` and `chelonia/reset` end this caller's wait, not the shared
sync), and runs the attempt again … Never await it from one of `contractID`'s
`sideEffect`s, a raw pubsub handler for it, or a `queueInvocation` body for it.

The same behaviour is now described in five places (api.md, kv.md, kv.ts, kv-height.ts, chelonia-utils.ts), which is how they drifted apart. Keep one source of truth and link to it from the others.

5. ⚪ maxHeightRecoveries: 0 and the passive wait mean different things in different selectors

• Addressed

• Dismissed

• update, clear, queuedSet, withHeightRecovery. 0 means "wait passively, never sync" (src/kv-height.ts:563-573, 602).

• Single-key sync. 0 behaves exactly like 'reject': no wait and no sync (src/kv.ts:4058). On every other round it first waits passively (up to 1.5 s) and only then syncs (src/kv.ts:4066-4076). docs/kv.md:362-372, however, lists single-key sync among the selectors that sync "at once, without first waiting for a pubsub event".

• update's pre-reducer reload. It goes _loadSlotNow → kv/get, which rejects at once. The rejection is then rethrown (src/kv.ts:3689-3708) without regard to heightWaitMs. So when a pubsub frame has marked the slot ahead, update({ maxHeightRecoveries: 0 }) rejects at once instead of waiting passively, as KV-REVAMPED.md §4.2 step 5 promises.

An app that drops its guard and sets one value everywhere gets a different behaviour from each selector. Either align single-key sync and the pre-reducer reload with the writers (only the last round waits passively; 0 means "wait, don't sync"), or document the differences precisely in docs/kv.md and the spec.

6. ⚪ The kv-guard.js replacement table is inaccurate in three places

• Addressed

• Dismissed

• The conflict retry (docs/kv.md:437). The guard retried isKvConflictError after re-syncing the key. The table says "a larger maxAttempts" replaces that. For raw kv/set / queuedSet without onconflict, a 412 rejects on the first attempt (src/chelonia.ts:3272, !hasOnconflict || --maxAttempts <= 0), and withHeightRecovery only retries height errors (src/kv-height.ts:606). Say "pass an onconflict (or use a slot's update)", or add an opt-in conflict retry to withHeightRecovery.

• The pre-flight cost (docs/kv.md:372-374). "A current client … would pay for the pre-flight's sync on every write" is wrong. ensureContractHeightCurrent was throttled to once per 10 s per contract and only cost a latestHEADinfo request. Suggested text: "A pre-flight costs a latestHEADinfo round trip per throttle window even when the client is current; the selectors only pay for a sync after the server rejected a write (a stale client then pays for one rejected upload)."

• The hot-lane row (docs/kv.md:440). "Nothing: keep the default" understates how long an awaited write can hold the caller's lane. The guard did one sync and one retry. The default can do two recovery syncs (30 s each), then a last attempt with kv/set's passive waits: up to 5 × 1.5 s for stale stamps, plus up to 1.5 s for each 412 that is ahead. State that bound, and recommend not awaiting the write, or maxHeightRecoveries: 1 if it must be awaited. The same advice appears in KV-REVAMPED.md around lines 1016-1017.

7. ⚪ Raw KV handlers silently lose pubsub frames that are ahead of the local contract

• Addressed
• Dismissed

When a frame is ahead (src/chelonia.ts:1291-1321), only a slot gets a deferred reload (_registerHeightWait). A raw NOTIFICATION_TYPE.KV handler is never called for that frame, later or otherwise. There is a console.warn, but no event and no marker. An app that relies on the raw handler (no slot for the key) can't tell that a newer value exists, so it still needs its own polling or re-sync guard. The library has requiredHeight and the height-listener machinery, so it could at least announce the frame:

// events.ts
export const CHELONIA_KV_HEIGHT_AHEAD = 'chelonia-kv-height-ahead'
// chelonia.ts, `frame.kind === 'ahead'` branch
sbp('okTurtles.events/emit', CHELONIA_KV_HEIGHT_AHEAD, {
  contractID: msg.channelID, key: msg.key, requiredHeight: height
})

8. ⚪ The conflict-exhaustion cause can carry an earlier response's etag

• Addressed
• Dismissed

The comment at src/chelonia.ts:3273-3274 promises "that response's etag". But when the final read fails, src/chelonia.ts:3320 falls back to conflictEtag:

etag: (last ? last.etag : conflictEtag) ?? null

conflictEtag holds the etag of an earlier 412/409 or recovery GET. Example: POST → 412 (etag e1) → merge → POST → 412 with a non-JSON body and no etag header. The call rejects with cause.etag === 'e1', and a caller that retries with that if-match sends a stale precondition. Use only the final response:

etag: (last ? last.etag : responseEtag(response)) ?? null

After that, conflictEtag and its assignments (:3031, :3091, :3144, :3255) become dead code and can go.

A related inconsistency: after a recovery GET, heightAheadError(..., etag, response.status) (:3160, :3293-3295) reports the etag from the GET together with the original 412's status. That contradicts KvHeightAheadCause.httpStatus ("the response that revealed the height gap", src/types.ts:338).

9. ⚪ A deferred reload that verified nothing still clears the heightAhead marker

• Addressed
• Dismissed

fireHeightWait clears the marker whenever the reload resolves (src/kv.ts:1083-1086). _loadSlotNow already clears it itself when it verifies the key (dropMootWait), so this handler only matters when the reload resolved without verifying anything:

• the slot was replaced or released mid-load (src/kv.ts:2729-2731, 2780-2783);
• a filter vetoed chelonia/queueInvocation inside _loadSlot (src/kv.ts:2517-2526); Promise.resolve(undefined) then resolves.

In those cases mirrorBasisUnverified (src/kv.ts:1376-1390) turns false, and the next update runs its reducer on the stale mirror. That contradicts both the comment at :1044-1047 and the KvMirrorEntry.heightAhead docs (src/types.ts:297-298). Let _loadSlotNow be the only place that clears the marker:

wait.reload = reload
reload.catch((e: unknown) => {
  logBackgroundLoadRejection(`[chelonia/kv] deferred reload rejected for ${waitKey}`, e)
}).finally(() => {
  if (ctx.kvHeightReloadsQueued.get(waitKey) === token) ctx.kvHeightReloadsQueued.delete(waitKey)
})

Relatedly, settleUnstartedLoad (src/kv.ts:1108) only settles a 'loading' slot with no value. A re-activated slot that holds a value (settled: false) and whose deferred reload can't start is never settled, so whenSettled hangs. settleHeightAheadFailure's comment (:1114-1126) says the opposite should happen.

10. ⚪ Reset and restore edge cases around the height session and settled

• Addressed

• Dismissed

• Height-session hold leak. chelonia/reset takes a hold in _endHeightSession (src/chelonia.ts:778). Only the catch (:798) and _finishHeightSession (:824) release it. The synchronous teardown in between (:810-823) calls app-provided reactiveDel/reactiveSet. If one of them throws, the hold leaks, and every later reset aborts the fresh height session. All height recovery then rejects with AbortError until the page reloads. Release the hold in a finally.

• Stale settled after _init restore. chelonia/_init keeps a restored _kv (src/chelonia.ts:468-470), but only reset(newState) sets restored entries back to settled: false (:852-865). On the _init path, store readers that gate on settled && status !== 'loading' (docs/kv.md:89-93) can mistake an earlier session's flag for this one's. Reset settled in _init the same way.

• Stale comment. src/types.ts:287-289 still says a persisted entry "keeps its old value until its slot is activated again". That is no longer true after reset(newState).

11. ⚪ withHeightRecovery always syncs contractID and throws synchronously for null options

• Addressed

• Dismissed

• It syncs the wrong contract when the error came from another one. withHeightRecovery treats any ChelErrorKvHeightAhead as belonging to contractID (src/kv-height.ts:606-609). KvHeightAheadCause carries no contract ID, so the code can't tell. The docs make it the caller's job (docs/kv.md:417-419). An attempt that also reads another contract's key syncs the wrong contract up to maxHeightRecoveries times. The library knows the contract when it builds the error; add contractID (and key) to the cause and rethrow when it doesn't match.

• null options throw synchronously. { label, ...options } = {} (src/kv.ts:2007) only applies the default for undefined. A null third argument throws a synchronous TypeError out of sbp(...), even though the selector carefully returns Promise.reject for a bad attempt. Use opts ?? {}.

12. ⚪ The public types for the new API aren't exported

• Addressed

• Dismissed

• chelonia/kv/withHeightRecovery is public, but KvHeightRecoveryOptions and KvHeightRecoveryAttempt live in src/kv-height.ts, which is deliberately not re-exported. TypeScript apps can't name them. src/chelonia-utils.ts:86-88 re-declares the attempt type inline, so the two can drift apart.

• isKvConflict (src/errors.ts:129-133) documents .cause as { currentStatus, currentData, etag }, but its narrowed type has no cause. The shape exists only as the private KvConflictCause (src/kv.ts:1393-1397).

Move all three types to src/types.ts, narrow isKvConflict to cause?: KvConflictCause, and use KvHeightRecoveryAttempt in queuedSet.

13. ⚪ kv/set edge cases: retries during a catch-up sync, and unvalidated maxAttempts

• Addressed

• Dismissed

• Repeated stale uploads during a catch-up sync. After a 409, kv/set waits only for postedHeight + 1 (src/chelonia.ts:3256-3266), and height listeners fire after every applied event (src/internals.ts:3549-3552). While a catch-up sync replays a backlog, the waiter wakes at the next replayed event and re-sends at a height that is still stale. That repeats up to KV_MAX_STALE_STAMP_RETRIES (5) full uploads, then rejects just before the sync would have caught up. While this.currentSyncs[contractID] is set, wait for the internal lane to drain instead of for +1.

• maxAttempts isn't validated. heightWaitMs is now validated (:3008-3013), but maxAttempts isn't (:3014). With maxAttempts: NaN, --maxAttempts <= 0 is never true, so a contended key loops on 412 forever.

14. ⚪ Stale or inaccurate comments and messages

• Addressed

• Dismissed

• src/types.ts:325-329. The comment says requiredHeight is the target of the passive wait, the forced syncs "and of the slot's heightAhead marker (all bounded)". The marker is not bounded (KV-REVAMPED.md §12.10; src/kv.ts:1376-1389). This sits next to the malicious-server discussion, so it shouldn't promise a bound that doesn't exist.

• src/kv.ts:3806-3809. The comment still cites lastRecoveredValue, which this branch removed from kv/set.

• src/chelonia.ts:820-821. The comment says the maps are cleared "alongside subscriptionSet below". clearKvRuntime actually runs on the next line, before subscriptionSet.clear().

• src/chelonia-utils.ts:6-12. The header says the file uses "only public Chelonia selectors", but it now imports the internal isKvMaxAttempts.

• src/chelonia-utils.ts:33-36. "…so only when the caller re-issues it itself" has no main verb.

• src/chelonia.ts:3519-3523. The malformed-stamp message gets a doubled prefix: [chelonia/kv] cid::key: [chelonia] Invalid height stamp ….

• src/kv.ts:955-957. "the slot … isn't active for this contract while waiting for it to settle" reads oddly. Prefer stopped waiting for ${waitKey} to settle: the slot ${why}.

• docs/kv.md:91. "hasn't told anything yet" should be "hasn't reported an outcome yet".

• AGENTS.md:244-245. isKvConflict also matches a 412 without onconflict, not only "a conflict that ran out of attempts".

15. ⚪ KV-REVAMPED.md contradicts the code or itself in several places

• Addressed

• Dismissed

• :662-663, :970-971. The spec says "a 409 re-sends the same value". With the defaults, every non-last attempt passes heightWaitMs: 0, so a 409 rejects at once and the recovery re-runs the whole body, reducer included (src/kv-height.ts:602, src/chelonia.ts:3264-3265).

• :851. The AbortError row says "the call itself changes nothing else". Step 5a (:1027-1030) and runSlotWrite (src/kv.ts:1626-1631) register a height wait, which sets the marker and can move a 'non-init' slot to 'loading'.

• :246, :1696-1699. The spec says "a reload settles the slot" and that a KV_NOOP update settles it. The silent data-loss-guard reload doesn't settle it (src/kv.ts:2653-2658).

• :1745-1746. It says other slots load on their first update. §3.3 and step 2 say update doesn't reload an unsettled slot.

• :367-374. It implies a passive wait with allowUnverifiedConflict. A blind writer skips the wait (src/chelonia.ts:3078).

• :1474-1485. §4.9 leaves out that a committed local write also clears the marker.

• :461-473. §3.5 sends readers to §4.2 step 5a for chelonia/kv/withHeightRecovery, but no section documents that public selector's signature and rules.

• :28-37. §1.1 "The KV store today" now mixes post-revamp rows with the pre-revamp "no built-in event … no local state mirror".

16. ⚪ Test quality: unbounded awaits, a test that can't fail, and duplicated setup

• Addressed

• Dismissed

• Unbounded awaits. AGENTS.md requires within(...) for promises that might never settle. These tests await kv/set directly while the only thing that settles it is the timer or height listener under test:

  • src/chelonia-kv-set.test.ts:404-437, 439-468, 740-777, 848-909, 911-999, 1036-1086
  • src/kv-height-events.test.ts:67, 153

  A regression there would only fail at the 60 s runner timeout, with no label. The setTimeout at src/kv-height-events.test.ts:130 is never cleared.

• A test that can't fail. src/kv-height-pubsub.test.ts:202-218 asserts that a 409 reaches the sync within 500 ms. It keeps the harness's 50 ms waitMs (src/kv-height-harness.ts:51), and the closed socket skips the wait anyway, so a regression to "passive wait first" still passes. Set waitMs: 1500 as the sibling tests do.

• A control test that controls nothing. src/kv-height.test.ts:3819-3825 uses a different setup (twoKeysAhead) from the test it controls (:3757-3817), so it only re-checks the ordinary one-recovery path. Share 3757's setup and delete twoKeysAhead.

• Tests don't use the public predicates. Many tests check instanceof ChelErrorKvHeightAhead / ChelErrorKvConflict instead of the predicates apps are told to use:

  • src/kv-height.test.ts:217, 233, 263-312, 666, 777, 3544-3641
  • src/chelonia-kv-set.test.ts:333-399, 1078-1082

  So the realm-safe path apps depend on is barely exercised.

• No fetch-first tests. kv/set with data === undefined (absent / ahead / present) has no tests, although this branch rewrote that path.

• Duplicated setup. The update / clear / queuedSet write table is copied three times (src/kv-height.test.ts:897-905, 941-949, 966-977). The "NS_CACHE written ahead by device B, then subscribe" setup and the merge callbacks are repeated about eight times. Extract helpers.

17. ⚪ Permanently removing a contract that never finished syncing isn't announced (low confidence)

• Addressed
• Dismissed

At src/internals.ts:683-692, removeContractState(..., permanent) writes the null marker into state.contracts, but CONTRACTS_MODIFIED isn't emitted. chelonia/externalStateSetup (src/local-selectors/index.ts:154-160) only learns about removals from that event. After a failed re-sync, the store's last word for the contract is removed with resync: true, so it never learns the contract is gone for good. The comment's reasoning ("already announced") covers only the temporary removal. Consider emitting { removed: [contractID], permanent: true } when params?.permanent is set.

[taoeffect]

Note the unreviewed file it mentions:

The worker for this slice failed (a tool timeout), so these files were not reviewed:

• src/sync-contract.test.ts (added): slice slice-14, whole file
• src/test-utils.ts (modified): slice slice-14, whole file (includes until, within and the simulated KV server makeKvServer

[corrideat]

Opus 5.5 generated PR description

PR #107: what it does and why

Why

Group Income kept hitting the same limits in @chelonia/lib's key/value (KV) API, so its PR #3137 added a workaround file, kv-guard.js. This PR moves those fixes into the library, so that Group Income can delete the file and other apps never need one.

The guard worked around three problems:

1. Contract heights. Every KV value is stamped with the contract height it was written at, and a client can only check a value once its local copy of the contract has reached that height. The server also rejects writes stamped with an out-of-date height (409). Apps had to:
   • spot these errors by walking .cause chains and matching error names;
   • sync the contract by hand, sometimes before every write;
   • retry.
2. Conflicts. 412 conflicts needed extra app-side retries.
3. Empty vs. not loaded yet. A slot whose key holds no value looked the same as one that simply hadn't loaded, or whose value couldn't be checked yet.

What it changes

Errors

• kv/get and kv/set now reject with two new errors:
  • ChelErrorKvHeightAhead: the value is ahead of the local contract, and syncing helps.
  • ChelErrorKvHeightMalformed: the height stamp is missing or broken, and syncing never helps.
• New checks isKvHeightAhead, isKvHeightMalformed and isKvConflict match by error name, so they work across bundles where instanceof fails.

Built-in height recovery

• update, clear, queuedSet and single-key sync now recover on their own: they sync the contract and retry.
  • Two new options control this: onHeightAhead ('sync' or 'reject') and maxHeightRecoveries.
  • The recovery doesn't block the contract's queue, and writes recovering at the same time share one sync.
• kv/set re-signs after a 409 once the local contract catches up, and can wait for a sync that is still running.
• Conflict handlers can now tell whether the server value is 'absent', 'present', 'ahead' or 'malformed'.
• A new selector, chelonia/kv/withHeightRecovery, gives raw kv/get / kv/set callers the same recovery.

Empty vs. not loaded yet

• Each mirror entry gains two fields:
  • settled: the slot has reached an outcome;
  • heightAhead: the server holds a newer value this client can't check yet.
• Two new selectors use them: chelonia/kv/settledStatus and chelonia/kv/whenSettled.
• A load or pubsub message that finds a value that is ahead keeps the slot pending and reloads it once the contract catches up. If that doesn't happen, the slot falls back to a forced sync after 10 s, then to 'error'.

Safety around lifecycle

• Calling a KV write synchronously from the same contract's onUpdate now rejects with ChelErrorKvReentrant instead of hanging.
• A write's signal now cancels it while it is still queued, and chelonia/reset cancels queued writes and ends pending height waits.
• KV state is now cleaned up when a contract is removed or resynced, including a contract whose first sync never finished.

Supporting work

• The code is split into new internal modules: kv-height.ts, kv-runtime.ts and error-names.ts.
• docs/kv.md gains a section mapping each kv-guard.js helper to its replacement; docs/api.md and the KV-REVAMPED spec are updated too.
• About 7,000 lines of new tests, plus a shared test harness and helpers that make a hung test fail instead of stalling.
• One unrelated change: in signedData.ts, the "fail loudly in CI" behaviour for an unauthorized signing key now only fires in the host app's CI, not in this library's own test runs.

[taoeffect]

This comment is way too long. Check other places too for similar nonsense.

[taoeffect]

/review {{kimi}}

---

AI review started.

[github-actions]

EDIT by @taoeffect: The Kimi plan got used up with that one review. I had 11% remaining when it started, so it's theoretically possible it might've finished with 0% usage at the start, but this would mean either way that we would get at most 1 kimi review per 5 hours with this plan.

Advanced AI Review

• Type: Agentic (crush)
• Model: k3.1-pro-highspeed

Click to expand review

Agentic review unavailable (crush produced no review).

---

Review generated using k3.1-pro-highspeed via Kimi. Comment /review {{kimi}} to re-run.

[taoeffect]

/review {{kimi}} - let me try one more time with 0% usage to see what happens

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: k3.1-pro-highspeed

Click to expand review

Agentic review unavailable (crush produced no review).

---

Review generated using k3.1-pro-highspeed via Kimi. Comment /review {{kimi}} to re-run.

[corrideat]

Replaced by #118 and #117