Skip to content

feat(harness): persist shared Agent Map proposals - #775

Open
ynadge wants to merge 7 commits into
yashnadge/sap-3061-api-define-typed-agent-map-proposal-operationsfrom
yashnadge/sap-3059-backend-persist-one-shared-versioned-map-proposal
Open

feat(harness): persist shared Agent Map proposals#775
ynadge wants to merge 7 commits into
yashnadge/sap-3061-api-define-typed-agent-map-proposal-operationsfrom
yashnadge/sap-3059-backend-persist-one-shared-versioned-map-proposal

Conversation

@ynadge

@ynadge ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Prepare the one project-wide Agent Map proposal as a strict crash-atomic aggregate for the SAP-3060 MCP transport. Typed batches allocate stable IDs, retain server-authored attribution, conservatively rebase disjoint stale writes from authoritative proposal history, and expose post-commit accepted deltas through a transport-neutral callback.

This PR is stacked on PR #774. Its base is intentionally the SAP-3061 branch so this diff contains only SAP-3059.

Changes

  • extract the catalog owner-file protocol into reusable DurableFileLock and use it for catalog and per-project map state
  • migrate exact E1 workspace records into one fsync/rename aggregate containing workspace, proposal, history, and bounded private receipts
  • add UUIDv7 allocation and transport-neutral AgentMapProposalService with atomic validation/materialization, trusted attribution, canonical idempotent replay, stale conflict preview/rebase, and existing-ID collision fencing
  • derive stale conflict touch sets from proposal history so receipt retention cannot weaken conflict detection
  • retain compact exact-replay data for the latest 256 accepted batches; older session/request IDs are permanent history tombstones and return actionable request_id_expired plus new_request
  • validate all persisted nested shapes and trusted actor identity using one Zod-free shared codec that is safe in the browser bundle
  • evolve the authenticated read route and mock fixture to the coherent versioned workspace-and-proposal snapshot
  • expose only the named browser-safe Agent Map types and constants required by the accepted-delta public bus contract
  • keep writes transport-neutral until SAP-3060; no dead HTTP router option or unreachable server construction remains
  • restore the unrelated @electron/node-gyp integrity hash while retaining only intentional uuid lock changes

Review refinements

  • eliminated duplicate operation/delta/touch-set storage from receipts and added pruning/tombstone regression coverage
  • made AgentMapWorkspaceResponse an alias of AgentMapReadSnapshot
  • removed unreachable proposal service wiring and premature analytics event union members
  • removed Zod from the web import graph and unified DEL/control-character handling
  • reject actor identities before they can create durable state that a later read rejects
  • distinguish digest misuse from expired exact-replay data so callers cannot enter a reread loop
  • replay each historical prefix once and reuse the read graph for touch-set derivation; full attributed history remains append-oriented by contract and is not unsafely pruned

Testing

  • ticket-focused Vitest coverage: 8 files, 70 tests
  • pnpm --filter @sapiom/harness typecheck
  • pnpm --filter @sapiom/harness lint
  • pnpm --filter @sapiom/harness build:server
  • pnpm --filter @sapiom/harness build:web and no Zod runtime symbols in the emitted JS
  • targeted Prettier check and git diff --check

Related

Checklist

  • Code follows project guidelines
  • No hardcoded secrets or local paths in public state or telemetry
  • E1 flat-record migration remains supported
  • Legacy SystemGraph remains unchanged
  • Self-reviewed
  • No merge performed

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — PR #775 (round 1)

No confidentiality problems: the changeset and new comments are provider-neutral, name no company but Sapiom, and the SAP-#### JSDoc refs match long-standing convention in this repo (agent-core/src/*). Findings below are correctness / published-package hygiene.

1. Lockfile drops an integrity hash (supply chain)

pnpm-lock.yaml:1129 — this PR removes the integrity: sha512-MXgz… field from the git-hosted @electron/node-gyp resolution (origin/main:1123 still has it). With no integrity, pnpm install fetches that tarball from codeload with nothing to verify it against, in CI and on every desktop-build machine. Unrelated to SAP-3059 — restore the line (git checkout origin/main -- pnpm-lock.yaml then re-add only the uuid entries).

2. Receipts grow without bound, and cannot be pruned

core/agent-map-proposal-service.ts:416 appends a receipt on every accepted batch and nothing ever removes one. Each receipt stores the whole ProposalBatchResult, including delta.operations — so every operation is persisted twice (also in proposal.history) — plus its touchSet. Every propose re-serializes the entire aggregate pretty-printed (JSON.stringify(aggregate, null, 2)) and fsyncs it, and every workspace GET reads and parses all of it under the 5 s DurableFileLock (readSnapshotreadAggregatelocked).

Failure: a project whose shared proposal accumulates a few thousand operations turns each single-node edit into a multi-MB rewrite; once a write exceeds the lock timeout, both propose and the plain GET /projects/:id/workspace fail with storage_unavailable, permanently — there is no compaction path.

Worse, the obvious fix is blocked by design: stale-write conflict detection reads the receipt log (:305, receipts.filter(c => c.result.version > expectedVersion)), so pruning receipts silently converts overlapping stale writes into accepted ones. Conflict detection needs to be derivable from proposal.history (which already carries acceptedVersion and the operation) rather than from a retention-bound side log; then receipts can be capped/TTL'd to live sessions, as the changeset's "session-scoped" wording already implies.

3. BusMessage gains a variant whose payload type is not exported

shared/types.ts:557 adds { type: "agent-map.proposal.changed"; delta: import("./agent-map.js").AcceptedProposalDelta }. BusMessage is public (src/index.ts:6 does export * from "./shared/types.js"), but AcceptedProposalDelta is not exported from the package entry. Consumers that switch exhaustively over BusMessage with a never guard stop compiling on upgrade and cannot name the new payload type to handle it. Either export AcceptedProposalDelta (and the Plan*/MapChangeProposal types it references) from src/index.ts, or keep the variant off the published union. Either way the changeset should say so — right now the body advertises only persistence.

4. The proposal service is unreachable; the changeset says otherwise

server/agent-map.ts:42 adds proposalService?: AgentMapProposalService to AgentMapRouterOptions and server/index.ts:2836 passes it, but no route reads it — grep proposalService shows only the declaration and the assignment. Nothing in the shipped product can create a proposal in this PR, yet the minor changeset tells npm consumers that attributed, idempotent proposal writes exist. Drop the dead option until SAP-3060 wires a transport (the service is constructible from startServer internals), and scope the changeset body to what is reachable: the storage format migration and the widened workspace read.

5. Browser parser re-declares three shared enums, with no test on the populated path

web/src/lib/agent-map.ts:213,240,246 hardcodes ["agent","subagent","resource","connector","artifact"], the six relationship kinds, and the four execution modes, instead of importing PLAN_NODE_KINDS / RELATIONSHIP_KINDS / EXECUTION_MODES from shared/agent-map.ts — the same module the file already imports types from. Add a node kind to the shared constant and the server will happily persist it, then parseAgentMapWorkspaceResponse returns undefined for the proposal and the UI throws a generic Invalid Agent Map workspace response for the whole workspace, with no way for a user to recover the map. web/src/lib/agent-map.test.ts only ever passes proposal: null, so ~130 lines of new exact-key validation have no happy-path coverage — a single wrong key name here breaks every read and no test notices.

6. Server trusts its own aggregate far less strictly than the client does

core/agent-map-workspace-store.ts:180 casts value.proposal as MapChangeProposal and validates only the top-level scalars; nodes, relationships, history entries and receipt.result are never checked (:217 re-emits value.receipts as-is). A truncated or hand-edited workspace.json therefore passes parseAggregate, is served by the route, and is rejected by the strict browser parser — so the user gets an unrecoverable client-side error instead of the store's bounded malformed_state, which is the code the route and UI recovery path are built around. Validate the nested shapes here, or reuse the browser parser's predicates.

7. Two exported types for one wire shape

AgentMapWorkspaceResponse (shared/agent-map.ts:208) is now structurally identical to AgentMapReadSnapshot (:330), added by the stacked base PR and used only by agent-map-proposal-schema.test.ts:298. The route satisfies one, the contract test satisfies the other; they will drift. Make one an alias of the other.

Verdict: Request changes — restore the lockfile integrity hash (#1) and address the unbounded, un-prunable receipt log (#2) before merge; #3#4 need the changeset and public surface reconciled.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — PR #775 (round 2, delta since 1e43470)

One commit moved (76a96b0 "harden shared proposal persistence"). It fixes round-1 #5 and #6: the browser parser and the store now share shared/agent-map-codec.ts, and populated proposals/receipts have happy-path coverage. No confidentiality issues in the delta — no prose changed; the changeset is untouched and still provider-neutral.

New findings

1. The write path persists what the new read path rejects. agent-map-codec.ts:96 requires actor.userId/sessionId (and assignment.agentId) to be non-empty, trimmed, control-char-free and ≤256 chars, but propose copies them straight off PlanningSessionIdentity (agent-map-proposal-service.ts:105) and transact writes next with no re-validation (agent-map-workspace-store.ts:427). A caller passing userId: "" or a >256-char session id gets an accepted, fsynced write; from then on every readAggregate/readSnapshot — so the workspace GET and all further propose calls — throws malformed_state for that project, with no repair path. Validate the actor at the service boundary, or run the codec over next before writing.

2. Two zod modules now describe the same shapes. shared/agent-map-codec.ts duplicates UUID_V7, bounded/boundedText, contractRefs, node, relationship and both *Changes schemas from core/agent-map-proposal-schema.ts. They already disagree: the codec's bounded ignores 0x7f, which boundedText rejects (:20). Each future divergence is finding 1.

3. zod now enters the browser bundle. Nothing under web/src imported zod before; web/src/lib/agent-map.ts:9 does, adding ~50–60 KB gz to the SPA shipped in the harness tarball for one parser.

Round-1 findings not fixed

Nothing in round 1 looks wrong on re-read.

Verdict: Request changes — #1 and #2 above plus round-1 #1/#2/#4 remain blocking.

ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — PR #775 (round 3, delta since 76a96b0c)

One commit (fd978cf) clears every open finding from rounds 1–2: lockfile integrity hash
restored, receipts compacted and capped at 256 with conflict detection replayed from
proposal.history, actor validated at the service boundary and transact re-parsing
next before persist, zod out of web/src, codec primitives shared with
agent-map-proposal-schema.ts, dead proposalService option removed,
AgentMapWorkspaceResponse aliased. Changeset rewrite is provider-neutral; the bare
SAP-3060 ref matches repo convention. Nothing from rounds 1–2 reads wrong in hindsight.

New findings

1. src/index.ts:7 publishes the whole module, not the contract.
export * from "./shared/agent-map.js" commits ~60 names to the public API, including four
on-disk format constants (AGENT_MAP_WORKSPACE_SCHEMA_VERSION,
STUDIO_PROJECT_CATALOG_SCHEMA_VERSION, AGENT_MAP_INITIAL_RECORD_VERSION,
STUDIO_WORKSPACE_PREFERENCE_SCHEMA_VERSION) and the planner HTTP request/response shapes.
Bumping a storage version now becomes a visible API change, and consumers can pin on shapes
this PR never meant to guarantee. Round-1 #3 asked for AcceptedProposalDelta and what it
references — use named re-exports.

2. request_id_reused is overloaded and un-actionable.
core/agent-map-proposal-service.ts:481 raises it for a retry whose receipt was pruned
(batch applied, exact result unrecoverable, client must mint a new request id);
:467 raises the same code for a digest mismatch (caller bug). Both carry
recovery: "reread", so a client that rereads and retries with the same requestId loops
forever with no way to tell the two cases apart. Split the code, or give the pruned case a
recovery that says "new request id".

Round-1 #2, residual half

proposal.history is unbounded and now unprunable by construction —
parseMapChangeProposal requires non-empty history with contiguous versions from 1 — and
every stale propose replays it twice (graphAt + touchSetAfter, each applyOperations
copying the graph per record) inside the 5 s DurableFileLock. Receipt growth is fixed;
aggregate growth and rewrite cost are not.

Nit

AgentMapProposalService is referenced only by its own tests, yet the changeset's first
sentence still sells idempotency receipts and rebasing to npm consumers; only the closing
clause hedges.

Verdict: Approve with comments — #1 is worth fixing before publish (an export * is
hard to narrow later); #2 and the residual can follow in SAP-3060.

ynadge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review — PR #775 (round 4, delta since fd978cfe)

One commit (5718212 "narrow proposal recovery contracts"). It resolves round-3 #1
(export * replaced with named exports; every type transitively reachable from
AcceptedProposalDelta is exported, only the load-bearing schema constant is public, and no
in-repo consumer imported a removed name — main's entry never exported the module, so the
narrowing is not breaking) and the substance of round-3 #2 (pruned-receipt retry now
request_id_expired / new_request, digest mismatch keeps request_id_reused). The
graphAt/touchSetAfter refactor is behavior-preserving: readGraph is exactly the graph the
callee used to recompute, and neither validateMapOperationBatch nor canonicalizeAgentMapGraph
mutates it. Changeset rewrite is provider-neutral and names no company but Sapiom.

New findings

1. The changeset advertises a code consumers cannot type or receive.
.changeset/shared-proposals-persist.md:5 tells npm consumers that "older same-session request
IDs return an actionable request_id_expired conflict", but src/index.ts exports neither
ProposalConflict/ProposalConflictCode nor AgentMapProposalConflictError, and no transport
reaches AgentMapProposalService in this PR. A consumer reading the CHANGELOG has nothing to
catch and no type to narrow on. Either export the conflict contract alongside the delta types, or
drop the code name from the changeset until SAP-3060 makes it reachable.

2. The surviving request_id_reused branch lost its only test.
agent-map-proposal-service.test.ts:179 was retargeted to request_id_expired, so the digest
mismatch at agent-map-proposal-service.ts:470 — same requestId, different payload — now has
zero coverage. A regression that turns that branch into an accepted double-apply would pass CI.

Earlier findings not fixed

  • Round-3 update SDKs to use payment protocol data #2, half: digest mismatch still carries recovery: "reread"; rereading cannot clear a
    persisted receipt, so a caller that obeys recovery still loops. Only the code distinguishes
    the two cases now.
  • Round-3 residual: proposal.history is still unbounded and unprunable by construction; per-batch
    replay in touchSetAfter remains inside the 5 s DurableFileLock (the duplicate replay is gone).

Nothing from rounds 1–3 reads wrong in hindsight.

Verdict: Approve with comments — fix #1 before publish (changeset text cannot be retracted);
#2 and the residuals can land with SAP-3060.

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