-
Notifications
You must be signed in to change notification settings - Fork 44
feat(agent): emit status notifications for orchestrator streaming #2732
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
3c39135
8ec2712
f5caa29
6b7c0e8
b446d87
41f9025
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,60 @@ function bashCommandFromToolUse( | |
| return typeof command === "string" ? command : undefined; | ||
| } | ||
|
|
||
| const TOOL_ARGS_PREVIEW_LIMIT = 240; | ||
|
|
||
| const TOOL_ARGS_PREVIEW_KEYS = [ | ||
| "file_path", | ||
| "notebook_path", | ||
| "path", | ||
| "code", // MCP exec / hogql / sql payloads | ||
| "query", // search queries | ||
| "pattern", // grep / glob patterns | ||
| "url", | ||
| "description", | ||
| "prompt", // Task / Agent sub-agent prompt | ||
| "name", // schema lookups | ||
| "title", | ||
| ]; | ||
|
|
||
| function toolArgsPreview( | ||
| chunk: ToolUseCache[string], | ||
| bashCommand: string | undefined, | ||
| ): string { | ||
| const input = chunk.input as Record<string, unknown> | undefined; | ||
| const tryField = (key: string): string | undefined => { | ||
| const v = input?.[key]; | ||
| return typeof v === "string" && v ? v : undefined; | ||
| }; | ||
|
|
||
| let raw = bashCommand; | ||
| if (!raw) { | ||
| for (const key of TOOL_ARGS_PREVIEW_KEYS) { | ||
| const v = tryField(key); | ||
| if (v) { | ||
| raw = v; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| // Fallback: take the first short-string arg of the input. Avoids returning | ||
| // the empty string when an MCP tool uses an arg name we don't enumerate | ||
| // above. Bound by ``TOOL_ARGS_PREVIEW_LIMIT`` after the truncation below. | ||
| if (!raw && input) { | ||
| for (const value of Object.values(input)) { | ||
| if (typeof value === "string" && value.trim()) { | ||
| raw = value; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (!raw) return ""; | ||
| const oneLine = raw.replace(/\s+/g, " ").trim(); | ||
| return oneLine.length > TOOL_ARGS_PREVIEW_LIMIT | ||
| ? `${oneLine.slice(0, TOOL_ARGS_PREVIEW_LIMIT - 1)}…` | ||
| : oneLine; | ||
| } | ||
|
|
||
| function handleTextChunk( | ||
| chunk: { text: string }, | ||
| role: Role, | ||
|
|
@@ -249,6 +303,30 @@ function handleToolUseChunk( | |
| cwd: ctx.cwd, | ||
| }); | ||
|
|
||
| // Broadcast a live "agent is doing X" status when a tool first starts so | ||
| // downstream consumers (the Slack orchestrator) can render it as a status | ||
| // line in the thread without inferring intent from raw tool names. The | ||
| // `tool_name` + `tool_args_preview` fields let renderers show the bare tool | ||
| // name on the plan-block step and a short preview of the args (file path, | ||
| // command, query) on the `details` line — same shape as Slack's | ||
| // task_update chunk. | ||
| if (!alreadyCached && toolInfo.title) { | ||
| void ctx.client | ||
| .extNotification(POSTHOG_NOTIFICATIONS.STATUS, { | ||
| sessionId: ctx.sessionId, | ||
| status: "tool_use", | ||
| text: toolInfo.title, | ||
| tool_name: chunk.name, | ||
| tool_args_preview: toolArgsPreview( | ||
| chunk, | ||
| bashCommandFromToolUse(chunk), | ||
| ), | ||
| }) | ||
| .catch(() => { | ||
| // Best-effort — a failed status broadcast must not break tool execution. | ||
| }); | ||
|
Comment on lines
+324
to
+327
Contributor
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 Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts
Line: 261-264
Comment:
**Silent error suppression makes systematic failures invisible**
The `.catch(() => {})` swallows all errors without logging. If the `extNotification` call is systematically failing (e.g. due to a schema mismatch, a disconnected client, or a downstream bug), there will be no signal in any log output. A single `ctx.logger.warn(...)` call in the catch handler would make these failures observable without changing the fire-and-forget semantics.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| } | ||
|
|
||
| const meta: Record<string, unknown> = { | ||
| ...toolMeta( | ||
| chunk.name, | ||
|
|
||
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.
In the streaming path,
handleToolUseChunkis first called fromstreamEventToAcpNotifications → content_block_start. At that pointchunk.inputis always{}because the input arrives later viainput_json_deltadeltas (which go throughinputJsonDeltaToAcpNotifications, never through this function again). So the notification fires immediately with the fallback title —"Execute command"for Bash,"Read File"for Read,"Edit"for Edit — not the specific"Running tests"/"Reading api.py"values the PR describes.When the consolidated
SDKAssistantMessagearrives with the full input,alreadyCachedis alreadytrue, so the updated title is never sent. The only path that produces a specific title is a non-streaming scenario wherehandleUserAssistantMessageis the first call site for a given tool.Prompt To Fix With AI