fix(sync): complete selected public VM catch-up efficiently

[branarakic]

User impact

An edge node selecting a public context graph now uses the chain as the authoritative VM inventory and actively reconciles missing finalized assets. Providers retain only the bounded exact graph plan across wire pages, avoiding repeated full manifest scans while keeping payload rows page-only.

RFC-64 catalogs remain SWM-only. This PR does not create a second VM catalog and does not sync every public context graph.

Before

sequenceDiagram
    participant E as Selected edge node
    participant C as Chain
    participant P as Provider
    E->>P: Recover selected SWM from RFC-64 catalog
    E->>C: Observe finalized VM inventory gradually
    loop Every 64-row exact page
        E->>P: Request next exact VM page
        P->>P: Rebuild exact manifest plan
        P-->>E: One page
    end
    Note over E,P: VM completion can lag or time out

Loading

After

sequenceDiagram
    participant E as Selected edge node
    participant C as Chain
    participant P as Provider
    E->>P: Recover selected SWM from RFC-64 catalog
    E->>C: Read bounded finalized VM inventory slice
    E->>P: Request missing exact VM assets
    P->>P: Build and retain bounded graph plan
    loop Conservative 64-row payload pages
        P-->>E: Next verified page
    end
    E->>E: Verify and materialize exact finalized graphs

Loading

Safety

• Selection remains explicit and CG-scoped; there is no sync-all edge behavior.
• The blockchain remains the VM inventory and integrity authority.
• The retained plan contains graph IRIs and committed counts only, never payload rows.
• Existing authentication, paging, digest checks, and false-signature protections remain in force.
• Reconciliation is one bounded pass per selected-CG scheduling event and coalesces duplicate work.

Validation

• Focused exact-fetch and coalescing suites: 47/47 pass.
• Broader requester, responder, and lifecycle slice: 95/95 pass.
• Agent build, type tests, package-root test, CLI build, and diff check pass.
• Testnet local receiver, cold from the corpus perspective and without manual catch-up: SWM 500/500 and VM 500/500 on exact commit 74414be.
• The final remote-receiver run is still required before release certification.

[otReviewAgent]

🟡 Issue: Exact-plan cache can reuse a superseded in-flight session plan

What's wrong
The new plan cache is described as session-scoped, but the key is only peer/context/selection scoped and the underlying exact-plan memo coalesces in-flight loads before checking refresh. A new sync session can therefore inherit a plan started for a previous session, which breaks the intended page-zero snapshot boundary and can serve stale empty/count metadata for exact asset reads.

Example
Peer P starts an exact asset page-0 request for CG C and asset A, creating an in-flight plan under durable-data:P:C:exact:A. Before that load completes, P starts a new page-0 session for the same asset after local manifest state changed. prepareResponderSession sets refreshRowList=true, but createSessionPlanMemo returns the old in-flight promise for the same key, so the new session can reuse a plan from the superseded session instead of rebuilding at its own boundary.

Suggested direction
Carry refreshGeneration into the exact-plan memo path, or key the plan by the server-derived session generation, so a new offset-0 session does not coalesce with an older pending load for the same peer/CG/asset selection.

Confidence note
This depends on overlapping sync sessions for the same peer/context graph/asset selection while the first exact-plan load is still in flight, but the session code explicitly supports superseding page-zero sessions.

For Agents
In packages/agent/src/sync/responder/graph-plan.ts, make exactGraphPlanCacheScope honor the same session generation semantics as row-list caches. Either include the sync session generation/token in the exact-plan cache key or extend ExactGraphPagePlanMemo/createSessionPlanMemo to track refreshGeneration and avoid joining stale in-flight loads when a refresh supersedes them. Add a concurrency test with two offset-0 sessions for the same peer/asset where the second refresh must not observe the first session's pending plan.

[otReviewAgent]

🟡 Issue: Do not duplicate the session-generation memo state machine

What's wrong
This adds another copy of a subtle concurrency state machine. The logic is not just cache plumbing; it encodes session-boundary correctness around aborted callers, superseded in-flight loads, and stale cached entries. Duplicating that policy increases drift risk and makes future cache behavior harder to audit.

Example
The loop at graph-plan.ts:433 mirrors the snapshot-cache generation fence: compare refreshGeneration, wait for older in-flight work, ignore older failure unless this caller aborted, then rebuild. If the retry/abort/expiry policy changes, maintainers must now update separate implementations for row snapshots, graph lists, subgraph names, and exact plans.

Suggested direction
Pull the generation-aware cache/in-flight handling into one small reusable primitive, or make createSessionPlanMemo the canonical primitive and reuse it for the other scalar plan caches. The important part is that refreshGeneration semantics live in one implementation.

For Agents
Look at createSessionPlanMemo in graph-plan.ts and createResponderSyncRowListMemo in snapshot-cache.ts. Preserve generation isolation for newer sessions, but extract a shared generation-aware memo primitive or helper for the common in-flight/cached entry policy, then have plan/list/subgraph memos use that instead of duplicating the state machine.

[otReviewAgent]

🟡 Issue: Make session generation part of the cache identity instead of memo side state

What's wrong
The new refreshGeneration plumbing gives the memo two identities for the same entry: the string key and a separate generation field. That forces custom supersession, replacement, and option threading through otherwise generic helpers. A cleaner model is to put the generation into the cache key so ordinary map semantics do the work.

Example
The new concurrent-interleaving test has old-session and new-session using the same key. Because generation is side metadata instead of part of the key, the newer request must wait for the old pending promise to settle, discard it, then call load again. If the key were generation-scoped, same-generation requests would coalesce and different-generation requests would naturally be distinct cache entries.

Suggested direction
Compose the cache key from the durable key plus session scope/generation, or introduce a small typed helper that builds that key. Then the generic memo can stay a simple key-to-inflight/value cache and does not need to understand refresh generations.

Confidence note
The pattern mirrors nearby memo code, but this PR extends it into the exact-plan memo and threads it through several more APIs, so the newly added surface is still worth tightening.

For Agents
Look at createSessionPlanMemo, createSessionPlanGetter, readDurableDataPage, and sync-handler's exactGraphPlanCacheScope wiring. Preserve: exact payload rows remain page-only, the exact manifest plan is reused within one authenticated session, and a newer session must not inherit a superseded pending plan. Refactor toward a composed session cache key that includes scope/generation where needed, then remove refreshGeneration from the generic memo API and its supersession loop.

[otReviewAgent]

🟡 Issue: Cached exact-plan generation replacement is not directly tested

What's wrong
This change adds generation-aware replacement for session plan cache entries, but the added tests only prove that a newer session does not inherit an older pending load. If the cached-entry generation check regressed, the current tests could still pass while a new exact VM session reuses a stale manifest/count plan from a previous session.

Example
Failing-test sketch: call createResponderExactGraphPagePlanMemo().get(key, loadOld, { refresh: true, refreshGeneration: 'old' }) and resolve it; then call get(key, loadNew, { refresh: true, refreshGeneration: 'new' }). The second call should invoke loadNew and return the new entries, not the cached old entries.

Suggested direction
Add a regression test for the cached-plan path, not only the pending-plan path.

For Agents
Look in packages/agent/test/sync-responder-concurrent-interleaving.test.ts near the new exact-plan memo test. Add a cached-generation regression case, or an integration responder test with two page-zero exact sessions where the manifest changes between sessions. Preserve same-generation reuse, but prove a different refreshGeneration reloads the exact graph plan after the old plan is already cached.

[otReviewAgent]

🟡 Issue: Session-generation cache invalidation is duplicated instead of centralized

What's wrong
The new refreshGeneration path copies a subtle concurrency contract into another responder memo. The code is correct-looking but expensive to reason about: in-flight superseding, abort behavior, stale cache eviction, TTL, and optional budget accounting are all interleaved in a 4k+ line planner module. This is exactly the kind of logic that should have one canonical implementation.

Example
The same invariant now has several hand-written versions: a newer page-zero generation must wait for older in-flight work, must not inherit the older value or failure, and must evict stale cached state. Any future tweak to that contract has to be re-applied across multiple memo implementations.

Suggested direction
Extract a canonical generation-aware async memo/cache primitive, with hooks for cloning values, LRU/budget accounting, and expiration policy. Then make the graph-list, subgraph-name, exact-plan, and snapshot caches small adapters over that primitive.

Confidence note
This is a structural concern from inspecting the local cache implementations; it may be acceptable if the team intentionally wants each memo to stay self-contained, but the duplicated generation invalidation logic is already substantial.

For Agents
Start with createSessionPlanMemo in graph-plan.ts and compare it with snapshot-cache.ts generation handling. Preserve TTL, LRU/budget accounting, requireExisting, and abort-racing behavior. The interleaving tests around exact-plan session superseding should continue to pass.

[otReviewAgent]

🟡 Issue: Keep RFC-64 VM recovery behind the durable recovery abstraction

What's wrong
This adds feature-specific recovery orchestration to an already large lifecycle method and creates a parallel implementation of the durable recovery result contract. That makes future changes to recovery accounting, coordination, or peer attribution easy to apply to one path and miss in the other.

Example
If DurableRecoveryExecution grows another coordinated field or the runner changes how it aggregates peer progress, the normal path gets that from DurableRecoveryRunner.finish, while this RFC-64 path has to be updated separately. It is already synthesizing a peer result for a peer that did not serve the VM bytes, which is a sign the abstraction boundary is being bent.

Suggested direction
Instead of an inline special-case branch in the lifecycle method, add a small strategy/hook to the durable recovery runner for chain-inventory recovery, or extract a dedicated runner that shares the same result assembly and coordination policy. The lifecycle layer should select the recovery mode; it should not duplicate the runner’s bookkeeping contract.

For Agents
Look at syncDurableRecoveryContextGraph and sync/durable-recovery-runner.ts. Preserve the behavior that selected public RFC-64 CGs run one bounded runVmReconcileForCg(contextGraphId, 'manual') slice instead of a broad durable pull, but move this behind a recovery strategy/dependency so the runner/coordinator remains the only place constructing DurableRecoveryExecution. Keep the existing coalescing test proving broad durable sync is not called.

[otReviewAgent]

🟡 Issue: Selected-public VM recovery is bolted into the generic durable recovery entrypoint

What's wrong
This adds a special-case lane directly inside the generic durable recovery orchestration and duplicates the durable result/peer result/outcome construction outside the durable recovery runner. The code works locally, but it makes durable recovery harder to evolve because there are now two places that manufacture the same execution contract.

Example
A future change that adds a field or changes outcome accounting in DurableRecoveryRunner.finish must now remember to update this RFC-64 branch too; otherwise selected-public VM recovery and ordinary durable recovery silently diverge in accounting shape.

Suggested direction
Keep the behavior, but make selected-public VM recovery a first-class strategy/runner or shared durable-recovery adapter instead of an early feature-specific branch in the 10k-line lifecycle method.

For Agents
Look at packages/agent/src/dkg-agent-lifecycle.ts around syncDurableRecoveryContextGraph and packages/agent/src/sync/durable-recovery-runner.ts. Preserve the selected-public behavior of using one VM reconcile slice instead of a broad peer pull, but move the execution/result-shaping boundary into a dedicated selected-public recovery runner or shared DurableRecoveryExecution factory, with coverage proving selected-public recovery still reports partial and terminal outcomes.

[otReviewAgent]

🟡 Issue: Keep selected-public recovery inside the durable recovery abstraction

What's wrong
This is a structural fork of a complex orchestration path inside an already very large lifecycle file. The comment explains the behavior, but the implementation embeds feature-specific recovery semantics directly into the public durable recovery entrypoint and duplicates runner-owned accounting. That makes later changes to durable recovery harder to reason about because there are now two places that manufacture the same execution shape.

Example
The branch now locally decides outcome, peerResults, slices, safeOffset, failed-peer accounting, and terminal mapping. Any future change to DurableRecoveryRunner's execution shape or accounting has to be remembered here too, or selected-public recovery silently diverges.

Suggested direction
Instead of forking the runner inline, teach DurableRecoveryRunner about an alternate slice provider or extract a small selected-public recovery executor that reuses the same finish/accounting path. The lifecycle method should select the strategy, not duplicate the orchestration contract.

For Agents
Look at syncDurableRecoveryContextGraph and packages/agent/src/sync/durable-recovery-runner.ts. Preserve the behavior that selected public VM catch-up uses chain inventory and avoids the broad durable pull, but move this behind a recovery strategy/runner dependency or shared result builder so the runner remains the single owner of DurableRecoveryExecution accounting. Existing selected-public recovery tests should still pass.

[otReviewAgent]

🟡 Issue: Isolate the selected-public VM recovery mode instead of growing the main durable orchestration method

What's wrong
This bolts a feature-specific recovery mode into an already very large lifecycle method and makes durable recovery harder to reason about as one abstraction. The branch has its own reentrancy policy, peer accounting, terminal mapping, and error shaping, so the durable-recovery contract is now split between the runner and an inline special case.

Example
The method now has two materially different implementations of “durable recovery execution”: the normal path via durableRecoveryRunnerFor(this).run(...), and the selected-public path that hand-builds { outcome, result, peerResults, slices, peerId, safeOffset } inline. Any future change to durable accounting or peer-result shape has to remember this bespoke branch too.

Suggested direction
Keep syncDurableRecoveryContextGraph as a thin dispatcher and move the selected-public VM adapter/accounting into a focused recovery implementation, ideally sharing the durable runner’s result construction helpers rather than duplicating the execution shape.

For Agents
Move the selected-public VM recovery path behind a dedicated helper/strategy near the durable recovery runner boundary, e.g. runSelectedPublicVmDurableRecovery(...), and have syncDurableRecoveryContextGraph choose a recovery executor rather than assemble execution results inline. Preserve the no-reentry behavior, failure accounting, and SWM catch-up independence; prove it with the existing selected-public recovery tests plus a test that the normal durable runner shape is unchanged.

[otReviewAgent]

🟡 Issue: Do not build a second durable recovery runner inside the lifecycle method

What's wrong
This adds a feature-specific mini-runner inside an already very large lifecycle file. It duplicates durable recovery result construction and leaks VM dispatcher reentrancy details into a method whose ordinary path delegates to DurableRecoveryRunner. That makes future changes to durable execution semantics harder to reason about because there are now two implementations of the same outer contract.

Example
If DurableRecoveryExecution gains another field or DurableRecoveryRunner changes how peer progress is aggregated, the ordinary durable path gets it through finish(), while this selected-public branch must be found and updated by hand. The behavior may be correct today; the maintainability risk is the duplicated runner logic.

Suggested direction
Push the selected-public VM lane into a dedicated recovery strategy/facade owned beside DurableRecoveryRunner, or extend the runner with a mode that can execute the chain-inventory slice while preserving the runner-owned result shaping. The lifecycle method should choose the lane, not hand-code DurableRecoveryExecution.

For Agents
Look at packages/agent/src/sync/durable-recovery-runner.ts and the selected-public logic in dkg-agent-lifecycle.ts. Preserve: selected public RFC-64 VM recovery uses chain inventory, avoids broad durable pulls, avoids self-await when the VM dispatcher is active, and reports exceptions as failed peer attempts. Move the selected-public execution/adaptation behind the durable recovery runner or a focused recovery facade, then keep syncDurableRecoveryContextGraph as orchestration/dependency selection only. Existing sync-fetch-coalescing tests should still cover those cases.

[otReviewAgent]

🟡 Issue: Move the selected-public durable recovery lane out of the giant orchestrator

What's wrong
This adds a feature-specific recovery mode as a 60+ line special case inside an already very large lifecycle orchestrator. The code is behaviorally targeted, but structurally it bypasses the runner abstraction that owns durable recovery execution and recreates its result/accounting contract by hand. That makes the method harder to scan and creates a second place where durable recovery semantics can drift.

Example
A future change to durable recovery accounting, peer result shape, or safeOffset semantics now has to remember both durableRecoveryRunnerFor(this).run(...) and this hand-built selected-public path. The long explanatory comments are compensating for a missing lane/strategy abstraction.

Suggested direction
Extract this branch into a dedicated selected-public VM durable recovery runner, or teach the existing durable recovery runner to accept a physical recovery strategy. The goal is to keep the public method as a thin dispatcher and avoid duplicating durable accounting/result assembly in-line.

For Agents
Look at syncDurableRecoveryContextGraph, vmReconcileSliceAsDurableResult, and durableRecoveryRunnerFor. Preserve the current behavior: selected public RFC-64 CGs must avoid broad durable pulls, in-flight VM reconcile should return no-progress, exceptions should count as failed peer attempts. Move the selected-public implementation behind a small durable-recovery lane/strategy or helper that returns DurableRecoveryExecution, and keep syncDurableRecoveryContextGraph as dispatch/orchestration only. Existing tests in sync-fetch-coalescing should continue to prove the branch behavior.

[otReviewAgent]

🟡 Issue: Selected-VM recovery is bolted into the lifecycle method as a second runner

What's wrong
This adds a special-case orchestration path inside an already very large lifecycle class, bypassing the durable recovery runner and duplicating its responsibility for execution shape, terminal outcome, peer-result accounting, and failure mapping. The behavior may be right, but the structure makes durable recovery harder to reason about because there are now two owners of the same orchestration contract.

Example
The ordinary path delegates execution shaping to DurableRecoveryRunner, while the new selected-VM path reconstructs the DurableRecoveryExecution object inline. A future change to outcome classification or peer accounting now has two implementations to update.

Suggested direction
Keep syncDurableRecoveryContextGraph as a dispatcher, but put the selected-VM slice orchestration and ContextGraphReconcileResult-to-DurableRecoveryExecution mapping behind the same recovery-runner boundary as the legacy path.

For Agents
Move the selected-public VM recovery path into a dedicated DurableRecoveryExecution producer under packages/agent/src/sync, or extend DurableRecoveryRunner with a lane/strategy abstraction. Preserve the no-broad-durable-pull behavior, the in-flight no-progress behavior, and the exception-as-failed-peer accounting covered by the new selected-public tests.

[otReviewAgent]

🟡 Issue: Selected-VM recovery is bolted into the generic durable recovery path

What's wrong
This adds a large feature-specific mode switch to an already oversized lifecycle file and makes three concepts share one control path: chain-inventory VM reconciliation, durable sync result accounting, and catch-up peer success bookkeeping. The synthetic selectedVmTerminalCompletions counter exists largely to make the special lane fit the legacy durable shape, which spreads the abstraction leak into unrelated classifiers and diagnostics.

Example
A future terminal signal for selected-public VM recovery would now require coordinated edits in vmReconcileSliceAsDurableResult, syncDurableRecoveryContextGraph, classifyDurableProgress, and runCatchupOverPeers diagnostics, even though the behavior belongs to one recovery lane.

Suggested direction
Move this into a dedicated selected-VM durable recovery strategy/runner, or let durableRecoveryRunnerFor dispatch to a chain-inventory lane. Keep the lifecycle method as a shallow orchestrator instead of making it translate between VM reconcile, durable diagnostics, and peer accounting.

For Agents
Look at LifecycleSyncMethods.syncDurableRecoveryContextGraph, sync/durable-recovery-runner.ts, and the selected-VM reconcile helpers. Preserve the no broad durable pull behavior, self-reentry no-progress behavior, and catch-up readiness semantics. Prove with the existing selected-public VM recovery tests in sync-fetch-coalescing.test.ts.

[otReviewAgent]

🟡 Issue: Model selected-VM recovery as its own lane instead of splicing it into durable sync accounting

What's wrong
This change makes selected-public chain reconciliation masquerade as a durable sync result. That leaks a feature-specific concept into shared durable diagnostics, reconnect classification, catch-up readiness, and runner progress totals. The behavior may be intentional, but structurally it increases coupling and creates a parallel recovery path in an already very large lifecycle class.

Example
The method now has two durable-recovery implementations: the new branch returns { outcome, result, peerResults, slices, safeOffset } by hand, while the normal path delegates to durableRecoveryRunnerFor(this).run(...). Any future change to recovery execution accounting has to remember both shapes.

Suggested direction
Extract the selected-public VM recovery adapter out of the lifecycle method and stop representing chain-inventory terminal state as a durable-transfer counter. A small SelectedVmRecoveryRunner or a recovery-runner strategy that returns normalized readiness evidence would keep the orchestration contract in one place without spreading feature-specific counters through generic durable progress code.

For Agents
Look at syncDurableRecoveryContextGraph, vmReconcileSliceAsDurableResult, and classifyDurableProgress. Preserve the selected-public behavior: no broad durable pull, no self-reentry peer success, and terminal readiness from chain inventory. Move the selected-VM lane behind a dedicated runner/strategy or first-class readiness evidence type, then keep generic durable counters about durable transfer lanes only. Existing selected-public recovery tests should keep proving the same outcomes.

[otReviewAgent]

🟡 Issue: Keep selected-VM recovery behind the durable-recovery abstraction instead of branching the orchestration method

What's wrong
This adds a second recovery execution model inside an already large orchestration class. It duplicates outcome construction and peer accounting outside the canonical durable recovery owner, so future changes to recovery classification now have to remember this special branch too.

Example
The selected path now needs its own vmReconcileSliceAsDurableResult, self-reentry result shape, exception-to-failed-peer mapping, and terminal outcome mapping. Those are the same categories of decisions the durable recovery runner already owns for the legacy lane.

Suggested direction
Push the selected-public VM lane into sync/durable-recovery-runner.ts or a sibling recovery executor with the same DurableRecoveryExecution contract. The lifecycle method should pick a recovery strategy, not hand-roll a second mini runner inside a 10k-line class.

For Agents
Look at packages/agent/src/dkg-agent-lifecycle.ts around syncDurableRecoveryContextGraph and packages/agent/src/sync/durable-recovery-runner.ts. Preserve the selected-public behavior, including no broad durable pull and no self-reentry peer success. Move this into a dedicated runner strategy/dependency or a selected-VM recovery adapter so lifecycle only chooses/wires the recovery mode. Tests should keep the selected VM routing, skipped reentry, failure accounting, and terminal readiness cases.

[otReviewAgent]

🟡 Issue: Model exact-plan caching explicitly instead of adding another optional scope

What's wrong
The new API represents plan-only caching as an extra optional scope layered onto the row-list cache path. This muddies the boundary between payload snapshot retention and manifest-plan retention, and the fallback logic means maintainers have to remember which combinations are meaningful.

Example
A caller can now supply exactGraphPlanMemo without exactGraphPlanCacheScope, exactGraphPlanCacheScope without a memo, or both row and plan scopes with different values. All compile, and the lower-level loader silently decides whether to rebuild, use the row snapshot key, or use the plan key. That is too many implicit modes for a cache boundary.

Suggested direction
Make the cache policy explicit at the boundary: compute a named exact-plan cache key in the handler and pass one complete plan-cache object to the planner. Keep row snapshot caching and plan-only caching as separate concepts rather than overloading durableDataRowListCacheKey and refreshRowList for both.

Confidence note
This is a maintainability concern from the local API shape; I am not claiming a behavioral regression from the current call sites.

For Agents
Refactor readDurableDataPage, readPagedRowsFromExactGraphPlanLoader, and the sync handler call site. Preserve the behavior that authenticated exact-asset page-only sessions reuse the tiny graph/count plan while payload rows remain page-only. Prefer explicit objects like rowSnapshotCache?: RowListCache and exactGraphPlanCache?: { memo, key, refresh }, or encode this in DataRequestPolicy, so the loader no longer infers plan caching from row snapshot state.

[otReviewAgent]

🟡 Issue: Exact-plan caching is expressed through row-list cache plumbing

What's wrong
The new parameter solves a real cache need, but it muddies the planner boundary: a non-row cache is keyed with the durable row-list cache helper and refreshed via refreshRowList. That optionality makes the control flow harder to scan and easier to misuse when another exact-graph caller is added.

Example
The invariant is: page-only exact-asset requests disable row retention but still retain a tiny manifest plan. That invariant is encoded indirectly through exactGraphPlanCacheScope, refreshRowList, and a row-list cache key helper instead of as a direct exact-plan cache policy.

Suggested direction
Make the exact plan cache contract explicit instead of adding another optional scope that reuses row-list naming and refresh semantics. This should let readPagedRowsFromExactGraphPlanLoader consume one clear plan-cache policy and avoid the rowListCacheScope ?? exactGraphPlanCacheScope fallback.

Confidence note
This is a maintainability concern rather than a behavior claim; the current behavior appears intentionally limited to exact-asset page-only sessions.

For Agents
In packages/agent/src/sync/responder/graph-plan.ts and the durable data call site in sync-handler.ts, preserve one-manifest-read-per-session behavior for exact-asset page-only fetches. Consider passing a first-class exactGraphPlanCache: { key, refresh } or a shared responder-session cache policy object so row snapshot caching and exact-plan caching are explicit independent lanes.

[otReviewAgent]

🟡 Issue: Model row caching and exact-plan caching as explicit separate concerns

What's wrong
The new planCache parameter creates a partially overlapping cache API next to RowListCache. The helper now has to merge two optional sources of key, refresh, and generation state, which makes the boundary harder to scan and easier to misuse as more durable data modes are added.

Example
readPagedRowsFromExactGraphPlanLoader now has two cache concepts with overlapping key/refresh/generation fields. A caller has to understand that rows may be page-only while the exact graph plan is session cached, and the helper hides precedence through nullish coalescing rather than a typed paging-session model.

Suggested direction
Make the two cache modes explicit in the type boundary instead of threading a second optional cache bag through a generic helper. This would remove the precedence logic and make page-only payload plus cached manifest plan a named mode rather than an incidental combination of undefined fields.

Confidence note
This is a maintainability concern rather than a proven behavioral defect; it depends on whether the team expects row snapshot caching and exact-plan caching to evolve independently.

For Agents
Look at readDurableDataPage and readPagedRowsFromExactGraphPlanLoader. Preserve page-only payload reads with retained exact manifest plans, but replace the parallel optional cache parameters with an explicit object such as { rowSnapshot?: RowListCache, exactPlan?: PlanCacheOptions }, or split the exact-asset/page-only path into a focused helper so the cache ownership is visible at the call boundary.

[otReviewAgent]

🟡 Issue: Exact plan caching is modeled as parallel optional scopes instead of a session cache

What's wrong
The new exactGraphPlanCacheScope is a second cache lane threaded beside the existing row-list cache lane. Because durableCacheKey is derived from either scope, readers have to reconstruct an implicit state machine from several optional parameters to know whether rows, plans, both, or neither are retained. That is unnecessary optionality churn in an already dense planner.

Example
For page-only exact reads, rowListMemo is intentionally undefined, rowListCacheScope is undefined, and exactGraphPlanCacheScope is set. That only works because readDurableDataPage falls back from one scope to the other and later passes a separate planCache only in the assetUals branch.

Suggested direction
Make the cache boundary explicit. For example, have the handler build one typed session/cache descriptor that says which caches are active for the request, or pass a dedicated exact-plan cache object with an already-derived key. Avoid making readDurableDataPage infer row-cache and plan-cache behavior from fallback scope parameters.

For Agents
In sync-handler.ts and sync/responder/graph-plan.ts, preserve page-only payload reads and retained exact manifest plans. Replace the parallel optional scopes with an explicit responder session/cache object or dedicated exactPlanCache option carrying { key, refresh, generation, memo }. Tests should still show the manifest is queried once across an exact asset session.

[otReviewAgent]

🟡 Issue: Model exact-plan session caching explicitly instead of adding a parallel optional cache contract

What's wrong
The PR correctly separates payload row retention from lightweight plan retention, but implements it by threading a second cache contract beside the existing RowListCache. The helper now has two sources for key, refresh, and generation state, with precedence rules hidden in the call site. This makes the caching lifecycle harder to reason about and easier to misuse as more responder lanes are added.

Example
The exact page-only path now requires the caller to pass exactGraphPlanMemo, exactGraphPlanCacheScope, refreshRowList, and refreshGeneration in the right combination. Passing the memo without the scope silently disables plan caching; passing both row and plan scopes makes the shared durableCacheKey choose rowListCacheScope first. Those are signs that the cache boundary is encoded by optional parameter choreography rather than a clear model.

Suggested direction
Introduce a small typed session-cache/options object that names the row snapshot cache and exact graph plan cache separately, with keys computed once at the boundary. That would preserve the behavior while deleting the fallback precedence and optional-parameter coupling inside the paging helper.

For Agents
Look at readDurableDataPage, readPagedRowsFromExactGraphPlanLoader, and registerSyncHandler. Preserve payload page-only behavior while retaining the lightweight exact manifest plan across a sync session. Replace the overlapping RowListCache/planCache inputs with an explicit typed responder-session cache model, e.g. separate rows and exactPlan entries with their own keys, both derived once from prepareResponderSession. Existing exact-asset wire parsing and responder interleaving tests should still pass.

[otReviewAgent]

🟡 Issue: Make exact-plan caching a first-class cache instead of piggybacking on row-list cache keys

What's wrong
The new exact-plan cache path is wired through row-list naming and fallback semantics even when payload rows deliberately remain page-only. That makes cache ownership hard to reason about: one helper now has to understand two unrelated retention policies, and the refresh-generation side channel becomes another optional knob threaded through generic memo APIs.

Example
A page-only exact read with no rowListMemo still goes through durableDataRowListCacheKey(...) so the manifest plan can be cached. A future caller reading rowListCacheScope as payload-snapshot-only can miss that the same key also controls exact plan identity and refresh generation.

Suggested direction
Introduce an explicit ExactGraphPlanCacheSession or exact-plan cache key object with key, refresh, and generation. Keep it separate from RowListCache so payload snapshot retention and lightweight manifest-plan retention do not share hidden fallback rules.

Confidence note
The current implementation may be preserving subtle session-expiry behavior; the suggested direction is about ownership and readability, not changing cache semantics.

For Agents
Focus on readDurableDataPage, readPagedRowsFromExactGraphPlanLoader, and createSessionPlanGetter. Preserve page-only exact payloads and one manifest-plan read per authenticated session. Replace the ad-hoc exactGraphPlanCacheScope/planCache threading with a typed exact-plan session/cache key, or fold generation into the exact-plan memo key directly. Tests around exact asset manifest query count and superseded exact-plan sessions should still pass.

[otReviewAgent]

🔴 Bug: Relaxed target currency can drop durable VM watermark persistence

What's wrong
The change lets VM reconciliation survive harmless subscription snapshot replacements, but the watermark write path still requires original object identity. That combination lets a successful slice advance only the in-memory cursor and skip the durable subscription watermark, so progress can be lost across restart.

Example
Start a VM reconcile slice at lastReconciledOrdinal = 100. While it runs, SWM readiness replaces the subscription record with { ...sub, metaSynced: true }. The slice can advance to watermark 150, but persistVmReconcileWatermark() skips saving because object identity changed. After restart the node resumes from the old durable watermark.

Suggested direction
Make the persistence fence match the new currency rule: persist against the current same-binding subscription snapshot, while still rejecting unsubscribe, rebind, deletion, and cursor reset.

For Agents
Update the subscription-target watermark persistence path to tolerate the same harmless snapshot replacements that isVmReconcileTargetCurrent now accepts. Preserve binding-generation and cursor safety fences. Add a test that replaces readiness fields during reconcile and proves lastReconciledOrdinal persists while unsubscribe/rebind still fails closed.

VM subscription liveness is now duplicated as ad-hoc predicates

What's wrong
The PR correctly moves away from object identity, but it replaces that with repeated structural checks scattered across two flows. The dynamic fallback in healStrandedScopedKCs is especially muddy: it makes a production method carry test-double tolerance instead of declaring the boundary it needs.

Example
If the definition of a valid VM subscription target changes again, a maintainer has to update both isVmReconcileTargetCurrent and the local canApply closure. Missing one leaves two subtly different liveness policies in the same VM reconcile path.

Suggested direction
Extract a single helper such as currentSubscriptionTargetStillCurrent(localCgId, target) or extend the binding-state boundary so these paths share one typed rule. Remove the instanceof Map fallback from production code and let tests provide the real dependency shape.

For Agents
Refactor the subscription-target liveness rule in packages/agent/src/dkg-agent-swm-host.ts. Preserve readiness-refresh tolerance and unsubscribe/rebind fail-closed behavior, but centralize the active-subscription plus binding-generation check in one helper and call it from both target-current and RS-heal paths.

[otReviewAgent]

🔴 Bug: A racing readiness write can still persist an old VM watermark

What's wrong
The new code updates the latest in-memory subscription snapshot after the strict save, but it does not ensure that snapshot is the final durable write. A harmless readiness replacement can enqueue a later persistence record with the old watermark, causing durable VM progress to regress on restart.

Example
Start persistVmReconcileWatermark(cg, 17, target) with durable store save() blocked after its strict current check. While it is blocked, a harmless readiness update calls setContextGraphSubscription(cg, { ...current, metaSynced: true }), whose queued record still has lastReconciledOrdinal: 0. When the watermark save unblocks, line 3692 mutates the live snapshot to 17, but the queued readiness save can run afterward and persist 0. After restart, the reconciler scans from the old ordinal despite having emitted cursor advancement.

Suggested direction
After detecting a latest same-binding replacement, durably persist the merged latest snapshot or change the queued subscription persistence path so later readiness writes cannot overwrite a newer lastReconciledOrdinal.

Confidence note
This depends on the existing per-context subscription persistence queue allowing another readiness write to be enqueued while the strict watermark save is already executing, which the surrounding code appears to allow.

For Agents
Inspect persistVmReconcileWatermark, persistContextGraphSubscriptionStrict, and setContextGraphSubscription persistence queue behavior. Ensure the watermark is merged into the durable latest snapshot when a same-binding replacement races the save, or make queued subscription writes compute/merge lastReconciledOrdinal at execution time. Add a store-backed test with a blocked save and a concurrent readiness replacement, then assert the final persisted record keeps the new watermark.

[otReviewAgent]

🟡 Issue: Collapse the repeated live-subscription binding predicate into one helper

What's wrong
The PR fixes object-identity fencing by scattering a new semantic-currentness predicate across several paths. That reduces one coupling but introduces another: future changes to subscription ownership, coreHosted handling, or binding-generation semantics now need synchronized edits in multiple busy methods.

Example
The same concept, “return the current subscription snapshot if it still represents this VM target,” is implemented three times with slightly different shapes. healStrandedScopedKCs even calls currentSubscription() multiple times inside one predicate, while the watermark path snapshots sub and later latest; that difference is incidental rather than part of the model.

Suggested direction
Centralize the semantic currentness check so the code asks for “the current same-binding subscription” instead of reassembling membership, binding-generation, and cursor checks at each call site.

For Agents
Add one small helper in SwmHostModeMethods or ContextGraphBindingState, such as currentVmSubscriptionTarget(localCgId, target): ContextGraphSub | undefined, that performs membership and binding-generation checks once. Use it from isVmReconcileTargetCurrent, persistVmReconcileWatermark, and healStrandedScopedKCs. Preserve the readiness-refresh behavior and the unsubscribe/rebind fail-closed behavior; existing tests in vm-reconcile-self-prime.test.ts should continue to cover both.

[otReviewAgent]

🟡 Issue: Centralize the subscription-target freshness predicate

What's wrong
The diff replaces object identity with the right domain predicate, but spreads that predicate as ad-hoc checks across a large host-mode file. That keeps the old fragility in a new form: the invariant is no longer identity, but it still has no single owner.

Example
A future change to what counts as a live subscription VM target would need coordinated edits in at least three places. The heal path also calls currentSubscription() multiple times inside one predicate, which makes the snapshot being checked less obvious than a single helper returning the current same-binding subscription.

Suggested direction
Extract the common same-binding live-subscription check into one helper and reuse it from the VM target-current, watermark, and RS-heal paths. Keep cursor and lifecycle checks as explicit additions where needed.

Confidence note
This is a structural maintainability concern from the diff and surrounding code; the current behavior may be covered by the new tests, but the predicate ownership is still scattered.

For Agents
Look at isVmReconcileTargetCurrent, persistVmReconcileWatermark, and healStrandedScopedKCs. Preserve the new behavior that readiness-only subscription replacements do not stale a VM target, while unsubscribe/rebind/cursor reset still fails closed. Extract a shared helper such as currentSameBindingSubscriptionTarget or isLiveSubscriptionVmTarget, and have each call site layer only its extra cursor/lifecycle requirement on top. The vm-reconcile-self-prime tests should cover the preservation.

[otReviewAgent]

🟡 Issue: A second readiness replacement can still abort VM watermark persistence

What's wrong
The new code explicitly allows VM reconciliation to continue across readiness-only subscription snapshot replacements, but the strict persistence helper still fails closed on any replacement that happens while the write is waiting behind earlier subscription persistence. That means ordinary readiness bookkeeping can still drop the VM watermark and turn a valid reconcile slice into a retry, leaving durable cursor state behind materialized data.

Example
A VM slice computes watermark 17 and calls persistVmReconcileWatermark. While its strict write is queued, catch-up flips metaSynced or sharedMemorySynced, replacing the immutable subscription object without changing the on-chain binding. The strict helper rejects before latest.lastReconciledOrdinal = watermark runs, so the reconciler reports a failed slice and the watermark is not persisted even though the replacement was meant to be harmless.

Suggested direction
Use a watermark-specific persistence path that validates the binding generation/current cursor rather than the immutable subscription object identity, and persist a record built from the latest same-binding subscription state.

Confidence note
This depends on the existing strict persistence helper continuing to reject subscription object replacement while a queued write is pending, which is visible in the surrounding code but not changed in this diff.

For Agents
Look at persistVmReconcileWatermark in packages/agent/src/dkg-agent-swm-host.ts and persistContextGraphSubscriptionStrict in packages/agent/src/dkg-agent-lifecycle.ts. Preserve the binding-generation and active membership checks, but avoid the strict object-identity fence for VM watermark-only updates or re-read/merge the latest same-binding subscription inside the queued write. Add a test where a readiness-only setContextGraphSubscription happens after the watermark persistence call is queued and prove the watermark still persists without overwriting readiness flags.

[otReviewAgent]

🟡 Issue: The RS-heal readiness-refresh path is not directly verified

What's wrong
The change is meant to keep RS healing active when readiness bookkeeping replaces the subscription object. The current new test only proves the separate VM target-current helper accepts that replacement. A regression in this healer-specific guard would still leave stranded KCs unhealed after readiness updates without failing the added test.

Example
Seed a stranded legacy KC, resolve a VM reconcile target, replace the subscription record with the same onChainId but updated readiness flags, then call healStrandedScopedKCs(...). Expected: the heal completes and scoped triples are present. With the old identity check left in this method, it would return { status: 'skipped', reason: 'not-current' } while the new isVmReconcileTargetCurrent test would still pass.

Suggested direction
Cover the changed healer guard itself, preferably with a stranded-KC fixture and a subscription record replacement before invoking healStrandedScopedKCs.

For Agents
Add a regression in packages/agent/test/rs-heal-stranded-kc.test.ts or vm-reconcile-self-prime.test.ts that drives the real healStrandedScopedKCs after a harmless subscription snapshot replacement. Preserve unsubscribe/rebind fail-closed behavior, and prove the replacement case performs the heal instead of skipping as not-current.

[otReviewAgent]

🟡 Issue: RS-heal currentness change is not covered by the new replacement tests

What's wrong
The PR broadens RS-heal admission from captured object identity to the live same-binding subscription snapshot, but the added tests only prove related target-current and watermark paths. Because healStrandedScopedKCs has its own currentSubscription()/canApply() guard, a regression in this path could silently disable stranded-KC repair after readiness bookkeeping replaces the subscription object while all new tests still pass.

Example
A focused regression test would resolve a VM subscription target, replace the immutable subscription snapshot with the same binding/readiness flags, then call healStrandedScopedKCs and assert it reaches the guard/enumeration or completes instead of returning { status: 'skipped', reason: 'not-current' }.

Suggested direction
Add a regression test that drives healStrandedScopedKCs itself through the harmless subscription snapshot replacement this PR is allowing.

For Agents
Add RS-heal coverage in packages/agent/test/rs-heal-stranded-kc.test.ts or the VM reconcile self-prime suite. Create an agent-like object with subscribedContextGraphs as a Map, capture a VmReconcileSubscriptionTarget, replace the subscription object without changing binding, and prove healStrandedScopedKCs still applies. Keep an unsubscribe/rebind negative case failing closed.

[otReviewAgent]

🟡 Issue: RS-heal snapshot replacement behavior is not directly tested

What's wrong
This diff changes RS-heal’s current-target fence so readiness updates no longer abort a valid heal, but the added tests cover adjacent VM target and watermark helpers rather than the RS-heal sweep itself. Because healStrandedScopedKCs rechecks canApply after several async store operations, a future regression in this guard could silently strand KCs while the current tests remain green.

Example
A regression test could seed a stranded KC, set agentLike.subscribedContextGraphs = new Map([[cg, initial]]), capture a target from initial, replace the map entry with { ...initial, metaSynced: true, sharedMemorySynced: true } during the sweep, and assert healStrandedScopedKCs completes and the scoped KC is materialized instead of returning { status: 'skipped', reason: 'not-current' }.

Suggested direction
Add a focused RS-heal regression test that exercises this method’s own canApply checks after a non-binding subscription snapshot replacement.

For Agents
Add coverage in packages/agent/test/rs-heal-stranded-kc.test.ts for healStrandedScopedKCs with a Map-backed live subscription. Preserve the same-binding readiness replacement behavior while still proving unsubscribe/rebind skips as not-current.

[otReviewAgent]

🟡 Issue: RS-heal snapshot replacement path lacks a regression test

What's wrong
This change is meant to keep stranded-KC repair active across immutable subscription record replacements, but the added tests stop short of the RS-heal guard. A future regression back to object identity would silently skip the repair pass while the current readiness/watermark tests still pass.

Example
Resolve a subscription VM target, replace the subscription with the same binding plus readiness flags via setContextGraphSubscription, then call healStrandedScopedKCs with a no-work/fake store and assert it does not return { status: 'skipped', reason: 'not-current' }. The old identity guard would fail that scenario.

Suggested direction
Add a focused test that exercises healStrandedScopedKCs after a harmless subscription readiness replacement.

For Agents
Add coverage in packages/agent/test/vm-reconcile-self-prime.test.ts or the RS-heal suite for a same-binding subscription snapshot replacement before healStrandedScopedKCs runs. Preserve the existing fail-closed behavior for unsubscribe/rebind while proving readiness-only replacement remains current.

[otReviewAgent]

🟡 Issue: The post-write watermark race is not verified

What's wrong
The change adds behavior for an asynchronous race where readiness bookkeeping replaces the immutable subscription snapshot while watermark persistence is awaiting durable storage. The current test covers only a pre-existing replacement, so it would not catch a regression that writes the watermark to a stale snapshot after the await.

Example
A regression that changed the post-write code back to sub.lastReconciledOrdinal = watermark would still pass the new test, because the test never replaces the subscription during persistContextGraphSubscriptionStrict. A failing-test sketch: inside the persistContextGraphSubscriptionStrict stub, call setContextGraphSubscription(...) again with the same binding/readiness fields, then assert the map's newest object gets lastReconciledOrdinal: 17.

Suggested direction
Add a regression that performs the harmless readiness replacement while the watermark persist is in flight, not only before the call starts.

For Agents
In vm-reconcile-self-prime.test.ts, extend the watermark regression to replace the subscription snapshot during the awaited persistContextGraphSubscriptionStrict stub and assert the latest same-binding snapshot receives the watermark. Consider a matching healStrandedScopedKCs readiness-replacement test around canApply() so the long-running RS heal path is covered too.

[otReviewAgent]

🟡 Issue: RS-heal same-binding snapshot refresh is not directly verified

What's wrong
The PR changes this repair path to tolerate immutable subscription snapshot replacement, but the added tests do not drive the repair path. A future regression back to object-identity gating could pass the current new tests while stranded-KC repair still aborts after ordinary readiness bookkeeping.

Example
Scenario to cover: resolve a subscription target for a CG with a stranded KC, replace the subscription record with the same binding but updated readiness flags, then run healStrandedScopedKCs. Expected behavior is repair continues; an unsubscribe or binding change should still return not-current.

Suggested direction
Exercise healStrandedScopedKCs itself under a non-binding subscription replacement, not only the shared target-current helper.

Confidence note
This is based on the diff plus surrounding test search; tests were not run in the read-only workspace.

For Agents
Add a regression in packages/agent/test/rs-heal-stranded-kc.test.ts or vm-reconcile-self-prime.test.ts that gives healStrandedScopedKCs a real subscribedContextGraphs Map, replaces the subscription snapshot before or during the heal, and proves same-binding readiness updates do not skip the repair while a real ownership transition still skips.

[otReviewAgent]

🟡 Issue: RS-heal snapshot replacement path lacks a regression test

What's wrong
The changed behavior is meant to keep a long-running RS-heal slice current across immutable subscription readiness updates. Current tests do not prove this method’s own guard permits that scenario, so a future identity-check regression in the heal path could pass.

Example
Start an RS-heal pass, replace subscribedContextGraphs.get(localCgId) with a same-binding readiness-updated snapshot before a later canApply() check, and assert the stranded KC is still copied. The old identity-based behavior would skip with not-current.

Suggested direction
Exercise the actual heal path across a harmless subscription record replacement, not only the shared target-current helper.

For Agents
Add an RS-heal regression in rs-heal-stranded-kc.test.ts or vm-reconcile-self-prime.test.ts that mutates the subscription map during healStrandedScopedKCs while preserving the binding/cursor, then proves materialization continues; also keep an unsubscribe/rebind case failing closed if not already covered.

[otReviewAgent]

🔴 Bug: Selected VM terminal results do not satisfy catch-up readiness accounting

What's wrong
The new adapter makes selected public CG recovery report a terminal durable result without any of the signals the existing readiness code recognizes as a clean durable data completion. For selected public CGs that are also subscriptions or hosted CGs, catch-up can complete VM reconciliation but leave the subscription readiness flag unset.

Example
A public RFC-64 CG is in syncContextGraphs and is also an active subscription. runVmReconcileForCg returns status: 'current', unresolvedOrdinals: 0, so this adapter returns { complete: true, completedPhases: 1, insertedDataTriples: 0, verifiedPrivateOnlyResponses: 0 }. In durable-only catch-up, cleanDurableDataSynced and cleanDurablePrivateOnlyCompletions stay zero, so markContextGraphSubscriptionState(..., { synced: true }) is not called even though VM reconciliation reached the terminal boundary.

Suggested direction
Either bypass this adapter for actual subscribed/core-hosted graphs that still need legacy durable readiness accounting, or extend the durable progress contract/classifier with a selected-VM terminal signal that runCatchupOverPeers treats as a clean durable completion.

For Agents
Look at vmReconcileSliceAsDurableResult and runCatchupOverPeers readiness accounting. Preserve the no-broad-durable-pull behavior for selected public VM recovery, but add an explicit completion signal or catch-up classification path so a terminal selected-VM reconcile can mark subscribed/core-hosted CGs durable-synced. Add a test where a selected public subscribed CG reaches current via VM reconcile and catch-up marks synced without calling syncFromPeerDetailed.

[otReviewAgent]

🟡 Issue: Watermark-only VM progress is not covered

What's wrong
The adapter has a distinct progress path for cursor advancement without reconciledOrdinals, but the tests only prove the materialized-ordinal path. A regression that drops watermarkAfter > watermarkBefore would misclassify that slice as no-progress and the current tests would not catch it.

Example
Add a selected-public reconcile fixture like { status: 'progress', reconciledOrdinals: 0, watermarkBefore: 10, watermarkAfter: 20, unresolvedOrdinals: 1 } and assert syncDurableRecoveryContextGraph returns outcome: 'partial-progress' with checkpointAdvances: 1.

Suggested direction
Add a regression test for a reconcile slice that advances the watermark without materializing new ordinals.

For Agents
Look in packages/agent/test/sync-fetch-coalescing.test.ts near the new selected-public VM catch-up tests. Add a case for watermark-only advancement and preserve the durable accounting contract that cursor progress is not reported as no-progress.

[otReviewAgent]

🔴 Bug: Skipped selected-public VM recovery is reported as a clean peer attempt

What's wrong
The branch is intentionally doing no durable work to avoid re-entering the active VM reconcile dispatcher, but it still emits a successful-looking per-peer durable result. Existing catch-up accounting uses the presence of peerResults to decide which durable peers were attempted, so this can make an external durable-only trigger look successful while the actual chain-inventory owner is merely still running.

Example
Start a selected public VM reconcile, then call durable-only catch-up for the same CG while it is still in flight. This branch returns one peerResults entry with an incomplete, non-failed result. The catch-up aggregation can return peersSucceeded: 1 and dataSynced: 0 even though this trigger did not run a VM slice and the active reconcile may still fail later.

Suggested direction
Return no attempted peer for the self-reentry/no-progress path, or use an explicit deferred/failure diagnostic that prevents catch-up success accounting from treating the skipped durable lane as successful.

For Agents
Look at the selected-public isInFlight branch in syncDurableRecoveryContextGraph and the runCatchupOverPeers peer accounting. Preserve the self-reentry guard, but do not report an unattempted candidate as a clean peer result; either omit peerResults for this branch or mark it with an accounting state that cannot be counted as success. Add a durable-only catch-up test where vmReconcileDispatcher.isInFlight(cg) is true and assert no peer success is reported from the skipped VM lane.

[branarakic]

Addressed both current P1 findings. 1f6e41c adds explicit selectedVmTerminalCompletions evidence and uses it for clean readiness promotion without a broad durable pull. 5e8034e makes the in-flight self-reentry path report zero attempted peerResults, so durable-only catch-up now returns peersTried/responded/succeeded = 0. Focused sync-fetch-coalescing + durable-progress coverage passes 68/68 across the two suites. Leaving this thread open for the separate watermark-only P2 test request.

[otReviewAgent]

🔴 Bug: In-flight selected VM recovery is reported as a successful peer attempt

What's wrong
The guard is meant to avoid re-entering an active chain-inventory reconcile, but it still fabricates a clean per-peer durable result. Downstream catch-up accounting interprets that as a successful peer round despite there being no durable attempt, no response, and no progress. That can make subscribe/catch-up status report success and can suppress retry-oriented behavior while the actual VM work is merely still in flight.

Example
During runCatchupOverPeers for a selected RFC-64 public CG, if vmReconcileDispatcher.isInFlight(contextGraphId) is already true, this branch returns { outcome: 'no-progress', slices: 0, peerResults: [{ peerId, result: incomplete }] }. The caller then treats that peer as attempted and successful even though no durable VM slice ran and no peer responded. Expected behavior for a fail-closed no-progress re-entry is that it does not create successful peer evidence.

Suggested direction
Return no peer result for the in-flight zero-slice path, or classify that synthetic result as non-successful in catch-up accounting so status/retry logic cannot confuse an active owner with a completed peer round.

For Agents
Look at the selected-public branch in syncDurableRecoveryContextGraph and its runCatchupOverPeers consumer. Preserve the self-reentry avoidance, but do not emit clean peer evidence for a zero-slice no-progress result, or mark it with a diagnostic that the catch-up success classifier will not count as success. Add a test that drives runCatchupOverPeers while vmReconcileDispatcher.isInFlight() is true and proves peersSucceeded stays 0 unless SWM independently succeeds.

[otReviewAgent]

🟡 Issue: Missing boundary test for unresolved current VM reconcile slices

What's wrong
The new chain-inventory adapter makes selected-public VM readiness depend on a compound terminal predicate, but the tests only exercise the two easy sides of that predicate. That leaves the data-completeness guard unverified for a realistic boundary where the reconciler has reached the head watermark but still reports unresolved ordinals.

Example
A regression changing the condition to result.status === 'current' would still pass the new tests, but a reconcile result like { status: 'current', unresolvedOrdinals: 1, watermarkAfter: headOrdinal } would incorrectly emit selectedVmTerminalCompletions and promote durable readiness.

Suggested direction
Add a regression test for the current plus unresolved-ordinals case so the terminal readiness evidence is only accepted when both parts of the new guard hold.

For Agents
Add a selected-public VM durable recovery test in packages/agent/test/sync-fetch-coalescing.test.ts where runVmReconcileForCg returns status: 'current' with unresolvedOrdinals > 0; assert the result is not complete, has no selectedVmTerminalCompletions, and does not mark readiness/peer success as terminal.

[otReviewAgent]

🟡 Issue: Avoid classifying transport state from raw error-message text

What's wrong
The PR adds a magic string check in page-fetch for a transport-layer phrase. That pulls low-level libp2p wording into page sizing policy and creates another place where sync transport classification can drift from the existing error-tags boundary.

Example
The current branch treats any Error whose text contains 'during opening' as non-capacity evidence, even if that wording changes upstream or another layer emits the same phrase for a different condition. There is no honest payload example needed here; the issue is the brittle boundary and duplicated classification policy.

Suggested direction
Replace the inline string match with a canonical typed classifier or tag at the transport/error boundary. Page sizing should consume structured evidence about whether the peer could have inspected the requested page, not libp2p wording.

For Agents
Move this distinction into the sync transport/error-tag boundary, for example a typed tag or classifier exposed from sync/error-tags.ts and set by p2p/sync-transport.ts when the send failure is known to be pre-response stream-opening churn. Preserve fetchSyncPages behavior: local request failures should not shrink pages, responder capacity/validation failures still may shrink, and the 'Remote closed connection during opening' retry should keep the initial page size.

[otReviewAgent]

🟡 Issue: Transport classification is hard-coded by libp2p message text in the requester

What's wrong
This local substring check leaks a transport implementation detail into requester pagination policy. It bypasses the codebase's canonical error-tag boundary, so future transport wording or classifier changes can make page-size behavior drift in a place maintainers will not naturally inspect.

Example
A future router/libp2p error like stream closed while opening would miss this local check and collapse page size through isSyncTransportFailure, while an unrelated error message containing during opening would skip reduction. The requester should not own those transport wording details.

Suggested direction
Keep page-size adaptation expressed in terms of sync-level facts: whether the peer responded and whether the responder rejected capacity. Put libp2p/router message matching, if it must exist, behind the existing sync transport classifier/tagging layer.

For Agents
Move this classification to the transport/error boundary. Either tag the error in sync-transport/ProtocolRouter or add a named classifier in sync/error-tags.ts for pre-response transport-open failures, then have page-fetch.ts consume that. Keep the existing retry behavior and the regression that preserves the initial page size for connection churn before response.

[otReviewAgent]

🟡 Issue: Page-size policy depends on a libp2p error-message substring

What's wrong
This leaks a low-level libp2p message string into requester pagination policy. It is brittle and obscures the real invariant: page-size reduction should depend on whether the responder had a chance to reject or serialize the page, not on English text in an Error message.

Example
The test case throws Error('Remote closed connection during opening') and the page-size profile stays at 512. The same transport phase wrapped with different wording would take the opposite branch, while an unrelated error containing the phrase would also suppress reduction.

Suggested direction
Move the transport-phase knowledge to the transport boundary and expose it as a typed classification, such as a tagged SyncTransportFailure with phase/opening metadata or a reducePageSize=false marker.

For Agents
Add a typed pre-response/opening failure marker in the router or sync error-tag layer, then have shouldReducePageSize consult that marker. Update the test to throw the tagged error rather than depending on libp2p wording.

[otReviewAgent]

🟡 Issue: Page-size policy depends on a transport error-message substring

What's wrong
shouldReducePageSize is a page-budget policy, but this change makes it know about a specific libp2p message fragment. That is brittle, stringly typed, and duplicates transport-layer knowledge in requester pagination. It also makes the policy harder to extend because future transport exceptions will likely add more ad-hoc message checks here.

Example
If libp2p changes the text to stream closed while opening, page-size reduction silently regresses. If another error happens to contain during opening, the page-size policy treats it as pre-response stream churn even if it came from a different layer.

Suggested direction
Normalize this at the transport boundary with a typed error/code or canonical predicate such as isPreResponseStreamOpenFailure(error), then have shouldReducePageSize depend on that stable classification instead of raw libp2p text.

For Agents
Inspect shouldReducePageSize, sync transport error helpers, and ProtocolRouter.send. Preserve the behavior that pre-response stream-opening churn does not reduce page size. Add or update the requester page-size test to assert the typed/code-based classification rather than a raw message substring.

[otReviewAgent]

🟡 Issue: Replace the transport message substring check with a typed failure classification

What's wrong
This adds a libp2p-specific string match inside requester pagination policy. It is a brittle boundary leak: page sizing now depends on incidental error wording instead of the transport layer exposing whether the peer ever inspected the request.

Example
If libp2p changes the wording of the opening failure, or another transport emits a different message for the same pre-response state, this capacity policy silently changes even though the underlying failure category did not.

Suggested direction
Move this distinction into the transport/error-tag layer, for example an isPreResponseStreamOpenFailure classifier or error tag, and let page-size policy depend on that typed boundary.

For Agents
Look at shouldReducePageSize and the sync transport/error tagging helpers. Preserve the rule that pre-response stream-opening churn should not reduce page size. Add a typed error tag/classifier at the transport boundary and have page-fetch consume that classifier instead of matching message text. Keep the new retry-size test but assert the typed classification path where possible.

[otReviewAgent]

🟡 Issue: Do not encode transport phase policy as a magic error-message substring

What's wrong
The page-size state machine now depends on a brittle libp2p message fragment. That leaks transport implementation detail into requester pagination and makes the adaptive policy harder to audit or evolve.

Example
Any future libp2p wording change, wrapper message, localization, or different transport with the same condition but different text would silently change page-size learning behavior. Conversely, an unrelated error containing the phrase during opening would be treated as pre-response churn.

Suggested direction
Have the router/sync transport expose a typed reason for pre-response stream-opening failures, then let shouldReducePageSize branch on that structured classification. If typing the thrown error is too invasive, centralize the string mapping in the transport error classifier rather than inside pagination state.

Confidence note
The current string likely matches the observed libp2p error, but the maintainability concern is the boundary: a transport phase decision is now encoded as free-text parsing in requester pagination.

For Agents
Look in packages/agent/src/sync/requester/page-fetch.ts and the transport/router error creation path in packages/core/src/protocol-router.ts or sync transport wrappers. Preserve the behavior that pre-response stream-opening failures do not shrink page size. Add or reuse a typed error/classification such as a transport phase/retry reason and test the classifier without relying on free-text wording.

[otReviewAgent]

🟡 Issue: Relay admission adds a second timeout orchestration layer around sendInner

What's wrong
The wrapper solves admission outside the router's canonical send flow, duplicating option normalization, deadline creation, abort-signal composition, and remaining-budget calculation. In a nearly 2k-line router that already has pooled, one-shot, retry, and multipath branches, this extra outer layer makes the control flow materially harder to reason about.

Example
A single relay-only send() now has an outer AbortSignal.timeout(timeoutMs) for queue admission and then a fresh inner AbortSignal.timeout(remainingMs) inside sendInner. Future changes to timeout composition, stop-signal handling, or telemetry have to consider both layers.

Suggested direction
Integrate the per-peer relay admission into sendInner, or extract a tiny withPeerSerialAdmission(peerId, signal, work) called from inside the existing send flow. The queue can remain, but the public send budget and abort ownership should stay in one place.

For Agents
Refactor ProtocolRouter.sendInner so the relay-only single-use turn is acquired after singleUsePayload, overallStartedAt, and overallSignal are established, before the one-shot stream opening. Preserve one-at-a-time behavior for relay-only single-use sends, parallelism for direct/different peers, and abortable queue waits.

[otReviewAgent]

🟡 Issue: Model session generation as cache identity, not side-channel state

What's wrong
The memo now carries refreshGeneration beside the cache key and uses bespoke control flow to decide whether an in-flight or cached value belongs to the caller. This makes a simple identity boundary feel temporal and stateful, increasing the number of branches a reader must audit for every session-plan cache user.

Example
A newer session for the same key now enters a loop, waits for the older in-flight load to settle, suppresses that result, and then rebuilds. If the generation were part of the key, the two immutable plan identities would naturally be isolated by the Map with no supersedesPending branch.

Suggested direction
Promote refreshGeneration into the memo key or a typed cache-key object so cache separation falls out of ordinary Map semantics. That should let the supersedesPending loop, cached-generation mismatch deletion, and refreshGeneration fields on memo entries disappear.

For Agents
Change the plan cache key construction in readDurableDataPage/readSwmMetaPage callers to include the session generation when available, or introduce a structured key type for {scope, generation, resource}. Then remove refreshGeneration from createSessionPlanMemo/ExactGraphPagePlanMemo and keep the existing newer-session test passing through distinct keys.

[otReviewAgent]

🟡 Issue: Use the cache key as the only session identity instead of adding a parallel refresh-generation state machine

What's wrong
This makes the memo harder to reason about by splitting identity across key and refreshGeneration. The resulting control flow serializes a superseding request behind stale inflight work and requires special-case replacement logic in several places. The simpler model is that different immutable session generations are different keys.

Example
A newer page-zero request with refreshGeneration: 'new-session' for the same key waits for the old pending plan to settle, then rebuilds. If the generation were part of the memo key, the existing Map/inflight behavior would naturally isolate both sessions without the supersession loop, cached-entry generation checks, or inflight metadata.

Suggested direction
Fold refreshGeneration into the memo key at the boundary that knows the session, or introduce a tiny typed key builder. That should let createSessionPlanMemo go back to one cached map, one inflight map, and no bespoke supersession loop.

For Agents
Look in packages/agent/src/sync/responder/graph-plan.ts around createSessionPlanMemo, readDurableDataPage, and createSessionPlanGetter. Preserve the behavior that offset>0 reuses the same session plan and a new session cannot inherit an old pending plan. Refactor toward a single canonical cache key that includes session generation where needed; tests should cover the concurrent old/new exact-plan session case.

[otReviewAgent]

🟡 Issue: The new durable counter weakens the diagnostics type boundary

What's wrong
The field is treated as a real durable diagnostic counter everywhere it is produced, reduced, and classified, but the public diagnostic/result interface makes it optional. That blurs the invariant and encourages more nullish fallback plumbing instead of one normalized result shape.

Example
DurableProgressSummary is already the optional projection type for partial progress objects. DurableSyncDiagnostics is the concrete counter bag used by results and catch-up diagnostics, so this new counter should follow the same required-zero convention as fetchedDataTriples or checkpointAdvances.

Suggested direction
Use a required numeric counter at the DurableSyncDiagnostics boundary and reserve optionality for projection/summary types that intentionally accept partial data.

Confidence note
This may have been left optional for external compatibility, but the surrounding accumulator code already normalizes the counter to zero internally.

For Agents
Make selectedVmTerminalCompletions required on DurableSyncDiagnostics/DurableSyncResult and keep it optional only on DurableProgressSummary. Ensure createDurableSyncDiagnosticsBase, catch-up diagnostics initialization, and test helpers all initialize it to 0.

[otReviewAgent]

🟡 Issue: New durable counter is optional at the canonical type boundary

What's wrong
The PR adds a new first-class durable diagnostic counter but declares it optional in the canonical diagnostics interface. That muddies the invariant: some code treats durable results as fully initialized counters, while the type says this one may be absent. The result is extra ?? 0 handling and weaker compile-time protection for future aggregators.

Example
DurableSyncResult can type-check without selectedVmTerminalCompletions, but initialized durable results are expected to carry the counter. Callers then have to write ?? 0 around a field that the canonical factory should guarantee.

Suggested direction
Make selectedVmTerminalCompletions required wherever a full durable result/diagnostic object is represented, and reserve optional fields for partial summaries or legacy projections.

For Agents
Update the durable sync type boundary: make the counter required on DurableSyncDiagnostics/DurableSyncResult if it is part of the canonical result, while keeping optionality only on projection/classification inputs like DurableProgressSummary. Verify accumulator factories and diagnostics initializers still compile without local fallbacks.

[otReviewAgent]

🟡 Issue: Selected-VM terminal readiness lacks negative regression tests

What's wrong
This new signal can now promote subscription readiness without payload bytes. The only added end-to-end-style test proves the happy path, so the riskier contract, that terminal evidence must stay clean and complete before readiness promotion, is not pinned.

Example
A regression that allowed { selectedVmTerminalCompletions: 1, completedPhases: 1, failedPeers: 1 } or the same result with { complete: false } to produce completedReadinessCleanly === true would not be caught by the new tests.

Suggested direction
Cover the new counter directly in the durable-progress classifier tests, mirroring the existing private-only and inserted-data failure matrices.

For Agents
Add classifier-level tests in packages/agent/test/durable-progress.test.ts for the selected-VM terminal counter: one clean completed phase should be progress/readiness-clean, and each blocking failure plus complete: false should prevent clean readiness promotion.

[otReviewAgent]

🟡 Issue: Selected VM terminal reconnect progress is not directly tested

What's wrong
The PR changes reconnect progress semantics, but the added tests only exercise durable recovery routing and catch-up readiness. That leaves the on-connect/reconnect side of the new terminal-evidence contract unverified.

Example
A regression that removed || hasSelectedVmTerminalCompletion from madeReconnectProgress would still leave the new catch-up readiness test green, but on-connect accounting would stop treating a clean selected-public VM terminal result as reconnect progress.

Suggested direction
Cover the new counter in the central classifier or an on-connect accounting test so reconnect behavior is pinned.

For Agents
Add a focused durable-progress.test.ts case for { selectedVmTerminalCompletions: 1, completedPhases: 1 } asserting hasSelectedVmTerminalCompletion, madeReconnectProgress, madeReadinessProgress, and completedReadinessCleanly; preserve failure gating with complete: false or failed counters if that behavior matters.

[otReviewAgent]

🟡 Issue: The new exact-page store response cap is not verified

What's wrong
This change is specifically meant to bound SPARQL JSON before parsing thousands of exact-page rows into JS objects. The current tests exercise the final wire cap and row limit, but they would not fail if the pre-parse store response bound disappeared, leaving the memory-protection behavior unverified.

Example
A regression that removed the maxResponseBytes option from the exact graph row query would still pass the added tests: exact-asset-wire-parse.test.ts checks the serialized response byte length and row count after Oxigraph has already parsed bindings, but it never asserts the store query was called with the new SPARQL JSON response cap.

Suggested direction
Extend the exact-asset page-only responder coverage to assert the store-level query option, not just the final serialized response size.

For Agents
Look at readRowsPageFromExactGraphPlan and the exact-asset responder tests. Add a test that intercepts store.query for the ORDER BY ?s ?p ?o exact payload query and asserts options.maxResponseBytes is present and no greater than SYNC_EXACT_PAGE_STORE_RESPONSE_MAX_BYTES, while preserving the existing serialized wire-byte assertions.