-
Notifications
You must be signed in to change notification settings - Fork 183
feat(acp): ACP server over WebSocket (revives #1260) #1418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d66cf37
8ab31b6
68d0748
0c099df
c958331
d255e6f
aef2555
50e95b0
0096b22
e43d577
ff6818c
df4406a
455164c
99d3c59
0b04838
9705988
2643349
76c6554
bc46be1
33fded8
dd3dc4e
988193e
6728195
a9ea9f0
6c0c20f
197eaf3
ac50309
8689e36
0d3a74c
279b045
1be48d3
99ff7f8
560e5a7
7e07622
a53f682
45375e9
c51b0f6
f4cca5e
acb0bde
70847fe
eb3cf07
1388591
c314ece
f7c9378
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,20 @@ pub struct OutputDirectives { | |
| pub reply_to: Option<String>, | ||
| } | ||
|
|
||
| /// Chunk limit for delivering a reply on `platform`. ACP is a WebSocket transport with | ||
| /// no small per-message limit, and its reply route is closed after the first delivered | ||
| /// message — so splitting a long reply into multiple messages truncates it over ACP | ||
| /// (review F2). ACP therefore delivers whole (`usize::MAX` → a single chunk); every | ||
| /// other platform keeps the adapter's chunk limit. Overflow-safe: the only arithmetic on | ||
| /// the result is `saturating_sub` (mention-footer reserve). | ||
| fn reply_message_limit(platform: &str, adapter_limit: usize) -> usize { | ||
| if platform == "acp" { | ||
| usize::MAX | ||
| } else { | ||
| adapter_limit | ||
| } | ||
| } | ||
|
|
||
| /// Parse `[[key:value]]` directives from the beginning of agent output. | ||
| /// Returns parsed directives and the remaining content (directives stripped). | ||
| pub fn parse_output_directives(content: &str) -> (OutputDirectives, String) { | ||
|
|
@@ -686,8 +700,16 @@ impl AdapterRouter { | |
| ) -> Result<()> { | ||
| let adapter = adapter.clone(); | ||
| let thread_channel = thread_channel.clone(); | ||
| let message_limit = adapter.message_limit(); | ||
| let streaming = adapter.use_streaming(other_bot_present); | ||
| let message_limit = reply_message_limit(&thread_channel.platform, adapter.message_limit()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 F2 — Make ACP streaming completion and replacement explicit An unbounded final chunk fixes send-once overflow, but it does not fix streaming: placeholder finalization can end with edit_message and no Done, while dummy-draft snapshots can rewrite earlier text during tool/table finalization and corrupt append-only deltas. Requested change: Carry a request-scoped completion event independent of message commands and define append-versus-replace semantics; test placeholder, rewritten-snapshot, and long send-once paths end to end.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The long-reply truncation is fixed ( |
||
| // ACP must not inherit the unified adapter's Telegram streaming flag (wrong | ||
| // coupling): it streams append-only `agent_message_chunk` deltas built from the | ||
| // post+edit (`edit_message` snapshot) path, i.e. streaming=false. Decide it | ||
| // explicitly by platform rather than by whatever Telegram happens to be set to. | ||
| let streaming = if thread_channel.platform == "acp" { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 F3 — Preserve progressive ACP updates For Requested change: Add a dedicated append-only ACP delivery path or narrow the documented contract to final-chunk delivery, with a test proving multiple updates precede the prompt response. |
||
| false | ||
| } else { | ||
| adapter.use_streaming(other_bot_present) | ||
| }; | ||
| // Keep the full turn text (incl. inter-tool narration) when streaming | ||
| // (it was already shown live) OR when `[reactions] narration_display` is | ||
| // set. Otherwise a send-once turn delivers only the final answer block. | ||
|
|
@@ -706,6 +728,10 @@ impl AdapterRouter { | |
| self.table_mode | ||
| }; | ||
| let tool_display = self.reactions_config.tool_display; | ||
| // ACP streams over an append-only `agent_message_chunk`; a re-rendered tool-status | ||
| // prefix from `compose_display` would corrupt the deltas, so ACP streams the raw | ||
| // append-only answer text and surfaces tools separately (review F2 / roadmap). | ||
| let platform_is_acp = thread_channel.platform == "acp"; | ||
| let prompt_hard_timeout = self.prompt_hard_timeout; | ||
| let liveness_check_interval = self.liveness_check_interval; | ||
|
|
||
|
|
@@ -933,7 +959,8 @@ impl AdapterRouter { | |
| } | ||
| } | ||
| } else if let Some(tx) = &buf_tx { | ||
| let _ = tx.send(compose_display( | ||
| let _ = tx.send(display_for( | ||
| platform_is_acp, | ||
| &tool_lines, | ||
| &text_buf, | ||
| true, | ||
|
|
@@ -982,7 +1009,8 @@ impl AdapterRouter { | |
| } | ||
| // Post+edit live update (no-op under native streaming: buf_tx is None). | ||
| if let Some(tx) = &buf_tx { | ||
| let _ = tx.send(compose_display( | ||
| let _ = tx.send(display_for( | ||
| platform_is_acp, | ||
| &tool_lines, | ||
| &text_buf, | ||
| true, | ||
|
|
@@ -1027,7 +1055,8 @@ impl AdapterRouter { | |
| }); | ||
| } | ||
| if let Some(tx) = &buf_tx { | ||
| let _ = tx.send(compose_display( | ||
| let _ = tx.send(display_for( | ||
| platform_is_acp, | ||
| &tool_lines, | ||
| &text_buf, | ||
| true, | ||
|
|
@@ -1086,7 +1115,7 @@ impl AdapterRouter { | |
|
|
||
| // Build final content | ||
| let final_content = | ||
| compose_display(&tool_lines, &text_buf, false, tool_display); | ||
| display_for(platform_is_acp, &tool_lines, &text_buf, false, tool_display); | ||
| let final_content = if final_content.is_empty() { | ||
| if turn_result.is_silent_failure() { | ||
| warn!( | ||
|
|
@@ -1474,6 +1503,25 @@ pub(crate) fn classify_empty_turn( | |
| } | ||
| } | ||
|
|
||
| /// Content to stream/deliver for a reply. ACP gets the raw append-only answer `text` | ||
| /// (its `agent_message_chunk` stream is append-only, so a re-rendered `compose_display` | ||
| /// tool-status prefix would corrupt the deltas — review F2); tool activity is surfaced | ||
| /// separately as structured `tool_call` updates (roadmap). Every other platform gets the | ||
| /// tool-merged display. | ||
| fn display_for( | ||
| platform_is_acp: bool, | ||
| tool_lines: &[ToolEntry], | ||
| text: &str, | ||
| streaming: bool, | ||
| tool_display: ToolDisplay, | ||
| ) -> String { | ||
| if platform_is_acp { | ||
| text.to_string() | ||
| } else { | ||
| compose_display(tool_lines, text, streaming, tool_display) | ||
| } | ||
| } | ||
|
|
||
| fn compose_display( | ||
| tool_lines: &[ToolEntry], | ||
| text: &str, | ||
|
|
@@ -1686,6 +1734,18 @@ fn propagate_mentions_to_chunks( | |
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn acp_reply_limit_is_unbounded_others_use_adapter_limit() { | ||
| // ACP delivers whole (no chunking → no truncation, review F2); other platforms | ||
| // keep the adapter's limit. | ||
| assert_eq!(reply_message_limit("acp", 4096), usize::MAX); | ||
| assert_eq!(reply_message_limit("discord", 2000), 2000); | ||
| assert_eq!(reply_message_limit("slack", 4096), 4096); | ||
| // and a long reply under the ACP limit is a single chunk (delivered whole) | ||
| let long = "x".repeat(50_000); | ||
| assert_eq!(crate::format::split_message(&long, reply_message_limit("acp", 4096)).len(), 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn select_delivery_text_send_once_keeps_only_final_block() { | ||
| // Simulates: narration "n1" → tool (answer_start→2) → narration "n2" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 F3 — Do not advertise incomplete unified support
This enables ACP in unified builds, but the unified Axum router does not mount
/acpandUnifiedGatewayAdapter::dispatch_replyhas no"acp"arm, so the feature is unreachable and replies would be dropped.Requested change: Wire both route and reply dispatch in unified mode, or remove
acpfrom this feature until that integration is complete.