feat(harness): launch read-only builder planning sessions

[ynadge]

Summary

• add a planner-only, two-phase MCP flow: once the exact build plan is planningEligible, the planner prepares the top-level session set, summarizes every agent mission and exact brief version, asks for explicit conversational consent, and stops for the reply
• after an affirmative reply, let the planner attest that reply and open or reuse the prepared sessions; there is no Studio button or browser mutation route
• mechanically require a non-empty user submission to enter the same planner session after preparation before a pending consent can open anything; queued inputs retain their original acceptance time, non-empty raw terminal lines are stamped synchronously at the authenticated PTY boundary, and empty/TUI Enter gestures or programmatic prompts do not count
• persist consent against both user-turn receipts plus the exact user, planner session, architecture source version/digest, build-plan version/digest, complete assignment set, and focused brief versions/digests; revalidate all of it transactionally before any process side effect
• fail stale scopes closed so any source, plan, assignment, or brief change requires the planner to prepare the changed list and ask again
• open sessions only for top-level planned agents under adapter-enforced planning-readonly; implementation and deployment remain behind a separate execution gate
• preserve one stable primary binding per assignment with lifecycle epochs, spawn and kickoff leases, acknowledged delivery, and idempotent retry behavior so retries cannot create duplicate processes or tabs
• leave ordinary user-started and other unplanned sessions outside the fan-out path, without changing their identity, execution policy, lifecycle, or tabs
• isolate coordinator-local reachability after the atomic fan-out claim and report ordered partial outcomes without mutating foreign ownership
• provide exact trusted bootstrap context and persist strict, immutable, assignment-bound planning results with durable idempotent replay and owned proposal-lineage validation

Consent and Studio UX

1. An exact build-plan read, apply, or rebase reports planningEligible=true.
2. The planner calls build_plan_prepare_planning_sessions with that exact source, plan, and complete active assignment set.
3. The tool returns a durable consent ID plus the top-level sessions, missions, brief IDs, brief versions, brief digests, and read-only policy. It creates no sessions.
4. The planner presents that list in the conversation, explicitly says these are read-only planning sessions, asks for consent, and waits.
5. Studio durably records a non-empty user submission that entered that planner session after preparation. A same-turn open, empty/TUI Enter, or message queued before preparation is rejected with user_reply_required; the planner must stop and wait.
6. Only after an affirmative user reply does the planner call build_plan_open_planning_sessions with the unchanged prepared scope and user-confirmed attestation.
7. Studio revalidates the distinct turn token, readiness, and exact scope, then opens or reuses the planning tabs. A stale scope is rejected and must be prepared and approved again.

The existing project-wide tab strip remains the session UX. Planning tabs visibly carry their read-only planning state. This epic does not authorize implementation or deployment.

Safety evidence

• only a live, owned map-planner session receives the prepare/open tools; builder and manual sessions cannot discover or call them
• nested planned-agent assignments fail closed instead of receiving sessions
• preparation has no spawn side effect
• prepare and open in the same user turn fail closed before process creation; semantic interpretation of the later reply remains an explicit planner attestation
• turn evidence survives Studio restart, while prerelease consent records without turn evidence are discarded and require fresh consent
• non-empty raw terminal lines are stamped before the planner can issue its follow-up MCP request; empty/TUI navigation Enter, greetings, kickoffs, and other programmatic submissions cannot manufacture consent turns
• queued messages keep their original server-acceptance time, so backlog created before preparation cannot later qualify merely because PTY delivery was delayed
• the accepted-input watermark is monotonic, so delayed old queue delivery cannot erase a newer qualifying reply
• Studio proves the post-preparation input boundary but deliberately does not interpret natural-language affirmation; that semantic decision remains an explicit planner attestation
• concurrent opens and repeated opens converge on the same durable primary binding and tab
• an unplanned manual session remains byte-for-byte unchanged across fan-out retries
• the former authenticated-browser approval endpoint is absent, and an authenticated browser POST receives 404
• source, plan, assignment, brief, planner-session, user, and consent-digest mismatches fail before process creation

Verification

• focused consent, lifecycle, MCP, REST, terminal, and codec suite: 7 files, 258 tests passed
• full harness unit/integration suite: 218 files, 3,509 tests passed
• full harness performance suite: 3 files, 10 tests passed
• Playwright session-tabs and Agent Map planning specs: 24 tests passed without retry
• typecheck (core, web, and public API): passed
• lint: passed
• dependency-aware production build: passed
• diff whitespace checks: passed

Depends on #786

Closes: SAP-3074

[ynadge]

Holding for the final dependency head before reopening this stacked PR.

[github-actions]

Review — PR #787 (feat(harness): launch read-only builder planning sessions)

🔒 Confidentiality: clean. No non-Sapiom company names, business arrangements, private hostnames, or internal links in the new prose (profiles/agent-map-builder-planning.ts, JSDoc, comments, e2e fixtures). SAP-#### refs match 60+ existing occurrences in shipped source, so they are in-policy here.

---

1. Unhandled rejection from deliverKickoff can kill the server process

core/builder-planning-session.ts:829, :910, :1087 all call void this.deliverKickoff(...) with no .catch. deliverKickoff awaits updateBinding twice (lines 957, 996) outside any try — workspaceStore.transact throws AgentMapWorkspaceStoreError on a lock timeout, a storage error, or a strict-codec rejection, and updateBinding itself throws BuilderPlanningSessionError("binding_stale"). There is no process.on("unhandledRejection") anywhere in packages/harness, so under Node's default mode this terminates the Studio server (and the Electron main process).

Every sibling fire-and-forget in server/index.ts — including the two new builderPlanningSessions.onSessionStatus / onEventPersisted calls added in this same PR — attaches .catch. These three do not.

Failure: workspace file lock contention (two Studio hosts, or a slow disk) during kickoff delivery → unhandled rejection → server exits mid-session.

2. No changeset, on a published package that gains public API

src/index.ts:6 is export * from "./shared/types.js", so SessionExecutionPolicy, BuilderPlanningLifecycleState, BuilderPlanningSessionMetadata, and the new HarnessSession.executionPolicy / .builderPlanning and LaunchOpts.executionPolicy fields all become @sapiom/harness public surface. The PR also adds two REST routes, changes ClaudeCodeAdapter/CodexAdapter launch args, and adds three required fields to the persisted aggregate — with no .changeset/*.md. The three changesets on disk belong to SAP-3068/3070. This ships inside the stacked minors with zero changelog entry describing it.

Needs a minor changeset. Worth stating in it whether LaunchOpts.executionPolicy is a contract third-party adapter implementors must honor, or internal-only.

3. planningSubmissionReceipts is never pruned and permanently wedges submissions

builder-planning-session.ts:1276-1280 throws invalid_request once planningSubmissionReceipts.length >= 1_024. Nothing ever removes a receipt (contrast fanoutApprovals, which uses .slice(-256) at line 448), and the codec caps the array at 1024 too.

Failure: after 1024 lifetime planning submissions in one project — each distinct requestId, including every builder retry, adds one — planning_result_submit fails forever for every assignment, with error code invalid_request and recovery hint reread. No recovery path short of hand-editing workspace.json.

4. The only Codex config combination that ships is the one not tested

adapters/codex.ts:480 pushes -c mcp_servers={} and then, immediately after, -c mcp_servers.agent-map.url=…. Production always hits both: server/index.ts:1164-1173 attaches agentMapMcp to every Agent Map session, and planning-readonly builders are Agent Map sessions. But the new test (codex.test.ts, "enforces the planning-readonly sandbox and clears inherited MCP servers") passes executionPolicy without agentMapMcp, and the existing agent-map test passes agentMapMcp without the policy.

If codex applies the table assignment after the dotted inserts, or replaces rather than merges, a Codex planning session launches with no agent-map server — no planning_result_submit, so the binding sits in planning forever. Add the combined-args assertion.

5. The consent surface understates what the user is approving

preview() sets expectedModelTurnCount: assignmentIds.length (builder-planning-session.ts:308) and AgentMapPane.tsx renders it verbatim as "N model turns" next to the "Open planning sessions" button. That number is kickoff prompts injected, not model turns consumed — each session then runs an unbounded planning loop. A user reading "3 model turns" before authorizing three agent sessions is being told the wrong order of magnitude. Either rename the field to what it counts (kickoffPromptCount) or drop it from the consent copy.

6. reconcileKickoffAttempt asserts a kickoff it does not guard

builder-planning-session.ts:241 spreads binding.kickoff!. It runs inside updateBinding, which re-reads the binding from the store — so it sees whatever landed since delivering was written, and ensureSession:809 sets kickoff: null on the exited-session path. {...null} yields {}, producing a kickoff missing kickoffId/inputId/attemptCount/deliveredAt/acknowledgedBy. kickoffSchema is .strict() with all of those required, so the transaction's re-parse rejects — which is finding #1's crash path, reached by a real race rather than by infrastructure failure. Guard on value.kickoff and return value unchanged when it is null.

7. "Open planning sessions" silently does nothing without an active planner session

AgentMapPane.tsx renders the button whenever planningFanout.available, and App.tsx:387 always supplies onOpenPlanningFanout. But openPlanningFanout (App.tsx:378) returns early on !activePlannerForProject, with no toast and no disabled state. A build plan exists without a live planner session, so the enabled-looking button is a dead click. Disable it, or pass undefined when there is no planner.

---

Verdict: Request changes. The unhandled-rejection sites (#1/#6) and the missing changeset (#2) should land before merge; #3 and #4 are cheap to fix now and expensive later.

[ynadge]

Resolved in 5d13e690197e159b683aad07ced9bf9f8e2a90ee.

Hosted review findings:

1. Every fire-and-forget kickoff delivery now attaches a rejection handler.
2. Added a minor harness changeset covering the new public/runtime contract.
3. Idempotency receipts are pruned to a rolling 256 entries; exact replay remains available from immutable submission history, so pruning cannot permit conflicting reuse or wedge the project at 1,024 receipts.
4. Codex has a combined planning-readonly plus scoped Agent Map MCP argv regression.
5. Consent copy now describes the count as kickoff prompts.
6. Kickoff reconciliation returns unchanged when no kickoff exists and preserves acknowledged terminal state.
7. Fan-out remains visible but disabled with an explicit unavailable reason when no active Planner owns the action.

The security/lifecycle remediation also covers builder-owned direct proposal successors, idempotent replay before mutable freshness checks, in-transaction source/ref validation, strict duplicate/opaque-ID parsing, side-effect-free project preview, durable spawn/kickoff CAS leases, stale-session mutation denial, exact Claude tool allow-listing, trusted same-ID REST resume, proactive targeted staleness, and visible lifecycle/additional-session behavior.

Exact-head evidence: 366 focused tests, 23 Playwright tests, 3,456 full unit tests, 10 performance tests, plus typecheck, lint, production build, formatting, and diff checks all pass. The stock-research browser assertion still proves the authorized coding-agent follow-up updates the live map with its attribution.

[github-actions]

Follow-up review — delta since 490e3c8

Delta is 5d13e690 plus the merge of #786's head (reviewed there). Confidentiality clean — the only new prose is two changesets, both generic.

Round 1: all 7 addressed. Catches on every void deliverKickoff; minor changeset added; receipts pruned to 256 with replay falling back to immutable submission history (covered by test); combined codex argv assertion; consent copy reads "kickoff prompts"; reconcileKickoffAttempt returns unchanged when kickoff is null and never downgrades delivered; fan-out button disabled with an explicit reason.

New

1. Secondary planning sessions are handed tools they can never use. openAdditionalSession (core/builder-planning-session.ts:1665) mints the session with agentMapIdentity, so the server attaches the Agent Map MCP and Claude allow-lists agent_map_propose + planning_result_submit. Both reject primary === false with binding_stale, and server/agent-map-mcp-tools.ts:246 maps that to recovery: "reread" — the model is told to re-read and retry a call that can never succeed. Failure: user opens a second tab on an assignment; that builder loops submit → reread → submit, burning turns. Withhold the two tools from non-primary sessions, or return a terminal recovery hint.

2. shared/types.ts:256 — builderPlanning's JSDoc still says "Present only on trusted primary planned-builder sessions", but additional sessions now carry it with primary: false. Wrong doc on an exported type.

3. resume() (builder-planning-session.ts:1613) writes state: "failed" / resume_failed to the durable binding on any throw, including SessionAlreadyLiveError from a double-click. openAdditionalSession refuses failed bindings, so a benign double-resume blocks "new session" until a resume succeeds.

Round 1 got wrong

Finding #2 asked whether LaunchOpts.executionPolicy is a contract third-party adapter authors must honor. SPAWNABLE_HARNESS_KINDS is a closed two-value const, so no third-party spawnable adapter exists — moot, and the changeset was right to omit it.

Verdict: approve once #1 is resolved; #2/#3 are one-liners.

[ynadge]

Resolved in 19142e5735c8205e39b22cf67e3a7d02ca46c8dc.

Hosted follow-up findings:

1. Secondary planning sessions now receive no Agent Map MCP identity or proposal/result allow-list. Their trusted profile no longer instructs them to call unavailable tools; exact argv, server configuration, and service tests cover the restricted surface.
2. The exported builderPlanning documentation now explicitly describes both primary and secondary trusted metadata.
3. Benign already-live/double-resume conflicts no longer mutate the durable binding, and secondary resume failures never poison primary authority. A genuine primary terminal resume failure remains visible.

The same follow-up closes the independent lifecycle/integrity audit: proposal receipt replay is checked after basic authenticated scope but before mutable freshness; changed request-ID payloads conflict deterministically; opaque IDs and unsafe ordinals fail at the request boundary; every binding mutation is fenced by immutable context plus operation-specific session/claim epoch; independent registries cannot infer global session death from a local miss; lost attach/resume races stop only the invocation-owned orphan; kickoff acknowledgement cannot revive a replacement context; and result status is consistent with owned direct-successor provenance across compatible later descendants. Claude's planning-readonly minimum is now 2.1.248 with boundary coverage.

Exact final-tree evidence:

• full harness unit suite: 218 files / 3,473 tests passed
• full performance suite: 3 files / 10 tests passed
• Playwright session-tabs and Agent Map planning specs: 23/23 passed
• typecheck, lint, and production build passed
• changed-file Prettier and git diff --check passed, with baseline formatting preserved in the two pre-existing nonconformant test files

The stock-research browser flow retains its live-map attribution assertion and now also covers trusted same-ID secondary resume.

[ynadge]

/review

[github-actions]

Follow-up review — delta since 5d13e690

Delta is one commit, 19142e57. Confidentiality clean — the only new prose is the secondary planning system prompt and JSDoc, both generic. No new changesets in this delta.

Round 2: all 3 addressed. Secondary sessions get no agentMapMcp, no --allowedTools, and a distinct prompt that never names the two tools (covered by adapter + wiring tests); builderPlanning JSDoc now describes primary: false; resume() rethrows SessionAlreadyLiveError and never mutates the durable binding on a secondary or benign-conflict failure.

New

1. MIN_CLAUDE_CODE_VERSION jumps 2.1.83 → 2.1.248 with no changeset and no doc. core/adapters/claude-code.ts:49 is a global floor: ClaudeCodeAdapter.doctor returns ok: false and cli/doctor.ts:96 prints "too old — need >= 2.1.248" for every session type, but the stated reason (--restricted, claude-code.ts:395) is only sent when executionPolicy === "planning-readonly". Failure: an existing @sapiom/harness consumer on claude 2.1.100 who never opens a planning session gets a red doctor and an upgrade remedy on a binary that runs every ordinary session fine. None of the four changesets mention the raised requirement, so it ships silently inside a minor. Either name the new floor (and the date it became available) in real-builder-planning-sessions.md, or gate --restricted support separately from the global floor.

2. Registry miss now silently no-ops the fan-out. builder-planning-session.ts:1137 returns the binding unchanged when current.sessionId is set but the session is absent from the registry — no state change, no failure code, no error to the caller. Failure: the session record is gone (pruned/hand-edited sessions.json); "Open planning sessions" returns success, spawns nothing, and claimSpawn:1087 refuses to re-spawn because sessionId is non-null — a permanently silent dead click. The previous state: "failed" was wrong for multi-coordinator setups but at least visible; surface a distinct non-terminal "unreachable" projection instead of returning as if nothing is wrong.

Verdict: Approve once #1's changeset line lands; #2 is a UX dead end worth a follow-up.

[ynadge]

Resolved in bd910a819edf7c63f56413d6710fa97dae5b833c.

This follow-up closes the complete seven-finding review set:

1. Session metadata projection now uses a durable monotonic lifecycle epoch plus full immutable-context compare-and-set at the SessionManager mutation boundary. Every caller goes through one projection helper; stale replacement-context and stale same-context lifecycle projections are no-ops.
2. New result submissions require the binding to be planning with a durably acknowledged delivered kickoff. Receipt/history replay remains ahead of mutable lifecycle checks. Terminal submitted/stale/failed states cannot be reclaimed or regressed by readiness or acknowledgement callbacks.
3. Only the durable spawn claimant may attach an unbound matching process. Losers cannot steal a live claim, expired-claim recovery may reconcile an existing process, and cleanup rereads durable ownership before killing so a same-ID attached winner remains live.
4. Kickoff completion is fenced by exact state, delivery claim, session, spawn epoch, and context. A stale reconciliation winning during adapter return remains stale, and subsequent callbacks remain no-ops.
5. Every planning-result text field now enforces the persisted safe-text contract at the request boundary, including C0/DEL and lone-surrogate rejection. Defensive final-codec failures map to bounded invalid_request errors without receipts or mutations.
6. The minor changeset now discloses the global Claude Code >=2.1.248 requirement and its 2026-08-27 npm publication date, verified from authoritative npm registry package metadata. Existing 2.1.247 rejection / 2.1.248 acceptance coverage remains green.
7. A durable session absent from the local registry now returns bounded session_unreachable rather than a false successful launch, while leaving possible foreign-coordinator ownership untouched. Service and authenticated route tests cover the non-destructive error path.

Deterministic regressions force the old-reconcile → replacement → old-projection ordering, winner-create → loser-open → winner-attach ordering, early-result ordering, post-submission delayed callbacks, and stale-reconcile during kickoff completion.

Exact final-tree evidence:

• affected suite: 10 files / 372 tests passed
• full harness unit suite: 218 files / 3,481 tests passed
• full performance suite: 3 files / 10 tests passed
• Playwright session-tabs and Agent Map planning specs: 23/23 passed
• typecheck, lint, production build, changed-file Prettier, and diff whitespace checks passed

The first browser attempt encountered the known strict-locator flake when two valid running-session dots were present; the isolated case and the complete 23-case rerun both passed without code changes.

[ynadge]

/review

[github-actions]

Follow-up review — delta since 19142e57

Delta is one commit, bd910a81. Confidentiality clean — the only new prose is the changeset line naming the Claude Code 2.1.248 floor and its npm publish date, both public facts.

Round 3: both addressed. The changeset now discloses the global >=2.1.248 requirement; the registry miss no longer returns success. But the fix for #2 traded a silent no-op for a wider failure — see #1.

New

1. One unreachable binding aborts the whole fan-out, permanently. ensureSession (core/builder-planning-session.ts:1203) now throws session_unreachable on a registry miss, and openOrReuse:1032-1041 awaits it in a sequential for loop with no per-assignment isolation. Failure: a project with 3 assignments where one durable binding points at a session missing from sessions.json (pruned, or owned by another coordinator) — "Open planning sessions" returns 409 and the other two assignments never launch, even though the transaction already committed their bindings. Every retry re-throws at the same assignment, so the whole project's fan-out is wedged until that one binding is hand-repaired. The previous round asked for a visible non-terminal projection, not a throw; catch per assignment and return the unreachable binding with its state surfaced.

2. delivery-uncertain bindings can no longer submit a result, and nothing can rescue them. submitPlanningResult:2318 now hard-requires state === "planning" && kickoff.state === "delivered". The only transition into planning in the whole file is onEventPersisted:1675, driven by a decorated prompt.submitted hook event. Failure: submitInput is accepted but the acknowledgement event is lost (server restart between submit and persist, event dropped) → the builder receives the prompt, plans, calls planning_result_submit, gets binding_stale/recovery: "reread" forever, and the plan is discarded. reconcile() re-registers the expectation but never re-emits the event, and re-opening resets state to kickoff-pending without clearing kickoff.state. Give delivery-uncertain a submission path or a reconcile-side promotion.

Nit: safeText (:97) dropped the old z.string().trim() transform, so planning text is now persisted and echoed untrimmed and its semantic digest varies with surrounding whitespace.

Verdict: Request changes — #1 is a regression introduced by this delta and is not covered by the new tests (both session_unreachable cases are single-assignment).

[ynadge]

/review

[ynadge]

Resolved in 7b911afe3a288f244d1b934fe3c96f76272f2d0c.

This append-only follow-up closes the confirmed projection and lifecycle findings:

1. Replacement now tombstones every local projection of the superseded exact context, including secondary tabs, at least at the committed replacement lifecycle epoch. Restart reconciliation applies the same lower-epoch/context fence. The forced durable-old-ack → replacement → stale projection → delayed-old-projection interleaving leaves the old tabs stale and the replacement authoritative.
2. Fan-out still performs planner/auth/consent/source/plan/brief validation and the all-assignment claim atomically. Only post-claim session_unreachable is isolated per assignment. Ordered bindings plus explicit unreachable assignment IDs reach REST, UI, and MCP; summaries distinguish opened/reused from unreachable. Regressions cover unreachable first and middle assignments, all-unreachable, later successful creates, foreign non-mutation, and owner retry.
3. A new planning result is accepted only after normal delivered kickoff or from the exact authenticated primary when both durable binding and kickoff are genuinely delivery-uncertain after an attempted delivery. The latter atomically stores submission, receipt, and terminal submitted state without fabricating a delivered acknowledgement. Pending/delivering and secondary callers remain denied; new IDs after terminal remain denied; exact replay remains ahead of mutable checks.
4. Kickoff acknowledgement and result submission are monotonic in both forced orders. If acknowledgement wins, submission follows normal delivered eligibility. If submission wins, the acknowledgement loses its CAS, rereads the terminal winner, clears local attribution, and no-ops without revival or an older projection. Startup reconciliation never repopulates terminal attribution.
5. Compatible fan-out, trusted same-ID resume, and restart preserve genuine delivery uncertainty. They cannot downgrade it to a mismatched kickoff-pending lifecycle.
6. Planning-result text is trimmed before minimum/maximum/safety validation, digesting, and persistence. Padding variants replay with one digest; trimmed length boundaries pass consistently; C0, DEL, and lone surrogates fail at the request boundary with no mutation.

Exact final-tree evidence:

• focused lifecycle, SessionManager, MCP, REST, and codec suite: 5 files / 216 tests passed
• full harness unit suite: 218 files / 3,496 tests passed
• full performance suite: 3 files / 10 tests passed
• Playwright session-tabs and Agent Map planning specs: 24/24 passed
• typecheck, lint, production build, changed-file Prettier, privacy scan, and diff whitespace checks passed

The branch remains stacked on #786 and this PR remains open and unmerged.

[github-actions]

Follow-up review — delta since bd910a81

Delta is one commit, 7b911afe. Confidentiality clean — new prose is code comments, one JSDoc line, two toast/tool strings; all generic. No changeset changes needed: the only public-surface change (PlanningFanoutOpenResponse.unreachableAssignmentIds) is a field on a type introduced by this same PR, so the existing minor changeset still covers it.

Round 4: both findings and the nit are addressed.

• cleanup HTTP SDKs #1 (fan-out wedged by one unreachable binding) — openOrReuseOnce now catches session_unreachable per assignment, returns the durable binding via readCompatibleBinding(..., {allowStale: true}), and reports unreachableAssignmentIds through REST, MCP, and the UI toast. Covered at ordered positions 0 and 1, all-unreachable, foreign non-mutation, and owner retry.
• update SDKs to use payment protocol data #2 (delivery-uncertain could never submit) — submitPlanningResult accepts a genuinely uncertain kickoff (state, kickoff.state, attemptCount > 0) from the exact primary only, and stateForReachableSession stops downgrading uncertainty to kickoff-pending on re-fanout/resume/restart. Secondary callers denied; deliverKickoff/onEventPersisted/startup reconcile all guard the resulting terminal state.
• Nit (untrimmed safeText) — .trim() restored ahead of min/max, with a padding-variant single-digest replay test.

No new findings at or above the severity floor. Two nits:

1. web/src/App.tsx:404,410 and server/agent-map-mcp-tools.ts:570 — "Opened or reused 1 planning sessions" / "Reconciled 1 planning sessions." The unreachable count is pluralized but the session count is not, and the singular form is now locked in by agent-map-planning.spec.ts:504.
2. shared/build-plan.ts:609 — the new exported JSDoc says these bindings are "durably owned by another coordinator", but the condition that produces them is a process-local registry miss, which also fires when this coordinator's own sessions.json was pruned or reset. It ships in the .d.ts; describe the observation, not an ownership guarantee.

Verdict: Approve. Round 4 is fully resolved; the two nits are cosmetic.

[ynadge]

Follow-up to the hosted review comment, resolved in eabc9b2a1d99ba15327de35cd83b43f849072bad.

• Agent Studio and the MCP fan-out summary now render 1 planning session and pluralize larger counts independently from the unreachable is/are grammar. The Playwright and MCP assertions lock the singular partial-fan-out text.
• unreachableAssignmentIds documentation now describes only the observable contract: durable bindings this coordinator cannot reach through its process-local registry, including after local pruning or reset.

Verification on the exact committed tree:

• focused MCP/REST unit tests: 2 files / 22 tests passed
• partial-fan-out Playwright case: 1/1 passed
• typecheck and lint passed
• changed-file Prettier, diff whitespace, and privacy checks passed

The PR remains open and unmerged.

[ynadge]

/review

[github-actions]

Follow-up review — delta since 7b911afe

Delta is one commit, eabc9b2a, and it is copy-only: pluralization of the fan-out toast/MCP summary and one JSDoc rewrite. Confidentiality clean — no company names, business arrangements, or internal references in the new prose. No public type/runtime change, so the existing minor changeset still covers it.

Round 5: both nits addressed. App.tsx:404,410 and agent-map-mcp-tools.ts:570 now pluralize the session count independently of the is/are grammar, and PlanningFanoutOpenResponse.unreachableAssignmentIds (shared/build-plan.ts:609) now documents the observable condition (unreachable through this coordinator's process-local registry, including after local pruning or reset) instead of asserting foreign ownership.

No new findings, and nothing from earlier rounds is left unfixed.

Nit: agent-map-mcp-tools.test.ts:87 dropped from three assignments to two, so both remaining assertions (:130, agent-map-planning.spec.ts:505) now exercise only the singular branch — the plural branch of the new ternaries is untested.

Verdict: Approve.

[ynadge]

Follow-up to the hosted review comment, resolved in test-only commit 110306e4615302cefa375d6e3e26d7aeb0452c38.

• The MCP fan-out test retains the singular assertion and now also exercises 3 total / 1 unreachable, asserting Reconciled 2 planning sessions; 1 is locally unreachable.
• The existing normal two-assignment stock-research browser path now asserts Opened or reused 2 planning sessions; the partial path retains its singular assertion.

Verification on the exact committed tree:

• focused MCP test: 8/8 passed
• relevant Agent Map Playwright cases: 2/2 passed
• typecheck passed
• changed-file Prettier, diff whitespace, and privacy checks passed

The PR remains open and unmerged.

[ynadge]

/review

[github-actions]

Follow-up review — delta since eabc9b2a

Delta is one commit, 110306e4, and it is test-only (agent-map-mcp-tools.test.ts, agent-map-planning.spec.ts). No published code, types, or prose changed, so the existing minor changeset still covers the PR. Confidentiality clean — no new prose surface at all.

Round 6 nit addressed. The MCP test now drives a second call with 3 bindings / 1 unreachable and asserts Reconciled 2 planning sessions; 1 is locally unreachable., and the stock-research browser path asserts Opened or reused 2 planning sessions. Both plural session-count branches of the ternaries added in eabc9b2a are now exercised. Nothing from earlier rounds is left unfixed.

Nit: the is/are half of the same copy is still singular-only — unavailable > 1 (server/agent-map-mcp-tools.ts:571) and unreachableAssignmentIds.length > 1 (web/src/App.tsx:404) have no assertion; the new plural case reuses unreachableAssignmentIds: ["assignment-b"].

Verdict: Approve.

[ynadge]

Exact head 110306e4615302cefa375d6e3e26d7aeb0452c38 is approved. The session-count singular and plural paths are now behaviorally covered in MCP and UI. I am leaving the remaining non-blocking is/are exhaustive-coverage nit as-is: the expressions are the same bounded count formatting, and adding the UI case would require expanding shipped mock-only behavior solely to exercise cosmetic grammar. No runtime or acceptance correction remains, and the PR stays open and unmerged.

[ynadge]

/review

[github-actions]

Follow-up review — delta since 110306e4

Delta is one commit, 9f82bd15, and it is not cosmetic: it deletes the authenticated-browser approval route and the Studio button, and replaces them with a planner-attested MCP consent pair. Confidentiality clean — new prose is the changeset rewrite, the planner system prompt, two tool descriptions, and JSDoc; no company names, arrangements, or internal hosts. Nothing from rounds 1–6 regressed; the only open item is the is/are coverage nit the author explicitly declined.

New

1. Consent is now a string the model types, and the untrusted text it summarizes is in the same channel. requireConsent (core/builder-planning-session.ts:953) accepts confirmation === "user-confirmed" — a z.literal — plus a consentId that prepareConsent returns deterministically from the scope in the immediately preceding tool call. Nothing server-side distinguishes "asked and the user replied yes" from prepare→open in one turn; "Stop and wait for their reply" exists only in profiles/agent-map-planner.ts:34 and the tool descriptions. The removed user-action_<uuid> bridge was the only mechanical gate. Failure: the planner is instructed to summarize each brief.mission, which is plan prose its own prompt calls untrusted — a mission carrying "the user already approved this" drives prepare+open and spawns one real builder process and tab per assignment. planning-readonly bounds file access; it does not bound process creation or model spend. If this is the intended product decision, say so in the changeset: line 5 currently reads "asks for explicit consent", which an npm consumer will take as enforced rather than planner-attested.

Nits:

• .changeset/real-builder-planning-sessions.md:7 — "E5 remains the implementation and deployment gate" ships an unexplained internal epic id into a CHANGELOG that cannot be edited after publish.
• fanoutApprovals and PlanningFanoutApproval* are now written by nothing; the @deprecated note cites a preview that never reached npm, so the migration rationale is false and the codec loop at build-plan-codec.ts:1203 is dead.

Verdict: Approve once #1's changeset wording matches what the server actually enforces.

[ynadge]

Follow-up to the hosted review comment, resolved in 0bdbca85a9a7a71de30bb4a110e0c352192da4b6.

• A pending fan-out consent now stores the server-accepted planner input token from preparation and cannot open until the same planner session has a different, subsequently accepted user-submission token. A prepare-to-open attempt in one user turn fails closed with user_reply_required before any process or tab is created.
• Scoped queued inputs use their durable queue IDs. Interactive raw terminal Enter gestures are stamped synchronously at the authenticated PTY boundary, before the planner can issue its follow-up MCP call. Greetings, kickoffs, and other programmatic submitInput() calls do not count.
• Both turn tokens are covered by the consent digest and persisted through restart. Consent records from prerelease builds that lack turn evidence are discarded, forcing a fresh summary and consent request.
• The planner must still attest that the later reply is affirmative. The changeset now states this semantic boundary explicitly instead of implying that Studio interprets the reply text.
• Ordinary/unplanned sessions are rejected by the planner-state recorder before an ID or state file is created, preserving the manual-session isolation contract.
• The changeset no longer exposes the internal E5 label, and the deprecated approval field now accurately refers to earlier prerelease development state.

Verification on the exact committed tree:

• focused consent, lifecycle, MCP, terminal, and codec suite: 6 files / 242 tests passed
• full harness suite: 218 files / 3,507 unit/integration tests and 3 files / 10 performance tests passed
• Playwright session-tabs and Agent Map planning specs: 24/24 passed
• lint, core/web/public-API typecheck, dependency-aware production build, and git diff --check passed

The PR remains open and unmerged.

[ynadge]

/review

[github-actions]

Follow-up review — delta since 9f82bd15

Delta is one commit, 0bdbca85: a durable planner-turn token (preparedFromUserInputId / confirmedByUserInputId) now gates openOrReuse, raw PTY Enter is stamped at SessionManager.write, and prerelease consents without turn evidence are dropped by the codec. Confidentiality clean — the changeset rewrite, prompt, and JSDoc name no companies, arrangements, or internal hosts.

Round 8 status

• cleanup HTTP SDKs #1 (prepare→open in one turn was unenforced) — mechanism added, but the claim it now makes is still wider than what is enforced; see New cleanup HTTP SDKs #1.
• Nit (E5 in the changeset) — removed from .changeset/, but the identical internal epic label still ships in profiles/agent-map-planner.ts:45 ("E5 remains the separate gate"), which is published in the tarball and is text the planner can repeat to a user. Same root cause, half fixed.
• Nit (fanoutApprovals @deprecated wording) — fixed.

New

1. The gate counts any CR on the planner PTY, not a user reply to the consent question. observeTrustedTerminalInput (core/session-manager.ts:1621) increments submissionCount for every \r outside a bracketed paste — empty composer, /resume picker selection, or Enter on a Claude Code permission dialog (the planner runs --permission-mode auto, not planning-readonly). Failure: the planner calls prepare, triggers any tool call that raises a permission prompt, the user presses ↓/Enter to allow it, the planner calls open in the same assistant turn — the token differs, so builder-planning-session.ts:1002 passes and one real builder process and tab spawn per assignment. Same root cause via the queue path: a message enqueued by the user before preparation but dequeued after it also advances lastAcceptedUserInputId (planner-greeting.ts:580), so an unrelated "and also add caching" typed while the planner was busy satisfies consent for a summary the user never saw. .changeset/real-builder-planning-sessions.md:5 sells this as "Studio mechanically requires a different, subsequently accepted user-submission token" — on the one surface that cannot be edited after publish, that reads as enforcement of a reply. Either filter to submissions that actually delivered composer text after preparedAt, or say the mechanical part only proves another keystroke-level turn occurred.

Nit: acceptedPlannerUserInputId (builder-planning-session.ts:716) throws user_reply_required for a missing/unreadable token during prepare too, so a fresh planner whose state file is not yet written reports MCP recovery wait_for_user_reply for a condition no user reply fixes.

Verdict: Approve once the changeset sentence matches what the token actually proves.

[ynadge]

Follow-up to the hosted review comment, resolved in b6371bbd059536b66791fe8fd3193a878a9fa212.

• The gate now records both the accepted-input ID and the time that input entered Studio. A pending consent requires an accepted receipt strictly after preparedAt; a message queued before preparation retains its original boundary time and cannot qualify when delayed PTY delivery eventually completes.
• Raw terminal tracking now requires a non-empty, non-command line. Empty Enter, arrow/navigation plus Enter, and /resume do not advance consent; ordinary typed or bracketed-paste replies do. Programmatic submitInput() remains excluded.
• The accepted input time and IDs are covered by the consent digest, validated by the strict codec, and persisted through restart. The prior prerelease token-only consent shape is discarded and must be prepared again.
• Preparation is now side-effect-free even when a fresh planner has no earlier input receipt. Opening still fails closed with user_reply_required until a qualifying post-preparation receipt exists.
• The changeset now says exactly what is enforced: Studio proves a non-empty post-preparation input boundary but does not interpret natural-language affirmation, which remains planner-attested.
• The remaining published E5 wording was replaced with the product-level “separate execution gate” description.
• Whitespace-only scoped planner messages are rejected at the authenticated request boundary and cannot create a consent receipt.

Verification on the exact committed tree:

• focused consent, lifecycle, MCP, REST, terminal, and codec suite: 7 files / 257 tests passed
• full harness suite: 218 files / 3,508 unit/integration tests and 3 files / 10 performance tests passed
• Playwright session-tabs and Agent Map planning specs: 24/24 passed
• lint, core/web/public-API typecheck, dependency-aware production build, and git diff --check passed

The PR remains open and unmerged.

[ynadge]

/review

[ynadge]

/review

[github-actions]

Follow-up review — delta since 0bdbca85

Delta is one commit, b6371bbd: the consent gate now requires a non-empty, non-slash raw line accepted strictly after preparedAt, queued inputs keep their enqueue time, and prepare no longer fails on a missing receipt. Confidentiality clean — the changeset rewrite, prompt, and JSDoc name no companies or internal hosts; the last E5 occurrence in profiles/agent-map-planner.ts is gone and asserted absent.

Round 9 status

• cleanup HTTP SDKs #1 (gate counted any CR; changeset overclaimed) — fixed. session-manager.ts:1634 counts only a non-empty, non-/ line, so empty Enter, arrow+Enter, and /resume no longer qualify (covered at session-manager.test.ts:217-223); the queue path retains input.acceptedAt; .changeset/real-builder-planning-sessions.md:5 now says Studio proves a post-preparation input boundary and does not interpret the reply, which matches the code.
• Nit (prepare threw user_reply_required) — fixed; latestAcceptedPlannerUserInput returns null and only openOrReuse fails closed.

New

1. The consent watermark can move backwards and discard a real reply. planner-greeting.ts:1414 (drain) and :611 (reconcile) overwrite lastAcceptedUserInputAt with the dequeued input's original enqueue time, unconditionally. Failure: user queues a message in Studio at T1, the planner prepares at T2, the user types a reply in the planner terminal at T3 (recordRawUserSubmission → watermark T3), then the T1 message finally drains at T4 and resets the watermark to T1. builder-planning-session.ts:1002 then sees T1 <= T2 and returns user_reply_required for a consent the user did answer. Fail-closed, but a false rejection with no test. Advance only when the new acceptedAt is later than the stored one.

Nit: session-manager.ts:1623 — inside a bracketed paste, \r/\n appends a space to trustedSubmissionLine with no TRUSTED_INPUT_LINE_MAX guard, unlike every other append site (:1627, :1664, :1695). A large multi-line paste grows the buffer without bound until the next CR.

Verdict: Approve; #1 is a liveness defect worth a follow-up, not a blocker.

[ynadge]

Follow-up to the approved hosted review, with both non-blocking findings resolved in fd3bea021028c00e3b74ced6e84ed2a53d6a169d.

• Accepted-input receipts now advance through one monotonic selector in normal dequeue, crash-ledger reconciliation, and raw-terminal recording. A delayed T1 queued message can no longer replace a T3 raw reply when it drains at T4.
• The regression forces exactly that ordering and verifies the newer raw receipt remains authoritative after the old queue item is delivered.
• Bracketed-paste CR/LF separators are no longer accumulated because the gate needs only bounded non-whitespace content detection. Every retained character still observes TRUSTED_INPUT_LINE_MAX.
• Persisted queued-input timestamps are now validated before planner state is accepted.

Verification on the exact committed tree:

• focused consent/lifecycle/MCP/REST/terminal/codec suite: 7 files / 258 tests passed
• full harness suite: 218 files / 3,509 unit/integration tests and 3 files / 10 performance tests passed
• Playwright session-tabs and Agent Map planning specs: 24/24 passed
• lint, core/web/public-API typecheck, dependency-aware production build, and git diff --check passed

The PR remains open and unmerged.

[ynadge]

/review

[github-actions]

Follow-up review — delta since b6371bbd

Delta is one commit, fd3bea02, touching planner-greeting.ts, its test, and session-manager.ts. Confidentiality clean — no changeset, README, or JSDoc prose changed; the only prose is one existing code comment. No public type or runtime-surface change, so the existing minor changeset still covers the PR.

Round 10 status

• cleanup HTTP SDKs #1 (watermark could move backwards and discard a real reply) — fixed. All three writers (reconcileAcceptedInputs:640, drain :1446, recordRawUserSubmission:1513) now go through the single newestAcceptedUserInput selector, which seeds from the persisted pair and only advances on a strictly later Date.parse(acceptedAt). planner-greeting.test.ts:188 forces exactly the T1-queue / T3-raw / T4-drain ordering and asserts the T3 receipt survives. The ?? null collapse cannot clear a live watermark: the seed is non-null whenever both persisted fields are set, and the codec (:268, :499) already normalizes a half-set pair to null. lastAcceptedUserInputAt is validated parseable at :186, so a bogus persisted timestamp can't wedge the comparison at NaN.
• Nit (unbounded trustedSubmissionLine growth inside bracketed paste) — fixed. CR/LF inside a paste is now dropped rather than appended, so every retained character passes TRUSTED_INPUT_LINE_MAX. Behaviorally equivalent for the gate: the only predicates on the line are non-empty and startsWith("/"), and dropping the separator can neither create nor remove a leading slash.

No new findings at or above the severity floor. Nothing from rounds 1–10 is left unfixed.

Nits:

1. planner-greeting.ts:623 — the crash-ledger reconcileAcceptedInputs branch of the new selector has no test; the regression only exercises the drain and raw paths, so restart-time monotonicity is asserted nowhere.
2. session-manager.ts:1620 — the comment still says CR inside a paste "is content", but the code it sits on now discards it.

Verdict: Approve.

[ynadge]

Follow-up to the approved hosted review, with both remaining nits resolved in test/documentation-only commit e0ca5cc9fefe0b03893e1d6bfe95d61d07bc0210.

• The accepted-ledger crash-recovery test now persists a newer T3 raw receipt beside an older T1 accepted queue entry, restarts the coordinator, drives reconcileAcceptedInputs, and asserts the T3 receipt remains authoritative after ledger repair.
• The bracketed-paste comment now accurately says CR/LF separators are discarded so they neither submit nor consume the bounded content buffer.

Exact-head verification for this delta: planner-greeting + SessionManager, 2 files / 160 tests passed; lint, core/web/public-API typecheck, and git diff --check passed. The immediately preceding production-identical tree also passed the full 218-file / 3,509-test harness suite, all 10 performance tests, the dependency-aware production build, and all 24 targeted Playwright scenarios.

The PR remains open and unmerged.

[ynadge]

Closing as superseded by SAP-3147 and the verified role-neutral replacement stack culminating in PR #811 (#804, #806, #807, #808, #811). Useful implementation evidence was preserved before closure. This PR remains unmerged; its branch and review history are retained.