Skip to content

feat(harness): add planner build plan authoring - #785

Open
ynadge wants to merge 7 commits into
conductor/e4-sap-3067-contracts-v1from
conductor/e4-sap-3068-authoring-tools
Open

feat(harness): add planner build plan authoring#785
ynadge wants to merge 7 commits into
conductor/e4-sap-3067-contracts-v1from
conductor/e4-sap-3068-authoring-tools

Conversation

@ynadge

@ynadge ynadge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

Trusted Agent Map planners need a structured, capability-bound way to author delivery intent without editing canonical files or giving builders global plan mutation authority. The E2 map tools cover architecture topology, but not exact-version build-plan creation, validation, atomic editing, or explicit source rebasing.

Summary and scope

Adds strict schemas and a transport-neutral BuildPlanService for read, validate, apply, and rebase operations. The existing Agent Map MCP endpoint registers these tools only for server-resolved map-planner identities while preserving the shipped E2 surface for all trusted roles. The service enforces exact plan/source versions, project scope, atomic plan/assignment/brief/receipt persistence, request-ID replay protection, bounded safe results and ID mappings, update-only canonical identities, and explicit assignment/repository/artifact rebase resolutions through injected compiler and impact boundaries. Client-correlated server IDs are scoped by the prospective plan version, keeping validate/apply mappings identical at one expected state while preventing later reuse of a client reference from aliasing immutable historical identities.

Planner create/resume context now carries a bounded exact-source/plan status, accurately describes the read/contracts-only transition, and tells the planner not to loop when authoring is unavailable. Content-free tool telemetry and security coverage verify role gating and prevent authored content from entering events. Production mutation composition fails closed with stable dependency errors until the follow-on installs the real brief compiler and impact evaluator. Confirmed-revision authoring likewise fails closed until the confirmed-revision persistence layer supplies an exact AgentMapRevisionSnapshot reader; the current upstream revision work exposes contracts but no durable reader. Session creation, architecture confirmation, builder-result acceptance, repository creation, deployment, and E5 behavior remain out of scope.

Related work

Closes: SAP-3068

Depends on: #784

Validation

pnpm --filter @sapiom/harness exec vitest run src/core/build-plan-schema.test.ts src/core/build-plan-service.test.ts src/core/build-plan-store.test.ts src/server/agent-map-mcp-tools.test.ts src/server/agent-map-mcp-wiring.test.ts src/core/planning-session.test.ts — passed (6 files / 81 tests)
pnpm --filter @sapiom/harness typecheck — passed
pnpm --filter @sapiom/harness lint — passed
pnpm --filter @sapiom/harness build — passed
pnpm --filter @sapiom/harness test — passed (213 standard files / 3,387 tests; 3 performance files / 10 tests)
git diff origin/conductor/e4-sap-3067-contracts-v1...HEAD --check — passed

Tests and documentation

Added strict-schema, service/idempotency/concurrency/atomicity/source-race/rebase, historical-read, mapping-boundary, canonical-ID authority, prospective-version identity, create-collision, atomic brief-history-limit, allocator-failure, MCP discovery/denial/telemetry, fail-closed full-server composition, and new/resumed focused-context tests. The identity regression verifies validate/apply mapping equality and a fresh canonical ID after create/remove/recreate with the same request-local client reference. Deterministic interleaving tests pause apply and rebase on their preflight store read, commit the same request reentrantly, and verify the outer call returns the persisted replay; a changed-payload race still returns idempotency_key_reused. Updated the planner profile and session-start guidance; the production integration requirements are documented above because this is a capability-bound internal MCP surface.

Compatibility and release impact

  • Breaking or externally visible changes: Map-planner sessions discover four new MCP tools; builder E2 tool discovery is unchanged. Existing aggregates remain readable because receipt result metadata is optional. Validation, application, and rebasing return authoring_unavailable until the follow-on supplies production compiler/impact dependencies, and confirmed-revision operations return revision_source_unavailable until an exact revision reader is wired.
  • Changeset: Added .changeset/quiet-planners-author.md for @sapiom/harness.

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I
    will follow the
    Security Policy for
    private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

OpenAI Codex implemented the scoped schemas, service, MCP wiring, tests, and planner guidance from the approved SAP-3068 plan. The result was self-reviewed against the exact stacked diff and verified with focused tests, harness typecheck/lint, and the complete harness standard/performance suites.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained
    any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

Add strict, capability-scoped build-plan validation, atomic plan versioning, explicit source rebasing, focused planner context, and content-free telemetry.

Closes: SAP-3068
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review — PR #785 (round 1)

Public-copy hygiene: clean. .changeset/quiet-planners-author.md is generic ("trusted Agent Map
planners"), names no company, no business arrangement, no internal host/incident reference. minor
is the right level — nothing new is exported from packages/harness/src/index.ts, files still
ships only dist, and no runtime dependency was added. The SAP-30xx comments match the
pre-existing convention in the same files, so I'm not flagging them.

Findings

1. The shipped briefCompiler stub returns briefs the store is guaranteed to reject

packages/harness/src/server/index.ts:2736 returns { briefs: currentBriefs, changes: [... "preserved"] }. BuildPlanService.apply/rebase forwards every returned brief to
BuildPlanStore.commitPlanVersion, which validates each one at build-plan-store.ts:400-412:
brief.plan.version !== plan.version and brief.version !== history.length + 1 both fail for an
unchanged brief, throwing BuildPlanStoreConflictError("version_conflict").

Failure: a project with one committed brief; planner calls build_plan_apply with any operation →
store conflict → surfaced as plan_version_conflict with recovery: "reread". Re-reading can never
clear it; the project's plan is permanently unauthorable. Only reachable once anything creates a
brief, but the "preserved" change kind is a declared part of the AgentBriefCompiler contract and
the production wiring returns it on every call. Every test compiler
(build-plan-service.test.ts:94, :114) returns either [] or freshly-versioned briefs, so the
path this PR actually ships is the one path with no coverage.

Fix: the service should commit only briefs whose change is created/changed, or the store should
accept an unchanged brief as a no-op.

2. build_plan_rebase has no resolution kind for repositoryIntents

rebaseResolutionSchema (build-plan-schema.ts:225) covers only remap-agent and
remove-assignment, but assertPlanReferences (build-plan-service.ts:790) also requires every
repositoryIntents[].plannedAgentId to be a top-level agent in the target graph.

Failure: plan holds a repository intent for agent A; a proposal removes A; the planner sends
{kind:"remove-assignment", plannedAgentId: A}. Assignments clean up, then rebase throws
invalid_reference with recovery: "correct" — and no rebase input can correct it. The only escape
is an out-of-band build_plan_apply set-repository-intents against the stale source first, which
neither the error nor the planner profile mentions. repositoryIntents / set-repository-intents
have zero test coverage in this PR.

3. build_plan_read with include: ["plan"] returns a plan that claims zero assignments

build-plan-service.ts:270-276 emits state: { ...plan, assignments: [] } while still carrying the
real semanticDigest and recordDigest. If the caller did not also ask for assignment-intents,
the returned object reads as an authoritative plan with no assignments, and its digests do not match
its own body. Either omit assignments from state entirely or always pair state with
assignmentIntents.

4. plan_not_found / stale-planId race reports cross_project_reference with recovery: "correct"

assertCurrent (build-plan-service.ts:654) throws cross_project_reference whenever
planId !== current.planId, including when the planner passes planId: null because it last saw no
plan. A second planner session that created v1 in between leaves the first one told to "correct" its
input when the right action is to re-read (recovery: "reread", as plan_version_conflict and
source_mismatch get). Distinguish "no plan seen but one exists" from a genuine cross-project id.

Nits

  • server/index.ts:2955 computes staleBriefCount with JSON.stringify(brief.source) !== JSON.stringify(plan.source) while BuildPlanService.read computes the identical concept with
    architectureSourceRefsEqual. One order-sensitive copy of the same rule; use the helper.
  • agent-map-mcp-tools.ts evaluates request.operations.length / request.resolutions.length
    outside instrument's try. preserveInvalidForService deliberately passes invalid values
    through, and null survives the !== undefined refine, so {"operations": null} throws a raw
    TypeError instead of the structured invalid_operation that seam exists to produce.
  • Large pure-reformatting blocks in agent-map-mcp.ts, agent-map-mcp-tools.ts and
    planning-session.ts (including de-indenting Boolean(...) continuations, which prettier would
    undo) inflate the diff against the PR's "no unrelated cleanup" claim.

Verdict

Request changes — finding 1 is a latent permanent-failure path in the production wiring this PR
ships, and findings 2–4 give the planner recovery hints that cannot succeed.

…v1' into conductor/e4-sap-3068-authoring-tools
…v1' into conductor/e4-sap-3068-authoring-tools
Address review findings around exact-source CAS, durable replay, explicit rebase resolutions, fail-closed production boundaries, and planner context safety.\n\nRefs: SAP-3068

ynadge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both review passes in e96668e (with the latest #784 head merged normally):

Automated findings:

  1. Preserved briefs are now explicit no-ops; only briefs compiled for the prospective plan/source are committed, with an existing-brief regression test.
  2. Rebase now has typed repository-intent remove/remap resolutions and requires them for broken ownership.
  3. include: ["plan"] returns the complete digest-bearing plan body; assignment projection no longer makes the body/digest dishonest.
  4. Concurrent initial creation now reports a rereadable plan-version conflict, while unknown plan IDs remain not-found rather than being mislabeled cross-project.
  5. The remaining source comparison uses architectureSourceRefsEqual.
  6. Operation/resolution counts are read only inside guarded instrumentation; null/non-array inputs return bounded invalid_operation results.
  7. Unrelated formatting-only changes were removed from the stacked diff.

Independent findings:

  1. Production composition now injects explicit fail-closed compiler/impact boundaries and returns actionable authoring_unavailable; the actual server composition is tested.
  2. Confirmed revisions fail closed with revision_source_unavailable until an exact persisted revision reader is injected; no snapshot is fabricated.
  3. Same-request apply/rebase races that reach the aggregate commit replay the original committed record when the request digest matches.
  4. Prospective plan, assignment, brief, and client-correlated subrecord IDs are deterministic/rollback-free; validation, compiler, CAS, and persistence failure tests assert no durable allocator consumption.
  5. Typed client-correlated create operations now allocate and return canonical milestone, criterion, deliverable, and decision mappings.
  6. Historical reads select briefs bound to the exact selected plan ref and expose current-pointer status separately.
  7. Rebase resolution coverage now includes deliverable artifact remove/remap in addition to assignments and repositories.
  8. Proposal validation checks observed current source, and commit rechecks proposal identity/version/digest under the aggregate lock; the proposal-replacement race is tested.
  9. Bounded receipt metadata preserves original brief changes, ID mappings, completeness, eligibility, and diagnostics so replay reconstructs the full original result.
  10. Planless focused context carries the exact active proposal identity/version/digest, or an explicit confirmed-revision-unavailable state.
  11. Tool telemetry uses capability-derived project/session correlation and remains content-free.

Security coverage also exercises planned/manual builder denial, capability rotation/revocation, strict rejection of model-supplied scope, malformed collections, and cross-project source rejection. The #784 writes → artifact ← reads semantics and production-valid ownerless-artifact fixture remain intact.

Fresh verification: 72 focused tests, typecheck, lint, build, 3,365 standard tests, and 10 performance tests all pass.

Correction at 9cf604dc26172310e393dd05fc9b8ee2c63f0243: the original wording overstated the concurrency coverage at e96668e. A matching request that committed after the initial receipt lookup but before the apply/rebase preflight reread could still surface plan_version_conflict. The later fix rechecks the receipt on pre-commit CAS/source conflicts and adds exact interleaving regressions for apply and rebase; the current-head response contains the updated evidence.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review — PR #785 (round 2, delta since 96082df)

Round‑1 findings 1–4 are all fixed (fail‑closed compiler + committableBriefs; repository‑intent
and artifact rebase resolutions; state: plan; planId: nullplan_version_conflict). Both
code nits fixed. Changeset text is unchanged and still clean. Nothing round 1 said was wrong.

New findings

1. idMappings is unbounded in the service but capped at 128 by the persisted receipt

build-plan-service.ts:946 emits idMappings with no .slice(0, 128) — the only bounded-list
field in that result without one (briefChanges, impactedAssignments, diagnostics all slice).
The receipt codec caps it: build-plan-codec.ts idMappings: …max(128).

Failure: one create-agent-assignment with 128 acceptanceCriteria plus one deliverable mints 129
mappings. Schema validation passes, prepare passes, then commitPlanVersion
parseAggregate(outcome.next) throws a ZodError that is neither BuildPlanStoreConflictError nor
BuildPlanStoreLimitError, so apply's catch rethrows it raw. The planner gets an unstructured
error instead of result_too_large/invalid_operation, with no recovery hint. Slice idMappings
to 128 and fail with a bounded code when truncation would lose a mapping.

2. The published copy promises authoring that the shipped wiring cannot perform

server/index.ts:2737 now wires unavailableAgentBriefCompiler, and prepare calls
compileBriefs — so build_plan_validate, build_plan_apply and build_plan_rebase all return
authoring_unavailable in the real server (agent-map-mcp-wiring.test.ts asserts exactly this).
Fail‑closed is the right call, but the surfaces shipped alongside it were not updated:
.changeset/quiet-planners-author.md advertises "atomic authoring, and explicit rebase tools" into
the npm CHANGELOG, and agent-map-planner.ts tells the planner to "validate a bounded atomic batch,
then apply it" while the user-facing session-start message promises "a validated delivery plan". A
consumer on this minor version, and the planner model itself, will loop on a tool hard-wired to
error until SAP-3070. Either register only build_plan_read until the compiler lands, or say in the
changeset and the profile that authoring is inert pending its production dependency.

Nits

  • The push added a second, larger round of unrelated reformatting — this time against the repo's
    .prettierrc.json printWidth: 80 (e.g. agent-map-mcp-tools.ts:32, planning-session.ts:106,
    server/index.ts:677). prettier --write would revert it. Round 1 already flagged the churn.
  • BuildPlanIdFactory (build-plan-service.ts:132-134) now has zero call sites after the switch to
    deterministicId, but is still a required dependency wired in server/index.ts.

Verdict

Request changes — finding 1 is an unstructured failure on a schema-valid request; finding 2 ships
changelog and prompt copy for a capability the package cannot execute.

ynadge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the exact-head follow-up findings in 67ce6eb1796d72158e3f177e3945d8754298c8b0:

  • ID-mapping bounds: the service now enforces the receipt codec's shared 128-mapping limit before compilation or persistence, returns bounded result_too_large guidance with split_batch recovery, accepts exactly 128 mappings, and rejects the reproduced 129-mapping request without side effects or truncation.
  • Transition copy: the changeset, planner prompt, focused context, and session-start orientation now state that reads/contracts are available while validation/application/rebase remain fail-closed pending production dependencies; planners are explicitly told not to retry or loop on authoring_unavailable.
  • Canonical identity authority: update-shaped operations accept canonical IDs only when the record already exists at the correct request-start and prospective scope. Newly created records use request-local client refs and deterministic server IDs; duplicate, cross-kind, forward, and cross-request client-ref aliasing fail closed. Exact request replay still returns the original mappings.
  • Create invariants: milestone, integration-criterion, plan-decision, and agent-assignment create operations cannot replace existing or earlier-in-batch records. An assignment upsert can still attach an architecture agent, but any nested identity must already exist or come from an explicit create declaration.
  • Atomic brief history: inline briefs in commitPlanVersion now enforce the configured per-brief history cap before persistence. The regression proves a second plan+brief commit leaves plan/brief histories, current pointers, receipts, and durable bytes unchanged.
  • Cleanup: removed the unused BuildPlanIdFactory service dependency and production wiring, kept legacy-file edits focused, and passed the scoped Prettier check.

Fresh verification at the pushed head:

  • Focused Vitest: 6 files, 77 tests passed.
  • Typecheck, lint, and production build passed.
  • Full harness: 213 files / 3,383 tests plus 3 performance files / 10 tests passed.
  • Exact stacked diff check passed; the PR remains based on feat(harness): add durable build planning contracts #784.

ynadge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the historical client-reference identity blocker in de139b310f224197515a1f69fe21d269d2ae3607:

  • Deterministic subrecord IDs now include the prospective target plan version. Validation and application therefore return identical mappings for the same expected state.
  • Each successfully committed plan version has a fresh identity namespace. A v1 create, v2 remove, and v3 recreate using the same request-local client reference now produces a new canonical ID instead of aliasing the immutable v1 record.
  • The regression covers validate/apply mapping equality, unchanged v1 history, the absent v2 record, the distinct v3 identity/content, and replay preserving the original mapping.
  • The earlier cross-request expectation was updated accordingly: a client reference is request-local correlation data, not a durable identity that may select an unrelated historical record.

Fresh verification at the pushed head: Prettier check; focused tests (6 files / 78 tests); typecheck; lint; production build; full harness suite (213 files / 3,384 tests plus 3 performance files / 10 tests); and stacked diff check all passed.

ynadge commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Correction and current-head remediation for the remaining concurrency gap, pushed in 9cf604dc26172310e393dd05fc9b8ee2c63f0243:

  • The earlier response's blanket statement that all concurrent same-request apply/rebase races replayed was too broad. The aggregate-commit race replayed, but a commit between the initial receipt lookup and the preflight reread could still surface plan_version_conflict.
  • Apply and rebase now recheck the capability-scoped receipt when pre-commit plan/source CAS fails. A matching canonical request digest returns the original persisted result; a different digest still returns idempotency_key_reused.
  • The apply regression pauses the outer call's second store read, commits the identical request reentrantly, resumes against the advanced plan, and verifies a full replay with only one plan version.
  • The rebase regression uses the same exact interleaving and verifies the outer call replays the committed v2 result with only the expected v1/v2 history.
  • A separate deterministic apply interleaving commits a changed payload under the same request ID and verifies idempotency_key_reused with no second version.

Fresh exact-head verification: scoped Prettier check; focused tests (6 files / 81 tests); typecheck; lint; production build; full harness suite (213 files / 3,387 tests plus 3 performance files / 10 tests); and stacked diff check all passed. This remediation is ready for independent re-review; this response does not claim that review is complete.

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