From 88679d39c705414fe41a3e722459e8870847ef97 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 29 May 2026 01:32:54 +0800 Subject: [PATCH 1/6] feat(telemetry): add tool spans and session.id to daemon/ACP path Add interaction-level and tool-level OTel spans to the daemon's ACP Session.ts, closing the observability gap described in #4602. Changes: - session-tracing.ts: emit session.id on llm_request, tool, and tool.execution spans via getCurrentSessionId() - Session.ts runTool(): wrap tool lifecycle in startToolSpan / runInToolSpanContext / endToolSpan; wrap invocation.execute() in startToolExecutionSpan / endToolExecutionSpan - Session.ts #executePrompt: emit logConversationFinishedEvent at turn end (inside withInteractionSpan, after #handleStopHookLoop) - Session.ts #executeCronPrompt: wrap body in withInteractionSpan so cron tool calls also get proper trace hierarchy --- .../src/acp-integration/session/Session.ts | 1339 +++++++++-------- .../core/src/telemetry/session-tracing.ts | 18 +- 2 files changed, 725 insertions(+), 632 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 4882646f6e5..a188d355b17 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -74,6 +74,13 @@ import { shouldRunAutoModeForCall, extractDaemonTraceContext, withInteractionSpan, + startToolSpan, + endToolSpan, + runInToolSpanContext, + startToolExecutionSpan, + endToolExecutionSpan, + logConversationFinishedEvent, + ConversationFinishedEvent, } from '@qwen-code/qwen-code-core'; import { getCommandSubcommandNames } from '../../services/commandMetadata.js'; import { getEffectiveSupportedModes } from '../../services/commandUtils.js'; @@ -829,8 +836,10 @@ export class Session implements SessionContext { } let nextMessage: Content | null = { role: 'user', parts }; + let turnCount = 0; while (nextMessage !== null) { + turnCount++; if (pendingSend.signal.aborted) { this.#getCurrentChat().addHistory(nextMessage); return { stopReason: 'cancelled' }; @@ -965,12 +974,24 @@ export class Session implements SessionContext { // Fire Stop hook loop (aligned with core path in client.ts) // This is triggered after model response completes with no pending tool calls - return this.#handleStopHookLoop( - pendingSend, - promptId, - hooksEnabled, - messageBus, - ); + let stopResult: { stopReason: PromptResponse['stopReason'] }; + try { + stopResult = await this.#handleStopHookLoop( + pendingSend, + promptId, + hooksEnabled, + messageBus, + ); + } finally { + logConversationFinishedEvent( + this.config, + new ConversationFinishedEvent( + this.config.getApprovalMode(), + turnCount, + ), + ); + } + return stopResult; }, (result) => (result.stopReason === 'cancelled' ? 'cancelled' : 'ok'), ); @@ -1540,115 +1561,129 @@ export class Session implements SessionContext { const promptId = this.config.getSessionId() + '########cron' + Date.now(); - try { - // Echo the cron prompt as a user message so the client sees it - await this.sendUpdate({ - sessionUpdate: 'user_message_chunk', - content: { type: 'text', text: prompt }, - _meta: { source: 'cron' }, - }); + let cronHadError = false; + await withInteractionSpan( + this.config, + { + promptId, + model: this.config.getModel(), + messageType: 'cron', + }, + async () => { + try { + // Echo the cron prompt as a user message so the client sees it + await this.sendUpdate({ + sessionUpdate: 'user_message_chunk', + content: { type: 'text', text: prompt }, + _meta: { source: 'cron' }, + }); + + // Prepend session-level system reminders (same rationale as the + // user-query path in #executePrompt). + const cronReminders = await this.#buildInitialSystemReminders(); + let nextMessage: Content | null = { + role: 'user', + parts: [...cronReminders, { text: prompt }], + }; + + while (nextMessage !== null) { + if (ac.signal.aborted) return; + + const functionCalls: FunctionCall[] = []; + let usageMetadata: GenerateContentResponseUsageMetadata | null = + null; + const streamStartTime = Date.now(); - // Prepend session-level system reminders (same rationale as the - // user-query path in #executePrompt). - const cronReminders = await this.#buildInitialSystemReminders(); - let nextMessage: Content | null = { - role: 'user', - parts: [...cronReminders, { text: prompt }], - }; + const sendResult = + await this.#sendMessageStreamWithAutoCompression( + promptId, + nextMessage.parts ?? [], + ac.signal, + ); + if (!sendResult.responseStream) { + this.#preserveUnsentMessageHistory( + nextMessage, + sendResult.stopReason === 'cancelled', + ); + if (sendResult.stopReason === 'max_tokens') { + this.#stopCronAfterTokenLimit(); + } + return; + } + const responseStream = sendResult.responseStream; + nextMessage = null; - while (nextMessage !== null) { - if (ac.signal.aborted) return; + for await (const resp of responseStream) { + if (ac.signal.aborted) return; - const functionCalls: FunctionCall[] = []; - let usageMetadata: GenerateContentResponseUsageMetadata | null = - null; - const streamStartTime = Date.now(); + if ( + resp.type === StreamEventType.CHUNK && + resp.value.candidates && + resp.value.candidates.length > 0 + ) { + const candidate = resp.value.candidates[0]; + for (const part of candidate.content?.parts ?? []) { + if (!part.text) continue; + this.messageEmitter.emitMessage( + part.text, + 'assistant', + part.thought, + ); + } + } - const sendResult = await this.#sendMessageStreamWithAutoCompression( - promptId, - nextMessage.parts ?? [], - ac.signal, - ); - if (!sendResult.responseStream) { - this.#preserveUnsentMessageHistory( - nextMessage, - sendResult.stopReason === 'cancelled', - ); - if (sendResult.stopReason === 'max_tokens') { - this.#stopCronAfterTokenLimit(); - } - return; - } - const responseStream = sendResult.responseStream; - nextMessage = null; + if ( + resp.type === StreamEventType.CHUNK && + resp.value.usageMetadata + ) { + usageMetadata = resp.value.usageMetadata; + } - for await (const resp of responseStream) { - if (ac.signal.aborted) return; + if ( + resp.type === StreamEventType.CHUNK && + resp.value.functionCalls + ) { + functionCalls.push(...resp.value.functionCalls); + } + } - if ( - resp.type === StreamEventType.CHUNK && - resp.value.candidates && - resp.value.candidates.length > 0 - ) { - const candidate = resp.value.candidates[0]; - for (const part of candidate.content?.parts ?? []) { - if (!part.text) continue; - this.messageEmitter.emitMessage( - part.text, - 'assistant', - part.thought, + if (usageMetadata) { + this.#recordPromptTokenCount(usageMetadata); + if (this.messageRewriter) { + this.messageRewriter.flushTurn(ac.signal); + } + const durationMs = Date.now() - streamStartTime; + await this.messageEmitter.emitUsageMetadata( + usageMetadata, + '', + durationMs, ); } - } - - if ( - resp.type === StreamEventType.CHUNK && - resp.value.usageMetadata - ) { - usageMetadata = resp.value.usageMetadata; - } - if ( - resp.type === StreamEventType.CHUNK && - resp.value.functionCalls - ) { - functionCalls.push(...resp.value.functionCalls); + if (functionCalls.length > 0) { + const toolResponseParts = await this.runToolCalls( + ac.signal, + promptId, + functionCalls, + ); + nextMessage = { role: 'user', parts: toolResponseParts }; + } } - } - - if (usageMetadata) { - this.#recordPromptTokenCount(usageMetadata); - // Kick off rewrite in background (non-blocking) - if (this.messageRewriter) { - this.messageRewriter.flushTurn(ac.signal); + } catch (error) { + if (ac.signal.aborted) return; + cronHadError = true; + debugLogger.error('Error processing cron prompt:', error); + const msg = + error instanceof Error ? error.message : String(error); + await this.messageEmitter.emitAgentMessage(`[cron error] ${msg}`); + } finally { + if (this.cronAbortController === ac) { + this.cronAbortController = null; } - const durationMs = Date.now() - streamStartTime; - await this.messageEmitter.emitUsageMetadata( - usageMetadata, - '', - durationMs, - ); } - - if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( - ac.signal, - promptId, - functionCalls, - ); - nextMessage = { role: 'user', parts: toolResponseParts }; - } - } - } catch (error) { - if (ac.signal.aborted) return; - debugLogger.error('Error processing cron prompt:', error); - const msg = error instanceof Error ? error.message : String(error); - await this.messageEmitter.emitAgentMessage(`[cron error] ${msg}`); - } finally { - if (this.cronAbortController === ac) { - this.cronAbortController = null; - } - } + }, + () => (cronHadError ? 'cancelled' : 'ok'), + ); }, ); } @@ -1941,6 +1976,7 @@ export class Session implements SessionContext { let args = (fc.args ?? {}) as Record; const startTime = Date.now(); + let spanError: string | undefined; const errorResponse = (error: Error) => { const durationMs = Date.now() - startTime; @@ -1975,6 +2011,7 @@ export class Session implements SessionContext { error: Error, toolName = fc.name ?? 'unknown_tool', ) => { + spanError = error.message; if (toolName !== ToolNames.TODO_WRITE) { await this.toolCallEmitter.emitError(callId, toolName, error); } @@ -1994,595 +2031,639 @@ export class Session implements SessionContext { return earlyErrorResponse(new Error('Missing function name')); } + const toolName: string = fc.name; const toolRegistry = this.config.getToolRegistry(); - const tool = toolRegistry.getTool(fc.name as string); + const tool = toolRegistry.getTool(toolName); if (!tool) { return earlyErrorResponse( - new Error(`Tool "${fc.name}" not found in registry.`), - ); - } - - // ---- L1: Tool enablement check ---- - const pm = this.config.getPermissionManager?.(); - if (pm && !(await pm.isToolEnabled(fc.name as string))) { - return earlyErrorResponse( - new Error( - `Qwen Code requires permission to use "${fc.name}", but that permission was declined.`, - ), - fc.name, + new Error(`Tool "${toolName}" not found in registry.`), ); } - // Detect TodoWriteTool early - route to plan updates instead of tool_call events - const isTodoWriteTool = tool.name === ToolNames.TODO_WRITE; - const isAgentTool = tool.name === ToolNames.AGENT; - const isExitPlanModeTool = tool.name === ToolNames.EXIT_PLAN_MODE; - - // Track cleanup functions for sub-agent event listeners - let subAgentCleanupFunctions: Array<() => void> = []; - - // Generate tool_use_id for hook tracking (aligned with core path) - const toolUseId = generateToolUseId(); - - // Get approval mode for hook context (defined outside try for catch block access) - const approvalMode = this.config.getApprovalMode(); + const toolSpan = startToolSpan(toolName, { + 'tool.call_id': callId, + tool_name: toolName, + }); + let spanSuccess = false; try { - const invocation = tool.build(args); - - // Production AgentTool always initializes `eventEmitter` on its - // invocation (`agent.ts:392`). Be defensive about the `undefined` - // case too so an incomplete/custom AgentTool invocation degrades - // gracefully (no sub-agent event forwarding) instead of throwing - // inside SubAgentTracker.setup — the `'eventEmitter' in invocation` - // key-presence check passed for `{ eventEmitter: undefined }` and - // the ensuing `eventEmitter.on(...)` blew up. - const taskEventEmitter = ( - invocation as { - eventEmitter?: AgentEventEmitter; - } - ).eventEmitter; - if (isAgentTool && taskEventEmitter) { - // Extract subagent metadata from AgentTool call - const parentToolCallId = callId; - const subagentType = (args['subagent_type'] as string) ?? ''; - - // Create a SubAgentTracker for this tool execution - const subSubAgentTracker = new SubAgentTracker( - this, - this.client, - parentToolCallId, - subagentType, - ); - - // Set up sub-agent tool tracking - subAgentCleanupFunctions = subSubAgentTracker.setup( - taskEventEmitter, - abortSignal, - ); - } - - // L3→L4→L5 Permission Flow (aligned with coreToolScheduler) - // - // L3: Tool's intrinsic default permission - // L4: PermissionManager rule override - // L5: ApprovalMode override (YOLO / AUTO_EDIT / PLAN) - // - // AUTO_EDIT auto-approval is handled HERE, same as coreToolScheduler. - // The VS Code extension is just a UI layer for requestPermission. - const isAskUserQuestionTool = fc.name === ToolNames.ASK_USER_QUESTION; - - // ---- L3→L4: Shared permission flow ---- - const toolParams = invocation.params as Record; - const flowResult = await evaluatePermissionFlow( - this.config, - invocation, - fc.name, - toolParams, - ); - const { finalPermission, pmForcedAsk, pmCtx, denyMessage } = flowResult; - - // ---- L5: ApprovalMode overrides ---- - const isPlanMode = approvalMode === ApprovalMode.PLAN; - - if (finalPermission === 'deny') { - return earlyErrorResponse( - new Error(denyMessage ?? `Tool "${fc.name}" is denied.`), - fc.name, - ); - } - - // Explicit allow (user rule matched, or tool's L3 default is 'allow') - // is authoritative — AUTO classifier must not be allowed to override - // it. Parallels coreToolScheduler.ts:1337-1366; without this, an ACP - // session in AUTO mode could see a user-written `Bash(git push *)` - // allow rule reach the classifier and get blocked by a conservative - // Stage-1 verdict. Also resets the denialTracking streak so a - // following classifier-eligible call doesn't surprise the user with - // a manual prompt right after an allow-rule call just worked. - let autoModeAllowed = finalPermission === 'allow'; - if (autoModeAllowed && approvalMode === ApprovalMode.AUTO) { - this.config.setAutoModeDenialState( - recordAllow(this.config.getAutoModeDenialState()), - ); - } - - // ── L5: AUTO mode three-layer filter (duplicated from - // coreToolScheduler.ts; ACP routes through this Session path). - // Returns 'allowed' / 'blocked' / 'fallback'. Blocked early-returns; - // allowed skips requestPermission; fallback drops through to the - // existing manual-approval flow below. - if (!autoModeAllowed && shouldRunAutoModeForCall(approvalMode, fc.name)) { - const denialState = this.config.getAutoModeDenialState(); - // `buildClassifierContents` retains only the most recent - // MAX_TRANSCRIPT_MESSAGES messages; ask the chat client for - // exactly that tail rather than triggering a `structuredClone` - // of the whole session on every non-fast-path AUTO call. - // Parallels coreToolScheduler.ts. - const messages = - this.config - .getGeminiClient?.() - ?.getHistoryTail(MAX_TRANSCRIPT_MESSAGES, false) ?? []; - const decision = await evaluateAutoMode({ - ctx: pmCtx, - pmForcedAsk, - toolParams, - messages, - config: this.config, - signal: abortSignal, - skipClassifier: shouldFallback(denialState).fallback, - }); - - // Apply decision via shared helper — eliminates ~40 lines of - // line-for-line duplication with coreToolScheduler.ts and makes - // the CLI / ACP paths share one source of truth for the - // switch + denial-tracking state updates + exhaustiveness - // guard. - const outcome = applyAutoModeDecision( - decision, - this.config, - denialState, - ); - switch (outcome.kind) { - case 'approved': - autoModeAllowed = true; - break; - case 'blocked': - return earlyErrorResponse(new Error(outcome.errorMessage), fc.name); - case 'fallback': - // Drop through to the manual-approval flow below. - break; - default: { - const _exhaustive: never = outcome; - void _exhaustive; - } - } - } - - let didRequestPermission = false; - let confirmationDetails: ToolCallConfirmationDetails | undefined; - - if ( - !autoModeAllowed && - needsConfirmation(finalPermission, approvalMode, fc.name) - ) { - confirmationDetails = - await invocation.getConfirmationDetails(abortSignal); - - // Centralised rule injection (for display and persistence) - injectPermissionRulesIfMissing(confirmationDetails, pmCtx); - - if ( - isPlanModeBlocked( - isPlanMode, - isExitPlanModeTool, - isAskUserQuestionTool, - confirmationDetails, - ) - ) { + return await runInToolSpanContext(toolSpan, async () => { + // ---- L1: Tool enablement check ---- + const pm = this.config.getPermissionManager?.(); + if (pm && !(await pm.isToolEnabled(toolName))) { return earlyErrorResponse( new Error( - `Plan mode is active. The tool "${fc.name}" cannot be executed because it modifies the system. ` + - 'Please use the exit_plan_mode tool to present your plan and exit plan mode before making changes.', + `Qwen Code requires permission to use "${toolName}", but that permission was declined.`, ), - fc.name, + toolName, ); } - const messageBus = this.config.getMessageBus?.(); - const hooksEnabled = !this.config.getDisableAllHooks?.(); - let hookHandled = false; + // Detect TodoWriteTool early - route to plan updates instead of tool_call events + const isTodoWriteTool = tool.name === ToolNames.TODO_WRITE; + const isAgentTool = tool.name === ToolNames.AGENT; + const isExitPlanModeTool = tool.name === ToolNames.EXIT_PLAN_MODE; - if (hooksEnabled && messageBus) { - const hookResult = await firePermissionRequestHook( - messageBus, - fc.name, - args, - String(approvalMode), - ); + // Track cleanup functions for sub-agent event listeners + let subAgentCleanupFunctions: Array<() => void> = []; - if (hookResult.hasDecision) { - hookHandled = true; - if (hookResult.shouldAllow) { - if (hookResult.updatedInput) { - args = hookResult.updatedInput; - invocation.params = - hookResult.updatedInput as typeof invocation.params; - } + // Generate tool_use_id for hook tracking (aligned with core path) + const toolUseId = generateToolUseId(); - await confirmationDetails.onConfirm( - ToolConfirmationOutcome.ProceedOnce, - ); - } else { - return earlyErrorResponse( - new Error( - hookResult.denyMessage || - `Permission denied by hook for "${fc.name}"`, - ), - fc.name, - ); + // Get approval mode for hook context (defined outside try for catch block access) + const approvalMode = this.config.getApprovalMode(); + + try { + const invocation = tool.build(args); + + // Production AgentTool always initializes `eventEmitter` on its + // invocation (`agent.ts:392`). Be defensive about the `undefined` + // case too so an incomplete/custom AgentTool invocation degrades + // gracefully (no sub-agent event forwarding) instead of throwing + // inside SubAgentTracker.setup — the `'eventEmitter' in invocation` + // key-presence check passed for `{ eventEmitter: undefined }` and + // the ensuing `eventEmitter.on(...)` blew up. + const taskEventEmitter = ( + invocation as { + eventEmitter?: AgentEventEmitter; } + ).eventEmitter; + if (isAgentTool && taskEventEmitter) { + // Extract subagent metadata from AgentTool call + const parentToolCallId = callId; + const subagentType = (args['subagent_type'] as string) ?? ''; + + // Create a SubAgentTracker for this tool execution + const subSubAgentTracker = new SubAgentTracker( + this, + this.client, + parentToolCallId, + subagentType, + ); + + // Set up sub-agent tool tracking + subAgentCleanupFunctions = subSubAgentTracker.setup( + taskEventEmitter, + abortSignal, + ); } - } - // AUTO_EDIT mode: auto-approve edit and info tools - // (same as coreToolScheduler L5 — NOT delegated to the extension) - if ( - approvalMode === ApprovalMode.AUTO_EDIT && - (confirmationDetails.type === 'edit' || - confirmationDetails.type === 'info') - ) { - // Auto-approve, skip requestPermission. - // didRequestPermission stays false → emitStart below. - } else if (!hookHandled) { - // Show permission dialog via ACP requestPermission - didRequestPermission = true; - const content = buildPermissionRequestContent(confirmationDetails); - - // Map tool kind, using switch_mode for exit_plan_mode per ACP spec - const mappedKind = this.toolCallEmitter.mapToolKind( - tool.kind, - fc.name, + // L3→L4→L5 Permission Flow (aligned with coreToolScheduler) + // + // L3: Tool's intrinsic default permission + // L4: PermissionManager rule override + // L5: ApprovalMode override (YOLO / AUTO_EDIT / PLAN) + // + // AUTO_EDIT auto-approval is handled HERE, same as coreToolScheduler. + // The VS Code extension is just a UI layer for requestPermission. + const isAskUserQuestionTool = + toolName === ToolNames.ASK_USER_QUESTION; + + // ---- L3→L4: Shared permission flow ---- + const toolParams = invocation.params as Record; + const flowResult = await evaluatePermissionFlow( + this.config, + invocation, + toolName, + toolParams, ); + const { finalPermission, pmForcedAsk, pmCtx, denyMessage } = + flowResult; + + // ---- L5: ApprovalMode overrides ---- + const isPlanMode = approvalMode === ApprovalMode.PLAN; - if (hooksEnabled && messageBus) { - void fireNotificationHook( - messageBus, - `Qwen Code needs your permission to use ${fc.name}`, - NotificationType.PermissionPrompt, - 'Permission needed', + if (finalPermission === 'deny') { + return earlyErrorResponse( + new Error(denyMessage ?? `Tool "${toolName}" is denied.`), + toolName, ); } - const params: RequestPermissionRequest = { - sessionId: this.sessionId, - options: toPermissionOptions(confirmationDetails, pmForcedAsk), - toolCall: { - toolCallId: callId, - status: 'pending', - title: invocation.getDescription(), - content, - locations: invocation.toolLocations(), - kind: mappedKind, - rawInput: args, - }, - }; - - const output = (await this.client.requestPermission( - params, - )) as RequestPermissionResponse & { - answers?: Record; - }; - const outcome = - output.outcome.outcome === 'cancelled' - ? ToolConfirmationOutcome.Cancel - : z - .nativeEnum(ToolConfirmationOutcome) - .parse(output.outcome.optionId); - - // Reset the AUTO-mode fallback streak when the user manually - // approves a prompt that was raised because denialTracking forced - // fallback. Without this, a single block-streak permanently - // downgrades the rest of the session to manual approval until the - // mode is toggled. Parallels coreToolScheduler.ts:1705-1717. - // Cancel / abort do NOT reset — treating rejection as a signal - // the classifier was right to block. - if (approvalMode === ApprovalMode.AUTO && isApproveOutcome(outcome)) { + // Explicit allow (user rule matched, or tool's L3 default is 'allow') + // is authoritative — AUTO classifier must not be allowed to override + // it. Parallels coreToolScheduler.ts:1337-1366; without this, an ACP + // session in AUTO mode could see a user-written `Bash(git push *)` + // allow rule reach the classifier and get blocked by a conservative + // Stage-1 verdict. Also resets the denialTracking streak so a + // following classifier-eligible call doesn't surprise the user with + // a manual prompt right after an allow-rule call just worked. + let autoModeAllowed = finalPermission === 'allow'; + if (autoModeAllowed && approvalMode === ApprovalMode.AUTO) { this.config.setAutoModeDenialState( - recordFallbackApprove(this.config.getAutoModeDenialState()), + recordAllow(this.config.getAutoModeDenialState()), ); } - await confirmationDetails.onConfirm(outcome, { - answers: output.answers, - }); - - // Persist permission rules when user explicitly chose "Always Allow". - // This branch is only reached for tools that went through - // requestPermission (user saw dialog and made a choice). - // AUTO_EDIT auto-approved tools never reach here. + // ── L5: AUTO mode three-layer filter (duplicated from + // coreToolScheduler.ts; ACP routes through this Session path). + // Returns 'allowed' / 'blocked' / 'fallback'. Blocked early-returns; + // allowed skips requestPermission; fallback drops through to the + // existing manual-approval flow below. if ( - outcome === ToolConfirmationOutcome.ProceedAlways || - outcome === ToolConfirmationOutcome.ProceedAlwaysProject || - outcome === ToolConfirmationOutcome.ProceedAlwaysUser + !autoModeAllowed && + shouldRunAutoModeForCall(approvalMode, toolName) ) { - await persistPermissionOutcome( - outcome, - confirmationDetails, - this.config.getOnPersistPermissionRule?.(), - this.config.getPermissionManager?.(), - { answers: output.answers }, + const denialState = this.config.getAutoModeDenialState(); + // `buildClassifierContents` retains only the most recent + // MAX_TRANSCRIPT_MESSAGES messages; ask the chat client for + // exactly that tail rather than triggering a `structuredClone` + // of the whole session on every non-fast-path AUTO call. + // Parallels coreToolScheduler.ts. + const messages = + this.config + .getGeminiClient?.() + ?.getHistoryTail(MAX_TRANSCRIPT_MESSAGES, false) ?? []; + const decision = await evaluateAutoMode({ + ctx: pmCtx, + pmForcedAsk, + toolParams, + messages, + config: this.config, + signal: abortSignal, + skipClassifier: shouldFallback(denialState).fallback, + }); + + // Apply decision via shared helper — eliminates ~40 lines of + // line-for-line duplication with coreToolScheduler.ts and makes + // the CLI / ACP paths share one source of truth for the + // switch + denial-tracking state updates + exhaustiveness + // guard. + const outcome = applyAutoModeDecision( + decision, + this.config, + denialState, ); + switch (outcome.kind) { + case 'approved': + autoModeAllowed = true; + break; + case 'blocked': + return earlyErrorResponse( + new Error(outcome.errorMessage), + toolName, + ); + case 'fallback': + // Drop through to the manual-approval flow below. + break; + default: { + const _exhaustive: never = outcome; + void _exhaustive; + } + } } - // After exit_plan_mode confirmation, send current_mode_update - if ( - isExitPlanModeTool && - outcome !== ToolConfirmationOutcome.Cancel - ) { - await this.sendCurrentModeUpdateNotification(outcome); - } + let didRequestPermission = false; + let confirmationDetails: ToolCallConfirmationDetails | undefined; - // After edit tool ProceedAlways, notify the client about mode change if ( - confirmationDetails.type === 'edit' && - outcome === ToolConfirmationOutcome.ProceedAlways + !autoModeAllowed && + needsConfirmation(finalPermission, approvalMode, toolName) ) { - await this.sendCurrentModeUpdateNotification(outcome); - } + confirmationDetails = + await invocation.getConfirmationDetails(abortSignal); - switch (outcome) { - case ToolConfirmationOutcome.Cancel: - return errorResponse( - new Error(`Tool "${fc.name}" was canceled by the user.`), + // Centralised rule injection (for display and persistence) + injectPermissionRulesIfMissing(confirmationDetails, pmCtx); + + if ( + isPlanModeBlocked( + isPlanMode, + isExitPlanModeTool, + isAskUserQuestionTool, + confirmationDetails, + ) + ) { + return earlyErrorResponse( + new Error( + `Plan mode is active. The tool "${toolName}" cannot be executed because it modifies the system. ` + + 'Please use the exit_plan_mode tool to present your plan and exit plan mode before making changes.', + ), + toolName, ); - case ToolConfirmationOutcome.ProceedOnce: - case ToolConfirmationOutcome.ProceedAlways: - case ToolConfirmationOutcome.ProceedAlwaysProject: - case ToolConfirmationOutcome.ProceedAlwaysUser: - case ToolConfirmationOutcome.ProceedAlwaysServer: - case ToolConfirmationOutcome.ProceedAlwaysTool: - case ToolConfirmationOutcome.ModifyWithEditor: - case ToolConfirmationOutcome.RestorePrevious: - break; - default: { - const resultOutcome: never = outcome; - throw new Error(`Unexpected: ${resultOutcome}`); } - } - } - } - if (!didRequestPermission && !isTodoWriteTool) { - // Auto-approved (L3 allow / L4 PM allow / L5 YOLO|AUTO_EDIT) - // → emit tool_call start notification - const startParams: ToolCallStartParams = { - callId, - toolName: fc.name, - args, - status: 'in_progress', - }; - await this.toolCallEmitter.emitStart(startParams); - } + const messageBus = this.config.getMessageBus?.(); + const hooksEnabled = !this.config.getDisableAllHooks?.(); + let hookHandled = false; + + if (hooksEnabled && messageBus) { + const hookResult = await firePermissionRequestHook( + messageBus, + toolName, + args, + String(approvalMode), + ); - // Fire PreToolUse hook (aligned with core path in coreToolScheduler.ts) - const hooksEnabledForTool = !this.config.getDisableAllHooks?.(); - const messageBusForTool = this.config.getMessageBus?.(); - const permissionMode = String(approvalMode); - - if (hooksEnabledForTool && messageBusForTool) { - const preHookResult = await firePreToolUseHook( - messageBusForTool, - fc.name, - args, - toolUseId, - permissionMode, - abortSignal, - ); + if (hookResult.hasDecision) { + hookHandled = true; + if (hookResult.shouldAllow) { + if (hookResult.updatedInput) { + args = hookResult.updatedInput; + invocation.params = + hookResult.updatedInput as typeof invocation.params; + } - if (!preHookResult.shouldProceed) { - // Hook blocked the tool execution - send notification to UI - const blockReason = - preHookResult.blockReason || 'Blocked by PreToolUse hook'; - await this.messageEmitter.emitAgentMessage( - `🚫 **PreToolUse blocked**: ${fc.name} - ${blockReason}`, - ); - return earlyErrorResponse(new Error(blockReason), fc.name); - } + await confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + } else { + return earlyErrorResponse( + new Error( + hookResult.denyMessage || + `Permission denied by hook for "${toolName}"`, + ), + toolName, + ); + } + } + } - // Add additional context from PreToolUse hook if provided - // Note: This context would need to be passed to the tool invocation - // For now, we just log it as the tool execution proceeds - if (preHookResult.additionalContext) { - debugLogger.debug( - `PreToolUse hook additional context for ${fc.name}: ${preHookResult.additionalContext}`, - ); - } - } + // AUTO_EDIT mode: auto-approve edit and info tools + // (same as coreToolScheduler L5 — NOT delegated to the extension) + if ( + approvalMode === ApprovalMode.AUTO_EDIT && + (confirmationDetails.type === 'edit' || + confirmationDetails.type === 'info') + ) { + // Auto-approve, skip requestPermission. + // didRequestPermission stays false → emitStart below. + } else if (!hookHandled) { + // Show permission dialog via ACP requestPermission + didRequestPermission = true; + const content = + buildPermissionRequestContent(confirmationDetails); + + // Map tool kind, using switch_mode for exit_plan_mode per ACP spec + const mappedKind = this.toolCallEmitter.mapToolKind( + tool.kind, + toolName, + ); - const toolResult: ToolResult = await invocation.execute(abortSignal); + if (hooksEnabled && messageBus) { + void fireNotificationHook( + messageBus, + `Qwen Code needs your permission to use ${toolName}`, + NotificationType.PermissionPrompt, + 'Permission needed', + ); + } - // Clean up event listeners - subAgentCleanupFunctions.forEach((cleanup) => cleanup()); + const params: RequestPermissionRequest = { + sessionId: this.sessionId, + options: toPermissionOptions(confirmationDetails, pmForcedAsk), + toolCall: { + toolCallId: callId, + status: 'pending', + title: invocation.getDescription(), + content, + locations: invocation.toolLocations(), + kind: mappedKind, + rawInput: args, + }, + }; + + const output = (await this.client.requestPermission( + params, + )) as RequestPermissionResponse & { + answers?: Record; + }; + const outcome = + output.outcome.outcome === 'cancelled' + ? ToolConfirmationOutcome.Cancel + : z + .nativeEnum(ToolConfirmationOutcome) + .parse(output.outcome.optionId); + + // Reset the AUTO-mode fallback streak when the user manually + // approves a prompt that was raised because denialTracking forced + // fallback. Without this, a single block-streak permanently + // downgrades the rest of the session to manual approval until the + // mode is toggled. Parallels coreToolScheduler.ts:1705-1717. + // Cancel / abort do NOT reset — treating rejection as a signal + // the classifier was right to block. + if ( + approvalMode === ApprovalMode.AUTO && + isApproveOutcome(outcome) + ) { + this.config.setAutoModeDenialState( + recordFallbackApprove(this.config.getAutoModeDenialState()), + ); + } - // Create response parts first (needed for emitResult and recordToolResult) - const responseParts = convertToFunctionResponse( - fc.name, - callId, - toolResult.llmContent, - ); + await confirmationDetails.onConfirm(outcome, { + answers: output.answers, + }); - // Fire PostToolUse hook on successful execution (aligned with core path) - if (hooksEnabledForTool && messageBusForTool && !toolResult.error) { - // Use the same response shape as core (llmContent/returnDisplay) - const toolResponse = { - llmContent: toolResult.llmContent, - returnDisplay: toolResult.returnDisplay, - }; - const postHookResult = await firePostToolUseHook( - messageBusForTool, - fc.name, - args, - toolResponse, - toolUseId, - permissionMode, - abortSignal, - ); + // Persist permission rules when user explicitly chose "Always Allow". + // This branch is only reached for tools that went through + // requestPermission (user saw dialog and made a choice). + // AUTO_EDIT auto-approved tools never reach here. + if ( + outcome === ToolConfirmationOutcome.ProceedAlways || + outcome === ToolConfirmationOutcome.ProceedAlwaysProject || + outcome === ToolConfirmationOutcome.ProceedAlwaysUser + ) { + await persistPermissionOutcome( + outcome, + confirmationDetails, + this.config.getOnPersistPermissionRule?.(), + this.config.getPermissionManager?.(), + { answers: output.answers }, + ); + } - // If hook indicates to stop, return an error response - if (postHookResult.shouldStop) { - const stopMessage = - postHookResult.stopReason || - 'Execution stopped by PostToolUse hook'; - debugLogger.info( - `PostToolUse hook requested stop for ${fc.name}: ${stopMessage}`, - ); - return earlyErrorResponse(new Error(stopMessage), fc.name); - } + // After exit_plan_mode confirmation, send current_mode_update + if ( + isExitPlanModeTool && + outcome !== ToolConfirmationOutcome.Cancel + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } - // Add additional context from PostToolUse hook if provided - if (postHookResult.additionalContext) { - // Append additional context to the tool response - const contextPart = { text: postHookResult.additionalContext }; - responseParts.push(contextPart); - } - } else if (hooksEnabledForTool && messageBusForTool && toolResult.error) { - // Fire PostToolUseFailure hook when tool returns an error (aligned with core path) - const failureHookResult = await firePostToolUseFailureHook( - messageBusForTool, - toolUseId, - fc.name ?? 'unknown_tool', - args, - toolResult.error.message, - false, // not an interrupt - permissionMode, - abortSignal, - ); + // After edit tool ProceedAlways, notify the client about mode change + if ( + confirmationDetails.type === 'edit' && + outcome === ToolConfirmationOutcome.ProceedAlways + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } - // Log additional context if provided - if (failureHookResult.additionalContext) { - debugLogger.debug( - `PostToolUseFailure hook additional context for ${fc.name}: ${failureHookResult.additionalContext}`, - ); - } - } + switch (outcome) { + case ToolConfirmationOutcome.Cancel: + return errorResponse( + new Error(`Tool "${toolName}" was canceled by the user.`), + ); + case ToolConfirmationOutcome.ProceedOnce: + case ToolConfirmationOutcome.ProceedAlways: + case ToolConfirmationOutcome.ProceedAlwaysProject: + case ToolConfirmationOutcome.ProceedAlwaysUser: + case ToolConfirmationOutcome.ProceedAlwaysServer: + case ToolConfirmationOutcome.ProceedAlwaysTool: + case ToolConfirmationOutcome.ModifyWithEditor: + case ToolConfirmationOutcome.RestorePrevious: + break; + default: { + const resultOutcome: never = outcome; + throw new Error(`Unexpected: ${resultOutcome}`); + } + } + } + } - // Handle TodoWriteTool: extract todos and send plan update - if (isTodoWriteTool) { - const todos = this.planEmitter.extractTodos( - toolResult.returnDisplay, - args, - ); + if (!didRequestPermission && !isTodoWriteTool) { + // Auto-approved (L3 allow / L4 PM allow / L5 YOLO|AUTO_EDIT) + // → emit tool_call start notification + const startParams: ToolCallStartParams = { + callId, + toolName, + args, + status: 'in_progress', + }; + await this.toolCallEmitter.emitStart(startParams); + } - // Match original logic: emit plan if todos.length > 0 OR if args had todos - if ((todos && todos.length > 0) || Array.isArray(args['todos'])) { - await this.planEmitter.emitPlan(todos ?? []); - } + // Fire PreToolUse hook (aligned with core path in coreToolScheduler.ts) + const hooksEnabledForTool = !this.config.getDisableAllHooks?.(); + const messageBusForTool = this.config.getMessageBus?.(); + const permissionMode = String(approvalMode); + + if (hooksEnabledForTool && messageBusForTool) { + const preHookResult = await firePreToolUseHook( + messageBusForTool, + toolName, + args, + toolUseId, + permissionMode, + abortSignal, + ); - // Skip tool_call_update event for TodoWriteTool - // Still log and return function response for LLM - } else { - // Normal tool handling: emit result using ToolCallEmitter - // Convert toolResult.error to Error type if present - const error = toolResult.error - ? new Error(toolResult.error.message) - : undefined; - - await this.toolCallEmitter.emitResult({ - callId, - toolName: fc.name, - args, - message: responseParts, - resultDisplay: toolResult.returnDisplay, - error, - success: !toolResult.error, - }); - } + if (!preHookResult.shouldProceed) { + // Hook blocked the tool execution - send notification to UI + const blockReason = + preHookResult.blockReason || 'Blocked by PreToolUse hook'; + await this.messageEmitter.emitAgentMessage( + `🚫 **PreToolUse blocked**: ${toolName} - ${blockReason}`, + ); + return earlyErrorResponse(new Error(blockReason), toolName); + } - const durationMs = Date.now() - startTime; - logToolCall(this.config, { - 'event.name': 'tool_call', - 'event.timestamp': new Date().toISOString(), - function_name: fc.name, - function_args: args, - duration_ms: durationMs, - status: 'success', - success: true, - prompt_id: promptId, - tool_type: - typeof tool !== 'undefined' && tool instanceof DiscoveredMCPTool - ? 'mcp' - : 'native', - }); + // Add additional context from PreToolUse hook if provided + // Note: This context would need to be passed to the tool invocation + // For now, we just log it as the tool execution proceeds + if (preHookResult.additionalContext) { + debugLogger.debug( + `PreToolUse hook additional context for ${toolName}: ${preHookResult.additionalContext}`, + ); + } + } - // Record tool result for session management - this.config.getChatRecordingService()?.recordToolResult(responseParts, { - callId, - status: 'success', - resultDisplay: toolResult.returnDisplay, - error: undefined, - errorType: undefined, - }); + const execSpan = startToolExecutionSpan(); + let toolResult: ToolResult; + try { + toolResult = await invocation.execute(abortSignal); + endToolExecutionSpan(execSpan, { + success: !toolResult.error, + error: toolResult.error ? 'tool_error' : undefined, + }); + } catch (execError) { + endToolExecutionSpan(execSpan, { + success: false, + error: abortSignal.aborted ? 'tool_cancelled' : 'tool_exception', + cancelled: abortSignal.aborted, + }); + throw execError; + } - return responseParts; - } catch (e) { - // Ensure cleanup on error - subAgentCleanupFunctions.forEach((cleanup) => cleanup()); - - const error = e instanceof Error ? e : new Error(String(e)); - - // Fire PostToolUseFailure hook (aligned with core path in coreToolScheduler.ts) - const hooksEnabledForError = !this.config.getDisableAllHooks?.(); - const messageBusForError = this.config.getMessageBus?.(); - const isInterrupt = abortSignal.aborted; - - if (hooksEnabledForError && messageBusForError) { - const failureHookResult = await firePostToolUseFailureHook( - messageBusForError, - toolUseId, - fc.name ?? 'unknown_tool', - args, - error.message, - isInterrupt, - String(approvalMode), - abortSignal, - ); + // Clean up event listeners + subAgentCleanupFunctions.forEach((cleanup) => cleanup()); - // Log additional context if provided - if (failureHookResult.additionalContext) { - debugLogger.debug( - `PostToolUseFailure hook additional context for ${fc.name}: ${failureHookResult.additionalContext}`, + // Create response parts first (needed for emitResult and recordToolResult) + const responseParts = convertToFunctionResponse( + toolName, + callId, + toolResult.llmContent, ); - } - } - // Use ToolCallEmitter for error handling - await this.toolCallEmitter.emitError( - callId, - fc.name ?? 'unknown_tool', - error, - ); + // Fire PostToolUse hook on successful execution (aligned with core path) + if (hooksEnabledForTool && messageBusForTool && !toolResult.error) { + // Use the same response shape as core (llmContent/returnDisplay) + const toolResponse = { + llmContent: toolResult.llmContent, + returnDisplay: toolResult.returnDisplay, + }; + const postHookResult = await firePostToolUseHook( + messageBusForTool, + toolName, + args, + toolResponse, + toolUseId, + permissionMode, + abortSignal, + ); - // Record tool error for session management - const errorParts = [ - { - functionResponse: { - id: callId, - name: fc.name ?? '', - response: { error: error.message }, - }, - }, - ]; - this.config.getChatRecordingService()?.recordToolResult(errorParts, { - callId, - status: 'error', - resultDisplay: undefined, - error, - errorType: undefined, - }); + // If hook indicates to stop, return an error response + if (postHookResult.shouldStop) { + const stopMessage = + postHookResult.stopReason || + 'Execution stopped by PostToolUse hook'; + debugLogger.info( + `PostToolUse hook requested stop for ${toolName}: ${stopMessage}`, + ); + return earlyErrorResponse(new Error(stopMessage), toolName); + } + + // Add additional context from PostToolUse hook if provided + if (postHookResult.additionalContext) { + // Append additional context to the tool response + const contextPart = { text: postHookResult.additionalContext }; + responseParts.push(contextPart); + } + } else if ( + hooksEnabledForTool && + messageBusForTool && + toolResult.error + ) { + // Fire PostToolUseFailure hook when tool returns an error (aligned with core path) + const failureHookResult = await firePostToolUseFailureHook( + messageBusForTool, + toolUseId, + toolName, + args, + toolResult.error.message, + false, // not an interrupt + permissionMode, + abortSignal, + ); + + // Log additional context if provided + if (failureHookResult.additionalContext) { + debugLogger.debug( + `PostToolUseFailure hook additional context for ${toolName}: ${failureHookResult.additionalContext}`, + ); + } + } - return errorResponse(error); + // Handle TodoWriteTool: extract todos and send plan update + if (isTodoWriteTool) { + const todos = this.planEmitter.extractTodos( + toolResult.returnDisplay, + args, + ); + + // Match original logic: emit plan if todos.length > 0 OR if args had todos + if ((todos && todos.length > 0) || Array.isArray(args['todos'])) { + await this.planEmitter.emitPlan(todos ?? []); + } + + // Skip tool_call_update event for TodoWriteTool + // Still log and return function response for LLM + } else { + // Normal tool handling: emit result using ToolCallEmitter + // Convert toolResult.error to Error type if present + const error = toolResult.error + ? new Error(toolResult.error.message) + : undefined; + + await this.toolCallEmitter.emitResult({ + callId, + toolName, + args, + message: responseParts, + resultDisplay: toolResult.returnDisplay, + error, + success: !toolResult.error, + }); + } + + const durationMs = Date.now() - startTime; + logToolCall(this.config, { + 'event.name': 'tool_call', + 'event.timestamp': new Date().toISOString(), + function_name: toolName, + function_args: args, + duration_ms: durationMs, + status: 'success', + success: true, + prompt_id: promptId, + tool_type: + typeof tool !== 'undefined' && tool instanceof DiscoveredMCPTool + ? 'mcp' + : 'native', + }); + + // Record tool result for session management + this.config + .getChatRecordingService() + ?.recordToolResult(responseParts, { + callId, + status: 'success', + resultDisplay: toolResult.returnDisplay, + error: undefined, + errorType: undefined, + }); + + spanSuccess = true; + return responseParts; + } catch (e) { + // Ensure cleanup on error + subAgentCleanupFunctions.forEach((cleanup) => cleanup()); + + const error = e instanceof Error ? e : new Error(String(e)); + spanError = error.message; + + // Fire PostToolUseFailure hook (aligned with core path in coreToolScheduler.ts) + const hooksEnabledForError = !this.config.getDisableAllHooks?.(); + const messageBusForError = this.config.getMessageBus?.(); + const isInterrupt = abortSignal.aborted; + + if (hooksEnabledForError && messageBusForError) { + const failureHookResult = await firePostToolUseFailureHook( + messageBusForError, + toolUseId, + toolName, + args, + error.message, + isInterrupt, + String(approvalMode), + abortSignal, + ); + + // Log additional context if provided + if (failureHookResult.additionalContext) { + debugLogger.debug( + `PostToolUseFailure hook additional context for ${toolName}: ${failureHookResult.additionalContext}`, + ); + } + } + + // Use ToolCallEmitter for error handling + await this.toolCallEmitter.emitError(callId, toolName, error); + + // Record tool error for session management + const errorParts = [ + { + functionResponse: { + id: callId, + name: toolName, + response: { error: error.message }, + }, + }, + ]; + this.config.getChatRecordingService()?.recordToolResult(errorParts, { + callId, + status: 'error', + resultDisplay: undefined, + error, + errorType: undefined, + }); + + return errorResponse(error); + } + }); // end runInToolSpanContext + } finally { + endToolSpan(toolSpan, { success: spanSuccess, error: spanError }); } } diff --git a/packages/core/src/telemetry/session-tracing.ts b/packages/core/src/telemetry/session-tracing.ts index bc5fc68ea02..71052733d15 100644 --- a/packages/core/src/telemetry/session-tracing.ts +++ b/packages/core/src/telemetry/session-tracing.ts @@ -26,7 +26,11 @@ import { } from './constants.js'; import { clearDetailedSpanState } from './detailed-span-attributes.js'; import { isTelemetrySdkInitialized } from './sdk.js'; -import { getSessionContext, setSessionContext } from './session-context.js'; +import { + getCurrentSessionId, + getSessionContext, + setSessionContext, +} from './session-context.js'; import { createDebugLogger } from '../utils/debugLogger.js'; const debugLogger = createDebugLogger('SESSION_TRACING'); @@ -437,7 +441,9 @@ export function startLLMRequestSpan(model: string, promptId: string): Span { // attaches to the tool span instead of skipping back to the session root. const ctx = resolveParentContext(parentCtx); + const sessionId = getCurrentSessionId(); const attributes: Attributes = { + ...(sessionId ? { 'session.id': sessionId } : {}), 'qwen-code.model': model, 'qwen-code.prompt_id': promptId, 'llm_request.context': parentCtx ? 'interaction' : 'standalone', @@ -585,7 +591,9 @@ export function startToolSpan( // tools-inside-tools cases before falling back to the session root. const ctx = resolveParentContext(parentCtx); + const sessionId = getCurrentSessionId(); const attributes: Attributes = { + ...(sessionId ? { 'session.id': sessionId } : {}), 'tool.name': toolName, ...attrs, }; @@ -699,9 +707,13 @@ export function startToolExecutionSpan(): Span { // subsystem) before falling back to the session root. const ctx = resolveParentContext(parentCtx); + const sessionId = getCurrentSessionId(); const span = getTracer().startSpan( SPAN_TOOL_EXECUTION, - { kind: SpanKind.INTERNAL }, + { + kind: SpanKind.INTERNAL, + attributes: sessionId ? { 'session.id': sessionId } : {}, + }, ctx, ); @@ -709,7 +721,7 @@ export function startToolExecutionSpan(): Span { const spanContextObj: SpanContext = { span, startTime: Date.now(), - attributes: {}, + attributes: sessionId ? { 'session.id': sessionId } : {}, type: 'tool.execution', }; activeSpans.set(spanId, new WeakRef(spanContextObj)); From a814e956dd44878940cbdadad1dcba207a90b721 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 29 May 2026 07:32:58 +0800 Subject: [PATCH 2/6] =?UTF-8?q?fix(telemetry):=20address=20Copilot=20revie?= =?UTF-8?q?w=20=E2=80=94=20cron=20abort=20status=20+=20exec=20span=20cance?= =?UTF-8?q?llation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cron path getResultStatus now checks ac.signal.aborted so aborted cron runs record turn_status='cancelled' instead of 'ok' - Tool execution span success path now checks abortSignal.aborted, aligning with coreToolScheduler cancellation semantics --- .../cli/src/acp-integration/session/Session.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index a188d355b17..63c6dd59b6a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1682,7 +1682,8 @@ export class Session implements SessionContext { } } }, - () => (cronHadError ? 'cancelled' : 'ok'), + () => + ac.signal.aborted ? 'cancelled' : cronHadError ? 'cancelled' : 'ok', ); }, ); @@ -2031,7 +2032,7 @@ export class Session implements SessionContext { return earlyErrorResponse(new Error('Missing function name')); } - const toolName: string = fc.name; + const toolName = fc.name; const toolRegistry = this.config.getToolRegistry(); const tool = toolRegistry.getTool(toolName); @@ -2463,9 +2464,15 @@ export class Session implements SessionContext { let toolResult: ToolResult; try { toolResult = await invocation.execute(abortSignal); + const aborted = abortSignal.aborted; endToolExecutionSpan(execSpan, { - success: !toolResult.error, - error: toolResult.error ? 'tool_error' : undefined, + success: !toolResult.error && !aborted, + error: aborted + ? 'tool_cancelled' + : toolResult.error + ? 'tool_error' + : undefined, + cancelled: aborted, }); } catch (execError) { endToolExecutionSpan(execSpan, { From 5712e691217cf970efa638a1bc6cefa36e4e7187 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 30 May 2026 21:32:55 +0800 Subject: [PATCH 3/6] fix(telemetry): correct session.id, span outcomes, conversation_finished coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address wenshao + Copilot review on the daemon/ACP telemetry path. - session-tracing.ts: derive session.id for llm_request/tool/tool.execution spans from the per-session parent span context (resolveSessionId) instead of the process-global getCurrentSessionId(). A daemon hosts many sessions in one process, so the global cross-stamped child spans with whichever session last initialized telemetry while the interaction span carried the correct id. Falls back to the global for the single-session CLI path. [wenshao Critical] - Session.ts #executePrompt: move logConversationFinishedEvent into a finally wrapping the whole turn loop so cancelled / no-stream / API-error / rate-limit terminal paths also emit (previously only the clean stop-hook path did). Emitted for all approval modes — an intentional divergence from the CLI's YOLO-only gating, since daemon turns run autonomously regardless of mode. [wenshao Critical + Suggestion, Copilot turnCount] - Session.ts #executeCronPrompt: emit conversation_finished on every terminal cron path (clean / abort / caught error). [wenshao Critical] - Session.ts runTool success path: reflect toolResult.error and cancellation in logToolCall / recordToolResult / the tool span instead of hardcoding success, so soft tool failures are no longer mislabeled as successful. [Copilot] - Session.ts tool-confirmation Cancel: route through earlyErrorResponse so spanError carries the cancellation reason (was the generic 'tool error') and the declined call is recorded. [wenshao Suggestion] - Session.ts startToolSpan: dual-emit the legacy call_id alias like CoreToolScheduler for backwards-compat dashboards. [Copilot] --- .../src/acp-integration/session/Session.ts | 292 ++++++++++-------- .../core/src/telemetry/session-tracing.ts | 31 +- 2 files changed, 197 insertions(+), 126 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 63c6dd59b6a..c80720b313a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -838,145 +838,152 @@ export class Session implements SessionContext { let nextMessage: Content | null = { role: 'user', parts }; let turnCount = 0; - while (nextMessage !== null) { - turnCount++; - if (pendingSend.signal.aborted) { - this.#getCurrentChat().addHistory(nextMessage); - return { stopReason: 'cancelled' }; - } - - const functionCalls: FunctionCall[] = []; - let usageMetadata: GenerateContentResponseUsageMetadata | null = - null; - const streamStartTime = Date.now(); - - try { - const sendResult = - await this.#sendMessageStreamWithAutoCompression( - promptId, - nextMessage?.parts ?? [], - pendingSend.signal, - ); - if (!sendResult.responseStream) { - this.#preserveUnsentMessageHistory( - nextMessage, - sendResult.stopReason === 'cancelled', - ); - return { stopReason: sendResult.stopReason }; + // conversation_finished must fire on every terminal path of the + // turn — the loop below has cancel/abort/no-stream early-returns + // and API-error throws — so the emission lives in a finally that + // wraps the whole turn, not just the stop-hook loop. Daemon turns + // run autonomously in all approval modes (approvals are mediated by + // the ACP client rather than by gating this loop), so unlike the + // CLI reference (useGeminiStream.ts, which only emits in YOLO) this + // is intentionally emitted for every mode. + try { + while (nextMessage !== null) { + turnCount++; + if (pendingSend.signal.aborted) { + this.#getCurrentChat().addHistory(nextMessage); + return { stopReason: 'cancelled' }; } - const responseStream = sendResult.responseStream; - nextMessage = null; - for await (const resp of responseStream) { - if (pendingSend.signal.aborted) { - return { stopReason: 'cancelled' }; + const functionCalls: FunctionCall[] = []; + let usageMetadata: GenerateContentResponseUsageMetadata | null = + null; + const streamStartTime = Date.now(); + + try { + const sendResult = + await this.#sendMessageStreamWithAutoCompression( + promptId, + nextMessage?.parts ?? [], + pendingSend.signal, + ); + if (!sendResult.responseStream) { + this.#preserveUnsentMessageHistory( + nextMessage, + sendResult.stopReason === 'cancelled', + ); + return { stopReason: sendResult.stopReason }; } + const responseStream = sendResult.responseStream; + nextMessage = null; - if ( - resp.type === StreamEventType.CHUNK && - resp.value.candidates && - resp.value.candidates.length > 0 - ) { - const candidate = resp.value.candidates[0]; - for (const part of candidate.content?.parts ?? []) { - if (!part.text) { - continue; + for await (const resp of responseStream) { + if (pendingSend.signal.aborted) { + return { stopReason: 'cancelled' }; + } + + if ( + resp.type === StreamEventType.CHUNK && + resp.value.candidates && + resp.value.candidates.length > 0 + ) { + const candidate = resp.value.candidates[0]; + for (const part of candidate.content?.parts ?? []) { + if (!part.text) { + continue; + } + + this.messageEmitter.emitMessage( + part.text, + 'assistant', + part.thought, + ); } + } - this.messageEmitter.emitMessage( - part.text, - 'assistant', - part.thought, - ); + if ( + resp.type === StreamEventType.CHUNK && + resp.value.usageMetadata + ) { + usageMetadata = resp.value.usageMetadata; } - } + if ( + resp.type === StreamEventType.CHUNK && + resp.value.functionCalls + ) { + functionCalls.push(...resp.value.functionCalls); + } + } + } catch (error) { + // Fire StopFailure hook (fire-and-forget, replaces Stop event for API errors) + // Aligned with useGeminiStream.ts handleFinishedWithErrorEvent + const errorStatus = getErrorStatus(error); + const errorMessage = + error instanceof Error ? error.message : String(error); + const errorType = classifyApiError({ + message: errorMessage, + status: errorStatus, + }); + + const hookSystem = this.config.getHookSystem?.(); + const hooksEnabledForStopFailure = + !this.config.getDisableAllHooks?.(); if ( - resp.type === StreamEventType.CHUNK && - resp.value.usageMetadata + hooksEnabledForStopFailure && + hookSystem && + this.config.hasHooksForEvent?.('StopFailure') ) { - usageMetadata = resp.value.usageMetadata; + // Fire-and-forget: don't wait for hook to complete + hookSystem + .fireStopFailureEvent(errorType, errorMessage) + .catch((err) => { + debugLogger.warn(`StopFailure hook failed: ${err}`); + }); } - if ( - resp.type === StreamEventType.CHUNK && - resp.value.functionCalls - ) { - functionCalls.push(...resp.value.functionCalls); + if (errorStatus === 429) { + throw new RequestError( + 429, + 'Rate limit exceeded. Try again later.', + ); } - } - } catch (error) { - // Fire StopFailure hook (fire-and-forget, replaces Stop event for API errors) - // Aligned with useGeminiStream.ts handleFinishedWithErrorEvent - const errorStatus = getErrorStatus(error); - const errorMessage = - error instanceof Error ? error.message : String(error); - const errorType = classifyApiError({ - message: errorMessage, - status: errorStatus, - }); - const hookSystem = this.config.getHookSystem?.(); - const hooksEnabledForStopFailure = - !this.config.getDisableAllHooks?.(); - if ( - hooksEnabledForStopFailure && - hookSystem && - this.config.hasHooksForEvent?.('StopFailure') - ) { - // Fire-and-forget: don't wait for hook to complete - hookSystem - .fireStopFailureEvent(errorType, errorMessage) - .catch((err) => { - debugLogger.warn(`StopFailure hook failed: ${err}`); - }); + throw error; } - if (errorStatus === 429) { - throw new RequestError( - 429, - 'Rate limit exceeded. Try again later.', + if (usageMetadata) { + this.#recordPromptTokenCount(usageMetadata); + // Kick off rewrite in background (non-blocking, runs parallel to tools) + if (this.messageRewriter) { + this.messageRewriter.flushTurn(pendingSend.signal); + } + + const durationMs = Date.now() - streamStartTime; + await this.messageEmitter.emitUsageMetadata( + usageMetadata, + '', + durationMs, ); } - throw error; - } - - if (usageMetadata) { - this.#recordPromptTokenCount(usageMetadata); - // Kick off rewrite in background (non-blocking, runs parallel to tools) - if (this.messageRewriter) { - this.messageRewriter.flushTurn(pendingSend.signal); + if (functionCalls.length > 0) { + const toolResponseParts = await this.runToolCalls( + pendingSend.signal, + promptId, + functionCalls, + ); + nextMessage = { role: 'user', parts: toolResponseParts }; } - - const durationMs = Date.now() - streamStartTime; - await this.messageEmitter.emitUsageMetadata( - usageMetadata, - '', - durationMs, - ); } - if (functionCalls.length > 0) { - const toolResponseParts = await this.runToolCalls( - pendingSend.signal, - promptId, - functionCalls, - ); - nextMessage = { role: 'user', parts: toolResponseParts }; + // Wait for any pending rewrite before returning + if (this.messageRewriter) { + await this.messageRewriter.waitForPendingRewrites(); } - } - // Wait for any pending rewrite before returning - if (this.messageRewriter) { - await this.messageRewriter.waitForPendingRewrites(); - } - - // Fire Stop hook loop (aligned with core path in client.ts) - // This is triggered after model response completes with no pending tool calls - let stopResult: { stopReason: PromptResponse['stopReason'] }; - try { - stopResult = await this.#handleStopHookLoop( + // Fire Stop hook loop (aligned with core path in client.ts) + // This is triggered after model response completes with no pending tool calls + return await this.#handleStopHookLoop( pendingSend, promptId, hooksEnabled, @@ -991,7 +998,6 @@ export class Session implements SessionContext { ), ); } - return stopResult; }, (result) => (result.stopReason === 'cancelled' ? 'cancelled' : 'ok'), ); @@ -1570,6 +1576,7 @@ export class Session implements SessionContext { messageType: 'cron', }, async () => { + let turnCount = 0; try { // Echo the cron prompt as a user message so the client sees it await this.sendUpdate({ @@ -1587,6 +1594,7 @@ export class Session implements SessionContext { }; while (nextMessage !== null) { + turnCount++; if (ac.signal.aborted) return; const functionCalls: FunctionCall[] = []; @@ -1680,6 +1688,16 @@ export class Session implements SessionContext { if (this.cronAbortController === ac) { this.cronAbortController = null; } + // Mirror the user-query path: emit conversation_finished on every + // terminal cron path (clean finish, abort, or caught error) so + // cron turns are not silently missing from conversation metrics. + logConversationFinishedEvent( + this.config, + new ConversationFinishedEvent( + this.config.getApprovalMode(), + turnCount, + ), + ); } }, () => @@ -2044,6 +2062,10 @@ export class Session implements SessionContext { const toolSpan = startToolSpan(toolName, { 'tool.call_id': callId, + // Dual-emit the legacy call_id/tool_name aliases like CoreToolScheduler + // (coreToolScheduler.ts) so pre-Phase-2 dashboards keyed off call_id keep + // matching daemon/ACP tool spans during the migration window. + call_id: callId, tool_name: toolName, }); let spanSuccess = false; @@ -2393,8 +2415,13 @@ export class Session implements SessionContext { switch (outcome) { case ToolConfirmationOutcome.Cancel: - return errorResponse( + // Route through earlyErrorResponse so spanError carries the + // cancellation reason (plain errorResponse leaves it unset, + // which makes endToolSpan fall back to the generic 'tool + // error' message) and the declined call is still recorded. + return earlyErrorResponse( new Error(`Tool "${toolName}" was canceled by the user.`), + toolName, ); case ToolConfirmationOutcome.ProceedOnce: case ToolConfirmationOutcome.ProceedAlways: @@ -2585,14 +2612,28 @@ export class Session implements SessionContext { } const durationMs = Date.now() - startTime; + // A tool can fail "softly" by returning toolResult.error without + // throwing (and can be cancelled mid-flight). Reflect that real + // outcome on logToolCall / recordToolResult / the tool span instead + // of hardcoding success — otherwise failed daemon/ACP tools are + // mislabeled as successful in telemetry and session replay. + const aborted = abortSignal.aborted; + const status: 'success' | 'error' | 'cancelled' = aborted + ? 'cancelled' + : toolResult.error + ? 'error' + : 'success'; + const succeeded = status === 'success'; logToolCall(this.config, { 'event.name': 'tool_call', 'event.timestamp': new Date().toISOString(), function_name: toolName, function_args: args, duration_ms: durationMs, - status: 'success', - success: true, + status, + success: succeeded, + error: toolResult.error?.message, + error_type: toolResult.error?.type, prompt_id: promptId, tool_type: typeof tool !== 'undefined' && tool instanceof DiscoveredMCPTool @@ -2605,13 +2646,18 @@ export class Session implements SessionContext { .getChatRecordingService() ?.recordToolResult(responseParts, { callId, - status: 'success', + status, resultDisplay: toolResult.returnDisplay, - error: undefined, - errorType: undefined, + error: toolResult.error + ? new Error(toolResult.error.message) + : undefined, + errorType: toolResult.error?.type, }); - spanSuccess = true; + spanSuccess = succeeded; + if (toolResult.error) { + spanError = toolResult.error.message; + } return responseParts; } catch (e) { // Ensure cleanup on error diff --git a/packages/core/src/telemetry/session-tracing.ts b/packages/core/src/telemetry/session-tracing.ts index 71052733d15..ef28d2ee34e 100644 --- a/packages/core/src/telemetry/session-tracing.ts +++ b/packages/core/src/telemetry/session-tracing.ts @@ -158,6 +158,29 @@ const NOOP_SPAN = trace.wrapSpanContext({ const interactionContext = new AsyncLocalStorage(); const toolContext = new AsyncLocalStorage(); +/** + * Resolve the session.id for a child span (llm_request / tool / tool.execution) + * from the per-session value carried on the parent span context, falling back + * to the process-global getCurrentSessionId() only when no parent is present. + * + * A daemon hosts many sessions in one process, but getCurrentSessionId() is a + * single module-global set at telemetry init — so reading it directly would + * stamp a child span with whichever session last touched the global rather than + * the session that owns the parenting interaction span. The interaction span + * sets 'session.id' from the per-session config.getSessionId(), so deriving it + * from the parent context keeps multi-session traces correctly attributed. The + * global fallback preserves the single-session CLI path, where no interaction + * span context exists around standalone spans. + */ +function resolveSessionId( + parentCtx: SpanContext | undefined, +): string | undefined { + const fromParent = parentCtx?.attributes?.['session.id']; + return typeof fromParent === 'string' && fromParent + ? fromParent + : getCurrentSessionId(); +} + const activeSpans = new Map>(); const strongSpans = new Map(); @@ -441,7 +464,7 @@ export function startLLMRequestSpan(model: string, promptId: string): Span { // attaches to the tool span instead of skipping back to the session root. const ctx = resolveParentContext(parentCtx); - const sessionId = getCurrentSessionId(); + const sessionId = resolveSessionId(parentCtx); const attributes: Attributes = { ...(sessionId ? { 'session.id': sessionId } : {}), 'qwen-code.model': model, @@ -591,7 +614,7 @@ export function startToolSpan( // tools-inside-tools cases before falling back to the session root. const ctx = resolveParentContext(parentCtx); - const sessionId = getCurrentSessionId(); + const sessionId = resolveSessionId(parentCtx); const attributes: Attributes = { ...(sessionId ? { 'session.id': sessionId } : {}), 'tool.name': toolName, @@ -707,7 +730,9 @@ export function startToolExecutionSpan(): Span { // subsystem) before falling back to the session root. const ctx = resolveParentContext(parentCtx); - const sessionId = getCurrentSessionId(); + const sessionId = resolveSessionId( + parentCtx ?? interactionContext.getStore(), + ); const span = getTracer().startSpan( SPAN_TOOL_EXECUTION, { From 6ec8bd2964d49b34a3d5199d73bc8e6cd8438220 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 30 May 2026 21:42:31 +0800 Subject: [PATCH 4/6] test(telemetry): cover session.id propagation, conversation_finished, tool outcome Address wenshao's no-test-coverage CHANGES_REQUESTED on the daemon/ACP telemetry changes. - session-tracing.test.ts: assert tool / llm_request / tool.execution spans derive session.id from the owning interaction context (not the process- global) including a multi-session isolation case and the CLI global fallback. - Session.test.ts: assert conversation_finished is emitted on the normal turn AND on the error/throw path (the path that previously dropped it), and that a soft tool failure (toolResult.error) is recorded with status 'error'. --- .../acp-integration/session/Session.test.ts | 105 ++++++++++++++++++ .../src/telemetry/session-tracing.test.ts | 97 ++++++++++++++++ 2 files changed, 202 insertions(+) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 2c7f3a76b2a..5b0ff72f6c4 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -863,6 +863,111 @@ describe('Session', () => { ); }); + describe('conversation_finished telemetry (#4602 review)', () => { + it('emits conversation_finished once when a turn completes normally', async () => { + const finishedSpy = vi + .spyOn(core, 'logConversationFinishedEvent') + .mockImplementation(() => {}); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValue(createEmptyStream()); + + await expect( + session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'hello' }], + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(finishedSpy).toHaveBeenCalledTimes(1); + }); + + it('still emits conversation_finished when the turn throws (telemetry not lost on the error path)', async () => { + const finishedSpy = vi + .spyOn(core, 'logConversationFinishedEvent') + .mockImplementation(() => {}); + mockChat.sendMessageStream = vi + .fn() + .mockRejectedValue(new Error('stream boom')); + + // The turn surfaces the failure (rejection or error stopReason); either + // way the finally wrapping the whole turn must have fired the event + // before unwinding — the regression wenshao flagged was that only the + // clean stop-hook path emitted it. + await session + .prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'hello' }], + }) + .catch(() => undefined); + + expect(finishedSpy).toHaveBeenCalled(); + }); + }); + + describe('tool outcome telemetry (#4602 review)', () => { + it('records a soft tool failure (toolResult.error) as error, not success', async () => { + const logToolCallSpy = vi + .spyOn(core, 'logToolCall') + .mockImplementation(() => {}); + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + + const tool = { + name: 'read_file', + kind: core.Kind.Read, + build: vi.fn().mockReturnValue({ + params: { path: '/tmp/test.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + // Soft failure: resolves (does not throw) but carries an error. + execute: vi.fn().mockResolvedValue({ + llmContent: 'nope', + returnDisplay: 'failed', + error: { message: 'tool blew up' }, + }), + }), + }; + mockToolRegistry.getTool.mockReturnValue(tool); + + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-1', + name: 'read_file', + args: { path: '/tmp/test.txt' }, + }, + ], + }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'read the file' }], + }); + + const toolEvent = logToolCallSpy.mock.calls + .map( + ([, ev]) => + ev as { + function_name?: string; + status?: string; + success?: boolean; + }, + ) + .find((ev) => ev.function_name === 'read_file'); + expect(toolEvent?.status).toBe('error'); + expect(toolEvent?.success).toBe(false); + }); + }); + describe('auto-compress', () => { it('runs automatic compression before sending an ACP prompt', async () => { mockChat.sendMessageStream = vi diff --git a/packages/core/src/telemetry/session-tracing.test.ts b/packages/core/src/telemetry/session-tracing.test.ts index eb96b7e0fda..15ac27c3987 100644 --- a/packages/core/src/telemetry/session-tracing.test.ts +++ b/packages/core/src/telemetry/session-tracing.test.ts @@ -734,6 +734,103 @@ describe('session-tracing', () => { }); }); + describe('session.id derives from the owning session, not the process-global (#4602 review)', () => { + it('stamps a tool span with the interaction session.id even when the process-global belongs to another session', () => { + // Daemon scenario: telemetry init left the process-global pointing at + // session B, but the active interaction belongs to session A. + setSessionContext(undefined, 'session-B-global'); + startInteractionSpan(createMockConfig({ sessionId: 'session-A' }), { + promptId: 'p-a', + model: 'm', + messageType: 'acp_prompt', + }); + + const span = startToolSpan('Bash', { 'tool.call_id': 'c1' }); + endToolSpan(span, { success: true }); + + const toolSpan = mockSpans.find((s) => s.name === 'qwen-code.tool'); + expect(toolSpan?.attributes['session.id']).toBe('session-A'); + }); + + it('stamps an llm_request span with the interaction session.id, not the global', () => { + setSessionContext(undefined, 'session-B-global'); + startInteractionSpan(createMockConfig({ sessionId: 'session-A' }), { + promptId: 'p-a', + model: 'm', + messageType: 'acp_prompt', + }); + + const span = startLLMRequestSpan('m', 'p-a'); + endLLMRequestSpan(span, { success: true }); + + const llmSpan = mockSpans.find((s) => s.name === 'qwen-code.llm_request'); + expect(llmSpan?.attributes['session.id']).toBe('session-A'); + }); + + it('stamps a tool.execution span with the owning session id via the tool span context', () => { + setSessionContext(undefined, 'session-B-global'); + startInteractionSpan(createMockConfig({ sessionId: 'session-A' }), { + promptId: 'p-a', + model: 'm', + messageType: 'acp_prompt', + }); + + const toolSpan = startToolSpan('Bash', { 'tool.call_id': 'c1' }); + let execSpan!: ReturnType; + runInToolSpanContext(toolSpan, () => { + execSpan = startToolExecutionSpan(); + }); + endToolExecutionSpan(execSpan, { success: true }); + endToolSpan(toolSpan, { success: true }); + + const exec = mockSpans.find((s) => s.name === 'qwen-code.tool.execution'); + expect(exec?.attributes['session.id']).toBe('session-A'); + }); + + it('isolates concurrent sessions: each tool span carries its own session id', async () => { + // Two interactions for two different sessions while the global is stale. + setSessionContext(undefined, 'stale-global'); + + await withInteractionSpan( + createMockConfig({ sessionId: 'session-A' }), + { promptId: 'pa', model: 'm', messageType: 'acp_prompt' }, + async () => { + endToolSpan(startToolSpan('Read', { 'tool.call_id': 'a1' }), { + success: true, + }); + }, + ); + await withInteractionSpan( + createMockConfig({ sessionId: 'session-B' }), + { promptId: 'pb', model: 'm', messageType: 'acp_prompt' }, + async () => { + endToolSpan(startToolSpan('Write', { 'tool.call_id': 'b1' }), { + success: true, + }); + }, + ); + + const readSpan = mockSpans.find( + (s) => s.attributes['tool.name'] === 'Read', + ); + const writeSpan = mockSpans.find( + (s) => s.attributes['tool.name'] === 'Write', + ); + expect(readSpan?.attributes['session.id']).toBe('session-A'); + expect(writeSpan?.attributes['session.id']).toBe('session-B'); + }); + + it('falls back to the process-global session id for standalone spans (single-session CLI path)', () => { + // No interaction context — single-session CLI: the global is correct. + setSessionContext(undefined, 'cli-session'); + const span = startToolSpan('Bash', { 'tool.call_id': 'c1' }); + endToolSpan(span, { success: true }); + + const toolSpan = mockSpans.find((s) => s.name === 'qwen-code.tool'); + expect(toolSpan?.attributes['session.id']).toBe('cli-session'); + }); + }); + describe('tool execution sub-spans', () => { it('creates a tool execution span as child of tool span via runInToolSpanContext', () => { const toolSpan = startToolSpan('Bash'); From 2da89e55dce8940cf5bc0da32d8792f5bf376a63 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 30 May 2026 22:35:29 +0800 Subject: [PATCH 5/6] fix(telemetry): annotate withInteractionSpan result param to avoid implicit any The interaction-span getResultStatus callback relied on generic inference of T from the turn-loop return type; under the branch's pre-existing upstream type errors that inference degrades to any, surfacing a noImplicitAny error at the callback. Annotate the param explicitly so it no longer depends on inference. (wenshao verification report) --- packages/cli/src/acp-integration/session/Session.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index c80720b313a..18644563a2f 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -999,7 +999,8 @@ export class Session implements SessionContext { ); } }, - (result) => (result.stopReason === 'cancelled' ? 'cancelled' : 'ok'), + (result: { stopReason: PromptResponse['stopReason'] }) => + result.stopReason === 'cancelled' ? 'cancelled' : 'ok', ); }, ); From 1737887bade5362e5d7a69f9c3babfc1c6dbffba Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Sat, 30 May 2026 23:08:16 +0800 Subject: [PATCH 6/6] fix(telemetry): distinguish error from cancelled in interaction/tool outcomes Address wenshao review round 2 (telemetry-accuracy suggestions). - session-tracing.ts: extend InteractionSpanResultStatus to 'ok' | 'error' | 'cancelled' and have withInteractionSpan's finally set SpanStatusCode.ERROR when getResultStatus reports 'error' on a non-throwing path. Guarded so a thrown error's specific message is not overwritten by the generic one. - Session.ts #executeCronPrompt: map caught cron errors to 'error' (was 'cancelled'), so turn_status dashboards no longer miss cron API failures. - Session.ts runTool success path: compute aborted/status/succeeded once before emitResult so the client-facing success flag matches telemetry on abort-induced cancellation (previously emitResult used !toolResult.error). - Session.ts runTool error paths: errorResponse and the catch-block recordToolResult now label aborted calls 'cancelled' instead of 'error'. Tests: withInteractionSpan 'error' status -> span ERROR, and thrown-error message preserved. --- .../src/acp-integration/session/Session.ts | 45 +++++++++++-------- .../src/telemetry/session-tracing.test.ts | 31 +++++++++++++ .../core/src/telemetry/session-tracing.ts | 13 +++++- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 18644563a2f..959755955a8 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1702,7 +1702,7 @@ export class Session implements SessionContext { } }, () => - ac.signal.aborted ? 'cancelled' : cronHadError ? 'cancelled' : 'ok', + ac.signal.aborted ? 'cancelled' : cronHadError ? 'error' : 'ok', ); }, ); @@ -2007,7 +2007,8 @@ export class Session implements SessionContext { function_name: fc.name ?? '', function_args: args, duration_ms: durationMs, - status: 'error', + // An aborted signal means the call was cancelled, not a genuine error. + status: abortSignal.aborted ? 'cancelled' : 'error', success: false, error: error.message, tool_type: @@ -2580,6 +2581,21 @@ export class Session implements SessionContext { } } + // A tool can fail "softly" by returning toolResult.error without + // throwing, and can be cancelled mid-flight. Compute the real outcome + // once and reflect it on the client-facing emitResult as well as + // logToolCall / recordToolResult / the tool span, instead of + // hardcoding success — otherwise failed/cancelled daemon/ACP tools + // are mislabeled as successful in telemetry, session replay, and the + // client UI. + const aborted = abortSignal.aborted; + const status: 'success' | 'error' | 'cancelled' = aborted + ? 'cancelled' + : toolResult.error + ? 'error' + : 'success'; + const succeeded = status === 'success'; + // Handle TodoWriteTool: extract todos and send plan update if (isTodoWriteTool) { const todos = this.planEmitter.extractTodos( @@ -2596,10 +2612,11 @@ export class Session implements SessionContext { // Still log and return function response for LLM } else { // Normal tool handling: emit result using ToolCallEmitter - // Convert toolResult.error to Error type if present const error = toolResult.error ? new Error(toolResult.error.message) - : undefined; + : aborted + ? new Error('Tool execution was cancelled') + : undefined; await this.toolCallEmitter.emitResult({ callId, @@ -2608,23 +2625,11 @@ export class Session implements SessionContext { message: responseParts, resultDisplay: toolResult.returnDisplay, error, - success: !toolResult.error, + success: succeeded, }); } const durationMs = Date.now() - startTime; - // A tool can fail "softly" by returning toolResult.error without - // throwing (and can be cancelled mid-flight). Reflect that real - // outcome on logToolCall / recordToolResult / the tool span instead - // of hardcoding success — otherwise failed daemon/ACP tools are - // mislabeled as successful in telemetry and session replay. - const aborted = abortSignal.aborted; - const status: 'success' | 'error' | 'cancelled' = aborted - ? 'cancelled' - : toolResult.error - ? 'error' - : 'success'; - const succeeded = status === 'success'; logToolCall(this.config, { 'event.name': 'tool_call', 'event.timestamp': new Date().toISOString(), @@ -2658,6 +2663,8 @@ export class Session implements SessionContext { spanSuccess = succeeded; if (toolResult.error) { spanError = toolResult.error.message; + } else if (aborted) { + spanError = 'Tool execution was cancelled'; } return responseParts; } catch (e) { @@ -2707,7 +2714,9 @@ export class Session implements SessionContext { ]; this.config.getChatRecordingService()?.recordToolResult(errorParts, { callId, - status: 'error', + // A throw caused by abort (e.g. AbortError) is a cancellation, not + // a genuine tool error — keep it consistent with the success path. + status: abortSignal.aborted ? 'cancelled' : 'error', resultDisplay: undefined, error, errorType: undefined, diff --git a/packages/core/src/telemetry/session-tracing.test.ts b/packages/core/src/telemetry/session-tracing.test.ts index 15ac27c3987..8e3e244711a 100644 --- a/packages/core/src/telemetry/session-tracing.test.ts +++ b/packages/core/src/telemetry/session-tracing.test.ts @@ -217,6 +217,37 @@ describe('session-tracing', () => { expect(mockSpans[0]!.statuses.at(-1)?.code).toBe(SpanStatusCode.OK); }); + it('marks the interaction span ERROR when getResultStatus returns "error"', async () => { + const config = createMockConfig(); + await withInteractionSpan( + config, + { promptId: 'p-cron-err', model: 'm', messageType: 'cron' }, + async () => 'done', + () => 'error', + ); + + const span = mockSpans.find((s) => s.name === 'qwen-code.interaction'); + expect(span?.attributes['qwen-code.turn_status']).toBe('error'); + expect(span?.statuses.at(-1)?.code).toBe(SpanStatusCode.ERROR); + }); + + it('keeps a thrown error message instead of the generic error-status message', async () => { + const config = createMockConfig(); + await expect( + withInteractionSpan( + config, + { promptId: 'p-throw', model: 'm', messageType: 'cron' }, + async () => { + throw new Error('boom from fn'); + }, + ), + ).rejects.toThrow('boom from fn'); + + const span = mockSpans.find((s) => s.name === 'qwen-code.interaction'); + expect(span?.statuses.at(-1)?.code).toBe(SpanStatusCode.ERROR); + expect(span?.statuses.at(-1)?.message).toBe('boom from fn'); + }); + it('ends interaction span with error status', () => { const config = createMockConfig(); startInteractionSpan(config, { diff --git a/packages/core/src/telemetry/session-tracing.ts b/packages/core/src/telemetry/session-tracing.ts index ef28d2ee34e..15b1843cdbb 100644 --- a/packages/core/src/telemetry/session-tracing.ts +++ b/packages/core/src/telemetry/session-tracing.ts @@ -47,7 +47,7 @@ export interface EndInteractionOptions { errorMessage?: string; } -export type InteractionSpanResultStatus = 'ok' | 'cancelled'; +export type InteractionSpanResultStatus = 'ok' | 'error' | 'cancelled'; export interface LLMRequestMetadata { inputTokens?: number; @@ -418,6 +418,7 @@ export async function withInteractionSpan( return await otelContext.with(activeContext, async () => interactionContext.run(spanContextObj, async () => { let terminalStatus: InteractionStatus = 'ok'; + let errorStatusSet = false; try { const result = await fn(); terminalStatus = getResultStatus?.(result) ?? 'ok'; @@ -430,6 +431,7 @@ export async function withInteractionSpan( error instanceof Error ? error.message : String(error), ), }); + errorStatusSet = true; throw error; } finally { if (!spanContextObj.ended) { @@ -441,6 +443,15 @@ export async function withInteractionSpan( }); if (terminalStatus === 'ok') { span.setStatus({ code: SpanStatusCode.OK }); + } else if (terminalStatus === 'error' && !errorStatusSet) { + // getResultStatus reported 'error' on a non-throwing path (e.g. the + // cron path swallows API errors and surfaces them via status), so + // the catch above didn't run. Mark the span ERROR here, guarded so + // a thrown error's specific message is never overwritten. + span.setStatus({ + code: SpanStatusCode.ERROR, + message: 'interaction error', + }); } span.end(); activeSpans.delete(spanId);