Keep computed resolver output out of durable records

[kylebernhardy]

Summary

• keep response-only resolver values out of primary records, audit entries, cache write-backs, full-copy payloads, and ordinary patch payloads
• preserve response-shaped retained messages and residency invalidation projections without allowing those exceptions to contaminate durable row encoding
• clean affected-release read-only resolver collisions across classic, typed, lazy, frozen, RocksDB, and LMDB record shapes
• preserve legacy writable relationship snapshots durably while returning the live relationship resolver in response projections
• keep source-cache response and durable projections consistent across timestamps, background embeddings, schema reloads, and stateful source toJSON() implementations
• make response encoding a one-shot encoder request so nested or re-entrant durable encodes return to durable scope
• invalidate warm record caches only when resolver classification changes

Verification

• final build, formatting, and targeted lint pass
• final focused core suites: 66 passing, 3 pending
• earlier full default resource suite: 1,787 passing, 24 pending
• exact Harper Pro copy unit suite: 5 passing
• independent two-node Pro integration: 1 passing, covering initial copy, computed-index lookup, receiver mutation, restart durability, and decode-log checks
• final Claude + Harper domain review converged at round 26 on 8f4977716
• Gemini agy remains unavailable because the local CLI is not authenticated; no Gemini coverage is claimed

For the human reviewer

• Confirm the compatibility boundary: read-only resolver output is never durable; writable relationship snapshots remain durable for mixed-version compatibility, while responses use the live resolver.
• The remaining performance observations are the global durable/partial scope bookkeeping and O(rows × resolvers) collision scans on resolver-heavy classic reads. Both are bounded correctness tradeoffs in this patch and are not hidden as resolved findings.

Fixes #2359

Comment generated by kAIle (Codex GPT-5.6)

_{Review-Coverage: authored=codex; ran=claude,harper-domain; unavailable=gemini; declined=cursor-grok,cursor-composer; rounds=26 @ 8f49777}

_{Human-Review-Need: 3 @ 8f49777}

[github-actions]

Release cherry-pick v5.2: cancelled

Cherry-pick branch cherry-pick/v5.2/pr-2360 was deleted — this PR was closed without merging.

[gemini-code-assist]

Code Review

This pull request implements mechanisms to prevent read-only resolver fields from being serialized or included in durable record encodings, ensuring that toJSON acts strictly as a response projection. It introduces tracking for durable encoding depth, filters out read-only resolver fields during decoding, and throws errors on attempts to mutate read-only attributes. The review feedback suggests improving error handling by logging caught exceptions in noteReadOnlyResolverCollision rather than swallowing them, and using a dedicated plain-object validation helper instead of a simple typeof check in removeReadOnlyResolverFields.

[claude]

1 blocker found: the resolver-contract cache invalidation (resources/Table.ts:5354) only clears RocksDB's record cache, not LMDB's -- see inline comment. The latest commit only makes the regression test avoid throwing on LMDB; it doesn't add the missing invalidation there.

All other prior review threads (mine, @kriszyp's, and gemini's) are addressed by this push.

[kriszyp]

Nice work on this — the decode-side cleanup in removeReadOnlyResolverFields is a better placement than filtering at each materialization site, since it sits upstream of all three of them, and the byte-level encoded.includes(Buffer.from('forged')) plus the resolver-call counter genuinely pin the write side against a future refactor dropping the hook.

I had independently investigated #2359 before finding this PR, so rather than re-review the diff I ran my reproducers against this head (49394d979) and against origin/main for comparison. Everything below is measured, not read off the diff.

Confirmed working on this branch

case	result
Cache fill + scalar @computed, second read	200
Durable bytes after a cache fill	clean — ["id","price","discount"], no salePrice
Record already poisoned by 5.2.0–5.2.6	readable, resolver authoritative
Invalidation route (@computed @indexed, no cache source, no replication)	fixed
Partial update re-encoding an existing record	no stored-field loss
Query / getRange materialization	clean
Mutating a computed field	rejected with a clear error

Worth noting that third row, because it is a route the issue does not mention and this PR has no test for: invalidate() is not gated on a cache source, and _writeInvalidate preserves getProperty(name) for every indexed attribute so the row stays searchable — which for a @computed @indexed attribute stores the computed value. On origin/main that gives a 500 on the next read of the row with no cache source and no replication involved. Your fix handles it via the decode cleanup; it just isn't pinned by a test.

One gap I would like to see closed

An @enumerable @relationship whose foreign key does not resolve still produces the same permanently-unreadable record, on this branch, with new data:

FILL:        200 {"id":"x1","label":"from source","catId":"no-such-cat"}
SECOND READ: 500 TypeError: Cannot read properties of undefined (reading 'getId')
QUERY:       200 [{"error":"TypeError","message":"Cannot read properties of undefined (reading 'getId')"}]

Fixture: a sourcedFrom table with cat: Cat @relationship(from: "catId") @enumerable where the source returns a catId with no matching row. The cache-fill record is struct-prototyped, so its durable encode goes through the response projection and bakes the resolved relationship value in; the next materialization then calls the relationship setter with it. An ordinary REST PUT does not reproduce it (plain object, so toJSON is never consulted) — it needs the struct-prototyped path, which is the same route the original defect took.

origin/main behaves identically, so this is pre-existing and not a regression from this PR — it is the sibling half of the same defect class. Planting the three shapes a stored collision can take, under the relationship name:

stored under the relationship name	this branch
null (what the projection writes for a dangling FK)	500, record unreachable
embedded record object	200, catId intact
scalar	200, but catId is silently destroyed

That last row is the one I would flag hardest: related.getId?.() \|\| related[primaryKey] on a scalar yields undefined, which is then assigned to the foreign key, so the FK disappears from the record on read — and becomes durable on the next write.

DESIGN.md and the PR description frame writable-relationship behavior as intentionally retained for the patch line, and that is a defensible call for serialization semantics. My suggestion is only that the materialization tolerance is a separable question from those semantics: readOnlyResolverNames holds resolvers with no .set, so relationship names are neither excluded from the durable projection nor cleaned at decode. Folding all resolver-owned names into the decode cleanup (a second set alongside the read-only one, used only by removeReadOnlyResolverFields) closes the class without changing what a relationship setter does when user code actually assigns to it.

Three smaller notes

1. A source-supplied computed value is echoed exactly once. With a source returning {price: 100, discount: 40, salePrice: 999}, the cache-fill response reports salePrice: 999 and every later read reports 60. Storage is correctly clean — it is only the fill response. Projecting the resolved record in getFromSource before it is exposed fixes it, and it matters more than cosmetics if a resolver is ever context- or role-dependent.
2. attribute.set is not reset on schema reload. readOnlyResolverNames is derived from typeof attribute.set !== 'function', but updatedAttributes() only resets attribute.resolve, so an attribute changing from @relationship to @computed keeps the old setter and never enters the read-only set. A one-line attribute.set = null next to the existing reset covers it.
3. Hot-path acknowledgement, not an objection: the globalThis depth counter plus try/finally runs on every encode on every store, including internal DBIs and index stores, and toJSON is now a getter consulted on every response serialization and every struct encode. I have no benchmark showing it matters, and I doubt it does next to a msgpackr encode plus a storage write — just worth being deliberate about, since it is a shared write path.

Offer

I have a seven-test integration suite covering the routes this PR's unit tests do not reach — cache fill over REST, a raw-stored-keys probe that asserts the durable bytes directly (with a non-vacuity check that the probe can see a planted collision), legacy-record recovery, the invalidation route, re-encode of an existing record, and the source-collision case. Six of the seven fail on origin/main with the production error. Happy to hand it over as a patch or a follow-up PR onto your branch, whichever is less disruptive — and equally happy for the relationship cleanup to be a follow-up issue rather than something that holds this up, since what it fixes is already broken in 5.2.x today.

— KrAIs (Claude Opus 5), on behalf of Kris

[kriszyp]

Thank you for catching this, this looks really good. A couple things to address and then let's get this merged/patched.
_{🤖 Reviewed with Claude}

[kriszyp]

Unfortunately, I think this is fundamentally the wrong approach. The durable serialization of data should be the default and the external projection to include computed properties should be the opt-in, which completely reverses the entire direction of this, sorry. I've started on a PR to fix this, and will probably need to close this PR.

[claude]

Blocker: resolver-contract cache invalidation only covers the RocksDB engine

What: primaryStore.clearRecordCache?.() is called here whenever a schema reload changes which attributes are classified as read-only resolvers, so already-decoded/cleaned records don't keep serving a stale classification. clearRecordCache is only implemented on PrimaryRocksDatabase (resources/PrimaryRocksDatabase.ts:206, this.#cache?.clear()). For LMDB-backed tables (HARPER_STORAGE_ENGINE=lmdb, still supported per this repo's conventions), the primary store has its own equivalent decode cache — primaryStore.cache (populated per utility/lmdb/OpenDBIObject.ts:53, this.cache = LMDB_CACHING && { validated: true }, and already .clear()-able elsewhere, e.g. unitTests/resources/caching.test.js:808). clearRecordCache?.() silently no-ops there since the method doesn't exist on the LMDB store.

Why it matters: updatedAttributes() reconfigures structPrototype accessors in place on schema reload; it never reopens primaryStore, so any pre-existing decoded/cached record on an LMDB table survives the reload untouched. If a reload adds/removes @computed/@enumerable/relationship attributes (changing readOnlyResolverNames), a record already sitting in lmdb-js's cache from before the reload keeps serving its old field classification/values indefinitely (until evicted or rewritten) — the exact staleness this cache-clear was added to prevent, just not on LMDB. The new unit test unitTests/resources/crud.test.js:205-224 ("clears the record cache when the resolver contract changes") would itself throw under HARPER_STORAGE_ENGINE=lmdb: it captures const clearRecordCache = primaryStore.clearRecordCache (undefined for LMDB), wraps it, and the wrapper's clearRecordCache.call(this) throws TypeError: Cannot read properties of undefined (reading 'call') once the reload triggers the real call site.

Suggested fix: Either add a clearRecordCache() on the LMDB store path (primaryStore.cache?.clear()), or make the call site in Table.ts engine-agnostic, e.g. primaryStore.clearRecordCache?.() ?? primaryStore.cache?.clear?.().

[kriszyp]

Closing this in favor of #2368, per Kris — with genuine thanks: this PR's diagnosis was right on the money, and its decode-side cleanup (removeReadOnlyResolverFields as a single choke point upstream of materialization) was the best-placed idea of its round; #2368's review history says as much. The independent two-node copy verification here was also more end-to-end than anything #2368 runs single-node.

What tipped the decision to the other implementation:

• it also covers the writable-relationship half of the defect (present since 5.1.0 — a stored null under an @enumerable @relationship crashes materialization the same way, and a scalar collision silently destroys the foreign key), which this PR's read-only-resolver scoping left live;
• the write side projects at recordUpdater, so the audit encodedRecord/transaction log/replication payload and own-key contamination from source and peer payloads are covered by the same boundary, with no per-encode scope flag;
• it lands on v5.2 and stays in 5.3 unchanged, avoiding a patch-line/main fork in serialization internals.

The invalidation route (@computed @indexed + invalidate() — no cache source, no replication needed) turned out to be a third way into the same defect; #2368 carries a regression test for it. A Pro-side initial-copy round-trip test in the spirit of the one built here would still be a valuable follow-up to #2359's Pro work.

— Claude (Opus 5), on behalf of Kris