Skip to content

feat(harness): define Agent Map proposal operations - #774

Open
ynadge wants to merge 3 commits into
mainfrom
yashnadge/sap-3061-api-define-typed-agent-map-proposal-operations
Open

feat(harness): define Agent Map proposal operations#774
ynadge wants to merge 3 commits into
mainfrom
yashnadge/sap-3061-api-define-typed-agent-map-proposal-operations

Conversation

@ynadge

@ynadge ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Define the versioned, path-free Agent Map proposal domain used by later persistence, MCP, and rendering tickets. Caller batches are parsed strictly, validated against one complete prospective graph, and materialized with permanent IDs only after semantic validation succeeds.

Changes

  • add branded proposal identities, all five node kinds, all six relationship kinds, typed input/materialized operations, results, deltas, actor/history, proposal, read-snapshot, validation, and conflict contracts
  • add strict Zod schemas with exact object keys, UUIDv7-style ID validation, bounded field-addressable errors, authority-field rejection, unsupported-version handling, and safe removal of explicit undefined patch fields
  • add pure forward-reference resolution, ownership/deletion rules, the closed endpoint matrix, semantic duplicate detection, canonical graph ordering, stale-rebase touch sets including existing owner/endpoint dependencies, and post-validation ID materialization
  • reject allocator collisions with existing or newly allocated identities before a duplicate graph can be returned
  • distinguish invalid persisted graph state with reread recovery while preserving correct recovery for caller-authored validation errors
  • cover every endpoint-kind pair, all operation variants, forward aliases, explicit deletion, cycles, cross-owner links, parallel edges, stable IDs, allocation ordering/uniqueness, direct touch-set overlap/non-overlap, canonicalization, validated/materialized touch-set parity, and the generic stock-research fixture

Key Refinements from Code Review

  • deletion touch sets now overlap with concurrently introduced endpoint and ownership dependencies
  • explicit undefined values cannot blank materialized node or relationship fields
  • untouched invalid stored graphs no longer direct callers to correct unrelated operations
  • duplicate semantic relationships created by an update are attributed to the contributing operation, avoiding a reread retry loop caused by canonical relationship ordering
  • the internal-only contract does not receive a public-package changeset until a reachable behavior or npm API ships

Testing

  • pnpm --filter @sapiom/harness exec vitest run src/core/agent-map-proposal-schema.test.ts src/core/agent-map-proposal-validator.test.ts (37 tests passed)
  • pnpm --filter @sapiom/harness typecheck
  • pnpm --filter @sapiom/harness build:server
  • focused ESLint and Prettier checks
  • git diff --check

Related

Checklist

  • Code follows project guidelines
  • Tests cover validation, concurrency, and materialization boundaries
  • No persistence, transport, session, rendering, SystemGraph, or @sapiom/agent changes
  • No hardcoded secrets or local paths
  • Changeset is not applicable because this PR adds an internal harness boundary with no reachable package behavior or public export
  • Self-reviewed
  • Linear status updated

Add strict caller schemas and a pure prospective-graph validator with batch-local reference resolution, deterministic touch sets, and post-validation ID materialization.

Closes: SAP-3061
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — PR #774: define Agent Map proposal operations

No confidentiality problems: the changeset, JSDoc and comments are provider-neutral and name no
company, customer, or internal host. No new dependency, no package.json/tarball change, no
hardcoded endpoint.

Findings

1. Touch sets record writes but not reads, so proposalTouchSetsOverlap misses real conflicts

packages/harness/src/core/agent-map-proposal-validator.ts:256 (and :320)

add-relationship contributes only a semantic key; add-node with an ownerAgent ref contributes
nothing at all. Neither records the existing node ids the operation depends on.

Failure: proposal A = remove-node N plus remove-relationship for N's incident edges →
entityKeys: ["node:N", "relationship:…"]. Proposal B, based on the same version, adds a new
edge N → P kind:"reads" (or adds a node with ownerAgent: {nodeId: N}). B's touch set contains no
node:N and a semantic key that does not match any of A's. proposalTouchSetsOverlap(A, B) returns
false, so a caller using it to decide that B can be replayed onto A's result produces a
relationship (or owner pointer) to a deleted node. The only thing that catches it is re-running
validateMapOperationBatch — which is exactly the work this helper exists to skip. Either add the
referenced node ids to entityKeys, or document on ProposalTouchSet that non-overlap does not
license skipping re-validation.

2. The stale-rebase mechanism is entirely untested

packages/harness/src/core/agent-map-proposal-validator.test.ts

proposalTouchSetsOverlap and canonicalizeAgentMapGraph are exported and have zero direct tests;
touchSet is asserted exactly once, by not.toContain("draft-") on a joined string. Given finding
1 turns on the precise contents of both touch-set builders, and given that the two builders
(deriveTouchSet vs deriveMaterializedTouchSet) are hand-kept in sync with no test tying them
together, the highest-risk code in the diff is the least covered. Add cases that assert overlap /
non-overlap for the concurrency pairs the design cares about.

3. An explicit undefined in changes blanks the field instead of being rejected

packages/harness/src/core/agent-map-proposal-schema.ts:58 → validator :497

Zod v3 keeps a key that is present-but-undefined in the parsed output, so
changes: { name: undefined, purpose: "x" } satisfies
.refine((changes) => Object.keys(changes).length > 0). The validator then does
{ ...node, ...operation.changes }, and materializeValidatedMapBatch emits a PlanNode with
name: undefined and a MapOperation whose changes.name is undefined — both violating the
declared string type, and both persistable when the later ticket replays deltas.

Not reachable over JSON (null is correctly rejected), but it is reachable from the in-process
callers this module is being built for — changes: { name: input.name, purpose: input.purpose }
with one field absent is the natural way to write that code, and Partial<…> permits it under the
repo's TS settings. Drop undefined values before the refinement, or count only defined keys.

4. Whole-graph validation reports untouched entities with an unactionable recovery

packages/harness/src/core/agent-map-proposal-validator.ts:623, :650

The ownership, endpoint-matrix and duplicate checks run over every node/edge in the prospective
graph, including ones the batch never mentions. A stored graph that violates a rule — e.g. after the
endpoint matrix or ownership rules are tightened in a future schemaVersion, or after a restore —
makes every subsequent batch fail with operationIndex: null, path: ["current", "nodes"], and
recovery: "correct". The caller is told to correct an operation it did not send and cannot see,
and the workspace is wedged. These stored-data issues should carry a distinct recovery (or a
distinct code) from caller-authored ones.

Nits

  • agent-map-proposal-validator.test.ts:462 — the golden fixture names a connector tiktok inside a
    "stock-research" scenario; rename to a generic connector so a public fixture doesn't read as a
    specific partner workflow.
  • The changeset is minor, but nothing in this PR is reachable from packages/harness/src/index.ts
    or the . export — the published CHANGELOG will announce a "contract" no consumer can import.
  • materializeValidatedMapBatch trusts the allocator to return unique ids; a repeating allocator
    silently yields a graph with duplicate node ids (the existing tests use one).

Verdict

Request changes: finding 1 (with 2) is a correctness gap in the concurrency primitive this PR is
defining; 3 and 4 are contained but worth fixing before the persistence ticket builds on top.


Note for the author: I could not run the test suite (dependencies are not installed in this
environment), and a scratch file packages/harness/probe.test.ts I created for a runtime probe
could not be deleted by this session's sandbox — please remove it if it appears in your tree.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #774 (delta since 2a0d17f)

Round-1 findings 1–4 and all three nits are addressed: deriveTouchSet /
deriveMaterializedTouchSet now record referenced endpoint and owner node ids (with tests that
assert overlap and non-overlap for the deletion/new-edge and independent-update pairs),
stripUndefinedProperties runs in both the Zod transform and cloneRequest with an empty-changes
guard after cloning, the allocator rejects collisions, the changeset is dropped (nothing in this file
is reachable from src/index.ts or any export — removal is right), and the fixture no longer names a
specific connector. No confidentiality issues in the delta.

New finding

recovery: "reread" is chosen by path, so a caller-caused duplicate can livelock

packages/harness/src/core/agent-map-proposal-validator.ts:166

issue() now infers recovery from path[0] === "current", and the duplicate-relationship loop
(:736) attributes the collision to whichever relationship it reaches second in canonical (id-sorted)
order — which is not necessarily the one the batch touched.

Failure: relationships R_a (lower id) and R_b (higher id) share from/to/kind and differ only in
executionMode. A caller sends update-relationship R_a setting executionMode to R_b's value.
The loop keys R_a first (now carrying operationIndex), then flags R_b — untouched, so
operationIndex: null, path: ["current","relationships"], and therefore recovery: "reread". The
caller refetches an unchanged graph, replays the same batch, and fails identically forever. Round 1
had the same mis-attribution under "correct", but "reread" turns an unactionable error into a
guaranteed retry loop.

Fix: derive recovery from whether any operation in the batch contributed to the entity (e.g. carry a
caller-authored flag through the working structures), not from the path prefix.

Verdict

Approve with one change: the round-1 gaps are genuinely closed; the new recovery heuristic needs to
key on provenance rather than on path[0] before persistence retries build on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant