feat(telemetry): Phase 2 — tool.blocked_on_user + hook spans (#3731)

packages/core/src/core/toolHookTriggers.test.ts

@@ -58,7 +58,7 @@ describe('toolHookTriggers', () => {
       expect(result).toEqual({ shouldProceed: true });
     });
 
-    it('should return shouldProceed: true when hook execution fails', async () => {
+    it('should return shouldProceed: true with sentinel hookError when hook execution fails without an error message', async () => {
       const mockMessageBus = createMockMessageBus();
       (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
         success: false,
@@ -72,7 +72,38 @@ describe('toolHookTriggers', () => {
         'auto',
       );
 
-      expect(result).toEqual({ shouldProceed: true });
+      // #4321 review-7 SF-H1: runner contract violation (success:false
+      // with no error.message) used to silently return allow with no
+      // telemetry. Now synthesizes a sentinel hookError so the span
+      // records `success: false` + the description of what went wrong.
+      expect(result.shouldProceed).toBe(true);
+      expect(result.hookError).toMatch(/success: false/);
+    });
+
+    it('synthesizes sentinel hookError when runner returns empty-string error message (#4321)', async () => {
+      // #4321 review-9: pin the `||` (not `??`) semantics. A future
+      // regression back to `??` would preserve `hookError: ""` here
+      // which downstream `r.hookError ? ...` truthiness then silently
+      // drops — same allow-without-telemetry pathology SF-H1 closed.
+      const mockMessageBus = createMockMessageBus();
+      (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
+        success: false,
+        error: { message: '' },
+      });
+
+      const result = await firePreToolUseHook(
+        mockMessageBus,
+        'test-tool',
+        {},
+        'test-id',
+        'auto',
+      );
+
+      expect(result.shouldProceed).toBe(true);
+      expect(result.hookError).toMatch(/success: false/);
+      // Specifically NOT empty: an empty string would round-trip through
+      // a downstream truthiness check as missing.
+      expect(result.hookError).not.toBe('');
     });
 
     it('should return shouldProceed: true when hook output is empty', async () => {
@@ -215,7 +246,13 @@ describe('toolHookTriggers', () => {
         'auto',
       );
 
-      expect(result).toEqual({ shouldProceed: true });
+      // #4321 review: hookError surfaces the swallowed transport error so
+      // observers (telemetry spans, debug logs) can distinguish a failed
+      // hook from a successful "allow" decision.
+      expect(result).toEqual({
+        shouldProceed: true,
+        hookError: 'Network error',
+      });
     });
   });
 
@@ -233,7 +270,7 @@ describe('toolHookTriggers', () => {
       expect(result).toEqual({ shouldStop: false });
     });
 
-    it('should return shouldStop: false when hook execution fails', async () => {
+    it('should return shouldStop: false with sentinel hookError when hook execution fails without an error message', async () => {
       const mockMessageBus = createMockMessageBus();
       (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
         success: false,
@@ -248,7 +285,31 @@ describe('toolHookTriggers', () => {
         'auto',
       );
 
-      expect(result).toEqual({ shouldStop: false });
+      // #4321 review-7 SF-H1 — see firePreToolUseHook counterpart.
+      expect(result.shouldStop).toBe(false);
+      expect(result.hookError).toMatch(/success: false/);
+    });
+
+    it('synthesizes sentinel hookError when runner returns empty-string error message (#4321)', async () => {
+      // #4321 review-9 — see firePreToolUseHook counterpart.
+      const mockMessageBus = createMockMessageBus();
+      (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
+        success: false,
+        error: { message: '' },
+      });
+
+      const result = await firePostToolUseHook(
+        mockMessageBus,
+        'test-tool',
+        {},
+        {},
+        'test-id',
+        'auto',
+      );
+
+      expect(result.shouldStop).toBe(false);
+      expect(result.hookError).toMatch(/success: false/);
+      expect(result.hookError).not.toBe('');
     });
 
     it('should return shouldStop: false when hook output is empty', async () => {
@@ -338,7 +399,9 @@ describe('toolHookTriggers', () => {
         'auto',
       );
 
-      expect(result).toEqual({ shouldStop: false });
+      // #4321 review: hookError now surfaced to caller (see PreToolUse parallel test).
+      expect(result.shouldStop).toBe(false);
+      expect(result.hookError).toBeDefined();
     });
   });
 
@@ -355,7 +418,7 @@ describe('toolHookTriggers', () => {
       expect(result).toEqual({});
     });
 
-    it('should return empty object when hook execution fails', async () => {
+    it('should return sentinel hookError when hook execution fails without an error message', async () => {
       const mockMessageBus = createMockMessageBus();
       (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
         success: false,
@@ -369,7 +432,28 @@ describe('toolHookTriggers', () => {
         'error message',
       );
 
-      expect(result).toEqual({});
+      // #4321 review-7 SF-H1 — see firePreToolUseHook counterpart.
+      expect(result.hookError).toMatch(/success: false/);
+    });
+
+    it('synthesizes sentinel hookError when runner returns empty-string error message (#4321)', async () => {
+      // #4321 review-9 — see firePreToolUseHook counterpart.
+      const mockMessageBus = createMockMessageBus();
+      (mockMessageBus.request as ReturnType<typeof vi.fn>).mockResolvedValue({
+        success: false,
+        error: { message: '' },
+      });
+
+      const result = await firePostToolUseFailureHook(
+        mockMessageBus,
+        'test-id',
+        'test-tool',
+        {},
+        'error message',
+      );
+
+      expect(result.hookError).toMatch(/success: false/);
+      expect(result.hookError).not.toBe('');
     });
 
     it('should return empty object when hook output is empty', async () => {
@@ -429,7 +513,9 @@ describe('toolHookTriggers', () => {
         'error message',
       );
 
-      expect(result).toEqual({});
+      // #4321 review: hookError now surfaced to caller.
+      expect(result.hookError).toBeDefined();
+      expect(result.additionalContext).toBeUndefined();
     });
   });
 

packages/core/src/core/toolHookTriggers.ts

@@ -43,6 +43,14 @@ export interface PreToolUseHookResult {
   blockType?: 'denied' | 'ask' | 'stop';
   /** Additional context to add */
   additionalContext?: string;
+  /**
+   * Set when the hook helper caught and absorbed a transport / dispatch
+   * error. The tool execution still proceeds (existing non-blocking
+   * contract), but observers (telemetry spans, debug logs) can detect
+   * that the hook itself failed instead of treating the safe-default
+   * response as a successful "allow" decision (#4321 review).
+   */
+  hookError?: string;
 }
 
 /**
@@ -55,6 +63,8 @@ export interface PostToolUseHookResult {
   stopReason?: string;
   /** Additional context to append to tool response */
   additionalContext?: string;
+  /** See PreToolUseHookResult.hookError. */
+  hookError?: string;
 }
 
 /**
@@ -63,6 +73,8 @@ export interface PostToolUseHookResult {
 export interface PostToolUseFailureHookResult {
   /** Additional context about the failure */
   additionalContext?: string;
+  /** See PreToolUseHookResult.hookError. */
+  hookError?: string;
 }
 
 /**
@@ -107,7 +119,27 @@ export async function firePreToolUseHook(
     );
 
     if (!response.success || !response.output) {
-      return { shouldProceed: true };
+      // Hook runner reported failure (URL validation, fn exception,
+      // prompt-runner crash, ...). The `response.error` from the runner
+      // is the canonical cause — forward it so telemetry and operators
+      // see the actual failure instead of a fake "allow" success
+      // (#4321 review silent-failure-hunter HIGH).
+      //
+      // If runner returned `{ success: false }` (or missing output) with no
+      // `error.message`, synthesize a sentinel so the contract violation is
+      // still visible on the span instead of silently degrading to an allow
+      // with empty telemetry (#4321 review-7 silent-failure-hunter HIGH-1).
+      // `||` (revert from `??`): downstream consumers in
+      // coreToolScheduler.ts gate on `r.hookError ? ...`, so an
+      // empty-string message would be silently dropped — the previous
+      // `??` change defeated its own intent. Empty-string error
+      // messages carry no operator value; the sentinel is more
+      // actionable. (#4321 review-9 wenshao Suggestion refines
+      // review-8.)
+      const message =
+        response.error?.message ||
+        `hook runner returned ${response.success ? 'no output' : 'success: false'} without error detail`;
+      return { shouldProceed: true, hookError: message };
     }
 
     const preToolOutput = createHookOutput(
@@ -155,10 +187,9 @@ export async function firePreToolUseHook(
     };
   } catch (error) {
     // Hook errors should not block tool execution
-    debugLogger.warn(
-      `PreToolUse hook error for ${toolName}: ${error instanceof Error ? error.message : String(error)}`,
-    );
-    return { shouldProceed: true };
+    const message = error instanceof Error ? error.message : String(error);
+    debugLogger.warn(`PreToolUse hook error for ${toolName}: ${message}`);
+    return { shouldProceed: true, hookError: message };
   }
 }
 
@@ -207,7 +238,18 @@ export async function firePostToolUseHook(
     );
 
     if (!response.success || !response.output) {
-      return { shouldStop: false };
+      // See firePreToolUseHook for the rationale.
+      // `||` (revert from `??`): downstream consumers in
+      // coreToolScheduler.ts gate on `r.hookError ? ...`, so an
+      // empty-string message would be silently dropped — the previous
+      // `??` change defeated its own intent. Empty-string error
+      // messages carry no operator value; the sentinel is more
+      // actionable. (#4321 review-9 wenshao Suggestion refines
+      // review-8.)
+      const message =
+        response.error?.message ||
+        `hook runner returned ${response.success ? 'no output' : 'success: false'} without error detail`;
+      return { shouldStop: false, hookError: message };
     }
 
     const postToolOutput = createHookOutput(
@@ -232,10 +274,9 @@ export async function firePostToolUseHook(
     };
   } catch (error) {
     // Hook errors should not affect tool result
-    debugLogger.warn(
-      `PostToolUse hook error for ${toolName}: ${error instanceof Error ? error.message : String(error)}`,
-    );
-    return { shouldStop: false };
+    const message = error instanceof Error ? error.message : String(error);
+    debugLogger.warn(`PostToolUse hook error for ${toolName}: ${message}`);
+    return { shouldStop: false, hookError: message };
   }
 }
 
@@ -287,7 +328,18 @@ export async function firePostToolUseFailureHook(
     );
 
     if (!response.success || !response.output) {
-      return {};
+      // See firePreToolUseHook for the rationale.
+      // `||` (revert from `??`): downstream consumers in
+      // coreToolScheduler.ts gate on `r.hookError ? ...`, so an
+      // empty-string message would be silently dropped — the previous
+      // `??` change defeated its own intent. Empty-string error
+      // messages carry no operator value; the sentinel is more
+      // actionable. (#4321 review-9 wenshao Suggestion refines
+      // review-8.)
+      const message =
+        response.error?.message ||
+        `hook runner returned ${response.success ? 'no output' : 'success: false'} without error detail`;
+      return { hookError: message };
     }
 
     const failureOutput = createHookOutput(
@@ -301,10 +353,11 @@ export async function firePostToolUseFailureHook(
     };
   } catch (error) {
     // Hook errors should not affect error handling
+    const message = error instanceof Error ? error.message : String(error);
     debugLogger.warn(
-      `PostToolUseFailure hook error for ${toolName}: ${error instanceof Error ? error.message : String(error)}`,
+      `PostToolUseFailure hook error for ${toolName}: ${message}`,
     );
-    return {};
+    return { hookError: message };
   }
 }
 

packages/core/src/telemetry/constants.ts

@@ -64,3 +64,7 @@ export const SPAN_INTERACTION = 'qwen-code.interaction';
 export const SPAN_LLM_REQUEST = 'qwen-code.llm_request';
 export const SPAN_TOOL = 'qwen-code.tool';
 export const SPAN_TOOL_EXECUTION = 'qwen-code.tool.execution';
+/** Brackets the time a tool spends in `awaiting_approval` waiting on the user. */
+export const SPAN_TOOL_BLOCKED_ON_USER = 'qwen-code.tool.blocked_on_user';
+/** Wraps each pre/post-tool-use hook fire site for per-hook latency / decision tracking. */
+export const SPAN_HOOK = 'qwen-code.hook';

packages/core/src/telemetry/index.ts

@@ -146,13 +146,23 @@ export {
   runInToolSpanContext,
   startToolExecutionSpan,
   endToolExecutionSpan,
+  startToolBlockedOnUserSpan,
+  endToolBlockedOnUserSpan,
+  startHookSpan,
+  endHookSpan,
   getActiveInteractionSpan,
+  truncateSpanError,
 } from './session-tracing.js';
 export type {
   StartInteractionOptions,
   EndInteractionOptions,
   LLMRequestMetadata,
   ToolSpanMetadata,
+  ToolBlockedDecision,
+  ToolBlockedSource,
+  HookEvent,
+  StartHookSpanOptions,
+  HookSpanMetadata,
 } from './session-tracing.js';
 export {
   addUserPromptAttributes,