📋 Review Summary

This PR is described as a follow-up to #4730 to restore isForkSubagentEnabled() functionality lost during merge conflict resolution. However, the diff contains extensive unrelated changes beyond the stated scope, including MCP transport pool additions, runtime MCP server management, permission audit infrastructure, daemon logger integration, and a 6184-line test file deletion. The core fork subagent changes are straightforward, but the additional modifications significantly expand the PR's surface area and should be carefully evaluated for whether they belong in this "fix" PR or should be split into separate PRs.

🔍 General Feedback

• Scope creep concern: The PR title/description suggests a narrow merge-resolution fix, but the actual diff touches telemetry interfaces, daemon serving infrastructure, MCP client management, permission auditing, and deletes an entire test file. This makes review difficult and increases merge risk.
• Positive: The core fork subagent feature gate implementation (isForkSubagentEnabled()) is clean and follows the established pattern of combining env var + programmatic config.
• Architectural consistency: The telemetry runtime config extension is well-structured, adding interface methods that match the existing pattern.
• Test deletion: The removal of httpAcpBridge.test.ts (6184 lines) is significant and needs justification — is this file being moved, replaced, or are these tests genuinely no longer needed?

🎯 Specific Feedback

🔴 Critical

• packages/cli/src/serve/httpAcpBridge.test.ts:1 — Entire test file deleted (6184 lines). This is a critical concern for a "fix" PR. If these tests are being moved elsewhere, the new location should be referenced. If they're obsolete, the PR description should explain why. Deleting this much test coverage without explanation risks regressions going undetected.

• packages/core/src/config/config.ts:1024-1041 — disabledTools field mutability change. Changing from private readonly disabledTools: ReadonlySet<string> to private disabledTools: ReadonlySet<string> with a new setDisabledTools() method is a significant architectural change that affects thread-safety guarantees. The JSDoc mentions this is for daemon mutation via setWorkspaceToolEnabled, but this belongs in a separate refactor PR, not a merge-fix PR.

• packages/core/src/config/config.ts:1041 — New runtimeMcpServers Map added. This introduces a whole new MCP server layer (runtime overlay over settings layer) with addRuntimeMcpServer() and removeRuntimeMcpServer() methods. This is feature work, not a merge fix.

🟡 High

• packages/core/src/config/config.ts:919-935 — skipMcpDiscovery option added. The JSDoc references "proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 commit 6 review fix" and describes closing a "2N subprocess leak." This is substantive bugfix/feature work that should be its own PR with proper test coverage.

• packages/core/src/config/config.ts:2653-2702 — setDisabledTools() method with extensive JSDoc. The JSDoc references "feat(serve): approval / tools / init / MCP-restart mutation routes (#4175 Wave 4 PR 17) #4282 fold-in 5" and describes runtime tool toggling semantics. This is part of a larger feature initiative and should be reviewed in that context.

• packages/core/src/config/config.ts:2702-2754 — MCP transport pool integration (mcpTransportPool, setMcpTransportPool(), getMcpTransportPool()). References "F2 (proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 commit 4)" — this is major infrastructure work for workspace-shared MCP transport pooling.

• packages/cli/src/serve/runQwenServe.ts:119-122 — Telemetry runtime config implementation. While the implementation is correct, it's part of a much larger telemetry expansion that includes many other changes throughout the file (permission audit ring, daemon logger, device flow registry disposal, etc.).

🟢 Medium

• packages/core/src/tools/agent/fork-subagent.ts:4-13 — JSDoc removal. The removed JSDoc comment actually provides valuable context about the feature gate's two conditions (env var + interactive session). Consider keeping the documentation even if the function body is simplified.

• packages/cli/src/serve/runQwenServe.ts:entire file — Massive expansion (~900+ lines added). Includes permission policy parsing, context filename extraction, audit ring allocation, filesystem factory resolution, diagnostic sink integration, and extensive shutdown telemetry handling. All of this is valuable work but belongs in separate PRs with focused scopes.

🔵 Low

• packages/core/src/config/config.ts:3334 — Blank line before isComputerUseEnabled(). Minor formatting inconsistency — the new isForkSubagentEnabled() method ends with a trailing blank line that doesn't match the style of adjacent methods.

✅ Highlights

• Fork subagent feature gate: The core isForkSubagentEnabled() implementation is clean, combining env var (QWEN_CODE_ENABLE_FORK_SUBAGENT=1) with programmatic config and the isInteractive() check for proper feature gating.

• Telemetry interface consistency: The TelemetryRuntimeConfig interface additions follow the existing getter pattern and integrate cleanly with the daemon telemetry setup.

• Daemon logger integration: The DaemonLogger wiring throughout runQwenServe.ts improves observability for daemon lifecycle events, replacing ad-hoc writeStderrLine calls with structured logging.

Recommendation

This PR needs to be split. The stated purpose is "add missing isForkSubagentEnabled from main merge," but the diff includes:

1. ✅ Fork subagent feature (the stated fix)
2. ❌ MCP transport pool infrastructure (proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175)
3. ❌ Runtime MCP server overlay (proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 T2.8)
4. ❌ Disabled tools mutability (feat(serve): approval / tools / init / MCP-restart mutation routes (#4175 Wave 4 PR 17) #4282)
5. ❌ Permission audit ring + publisher (F3)
6. ❌ Daemon logger integration
7. ❌ Telemetry expansion
8. ❌ 6184-line test file deletion

Suggested action: Create a minimal PR with only:

• packages/core/src/config/config.ts: forkSubagentEnabled param/field + isForkSubagentEnabled() method
• packages/core/src/tools/agent/fork-subagent.ts: isForkSubagentEnabled() export
• packages/core/src/telemetry/runtime-config.ts: isInteractive() method (required dependency)
• packages/cli/src/serve/runQwenServe.ts: isInteractive() implementation (required dependency)

Move all other changes to their respective feature/fix PRs with proper context and test coverage.