feat(agent): emit status notifications for orchestrator streaming#2732
Draft
VojtechBartos wants to merge 3 commits into
Draft
feat(agent): emit status notifications for orchestrator streaming#2732VojtechBartos wants to merge 3 commits into
VojtechBartos wants to merge 3 commits into
Conversation
Emit a POSTHOG_NOTIFICATIONS.STATUS notification when a tool first starts. The payload carries the existing toolInfo.title as a human-readable text so downstream consumers can render a live "agent is doing X" status line without inferring intent from raw tool names. The orchestrator consumes this in a follow-up PR; existing ACP clients already ignore unknown status values, so this is backwards-compatible.
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts:255-264
**Generic title fired during streaming; specific title never sent**
In the streaming path, `handleToolUseChunk` is first called from `streamEventToAcpNotifications → content_block_start`. At that point `chunk.input` is always `{}` because the input arrives later via `input_json_delta` deltas (which go through `inputJsonDeltaToAcpNotifications`, 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 `SDKAssistantMessage` arrives with the full input, `alreadyCached` is already `true`, so the updated title is never sent. The only path that produces a specific title is a non-streaming scenario where `handleUserAssistantMessage` is the first call site for a given tool.
### Issue 2 of 2
packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts:261-264
**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.
Reviews (1): Last reviewed commit: "feat(agent): broadcast tool-use status n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+255
to
+264
| if (!alreadyCached && toolInfo.title) { | ||
| void ctx.client | ||
| .extNotification(POSTHOG_NOTIFICATIONS.STATUS, { | ||
| sessionId: ctx.sessionId, | ||
| status: "tool_use", | ||
| text: toolInfo.title, | ||
| }) | ||
| .catch(() => { | ||
| // Best-effort — a failed status broadcast must not break tool execution. | ||
| }); |
Contributor
There was a problem hiding this comment.
In the streaming path, handleToolUseChunk is first called from streamEventToAcpNotifications → content_block_start. At that point chunk.input is always {} because the input arrives later via input_json_delta deltas (which go through inputJsonDeltaToAcpNotifications, 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 SDKAssistantMessage arrives with the full input, alreadyCached is already true, so the updated title is never sent. The only path that produces a specific title is a non-streaming scenario where handleUserAssistantMessage is the first call site for a given tool.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts
Line: 255-264
Comment:
**Generic title fired during streaming; specific title never sent**
In the streaming path, `handleToolUseChunk` is first called from `streamEventToAcpNotifications → content_block_start`. At that point `chunk.input` is always `{}` because the input arrives later via `input_json_delta` deltas (which go through `inputJsonDeltaToAcpNotifications`, 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 `SDKAssistantMessage` arrives with the full input, `alreadyCached` is already `true`, so the updated title is never sent. The only path that produces a specific title is a non-streaming scenario where `handleUserAssistantMessage` is the first call site for a given tool.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+261
to
+264
| }) | ||
| .catch(() => { | ||
| // Best-effort — a failed status broadcast must not break tool execution. | ||
| }); |
Contributor
There was a problem hiding this comment.
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.
Prompt To Fix With AI
This 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!
…eview The downstream Slack orchestrator now renders each agent action as a plan-block step (task_update chunk) with the bare tool name as the title and a short preview of the args as the details line. Match Slack's task_update field shape so the orchestrator can pass these through with no further transformation. tool_args_preview is derived from the first matching arg field (bash command / file_path / path / query / pattern / url / description), collapsed to a single line and trimmed to 240 chars (well under the 256-char Slack limit).
The Slack orchestrator suppresses the post-turn 'reply' relay when the streaming plan-block path is active (the streamed message already carries the agent's text). Questions still need to render as a standalone thread message so the user can see and respond, so they're passed through with kind="question". Default stays "reply" — pre-existing callers continue to behave the same; only the question relay opts into the new kind.
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.
Problem
When PostHog Code runs a task triggered from Slack, the user only sees a static "Working on task…" placeholder in the thread for the entire run. They have no visibility into what the agent is actually doing.
The orchestrator can render live progress, but it needs a semantic signal — inferring intent from raw
session/updatetool events is brittle (Bash≠ "running tests"). Cleaner to have the agent describe its own intent as it works.Changes
packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts— emitPOSTHOG_NOTIFICATIONS.STATUS(already declared) with the existingtoolInfo.titleas a human-readable text whenever a tool first starts. Fire-and-forget (void); a failed broadcast must never break tool execution.statusvalues.How did you test this code?
Agent-authored. The 15-line change reuses the existing
toolInfo.titlerendering (covered by the surroundinghandleToolUseChunktests) and the existingclient.extNotification(STATUS, …)flow (already tested forcompacting). End-to-end verification is paired with PostHog/posthog#64293 which consumes the new emit — once both deploy and theposthog-code-slack-agent-statusflag is on, Slack threads will show live "Reading api.py" / "Running tests" status lines.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Model: Claude Opus 4.7. Paired with PostHog/posthog#64293 which consumes these events on the orchestrator side. Decision: chose to put status-string responsibility on the agent (knows intent) rather than the orchestrator (would have to infer from tool names) — sketched the inference path first, rejected for brittleness.