From bae6141398854280dec9abf42ef037281221bac3 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:45:17 +0800 Subject: [PATCH] fix(hooks-claude): build subagent payloads from base(), run SubagentStop in the child cwd, drop agentType MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the D agentType removal + two #124 review findings on the CC bridge's subagent points: - **Payloads from base()**: `subagentStart/StopPayload` bypassed `base()`, so the SubagentStart/SubagentStop stdin payloads omitted the CC-promised `session_id` and `cwd`. Replaced both with a single `subagentPayload()` built from `base(child)` (the child's session_id/cwd when the child is available) + `agent_id` + `agent_type` (+ `stop_hook_active` on Stop). - **SubagentStop runs in the child cwd**: the listener called `runPoint(..., {})` with no agent, so the hook ran in the executor/server cwd. It now looks the child up via `ctx.get('agents').get(info.id)` — still recoverable because `subagent/end` fires from the service's detached `.then` BEFORE the tool caller disposes the child — and passes `{ agent: child }`, matching SubagentStart. New regression: server cwd ≠ child cwd, a `pwd` SubagentStop hook proves it ran in the CHILD workspace (proven red by neutering the lookup). - **agent_type is a constant**: `info.agentType` no longer exists (removed on the subagent branch); both points now report the `SUBAGENT_TYPE = "general-purpose"` constant (Claude Code's Task-tool default), so a hooks.json default/`*`/empty `agent_type` matcher fires. Updated the README matcher-subject note and the bridge/coverage tests (dropped their agentType emits). - **e2e comment**: hooks.e2e.ts said `./hooks.json` loads from the session cwd; corrected to process-level (server launch cwd), with the hook itself running in the session cwd. --- examples/acp-agent/tests/hooks.e2e.ts | 11 ++-- packages/hooks/hooks-claude/README.md | 2 +- packages/hooks/hooks-claude/src/index.ts | 46 +++++++++++++---- .../hooks/hooks-claude/tests/bridge.spec.ts | 4 +- .../hooks/hooks-claude/tests/coverage.spec.ts | 51 ++++++++++++++++--- 5 files changed, 89 insertions(+), 25 deletions(-) diff --git a/examples/acp-agent/tests/hooks.e2e.ts b/examples/acp-agent/tests/hooks.e2e.ts index 19a80df03c..40a8d37457 100644 --- a/examples/acp-agent/tests/hooks.e2e.ts +++ b/examples/acp-agent/tests/hooks.e2e.ts @@ -19,11 +19,14 @@ import { /** * With-key e2e: the Claude Code hook bridge running against the REAL acp-agent * subprocess and the REAL model. The example `cordis.yml` loads `dsh-hooks-claude` - * pointed at `./hooks.json` in the session cwd; this test writes a `hooks.json` - * with a PreToolUse hook that BLOCKS every bash command, then asks the live model - * to write a file — and verifies the WORLD (the file never appears on disk), + * with a PROCESS-LEVEL `configPath` of `./hooks.json`, resolved once at load + * against the ACP server's launch cwd (NOT per-session); this test sets that + * launch cwd to the temp workspace and writes a `hooks.json` there with a + * PreToolUse hook that BLOCKS every bash command, then asks the live model to + * write a file — and verifies the WORLD (the file never appears on disk), * proving the hook actually intercepted execution rather than the agent merely - * claiming it couldn't. Key-gated; owns and disposes its subprocess. + * claiming it couldn't. (The hook itself then runs in the session cwd.) + * Key-gated; owns and disposes its subprocess. * * A keyless companion lives in acp.e2e.ts (stdout purity + session/new); the * full hook-fires-end-to-end transcript is the keyless `hook-prompt-block` diff --git a/packages/hooks/hooks-claude/README.md b/packages/hooks/hooks-claude/README.md index 82980ff601..f9520f69be 100644 --- a/packages/hooks/hooks-claude/README.md +++ b/packages/hooks/hooks-claude/README.md @@ -41,7 +41,7 @@ The hooks **themselves** run in the agent's session workspace: for the agent-sco | `SubagentStart` | `subagent/start` (emit) | additionalContext → `agent.inject()` into the live child | | `SubagentStop` | `subagent/end` (emit) | observe-only | -The matcher subject is the tool name (`PreToolUse`/`PostToolUse`), the session source (`SessionStart`), or the child's agent type (`SubagentStart`/`SubagentStop`); `UserPromptSubmit`/`Stop` ignore matchers. Multiple file-configured hooks on one point run **serially, in config order**, and fold most-restrictively (`deny > ask > allow`, see `dsh-hook-protocol`); serial keeps each hook's `hook/invoked`/`hook/result` pair adjacent in the log, and the fold is order-independent for the decision (see the RFC's "run serially, not concurrently" note). +The matcher subject is the tool name (`PreToolUse`/`PostToolUse`), the session source (`SessionStart`), or a constant `agent_type` of `general-purpose` (`SubagentStart`/`SubagentStop` — the harness subagent seam carries no per-kind label, so the bridge reports Claude Code's own Task-tool default; a default/`*`/empty `agent_type` matcher fires, a specific-kind matcher does not); `UserPromptSubmit`/`Stop` ignore matchers. Multiple file-configured hooks on one point run **serially, in config order**, and fold most-restrictively (`deny > ask > allow`, see `dsh-hook-protocol`); serial keeps each hook's `hook/invoked`/`hook/result` pair adjacent in the log, and the fold is order-independent for the decision (see the RFC's "run serially, not concurrently" note). ## Context source diff --git a/packages/hooks/hooks-claude/src/index.ts b/packages/hooks/hooks-claude/src/index.ts index 5b56e5f014..7e21f7ea82 100644 --- a/packages/hooks/hooks-claude/src/index.ts +++ b/packages/hooks/hooks-claude/src/index.ts @@ -271,10 +271,15 @@ export function apply(ctx: Context, config: Config): void { // --- SubagentStart / SubagentStop: observe-only emits (the subagent seam is // observe-only this cut). A SubagentStart hook's additionalContext is injected - // into the live child; SubagentStop only observes. No matcher subject. --- + // into the live child; SubagentStop only observes. Both look the live child up + // so the hook runs in the child's session workspace and the payload carries + // the child's session_id/cwd (see subagentPayload). The matcher subject is the + // CC-default `agent_type` (SUBAGENT_TYPE) — the harness seam carries no + // per-kind label, so a config's default/`*`/empty agent_type matcher fires and + // a specific-kind matcher does not (documented in the RFC). --- ctx.on('subagent/start', (info) => { const child = ctx.get('agents')?.get(info.id) - void runPoint('SubagentStart', info.agentType ?? '', subagentStartPayload(info), { ...child ? { agent: child } : {} }) + void runPoint('SubagentStart', SUBAGENT_TYPE, subagentPayload('SubagentStart', info, child), { ...child ? { agent: child } : {} }) .then((merged) => { const context = contextFrom(merged) if (context && child) child.inject(context.content, { source: context.source }) @@ -282,13 +287,24 @@ export function apply(ctx: Context, config: Config): void { .catch((error: unknown) => { ctx.logger.warn(`hooks-claude: SubagentStart hook failed: ${String(error)}`) }) }) ctx.on('subagent/end', (info) => { - // No `.then`/inject here (SubagentStop only observes) and no session is - // passed, so runPoint cannot reject — no `.catch` is needed (one would be - // dead code). The observe-only run is fire-and-forget. - void runPoint('SubagentStop', info.agentType ?? '', subagentStopPayload(info), {}) + // Look up the child (still recoverable: `subagent/end` fires from the + // service's detached `.then` BEFORE the tool caller's `await run.result` + // disposes it) so the hook runs in the child's cwd, not the server default. + // No `.then`/inject (SubagentStop only observes) and no session is passed, so + // runPoint cannot reject — no `.catch` is needed. Fire-and-forget. + const child = ctx.get('agents')?.get(info.id) + void runPoint('SubagentStop', SUBAGENT_TYPE, subagentPayload('SubagentStop', info, child), { ...child ? { agent: child } : {} }) }) } +/** + * The `agent_type` value the bridge reports for SubagentStart/Stop. The harness + * subagent seam carries no per-kind label, so the bridge uses Claude Code's own + * Task-tool default — a hooks.json with a default/`*`/empty `agent_type` matcher + * fires; a config matching a specific kind (e.g. `code-reviewer`) does not. + */ +const SUBAGENT_TYPE = 'general-purpose' + // --- Per-event stdin payloads (the CC DIALECT shape). Field names match CC's // hook input schema; this is the part a bridge owns. --- @@ -330,9 +346,17 @@ function postToolPayload(exec: ToolExecution, result: ToolExecutionResult): Reco function stopPayload(agent: Agent): Record { return { ...base(agent, 'Stop'), stop_hook_active: false } } -function subagentStartPayload(info: { id: string; agentType?: string }): Record { - return { hook_event_name: 'SubagentStart', agent_id: info.id, ...info.agentType !== undefined ? { agent_type: info.agentType } : {} } -} -function subagentStopPayload(info: { id: string; agentType?: string }): Record { - return { hook_event_name: 'SubagentStop', agent_id: info.id, stop_hook_active: false, ...info.agentType !== undefined ? { agent_type: info.agentType } : {} } +/** + * Build a SubagentStart/SubagentStop payload from the CC base (the child's + * `session_id`/`cwd` when the child agent is available) plus the subagent-hook + * fields. `agent_type` is the CC-default {@link SUBAGENT_TYPE}; `stop_hook_active` + * is present on SubagentStop only (the loop-guard flag, always false this cut). + */ +function subagentPayload(event: 'SubagentStart' | 'SubagentStop', info: { id: string }, child: Agent | undefined): Record { + return { + ...base(child, event), + agent_id: info.id, + agent_type: SUBAGENT_TYPE, + ...event === 'SubagentStop' ? { stop_hook_active: false } : {}, + } } diff --git a/packages/hooks/hooks-claude/tests/bridge.spec.ts b/packages/hooks/hooks-claude/tests/bridge.spec.ts index 0b6afd7e4c..3e36231e66 100644 --- a/packages/hooks/hooks-claude/tests/bridge.spec.ts +++ b/packages/hooks/hooks-claude/tests/bridge.spec.ts @@ -289,8 +289,8 @@ describe('hooks-claude bridge — SubagentStart / SubagentStop (observe)', () => // Drive the observe-only lifecycle events directly (no real child needed — the // bridge just listens). The agents registry is absent here, so SubagentStart's // child lookup yields undefined and it simply runs the hook. - ctx.emit('subagent/start', { provider: 'inproc', id: AgentId('child-1'), agentType: 'researcher' }) - ctx.emit('subagent/end', { provider: 'inproc', id: AgentId('child-1'), agentType: 'researcher', stopReason: 'completed', lastAssistantMessage: [{ type: 'text', text: 'done' }] }) + ctx.emit('subagent/start', { provider: 'inproc', id: AgentId('child-1') }) + ctx.emit('subagent/end', { provider: 'inproc', id: AgentId('child-1'), stopReason: 'completed', lastAssistantMessage: [{ type: 'text', text: 'done' }] }) // Both hooks run async (detached .then); poll for their marker files rather // than a fixed sleep that flakes under load. diff --git a/packages/hooks/hooks-claude/tests/coverage.spec.ts b/packages/hooks/hooks-claude/tests/coverage.spec.ts index a1b650f33d..097e026d1a 100644 --- a/packages/hooks/hooks-claude/tests/coverage.spec.ts +++ b/packages/hooks/hooks-claude/tests/coverage.spec.ts @@ -185,7 +185,7 @@ describe('hooks-claude coverage — Stop continuation + subagent inject/catch', const injected: string[] = [] const child = { id: AgentId('child-x'), inject: (content: { type: string; text?: string }[]) => { injected.push(content.map(b => b.text ?? '').join('')) }, session: { header: { id: 'child-x' } } } as unknown as Parameters[0] ctx.agents.register(child) - ctx.emit('subagent/start', { provider: 'p', id: AgentId('child-x'), agentType: 'r' }) + ctx.emit('subagent/start', { provider: 'p', id: AgentId('child-x') }) await waitFor(() => injected.includes('child guidance')) expect(injected).toContain('child guidance') }) @@ -236,17 +236,16 @@ describe('hooks-claude coverage — default reasons + sparse payloads', () => { expect(result?.type === 'tool/result' && result.data.content.some(b => b.type === 'text' && b.text.includes('blocked by PostToolUse hook'))).toBe(true) }) - it('SubagentStop with no agentType + a rejecting hook run is contained', async () => { + it('SubagentStop with no registered child runs the hook cleanly (fire-and-forget)', async () => { const d = dir() - // Make the SubagentStop runPoint reject by registering a session whose append - // throws — simplest: a hook that emits invalid output is fine; force the - // .catch by making the session's append throw via a poisoned agent is hard, - // so instead assert the no-agentType payload path runs cleanly (no crash). + // The agents registry has no entry for the id, so the child lookup yields + // undefined and the payload falls back to base(undefined) — assert the + // observe-only SubagentStop run still executes the hook without crashing. const marker = join(d, 'stopran') const s = sh(d, 'stop.sh', `#!/usr/bin/env bash\ntouch "${marker}"\n`) const path = hooks(d, { SubagentStop: [{ hooks: [{ type: 'command', command: s }] }] }) const ctx = await harness(path, new MockAdapter([])) - ctx.emit('subagent/end', { provider: 'p', id: AgentId('child-z'), stopReason: 'completed' }) // no agentType + ctx.emit('subagent/end', { provider: 'p', id: AgentId('child-z'), stopReason: 'completed' }) await waitFor(() => existsSync(marker)) expect(existsSync(marker)).toBe(true) }) @@ -492,6 +491,44 @@ describe('hooks-claude coverage — hook runs in the session cwd, not the server expect(where.endsWith(sessionDir.split('/').pop()!)).toBe(true) await handle.dispose() }) + + it('runs a SubagentStop hook in the CHILD session workspace, not the server cwd', async () => { + // The bug: SubagentStop ran runPoint(..., {}) with no agent, so the hook fell + // back to the executor default (server cwd). SubagentStop must look the child + // up (still recoverable at subagent/end) and run in the CHILD's session cwd. + // Here the executor default and the child session cwd are DIFFERENT dirs; a + // SubagentStop hook writes `pwd` to a marker and we assert it ran in the CHILD + // dir. (Proven to regress: neuter the child lookup and the marker lands in the + // server dir instead.) + const serverDir = dir() + const childDir = dir() + const marker = join(childDir, 'stopwhere') + hooks(serverDir, { SubagentStop: [{ hooks: [{ type: 'command', command: 'pwd > stopwhere' }] }] }) + const ctx = new Context() + await ctx.plugin(LlmService) + await ctx.plugin(SessionStore) + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(AgentRegistry) + await ctx.plugin(AgentLoop, { agents: [] }) + // Executor default cwd = serverDir (deliberately NOT the child session cwd). + await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000, cwd: serverDir }) + await ctx.plugin(HooksClaude, { configPath: join(serverDir, 'hooks.json') }) + ctx.llm.registerAdapter(['mock'], new MockAdapter([])) + + // Register a live child on its own session cwd; emit subagent/end with its id. + const { SessionId } = await import('@deepseek-ai/dsh-session') + const childHandle = ctx.agents.create({ agentId: AgentId('child-stop'), sessionId: SessionId('child-stop-session'), meta: { cwd: childDir }, agentOptions: { model: 'mock' } }) + ctx.emit('subagent/end', { provider: 'inproc', id: childHandle.agent.id, stopReason: 'completed' }) + + await waitFor(() => existsSync(marker)) + expect(existsSync(marker)).toBe(true) // the marker landed in the CHILD dir + const { readFileSync } = await import('node:fs') + const where = readFileSync(marker, 'utf8').trim() + // `pwd` may resolve symlinks (/var → /private/var etc.), so compare basenames. + expect(where.endsWith(childDir.split('/').pop()!)).toBe(true) + await childHandle.dispose() + }) }) describe('hooks-claude coverage — systemMessage is warned, not surfaced', () => {