fix(agent): preserve SWM head operation identity across catch-up (GH#2273 2/3) - #2286
fix(agent): preserve SWM head operation identity across catch-up (GH#2273 2/3)#2286Jurij89 wants to merge 20 commits into
Conversation
GH#2273 part 2/3 - the Core Rule: when the locally stored operation rows and an incoming descriptor's operation rows are equivalent over the operation- identity allow-list, the identity the local store already carries wins. Catch-up may retain the remote operation subject as immutable history, but must not add, remove, or replace the head's shareOperationId. Any genuine difference (content digest, counts, private root, access policy, allowed peers, ownership, version) routes to today's behavior - remote authority. Mechanism fixed (both stages): the per-KA skip for already-materialized content let the round's bulk verified-meta union-insert stack the peer's head-id row beside the local one (stage 1); the next round's needsRepair then deleted the head AND the local operation subject, re-inserting only the remote identity (stage 2) - terminally killing queued VM-publish jobs whose preflight froze the local id at admission. - graph-scoped-swm-recovery: operationIdentityKey over an explicit ALLOW-LIST (deny-listing fails: publicSnapshotGraph embeds the operation id in its VALUE; publisherPeerId/publishedAt are legitimately per-node), with wire-vs-stored normalization (xsd:string, integer canonical form). - swm-snapshot-materializer: readStoredHead exposes the single unambiguous shareOperationId (null when absent or multi-valued); selectRepairIdentity decides preservation under the held KA lock; repairHeadPreservingIdentity heals a (possibly multi-valued) head to the winning stored identity WITHOUT ever deleting the winner's operation rows. - shared-memory-sync: row-level suppressRows on the commit coordinator (descriptor-level would withhold the four required head rows after a repair and manufacture a permanently corrupt head); prefer-stored at the healthy-skip exit; repairOrReplaceHead at BOTH replaceHeadMetadata call sites; the version-superseded exit now withholds the stale descriptor's head rows from the bulk append (two-VERSION heads were the same hazard arriving via the metadata side). Tests (all fail-before proven against the old lane): round-trip identity-key spike (wire vs Oxigraph read-back - the comparison the whole fix rests on), two-stage repro, materialize-path repro, prefer-stored polarity row, version-superseded row, graph-backed-KA control (bulk insert remains their only head writer - suppression is decision-driven, never blanket), repair rows, and an e2e row in e2e-memory-layers where a REAL agent share + queued intent survives a catch-up round offering an equivalent id, verified by the REAL preflight (peer batch built by reading the local rows back and re-labeling, so equivalence is by construction; anti-vacuity pinned via insertedMetaTriples > 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * The loser id row is suppressed so neither the per-KA meta insert nor | ||
| * the round's bulk append re-stacks it onto the repaired head. | ||
| */ | ||
| const repairOrReplaceHead = async ( |
There was a problem hiding this comment.
🟡 Issue: Identity preservation is split across orchestration and row-suppression side effects
What's wrong
This PR embeds GH#2273-specific head identity policy into an already large sync orchestration function. The materializer owns store state, but the requester owns part of the decision by selecting identities, calling repair, suppressing only certain rows, and then inserting verified metadata. That makes the behavior depend on scattered sequencing rather than a single head-commit contract, and it worsens the readability and extensibility of the sync loop.
Example
A future head-write path can call replaceHeadMetadata or repairHeadPreservingIdentity and forget to also suppress the losing descriptor head row, allowing the final bulk append to re-stack the rejected shareOperationId onto the head. The current code already has to remember this coupling in two separate branches.
Suggested direction
Move the whole head identity decision into one explicit abstraction, such as a materializer/commit-coordinator method that returns a HeadMetadataDisposition including the rows to suppress. The sync loop should not need to know which exact metadata row must be withheld after a preserved repair.
For Agents
Look at shared-memory-sync.ts around descriptorHeadIdRows, repairOrReplaceHead, and the already-materialized branch, plus the new materializer interface methods. Preserve behavior: equivalent stored IDs win and descriptor head-id rows are suppressed; genuine changes still adopt descriptor metadata. Prove both repair and already-materialized paths share the same disposition/helper.
There was a problem hiding this comment.
Applied in b6c1f98 in the contained form: withholdDescriptorIdentity is now the ONE preserve step owning suppress+log, and both preserve paths (healthy-skip and repair) go through it — deciding without withholding is no longer expressible in this loop. I kept the requester/materializer split itself: the materializer owns store mutations, the requester owns round-scoped commit bookkeeping (the suppression ledger is intrinsically a property of THIS round's bulk append, not of the store), and a single head-commit object crossing that boundary is the kind of restructure that belongs with the post-chain module extraction agreed on the part-1 PR.
There was a problem hiding this comment.
🟡 Issue: Collapse preserve/repair/suppress into one operation
What's wrong
The comment says there is one preserve decision, but the implementation spreads that decision across the sync loop, materializer, and row-suppression ledger. This adds feature-specific branching to an already large orchestration function and makes the invariant dependent on every caller remembering the same sequence.
Example
A future materialization branch could call selectRepairIdentity and repair/preserve the head but forget to call snapshotCommit.suppressRows(...); the round's bulk insert would then re-stack the rejected descriptor id. The current design relies on call choreography rather than a cohesive operation.
Suggested direction
Expose a single operation that returns the preserved identity and the exact metadata rows to suppress, or let the commit coordinator own the preserve decision directly. The sync loop should only choose high-level states like skip, repair, or materialize, not coordinate identity-ledger internals.
For Agents
Work in shared-memory-sync.ts and swm-snapshot-materializer.ts. Move identity selection, preserved-head rewrite decision, and suppression-row reporting behind one materializer/commit-coordinator abstraction. Preserve both current paths: repairing a bad head and suppressing a foreign equivalent id on a healthy head.
There was a problem hiding this comment.
Applied in f69045e — decideAndWithholdStoredIdentity is now the one preserve operation: it selects the winner AND suppresses the losing descriptor id row in the same call, so deciding without withholding is no longer expressible; the sync loop's two branches just call it and act on the returned winner (repair vs nothing). The materializer keeps store mutations, the coordinator keeps the round-scoped ledger — moving the decision fully into the coordinator crosses the store/round boundary and belongs with the post-chain module extraction agreed on the part-1 PR.
There was a problem hiding this comment.
🟡 Issue: Identity preservation is wired into the sync loop as scattered orchestration
What's wrong
This adds feature-specific control flow to an already large orchestration function instead of reducing the concept to a single reusable head-write policy. The comments are compensating for an implicit protocol, and the unused helper is a symptom that the local abstraction did not settle cleanly.
Example
A future head-rewrite branch must know to call decideAndWithholdStoredIdentity, maybe call repairHeadPreservingIdentity, maybe suppress rows, and maybe insert descriptor metadata. That is an implicit protocol spread across the sync loop instead of a single head-write abstraction.
Suggested direction
Make the identity-preserving head rewrite a first-class operation owned by the materialization layer or commit coordinator. The sync loop should select between high-level outcomes, not manually coordinate winner selection, suppression ledger updates, logging, repair, replacement, and metadata insertion in several branches.
For Agents
Focus on runSharedMemorySync, GraphScopedSnapshotCommitCoordinator, and the materializer interface. Preserve the current row suppression and head-repair behavior, but collapse the decision/write/suppression protocol behind one operation such as a materializer-returned head write plan or applyDescriptorHeadMetadata. Add or keep tests covering both existing call sites and the bulk-append suppression.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Head preservation is bolted into the large sync loop
What's wrong
This adds another special-case orchestration layer inside an already sprawling sync function. The amount of explanatory commenting is a symptom: the invariants are not captured by the abstraction, so the reader must reconstruct them from closures over snapshotCommit, snapshotMaterializer, pid, and summary.
Example
The materialized-content branch calls repairOrReplaceHead, which also inserts descriptor metadata. The healthy-but-different-id branch calls decideAndWithholdStoredIdentity and repairHeadPreservingIdentity directly. A future call site has to know which helper writes metadata, which only suppresses rows, and which ordering constraints are implicit.
Suggested direction
Extract a focused head-reconciliation workflow so the sync loop stays at the level of superseded/materialized/replace-graph decisions. The preservation-specific suppression, repair, and metadata-write ordering should be centralized behind one cohesive API.
For Agents
Refactor the new preservation orchestration out of runSharedMemorySync around materializeReadySnapshot. Preserve lock ordering, graph-before-head ordering, suppression-before-bulk-append, and summary counting. A small workflow object or pure reconciler should own the preserve/replace decision and expose one call per branch.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Collapse identity preservation into a single repair abstraction
What's wrong
The new flow is guarded by comments rather than by the abstraction. Three consecutive docblocks explain that deciding and withholding are inseparable, but the implementation still separates decision, ledger mutation, and repair across two objects and several branches in runSharedMemorySync. That makes the hot sync path harder to scan and leaves a fragile protocol for future maintenance.
Example
The intended atomic idea is "preserve stored identity and withhold the losing head row", but callers have to perform selectRepairIdentity -> suppressRows -> repairHeadPreservingIdentity in the right order. A future call site can easily preserve without withholding, or withhold without repairing, because the type boundary still exposes the pieces separately.
Suggested direction
Have the materializer or a dedicated head-repair coordinator return/apply an explicit HeadRepairPlan that includes the winner and rows to suppress, or expose one repairOrReplaceHeadMetadata operation that owns the preservation workflow. The sync loop should orchestrate phases, not manually coordinate the invariants of the repair protocol.
For Agents
Look at GraphScopedSnapshotCommitCoordinator, SharedMemorySnapshotMaterializer.selectRepairIdentity, and repairHeadPreservingIdentity. Preserve current ordering and counters, but collapse the decision, suppression plan, and head rewrite into a single typed repair plan or higher-level head repair operation. Add/update a test proving the sync loop cannot select a preserved identity without also withholding the losing descriptor row.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
…preserve step PR2 review round 1 (otReviewAgent), both reds + two yellows applied: - repairHeadPreservingIdentity rewrites the HEAD first and deletes loser operation subjects AFTER: a crash between the two now leaves a healthy single-valued head plus benign identity-equivalent residue, where the previous order left a multi-valued head naming operations whose rows were already gone - readers failed closed on a half-repaired state. - selectRepairIdentity refuses winners that are identity-equivalent but NOT resolvable operations (id-echo missing, or publisherPeerId absent / multi-valued - both deliberately outside the identity key): preserving such a winner would write a head the resolver permanently fails as corrupt, and the next round's prefer-stored decision would preserve it again, wedging the KA. Descriptor-wins is the fallback. Fail-before proven (new sub-row fails against the unguarded selection). - one withholdDescriptorIdentity step owns suppress+log for BOTH preserve paths - deciding without withholding is no longer expressible. - new sync-loop row for the r26 state (head + local op present, content absent): the graph materializes from the equivalent peer snapshot AND the local identity survives - pins the materialize-path call site a revert to replaceHeadMetadata would silently break. - new allow-list polarity rows: same-digest descriptors adding an allowList envelope or a different author are NOT identity-equivalent. Agent SWM lanes 121 green + e2e catch-up-survival row green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… preserve operation PR2 review round 2 (otReviewAgent), all three applied: - the red was correct again: publishedAt is outside the identity key (per-node clocks) AND was outside the resolvability guard, so a stored winner with a corrupt timestamp would be preserved, fail every reader as corrupt, and be preserved again next round. selectRepairIdentity now mirrors the resolver's timestamp rule (every stored publishedAt must parse to a safe non-negative epoch). Fail-before proven (new sub-row). - normalizeIdentityObject now delegates literal-term semantics to @origintrail-official/dkg-rdf-utils (parseRdfLiteralTerm + formatCanonicalRdfLiteralTerm: plain==xsd:string, escaping, language tags, datatype brackets - the same rules storage/hash paths recognize), keeping only the identity-specific integer-value canonicalization as a wrapper. Corrects my earlier claim that no shared helper existed - the reviewer was right and the package was there. New workspace dep wired (agent package.json + lockfile). - decideAndWithholdStoredIdentity is the ONE preserve operation: deciding IS withholding (the losing id row is suppressed in the same call that selects the winner), so no call site can preserve and forget the ledger. Agent SWM lanes 121 green; e2e catch-up-survival row green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * author) under different operation ids — the residue storage-ACK persistence | ||
| * and originator persistence legitimately produce for one share. | ||
| */ | ||
| export function operationIdentityKey(rows: readonly Quad[]): string | null { |
There was a problem hiding this comment.
🟡 Issue: Avoid maintaining two operation-equivalence models
What's wrong
This adds a second definition of when two share operations are “the same” while the existing resolver keeps its own equivalence construction. That makes the most delicate invariant in this change easy to drift: one path can be updated while the other silently keeps different rules.
Example
A future operation metadata predicate now has two places to reason about equivalence: add it to OPERATION_IDENTITY_PREDICATES.compared for repair decisions, and separately decide whether the parser's raw equivalenceKey should include or ignore it.
Suggested direction
Move operation identity into a dedicated helper/module and use it for both multi-head descriptor resolution and stored-vs-descriptor repair comparison. If the semantics truly differ, encode that as a named mode rather than two independent key builders.
For Agents
Look in graph-scoped-swm-recovery.ts. Extract one shared operation-identity/equivalence abstraction and route both resolveEquivalentHeadOperation and materializer repair selection through it, with any intentional parser-vs-repair differences made explicit and covered by existing identity tests.
There was a problem hiding this comment.
Deferring, with the concrete reason: unifying the two relations means changing the SHIPPED intra-payload equivalence (resolveEquivalentHeadOperation's deny-list key includes publicSnapshotGraph, whose value embeds the operation id — which is why byte-identical snapshot-graph-backed ops currently throw 'ambiguous shareOperationId' on snapshot-disabled nodes). Unification on the allow-list would silently start ACCEPTING payloads that are rejected today — a behavior change to the shipped parser that deserves its own review, and it is exactly the F3 follow-up recorded on the part-1 PR alongside the module extraction where both relations will live together. The drift risk in the interim is bounded: both relations are pinned by fail-closed tests in both polarities.
There was a problem hiding this comment.
🟡 Issue: Avoid introducing a second operation-identity model
What's wrong
This adds a new semantic identity key beside an existing equivalence calculation. The codebase now has two places that define what “same share under a different operation id” means, which is a structural drift risk for a very sensitive path.
Example
If a future metadata field affects share identity, maintainers now have to update OPERATION_IDENTITY_PREDICATES and the older equivalenceKey construction separately. If they drift, parser-time multi-head resolution and repair-time identity preservation can disagree about whether two operation ids represent the same share.
Suggested direction
Move the identity logic into a dedicated operation identity module or typed decoder, and have the existing head resolver delegate to it instead of maintaining two comparison schemes.
For Agents
Look in packages/agent/src/sync/graph-scoped-swm-recovery.ts around operationIdentityKey and resolveEquivalentHeadOperation. Preserve current behavior, but extract one canonical operation identity/decoded-operation model and make both descriptor parsing and materializer repair use it. Add a focused regression around a multi-id head to prove both paths make the same identity decision.
There was a problem hiding this comment.
Standing position (fourth iteration of this finding), now final for this PR: the two relations are DELIBERATELY different today — the parser's deny-list key includes publicSnapshotGraph (op-id-embedding, the F3 self-ambiguity) and requires a single publishedAt, while the identity key excludes per-node rows precisely so the storage-ACK residue compares equal. Unifying them changes the SHIPPED parser's acceptance behavior and is exactly the F3 follow-up recorded on the part-1 PR, where both relations move into one module and get one owner. Both are pinned in both polarities in the meantime. If you consider this blocking for the bugfix, flag 🔴 with the failure scenario.
There was a problem hiding this comment.
🟡 Issue: Consolidate the operation identity model instead of adding a second one
What's wrong
The PR adds a second definition of operation equivalence beside an existing one. Even if both are individually defensible, the codebase now has multiple places where identity semantics are encoded with different predicate sets and normalization rules, which is a long-term maintainability trap.
Example
A future operation row such as another access-envelope predicate, locator shape, or per-node metadata field now has to be classified consistently in several places: OPERATION_IDENTITY_PREDICATES, resolveEquivalentHeadOperation's equivalence key, isResolvableWorkspaceOperationRows, and the materializer's manual locator checks. Missing one of those spots changes which identities are considered equivalent in one path but not another.
Suggested direction
Introduce one canonical helper/model that decodes operation rows into stable identity fields, per-node fields, and locator fields, then use that from both the recovery parser and the preservation decision. The code should express mode differences explicitly instead of relying on parallel string-key implementations.
For Agents
Look at packages/agent/src/sync/graph-scoped-swm-recovery.ts and packages/agent/src/sync/requester/swm-snapshot-materializer.ts. Preserve current behavior, but extract a single typed workspace-operation identity/validation model that both descriptor parsing and repair selection consume. Tests should keep the current equivalent-id, changed-content, envelope, subgraph, and locator cases green.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Operation identity policy is split across ad hoc key builders
What's wrong
The new identity-preservation logic introduces a second operation-equivalence model beside the parser's existing byte-equivalence model. The comments explain why they differ, but the structure leaves future maintainers to synchronize two implicit policies manually, which is exactly the kind of hidden coupling this change should be deleting.
Example
If a new operation predicate is added, the author must remember whether it participates in same-payload byte ambiguity, cross-store identity preservation, both, or neither. Today that policy is encoded in two separate loops plus long comments instead of one canonical model.
Suggested direction
Extract the equivalence policy into a dedicated module with named modes and typed predicate classification. That would make the intentionally different comparison rules discoverable without duplicating policy in parser and materializer code paths.
For Agents
Create a focused SWM operation-equivalence module. Preserve current behavior by exposing two explicit modes, for example samePayloadEquivalenceKey and crossStoreIdentityKey, backed by shared predicate classification and literal normalization helpers. Update the parser and materializer to import that module.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Unify the operation identity policy before adding another equivalence model
What's wrong
The PR adds a second operation-equivalence model next to the existing parser equivalence and relies on long comments to explain why they differ. That preserves behavior, but it increases the number of concepts future changes must hold in their head and makes predicate evolution error-prone.
Example
A future operation predicate now has to be classified across two hidden policies: the parser byte key includes it automatically, while cross-store preservation ignores it unless someone also updates OPERATION_IDENTITY_PREDICATES. The comment notes this split as follow-up F3, which is a sign the PR is knowingly landing the design before the abstraction exists.
Suggested direction
Create a small workspace-operation-equivalence/identity-policy module that owns both key builders and the predicate classification rules. The parser and repair selector should consume that module instead of carrying parallel explanations and implicit drift points.
For Agents
Extract the operation-equivalence policy out of graph-scoped-swm-recovery.ts into a focused module, with explicit modes for same-payload byte equivalence and cross-store identity equivalence. Preserve the existing two behaviors, but make predicate classification and normalization live in one place and update the current tests to call that module directly.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
| * any stored operation's rows are missing or non-equivalent, or when either | ||
| * side's identity key is unprovable. Callers MUST hold the KA write lock. | ||
| */ | ||
| selectRepairIdentity( |
There was a problem hiding this comment.
🟡 Issue: Collapse the temporal coupling in head identity repair
What's wrong
The new API splits one invariant across layers: the materializer decides identity equivalence and performs preserving repair, while the sync loop must know which rows to withhold from later appends. The result is a fragile call-order contract enforced by comments rather than types.
Example
A future call site can call selectRepairIdentity, get a winning stored id, but forget to suppress the losing descriptor head row from the bulk append; the comments describe exactly that as invalid, but the API still permits it.
Suggested direction
Make the materializer own the complete head-repair decision and return the exact suppression/write plan needed by the sync commit coordinator. That would remove the public “select, then maybe repair, and separately suppress rows” protocol.
For Agents
Look at SharedMemorySnapshotMaterializer and the nested helpers in runSharedMemorySync. Replace the split select/repair/suppress protocol with one typed materializer operation or plan, e.g. reconcileHeadMetadata(...) -> { rowsToSuppress, action }, and keep the sync loop responsible only for orchestration and commit-ledger application.
There was a problem hiding this comment.
Applied the type-level half in afb4b00: selectRepairIdentity now returns { winnerShareOperationId, withholdRows } — the decision carries its complete suppression plan in one value, and the sync loop's single decideAndWithholdStoredIdentity operation consumes that plan (no re-derivation, no second lookup to forget). Full coordinator ownership of the ledger crosses the store/round boundary and stays with the post-chain module extraction, same position as the prior round.
There was a problem hiding this comment.
🟡 Issue: Collapse the split identity-preservation protocol
What's wrong
The new API encodes one logical operation as a multi-step caller protocol spread across the materializer, the sync loop, and the commit coordinator. That makes the flow harder to reason about and relies on comments to enforce ordering instead of the type boundary.
Example
The comments say deciding and withholding must be inseparable, but the API makes them separable: a future caller can select a winner without suppressing the losing head row, or repair with a winner that was selected under a stale read.
Suggested direction
Make the materializer return/apply a single head-rewrite plan, or move the whole decision and repair behind one method, so callers cannot forget half of the protocol.
For Agents
Refactor SharedMemorySnapshotMaterializer and the call sites in shared-memory-sync.ts. Preserve the current preserve-vs-descriptor behavior, but expose one operation such as planHeadRewrite/applyHeadRewrite or repairOrReplaceHeadMetadata that owns decision, mutation, and returned suppression rows as one typed result. Existing tests around identity preservation should still pass.
There was a problem hiding this comment.
Standing position (fourth iteration), final for this PR: the current shape is one decision API that returns its complete plan ({winner, withholdRows}), one loop-side operation that consumes it atomically, and a materializer that owns every store mutation — the stale-read concern is bounded by the KA write lock both the decision and the repair run under. Fusing decision+repair+ledger into one method requires the materializer to own the commit coordinator's round state, which is the module-extraction boundary move. If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: Collapse the identity-preserving repair protocol into one explicit plan
What's wrong
The preservation invariant is not represented as one abstraction. It is split across two materializer methods, a nested helper inside a 1.8k-line sync function, and several comments saying callers must hold locks and must suppress rows. That is a brittle protocol boundary rather than a maintainable model.
Example
A future head-rewrite branch could call selectRepairIdentity, then forget snapshotCommit.suppressRows(preserved.withholdRows), or call repairHeadPreservingIdentity with a winner that did not come from the selector. The current implementation relies on comments and a local closure to keep that protocol intact.
Suggested direction
Replace the split selectRepairIdentity / repairHeadPreservingIdentity protocol with a discriminated HeadRepairPlan or a single materializer operation that owns the selection and exposes exactly the suppression rows plus the allowed rewrite. The sync loop should consume a plan, not reassemble an invariant spread across methods and comments.
For Agents
Look at SharedMemorySnapshotMaterializer, decideAndWithholdStoredIdentity, and the two call sites in runSharedMemorySync. Preserve the same repair/replace behavior, but collapse the multi-step protocol into one typed operation or repair plan so selection, suppression rows, and the permitted write action are produced together and cannot be recombined incorrectly.
New materializer contract is not verified against all typed implementations
What's wrong
The PR expands the materializer interface, but at least one existing typed materializer wrapper in the tests was left with the old method set. That leaves the interface change only partially validated and can hide breakage in callers that provide their own materializer object.
Example
A type-aware validation of that test file would fail with the object missing selectRepairIdentity and repairHeadPreservingIdentity. If tests are not typechecked, this also means custom materializer implementations can drift from the new contract without validation.
Suggested direction
Add the new methods to the remaining typed materializer wrapper and include a validation step that typechecks or otherwise exercises these test doubles.
Confidence note
I could not run the stated pnpm/tsc command because pnpm is unavailable here, but the repository text shows a concrete typed implementation that was not updated for the new interface methods.
For Agents
Update every in-repo SharedMemorySnapshotMaterializer test double/wrapper, especially packages/agent/test/sync-requester-progress.test.ts, to delegate or stub selectRepairIdentity and repairHeadPreservingIdentity; then run the relevant typecheck/test validation that covers test helpers and fakes.
There was a problem hiding this comment.
Position (final): with e9ae36e the decision and its enactment are now one call-site pairing in BOTH lanes (the private lane inside the materializer method; the kept-head branch decide-then-enact under the same held KA lock), and the withhold plan is produced only by the materializer. Collapsing the public lane's remaining choreography into a single method is the recorded cross-lane follow-up — the pattern to migrate to already exists in this chain. If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🔴 Bug: Preservation can leave an unsuppressed losing head id row
What's wrong
The new preserve decision withholds only the exact head id row carried in descriptor.metadataQuads. When the incoming metadata contains multiple accepted RDF terms for the same descriptor operation id, one unselected losing row can remain in the bulk insert and re-stack the remote operation id onto the head that was just repaired.
Example
A peer can send an equivalent descriptor whose head has both "op-remote" and "op-remote"^^<http://www.w3.org/2001/XMLSchema#string> as dkg:shareOperationId. The parser collapses both to the same descriptor id and preservation chooses the local winner, but only one remote id row is withheld. The final bulk append can still add the other remote id row beside the preserved local id, leaving the head multi-valued/corrupt again.
Suggested direction
Build the withhold plan from the actual verified/canonicalized rows, or make the plan value-based so every descriptor head id row for the losing operation is suppressed before the bulk append.
For Agents
Look at selectRepairIdentity and the suppression ledger in shared-memory-sync.ts. Suppress all verified head shareOperationId rows that semantically denote the losing descriptor id, or reject duplicate lexical head id rows before preservation. Add a sync-level test with duplicate same-value descriptor head id rows in different RDF literal forms and assert the repaired head remains single-valued with the stored winner.
There was a problem hiding this comment.
Applied in eae96df — verified real end-to-end first: a served duplicate typed-variant id row ("storage-ack-2273b"^^xsd:string beside the plain literal) reproduced exactly your scenario — the head ended [op-v1, storage-ack-2273b] after the preserve round. Root cause was one layer below the suppression ledger: the parser's metadataQuads kept only the ONE selected id row, so the plan built from those rows missed the variant while the value-based insert canonicalization passed it. Fixed at the parser: metadataQuads now carries EVERY head id row whose value is the selected id, so byte-keyed withhold plans cover all lexical forms in both lanes. The new sync-level row pins the single-valued preserved head; the now-unused selected-row property is removed (its presence validation stays). All 143 tests across the six affected suites green.
There was a problem hiding this comment.
🟡 Issue: The materializer API leaks the sync write ledger
What's wrong
selectRepairIdentity is a store/materialization concern, but it now returns exact descriptor rows for the caller's suppression ledger. That couples persistence selection to the sync loop's verified-key bookkeeping and makes future changes to metadata canonicalization or suppression semantics require changes in the materializer boundary.
Example
The fake materializer in swm-public-snapshot-materialization now has to implement selectRepairIdentity and repairHeadPreservingIdentity even when the test only cares about sync ordering, because the store adapter interface now includes caller-ledger concepts.
Suggested direction
Separate the store-backed identity decision from the per-round verified-meta suppression plan. The materializer should expose stored state and perform store mutations; the sync commit coordinator or extracted workflow should own which descriptor rows are withheld from later writes.
For Agents
Rework selectRepairIdentity so it returns a domain decision, such as the winning stored id and losing descriptor id, or move the whole repair plan into a dedicated reconciliation workflow. Let GraphScopedSnapshotCommitCoordinator own row suppression, for example through suppressDescriptorShareId(descriptor, losingId), so ledger keying stays in one layer.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
| } | ||
|
|
||
| /** realHarness's shape (see the end-to-end describe) without the counters. */ | ||
| function identityHarness(store: TripleStore, served: typeof v1) { |
There was a problem hiding this comment.
💡 Suggestion: Extract the duplicated sync harness before this test file sprawls further
Why it matters
The new block adds another near-copy of a large orchestration fixture in a file that is already just under 1k lines. A shared harness would make the tests smaller and reduce incidental churn when the sync context changes.
Suggestion
Factor the repeated runSharedMemorySync fixture into one configurable helper and use it from the existing real-materializer tests, partial-round tests, and the new identity-preservation block.
There was a problem hiding this comment.
Acknowledged — deferring the harness consolidation to the same test-structure pass as the module extraction: hoisting the fixture means touching the wiring of five green suites in a file that three in-flight chained PRs assert against, and the two copies are behavior-pinned by the suites themselves in the meantime.
…d plan
PR2 review round 3 (otReviewAgent):
- selectRepairIdentity also refuses a stored winner with NO publishedAt row:
the plain resolver tolerates it, but the published-head wrapper (RFC64
inventory ordering) fails it as corrupt and every production writer
stamps one - descriptor-wins installs a canonically stamped operation
instead of preserving the anomaly forever. New sub-row.
- new private-commitment polarity sub-row: same public digest/counts but a
different privateMerkleRoot is NOT identity-equivalent (pins the private
half of the allow-list against dropped/mis-normalized commitment rows).
- selectRepairIdentity returns { winnerShareOperationId, withholdRows }:
the decision carries its complete suppression plan in one value, and the
sync loop's decide-and-withhold consumes that plan rather than
re-deriving the rows.
Agent SWM lanes 121 green; turbo build green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d-id pins PR2 review round 4 (otReviewAgent): - the GH#2273 identity-preservation suite moves to swm-head-identity-preservation.test.ts (the materializer file returns to ~612 lines; the small harness copies are deliberate - consolidating the shared sync fixture is queued with the post-chain test-structure pass). Registered in the unit config; CI shards auto-discover it. - new regression pin: repairHeadPreservingIdentity's kaUal ownership guard spares another KA's operation subject referenced by a corrupted head (re-proves on the NEW method what the replaceHeadMetadata rows pin). - new regression pin: a dirty head with one equivalent AND one non-equivalent stored id refuses preservation - the contract quantifies over EVERY referenced operation, and a first-match-return regression passes the single-id rows and fails only here. (Both rows pin properties the implementation has had since its first commit - regression pins, not fail-before rows.) - dead descriptorHeadIdRows helper removed (unused since the selection started returning its own withhold plan). Agent SWM lanes 123 green across 5 files; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…echo-guard pin PR2 review round 5 (otReviewAgent), all three applied: - the mirrored resolver rules in selectRepairIdentity are GONE: publisher now exports isResolvableWorkspaceOperationRows, built ON the resolver's own operation decoder, and the preservation gate calls it - the gate can no longer drift from what readers actually accept. (This round's reuse ask was implementable where round 4's was not: the resolver's decoder tolerates duplicate publishedAt, unlike the parser's requireSingle, so reuse preserves the benign duplicate-stamp state.) requirePublishedAt additionally enforces the published-head wrapper's rule. Every existing unresolvable-winner sub-row stays green against the unified gate. - ONE runSharedMemorySync fixture (test/_helpers/swm-sync-harness.ts) now serves the identity suite; the copied in-file harness is deleted. - new echo-guard regression pin: a stored operation whose own shareOperationId row does not echo the head's reference refuses preservation (isolates the id-echo conjunct of the resolvability gate). Agent SWM lanes 124 green across 5 files; e2e catch-up-survival row green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ow; e2e via harness PR2 review round 6 (otReviewAgent), the three substantive items applied: - collectOwnedHeadOperationSubjects is the ONE discovery of which operation subjects a head references and this KA owns (subject format + ownership ASK), consumed by BOTH replaceHeadMetadata and the identity-preserving repair - the delicate cleanup semantics can no longer drift between the paths. excludeShareOperationId spares a preserved winner. - new sync-LEVEL polarity row: a same-digest descriptor whose operation adds an allowList envelope is NOT suppressed by the healthy-skip branch (head goes two-valued, next round converges to remote authority) - a regression that suppresses whenever ids differ passes every decision-level row and fails only here. - the e2e catch-up-survival scenario drops its bespoke sync wiring for the shared makeSwmSyncHarness (the assertion is now most of the test). The identity-model unification and the single-plan protocol asks keep their standing positions (F3 / post-chain module extraction), stated in-thread. Agent lanes 50 green (identity/materializer/recovery) + e2e row green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * correct outcome (e.g. an added accessPolicy or allowedPeer IS a change the | ||
| * stale-intent machinery must see). | ||
| */ | ||
| export const OPERATION_IDENTITY_PREDICATES = { |
There was a problem hiding this comment.
🟡 Issue: Avoid defining workspace-operation identity twice
What's wrong
The new identity key is a parallel schema model in the agent: it hard-codes which RDF predicates define operation identity, how literals canonicalize, and which fields are ignored. That makes the codebase harder to maintain because the reader contract lives in publisher while the preservation equivalence contract now lives in agent, with comments warning about exactly the drift this structure invites.
Example
If a future workspace operation adds another resolver-significant envelope field, the resolver decoder and this allow-list both need to change. Missing the agent-side allow-list update would make identity preservation compare against a stale projection, even though the publisher already has the typed operation model.
Suggested direction
Use the publisher resolver layer as the canonical owner of operation-row semantics. That should let this PR delete the agent-side predicate allow-list, literal normalizer, and new direct dkg-rdf-utils dependency from packages/agent.
For Agents
Move the identity projection next to decodeWorkspaceOperationRows in packages/publisher/src/workspace-resolution.ts, expose a typed/canonical workspaceOperationIdentityKey or decodeWorkspaceOperationIdentity, and have the agent call that for descriptor and stored rows. Preserve the current equality semantics, including integer normalization and ignored per-node fields; add a unit asserting the agent no longer owns a separate predicate allow-list.
There was a problem hiding this comment.
Standing position, fifth iteration, final: the reader contract IS now consumed from the publisher (isResolvableWorkspaceOperationRows wraps the resolver's own decoder), so the drift your example describes fails closed — an allow-list omission makes preservation REFUSE (descriptor-wins), never silently preserve. What remains agent-side is the equivalence allow-list, whose unification with the parser's relation is the F3 follow-up where both move into one module. If blocking, flag 🔴 with a failure scenario.
PR2 review round 7 (otReviewAgent). The red was correct a third time, and in the same family: the snapshot LOCATOR sits outside both the identity key (its graph form embeds the operation id) and the head decoder (which never consumes snapshot pointers), so a stored winner with a stale or empty node-local snapshot graph could be preserved while the sync responder can no longer serve its public quads to peers. - selectRepairIdentity now also requires locator coherence on the winner: at most one locator form; a graph-form locator must actually hold the operation's public-quad count. Ref-form or absent locators are content-addressed (no worse than the descriptor's own) and pass. Fail-before proven: the stale-locator sub-row selects the winner against the ungated code. - rows: stale-graph refuse / populated-graph preserve / dual-locator refuse; plus key-level subGraphName rows (same lane equal, cross-lane and one-sided different) pinning lane isolation in the identity predicate. Agent lanes 52 green (identity/materializer/recovery); turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ive) Completing the reader-contract family from PR2 rounds 2/3/7 proactively: the sync responder's serving join (readFreshSwmMeta* and both legacy plans) requires rdf:type WorkspaceOperation on the operation subject - a predicate outside both the identity key and the head decoder, so a typeless preserved winner would resolve locally but be unservable to peers. selectRepairIdentity now requires the type row; new sub-row pins it. This closes the audit of every identity-key exclusion against the reader contracts (publishedAt, publisherPeerId, id echo, snapshot locator, rdf:type - each now gated or covered by the resolver decoder). Agent lanes 53 green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reader-contract audit closed (12c68bc): every predicate the identity key deliberately excludes has now been checked against every reader that consumes a preserved operation, and each is either covered by the resolver decoder (
The preserved-winner contract is now: identity-equivalent under the allow-list AND acceptable to every reader that will consume it — with descriptor-wins as the fallback for anything unprovable. |
| * published-head wrapper's rule (RFC64 inventory ordering requires a stamp, | ||
| * and every production writer emits one). | ||
| */ | ||
| export function isResolvableWorkspaceOperationRows( |
There was a problem hiding this comment.
🟡 Issue: The new publisher export exposes a partial internal validation boundary
What's wrong
The new export leaks the resolver’s internal row-shape decoder as a generic boolean helper, but it does not actually cover the full operation usability contract needed by the caller. The optional requirePublishedAt flag also mixes two layers of validation into a low-level rows API, which makes the boundary harder to reason about.
Example
A future caller can see isResolvableWorkspaceOperationRows and assume it fully answers whether operation rows are usable, while GH#2273 still needs separate snapshot locator coherence checks in the agent. That is an unclear boundary for a public publisher export.
Suggested direction
Prefer a typed decoder/validation result or a cohesive publisher-owned preservation validator over a public boolean wrapper with caller-specific options. If this is only for the agent, keep the API narrow and clearly internal rather than adding a broad-looking export.
Confidence note
This is a boundary/design concern rather than a behavioral defect; the helper may be expedient, but it still exposes a low-level internal contract across packages.
For Agents
Look at packages/publisher/src/workspace-resolution.ts and the call in packages/agent/src/sync/requester/swm-snapshot-materializer.ts. Preserve decoder reuse, but avoid exporting a partial boolean wrapper as a general publisher API. Consider exporting a typed internal validator/result that includes the complete preservation-relevant contract, or moving the preservation decision behind a publisher-owned function.
There was a problem hiding this comment.
Applied in e373703 — renamed to isDecodableWorkspaceOperationRows with the doc stating exactly what it answers: head-decoder acceptance ONLY, with serving-side gates (locator coherence, responder-join type) explicitly named as the caller's responsibility. requirePublishedAt stays as the documented published-head-wrapper addendum — splitting it into a second export would recreate the two-halves-of-one-contract problem this thread family keeps flagging.
There was a problem hiding this comment.
🟡 Issue: Avoid exporting a narrow boolean wrapper around the workspace decoder
What's wrong
This turns a private decoder into public surface area while hiding all detail behind true/false and a caller-specific requirePublishedAt flag. It gives the agent just enough internals to depend on the resolver shape, but not enough abstraction to stop the agent from duplicating adjacent publisher/responder rules around it.
Example
The agent asks isDecodableWorkspaceOperationRows(..., requirePublishedAt: true), then separately checks rdf:type WorkspaceOperation, then separately checks snapshot locator coherence. Those are all parts of whether a preserved operation is usable, but the public API only exposes one boolean fragment of that concept.
Suggested direction
Replace the public boolean wrapper with a clearer boundary: either export the actual decoded operation/validation result with named failure modes, or add a publisher-owned helper that answers the full “can this operation be preserved and served?” question. That keeps resolver invariants from leaking into agent-side ad-hoc checks.
Confidence note
This is a structural API concern rather than a behavior defect; the current helper may work, but it makes the package boundary more brittle.
For Agents
Look at packages/publisher/src/workspace-resolution.ts and the call site in swm-snapshot-materializer.ts. Preserve the resolver’s existing validation semantics, but prefer exporting a typed validator/decoded result for the full published-operation contract or keep this helper private and move the preservation validation to a canonical publisher-owned helper.
There was a problem hiding this comment.
Standing position (boundary family), final: the boolean decoder wrapper is intentionally narrow — the publisher exposes exactly its own reader contract and nothing about serving-side rules it does not own (the responder's join and snapshot serving are agent/responder concerns). A richer publisher abstraction that also encodes responder rules would centralize knowledge the publisher does not have. The full-usability contract gets one home in the module extraction. Converging; if blocking, flag 🔴 with a failure scenario.
| KnowledgeAssetOperationPublicSnapshotNotFoundError, | ||
| KnowledgeAssetWorkspaceHeadCorruptError, | ||
| isKnowledgeAssetWorkspaceHeadCorruptError, | ||
| isResolvableWorkspaceOperationRows, |
There was a problem hiding this comment.
🟡 Issue: Avoid publishing an agent repair hook as root publisher API
What's wrong
The PR solves an internal agent boundary problem by widening the publisher package's public surface. That leaks implementation details from the workspace resolver and makes future cleanup of the decoder harder.
Example
An external user can now import isResolvableWorkspaceOperationRows from @origintrail-official/dkg-publisher and depend on a low-level row-bag boolean with repair-specific options. Later refactoring decodeWorkspaceOperationRows or changing the published-head rule becomes a public API compatibility concern.
Suggested direction
Do not expose this narrow boolean from the package root unless it is intentionally part of the public SDK. Prefer an internal subpath/shared package for the repair decision, or expose a higher-level stable validator whose contract is not coupled to the agent's preservation workflow.
Confidence note
If the project intentionally treats every package index export as internal-only despite the package being publishable, this can be downgraded; otherwise this is public API surface area.
For Agents
Review the intended boundary between publisher and agent. If the helper is only for agent repair, keep it off the root index or move the shared validator to an internal/shared module with an explicit non-public contract. If it is meant to be public, rename and document it as a stable domain API rather than a catch-up hook.
There was a problem hiding this comment.
Applied the honest-marking half in 2410231 and holding the export location by repo precedent: swmKaWriteLockKey/withKeyedLocks are exactly this class of agent-facing plumbing already root-exported from the publisher (there is no subpath-export convention in this repo — every package maps only '.'), and the doc now states explicitly: internal cross-package plumbing, not SDK surface, shape may change without compatibility notice. A dedicated internal subpath or shared package is a packaging-convention change that belongs with the module extraction.
There was a problem hiding this comment.
🟡 Issue: Do not publish an internal resolver hook from the package root
What's wrong
The PR solves an agent-side reuse problem by adding an internal decoder probe to the publisher package’s main export surface. That creates architectural drift: an implementation detail that is expected to change without compatibility notice is now exposed exactly where SDK users discover stable APIs.
Example
A downstream consumer can now import isDecodableWorkspaceOperationRows from @origintrail-official/dkg-publisher; if the resolver decoder shape changes, this supposedly internal helper has become part of the practical package API.
Suggested direction
Either make this an intentional public API with a stable name and contract, or keep it internal by exporting from a clearly internal entrypoint used only by the agent. The current root export contradicts the comment and leaks implementation detail across the package boundary.
For Agents
Look at packages/publisher/src/index.ts line 55 and packages/publisher/src/workspace-resolution.ts lines 422-437. Preserve the agent’s ability to reuse the decoder, but expose it through an explicit internal module/path or move the shared decoder contract to a more appropriate internal package instead of exporting it from the publisher root.
There was a problem hiding this comment.
Standing position, third statement, final: the repo has no internal-entrypoint convention (every package's exports map is only '.'), so 'clearly internal entrypoint' is a packaging-convention change, and root-exported agent-facing plumbing is the established pattern (swmKaWriteLockKey, withKeyedLocks — same file class). The helper is explicitly doc-marked internal/no-compat. Making the decoder contract a deliberate public API — or introducing an internal subpath convention repo-wide — belongs to the module extraction; doing either as a side effect of a bugfix chain is how accidental API happens.
There was a problem hiding this comment.
🟡 Issue: Internal decoder plumbing is being exported through the public publisher barrel
What's wrong
This change solves an agent implementation need by leaking publisher internals through the SDK-facing index. That makes the resolver’s private row-decoder shape look like stable public API, so future refactors of decodeWorkspaceOperationRows will have to account for external consumers or risk a hidden breaking change.
Example
A downstream SDK consumer importing from @origintrail-official/dkg-publisher now sees isDecodableWorkspaceOperationRows as part of the same public barrel as resolveKnowledgeAssetWorkspaceHead, even though the comment says the helper may change without compatibility notice.
Suggested direction
Move this behind an internal boundary instead of adding it to the package’s main export surface. If cross-package reuse is required, make that boundary explicit as an internal module/shared package rather than relying on a public export plus a warning comment.
For Agents
Keep the resolver decoder reuse, but avoid publishing this as a top-level SDK API. Look at packages/publisher/src/index.ts and packages/publisher/src/workspace-resolution.ts; either move the shared decoder contract into an internal/shared module consumed by agent and publisher, or expose it through an explicitly internal subpath that is not part of the public barrel. Preserve the agent preservation gate behavior and add/adjust a compile-level import test if this repo has one.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Internal decoder plumbing should not be exported as publisher API
What's wrong
The PR adds an explicitly internal, agent-specific decoder hook to the public package index. That makes a private resolver implementation detail look like supported API, so future resolver cleanup now has to worry about external consumers and raw-row compatibility.
Example
A downstream SDK consumer can now import isDecodableWorkspaceOperationRows from @origintrail-official/dkg-publisher and rely on its raw row shape or requirePublishedAt option, even though the comment says it may change with the resolver decoder.
Suggested direction
Keep the package root focused on stable SDK surface. Either move this helper to an internal shared module consumed by both publisher and agent, or move the agent's validation behind a publisher-owned domain method that does not expose decoder internals.
For Agents
Move the decoder acceptance helper out of the public root export. Preserve the agent's ability to validate rows against the workspace-head decoder, but expose it through an internal module/subpath or a higher-level publisher-owned preservation predicate. Run the TypeScript build after changing imports.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
There was a problem hiding this comment.
🟡 Issue: Don't expose internal decoder plumbing through the public publisher API
What's wrong
This change says the helper is internal, but then exports it from the package's main surface. That makes a partial implementation detail look supported and also spreads the resolver contract: the publisher owns the decoder, while the agent owns several additional gates needed to make the decoded operation usable. That is a brittle boundary for future resolver changes.
Example
The new helper is public enough for import { isDecodableWorkspaceOperationRows } from '@origintrail-official/dkg-publisher', but it only answers one slice of resolver acceptability; the agent then has to remember extra responder type, access envelope, and snapshot locator checks separately.
Suggested direction
Move this behind an internal subpath/shared package, or expose a higher-level typed operation validation API that genuinely owns the resolver-side contract. The top-level publisher export should not become a grab bag for agent-only internals.
For Agents
Look at packages/publisher/src/workspace-resolution.ts, packages/publisher/src/index.ts, and the materializer import. Preserve the current preservation behavior, but keep this decoder plumbing off the public barrel or replace it with one canonical internal validator that covers the preservation contract. Existing preservation tests should still pass through the new boundary.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
…est export name PR2 review round 8 (otReviewAgent), both reds + two yellows applied: - the red pair was correct: count equality does not prove content, and a SAME-SIZE stale snapshot graph was the remaining integrity hole. The locator gate now uses the count-then-digest ladder (the same shape as isGraphAssetMaterialized): the graph's content digest must equal the committed publicQuadsDigest. New same-count-wrong-content row (remoteChanged.payload deliberately equals the committed quad count); fail-before proven against the count-only gate. - candidate loading is ONE bounded query (head-join pulls every referenced operation's rows; identity, ownership and reader-contract validation run over the in-memory model) - the per-candidate ASK/read fan-out under the lock is gone. - the publisher export is renamed isDecodableWorkspaceOperationRows and its doc states exactly what it answers (head-decoder acceptance ONLY, serving- side gates remain the caller's) - the old name overpromised. Agent lanes 53+ green incl. all locator sub-rows; e2e row green; turbo build green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR2 review round 9. Key-level rows: a zero-padded integer lexical form
("02" vs "2" on publicQuadsCount) keys EQUAL (value comparison), while a
genuinely different count keys different - pins the normalization the
wire-vs-store round-trip relies on.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lumbing PR2 review round 9 follow-up: isDecodableWorkspaceOperationRows is the same class of agent-facing plumbing as swmKaWriteLockKey/withKeyedLocks - root- exported by repo convention for workspace-internal consumers, explicitly not SDK surface, no compatibility notice on shape changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR2 post-convergence round (otReviewAgent): selectRepairIdentity is now an orchestration of NAMED validators - operationIdentityMatches, storedWinnerIsDecodable, storedWinnerHasResponderType, snapshotLocatorIsServeable - each naming one invariant of the preserved-winner contract and each pinned by its own polarity row. Behavior byte-identical (all 54 lane tests green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR2: - snapshotLocatorIsServeable no longer free-passes ref-form locators: the resolver's read-both rule makes an explicit publicSnapshotRef row WIN over the digest fallback, so a stale ref on a preserved winner would be FOLLOWED by readers while the identity key (locators excluded) still matches. A ref is now serveable only when single-valued and equal to the committed public digest (putSnapshot's ref === digest convention); absent locators keep the digest-fallback pass. Two new rows (stale ref, multi-valued refs) verified failing under the pre-fix free pass. - Harness override for selectRepairIdentity typed against the real materializer return shape (withholdRows included) instead of a partial object no production caller can receive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…licies Review follow-up on GH#2273 PR2: - New row: two identity-equivalent stored ids on one dirty head (insertion order reversed from the expected winner) must select the sorted-first id and repairHeadPreservingIdentity must leave only that id — verified failing with the sort replaced by reverse(). - The parser's byte-level key is renamed samePayloadByteEquivalenceKey and both key builders carry cross-referencing policy docs: same-payload byte comparison (throws on ambiguity, predicate-agnostic) vs cross-store normalized allow-list identity; unification recorded as follow-up F3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR2: snapshotLocatorIsServeable now (1) rejects graph locators failing isSafeIri — the exact rule the public snapshot resolver applies before following a locator, so a winner readers would reject cannot be certified serveable — and (2) treats a locator-query failure as non-serveable instead of letting assertSafeIri (or the store) THROW out of the preservation check, which pre-fix stalled the sync round on exactly the corrupt rows descriptor-wins repair exists for. New row: a literal-valued publicSnapshotGraph (the malformed shape a store can actually hold — Oxigraph refuses schemeless IRI terms at insert) must yield a clean null; verified failing pre-fix (throws "Unsafe or empty IRI value"), passing after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| * ABSENT locators (no graph, no ref) fall back to the digest convention — | ||
| * content-addressed, no worse than the descriptor's own — and pass. | ||
| */ | ||
| const snapshotLocatorIsServeable = async ( |
There was a problem hiding this comment.
🟡 Issue: Snapshot locator policy is duplicated outside its canonical resolver
What's wrong
The agent now owns a parallel implementation of publisher snapshot-resolution semantics. This is exactly the kind of cross-layer policy duplication that drifts: the code even names reader-specific rules from another module, but encodes them locally with bespoke SPARQL and digest logic.
Example
If resolveKnowledgeAssetOperationPublicQuads changes the read-both rule for publicSnapshotRef versus publicSnapshotGraph, this agent-side validator must be updated manually or it can approve/refuse preserved identities under different rules than the actual reader.
Suggested direction
Push this serveability check behind a publisher-owned internal helper, or have the materializer call a canonical resolver/validator that already owns snapshot locator semantics. The materializer should orchestrate store state, not carry a second copy of reader rules.
For Agents
Look in packages/publisher/src/workspace-resolution.ts for the canonical public snapshot resolution rules and expose a narrow internal validator/resolver that the materializer can call. Preserve the current null-on-unserveable behavior in the sync decision, but remove the duplicate locator interpretation from swm-snapshot-materializer.ts.
There was a problem hiding this comment.
Position (final): the agent gate does not re-implement snapshot RESOLUTION — it applies the reader's acceptance predicates (isSafeIri from the same dkg-core module the resolver imports; digest equality against the descriptor's committed digest) to decide serveability BEFORE preserving, on the agent's own store. The resolution semantics live only in the publisher; what's shared is the two predicates, both imported from their canonical homes. Extracting a joint locator-coherence contract is part of the recorded workspace-resolution module extraction. If blocking, flag 🔴 with a failure scenario.
There was a problem hiding this comment.
🟡 Issue: Do not duplicate snapshot locator semantics in the materializer
What's wrong
The materializer now reimplements reader-specific snapshot locator policy locally. The large explanatory comment is a sign that the logic is living outside its natural owner, and it creates a second place to keep in sync with the publisher resolver.
Example
If resolveKnowledgeAssetOperationPublicQuads later changes how multi-valued refs, graph locators, or a new locator form are interpreted, this private snapshotLocatorIsServeable copy must be updated by hand or preservation can diverge from the reader contract.
Suggested direction
Push this into the canonical workspace-resolution layer, or make preservation call a shared resolver/validator that already knows how operation public snapshots are located and validated.
For Agents
Move the locator/readability check beside the publisher resolver, or expose one internal validator used by both the resolver and the materializer. Keep the materializer focused on store orchestration and keep the existing stale/wrong/ambiguous locator tests passing.
There was a problem hiding this comment.
Position unchanged (final, repeated): recorded follow-up owns this boundary. If blocking, flag 🔴.
…c repair path Review follow-up on GH#2273 PR2, two reds: - operationIdentityKey now keys accessPolicy by its EFFECTIVE value under the publisher's own default rule (absent => privateTripleCount > 0 ? ownerOnly : public, per async-lift-publish-options/dkg-publisher): an older stored operation without the row and a peer's explicit-default row are the SAME share, and raw-presence keying refused preservation for exactly the old-metadata interop case — reintroducing stale-intent rotation. Non-default/allowList changes still differ; multi-valued policy rows or unparsable counts key to null (fail toward remote authority). Count compared by VALUE, not lexical form. New row (absent vs explicit default equal; ownerOnly polarity) verified failing under raw-presence keying. - New sync-lane row seeds the PRE-upgrade two-valued head residue and runs the full harness once: the storedHead.needsRepair branch of runSharedMemorySync must preserve the stored id (verified failing with the repair branch bypassed to replaceHeadMetadata). The two-stage row's round-2 narrative is corrected to idempotent convergence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… rows Review follow-up on GH#2273 PR2: - The kept-head prefer-stored branch now ENACTS the preserving repair when a winner is decided (repairHeadPreservingIdentity: descriptor head rows + stored winner id) instead of suppress-only. Version/id cardinality is all the branch checks, but the resolver validates more head rows — a stale extra assertionGraph row was invisible here yet corrupt to readers, and suppress-only froze it in place round after round. Same decide-and-enact shape as the private recovery lane. New row (residue head row + equivalent foreign descriptor => resolver returns preserved local id, single assertionGraph row) verified failing under suppress-only. - Access-envelope isolation rows: allowedPeer set participates in identity independently of the policy row (order-independent, refusal on set change); private content's absent policy row equals explicit ownerOnly (BigInt path), with explicit-public polarity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR2: metadataQuads kept only the ONE selected head id row, so a same-value different-lexical-form row (plain vs xsd:string-typed literal — RDF 1.1 admits both) escaped the byte-keyed withhold plan, passed the value-based insert canonicalization, and re-stacked the losing id beside a just-preserved head. The parser now includes every head id row whose VALUE is the selected id, so downstream plans cover all forms. New sync-level row serves a duplicate typed-variant id row and asserts the preserved head stays single-valued — reproduced failing exactly as reported (head ended [op-v1, storage-ack-2273b]) before the fix. The now-unused selected-row candidate property is removed (the presence VALIDATION stays). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on GH#2273 PR2: - New reader-contract validator storedWinnerHasUsableAccessEnvelope: the VM-publish preflight requires the LIVE head's accessPolicy to be DEFINED, so preserving a policy-less stored winner parks the KA on an operation queued publishes reject as stale — descriptor-wins instead converges to the peer's explicit-policy op. Identity-KEY equality (absent == effective default, kept for old-metadata comparison) and envelope USABILITY are now separate concerns. Fail-before: the new gate row (policy-less stored winner + equivalent explicit-default descriptor => refuse) failed before the gate landed. - Fixture now stamps accessPolicy 'public' on operation rows — production share paths always write the effective policy row (verified on a live testnet op dump); policy-less ops are the old-metadata shape, built per-row where a test needs one. Two envelope-difference rows updated to REPLACE the default row rather than append (parser rejects multi-valued policies). - New key-level row pins plain-vs-xsd:string canonicalization on string-valued identity rows (passed immediately — the rdf-utils canonicalization path covers it; now regression-pinned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Part 2/3 of the bug(publisher): restart recovery can invalidate same-job quorum retry via rotated SWM operation ID #2273 fix chain (base: fix(publisher): fail closed on multi-valued SWM heads and classify as retryable (GH#2273 1/3) #2281 — chained; retarget to
testnet-canarywhen part 1 merges). This part fixes the public SWM catch-up lane so it preserves the workspace head's operation identity for semantically identical content — the Core Rule: when the locally stored operation rows and an incoming descriptor's operation rows are equivalent over an explicit operation-identity allow-list, the identity the local store already carries wins. Catch-up may retain the remote operation subject as immutable history, but must not add, remove, or replace the head'sshareOperationId. Any genuine difference (content digest, counts, private root, access policy, allowed peers, ownership, version) routes to today's behavior — remote authority.The mechanism being fixed, both stages. A queued async VM-publish job freezes the head's
shareOperationIdat admission. A peer — typically a storage-ACKing core — legitimately holds the SAME share under a DIFFERENT deterministic id. Stage 1: the per-KA path correctly skipped the already-materialized KA, but the round's bulk verified-meta union-insert then stacked the peer's head-id row beside the local one, leaving a multi-valued head. Stage 2: the next round'sneedsRepairdeleted the head AND the local operation subject, re-inserting only the remote identity — after which the preflight terminally failed the job aspublish_intent_stalefor content that never changed.Equivalence is an allow-list, not a deny-list:
operationIdentityKeycompares content commitment (digest, counts, private root), access envelope (policy, allowed peers), scope (UAL, version, subgraph) and ownership (prov:wasAttributedTo) — and deliberately excludesshareOperationId,publishedAt(per-node clocks),publisherPeerId(per-node), and the snapshot pointers (publicSnapshotGraphembeds the operation id in its value, so any deny-list keying on raw rows silently never matches and the whole fix degrades to a no-op). Wire-vs-stored normalization (xsd:string, integer canonical form) is pinned by a dedicated round-trip test against real Oxigraph.Every head rewrite goes through one decision.
selectRepairIdentity(under the held per-KA write lock) decides preservation;repairHeadPreservingIdentityheals a multi-valued head to the winning stored identity without ever deleting the winner's operation rows (they may be the only durable copy a queued job references). Suppression of the loser's head-id row is row-level on the commit coordinator — descriptor-level suppression would withhold the four required head rows after a repair and manufacture a permanently corrupt head. The version-superseded exit now also withholds a stale descriptor's head rows from the bulk append (a two-VERSION head is the same hazard arriving via the metadata side).What stays exactly as today: non-equivalent operations (genuine change) rewrite to remote authority; graph-backed KAs (no snapshot ref) keep the bulk insert as their only head writer — suppression is decision-driven, never blanket (the SWM catch-up can leave a partial unrecovered tail after phase timeouts under backpressure #2050 G7 invisibility class is pinned by a control test).
Review-driven hardening (post-initial rounds): the preserved-winner gate stack is named validators (identity key, resolver decodability, responder type row, snapshot-locator serveability, usable access envelope); ref-form locators must equal the committed digest (readers follow explicit refs) and malformed locators rank non-serveable instead of throwing;
operationIdentityKeycompares the EFFECTIVE access policy (absent row == publisher default derived from privateTripleCount) while a policy-less stored winner still refuses preservation (queued preflight requires a defined policy); the descriptor carries every lexical form of its selected id so withhold plans are value-complete; the kept-head branch ENACTS the preserving head rewrite (residue purged) rather than suppress-only; deterministic sorted tie-break and the dirty-head sync-lane repair path are regression-pinned. Every fix carries a fail-before-proven test row.Related
Diagrams
Catch-up offering an equivalent operation id
Before:
sequenceDiagram participant Peer as Peer with storage-ack id B participant Sync as Catch-up round participant Store as Local store with head id A Peer->>Sync: verified meta with head row B Sync->>Store: per-KA check finds content identical Note over Sync: per-KA path skips, head untouched Sync->>Store: bulk union-insert of all verified meta Note over Store: head now carries A and B Sync->>Store: next round sees needsRepair Sync->>Store: repair deletes head and operation A, installs B Note over Store: queued job frozen on A dies publish_intent_staleAfter:
sequenceDiagram participant Peer as Peer with storage-ack id B participant Sync as Catch-up round participant Store as Local store with head id A Peer->>Sync: verified meta with head row B Sync->>Store: per-KA check finds content identical Sync->>Store: selectRepairIdentity compares operation rows under the KA lock Note over Sync: equivalent, so stored identity A wins Sync->>Sync: suppress descriptor head-id row from the bulk append Sync->>Store: bulk insert lands operation B rows as immutable history only Note over Store: head stays single-valued on A, job preflight still authorizesFiles changed
packages/agent/src/sync/graph-scoped-swm-recovery.tsOPERATION_IDENTITY_PREDICATESallow-list +operationIdentityKeywith wire-vs-stored object normalization (xsd:string, canonical integers); shippedresolveEquivalentHeadOperationuntouchedpackages/agent/src/sync/requester/swm-snapshot-materializer.tsreadStoredHeadexposes the single unambiguousshareOperationId(null when absent or multi-valued — SAMPLE over ambiguity would be the arbitrary pick this chain exists to kill);selectRepairIdentity(preservation decision under the held lock);repairHeadPreservingIdentity(heals to the winning stored identity, never deletes the winner's operation rows; kaUal ownership guard preserved)packages/agent/src/sync/requester/shared-memory-sync.tssuppressRowson the commit coordinator;repairOrReplaceHeadat BOTHreplaceHeadMetadatacall sites; prefer-stored branch at the healthy-skip exit; version-superseded exit withholds stale head rows from the bulk appendpackages/agent/test/swm-snapshot-materializer.test.tsoperation identity preservation (GH#2273)describe: round-trip identity-key spike (the assumption the fix rests on), two-stage repro, materialize-path repro, prefer-stored polarity row (non-equivalent still rewrites), heal rows, version-superseded row, graph-backed control;readStoredHeadrows updated for the new fieldpackages/agent/test/swm-public-snapshot-materialization.test.tspackages/agent/test/e2e-memory-layers.test.tsinsertedMetaTriples > 0Test plan
All behavior rows have fail-before evidence (lane src reverted, rows run, restored):
selectRepairIdentityreturn null (descriptor wins)operationIdentityKeybyte-equal between wire quads and Oxigraph read-back; equivalent twin equal, changed share differentpreflightQueuedKnowledgeAssetVmPublishExecutionreturns{action: 'execute'}(pre-fix: head carried both ids after one round)pnpm build:packages(turbo + tsc) green🤖 Generated with Claude Code