feat(acp): close v2 draft DTO gaps - #160
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
AcpProtocolVersion.Latest reads as "the version to use", but its value is only the highest version whose wire contracts are modeled - and that version is a draft the live client refuses to negotiate. The doc comment said so; the name contradicted it. Latest shipped in SalmonEgg.Acp 1.0.0, so package validation forbids removing it: it stays as an obsolete alias pointing at the new name. The deprecation message is an internal constant so the contract can be asserted without duplicating the string, and the test reads the field through reflection so asserting on a deprecated member does not trip warnings-as-errors. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ACP negotiates one version per connection and both sides then act according to its specification, so a client is compliant serving exactly one version - it does not have to branch every method on a version. What it must do is close the connection when the Agent answers with a version it cannot run. The post-negotiation check asked IsSupported, which is true for draft v2 because the SDK models those wire contracts so they can be developed and asserted. Modeling is not serving: parsing v2 says nothing about having implemented its prompt lifecycle. Name that distinction as AcpProtocolVersion.RuntimeServed and have both the initialize gate and the negotiation check answer to it, instead of spelling out "!= V1" at each site. Today the two predicates only diverge above the served version, which the existing serverProtocolIsNewer test already reaches - a probe throw confirmed that, and confirmed a downgrade test written from the served side cannot reach the check at all, so no such test is added. That test now also pins why v2 is refused: the runtime does not serve it, not that the SDK cannot parse it. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
In v2 the session/prompt response is a bare acknowledgement and a turn ends when the Agent reports idle carrying a stop reason, so state_update is the wire shape the whole v2 prompt lifecycle rests on. Model it as an open hierarchy: the schema's trailing unconstrained member makes any state string valid, and unknown values that do not begin with _ are reserved for future ACP, so they round-trip verbatim instead of being downgraded here. The wire form is doubly flattened - "state" and its payload are siblings of the outer "sessionUpdate" discriminator. STJ resolves one discriminator per hierarchy and rejects a JsonConverter on a polymorphic derived type, so this variant is read and written by the containing params converter rather than a JsonDerivedType registration. Writing is fail-closed on the negotiated version, matching SessionReplayFrom: state_update does not exist in v1, so emitting one under a v1 write context would put a field on the wire a v1 Agent has no contract for. Reading stays version-agnostic, because gating reads would make the client the arbiter of the Agent's semantics. stopReason degrades to "no reason reported" on a malformed value, per its x-deserialize-default-on-error marking: losing the reason is recoverable, dropping the end-of-turn signal is not. Omitted and null are likewise kept distinct from end_turn, which would fabricate a completed turn out of a bare idle transition. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
v2 adds agent_message, user_message, and agent_thought, which carry a complete message rather than one streamed fragment. They do not replace the *_chunk updates: v2 keeps both families, so an Agent may stream fragments, send whole messages, or mix the two in one session. These are therefore new variants alongside the chunk ones, not a migration of them. content is three-state and each state is a distinct instruction: absent leaves the message unchanged, null clears it, and an array replaces the whole array including content accumulated from earlier chunks with the same message id. A plain nullable field cannot express that, so presence is reported separately by HasContent. JsonIgnore keeps that flag off the wire, which also means STJ never populates it - the params converter does, because without it every update would read as "content absent" and a clear could not be told from a no-op. user_message matters beyond symmetry: in v2 the Agent must report where the user's prompt landed in session history after accepting it, and that update is the source of truth for the agent-owned message id. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…streaming tool content Three v2 shapes that all concern how the Agent reports work in progress. Terminals: v2 has no terminal/* methods at all - the Client no longer creates, waits on, kills, or releases terminals over ACP. The Agent owns them and reports state through terminal_update and terminal_output_chunk, so these two notifications replace the entire v1 client-implemented terminal surface. The v1 request types stay on the public surface because ApiCompat forbids removing them, so a test asserts the split rather than inferring it from absence. Output chunks are independently base64-encoded, which the tests pin by proving that concatenating the base64 text gives a different result than decoding per chunk. Structured diff: replaces v1's flat path/oldText/newText, which could only describe one modified text file. Both shapes use the same "diff" discriminator, so they are told apart by structure - a changes array means v2. Malformed change items are skipped per x-deserialize-skip-invalid-items rather than failing the whole diff alongside its valid siblings. Streaming tool content: tool_call_content_chunk appends one item, unlike tool_call_update's content array which replaces. Without it, streaming tool output would mean resending everything produced so far on every fragment. Terminal patch fields get the same presence-flag treatment as whole messages, for the same reason: absent means leave unchanged, null means clear, and an empty exitStatus object still marks the terminal as exited. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…nges Complete the remaining draft v2 DTO surface that can be modeled without a v2 Agent: permission subjects, configId, object-marker capabilities, icons, opaque session cursors, text command input, plan_update content, and v2's bare prompt acknowledgement. V2 prompt responses no longer carry stopReason. The response converter records whether a v1 stopReason was actually present, so an empty v2 ack cannot silently fabricate an end_turn; its terminal reason instead comes from idle state_update. V2 config options rename id to configId. Reads accept both shapes while writes select the negotiated wire field, keeping a v1 peer from seeing v2-only fields and vice versa. Permission subject and plan-content unions preserve unknown payloads verbatim for forward compatibility. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… one The v2 state_update payload was modeled as SalmonEgg.Acp.Protocol.SessionState, which is an ambiguous reference wherever SalmonEgg.Domain.Models.Session is imported alongside it. The chat layer does exactly that in 17 files, so ChatService failed to compile with CS0104 and took the browserwasm head down with it. Renaming rather than aliasing at each use site: the type is new in this branch and absent from the published 1.0.0 surface, so it is free to rename, and "work state" is the more accurate name anyway. ACP's state_update reports whether foreground work is running, idle, or blocked, while the domain enum is a session lifecycle (active/waiting/cancelled/completed/error). The existing SessionMode pair genuinely shares a concept and keeps its using-aliases; this one never did. The ACP SDK gates build only the SDK and its tests, so they were green while a downstream consumer could not compile. Verified this time by compiling the net10.0-browserwasm head that failed: 0 errors, 0 warnings, with only the local emscripten native-link validation skipped. Refs #149. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
YoungSx
force-pushed
the
chore/acp-v2-gap-ledger-149
branch
from
September 2, 2026 12:27
8454436 to
f9607cc
Compare
This was referenced Sep 2, 2026
YoungSx
added a commit
that referenced
this pull request
Sep 3, 2026
PR #160 modeled the v2 wire contracts and put 37 types on the public surface with no compile-time signal at all. A 1.x consumer could build against a protocol whose client lifecycle is unimplemented and which no live connection negotiates, and find out only when initialize refuses the version. AGENTS.md already forbade shipping draft wire from production entry points; nothing enforced it past the SDK's own boundary. Every one of those 37 types now carries [Experimental(SEACP002)], which the compiler reports as an error by default rather than a warning. The id, the diagnostic text and the documentation link are single-sourced in AcpDraftProtocol because three places have to agree on them - the attributes, the NoWarn entries, and the gates - and a literal at each site lets any one of them drift silently. The default "for evaluation purposes only" wording is replaced: it is true but omits the fact that decides whether to use the type at all, and the CLI prints Message verbatim. The two project-level NoWarn entries are forced, not chosen. The JSON source generator emits AcpJsonContext.*.g.cs naming every registered draft type, and a #pragma in a hand-written file cannot reach generated code - without the suppression the SDK reports 615 errors and cannot compile its own contracts. Verified by removing it. The cost is stated where it lands: the marker is also silent inside AcpClient.cs, so the compiler no longer backstops the rule about production entry points, and gate tests have to carry that weight instead. The packaged README documents the id, both working suppressions, and the fact that [SuppressMessage] is not one of them - SEACP002 comes from the compiler, not an analyzer, so no attribute-based suppression applies at any category. It also documents the channel the attribute cannot close: AcpJsonContext exposes a JsonTypeInfo<T> per registered type without copying the attribute onto it, so 26 draft contracts remain reachable without ever naming one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
YoungSx
added a commit
that referenced
this pull request
Sep 3, 2026
PR #160 modeled the v2 wire contracts and put 37 types on the public surface with no compile-time signal at all. A 1.x consumer could build against a protocol whose client lifecycle is unimplemented and which no live connection negotiates, and find out only when initialize refuses the version. AGENTS.md already forbade shipping draft wire from production entry points; nothing enforced it past the SDK's own boundary. Every one of those 37 types now carries [Experimental(SEACP002)], which the compiler reports as an error by default rather than a warning. The id, the diagnostic text and the documentation link are single-sourced in AcpDraftProtocol because three places have to agree on them - the attributes, the NoWarn entries, and the gates - and a literal at each site lets any one of them drift silently. The default "for evaluation purposes only" wording is replaced: it is true but omits the fact that decides whether to use the type at all, and the CLI prints Message verbatim. The two project-level NoWarn entries are forced, not chosen. The JSON source generator emits AcpJsonContext.*.g.cs naming every registered draft type, and a #pragma in a hand-written file cannot reach generated code - without the suppression the SDK reports 615 errors and cannot compile its own contracts. Verified by removing it. The cost is stated where it lands: the marker is also silent inside AcpClient.cs, so the compiler no longer backstops the rule about production entry points, and gate tests have to carry that weight instead. The packaged README documents the id, both working suppressions, and the fact that [SuppressMessage] is not one of them - SEACP002 comes from the compiler, not an analyzer, so no attribute-based suppression applies at any category. It also documents the channel the attribute cannot close: AcpJsonContext exposes a JsonTypeInfo<T> per registered type without copying the attribute onto it, so 26 draft contracts remain reachable without ever naming one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
YoungSx
added a commit
that referenced
this pull request
Sep 3, 2026
PR #160 modeled the v2 wire contracts and put 37 types on the public surface with no compile-time signal at all. A 1.x consumer could build against a protocol whose client lifecycle is unimplemented and which no live connection negotiates, and find out only when initialize refuses the version. AGENTS.md already forbade shipping draft wire from production entry points; nothing enforced it past the SDK's own boundary. Every one of those 37 types now carries [Experimental(SEACP002)], which the compiler reports as an error by default rather than a warning. The id, the diagnostic text and the documentation link are single-sourced in AcpDraftProtocol because three places have to agree on them - the attributes, the NoWarn entries, and the gates - and a literal at each site lets any one of them drift silently. The default "for evaluation purposes only" wording is replaced: it is true but omits the fact that decides whether to use the type at all, and the CLI prints Message verbatim. The two project-level NoWarn entries are forced, not chosen. The JSON source generator emits AcpJsonContext.*.g.cs naming every registered draft type, and a #pragma in a hand-written file cannot reach generated code - without the suppression the SDK reports 615 errors and cannot compile its own contracts. Verified by removing it. The cost is stated where it lands: the marker is also silent inside AcpClient.cs, so the compiler no longer backstops the rule about production entry points, and gate tests have to carry that weight instead. The packaged README documents the id, both working suppressions, and the fact that [SuppressMessage] is not one of them - SEACP002 comes from the compiler, not an analyzer, so no attribute-based suppression applies at any category. It also documents the channel the attribute cannot close: AcpJsonContext exposes a JsonTypeInfo<T> per registered type without copying the attribute onto it, so 26 draft contracts remain reachable without ever naming one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
YoungSx
added a commit
that referenced
this pull request
Sep 3, 2026
PR #160 modeled the v2 wire contracts and put 37 types on the public surface with no compile-time signal at all. A 1.x consumer could build against a protocol whose client lifecycle is unimplemented and which no live connection negotiates, and find out only when initialize refuses the version. AGENTS.md already forbade shipping draft wire from production entry points; nothing enforced it past the SDK's own boundary. Every one of those 37 types now carries [Experimental(SEACP002)], which the compiler reports as an error by default rather than a warning. The id, the diagnostic text and the documentation link are single-sourced in AcpDraftProtocol because three places have to agree on them - the attributes, the NoWarn entries, and the gates - and a literal at each site lets any one of them drift silently. The default "for evaluation purposes only" wording is replaced: it is true but omits the fact that decides whether to use the type at all, and the CLI prints Message verbatim. The two project-level NoWarn entries are forced, not chosen. The JSON source generator emits AcpJsonContext.*.g.cs naming every registered draft type, and a #pragma in a hand-written file cannot reach generated code - without the suppression the SDK reports 615 errors and cannot compile its own contracts. Verified by removing it. The cost is stated where it lands: the marker is also silent inside AcpClient.cs, so the compiler no longer backstops the rule about production entry points, and gate tests have to carry that weight instead. The packaged README documents the id, both working suppressions, and the fact that [SuppressMessage] is not one of them - SEACP002 comes from the compiler, not an analyzer, so no attribute-based suppression applies at any category. It also documents the channel the attribute cannot close: AcpJsonContext exposes a JsonTypeInfo<T> per registered type without copying the attribute onto it, so 26 draft contracts remain reachable without ever naming one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the ACP v2 draft wire-model gaps tracked in #149. The live runtime stays intentionally V1-only.
HighestModeledand addsRuntimeServedto separate "the SDK models this version's wire contracts" from "a live client will run it".Lateststays as an[Obsolete]alias because it shipped in the publishedSalmonEgg.Acp1.0.0 surface and ApiCompat forbids removing it.IsSupported, which is true for draft v2, so an Agent could downgrade the connection onto a version whose lifecycle is unimplemented.state_update(running/idle/requires_action), whole-message upserts alongside the existing*_chunkfamily, Agent-owned terminal notifications, streaming tool-call content, structured diffs, permission subjects, object-marker capabilities,configId, icons, opaque session cursors, text command input, and plan envelopes.session/promptresponse a bare acknowledgement.HasStopReasondistinguishes an empty v2 ack from a v1 terminal result, so an ack can no longer fabricateend_turn.Validation
ACP_PACKAGE_BASELINE_VERSION=1.0.0 scripts/gates/run-acp-sdk-gates.sh Debug: formatting, analyzer builds, 399 contract tests, pack, and ApiCompat against the published 1.0.0 baseline all pass.SalmonEgg.ApplicationandSalmonEgg.Presentation.Coreboth 0 errors. The ACP SDK gates do not build consumers, so this is checked separately — an earlier revision of this work named a typeSessionState, which collided withSalmonEgg.Domain.Models.Session.SessionStateand brokeChatService; it is nowSessionWorkState.developimmediately before opening this PR.Not in scope
state_updatecross-layer completion semantics inChatService/ChatViewModel. Under single-version instances there is no live v2 connection, so it cannot be validated end to end; it belongs with the v2-goes-stable batch.session/load,session/set_mode, MCPsse). Both halves conflict with the single-version architecture — a v2 guard is unreachable dead code, and[Obsolete]would warn every legitimate v1 caller. Recorded on chore(acp): ACP v2 draft 缺口清账（state_update 生命周期、整消息 upsert、终端归属、diff 结构化等） #149 for a decision.Verification caveat
v2 live-lifecycle E2E is not achievable today. Local real agents
codex-acp0.0.46 andclaude-agent-acp0.48.0 both answerprotocolVersion: 1when asked for v2, and return pure v1 capability shapes. The strongest available validation is wire-level v1/v2 dual-line assertions plus reverse verification; "v2 live connection semantics" remains unverified and is documented as such on #149.Refs #149.
🤖 Generated with Claude Code