Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions packages/agent/src/adapters/claude/conversion/sdk-to-acp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 +313 to +327

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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 +324 to +327

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

}

const meta: Record<string, unknown> = {
...toolMeta(
chunk.name,
Expand Down
3 changes: 2 additions & 1 deletion packages/agent/src/posthog-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,14 @@ export class PostHogAPIClient {
taskId: string,
runId: string,
text: string,
kind: "reply" | "question" = "reply",
): Promise<void> {
const teamId = this.getTeamId();
await this.apiRequest<{ status: string }>(
`/api/projects/${teamId}/tasks/${taskId}/runs/${runId}/relay_message/`,
{
method: "POST",
body: JSON.stringify({ text }),
body: JSON.stringify({ text, kind }),
},
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/agent/src/server/agent-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2315,7 +2315,7 @@ ${signedCommitInstructions}

this.questionRelayedToSlack = true;
this.posthogAPI
.relayMessage(payload.task_id, payload.run_id, message)
.relayMessage(payload.task_id, payload.run_id, message, "question")
.catch((err) =>
this.logger.debug("Failed to relay question to Slack", { err }),
);
Expand Down
Loading