Skip to content

feat(harness): migrate shared project state [Agent Map 09/15] - #828

Merged
ynadge merged 1 commit into
mainfrom
review/agent-map-09-project-state
Sep 6, 2026
Merged

feat(harness): migrate shared project state [Agent Map 09/15]#828
ynadge merged 1 commit into
mainfrom
review/agent-map-09-project-state

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Existing E1/E2 map state and its writers must move together to one durable aggregate without exposing a migrated schema to an incompatible consumer.

Summary and scope

Atomically switch migration, locked storage, current pointers, map mutation/replay, strict neutral actor parsing, and attribution consumers while preserving the existing map HTTP projection.

Bound operation history before writes and classify exhausted quotas, corrupt aggregates, and unsupported storage schemas as terminal manual-intervention errors. Preserve the source schema version in migration telemetry.

How this increment fits

Storage migration, map writers, neutral actor decoding, and attribution consumers change atomically. Plan-authoring tools begin in part 10.

Stack and review boundary

  • Part 09 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-08-version-contracts.
  • Current head: 7d947b1d8c0c75cdae8d23f38a2a5273355c0fdd; 2,838 changed lines across 29 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #806, #811. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3149. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against c28d18989e02e13b69541ec41e3a35a2458bdce1. The final head changes only README terminology or commit ancestry; a complete tracked-file comparison confirms identical executable source and build inputs. The terminology gate was rerun on 7d947b1d8c0c75cdae8d23f38a2a5273355c0fdd.

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: E1/E2 migration, locked atomic writes, exact replay, current pointers, neutral attribution, graph projection, operation limits, corrupt data, and unsupported schemas.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Breaking: ProposalActor and proposal history expose trusted userId/sessionId without role or assignment. Supported persisted formats migrate; unsupported or corrupt state fails closed with actionable recovery.
  • Changeset: Included: .changeset/atomic-project-state-migration.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #828 (Agent Map 09/15)

No confidentiality problems: the changeset, README edit, JSDoc and allowlist reasons are
all generic, and the diff adds no company names, private hosts, or internal links.

1. Breaking public type change shipped as a patch changeset

.changeset/atomic-project-state-migration.md declares patch, but this PR removes two
required fields from a type exported from the package entry point.

ProposalActor is re-exported from packages/harness/src/index.ts:24 and is reachable
from AcceptedProposalDelta.actor. packages/harness/src/shared/agent-map.ts:336 narrows
it from {userId, sessionId, role, assignment} to ProjectAgentActorRef
({userId, sessionId}), and parseProposalActor
(packages/harness/src/shared/agent-map-codec.ts:359) now uses
hasExactKeys(value, ["userId","sessionId"]), so an actor carrying the old keys is
rejected rather than tolerated.

Any consumer that reads delta.actor.role fails to typecheck, and any consumer that passes
the old object to parseProposalActor now throws at runtime. The PR body says this
outright ("consumers of actor role/assignment must move to neutral session attribution"),
so the changeset contradicts the PR's own compatibility note. This needs a minor (0.x)
changeset with a Breaking heading and a migration line naming the replacement field.

2. New quota failures reach MCP agents as internal_error, recovery: "retry"

errorResult in packages/harness/src/server/agent-map-mcp-tools.ts:68 matches
AgentMapProposalValidationError, …ConflictError, …ProjectError,
AgentMapMcpProjectUnavailableError and AgentMapWorkspaceStoreError, then falls through
to { code: "internal_error", recovery: "retry" }. The two error classes this PR adds —
AgentMapProposalQuotaError (agent-map-proposal-service.ts:73) and
AgentBriefAppendQuotaError (agent-map-workspace-store.ts:95) — are not in that chain,
and neither is the AgentMapAggregateError that locked() can now throw from the write
path (agent-map-workspace-store.ts:277 calls parseProjectPlanningAggregate without the
AgentMapAggregateError → AgentMapWorkspaceStoreError conversion that readDisk applies).

Failure: a project reaches 1,024 map versions. Every subsequent agent_map_propose throws
AgentMapProposalQuotaError("map_versions"), the tool answers internal_error with
recovery: "retry", and the coding agent retries a permanently-failing call in a loop with
no signal that the map is full. Add an explicit branch returning a terminal recovery.

3. Operation-history cap is unreachable — writes wall off before the declared quota

parseMapOperationHistory (agent-map-aggregate-migration.ts:210) rejects any aggregate
with more than 65,536 operation records. propose caps only versions
(this.versionHistoryLimit, default BUILD_PLAN_VERSION_HISTORY_LIMIT = 1,024) and pushes
one history record per operation, while agent-map-proposal-schema.ts:154 allows 256
operations per batch. 1,024 × 256 = 262,144 records, four times the parser's ceiling.

Failure: after ~256 accepted 256-operation batches, locked() re-parses the candidate
aggregate, parseMapOperationHistory calls malformed(), and the write is rejected. The
map becomes permanently unwritable at a limit that is never declared or checked, and
(per finding 2) reports as a retryable internal_error. Either bound
mapOperationHistory.length in propose with a real quota error, or raise the parser cap
to versionHistoryLimit × 256.

4. ~150 lines of unreachable, untested code added to a published package

packages/harness/src/core/build-plan-store.ts (89 lines: BuildPlanStore,
appendRestoredBuildPlanVersion) has zero callers and no test file —
grep -rn "BuildPlanStore\|appendRestoredBuildPlanVersion" src web matches only its own
definition. AgentMapWorkspaceStore.appendBriefVersions
(agent-map-workspace-store.ts:313, ~60 lines) says so in its own comment — "SAP-3149 has
no caller" — and has no test either; agent-map-workspace-store.test.ts never mentions
briefs. Relatedly, AgentMapProposalServiceOptions.readBaseRevision is now read by nothing
(baseGraph was deleted) yet stays in the options type, so a caller supplying it is
silently ignored; it should carry a real @deprecated tag pointing at the replacement.

This contradicts the PR's "Tests travel with this increment's behavior" and ships untested
persistence logic into every consumer's tarball. Either land these with the caller in the
increment that needs them, or land them with tests.


Verdict: Request changes — the patch changeset understates a breaking public type
change, and two hard storage limits fail in ways an agent will retry forever.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #828 (Agent Map 09/15)

Delta since the reviewed commit, on this PR's own files, is exactly three commits:
a635950c (rebase of the reviewed content), 31c639f2, c28d1898. No confidentiality
problems in the new copy: the rewritten changeset is generic role language.

Fixed

  • Breaking type change shipped as patch.changeset/atomic-project-state-migration.md
    is now minor (correct max signal on 0.14.0) with a Breaking: heading and a migration
    line pointing role/assignment readers at sessionId.
  • Quota failures reported as retryable internal_errorerrorResult
    (packages/harness/src/server/agent-map-mcp-tools.ts:81) now branches on
    AgentMapProposalQuotaError, AgentBriefAppendQuotaError, and AgentMapAggregateError,
    returning manual_intervention for everything except storage_unavailable. Covered by the
    new it.each case in agent-map-mcp.test.ts:185.
  • Unreachable operation-history cappropose now bounds
    mapOperationHistory.length + request.operations.length against operationHistoryLimit
    (agent-map-proposal-service.ts:274), defaulting to the parser's exported
    AGENT_MAP_OPERATION_HISTORY_LIMIT, so the write wall and the parse wall are the same
    number. Test asserts the aggregate is untouched and replay still works at the limit.

Not fixed

  • Dead code still ships. packages/harness/src/core/build-plan-store.ts (89 lines) still
    has zero callers and no test file, and AgentMapWorkspaceStore.appendBriefVersions
    (agent-map-workspace-store.ts:313) still carries its own "SAP-3149 has no caller" comment
    with no coverage — untested persistence logic in the published tarball. The related
    readBaseRevision half of that finding is resolved: it now carries a real @deprecated
    tag naming the replacement.

New (nit)

  • errorResult returns { code: "quota_exceeded", recovery } and drops error.resource, so
    an agent cannot tell a full map from an exhausted receipt table; include resource in
    structuredContent.

Verdict: Approve with a nit — the three blocking findings are resolved; the untested,
uncalled build-plan-store.ts should land with its caller or its tests.

@ynadge
ynadge force-pushed the review/agent-map-08-version-contracts branch from a5724be to d5c26ae Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-09-project-state branch from c28d189 to 7d947b1 Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-08-version-contracts to main September 6, 2026 22:21
@ynadge
ynadge changed the base branch from review/agent-map-08-version-contracts to main September 6, 2026 22:21
@ynadge
ynadge merged commit e61310f into main Sep 6, 2026
1 of 2 checks passed
@ynadge
ynadge deleted the review/agent-map-09-project-state branch September 6, 2026 22:21
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