feat(acp): expose observable Goal and Plan execution

[Sun-GLiang]

Summary

• Expose Runtime Host Goal and Plan operations through six typed private ACP requests, with Session ownership checks and Host validation/error shapes.
• Retain one Session attachment for Goal status and Plan change notifications, including external changes and canonical subscription replacement. Reuse the existing non-prompt Turn observer for Plan approval and execution resume, so standard output, interactions, cancellation, and terminal status stay ordered.
• Preserve caller identities and never replay a dispatched command whose result is unknown. Document the extension, lifecycle behavior, and validation evidence.

Refs #3132

Verification

• Root build and workspace typecheck passed. ACP-focused tests: 299 passed. Full CLI test:dist: 1266 passed, 3 skipped, 0 failed. Full Runtime Host test:dist: 2124 passed, 12 skipped, 0 failed.
• Official ACP SDK tests run a real stdio child against a real Runtime Host and local controlled model. They cover Goal continuation, Plan submission/approval/execution/resume, ACP elicitation, paging/conflicts, external Host changes, duplicate starts, cancellation, close, and EOF. Focused tests cover lost dispatched responses and stale domain refreshes.
• Lint, format, ASF headers, CLI third-party notices, Desktop/UI knip, git diff --check, and the protocol epoch guard passed. No Host wire schema or SDK version changed.
• Desktop Electron E2E, Zed smoke, Windows/Linux CI, and external model services were not run locally. Detailed evidence is in packages/cli/src/acp/VALIDATION.md.

AI use

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Codex authored the implementation, tests, and documentation. The commit includes a Generated-by: Codex trailer.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[Astro-Han]

Reviewed the current revision f75d9967ec14c186ec7768e119057482479bc18f (15 files, +2711/−10). It exposes the Runtime Host's existing Goal and Plan operations over ACP as six _maka/... requests plus two opt-in notifications, and adds a per-attachment observer that keeps a client's view of both current.

One P2 and one P3 below. Everything else I checked came out clean, and I list what I verified so the next reader knows which ground is already covered.

What the change actually does

• packages/cli/src/acp/goal-plan-routes.ts (new): goalPlanRouteInput replaces the input decoder that the four existing _maka routes each wrote out inline — same HOST_OPERATION_SPECS[...].decodeInput call, same invalidParams mapping (maka-acp-agent.ts:152-199 is the pattern being replaced).
• packages/cli/src/acp/maka-acp-agent.ts: six onRequest registrations (lines 107-151), the _meta["_maka/goalPlan"] = {version: 1} advertisement (line 68), and notification hooks gated on the client's _meta["_maka/goalPlanStatus"] (lines 252-263), following the existing _maka/turnStatus gate (line 245).
• packages/cli/src/acp/goal-plan-operations.ts (new): the use-case layer. Each call is prepare → assertCurrent → request → exactly one of commit/rollback, and a lost-but-dispatched result becomes outcome_unknown (never resent) while persistence_failed keeps the Host code and the observation alive.
• packages/cli/src/acp/session-domain-observation.ts (new): per-attachment goal/plan notification state with epoch fencing across a canonical replacement, coalescing, and bounded retry for the plan path.
• packages/cli/src/acp/session-registry.ts: the wiring, the shared #prepareExternalObservation factored out of turn.resume.start (lines 396-419), the admission refcount in #pendingPlanAdmissions, and disposal on detach and on close.
• packages/cli/src/runtime-host-session-channel.ts: one new hook, onCanonicalReplacement (option at line 98, field at 126/179, fired at 740-741), on the path that already reports a canonical replacement.

P2 — a Goal status notification can be lost permanently. See the inline comment on session-domain-observation.ts:85-87 for the mechanics and two ways to reach it. In short: the dedup key is compared against the last delivered status while #sendGoal clears the pending slot at send time and only records delivery on success (lines 177, 180), and a rejected notify is logged and dropped without retry (182) even though the plan path retries. The result is a client left holding the older Goal until the Goal changes again, which the module's own contract forbids — the design states that a domain notification is the latest authoritative state and must not be rolled back by a late result (docs/architecture/acp-pr8-goal-plan-design.zh-CN.md:120), and the README calls these "latest-state hints".

P3 — no test covers a failed notification delivery. packages/cli/src/__tests__/acp-session-domain-observation.test.ts covers plan coalescing, stale-read rejection, the canonical-replacement re-read with an unchanged Goal, retry after a failed plan refresh, and disposal fencing — but nothing where a notify rejects, and nothing where a value returns to the last delivered one. The design's own verification matrix lists 通知失败 as a required case (docs/architecture/acp-pr8-goal-plan-design.zh-CN.md:216). This is the same ground as the P2, which is presumably why it went unnoticed.

What I checked and found correct

• The six operations exist in the Host specs, and the type system ties the ACP names to them: GoalPlanOperationName indexes HOST_OPERATION_SPECS, so a name without a spec cannot compile. Inputs and results are forwarded verbatim, so the caller's goalId/revision/operationId/turnId identity is preserved — no re-generation and no JSON-RPC request id standing in for a command id.
• This PR does not touch the Host protocol at all (only packages/cli and one doc), so it is an adapter over operations that already exist on main; the compatibility epoch is untouched.
• No second authority is introduced: Goal state still arrives through the channel's projection diff (runtime-host-session-channel.ts:730-731, 906-910) and Plan state through plan.query. The new module only decides when to tell the client.
• turn.resume.start keeps its behaviour. The shared helper can now return terminalReplay with no observation while the resume route throws registryClosedError when there is no observation — but the resume route passes a freshly generated turnId (session-registry.ts:1491), so that shortcut can never match its root Turn and the guard is reached in exactly the case it was before.
• Admission lifetime holds: finishAdmission counts outstanding preparations per observation and disposes only when the last one rolls back and none committed, so a concurrent committed call keeps it alive. The open flow's failure path disposes the observer (1214-1217) and #detachAttachment disposes the current one (1271-1272); the ordering between #domainObservations.set and #attachments.set is synchronous, so no detach can run in between.
• The plan path does not share the Goal problem: storeVersion is monotonic and every invalidation re-reads the current version, so a dropped or failed delivery heals on the next change.
• The README's page claim ("at most 16 items") matches PLAN_PAGE_MAX_ITEMS = 16.

What I could not judge

• I did not run the ACP child-process suite locally. test is green on this head (run 36213166683); the log shows the planner selecting all 15 files, packages/cli running, and zero failures.
• A real client (Zed or another ACP client) with these notifications was not exercised. The child-process tests drive the official SDK server over stdio, which is the closest evidence available here.
• Desktop Electron E2E, platform CI and the remaining race coverage the design lists are outside what I ran.

I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

This comparison is against the last delivered Goal, but #sendGoal clears the pending slot at send time (line 177) and only records the value as delivered after the notify resolves (line 180). A Goal that arrives while a different one is still in flight is therefore compared against a value the client already has, is dropped, and is never re-sent — because the follow-up at line 185 finds nothing pending. The client then keeps the in-flight status, which is the older state.

Two ways to reach it:

• Goal A delivered, Goal B sent and still awaiting the client, then the authoritative Goal returns to A — cleared and re-armed to the same projection, or any value that equals the last delivered one. key === this.#goalDelivered, so the update is dropped; when B's delivery completes the pending slot is empty and nothing follows. The client holds B while the Host holds A.
• The notify rejects: line 182 logs and drops it, where the plan path retries with backoff. Nothing re-reads the Goal either, so the client keeps whatever it had.

The plan path is immune because storeVersion is monotonic and each invalidation re-reads the current value; a Goal notification has no such re-read, so it never heals on its own — only the next Goal change or a canonical replacement fixes the client.

This contradicts what the module's design promises: a domain notification is the latest authoritative state and must not be regressed by a late result (docs/architecture/acp-pr8-goal-plan-design.zh-CN.md:120), and the README calls these "latest-state hints". Hitting it needs a Goal change or a delivery failure to overlap one notify round trip, and a client can still recover by calling _maka/goal/query — but the state this module exists to push is then wrong without saying so.

Comparing against the last attempted status, or re-sending when the pending value equals the last delivered one, closes both; a bounded retry would bring the Goal path in line with the plan path.

[Sun-GLiang]

Fixed the in-flight deduplication bug in fbfe620. goalChanged always replaces the pending snapshot, including a return to the last delivered value; deduplication now happens in #sendGoal after the in-flight guard and compares against the last successfully delivered snapshot. This preserves the final A in A → B (in flight) → A while still suppressing an idle duplicate. Comparing against the last attempted value would incorrectly suppress a later repeat after a failed send, so delivery success remains the deduplication boundary.

The same commit adds regression coverage for a return to null during delivery, coalescing newer revisions, duplicate changes during delivery, canonical replacement, and disposal after both successful and rejected notification sends. This addresses the missing failed-notify path from the P3 in the review body; the rejection test specifically checks disposal fencing, rather than automatic retry.

For the separate delivery-failure concern, 23623a7 makes the contract explicit in the README and design: both Goal and Plan domain notifications are best-effort, log rejected sends, and do not automatically retry them. Clients recover authoritative state through the corresponding query. The Plan backoff retries Host reads in #refreshPlan, not notification sends in #sendPlan, so Plan delivery is not immune to this failure either. The latest review's documentation option is now implemented.

All 10 domain-observation tests passed on the rebuilt current revision.

[Astro-Han]

Reviewed eb66d15601045c78e184c841a41c819cb57d6b6e (3 commits; the two new ones are fixes, fbfe620f and eb66d156). My earlier comment on f75d9967 is superseded by this one.

Both items I raised are fixed — and the second new commit fixes a real bug that my first pass missed. One small question is left open at the end.

Goal status notification (the P2 I raised): fixed.

goalChanged now always replaces the pending snapshot (session-domain-observation.ts:83-86), and #sendGoal evaluates the dedup against the last delivered value at the moment it actually sends, after clearing the pending slot (175-177). I re-ran the traces that failed before:

• Goal A delivered, then B sent and still awaiting the client, then the authoritative Goal returns to A: the pending slot keeps A while B is in flight (the single-flight guard at line 170 returns before touching it), and when B completes, the follow-up at line 185 sends A. The client no longer ends on the older state.
• A repeated identical value with nothing in flight is still suppressed (176-177), and because delivery is single-flight, notifications cannot be reordered.

The new tests pin exactly this ground: latest null Goal survives an in-flight notification (both variants), Goal delivery coalesces newer revisions and deduplicates only after delivery, duplicate Goal changes during delivery do not schedule a duplicate notification, and a late Goal delivery cannot suppress the same snapshot in a new canonical epoch.

Notification delivery failure (the P3 I raised): mostly closed, one small question left.

The failing path is now exercised (dispose fences a pending Goal after delivery failure), but what that test pins is fencing, not the consequence. A rejected notify is still logged and dropped without retry (line 182), where the plan path retries with backoff — so a transient failure leaves the client on the previous Goal until the next change (a later repeat of the same value is re-sent, because the failed one was never recorded as delivered). If dropping is deliberate — the README frames these as hints and _maka/goal/query always has the truth — one sentence in the design or README would settle it; otherwise the same bounded retry the plan path already uses is available. I would not hold a merge for this.

plan.turn.start replay after restore — the bug my first review missed.

At the head I reviewed, the shared preparation rejected any non-admitted observation (if (!(observation instanceof AcpAdmittedTurnObservation)) throw registryClosedError(operation)), and the Plan route is the one whose turnId comes from the client. So replaying plan.turn.start for a live Turn already observed after session/load or session/resume failed with registry_closed — which the validation entry records for both regression cases. I traced that branch for the turn.resume.start route only, where the route generates its own turnId and the branch is unreachable, and did not follow it into the Plan route. That was my miss, and its effect is a client that cannot replay after a restore.

The fix is right and minimal: return observedReplay and let the Host validate the replay (session-registry.ts:418-422), sharing that flag with the terminal-root shortcut (414-415) so both "the Host already observes this Turn" cases behave alike. The restored observer's lifecycle stays untouched, which is what a sole-consumer rule requires — with no observation in the prepared result, commit/rollback cannot fail or dispose it, and planTurnStart skipping markDispatched/settleStartRequest and having no reconcileAdmission is correct because there is no local admission to reconcile (goal-plan-operations.ts:112).

turn.resume.start is unaffected: it generates its own turnId, so the non-admitted branch remains unreachable there and its registryClosedError guard still fires only when #adoptTurn returns nothing.

Re-checked on this head, so the delta did not invalidate the earlier pass: the change set is still packages/cli plus one doc, so no Host protocol or compatibility-epoch change; the six operations still exist in the Host specs with the type binding; no second state authority; goalPlanRouteInput still mirrors the existing inline decode and invalidParams pattern; the README's page claim still matches PLAN_PAGE_MAX_ITEMS; the admission refcount and disposal paths are unchanged.

Checks: test is green on this head (run 36215292965; the planner selected packages/cli and every test passed, zero failures).

What I could not judge

• I did not run the child-process suite locally; the two new regression cases the validation entry describes are covered by CI only, as far as I verified.
• No real ACP client was exercised, and no Desktop or platform CI ran.

I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

This now closes the P2 I raised: the dedup runs at send time, after the pending slot is cleared, and it compares against the last value the client actually received. A Goal that returns to that value while a different one is in flight stays in the pending slot (the single-flight guard at line 170 returns before touching it) and is delivered when the in-flight one completes. A repeated identical value with nothing in flight is still suppressed, and single-flight delivery keeps the order.

One thing is still worth a decision: a rejected notify is logged and dropped here with no retry, where the plan path retries with bounded backoff. A transient failure therefore leaves the client on the previous Goal until the next change (a later repeat of the same value is re-sent, since the failed one was never recorded as delivered). If dropping is deliberate — the README calls these hints and _maka/goal/query always has the truth — a sentence in the design or README would settle it; otherwise the same bounded retry is available. I would not hold a merge for it.

[Sun-GLiang]

Agreed that the delivery-failure contract should be explicit. Addressed in 23623a7: the ACP README and design now state that both Goal and Plan domain notifications are best-effort. A rejected send is logged to stderr and is not automatically retried; without a subsequent domain update or canonical replacement, the client may retain an older view and should use _maka/goal/query or _maka/plan/query to recover authoritative state.

One correction to the comparison: the bounded backoff in #refreshPlan retries failed Host plan.query reads. #sendPlan catches and logs notification failures just like #sendGoal; it does not retry delivery either. So this is a shared notification contract, rather than a missing Goal counterpart to a Plan delivery retry. Keeping the existing best-effort hint semantics and documenting the recovery path is appropriate here.

Validation: rebuilt the workspace; all 10 domain-observation tests passed. Root lint, format check, build, workspace typecheck, Desktop/UI knip, and git diff --check passed. This follow-up changes documentation only; no runtime behavior changed.

[Sun-GLiang]

Follow-up to the initial review and the updated review:

• P2, Goal notification ordering: fixed in fbfe620 by retaining the latest pending snapshot and deduplicating only when sending, against the last successful delivery. The original inline thread now includes the fix and regression coverage.
• P3, rejected-notify coverage: fbfe620 adds the missing rejection path and disposal-fencing coverage, alongside the in-flight and canonical-replacement cases. As the updated review correctly notes, that test establishes fencing, not a delivery guarantee.
• Remaining delivery-failure decision: documented in 23623a7. Both domain notifications are best-effort; a failed send is logged without automatic retry, and clients use _maka/goal/query or _maka/plan/query to restore authoritative state. One correction to both reviews: Plan's bounded backoff applies to failed Host queries, not notification delivery. #sendPlan and #sendGoal have the same failure policy.
• Plan replay after load/resume: eb66d15 preserves the restored sole observer via observedReplay and leaves replay validation to the Host. Regression cases cover replay after both load and resume, rejection without disposing that observer, continued output, and one terminal notification. The existing Turn-resume route still generates its own Turn ID.

The other checks reported in the reviews require no additional change. The follow-up leaves Host protocol/compatibility epoch, ownership, admission bookkeeping, paging, and command replay rules unchanged.

For the documentation follow-up, root build, workspace typecheck, lint, format check, Desktop/UI knip, git diff --check, and all 10 domain-observation tests passed locally. No new real-client, child-process, Desktop E2E, or cross-platform run is claimed by this follow-up; the earlier validation remains recorded in packages/cli/src/acp/VALIDATION.md.

[jackwener]

[kabi-grok-reviewer]

I reviewed 23623a7226d403caa39257b2955dc082b587c727.

Design. Goal and Plan already exist on the Runtime Host. ACP clients (Zed and others) had no typed way to query, arm, control, or watch them. #3132 is the ACP adapter; this is the missing slice, not a new product. Six _maka/* requests map 1:1 onto existing HOST_OPERATION_SPECS (goal.query / arm / control, plan.query / control / turn.start). Notifications are opt-in (_maka/goalPlanStatus), same shape as _maka/turnStatus. That is a minimal extension surface: no new Host operations, no epoch bump.

Protocol. packages/runtime-host/src/protocol/index.ts is not in the diff. The agent advertises _meta["_maka/goalPlan"]: {version: 1} on initialize; clients that ignore it keep current behavior.

#5621. Restored Turns stay AcpTurnObservation (no local admission). plan.turn.start with a client turnId that load/resume already observes returns observedReplay and lets the Host validate, instead of registry_closed. That matches restore: do not invent a second observer and do not fail the Host Turn. #prepareExternalObservation at 414–422.

Earlier findings (AstroHan).

• P2 Goal in-flight dedup: closed. goalChanged always replaces #pendingGoal; #sendGoal compares to last delivered after clearing the pending slot, then follows up. A → B(in flight) → A keeps A pending.
• P3 notify drop without retry: documented, not merge-blocking. README 108–119: best-effort; recover with _maka/goal/query / _maka/plan/query. Commit 23623a722.

P0–P2 from this review: none.

Checked this round: maka-acp-agent.ts 61–151, 245–259; README.md 63–119; session-domain-observation.ts 73–186; session-registry.ts 400–424; git diff origin/main...HEAD -- packages/runtime-host/src/protocol/index.ts empty; AstroHan reviews 5324337653 / 5324700973 and author replies 4110295152 / 4110297838; git merge-tree --write-tree origin/main 23623a722 exit 0, tree 67d31c1a9.

Not checked: child-process suite, Host Goal/Plan internals (not in this diff), cancellation/replay races (sol's lane), tests run this round.

简体中文

我审查了 23623a7226d403caa39257b2955dc082b587c727。Goal/Plan 过 ACP 站得住：Host 已有能力，六条 _maka 一一对应，不改 epoch。#5621 的 observedReplay 一致。AstroHan 的 P2 已关。本轮没有新的 P0–P2。

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

[jackwener]

[kabi-sol] State/lifecycle review of 23623a7226d403caa39257b2955dc082b587c727: no new P0–P2 findings in the exercised scope.

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Earlier findings:

• Goal in-flight deduplication: fixed. An independent controlled-delivery probe sends null → active (held) → null through the production domain observer. The original f75d9967 implementation delivers [null, active]; this head delivers [null, active, null]. The current tests also pass for a queued clear, increasing revisions, duplicates, canonical replacement and disposal.
• Failed notification delivery: the documented best-effort contract closes the clarification request, rather than adding delivery retries. The README/design now explicitly describe dropped failed sends and query-based recovery for both Goal and Plan. My additional probe confirms a rejected Goal send logs once, the next identical domain update is attempted again, and a subsequent successfully delivered duplicate is suppressed. Plan backoff retries Host reads, not client notifications.
• Live Plan replay after load/resume: fixed in the exercised process paths. Both current regression cases run successfully through the ACP child process and real in-process Host: replay preserves the retained consumer; a rejected replay does not kill it; later text is delivered once; cancellation reports one terminal notification.

Validation on a fresh dependency installation and rebuilt workspace dependencies, ACP plugins and CLI: 163 tests passed, zero failed or skipped across the domain observer, Goal/Plan operations, Session registry and Goal/Plan child-process suites. The seven real-Host cases include passive Goal arm and subsequent continuation, Plan admission/execution and receipt replay, paging invalidation, external Goal control, exact-Turn cancellation, and close/EOF cleanup. The registry suite additionally exercises canonical replacement, terminal delivery and restore ordering. These use a controlled local model server; they do not contact a production model service.

Limits: no real editor ACP client, whole-Host process restart, remote/SSH, Windows, Electron or full-repository checks. I did not exhaust all concurrent cancel/prepare/delivery schedules or independently cover the other reviewers' design and permission lanes. The live head remained unchanged and its test check was completed/successful immediately before publication. COMMENT only; no approval or production changes.

[jackwener]

[kabi-opus-dev] Implementation and tests. Bound to 23623a7226d403caa39257b2955dc082b587c727; re-checked against GitHub immediately before posting and the head had not moved.

Conclusion: no P0-P2, one P3. I set out to answer one question by experiment rather than by reading — do these tests actually constrain the implementation? — and for the two behaviours I ablated, they do.

Ablation (the "would it fail against the old behaviour" question)

I mutated production code, rebuilt, re-ran, and restored. Both arms verified on this head.

1. Ownership guards — constrained. I replaced all three #assertOwned(sessionId) calls on the Goal/Plan path (session-registry.ts at the assertCurrent port callback and both sites in #prepareGoalPlan) with no-ops, confirmed the no-op reached dist, and re-ran:

suite	baseline	ownership ablated
acp-goal-plan-operations	3 / 0	3 / 0
acp-session-domain-observation	10 / 0	10 / 0
acp-agent	10 / 0	10 / 0
acp-stdio-server	26 / 0	26 / 0
acp-goal-plan-child-process	7 / 0	6 / 1

The one failure is owns, arms, queries and controls a Goal while observing domain state, which is the test that probes sessionId: 'unowned-session' and requires error.data.reason === 'unknown_session'. With the local guard gone the request reaches the Host, which still rejects it — but as Invalid params, so the assertion catches the change. Two things worth stating: the guard is pinned, and there is defence in depth behind it. The four unit suites do not constrain it at all, which is expected since they drive the operations class through a fake port.

2. The Goal-notification fix from the earlier review — constrained. I reverted it in the narrowest possible way, changing this.#pendingGoal = {...} to ??= in goalChanged so an older pending snapshot survives again (session-domain-observation.ts:85). Result: acp-session-domain-observation 10 / 0 → 8 / 2, failing on latest null Goal survives an in-flight notification (pending clear: true) and Goal delivery coalesces newer revisions and deduplicates only after delivery. Those tests have real force.

Status of the earlier findings

• P2, Goal status notification lost permanently — closed, and pinned. Ablation 2 above is the evidence; reintroducing the bug fails two tests by name.
• The plan.turn.start replay-after-restore bug — closed in code (observedReplay returned from the shared preparation, shared with the terminal-root shortcut). I read the code; I did not construct a restore-then-replay repro, so I am confirming the shape of the fix, not its behaviour.
• P3, no test for a failed notification delivery — the behaviour question is closed, the coverage requirement is not. The only change on this head is the docs commit, which answers the open "drop or retry" question by documenting the drop as deliberate, in both the design doc and the README. That is a clean answer. What it does not do is satisfy the design's own mandatory verification matrix — see the inline comment.

What I verified (each run this round, on this head)

• Base confirmed against GitHub's own file list: main@87fc9f69c, 15 files, +3107/-10. My first local merge-base gave a stale base and a 90-file diff; I discarded it.
• Clean npm ci + npm run build:test — exit 0, no TS errors, after rm -rf dist.
• Baseline suites: acp-goal-plan-operations 3, acp-session-domain-observation 10, acp-agent 10, acp-stdio-server 26, acp-goal-plan-child-process 7 — fail=0, and reproduced after restoring the tree.
• Ownership assertion coverage: every one of the six operations reaches prepare (which asserts open + owned) and then assertCurrent again after the connection is obtained; planTurnStart takes the same path. Read directly in goal-plan-operations.ts and session-registry.ts.
• The two ablations above, including confirming the mutation was present in dist and absent again afterwards.

What I did NOT check

• No real ACP client (Zed or otherwise); only the SDK-driven child process.
• No restore-then-replay repro for the plan.turn.start fix.
• I did not review the design/scope question, the Host protocol and lifecycle, the epoch/compatible-change declarations, or consistency with the ACP restore work in #5621 — other reviewers hold those, and I did not read their results before publishing.
• I ablated two behaviours, not all of them. "These tests have force" is established for ownership and the Goal-pending fix; it is not established for the rest of the suite.
• No Windows or cross-platform run, no Desktop E2E.

Automated review seat. Not an independent human review, and not independent of the other seats posting from this account.

[jackwener]

[P3] This row now specifies a behaviour that no test pins on a live observation.

The new row states the contract precisely: a failed domain-notification send is logged to stderr, not retried, and the client recovers via explicit query. The README says the same. That closes the open question from the previous review in the "document it" direction, which is a legitimate choice.

But the only test that rejects a delivery is dispose fences a pending Goal after delivery failure (acp-session-domain-observation.test.ts:130), and it calls observer.dispose() before delivery.reject(...). So what it pins is dispose-fencing: the assertion goals == [active] holds because the observer was disposed, not because a failure is left un-retried. There is no case where a send rejects on a live observer asserting that (a) no second send is attempted and (b) the failed value was not recorded as delivered, so a later identical value is sent again — the behaviour the previous review described.

This matters more now than before the docs commit, not less. While the behaviour was unspecified, a missing test was a gap; now that it is a written contract in two places, it is a contract that can drift silently. Section 8 (必须覆盖的验证矩阵) also still lists 通知失败 as required under 权限与交互 (line 219), so the document asks for this case itself.

A single test on a live observer — reject one goalNotify, assert exactly one console.error and no further send, then push the same value and assert it is re-sent — would pin the whole contract. I would not hold a merge for it.

Verified by ablation, for contrast: the other half of that earlier review (the Goal pending-snapshot fix) is firmly pinned — reverting it to ??= fails two tests by name. This gap is specific to the delivery-failure path.

[Sun-GLiang]

Agreed. Commit 1f5b008 adds live-observer failure coverage for both Goal and Plan. Each test checks one logged rejection, no automatic resend, resend after a later identical domain update, and deduplication after successful delivery. CLI build, all 12 domain-observer tests, Biome, and git diff --check pass.

[jackwener]

Approving at 1f5b0085f396141f55e17b1018b6fbdd4a19331c at the requester's ask. Three reviews at 23623a72 (state/lifecycle, design/protocol, implementation/tests) found no P0–P2. The only commit since then is test-only: 1f5b0085f test(acp): cover live domain notification failures (+60/−1 in acp-session-domain-observation.test.ts, no production change). It adds the live-observer delivery-failure case that the one P3 asked for. CI test is green on this head.

• Earlier findings closed:
  • Goal final state lost: an independent reproduction ended at active on the old implementation and null here. Reverting the fix (??= in goalChanged) fails the two covering tests.
  • Notification failure: closed under the now-documented best-effort contract, and this head adds the test for it.
• Ownership guards are constrained: no-op'ing the three #assertOwned calls fails the unowned-session child-process e2e, and the Host still rejects the request as a second layer.
• Plan load/resume retries: both real ACP child-process + Host regressions pass, keeping a single consumer, follow-up output and the cancelled terminal state.
• Design: an ACP adapter over existing Host Goal/Plan. The _maka/* methods map 1:1 to Host specs with opt-in notifications, the epoch is unchanged, and it is consistent with #5621's observedReplay.
• Not verified: a real ACP editor client, a full Host process restart, and Windows/Electron.

This is a feature, so whether to merge it remains a maintainer decision.

Automated review by an AI agent, approving at the requester's ask. This is not an independent human review.

[Astro-Han]

Reviewed 1f5b0085f396141f55e17b1018b6fbdd4a19331c. The delta since the revision I reviewed (eb66d156) is three files and 70 lines: the design doc, the ACP README, and one test file. No production file changed, so my production-side conclusions from the comment on eb66d156 still hold as written, and the three reviews already posted at 23623a72 cover that same production code.

No new P0–P2. My earlier P2 stays closed, the P3 I raised is now closed, and I owe one correction to my own earlier comment.

Correction: I was wrong about the Plan path. My previous inline said a rejected Goal notify is dropped "where the plan path retries with bounded backoff". The author's reply is right, and I verified it: #sendPlan catches and logs a rejected notification exactly as #sendGoal does (session-domain-observation.ts:196-204 against 169-187), and the bounded backoff in #refreshPlan (154-161) wraps await this.#queryPlan() (136) — a Host read, not a notification send. There is no delivery-retry asymmetry between the two domains; I mistook the read-path retry for a delivery retry. The inline comment below records the accurate, narrower difference.

The P3 is closed, and closed properly. The documentation now specifies the contract, and it corrects its own row while doing so: the old 状态刷新失败 row became Plan 状态刷新中的 Host 查询失败, with a separate row for notification send failures, and both the design and the README state that the bounded backoff applies to Host reads rather than to notification sends. That matches the code exactly. The new tests pin the contract on a live observer, which is precisely what the earlier coverage lacked — the previous failure test disposed the observer before rejecting the send, so it only pinned fencing. Each new case asserts one logged rejection, no automatic retry (after a 100 ms window), that the failed value was not recorded as delivered (the same value is re-sent on a later update), and that a subsequent successful delivery suppresses duplicates. Those assertions would fail if delivery were retried, or if a rejection were recorded as delivered, so they have real force.

What I verified here

• git diff --name-only eb66d156 1f5b0085 is the design doc, the README and the test file — no production change on this head.
• The new doc sentences match the code I re-read on this head: best-effort delivery for both domains, stderr logging, no automatic resend, explicit query for recovery, and the backoff limited to Host reads.
• The two new tests cover Goal and Plan symmetrically, and the Plan case re-sends through a fresh read rather than a send retry, which is what the contract now says.

What already exists at this revision, so it is not mistaken for mine: three COMMENTED reviews at 23623a72 (design/protocol, state/lifecycle, implementation/tests), an inline P3 about the missing live-observer failure case — which these commits answer — and an APPROVED at 1f5b0085 whose own notice states it is an automated approval placed at the requester's ask, not an independent human review.

What I could not judge: unchanged from my earlier comments — no real ACP editor client, no full Host process restart, no Windows or Electron checks, and I did not re-run the child-process suite against this head.

I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

Correction to my earlier comment on the Goal path, and the accurate version of the difference I was trying to describe.

I wrote that a rejected Goal notify is dropped "where the plan path retries with bounded backoff". That was wrong: #sendPlan catches and logs a rejected notification exactly as #sendGoal does, and the bounded backoff in #refreshPlan wraps await this.#queryPlan() — a Host read, not a send. I read the read-path retry as a delivery retry; there is no delivery-retry asymmetry between the two domains.

What does hold, and is narrower: after a failed or dropped Plan notification, the next invalidation re-reads the current version and sends it again, because a failure never records #planDelivered (line 198 only runs on success) — so a Plan notification can heal without any state change. A Goal notification has no equivalent re-derivation until the Goal itself changes or a canonical replacement arrives. The new doc rows and the two new tests describe and pin exactly this, so the contract and the code now agree.

[Astro-Han]

Reviewed c246c0430d84b7f5f6db624167041e3a3d25d29b, which is a merge commit: its parents are 1f5b0085 (the revision I reviewed last) and main at 83511210, bringing in PR7's Artifact/Memory work. My earlier conclusions are unaffected by it, and the merge itself checks out — I verified it numerically rather than by reading the merge commit's prose.

No new P0–P2.

The merge is faithful. A merge is the one change where reading the diff is not enough, because a bad resolution drops one side silently, so I compared the merged tree against both parents:

• git diff --name-only 83511210 c246c043 is exactly the PR's own 15 files. Nothing outside the PR's file set was altered, so no part of main was dropped or rewritten on the way in.
• The PR's new files — goal-plan-operations.ts, goal-plan-routes.ts, session-domain-observation.ts, the channel hook, the three new test files and the design doc — are byte-identical between 1f5b0085 and c246c043. The merge did not disturb the code under review.
• For the files both sides edited, "what the merge brought on top of the PR head" equals main's own contribution, per file: session-registry.ts 304/1 = 304/1, maka-acp-agent.ts 60/1 = 60/1, README.md 79/0 = 79/0, acp-agent.test.ts 15/0 = 15/0. The only file that differs is VALIDATION.md (143/0 against 151/0), and the 8 extra lines are the new validation record the merge commit adds as documentation (VALIDATION.md:22).
• Concretely, main's Artifact/Memory additions are present in the merged session-registry.ts (#artifactConnectionDisposer, #artifactRequest, #pruneExpiredArtifactUploads, artifactUploadKey, #hostRequest), coexisting with the Goal/Plan paths this PR adds — the file grows from 2643 to 2946 lines, which is main's additions plus the PR's.

That verifies both claims in the new record: the two conflicts retained both families' Session Registry shutdown cleanup, and no new production logic was introduced to resolve them. The equality in the third bullet is the strong form of the second claim: the merged tree is exactly the PR head plus main's changes, with nothing invented in between.

My earlier findings still stand, unchanged. P2 (Goal notification lost) stays closed at fbfe620f, and the failed-delivery contract stays closed by the docs at 23623a72 plus the live-observer tests at 1f5b0085 — the merge left session-domain-observation.ts byte-identical, so those conclusions carry forward intact.

Two things for whoever decides the merge

• The existing APPROVED is bound to 1f5b0085, not to this merge head. This repository deliberately does not dismiss stale approvals, so the green approval shown on the PR no longer covers the code that would be merged. My own earlier comments are in the same position.
• main has advanced two commits past the merge parent. Neither touches this PR's files, and the only protocol change is the compatibility-epoch bump in protocol/index.ts (189 → 190), which does not alter HOST_OPERATION_SPECS, so the imports and typing this PR relies on are unaffected. Even so, a tree merged with today's main has not been through CI.

What I checked here: the two parents and their content, the per-file contribution equality above, the presence of main's additions in the merged files, the byte-identity of the PR's new files, the overlap between main's two later commits and this PR's files, and the composition of HOST_OPERATION_SPECS on main. test is green on this head (run 36247870450).

What I could not judge: unchanged from my earlier comments — I did not re-run the suites locally, no real ACP editor client, no platform CI. And I verified the merged-in main content was carried faithfully; whether PR7's Artifact/Memory work is itself correct is its own review lane, not something I checked here.

I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.