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
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,37 @@ function resultSuccess(sessionId: string) {
};
}

function thinkingDelta(sessionId: string, thinking: string) {
return {
type: "stream_event",
parent_tool_use_id: null,
session_id: sessionId,
uuid: `think-${thinking}`,
event: {
type: "content_block_delta",
index: 0,
delta: { type: "thinking_delta", thinking },
},
};
}

function exitPlanModeAssistant(sessionId: string, apiId: string) {
return {
type: "assistant",
parent_tool_use_id: null,
session_id: sessionId,
uuid: `assistant-${apiId}`,
message: {
id: apiId,
role: "assistant",
content: [
{ type: "redacted_thinking", data: "encrypted" },
{ type: "tool_use", id: "tool-1", name: "ExitPlanMode", input: {} },
],
},
};
}

function messageChunkTexts(
calls: ClientMocks["sessionUpdate"]["mock"]["calls"],
): string[] {
Expand Down Expand Up @@ -216,4 +247,58 @@ describe("ClaudeAcpAgent.prompt — streamed assistant text wiring", () => {
"gateway answer",
]);
});

it("surfaces a fallback message when a turn ends with no prose", async () => {
const { agent, client } = makeAgent();
const sessionId = "s-empty";
const { query, input } = installFakeSession(agent, sessionId);

const promptPromise = agent.prompt({
sessionId,
prompt: [{ type: "text", text: "make a plan" }],
});
await tick();

// Plan-mode turn: encrypted thinking + ExitPlanMode, no text block.
await echoUserMessage(query, input);
await send(query, messageStart(sessionId, "msg_plan"));
await send(query, exitPlanModeAssistant(sessionId, "msg_plan"));
await send(query, resultSuccess(sessionId));

const result = await promptPromise;
expect(result.stopReason).toBe("end_turn");
expect((result._meta as { emptyOutput?: boolean }).emptyOutput).toBe(true);
const chunks = messageChunkTexts(client.sessionUpdate.mock.calls);
expect(chunks).toHaveLength(1);
expect(chunks[0]).toContain("without a written response");
});

it("does not surface a fallback when thinking-only turn also emits text", async () => {
const { agent, client } = makeAgent();
const sessionId = "s-thinking-text";
const { query, input } = installFakeSession(agent, sessionId);

const promptPromise = agent.prompt({
sessionId,
prompt: [{ type: "text", text: "hi" }],
});
await tick();

await echoUserMessage(query, input);
await send(query, messageStart(sessionId, "msg_t"));
await send(query, thinkingDelta(sessionId, "pondering"));
await send(query, textDelta(sessionId, "here is my answer"));
await send(
query,
assistantMessage(sessionId, "msg_t", "here is my answer"),
);
await send(query, resultSuccess(sessionId));

const result = await promptPromise;
expect(result.stopReason).toBe("end_turn");
expect((result._meta as { emptyOutput?: boolean }).emptyOutput).toBe(false);
expect(messageChunkTexts(client.sessionUpdate.mock.calls)).toEqual([
"here is my answer",
]);
});
});
Comment on lines 247 to 304

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 The two new tests both exercise the result completion path (via resultSuccess). The session_state_changed: idle completion path — which has its own surfaceEmptyTurnFallback call — has no coverage. A plan-mode empty turn that arrives over the idle path would follow different branching (idleStopReason, idleEmptyOutput), so it would be good to have at least one test that drives the idle path to confirm both the fallback fires and _meta.emptyOutput is set correctly there too.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts
Line: 247-304

Comment:
The two new tests both exercise the `result` completion path (via `resultSuccess`). The `session_state_changed: idle` completion path — which has its own `surfaceEmptyTurnFallback` call — has no coverage. A plan-mode empty turn that arrives over the idle path would follow different branching (`idleStopReason`, `idleEmptyOutput`), so it would be good to have at least one test that drives the idle path to confirm both the fallback fires and `_meta.emptyOutput` is set correctly there too.

How can I resolve this? If you propose a fix, please make it concise.

112 changes: 104 additions & 8 deletions packages/agent/src/adapters/claude/claude-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ const DEFAULT_FORCE_CANCEL_GRACE_MS = 30_000;
const MAX_TITLE_LENGTH = 256;
const LOCAL_ONLY_COMMANDS = new Set(["/context", "/heapdump", "/extra-usage"]);

// Shown when a turn completes (`end_turn`) without delivering any user-visible
// prose. Without it the UI renders a silent blank turn — most commonly a
// plan-mode turn where the model only produced (encrypted) thinking before
// calling `ExitPlanMode`, which carries no `agent_message_chunk` of its own.
const EMPTY_TURN_FALLBACK_TEXT =
"_The agent finished this turn without a written response. This can happen in plan mode when it reasons through a plan but doesn't post a message. You can ask it to continue or send your request again._";

// Best-effort: silent on ENOENT, logs other errors so permission failures
// aren't masked.
function readClaudeMdQuietly(cwd: string, logger: Logger): string | undefined {
Expand Down Expand Up @@ -501,10 +508,37 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
| undefined
)?.terminal_output === true;

// Track whether any user-visible prose (`agent_message_chunk`) reached the
// client during this turn. A turn that ends in `end_turn` having emitted
// none is a silent blank turn (e.g. plan-mode thinking + `ExitPlanMode`);
// the `end_turn` completion points below surface a fallback message in that
// case. Wrapping the client keeps the signal accurate across every emit
// path — streamed text, consolidated messages, and the direct emits below.
let emittedAgentMessageChunk = false;
const trackingClient = new Proxy(this.client, {
get: (target, prop, receiver) => {
if (prop === "sessionUpdate") {
return (
notification: Parameters<AgentSideConnection["sessionUpdate"]>[0],
) => {
if (
(notification as { update?: { sessionUpdate?: string } }).update
?.sessionUpdate === "agent_message_chunk"
) {
emittedAgentMessageChunk = true;
}
return target.sessionUpdate(notification);
};
}
const value = Reflect.get(target, prop, receiver);
return typeof value === "function" ? value.bind(target) : value;
},
});

const context = {
session: this.session,
sessionId: params.sessionId,
client: this.client,
client: trackingClient,
toolUseCache: this.toolUseCache,
toolUseStreamCache: this.toolUseStreamCache,
fileContentCache: this.fileContentCache,
Expand Down Expand Up @@ -615,7 +649,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
compactionInProgress
) {
compactionInProgress = false;
await this.client.sessionUpdate({
await trackingClient.sessionUpdate({
sessionId: params.sessionId,
update: {
sessionUpdate: "agent_message_chunk",
Expand All @@ -634,7 +668,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
const reason = message.compact_error
? `: ${message.compact_error}`
: ".";
await this.client.sessionUpdate({
await trackingClient.sessionUpdate({
sessionId: params.sessionId,
update: {
sessionUpdate: "agent_message_chunk",
Expand Down Expand Up @@ -672,7 +706,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
"Slash command produced no output; treating as unsupported",
{ sessionId: params.sessionId, command: cmd },
);
await this.client.sessionUpdate({
await trackingClient.sessionUpdate({
sessionId: params.sessionId,
update: {
sessionUpdate: "agent_message_chunk",
Expand Down Expand Up @@ -708,8 +742,20 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
},
});

const idleStopReason = this.session.cancelled
? "cancelled"
: "end_turn";
let idleEmptyOutput = false;
if (idleStopReason === "end_turn") {
idleEmptyOutput = await this.surfaceEmptyTurnFallback(
params.sessionId,
emittedAgentMessageChunk,
);
}

return {
stopReason: this.session.cancelled ? "cancelled" : "end_turn",
stopReason: idleStopReason,
_meta: { emptyOutput: idleEmptyOutput },
};
}
await handleSystemMessage(message, context);
Expand Down Expand Up @@ -814,7 +860,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
(message as { stop_reason?: string }).stop_reason === "refusal"
) {
if (lastRefusalExplanation) {
await this.client.sessionUpdate({
await trackingClient.sessionUpdate({
sessionId: params.sessionId,
update: {
sessionUpdate: "agent_message_chunk",
Expand Down Expand Up @@ -845,7 +891,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
message.subtype === "success" &&
message.result
) {
await this.client.sessionUpdate({
await trackingClient.sessionUpdate({
sessionId: params.sessionId,
update: {
sessionUpdate: "agent_message_chunk",
Expand All @@ -854,7 +900,28 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
});
}

return { stopReason: result.stopReason ?? "end_turn", usage };
const effectiveStopReason = result.stopReason ?? "end_turn";
// Guarantee the UI never renders a silent blank turn. A successful
// `end_turn` that delivered no prose (e.g. plan-mode thinking +
// `ExitPlanMode`) gets a fallback message. Skipped when the SDK's
// native structured output carried the response instead of a chunk.
let emptyOutput = false;
if (
effectiveStopReason === "end_turn" &&
(message as { structured_output?: unknown }).structured_output ==
null
) {
emptyOutput = await this.surfaceEmptyTurnFallback(
params.sessionId,
emittedAgentMessageChunk,
);
}

return {
stopReason: effectiveStopReason,
usage,
_meta: { emptyOutput },
};
}

case "stream_event": {
Expand Down Expand Up @@ -1133,6 +1200,35 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
}
}

/**
* Surfaces a fallback message when a turn finished without delivering any
* user-visible prose. Returns `true` when the fallback was shown (the turn
* produced empty output), `false` when real content already reached the
* client. Guarantees the UI never renders a silent blank turn.
*/
private async surfaceEmptyTurnFallback(
sessionId: string,
emittedAgentMessageChunk: boolean,
): Promise<boolean> {
if (emittedAgentMessageChunk) {
return false;
}
this.logger.warn(
"Turn ended with no agent_message_chunk; surfacing fallback",
{
sessionId,
},
);
await this.client.sessionUpdate({
sessionId,
update: {
sessionUpdate: "agent_message_chunk",
content: { type: "text", text: EMPTY_TURN_FALLBACK_TEXT },
},
});
Comment on lines +1222 to +1228

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 surfaceEmptyTurnFallback bypasses trackingClient and emits directly via this.client. As a class method it can't close over trackingClient, so after the fallback fires emittedAgentMessageChunk remains false. Right now this is harmless because every call site immediately returns, but if a second check of emittedAgentMessageChunk is ever added after one of those calls the signal will be stale. Accepting the local variable as an extra parameter (and updating it to true before returning) would make the state consistent.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/claude-agent.ts
Line: 1222-1228

Comment:
`surfaceEmptyTurnFallback` bypasses `trackingClient` and emits directly via `this.client`. As a class method it can't close over `trackingClient`, so after the fallback fires `emittedAgentMessageChunk` remains `false`. Right now this is harmless because every call site immediately `return`s, but if a second check of `emittedAgentMessageChunk` is ever added after one of those calls the signal will be stale. Accepting the local variable as an extra parameter (and updating it to `true` before returning) would make the state consistent.

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!

return true;
}

// Called by BaseAcpAgent#cancel() to interrupt the session
protected async interrupt(): Promise<void> {
this.session.cancelled = true;
Expand Down
67 changes: 50 additions & 17 deletions packages/core/src/sessions/sessionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ import {
type StoredLogEntry,
type TaskRunStatus,
} from "@posthog/shared";
import { ANALYTICS_EVENTS } from "@posthog/shared/analytics-events";
import {
ANALYTICS_EVENTS,
type StopReason,
} from "@posthog/shared/analytics-events";
import {
type CloudTaskPermissionRequestUpdate,
type CloudTaskUpdatePayload,
Expand Down Expand Up @@ -93,6 +96,33 @@ const GITHUB_AUTHORIZATION_REQUIRED_CODE = "github_authorization_required";
const AUTO_RETRY_MAX_ATTEMPTS = 2;
const AUTO_RETRY_DELAY_MS = 10_000;

const KNOWN_STOP_REASONS = new Set<StopReason>([
"end_turn",
"max_tokens",
"max_turn_requests",
"refusal",
"cancelled",
"error",
"timeout",
]);

// Maps the agent's raw turn stop reason onto the analytics `StopReason` union,
// falling back to "other" so a new/unknown reason is still counted.
function normalizeStopReason(raw: string | undefined): StopReason {
return raw && KNOWN_STOP_REASONS.has(raw as StopReason)
? (raw as StopReason)
: "other";
}

// Counts the prompts a session has sent — used as `prompts_sent` on run-level
// analytics. A `session/prompt` request is the canonical "a turn started"
// marker in the event log.
function countPromptsSent(session: AgentSession): number {
return session.events.filter(
(e) => "method" in e.message && e.message.method === "session/prompt",
).length;
}

class GitHubAuthorizationRequiredForCloudHandoffError extends Error {
constructor(
message = "Connect GitHub before continuing this task in cloud.",
Expand Down Expand Up @@ -1431,6 +1461,21 @@ export class SessionService {
const stopReason = (msg.result as { stopReason?: string }).stopReason;
const hasQueuedMessages = this.drainQueuedMessages(taskRunId, session);

// Record the completed turn so empty/odd outcomes are observable. The
// `_meta.emptyOutput` flag is set by the agent when a turn finished
// without delivering any user-visible prose (see `surfaceEmptyTurnFallback`).
const emptyOutput =
(msg.result as { _meta?: { emptyOutput?: unknown } })._meta
?.emptyOutput === true;
this.d.track(ANALYTICS_EVENTS.TASK_RUN_COMPLETED, {
task_id: session.taskId,
execution_type: "local",
duration_seconds: Math.round((Date.now() - session.startedAt) / 1000),
prompts_sent: countPromptsSent(session),
stop_reason: normalizeStopReason(stopReason),
empty_output: emptyOutput,
});

// Only notify when queue is empty - queued messages will start a new turn
if (stopReason && !hasQueuedMessages) {
this.d.notifyPromptComplete(
Expand Down Expand Up @@ -1920,17 +1965,11 @@ export class SessionService {
sessionId: session.taskRunId,
});

const durationSeconds = Math.round(
(Date.now() - session.startedAt) / 1000,
);
const promptCount = session.events.filter(
(e) => "method" in e.message && e.message.method === "session/prompt",
).length;
this.d.track(ANALYTICS_EVENTS.TASK_RUN_CANCELLED, {
task_id: taskId,
execution_type: "local",
duration_seconds: durationSeconds,
prompts_sent: promptCount,
duration_seconds: Math.round((Date.now() - session.startedAt) / 1000),
prompts_sent: countPromptsSent(session),
});

return result;
Expand Down Expand Up @@ -2360,17 +2399,11 @@ export class SessionService {
method: "cancel",
});

const durationSeconds = Math.round(
(Date.now() - session.startedAt) / 1000,
);
const promptCount = session.events.filter(
(e) => "method" in e.message && e.message.method === "session/prompt",
).length;
this.d.track(ANALYTICS_EVENTS.TASK_RUN_CANCELLED, {
task_id: session.taskId,
execution_type: "cloud",
duration_seconds: durationSeconds,
prompts_sent: promptCount,
duration_seconds: Math.round((Date.now() - session.startedAt) / 1000),
prompts_sent: countPromptsSent(session),
});

if (!result.success) {
Expand Down
Loading
Loading