fix(agent): decision-driven meta replacement in private SWM recovery (GH#2273 3/3) - #2287
Conversation
| // Foreign id on identical-version content: preserve ONLY on proven | ||
| // identity equivalence; a genuine change (policy, digest, author) | ||
| // returns null here and the curator's identity wins as today. | ||
| return (await materializer.selectRepairIdentity(deps.contextGraphId, descriptor)) !== null; |
There was a problem hiding this comment.
🟡 Issue: Do not duplicate the materializer's identity-preservation protocol in recovery
What's wrong
This makes the recovery path own part of a policy that the materializer already owns. The abstraction boundary gets muddy: one component decides equivalence, another guesses which metadata rows that decision implies, and the public and private lanes can drift even though they are protecting the same invariant.
Example
The public sync lane treats identity preservation as one operation: select the preserved identity, suppress the returned rows, then repair/replace through the materializer. The private recovery lane now has a second, partial version of that protocol, so future changes to which rows must be withheld or how preservation is repaired must be updated in two places.
Suggested direction
Move this decision into the canonical snapshot materialization layer, or introduce a small shared helper that returns both metaReplaceTargets and the exact rows to suppress. Recovery should consume the same withholdRows/repair contract as the public lane rather than re-deriving it from descriptor shape.
For Agents
Refactor packages/agent/src/sync/requester/swm-recovery.ts so skipped-asset preservation consumes a shared materializer/commit helper rather than reimplementing the decision inline. Preserve current outcomes for skipped equivalent, changed, absent-head, and multi-head cases; prove the refactor with the new GH#2273 recovery cases plus existing public materialization tests.
There was a problem hiding this comment.
Applied in 06886a4 (chain head) — evaluateStoredIdentityPreservation on the materializer is now the ONE preserve decision both lanes consult: it takes the KA lock itself and runs healthy-head + version-certification + the full reader-contract gate stack. The private lane's partial protocol copy is deleted; it makes one call and acts on the result.
There was a problem hiding this comment.
🟡 Issue: Use the materializer’s preservation plan instead of re-deriving withheld rows
What's wrong
The new API claims to centralize the preserve decision and the exact rows to withhold, but the private recovery implementation throws away half of that value and reconstructs the row filter itself. That creates a second owner for the same invariant and makes future changes to the materializer’s suppression rules easy to miss in this lane.
Example
If the materializer later decides that preservation must suppress another descriptor row, the public lane can consume preserved.withholdRows while private recovery silently keeps using only the locally re-derived head-id row set.
Suggested direction
Make the recovery flow consume the withholdRows returned by the materializer as the single source of truth. The local state should be a row-suppression plan, not a list of preserved descriptors that gets interpreted a second time.
For Agents
In packages/agent/src/sync/requester/swm-recovery.ts, change the preservation loop to accumulate preservation.withholdRows directly, and have the identical-id case contribute no rows. Consider extracting a small shared commit/suppression helper with the public lane’s GraphScopedSnapshotCommitCoordinator pattern. Preserve behavior with the identity-preservation suite and the public snapshot materialization tests.
There was a problem hiding this comment.
Applied in 0c71a3c — evaluateStoredIdentityPreservation now returns the withhold plan as part of its discriminated result, and the private lane consumes preservation.withholdRows verbatim: the local re-derivation (preservedHeadIdRowKeys built from descriptor.metadataQuads.filter(...)) is deleted and the insert filter is now new Set(preservedWithholdRows.map(quadKey)) accumulated straight from the decision's return. The existing preserve row (curator head-id row absent after recovery) now exercises exactly this consumption path.
There was a problem hiding this comment.
🟡 Issue: Reuse one metadata commit coordinator instead of adding a second row-suppression ledger
What's wrong
The recovery function is already the orchestration hub for fetching, materializing, replacing graphs, replacing root meta, inserting meta, and hydrating ownership. Adding another hand-rolled metadata commit ledger makes the function harder to scan and creates a second place that must stay aligned with canonicalization and row-withholding rules.
Example
The public lane already has GraphScopedSnapshotCommitCoordinator to suppress rows and produce final bulk rows. Recovery now has a second, ad-hoc version of that same ledger: collect withhold rows, canonicalize metadata, key-filter the insert, and separately track replacement targets.
Suggested direction
Use the existing coordinator concept for recovery too, then let the final insert consume bulkRows(...) rather than open-coding preservedWithholdRows and key filtering here. The unused preservedDescriptors should disappear as part of that simplification.
For Agents
Extract the row-suppression/bulk-insert coordinator from shared-memory-sync.ts into a shared requester module, or move this whole recovery meta-planning step behind the graph-asset recovery policy. Preserve the current behavior: rewritten graphs get meta replacement, preserved skipped graphs withhold only the losing head-id rows, and all remaining verified meta is inserted once.
There was a problem hiding this comment.
🟡 Issue: Private recovery lacks a regression test for withholding all lexical forms of a preserved losing id
What's wrong
The diff adds a new private recovery consumer of the materializer's withhold plan. Existing tests prove the materializer/public lane can handle lexical variants, but they do not prove this new recovery insertion filter consumes that plan correctly. That leaves a meaningful false-green gap around the behavior that prevents preserved heads from becoming multi-valued again.
Example
A recovery payload has the curator head id as both "storage-ack-x" and "storage-ack-x"^^<http://www.w3.org/2001/XMLSchema#string>, while the local stored head preserves op-local. A regression that withholds only one lexical row, or skips the preservedHeadIdRowKeys filtering in recoverContextGraphSwm, would reinsert the other curator id and leave the head multi-valued, but the new private recovery suite would still pass.
Suggested direction
Extend the private recovery suite with the same lexical-variant scenario already covered for the public lane, so this new recoverContextGraphSwm insert-filtering path fails if any losing descriptor id row is reinserted.
For Agents
Add a recoverContextGraphSwm test in packages/agent/test/swm-recovery-identity-preservation.test.ts that serves equivalent curator metadata with plain and xsd:string forms of the losing head shareOperationId, seeds the local preserved identity, and asserts the recovered head contains only op-local. This should exercise the private recovery consumer at swm-recovery.ts lines 569-576, not only the public sync lane.
There was a problem hiding this comment.
Applied in 1a5f175 — the private-lane mirror row: the curator serves its losing head id as BOTH plain and xsd:string-typed literals; the recovered head stays single-valued on the preserved local id, pinning the recovery insert filter against the materializer's withhold plan.
There was a problem hiding this comment.
Position unchanged (final, repeated) — and note the surface keeps shrinking as applied fixes land (the id-equal branch is now deleted; cleanup is a materializer method). The full plan-object unification remains the recorded cross-lane follow-up. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Recovery owns too much of the preservation state machine
What's wrong
The recovery orchestrator now contains a mini state machine for graph-asset identity preservation. Even though the comments say the materializer owns decision and enactment, the recovery layer still knows the outcome semantics and the raw-row withholding ledger. This makes the phase orchestration harder to scan and spreads one policy across two modules.
Example
Preservation requires remembering all of these pieces together: mark rewritten graph keys, skip those keys in the preservation loop, collect materializer withhold rows, call replacement for non-preserved descriptors, canonicalize all verified meta, and filter the withheld row keys before insert. A future recovery mode has to preserve that choreography exactly.
Suggested direction
Extract a dedicated recovery meta planner/materializer, for example planRecoveredGraphAssetMeta(...) or a materializer method that takes descriptors, rewritten keys, and verified meta and returns the exact insertable rows plus replacement actions. That would delete most of the local branching and keep the identity-preservation policy in one module.
Confidence note
This is a structural concern rather than a behavior claim; the current behavior can be preserved while moving the policy behind a cleaner boundary.
For Agents
Move the graph-asset metadata planning into swm-snapshot-materializer.ts or a focused recovery-meta module. Keep recoverContextGraphSwm responsible for fetch/apply phases, and have the helper return/apply { metaReplaceTargets, insertableMeta, insertedMetaQuads } while preserving current row filtering and count behavior.
There was a problem hiding this comment.
Position unchanged (final, repeated). If blocking, flag 🔴 with a failure scenario.
| * still meta-replaced (exactly today's behavior). Production callers SHOULD | ||
| * pass it. | ||
| */ | ||
| readonly snapshotMaterializer?: SharedMemorySnapshotMaterializer; |
There was a problem hiding this comment.
🟡 Issue: Make the recovery materialization mode explicit instead of optional
What's wrong
The new optional creates a hidden two-mode recovery path around a sensitive identity policy. That increases the number of configurations maintainers must reason about and reintroduces the same boundary looseness the materializer was designed to remove.
Example
A caller can provide isGraphAssetMaterialized and replaceMetaForGraphAssets but omit snapshotMaterializer; the function still compiles and runs, but skipped KAs follow a different identity policy with no explicit strategy name or warning.
Suggested direction
Replace the bare optional with a cohesive policy dependency, or require the materializer whenever graph-scoped recovery can skip already-materialized assets. If compatibility tests need old behavior, model that as an explicit strategy so readers can see the intended mode at the call site.
For Agents
Tighten RecoverContextGraphSwmDeps around graph-scoped recovery. Prefer one required recovery materialization policy object for production paths, with an explicit test/no-op strategy where old behavior is intentionally exercised. Preserve existing tests that intentionally omit materialization wiring by making that mode named rather than accidental.
There was a problem hiding this comment.
🟡 Issue: Model the skip/preserve dependency as one typed capability
What's wrong
The code now represents an invalid dependency combination in the type system and repairs it with a fail-fast runtime check. That is a maintainability smell because the central recovery path must keep reasoning about a configuration that production says must not exist.
Example
A new caller can compile with isGraphAssetMaterialized wired but no snapshotMaterializer; the mistake is only discovered at runtime. The recovery implementation then has to carry both the fail-fast guard and the !deps.snapshotMaterializer branch instead of working against a clean dependency shape.
Suggested direction
Use a discriminated union or a dedicated graphScopedMaterializationPolicy object instead of two optional properties plus a runtime guard. That would move the invariant to the type boundary and remove the scattered optional-mode checks from the recovery body.
For Agents
Refactor RecoverContextGraphSwmDeps around an explicit graph-scoped recovery policy. One variant should omit both skip/preserve behavior for legacy tests; the production variant should require both materialization detection and identity preservation. Keep existing behavior for callers that currently omit both.
There was a problem hiding this comment.
Applied in 06886a4 — the hidden two-mode shape is now unrepresentable: recoverContextGraphSwm THROWS fail-fast when a config can skip already-materialized KAs (isGraphAssetMaterialized) without the identity policy (snapshotMaterializer); wire both or neither. Legacy-shape tests wire neither and are unaffected.
There was a problem hiding this comment.
🟡 Issue: Model the skip/preservation dependency as one capability instead of optional peers
What's wrong
The PR adds a new required-in-practice dependency but represents it as another optional field plus a runtime guard. That obscures the real contract and leaves the main recovery flow carrying an invalid half-configured state as a branch, which is exactly the kind of optionality churn that makes this path harder to maintain.
Example
A caller can still construct { isGraphAssetMaterialized, snapshotMaterializer: undefined }; TypeScript accepts it and the invariant is enforced only after metadata fetch. Tests also preserve a legacy shape by omitting both, which keeps the production and test dependency models divergent.
Suggested direction
Collapse isGraphAssetMaterialized and snapshotMaterializer into one typed dependency object, then remove the runtime fail-fast and scattered !deps.snapshotMaterializer checks. That would make the invariant visible at the boundary and reduce the mode branching inside the recovery algorithm.
For Agents
In packages/agent/src/sync/requester/swm-recovery.ts, replace the loose optional pair with an explicit capability boundary, e.g. a discriminated union or graphAssetMaterialization?: { isGraphAssetMaterialized; snapshotMaterializer }. Preserve the legacy no-skip behavior by omitting the whole capability, and prove both modes still run through the existing recovery tests.
No regression test covers the new skip-capable fail-fast contract
What's wrong
This guard is the only runtime verification that a direct recoverContextGraphSwm caller cannot enable already-materialized skips without also providing the identity-preservation materializer. The new tests cover the happy path with both dependencies wired, but not the rejected partial-wiring case, so a future change could remove or bypass the guard while all added tests still pass.
Example
A small regression row could call recoverContextGraphSwm with otherwise minimal deps, a metadata page containing any graph-scoped descriptor, isGraphAssetMaterialized: async () => true, and no snapshotMaterializer, then expect rejection containing 'wire both or neither'. Without that row, deleting or weakening this guard would still leave the new identity-preservation tests green.
Suggested direction
Add a focused negative test for the dependency combination the guard is meant to reject, so future refactors cannot silently restore the pre-fix identity-rotation path for direct recovery callers.
For Agents
Add a focused test in packages/agent/test/swm-recovery.test.ts or packages/agent/test/swm-recovery-identity-preservation.test.ts that constructs graph-scoped metadata, provides isGraphAssetMaterialized but omits snapshotMaterializer, and asserts recoverContextGraphSwm rejects before running the skip-capable recovery path. Preserve existing legacy-shape tests that omit both dependencies.
There was a problem hiding this comment.
Applied in 0c71a3c — the two optional peers are now ONE graphAssetSkip capability object on RecoverContextGraphSwmOptions ({ isGraphAssetMaterialized, snapshotMaterializer }), so the half-configured state is structurally unrepresentable and the runtime fail-fast guard is deleted rather than relocated. Absent object = nothing is skipped (the legacy shape the pre-existing suites pin); the lifecycle type requires it (NonNullable) and wires it at both construction sites.
There was a problem hiding this comment.
Position (final): replaceMetaForGraphAssets is a PRE-EXISTING recovery dependency on testnet-canary (this chain did not add it), and the legacy suites pin the no-materializer shape through it. What this chain changed is that its production implementation now lives ON the materializer (one boundary, one store, one deletion-set rule as of c114f09) and both real wirings consume that method. Absorbing the dep itself into the materializer type is a pre-existing-surface restructure that belongs to the recorded follow-up. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: The dependency boundary still allows half-configured graph-asset recovery
What's wrong
The comments describe skip, preserve, and canonical meta replacement as one materializer-owned capability, but the interface still exposes replacement separately and both dependencies remain optional. That leaves the most important invariant enforced by convention instead of the type boundary.
Example
A future test or caller can pass snapshotMaterializer to enable marker skips and preservation, but omit replaceMetaForGraphAssets; descriptors that return replace then do not get the cleanup path the abstraction comments say is canonical. Conversely, a caller can still wire replacement from one store and the materializer from another because the type keeps them as separate capabilities.
Suggested direction
Make the ownership claim true in the type model: either remove the separate replaceMetaForGraphAssets dependency and always call deps.snapshotMaterializer.replaceMetaForGraphAssets, or introduce one non-optional grouped capability for graph-asset recovery policy. Optional legacy behavior should be an explicit mode, not accidental omission of one half of the capability.
For Agents
In RecoverContextGraphSwmDeps, collapse replaceMetaForGraphAssets into the materializer-owned capability or make a single required graph-asset recovery policy object for production paths. Preserve the legacy no-materializer behavior explicitly, but avoid a type shape where skip/preserve/replacement can be independently wired. Add/adjust compile-time call sites so lifecycle wiring passes one coherent object.
There was a problem hiding this comment.
Position unchanged (final, repeated). If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: Make materialization policy explicit instead of branching on optional wiring
What's wrong
The PR turns one optional predicate into an optional capability that controls skip, preservation, and meta replacement. That keeps a hidden mode flag in the middle of an already busy recovery function and weakens the intended boundary that skip and preserve must come from one store/lock owner.
Example
A reader now has to reason about two recovery modes inside the same function: with a materializer, marker-present graph assets may be skipped and identity-preserved; without it, the same descriptors fall through to graph/meta replacement. That mode switch is driven by optional wiring rather than an explicit policy object.
Suggested direction
Use a required GraphAssetRecoveryPolicy/materializer dependency with a null-object implementation for “never skip, always replace” behavior. The recovery flow can then be linear: ask the policy for each descriptor's meta action instead of checking whether the policy exists.
For Agents
In RecoverContextGraphSwmDeps and recovery tests, make the dependency required and provide a small null/legacy implementation for suites that need the old behavior, or split a legacy wrapper from the production recovery path. Preserve current production behavior and legacy test behavior, but remove optional chaining and !deps.snapshotMaterializer branches from the main algorithm.
There was a problem hiding this comment.
Position unchanged (final, repeated). If blocking, flag 🔴 with a failure scenario.
189bbe3 to
06886a4
Compare
31fc0b9 to
f647cfc
Compare
f647cfc to
2ab59e9
Compare
| /** The materializer's withhold plans, consumed verbatim by the raw insert. */ | ||
| const preservedWithholdRows: Quad[] = []; | ||
| const metaReplaceTargets: GraphScopedSwmRecoveryDescriptor[] = []; | ||
| for (const descriptor of graphScopedDescriptors) { |
There was a problem hiding this comment.
🟡 Issue: Make graph-asset recovery outcomes explicit
What's wrong
The recovery flow now couples distant phases through a negative condition on a string-key set. That makes the control flow harder to scan and brittle to future outcomes, because the code no longer says directly why a descriptor is eligible for preservation or replacement.
Example
A marker-only skip at line 336 only adds incrementallyReadyGraphs; graph rewrites add rewrittenGraphKeys; then line 543 reconstructs the descriptor’s intended metadata action from absence in that set. Any future branch that marks a graph ready without rewriting will automatically enter preservation unless every distant set update is kept in sync.
Suggested direction
Record a typed outcome at the point each descriptor is processed, such as rewritten, skippedAlreadyMaterialized, or pendingAggregate, and derive {replaceTargets, withholdRows} from that model. This would delete the second-pass inference and repeated string-key bookkeeping.
For Agents
Refactor recoverContextGraphSwm around an explicit per-descriptor outcome map or small commit coordinator. Preserve existing skip/rewrite/progress behavior, identity preservation for equivalent skipped KAs, replacement for rewritten/non-equivalent KAs, and legacy replacement when no materializer is supplied.
There was a problem hiding this comment.
Position (final, same family as the cross-lane coordinator follow-up): the lane's obligations after this chain are deliberately minimal — call the single decide-and-enact method for skipped graphs and exclude the returned rows at the ONE raw-insert site. A typed per-descriptor outcome model / shared commit coordinator is the recorded follow-up where it can serve both lanes; grafting it into the converged recovery function now is churn without a behavior change. If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: Recovery state is encoded as parallel side-effect sets instead of an explicit plan
What's wrong
The new identity-preservation flow spreads one decision across several mutable collections and multiple loops. That makes the control flow harder to audit and makes the meta policy depend on incidental bookkeeping rather than an explicit per-descriptor outcome.
Example
A future branch that marks a descriptor ready but forgets to update rewrittenGraphKeys will be routed through the skipped-asset preservation path later, even if that branch actually rewrote the graph or should force replacement.
Suggested direction
Model each descriptor’s recovery outcome directly, then derive replacement targets and withheld rows from that model.
Confidence note
This is a structural maintainability concern rather than a current behavior claim.
For Agents
In packages/agent/src/sync/requester/swm-recovery.ts, replace the parallel sets/lists with a per-descriptor recovery plan, e.g. one map from descriptor or graph key to { graphRewritten, metaAction, withholdRows }. Keep behavior identical, but make the final meta replacement and insert filtering consume that explicit plan.
There was a problem hiding this comment.
Position unchanged (final, repeated) — and the surface keeps shrinking as applied fixes land (id-equal branch deleted; one deletion-set collector as of c114f09). The typed per-descriptor outcome model is the recorded follow-up. If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: Identity preservation still leaks commit bookkeeping into recovery
What's wrong
The PR says the materializer is the single owner of the preserve decision, but the actual invariant is split across the recovery loop and the materializer. That makes the code harder to reason about because the correctness of preservation depends on several caller-side ledgers staying in sync with materializer internals.
Example
The materializer returns withholdRows, but recovery still has to remember to canonicalize processed.verifiedMeta, turn those rows into keys, filter the bulk insert, and route non-preserved descriptors into replaceMetaForGraphAssets. Any future caller or branch that misses one of those steps reintroduces the same class of stacked head rows this change is trying to eliminate.
Suggested direction
Move this whole graph-asset metadata reconciliation into one dedicated abstraction, ideally by reusing/exporting the existing graph-scoped commit coordinator or by adding a materializer method that returns/applyies the final insertable metadata. The recovery function should not manually coordinate skipped-vs-rewritten keys, preservation outcomes, replacement targets, and row suppression.
For Agents
Look at recoverContextGraphSwm around the graph-scoped metadata apply block and the existing GraphScopedSnapshotCommitCoordinator in shared-memory-sync.ts. Preserve behavior, but extract/reuse a single commit coordinator or materializer-level recovery operation that owns canonicalization, row suppression, replacement-target selection, and bulk insert filtering. Tests should continue proving preserved identities do not re-stack descriptor ids and replacement paths still install curator metadata.
There was a problem hiding this comment.
Position unchanged (final, repeated). If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: The one-capability invariant is still split across optional deps
What's wrong
The new design says the materializer owns skip, preserve, and cleanup as one boundary, but recoverContextGraphSwm still wires replacement through a separate optional dependency. That keeps the old half-configured mode alive and makes the type boundary contradict the intended invariant.
Example
A caller can still pass a materializer built over one store and replaceMetaForGraphAssets backed by another, or pass a materializer while omitting replaceMetaForGraphAssets; both states are representable even though the new comments say they should not be.
Suggested direction
Fold graph-asset meta replacement into the recovery materializer path, or model legacy fallback as a discriminated union. The recovery function should not independently accept a preservation materializer and a separate replacement callback when correctness depends on them sharing the same store and locks.
For Agents
Refactor RecoverContextGraphSwmDeps so graph-asset skip, preserve, and meta replacement cannot be wired independently. Preserve legacy no-materializer behavior if needed with an explicit discriminated config, then update lifecycle and tests to use the single recovery capability.
There was a problem hiding this comment.
Position unchanged (final, repeated). If blocking, flag 🔴 with a failure scenario.
0f5a0c3 to
f7684ce
Compare
a8a3981 to
30fb3a6
Compare
d951dc3 to
52e00da
Compare
GH#2273 part 3/3. The private curator-recovery lane rotated a member-author's head to the curator's operation id precisely BECAUSE content matched: the per-KA continue skipped only the graph replace, then the unconditional bulk replaceMetaForGraphAssets(graphScopedDescriptors) deleted the local head + operation subject and the raw verifiedMeta insert installed the curator's identity - the single-step form of the rotation that terminally kills queued VM-publish jobs frozen on the local id. - the bulk meta replacement is DECISION-DRIVEN: only descriptors whose assertion graph was actually (re)written this run, plus skipped descriptors that FAIL the preserve check, are replaced. A skipped KA is preserved only when the local head is healthy (single-valued), certifies the descriptor's version, and its operation is identity-equivalent under the KA write lock (same selectRepairIdentity primitive as part 2). Absent, multi-valued, wrong-version and non-equivalent states all replace, so the curator stays authoritative for genuine changes and the #2050 G7 absent-head repair is untouched. - the raw meta insert is canonicalized (the parser accepts equivalent two-id payloads; an uncanonicalized insert could stack both ids) and each preserved KA's head-id row is withheld so the union cannot re-stack the curator's id onto the preserved head; the curator's operation subject still lands as immutable history (same disposal as the public lane). - capability threaded as optional snapshotMaterializer through RecoverContextGraphSwmFromPeerDependencies and wired at BOTH lifecycle construction sites (on-connect recovery + the recover-shared-memory route); absent => byte-for-byte today's behavior. Tests (opt-in real replaceMetaForGraphAssets in the lifecycle's shape + real materializer; fail-before proven): preserve row and canonicalization row FAIL against the old lane; changed-share, absent-head and multi-valued polarity rows pin today's behavior in both states. Suite 23/23 green; existing 18 rows untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…le-pinned wiring PR3 review round 1 (otReviewAgent), all four findings addressed (three here, file split follows): - evaluateStoredIdentityPreservation on the materializer is now the ONE preserve decision BOTH lanes consult (takes the KA lock itself; healthy head + version certified + the full reader-contract gate stack) - the private lane's partial protocol copy is gone. - fail-fast: a recovery config that can SKIP already-materialized KAs (isGraphAssetMaterialized) without the identity policy (snapshotMaterializer) now THROWS instead of silently running the pre-fix rotation path - the hidden two-mode shape is unrepresentable at runtime. - the lifecycle-level dependency type REQUIRES snapshotMaterializer: removing a production construction-site wiring is now a COMPILE error (the low-level recovery dep stays optional for legacy-shape tests, which wire neither capability). Agent lanes 59 green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR3 review round 1, finding 4/4: the GH#2273 describe moves to swm-recovery-identity-preservation.test.ts (swm-recovery.test.ts returns under 1k lines and keeps the lane's transport/apply/progress rows). Registered in the unit config; CI shards auto-discover. 23/23 green across the pair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…consume withhold plan
Review round 2 of GH#2273 PR3:
- evaluateStoredIdentityPreservation now returns a discriminated
{outcome:'preserve'|'replace'} and handles the id-equal case INSIDE the
same health + version conjuncts: a same-id head with a stale or corrupt
assertionVersion row is replaced, not preserved (the round-1 fast path
skipped version certification and would have preserved it, letting the
raw insert union the descriptor's version row onto a stale head).
- The private recovery lane consumes preservation.withholdRows verbatim
for its insert filter instead of re-deriving the head-id rows locally.
- isGraphAssetMaterialized + snapshotMaterializer are now ONE
graphAssetSkip capability object on RecoverContextGraphSwmOptions:
half-configuration is unrepresentable, replacing the runtime fail-fast
guard; lifecycle wires the object at both construction sites.
- New fail-before row: same-id head with version row drifted to "2" while
the descriptor certifies "1" must be meta-replaced (head converges to a
single "1" version row). Verified failing under the reintroduced
round-1 fast path, passing after.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 3 of GH#2273 PR3: - evaluateStoredIdentityPreservation -> preserveStoredIdentityForSkippedAsset: on 'preserved' the method now REWRITES the head (repairHeadPreservingIdentity, descriptor rows + winner id) inside the same KA-lock hold as the decision. The health check models only version/id cardinality, so a preserve that merely skipped replacement kept residue head rows (e.g. an extra stale assertionGraph row) the pre-fix bulk replacement would have repaired. New row proves the rewrite purges residue; verified failing with the enactment removed. - New serialization row: holds the exact KA write lock through the materializer's own keying and proves the decision neither reads nor settles until release; verified failing with withKeyedLocks bypassed. - Canonicalization row now asserts the exact selected winner (storage-ack-z under the parser's latest-publishedAt/descending-id rule), not just single-valuedness. - Drop three dead imports left in swm-recovery.test.ts by the suite split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lacement shared Review round 4 of GH#2273 PR3: - SharedMemorySnapshotMaterializer now owns BOTH halves of the skip/preserve capability: new hasGraphAssetMarker (the private lane's marker-only ASK, moved verbatim from the lifecycle closures; digest-blindness documented as the F2 follow-up) next to preserveStoredIdentityForSkippedAsset. The recovery dep collapses to a single optional snapshotMaterializer — pairing a predicate from one store with a materializer over another is now unrepresentable, and the lifecycle wiring at both sites is one factory call instead of a hand-assembled object. - Production graph-asset meta replacement extracted to exported replaceWorkspaceMetaForGraphAssets (same module); the lifecycle delegates to it and the identity suite imports it — the test-local shadow copy and local marker predicate are deleted, so the suite exercises the canonical ownership guard and deletion set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 4 nit on GH#2273 PR3: withhold rows are the only consumed preservation output; the accumulator suggested a second effect that does not exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups on GH#2273 PR3: - The serialization row now counts every store access (query/insert/ deleteByPattern via a counting Proxy) and asserts ZERO before the lock releases: a regression that read the head before acquiring and only awaited the lock to return kept the settlement assertion green while racing live writes. Verified discriminating with a pre-lock readStoredHead mutant. - The repeated GH#2273 incident narratives in the preserve-method doc and the decision-loop comment are condensed to invariant-focused statements. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cleanup Review follow-ups on GH#2273 PR3: - The id-equal preserve branch is DELETED: when the stored id equals the descriptor's, replacement IS identity-preserving by construction (the descriptor reinstalls the same id) and is the only healer for op-subject corruption — a duplicate singleton row survives any union insert, and a preserve-skip parked the KA on rows the resolver fails closed on forever. 'preserved' now covers only foreign equivalent winners. New row (same-id skipped asset with a duplicate accessPolicy row => replaced, single verified policy row after recovery) verified failing under the preserve branch. - replaceMetaForGraphAssets moves ONTO the materializer interface (bound to its one store); the raw-TripleStore export is demoted to the private implementation, and both the lifecycle wiring and the recovery suite consume the materializer method — one boundary owns skip, preserve and canonical cleanup. - New private-lane row: the curator serving its losing head id in both plain and xsd:string forms leaves the preserved head single-valued (pins the recovery insert filter against the withhold plan). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR3: collectOwnedHeadOperationSubjects is reshaped onto the head-join query (op subjects carrying a head-linked shareOperationId AND the descriptor's kaUal — no contextGraphId needed, and non-convention subjects are found too), with the head subject itself excluded (it carries both joined predicates but is the thing being rewritten). replaceMetaForGraphAssets, replaceHeadMetadata and repairHeadPreservingIdentity now all consume this ONE collector, so the ownership/deletion rule cannot drift between full replacement and the preserving repair. The duplicate join implementation inside the moved cleanup helper is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR3: readStoredHead, selectRepairIdentity and repairHeadPreservingIdentity are hoisted to closed-over consts and the returned object is built from them — no self-referential dispatch, so decorating a public method cannot silently change internal composition, and internal steps are visibly distinct from extension points. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…marker Review follow-ups on GH#2273 PR3: - insertedMetaQuads (and the log line) now report the rows that actually reached the store after canonicalization and preserve-withholding, not the raw verified payload size. Count assertion added to the preserve row (payload minus the one withheld head-id row) — verified failing under the payload-size mutant. - New graph-backed row: an aggregate graph REWRITE routes the KA through meta replacement even when the local head is identity-equivalent (the content is now the curator's; preserving the old identity would certify content the local operation never produced) — verified failing with the rewrittenGraphKeys marker removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
52e00da to
8d95666
Compare
2 participants
Summary
continuefor an already-materialized KA skipped only the graph replace, after which the unconditional bulkreplaceMetaForGraphAssets(graphScopedDescriptors)deleted the member-author's head + operation subject and the rawverifiedMetainsert installed the curator's identity — rotating precisely because content matched, and terminally killing queued VM-publish jobs frozen on the local id.selectRepairIdentityprimitive as part 2, with all its reader-contract gates). Preservation is a decide-and-ENACT: the materializer rewrites the head (descriptor rows + preserved winner id, purging residue) under the same lock hold and returns the exact withhold plan the raw insert applies. A same-id skipped KA is deliberately replaced — replacement is identity-preserving by construction there and is the only healer for op-subject corruption. Absent, multi-valued, wrong-version and non-equivalent states all replace — the curator stays authoritative for genuine changes and the SWM catch-up can leave a partial unrecovered tail after phase timeouts under backpressure #2050 G7 absent-head repair is untouched.df59920d4, so an uncanonicalized insert could stack both ids onto a freshly recovered head), and each preserved KA's head-id row is withheld so the union cannot re-stack the curator's id onto the preserved head. The curator's operation subject still lands as immutable history — the same disposal as the public lane.hasGraphAssetMarker), preserve decision+enactment, and canonical meta replacement (one shared deletion-set collector) over one store and lock map; reportedinsertedMetaQuadscounts rows that actually reached the store. Wiring is optional and defaulted to today's behavior: the capability threads assnapshotMaterializer?throughRecoverContextGraphSwmFromPeerDependenciesand is wired at BOTH lifecycle construction sites (on-connect private recovery and therecover-shared-memoryroute). Absent, every skipped KA is still meta-replaced byte-for-byte as today.Related
selectRepairIdentitygate stack verbatim)Diagrams
Private curator recovery of an unchanged, already-materialized KA
Before:
sequenceDiagram participant Curator participant Lane as Private recovery lane participant Store as Member local store Curator->>Lane: full CG meta with curator op id X Lane->>Store: per KA check finds content already materialized Note over Lane: continue skips only the graph replace Lane->>Store: bulk replaceMetaForGraphAssets over ALL descriptors Note over Store: local head and operation Y deleted Lane->>Store: raw verifiedMeta insert installs curator id X Note over Store: identity rotated, queued job frozen on Y diesAfter:
sequenceDiagram participant Curator participant Lane as Private recovery lane participant Store as Member local store Curator->>Lane: full CG meta with curator op id X Lane->>Store: per KA check finds content already materialized Lane->>Store: preserve check under the KA lock Note over Lane: healthy head, version certified, operation equivalent Lane->>Store: meta replace runs ONLY for rewritten or failed-check KAs Lane->>Store: canonicalized insert without the curator head id row Note over Store: head keeps id Y, curator operation X lands as historyFiles changed
packages/agent/src/sync/requester/swm-recovery.tsmetaReplaceTargets(rewritten-graph tracking + preserve check via the optional materializer under the KA write lock); canonicalized, preserved-head-id-filtered raw meta insert; optionalsnapshotMaterializerdep (absent ⇒ today's behavior)packages/agent/src/dkg-agent-lifecycle.tssnapshotMaterializerthreaded throughRecoverContextGraphSwmFromPeerDependenciesand wired at both construction sites (on-connect + route)packages/agent/test/swm-recovery.test.tsreplaceMetaForGraphAssets+ real materializer (opt-in per row — existing rows deliberately unchanged): preserve row, changed-share polarity, absent-head G7 row, multi-valued-head row, two-id canonicalization rowTest plan
storage-ack-x/ two ids stacked); the changed-share, absent-head and multi-valued polarity rows pin today's behavior in BOTH statesswm-recovery.test.ts23/23 green (18 pre-existing rows untouched + 5 new)snapshotMaterializerabsent, the lane is byte-for-byte today's behavior (all 18 pre-existing rows run without the capability)pnpm build:packages(turbo + tsc) green on the chained head🤖 Generated with Claude Code