Thanks for the PR @doudouOUC!

Template: ✓ — all required headings present (What this PR does, Why it's needed, Reviewer Test Plan with How to verify / Evidence / Tested on, Risk & Scope, Linked Issues). Minor: <details> Chinese translation block still missing from the PR body — nice to have but not a blocker.

On direction: this addresses T3.9 from the tracking issue (#4514), adding read-only HTTP/ACP endpoints for hook configuration status. Remote clients (web-shell, SDK consumers) currently can't query hooks — this closes that gap cleanly. The scope (hooks portion only, leaving /extensions and /settings for later) is well-scoped and low-risk. No product-direction concerns.

On approach: +600 lines across 12 files is proportional — types flow through three packages (acp-bridge, cli, sdk-typescript) following the established pattern for status endpoints. The workspace/session endpoint split mirrors MCP, skills, and providers. The idle factory for event metadata and the serializeHookConfig switch are straightforward. One thought: HOOK_EVENT_DESCRIPTIONS (acpAgent.ts) and IDLE_HOOK_EVENTS (status.ts) carry the same event descriptions in two places — understandable given the package boundary (acp-bridge can't import from cli), but worth keeping in sync.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Thanks — updated the PR body to follow the required template headings (What this PR does, Why it's needed, Reviewer Test Plan with Evidence/Tested on, Risk & Scope, Linked Issues).

📋 Review Summary

This PR adds read-only HTTP/ACP endpoints for hook configuration status (/workspace/hooks and /session/:id/hooks), addressing issue #4514 T3.9. The implementation is well-structured, follows existing patterns, and provides comprehensive type safety. The changes span 12 files with ~600 additions, introducing diagnostic surfaces for both workspace-level config-sourced hooks and runtime session hooks registered by skills.

🔍 General Feedback

• Strong architectural consistency: The implementation follows the established workspace/* vs session/:id/* split used by other status endpoints (MCP, skills, providers, etc.)
• Excellent type safety: Uses Record<HookEventName, string> for event descriptions and Record<HookEventName, ServeHookEventMeta> for idle factory—new enum members will trigger compile errors as intended
• Security-conscious design: Function hook callbacks (callback/onHookSuccess) are correctly stripped from serialization, exposing only metadata
• Forward-compatibility: ServeUnknownHookConfig without index signature preserves discriminated union narrowing; default branch in serializeHookConfig handles future hook types gracefully
• Clean separation: Read-only status endpoints are cleanly separated from mutation control-plane methods via SERVE_STATUS_EXT_METHODS vs SERVE_CONTROL_EXT_METHODS

🎯 Specific Feedback

🟡 High

• packages/acp-bridge/src/status.ts:719 — ServeUnknownHookConfig includes name, description, timeout, and statusMessage as optional fields, but unknown hook types may not have these fields. Consider making all fields except type truly optional or document that unknown types should only provide type. The current design assumes all future hook types will share these common fields, which may not hold.
  • Suggestion: Either document this assumption or make the interface more flexible: export interface ServeUnknownHookConfig { type: string; [key: string]: unknown; } with a comment about metadata-only serialization.

🟢 Medium

• packages/cli/src/acp-integration/acpAgent.ts:2158-2162 — The buildWorkspaceHooksStatus error handler catches all errors but only attempts to recover disabled state. If config.getDisableAllHooks() also fails (comment acknowledges "config may be in a broken state"), it silently falls back to false. This could mask configuration issues.

  • Suggestion: Consider adding the error to the errors array even when recovery fails, so operators can see that the hook system is in an inconsistent state.

• packages/cli/src/acp-integration/acpAgent.ts:2096-2107 — The serializeHookConfig function for 'function' type hooks explicitly excludes the callback field (good for security), but this is implicit rather than explicit. A future contributor might accidentally add it.

  • Suggestion: Add an inline comment like // NOTE: callback intentionally omitted—metadata only to make the security decision explicit for future maintainers.

• packages/sdk-typescript/src/daemon/types.ts:1335-1351 — DaemonHookEventName uses (string & {}) widening for forward-compatibility, which is correct. However, the SDK duplicates all hook config interfaces (DaemonCommandHookConfig, etc.) rather than importing from @qwen-code/qwen-code-core or acp-bridge. This creates potential drift.

  • Suggestion: Consider re-exporting or importing the canonical types from core/acp-bridge if possible, or add a compile-time assertion that the SDK types match the bridge types.

🔵 Low

• packages/acp-bridge/src/status.ts:672-676 — ServeHookEventMeta has matcherKind as optional, which is correct for events like PostToolBatch and UserPromptSubmit that don't have matchers. However, consumers might forget to check for undefined.

  • Suggestion: Consider adding a JSDoc comment: /** Absent for events that don't support filtering (e.g., PostToolBatch, UserPromptSubmit) */

• packages/cli/src/acp-integration/acpAgent.ts:2044-2060 — HOOK_MATCHER_KINDS mapping is a Partial<Record<...>>, which is correct. The code handles missing entries properly. Consider adding a compile-time check (e.g., a type assertion function) that ensures every HookEventName is either in the mapping or intentionally omitted.

• packages/cli/src/ui/commands/hooksCommand.ts:49,199 — Changing supportedModes from ['interactive'] to ['interactive', 'non_interactive', 'acp'] is correct and aligns with the PR goals. The PR description mentions this change but doesn't explicitly test the ACP mode output path. The test plan mentions "/hooks slash command enabled for ACP mode (text output via listCommand)"—ensure the non-interactive text output is verified.

• packages/sdk-typescript/src/daemon/DaemonClient.ts:511-530 — The new workspaceHooks() and sessionHooks() methods follow the existing pattern well. Consider adding a brief JSDoc comment to each method explaining what it returns (e.g., "Returns workspace-level hook configuration from all sources").

✅ Highlights

• Excellent type design: The ServeHookEntry interface correctly captures all necessary metadata (eventName, config, source, matcher, sequential, enabled, hookId, skillRoot) without over-engineering
• Smart error handling: The buildWorkspaceHooksStatus method gracefully handles missing hook systems and configuration errors, returning structured error cells
• Consistent patterns: The idle factory createIdleWorkspaceHooksStatus follows the same pattern as createIdleWorkspaceMcpStatus, createIdleWorkspaceSkillsStatus, etc.
• Comprehensive event metadata: The IDLE_HOOK_EVENTS map provides a complete catalog of all 18 hook events with descriptions and matcher kinds, enabling clients to build rich UIs without hardcoding
• Clean API surface: HTTP routes (GET /workspace/hooks, GET /session/:id/hooks) are minimal and delegate properly to workspace service and bridge layers

Review response summary

Code review

Reviewed the full diff (824 lines, 12 files). The implementation is clean and follows established patterns precisely.

What works well:

• HTTP routes in server.ts follow the exact pattern of existing status endpoints (GET /workspace/mcp, GET /session/:id/skills, etc.) — buildWorkspaceCtx / requireSessionId + sendBridgeError error handling
• createIdleWorkspaceHooksStatus in status.ts follows the same idle factory pattern as createIdleWorkspaceMcpStatus, createIdleWorkspaceSkillsStatus, etc.
• serializeHookConfig correctly strips callback from function hooks (security-conscious — metadata only)
• buildWorkspaceHooksStatus gracefully handles missing hook system and config errors, returning structured error cells
• The Record<HookEventName, string> typing on HOOK_EVENT_DESCRIPTIONS and IDLE_HOOK_EVENTS ensures compile errors when new enum members are added — good forward-compat design
• SDK types (DaemonHookEventName with (string & {}) widening) handle forward-compat correctly
• hooksCommand.ts mode expansion from ['interactive'] to ['interactive', 'non_interactive', 'acp'] is minimal and correct

No blockers found. Two observations (not blocking):

1. env (command hooks) and headers (HTTP hooks) are serialized as-is — they may contain secrets. Author's rationale: auth-gated daemon, same exposure as CLI config list. Reasonable for V1.
2. The event metadata duplication between acpAgent.ts and status.ts is a consequence of the package boundary — not a bug, just a sync risk.

Testing

This PR adds new daemon HTTP endpoints (no modification of existing behavior). CI passed (all checks successful). Real-scenario testing requires the full daemon infrastructure (qwen serve + ACP child process), which is not available in this CI environment without the daemon_mode_b_main base branch runtime. The PR's test plan provides clear curl-based verification steps that a reviewer with the daemon running can follow.

CI status: All checks successful (1 skipped — Qwen PR Review action).

— Qwen Code · qwen3.7-max

Reflection

Stepping back: this PR does exactly what T3.9 asks for — exposes hook configuration as read-only HTTP/ACP endpoints so remote clients can query them. The implementation is mechanical in the best sense: it follows the established pattern for status endpoints step by step (types → bridge → agent → routes → workspace service → capabilities → SDK mirror → command mode expansion). Nothing surprising, nothing over-engineered.

My independent proposal would have been essentially identical — same package split, same type flow, same idle factory approach. The PR matches or exceeds it.

The serializeHookConfig function is verbose (~60 lines of repetitive spreads), but this is actually the right choice here: explicit per-type field mapping is easier to audit for security (making sure callback isn't leaked) than a generic abstraction. A future contributor reading this code will immediately see what gets serialized and what doesn't.

No correctness bugs, no security holes, no regressions. CI passes. The only outstanding CHANGES_REQUESTED was from the bot's initial template review — the author has since updated the template to match. That review is stale.

Verdict: approve. This is ready to merge once a maintainer dismisses the stale changes-requested review.

— Qwen Code · qwen3.7-max

Review response: wenshao round 1 — all items fixed in 7d8ed8a83

Review round — wenshao suggestion (post-approval)

Added workspaceHooksImpl / sessionHooksImpl to FakeBridgeOpts with call counters, plus two happy-path it(...) blocks for GET /workspace/hooks (200 + shape + call count) and GET /session/:id/hooks (200 + shape + call count).

Local Verification Report

Branch: feat/serve-hooks-status → daemon_mode_b_main
Commit: 16de39cb5
Environment: macOS Darwin 25.4.0 / Node.js / Vitest 3.2.4

Unit Tests

Test failure analysis (PR branch 17 failed vs base branch 15 failed):

New test failure — returns workspace hooks status from the bridge:

• Test expects only PreToolUse in the events object, but the implementation's HOOK_EVENT_DESCRIPTIONS returns all 18 hook events
• Test description mismatch: "Before a tool is executed" vs actual "Before tool execution"
• Root cause: Test assertion is too narrow — only checks one event instead of the full event map. Should either assert the complete map or use toMatchObject for partial matching.

ESLint

All 12 source files pass with --max-warnings 0 ✅

Files linted: bridge.ts, bridgeTypes.ts, status.ts (acp-bridge), acpAgent.ts, capabilities.ts, index.ts, server.ts, workspace-service/index.ts, workspace-service/types.ts, hooksCommand.ts (cli), DaemonClient.ts, types.ts (sdk-typescript)

TypeScript Type Check

CLI typecheck delta (after core + acp-bridge rebuild):

• 20 CLI typecheck errors were caused by stale dist/ in acp-bridge — the PR's new types (ServeHookConfig, ServeWorkspaceHooksStatus, IDLE_HOOK_EVENTS, etc.) were not in the built output. All 20 resolved after npm run build --workspace=packages/core && npm run build --workspace=packages/acp-bridge.
• 3 remaining "new" errors in Session.ts are the standard @google/genai dual-package resolution pattern (same as 40+ other pre-existing errors in HistoryReplayer.ts, BaseJsonOutputAdapter.ts, useGeminiStream.ts, etc.)
• 0 structurally new typecheck errors introduced by this PR ✅

Whitespace Check

git diff --check: clean ✅

Summary

Action Items Before Merge

1. Fix test assertion in returns workspace hooks status from the bridge: update expected events object to match the full HOOK_EVENT_DESCRIPTIONS map (18 events), or use expect(...).toMatchObject(...) for partial matching.

Verified locally by wenshao

Thanks for the thorough local verification!

Fixed the test assertion bug in 962c38f:

1. Missing queryWorkspaceStatus dispatch: fakeBridge.queryWorkspaceStatus() was missing the qwen/status/workspace/hooks case, causing it to fall through to idle() which returns all 18 events from IDLE_HOOK_EVENTS. Added the dispatch case.
2. Wrong description string: 'Before a tool is executed' → 'Before tool execution' to match IDLE_HOOK_EVENTS.

Review round — wenshao comments (rebase + fix)

Local Verification Report

Branch: feat/serve-hooks-status → daemon_mode_b_main
Environment: macOS Darwin 25.4.0, Node.js local

TypeScript Compilation (tsc --noEmit)

acp-bridge note: The +1 error is PostToolBatch does not exist in type 'Record<HookEventName, ...>' in the new IDLE_HOOK_EVENTS constant. PostToolBatch exists in core's source enum (hooks/types.ts:286) but the locally compiled dist/ is stale. CI builds fresh and this resolves. The pre-existing error (DaemonBridgeTelemetryMetrics) is the same on both branches.

Unit Tests (vitest)

server.test.ts failure: @qwen-code/acp-bridge/mcpTimeouts import resolution fails in vitest on both the PR branch AND daemon_mode_b_main. This is a pre-existing stale dist/ issue — CI builds fresh dist before running tests, so this doesn't reproduce there.

Code Review

This PR adds hooks diagnostic HTTP/ACP surfaces for issue #4514 T3.9:

New types (status.ts):

• ServeWorkspaceHooksStatus / ServeSessionHooksStatus — structured status payloads
• ServeHookEntry with discriminated ServeHookConfig union (command / http / function / prompt / unknown)
• IDLE_HOOK_EVENTS — static event catalog with descriptions and matcher kinds
• createIdleWorkspaceHooksStatus() — fallback for pre-channel-ready state

Server routes (server.ts):

• GET /workspace/hooks — workspace-level hook configuration
• GET /session/:id/hooks — session-scoped hook status

ACP integration (acpAgent.ts):

• serializeHookConfig() — serializes HookConfig to the wire ServeHookConfig type with undefined-conditional spreading (no nulls sent)
• buildWorkspaceHooksStatus() / buildSessionHooksStatus() — query config.getHookSystem() with error fallback

SDK types (sdk-typescript/types.ts):

• Mirror types with Daemon prefix and forward-compat (string & {}) arm on DaemonHookEventName
• DaemonClient.workspaceHooks() / sessionHooks() — typed fetch methods

Other:

• /hooks command now available in non_interactive and acp modes
• Capability registry entries workspace_hooks / session_hooks
• Browser bundle size cap bumped 106K → 108K
• Server tests: 2 new test cases for workspace/session hooks routes

Verdict

✅ Ready to merge — No regressions. All bridge tests pass. Server test failure and TSC errors are pre-existing stale-dist issues confirmed on daemon_mode_b_main. Clean implementation following existing status surface patterns.