📋 Review Summary

This PR (Phase 4a) implements TTFT (time-to-first-token) measurement and OTel GenAI semantic convention dual-emit for LLM request spans. The implementation is well-structured, thoroughly tested, and follows established patterns from the codebase. The design decisions are extensively documented with clear justifications. Overall assessment: strong implementation with minor suggestions for improvement.

🔍 General Feedback

• Excellent design documentation: The design doc (telemetry-llm-request-timing-design.md) is comprehensive with 8 locked decisions (D1-D8), each with "Why not X" justifications and cross-product comparisons (claude-code, opencode).
• Clean separation of concerns: The hasUserVisibleContent helper centralizes first-token detection logic, avoiding per-provider duplication.
• Correct concurrency handling: TTFT capture uses method-local closure variables (never instance fields) in LoggingContentGenerator.generateContentStream, which is critical since the class is one-per-ContentGenerator and shared across concurrent calls.
• Comprehensive test coverage: 16 test cases for hasUserVisibleContent part-type matrix and 17 test cases for endLLMRequestSpan Phase 4a timing decomposition + dual-emit.
• GenAI semantic convention dual-emit follows precedent: Private names (qwen-code.*) remain authoritative; gen_ai.* attributes serve as a compatibility layer for spec-aware backends (consistent with Phase 3 feat(telemetry): Phase 3 — qwen-code.subagent span with concurrent isolation (#3731) #4410).
• Phase 4b/4c placeholders properly declared: New fields (requestSetupMs, attempt, retryTotalDelayMs) are optional now, preventing schema churn in follow-up PRs.

🎯 Specific Feedback

🟡 High

• File: streamContentDetection.ts:53 - The thought field check uses === true which is correct per D1, but the comment says "Match strict === true rather than checking presence". Consider adding a defensive check for non-boolean thought values (e.g., if a provider emits "true" as string). Current code handles this via type coercion, but explicit validation would be safer:

  if (typeof p.thought === 'boolean' && p.thought === true) return true;

• File: session-tracing.ts:450 - The derived sampling_ms calculation uses Math.max(0, ...) to clamp for clock skew, but this masks potential underlying issues. Consider adding a debug log when clamping occurs (similar to existing debugLogger.warn patterns in the file) to help diagnose clock skew in production:

  if (samplingMs === 0 && duration > metadata.ttftMs + (metadata.requestSetupMs ?? 0)) {
    debugLogger.warn(`sampling_ms clamped to 0 due to clock skew: duration=${duration}, ttftMs=${metadata.ttftMs}, requestSetupMs=${metadata.requestSetupMs}`);
  }

🟢 Medium

• File: loggingContentGenerator.ts:533 - The TTFT capture condition if (ttftMs === undefined && hasUserVisibleContent(response)) is correct but could benefit from an inline comment explaining why we check ttftMs === undefined (to ensure we capture the first token only, not overwrite on subsequent chunks). The design doc D2 explains this, but a brief inline comment would help future maintainers:

  // Capture TTFT on first user-visible chunk only; subsequent chunks must not overwrite
  if (ttftMs === undefined && hasUserVisibleContent(response)) {

• File: session-tracing.ts:420-423 - The dual-emit for gen_ai.usage.cached_tokens notes "Experimental — may rename before Stable". Consider adding a TODO comment with a reference to the OTel spec version or a tracking issue number so future maintainers know when to update:

  // TODO(#XXXX): Update when OTel GenAI semconv stabilizes (currently Experimental)

• File: streamContentDetection.test.ts:116-121 - The test for non-object parts (null, undefined, 'string', 42) is excellent defensive testing. Consider adding one more edge case: an object with text as a non-string type (e.g., { text: 123 }) to verify the typeof p.text === 'string' guard works correctly.

🔵 Low

• File: session-tracing.ts:66-76 - The JSDoc for ttftMs mentions "diverges from claude-code's ttftMs" which is important context. Consider linking directly to the design doc section (D1) for easier navigation:

   * See docs/design/telemetry-llm-request-timing-design.md (D1) for rationale.

• File: streamContentDetection.ts:17 - The comment lists thought / reasoning content (provider-dependent; o1, qwen thinking, Anthropic <thinking>). Consider adding Gemini to this list since the design doc D1 explicitly mentions "Gemini's thought" as well.

• File: loggingContentGenerator.ts:468-474 - The TTFT comment block is comprehensive but could reference the design doc D2 directly for the "Why never instance fields" explanation, making it easier for future maintainers to find the full rationale.

✅ Highlights

• hasUserVisibleContent implementation (streamContentDetection.ts:34-57) - Clean, well-documented function with excellent type safety and defensive handling of edge cases. The explicit handling of thought: false vs thought: true vs missing thought shows deep understanding of the codebase conventions.

• Test coverage for edge cases (streamContentDetection.test.ts) - The 16-case matrix thoroughly covers all part types including Anthropic's <thinking> first chunk pattern ({ text: '', thought: true }), usage-only chunks, and defensive non-object handling.

• Phase 4a/4b/4c split discipline - The PR strictly implements Phase 4a (TTFT + dual-emit) while declaring Phase 4b/4c placeholders as optional fields. This prevents scope creep and keeps the PR reviewable at ~200 LOC of production code.

• GenAI dual-emit consistency (session-tracing.ts:403-426) - All dual-emits follow the same pattern with clear comments about stability levels (Stable vs Experimental) and the "private name authoritative" rule from Phase 3.

• Idle timeout handling (loggingContentGenerator.ts:493-517) - The span timeout mechanism correctly handles the "consumer abandoned the generator" case and sets spanEndedByTimeout to prevent redundant endLLMRequestSpan calls.

补充几点上面 review 没覆盖到的观察：

cli 包同样存在混用

packages/cli/src 的情况类似：282 个 camelCase 文件 vs 31 个 kebab-case 文件。目录层面 dualOutput/、nonInteractive/、remoteInput/ 用 camelCase，acp-integration/、prompt-processors/、test-commands/ 用 kebab-case。只是 cli 包 kebab-case 占比更低（约 7%），不一致性比 core 包更明显。

tools 目录的命名-语义脱钩问题

packages/core/src/tools/ 里还有一个更具体的问题：文件名命名风格与 toolName 的对应关系不一致。

kebab-case 文件名与 toolName 有清晰的 1:1 机械映射（- → _）：

• task-stop.ts → task_stop
• web-fetch.ts → web_fetch
• read-file.ts → read_file
• cron-create.ts → cron_create

但 camelCase 文件名与 toolName 的对应关系就模糊了：

• askUserQuestion.ts → ask_user_question（语义对应但风格不同）
• todoWrite.ts → todo_write（同上）
• ripGrep.ts → grep_search（完全脱钩，文件名和工具名无直接关系）
• syntheticOutput.ts → structured_output（完全脱钩）

如果统一到 kebab-case，文件名与 toolName 就能形成一致的转换规则（kebab-file.ts → kebab_file），降低新贡献者的认知负担。

这不是 blocking 问题，只是趁这个 PR 把观察到的全貌摆出来，方便后续做决策 🤔

回复 @doudouOUC 关于 kebab-case 统一的想法：

完全同意「机械规则 > 人工审查」的原则。命名规范如果只靠 reviewer 人肉盯，迟早会漏。以下是我目前觉得最可行的两条路径：

1. ESLint check-file 规则（CI 硬卡）

eslint-plugin-check-file 可以直接在 lint 阶段拦截不符合 kebab-case 的新文件名：

// eslint.config.js
import checkFile from 'eslint-plugin-check-file';

export default [
  {
    plugins: { 'check-file': checkFile },
    rules: {
      'check-file/filename': ['error', 'KEBAB_CASE', {
        // 逐步迁移：先 allowlist 存量 camelCase 文件
        // 每次重命名一批就从列表中移除
      }],
    },
  },
];

好处是零额外基础设施、CI 已有 ESLint pipeline，加一条规则就能跑。KEBAB_CASE 是插件内置 pattern，不需要手写正则。

对于存量文件，可以用一个 allowedFilenamePatterns 列表做渐进式迁移 —— 不要求一次性改完 185 个文件，但新增文件必须是 kebab-case，每次有人重命名旧文件就从 allowlist 中删一条。

2. AGENTS.md 补充（AI agent 软约束）

在 AGENTS.md 的 Code Conventions 小节加一段：

- **File naming**: All new `.ts` files must use `kebab-case`
  (e.g., `stream-content-detection.ts`, not `streamContentDetection.ts`).
  Existing `camelCase` files are legacy and should be renamed to
  `kebab-case` opportunistically when touched.

这条主要约束 AI coding agent（Qwen Code 自身、Copilot 等）在生成新文件时自动遵守 kebab-case。配合 ESLint 硬卡，形成"agent 自觉遵守 + CI 兜底拦截"的双层防线。

建议拆成独立 issue 推进，不卡本 PR。可以归到 #4063 的 #10（命名统一）下作为一个子任务，包含：

• ESLint 规则 + allowlist 初始数据
• AGENTS.md 更新
• 存量文件分批迁移计划（按目录粒度，每个目录一个 PR）

@doudouOUC 要不要开一个 issue 来跟踪？我可以帮忙把 ESLint 规则的 PoC 先跑出来 🤔

回复 @doudouOUC

关于「kebab-case 在 Node.js 生态中更常见」这个说法，确实说得过于笼统了 —— 不同子领域差异很大，web 框架圈和 infra/AI 圈的习惯完全不一样。这个论据站不住，收回。

但你后面提的可观测性管线兼容性这个角度是真正的硬论据，比我的「生态常见」说法有说服力得多：

• 文件名 kebab-case → span/metric name snake_case，变换路径最短（- → _，一一对应）
• Prometheus 强制 snake_case，OTel semconv 用 . 分隔（gen_ai.usage.input_tokens）
• Arize Phoenix / Langfuse 的 span 树和 trace 视图确实对 kebab/snake 的可读性更好
• 如果上游文件名就是 camelCase，到下游每个集成点都要做一次不规则的 case 转换，容易出错

这个思路其实和你在 PR 里做 GenAI semconv dual-emit 的逻辑是一脉相承的 —— 命名规范不是美学问题，是跨系统互操作的工程约束。

关于 AGENTS.md 的写法，你说得对，应该写成变换规则而不只是静态命名规范。建议这样措辞：

- **File naming**: Use `kebab-case` for all new `.ts` files.
  Conversion to downstream identifiers is mechanical:
  - File → tool name: replace `-` with `_` (e.g., `task-stop.ts` → `task_stop`)
  - File → OTel span attr: replace `-` with `.` prefix as needed
  - File → Prometheus metric: replace `-` with `_`
  Existing `camelCase` files are legacy; rename opportunistically when touched.

这样写的好处是贡献者（和 AI agent）一看就知道为什么选 kebab-case、怎么变换到下游系统，而不是单纯记一条风格规则。

ESLint check-file + AGENTS.md 双层防线 + 独立 issue 归到 #4063 #10 下，这个路径不变。要开 issue 的话我可以先把 ESLint PoC 和 AGENTS.md diff 准备好。

回复 @doudouOUC 关于共享工具函数和变换链路的问题

刚扫了一遍代码库，结论比较直接：

目前没有共享的 case 转换工具函数

• 无 toSnakeCase()、toKebabCase()、camelToSnake() 之类的 util
• 无第三方 case 库引用（lodash、change-case 等都没有）
• tool-names.ts 里的 tool name 全部是硬编码字面量（'task_stop'、'web_fetch'）
• telemetry/constants.ts 和 session-tracing.ts 里的 OTel attribute 也全部是硬编码字符串（'qwen-code.tool_call'、'gen_ai.usage.input_tokens'）
• Prometheus export 层的 metric name 同样是手写的

也就是说，你提到的 ~1200 LOC 双写路径里，每一跳的命名都是手工写死的字面量，没有经过任何程序化的 case 变换。文件名 → tool name → OTel attr → Prometheus metric 之间不存在自动转换链路。

这对 kebab-case 迁移 issue 的 scope 意味着什么

文件名 kebab-case 化能解决的只是第一跳的一致性：task-stop.ts → task_stop（tool name）。后面几跳（OTel attr 用 .、Prometheus 用 _）是各系统自己的规范，和文件名无关 —— 它们本来就该用各系统的 native convention。

所以建议 kebab-case 迁移 issue 的 scope 收窄为：

• 文件名统一 kebab-case（ESLint 规则 + AGENTS.md 变换规则文档）
• tool name 保持 snake_case，与文件名形成 - → _ 的机械映射
• OTel attr / Prometheus metric 命名不属于这个 issue 的范围（它们有自己的 semconv 规范）

关于你提的「变换规则文档」，与其做代码级的 snake_case() util（当前没有实际需求），不如在 AGENTS.md 里把映射关系写清楚就够了 —— 因为现阶段这些变换都是开发者/AI agent 在写代码时心算的，不是运行时计算的。AGENTS.md 的措辞上一轮已经给了草稿，可以直接用。

要开 issue 的话，scope 就是 ESLint 规则 + AGENTS.md 更新 + 存量文件分批迁移，不涉及运行时代码改动。我可以先准备 PoC。