fix(hooks-claude): build subagent payloads from base(), run SubagentStop in the child cwd, drop agentType
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.
This commit is contained in:
@@ -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`
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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<string, unknown> {
|
||||
return { ...base(agent, 'Stop'), stop_hook_active: false }
|
||||
}
|
||||
function subagentStartPayload(info: { id: string; agentType?: string }): Record<string, unknown> {
|
||||
return { hook_event_name: 'SubagentStart', agent_id: info.id, ...info.agentType !== undefined ? { agent_type: info.agentType } : {} }
|
||||
}
|
||||
function subagentStopPayload(info: { id: string; agentType?: string }): Record<string, unknown> {
|
||||
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<string, unknown> {
|
||||
return {
|
||||
...base(child, event),
|
||||
agent_id: info.id,
|
||||
agent_type: SUBAGENT_TYPE,
|
||||
...event === 'SubagentStop' ? { stop_hook_active: false } : {},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<typeof ctx.agents.register>[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', () => {
|
||||
|
||||
Reference in New Issue
Block a user