Skip to content

feat(harness): expose shared plan authoring [Agent Map 10/15] - #829

Merged
ynadge merged 1 commit into
mainfrom
review/agent-map-10-plan-tools
Sep 6, 2026
Merged

feat(harness): expose shared plan authoring [Agent Map 10/15]#829
ynadge merged 1 commit into
mainfrom
review/agent-map-10-plan-tools

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Every project session needs to read and update the same build plan without overwriting concurrent changes or requiring a planner role.

Summary and scope

Implement read, validate, apply, and rebase services and MCP tools with deterministic IDs, explicit conflict handling, dependency validation, and idempotent request receipts.

Prioritize validation errors before warnings when diagnostics hit their size bound. Timestamp semantic no-op receipts when the request is accepted, so replay and audit metadata remain accurate.

How this increment fits

Plan reads and writes return their committed result directly. Part 11 adds best-effort brief refresh after acceptance.

Stack and review boundary

  • Part 10 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-09-project-state.
  • Current head: 143787af9fd7765ad57de817f3b8768a96333b2c; 2,143 changed lines across 14 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #806. 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 68f0d85edbc02ade6282bfa8ae916f2785e2bac5. 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 143787af9fd7765ad57de817f3b8768a96333b2c.

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: Read/validate/apply/rebase, dependency validation, deterministic IDs, optimistic conflicts, exact request replay, warning saturation, and semantic no-op receipts.

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: Adds shared build-plan authoring services and MCP tools. Canonical request reuse is idempotent; changed content requires a new request key.
  • Changeset: Included: .changeset/neutral-shared-plan-versions.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 #829 (Agent Map 10/15, shared plan authoring)

1. Diagnostic truncation silently disables the error gate (correctness, high)

build-plan-contract-validator.ts:122-124 sorts diagnostics by path and then
.slice(0, BUILD_PLAN_DIAGNOSTIC_LIMIT) (64). Every caller that decides whether a write is
legal reads the truncated array:

  • build-plan-service.ts:428historicalDiagnostics.some(({ severity }) => severity === "error")
  • build-plan-service.ts:442 — the merged-content gate
  • build-plan-service.ts:569 — the rebase_resolution_required gate

Because the sort key is the path string, warnings routinely sort ahead of errors and evict them:

  • 64 status: "open" decisions produce 64 warnings at decisions[0..63]; "decisions[" sorts
    before "milestones[", "repositoryIntents[", "sequenceGates[". Add one duplicate milestone
    ordinal and the only error in the list is sliced off — prepareApply sees no errors and
    commits a plan with a duplicate ordinal. The schema allows 128 decisions plus 128
    unresolvedDecisions, so this is reachable with valid input.
  • Same shape with >64 assignments (schema max 128): diagnostics for the high-index assignments are
    dropped wholesale, so invalid-dependency and unknown-node-reference errors there never gate
    the write.

Until PR 11 wires briefs, activeBriefIds is empty and every assignment emits a missing-brief
warning, so the diagnostic list hits 64 on ordinary plans, not adversarial ones.

Fix: compute the error/no-error decision (and the affectedPaths for the thrown error) over the
untruncated list, or sort severity === "error" first and truncate only what is returned for
display. Add a test asserting an error survives when >64 warnings are present — the current suite
has no coverage of the limit at all.

2. malformed_state / unsupported_schema now tell the agent to retry forever (correctness)

agent-map-mcp-tools.ts changed the AgentMapWorkspaceStoreError branch from a flat
{ code: "storage_unavailable", recovery: "retry" } to { code: error.code, recovery: error.code === "storage_unavailable" ? "retry" : "reread" }. AgentMapWorkspaceStoreError carries three codes
(agent-map-workspace-store.ts:64-73): storage_unavailable, malformed_state,
unsupported_schema. The latter two are permanent — a corrupt or future-schema state file will
never parse — but they now advertise recovery: "reread", which is exactly the loop the
quota_exceededmanual_intervention design elsewhere in this PR exists to avoid. Use
manual_intervention for those two.

This also silently changes the error contract of the three pre-existing agent_map_* tools (they
previously only ever emitted storage_unavailable), which nothing in the changeset mentions.

3. The breaking note is on the wrong changeset (semver/changelog)

.changeset/neutral-shared-plan-versions.md is minor and carries Breaking: for the
ProposalActor role/assignment removal. That removal shipped in the previous commit
(d6e8c91a), whose changeset — atomic-project-state-migration.md — is patch.
immutable-project-version-contracts.md (patch) describes the same contract work a third time.
ProposalActor is a public export (src/index.ts:24).

The compiled packages/harness/CHANGELOG.md will therefore ship three overlapping descriptions of
one change, with the breaking removal appearing under two patch-level lines. A consumer on ^0.14
reading the changelog sees a patch entry for a type field that no longer exists. Put the migration
note on the changeset for the increment that actually removes the fields, and cut this changeset
down to what this PR adds (the four build_plan_* tools) rather than restating the stack.

4. A semantic no-op moves updatedAt backwards (bug)

build-plan-service.ts:599-623: on prepared.noOp, prepared.plan is the existing current
version, so next.updatedAt = prepared.plan.createdAt rewrites the aggregate timestamp to that
older version's creation time while recordVersion still increments. Any consumer ordering or
staleness-checking on updatedAt sees time go backwards after an idempotent re-apply. Use
this.now() (or leave updatedAt alone) on the no-op path.

5. build_plan.operation telemetry can never report a failure

BuildPlanServiceOptions.onOutcome declares outcomes "conflict" | "failed" and an
affectedCount, and shared/types.ts gains the build_plan.operation analytics event for them.
But emit() is only reached on success paths — every BuildPlanServiceError throws past it — and
affectedCount is passed 0 at all six call sites. So the new event type answers only "how often
did writes succeed", and the outcome / affected_count fields in server/index.ts:3080-3086 are
dead. Either emit from the catch paths (with error.details.affectedIds.length) or narrow the
declared union so the payload does not overstate what is collected.


Verdict: Request changes. No confidentiality or tarball-hygiene problems — no new deps, no
public exports added, docs/ is outside files. Finding 1 is the blocker: the write gate can be
bypassed by ordinary content, and the suite does not test the limit.

@ynadge
ynadge force-pushed the review/agent-map-09-project-state branch from d6e8c91 to c28d189 Compare September 5, 2026 11:57
@ynadge
ynadge force-pushed the review/agent-map-10-plan-tools branch from ad8c013 to 68f0d85 Compare September 5, 2026 11:57
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #829 (delta since ad8c0130)

Delta is one commit, 68f0d85e, touching 4 files (changeset, contract validator, service, service tests).

Not fixed from round 1

  • build_plan.operation telemetry still can't report failure or scope (round-1 finding 5).
    BuildPlanServiceOptions.onOutcome still declares "conflict" | "failed"
    (build-plan-service.ts:96) and an affectedCount, but all six emit() call sites are on
    success paths only (lines 337, 345, 356, 372, 384, 398) and every one passes affectedCount: 0.
    The outcome / affected_count fields wired into the public build_plan.operation event
    (shared/types.ts:833, server/index.ts:3080-3086) are therefore dead. Emit from the
    BuildPlanServiceError catch paths, or narrow the union to what is actually collected.

Fixed

  • Finding 1 (truncation disables the error gate): build-plan-contract-validator.ts:122 now sorts
    severity === "error" ahead of warnings before .slice(64), so a blocking error survives
    warning saturation. build-plan-service.test.ts covers it for both validate and apply.
  • Finding 2: agent-map-mcp-tools.ts:104 now maps non-storage_unavailable store codes to
    manual_intervention rather than reread.
  • Finding 3: neutral-shared-plan-versions.md is cut down to this PR's four build_plan_* tools;
    the ProposalActor Breaking: note now lives on atomic-project-state-migration.md, which
    was also raised to minor. No overlapping descriptions remain.
  • Finding 4: commit() uses committedAt = this.now() on the no-op path for both the receipt and
    updatedAt, so idempotent re-apply no longer moves the aggregate timestamp backwards.

New findings

None. No confidentiality, dependency, or tarball-hygiene changes in the delta; the rewritten
changeset text is provider-neutral and correctly scoped at minor.

Round-1 corrections

None — nothing in the earlier review was wrong.


Verdict: Approve once finding 5 is addressed; it is a telemetry-contract overstatement, not a
correctness blocker.

@ynadge
ynadge force-pushed the review/agent-map-09-project-state branch from c28d189 to 7d947b1 Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-10-plan-tools branch from 68f0d85 to 143787a Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-09-project-state to main September 6, 2026 22:21
@ynadge
ynadge merged commit a7f2f59 into main Sep 6, 2026
1 check passed
@ynadge
ynadge deleted the review/agent-map-10-plan-tools 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