📋 Review Summary

This PR adds telemetry coverage for daemon HTTP request spans by introducing qwen-code.client_id attribute extraction from the X-Qwen-Client-Id header and bridging it through prompt.dispatch spans. It also adds span attributes for permission vote routes and introduces the addDaemonRequestAttribute() helper for post-rebase prompt ID enrichment. The implementation is clean, well-tested, and follows existing telemetry patterns.

🔍 General Feedback

• The changes are minimal and focused on the stated objectives—no speculative features added
• Test coverage is comprehensive with both inclusion and omission cases for the new attributes
• The code follows existing patterns in the codebase for telemetry attribute handling
• Good defensive programming with try-catch blocks around telemetry operations
• The PR description clearly outlines the scope and test results

🎯 Specific Feedback

🟢 Medium

• File: packages/cli/src/serve/server.ts:2113-2120 - The inline client ID validation logic in daemonTelemetryMiddleware duplicates the logic in parseClientIdHeader function (lines 2098-2109). Consider refactoring to reuse parseClientIdHeader or extract the validation into a shared helper to avoid drift:

// Current inline logic:
const rawClientId = req.get(CLIENT_ID_HEADER);
const clientId =
  rawClientId !== undefined &&
  rawClientId !== '' &&
  rawClientId.length <= MAX_CLIENT_ID_LENGTH &&
  CLIENT_ID_RE.test(rawClientId)
    ? rawClientId
    : undefined;

// Could be:
const rawClientId = req.get(CLIENT_ID_HEADER);
const clientId = parseClientIdHeader(req, res) ?? undefined;
// Note: parseClientIdHeader sends 400 response on invalid, so adjust as needed

🔵 Low

• File: packages/core/src/telemetry/daemon-tracing.ts:168-177 - The addDaemonRequestAttribute function name is slightly misleading—it sets an attribute on the active span, not specifically on the request span. Consider renaming to addActiveSpanAttribute or adding a JSDoc comment clarifying its intended use case (i.e., only call when within a daemon request span context).

• File: packages/cli/src/serve/server.ts:279-281 - The return type change for resolveDaemonTelemetryRoute adds permissionRequestId?: string but the function comment/type documentation isn't updated. Consider adding a brief JSDoc explaining the function's purpose and return structure.

• File: packages/acp-bridge/src/bridge.test.ts:96-154 - The test now passes clientId via a fourth parameter to sendPrompt. Consider adding a comment explaining why clientId is passed separately from the PromptRequest object (i.e., it's telemetry context, not part of the prompt payload).

✅ Highlights

• Excellent test coverage with 8 test cases in daemon-tracing, including edge cases for attribute omission and no-op behavior
• The permission route regex patterns are well-structured and follow the existing pattern for session action routes
• Good use of conditional spread syntax for optional attributes—clean and readable
• The addDaemonRequestAttribute helper is a solid foundation for future telemetry enrichment (plumbing for feat(telemetry): cover qwen serve daemon end-to-end with OpenTelemetry #4554)
• Proper error isolation with try-catch blocks ensuring telemetry never affects request handling

Maintainer Verification Report

Reviewer: @wenshao
Environment: macOS Darwin 25.4.0 / Node.js local build
Branch: feat/daemon-otel-client-id-permission → feat/daemon-otel-e2e-design

Build

Unit Tests

New Test Coverage

Code Review Notes

Changes are well-scoped and correct:

1. daemon-tracing.ts: DaemonRequestSpanOptions extended with optional clientId / permissionRequestId; both conditionally spread into span attributes — clean pattern consistent with existing sessionId / workspaceHash handling. New addDaemonRequestAttribute() helper is correctly defensive (try/catch, optional chaining on getSpan()).

2. server.ts: resolveDaemonTelemetryRoute() now matches two new permission routes (POST /session/:id/permission/:requestId and POST /permission/:requestId) with proper regex extraction. clientId extraction from X-Qwen-Client-Id header validates against CLIENT_ID_RE (/^[A-Za-z0-9._:-]+$/) and MAX_CLIENT_ID_LENGTH (128) — reuses existing constants, consistent with the existing parseClientId path.

3. bridge.ts: Minimal 3-line change — conditionally spreads context.clientId into prompt.dispatch span attributes. Pattern matches the existing conditional spread style.

4. index.ts: Clean re-export of addDaemonRequestAttribute.

No concerns found: No security issues (input is validated before use), no breaking changes, no unnecessary abstractions. The addDaemonRequestAttribute helper is well-motivated as plumbing for #4554 promptId enrichment.

Verdict

✅ Ready to merge — all builds and tests pass, code is clean, well-tested, and consistent with existing patterns.