refactor: remove redundant subagent-question-blocker hook
Replace PreToolUse hook-based question tool blocking with the existing
tools parameter approach (tools: { question: false }) which physically
removes the tool from the LLM's toolset before inference.
The hook was redundant because every session.prompt() call already passes
question: false via the tools parameter. OpenCode converts this to a
PermissionNext deny rule and deletes the tool from the toolset, preventing
the LLM from even seeing it. The hook only fired after the LLM already
called the tool, wasting tokens.
Changes:
- Remove subagent-question-blocker hook invocation from PreToolUse chain
- Remove hook registration from create-session-hooks.ts
- Delete src/hooks/subagent-question-blocker/ directory (dead code)
- Remove hook from HookNameSchema and barrel export
- Fix sync-executor.ts missing question: false in tools parameter
- Add regression tests for both the removal and the tools parameter
This commit is contained in:
@@ -69,7 +69,6 @@
|
||||
"directory-readme-injector",
|
||||
"empty-task-response-detector",
|
||||
"think-mode",
|
||||
"subagent-question-blocker",
|
||||
"anthropic-context-window-limit-recovery",
|
||||
"preemptive-compaction",
|
||||
"rules-injector",
|
||||
|
||||
@@ -13,7 +13,6 @@ export const HookNameSchema = z.enum([
|
||||
"directory-readme-injector",
|
||||
"empty-task-response-detector",
|
||||
"think-mode",
|
||||
"subagent-question-blocker",
|
||||
"anthropic-context-window-limit-recovery",
|
||||
"preemptive-compaction",
|
||||
"rules-injector",
|
||||
|
||||
@@ -36,7 +36,6 @@ export { createStartWorkHook } from "./start-work";
|
||||
export { createAtlasHook } from "./atlas";
|
||||
export { createDelegateTaskRetryHook } from "./delegate-task-retry";
|
||||
export { createQuestionLabelTruncatorHook } from "./question-label-truncator";
|
||||
export { createSubagentQuestionBlockerHook } from "./subagent-question-blocker";
|
||||
export { createStopContinuationGuardHook, type StopContinuationGuard } from "./stop-continuation-guard";
|
||||
export { createCompactionContextInjector } from "./compaction-context-injector";
|
||||
export { createCompactionTodoPreserverHook } from "./compaction-todo-preserver";
|
||||
|
||||
@@ -1,29 +0,0 @@
|
||||
import type { Hooks } from "@opencode-ai/plugin"
|
||||
import { subagentSessions } from "../../features/claude-code-session-state"
|
||||
import { log } from "../../shared"
|
||||
|
||||
export function createSubagentQuestionBlockerHook(): Hooks {
|
||||
return {
|
||||
"tool.execute.before": async (input) => {
|
||||
const toolName = input.tool?.toLowerCase()
|
||||
if (toolName !== "question" && toolName !== "askuserquestion") {
|
||||
return
|
||||
}
|
||||
|
||||
if (!subagentSessions.has(input.sessionID)) {
|
||||
return
|
||||
}
|
||||
|
||||
log("[subagent-question-blocker] Blocking question tool call from subagent session", {
|
||||
sessionID: input.sessionID,
|
||||
tool: input.tool,
|
||||
})
|
||||
|
||||
throw new Error(
|
||||
"Question tool is disabled for subagent sessions. " +
|
||||
"Subagents should complete their work autonomously without asking questions to users. " +
|
||||
"If you need clarification, return to the parent agent with your findings and uncertainties."
|
||||
)
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -1,82 +0,0 @@
|
||||
import { describe, test, expect, beforeEach } from "bun:test"
|
||||
import { createSubagentQuestionBlockerHook } from "./index"
|
||||
import { subagentSessions, _resetForTesting } from "../../features/claude-code-session-state"
|
||||
|
||||
describe("createSubagentQuestionBlockerHook", () => {
|
||||
const hook = createSubagentQuestionBlockerHook()
|
||||
|
||||
beforeEach(() => {
|
||||
_resetForTesting()
|
||||
})
|
||||
|
||||
describe("tool.execute.before", () => {
|
||||
test("allows question tool for non-subagent sessions", async () => {
|
||||
// given
|
||||
const sessionID = "ses_main"
|
||||
const input = { tool: "question", sessionID, callID: "call_1" }
|
||||
const output = { args: { questions: [] } }
|
||||
|
||||
// when
|
||||
const result = hook["tool.execute.before"]?.(input as any, output as any)
|
||||
|
||||
// then
|
||||
await expect(result).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
test("blocks question tool for subagent sessions", async () => {
|
||||
// given
|
||||
const sessionID = "ses_subagent"
|
||||
subagentSessions.add(sessionID)
|
||||
const input = { tool: "question", sessionID, callID: "call_1" }
|
||||
const output = { args: { questions: [] } }
|
||||
|
||||
// when
|
||||
const result = hook["tool.execute.before"]?.(input as any, output as any)
|
||||
|
||||
// then
|
||||
await expect(result).rejects.toThrow("Question tool is disabled for subagent sessions")
|
||||
})
|
||||
|
||||
test("blocks Question tool (case insensitive) for subagent sessions", async () => {
|
||||
// given
|
||||
const sessionID = "ses_subagent"
|
||||
subagentSessions.add(sessionID)
|
||||
const input = { tool: "Question", sessionID, callID: "call_1" }
|
||||
const output = { args: { questions: [] } }
|
||||
|
||||
// when
|
||||
const result = hook["tool.execute.before"]?.(input as any, output as any)
|
||||
|
||||
// then
|
||||
await expect(result).rejects.toThrow("Question tool is disabled for subagent sessions")
|
||||
})
|
||||
|
||||
test("blocks AskUserQuestion tool for subagent sessions", async () => {
|
||||
// given
|
||||
const sessionID = "ses_subagent"
|
||||
subagentSessions.add(sessionID)
|
||||
const input = { tool: "AskUserQuestion", sessionID, callID: "call_1" }
|
||||
const output = { args: { questions: [] } }
|
||||
|
||||
// when
|
||||
const result = hook["tool.execute.before"]?.(input as any, output as any)
|
||||
|
||||
// then
|
||||
await expect(result).rejects.toThrow("Question tool is disabled for subagent sessions")
|
||||
})
|
||||
|
||||
test("ignores non-question tools for subagent sessions", async () => {
|
||||
// given
|
||||
const sessionID = "ses_subagent"
|
||||
subagentSessions.add(sessionID)
|
||||
const input = { tool: "bash", sessionID, callID: "call_1" }
|
||||
const output = { args: { command: "ls" } }
|
||||
|
||||
// when
|
||||
const result = hook["tool.execute.before"]?.(input as any, output as any)
|
||||
|
||||
// then
|
||||
await expect(result).resolves.toBeUndefined()
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -1 +0,0 @@
|
||||
export { createSubagentQuestionBlockerHook } from "./hook";
|
||||
@@ -19,7 +19,6 @@ import {
|
||||
createPrometheusMdOnlyHook,
|
||||
createSisyphusJuniorNotepadHook,
|
||||
createQuestionLabelTruncatorHook,
|
||||
createSubagentQuestionBlockerHook,
|
||||
createPreemptiveCompactionHook,
|
||||
} from "../../hooks"
|
||||
import { createAnthropicEffortHook } from "../../hooks/anthropic-effort"
|
||||
@@ -49,7 +48,6 @@ export type SessionHooks = {
|
||||
prometheusMdOnly: ReturnType<typeof createPrometheusMdOnlyHook> | null
|
||||
sisyphusJuniorNotepad: ReturnType<typeof createSisyphusJuniorNotepadHook> | null
|
||||
questionLabelTruncator: ReturnType<typeof createQuestionLabelTruncatorHook>
|
||||
subagentQuestionBlocker: ReturnType<typeof createSubagentQuestionBlockerHook>
|
||||
taskResumeInfo: ReturnType<typeof createTaskResumeInfoHook>
|
||||
anthropicEffort: ReturnType<typeof createAnthropicEffortHook> | null
|
||||
}
|
||||
@@ -149,7 +147,6 @@ export function createSessionHooks(args: {
|
||||
: null
|
||||
|
||||
const questionLabelTruncator = createQuestionLabelTruncatorHook()
|
||||
const subagentQuestionBlocker = createSubagentQuestionBlockerHook()
|
||||
const taskResumeInfo = createTaskResumeInfoHook()
|
||||
|
||||
const anthropicEffort = isHookEnabled("anthropic-effort")
|
||||
@@ -174,7 +171,6 @@ export function createSessionHooks(args: {
|
||||
prometheusMdOnly,
|
||||
sisyphusJuniorNotepad,
|
||||
questionLabelTruncator,
|
||||
subagentQuestionBlocker,
|
||||
taskResumeInfo,
|
||||
anthropicEffort,
|
||||
}
|
||||
|
||||
@@ -1,10 +1,38 @@
|
||||
import { describe, expect, test } from "bun:test"
|
||||
import { createToolExecuteBeforeHandler } from "./tool-execute-before"
|
||||
import type { CreatedHooks } from "../create-hooks"
|
||||
const { describe, expect, test } = require("bun:test")
|
||||
const { createToolExecuteBeforeHandler } = require("./tool-execute-before")
|
||||
|
||||
describe("createToolExecuteBeforeHandler", () => {
|
||||
test("does not execute subagent question blocker hook for question tool", async () => {
|
||||
//#given
|
||||
const ctx = {
|
||||
client: {
|
||||
session: {
|
||||
messages: async () => ({ data: [] }),
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
const hooks = {
|
||||
subagentQuestionBlocker: {
|
||||
"tool.execute.before": async () => {
|
||||
throw new Error("subagentQuestionBlocker should not run")
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
const handler = createToolExecuteBeforeHandler({ ctx, hooks })
|
||||
const input = { tool: "question", sessionID: "ses_sub", callID: "call_1" }
|
||||
const output = { args: { questions: [] } as Record<string, unknown> }
|
||||
|
||||
//#when
|
||||
const run = handler(input, output)
|
||||
|
||||
//#then
|
||||
await expect(run).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
describe("task tool subagent_type normalization", () => {
|
||||
const emptyHooks = {} as CreatedHooks
|
||||
const emptyHooks = {}
|
||||
|
||||
function createCtxWithSessionMessages(messages: Array<{ info?: { agent?: string; role?: string } }> = []) {
|
||||
return {
|
||||
@@ -13,7 +41,7 @@ describe("createToolExecuteBeforeHandler", () => {
|
||||
messages: async () => ({ data: messages }),
|
||||
},
|
||||
},
|
||||
} as unknown as Parameters<typeof createToolExecuteBeforeHandler>[0]["ctx"]
|
||||
}
|
||||
}
|
||||
|
||||
test("sets subagent_type to sisyphus-junior when category is provided without subagent_type", async () => {
|
||||
@@ -136,3 +164,5 @@ describe("createToolExecuteBeforeHandler", () => {
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
export {}
|
||||
|
||||
@@ -17,7 +17,6 @@ export function createToolExecuteBeforeHandler(args: {
|
||||
const { ctx, hooks } = args
|
||||
|
||||
return async (input, output): Promise<void> => {
|
||||
await hooks.subagentQuestionBlocker?.["tool.execute.before"]?.(input, output)
|
||||
await hooks.writeExistingFileGuard?.["tool.execute.before"]?.(input, output)
|
||||
await hooks.questionLabelTruncator?.["tool.execute.before"]?.(input, output)
|
||||
await hooks.claudeCodeHooks?.["tool.execute.before"]?.(input, output)
|
||||
|
||||
91
src/tools/call-omo-agent/sync-executor.test.ts
Normal file
91
src/tools/call-omo-agent/sync-executor.test.ts
Normal file
@@ -0,0 +1,91 @@
|
||||
const { describe, test, expect, mock } = require("bun:test")
|
||||
|
||||
mock.module("./session-creator", () => ({
|
||||
createOrGetSession: mock(async () => ({ sessionID: "ses-test-123" })),
|
||||
}))
|
||||
|
||||
mock.module("./completion-poller", () => ({
|
||||
waitForCompletion: mock(async () => {}),
|
||||
}))
|
||||
|
||||
mock.module("./message-processor", () => ({
|
||||
processMessages: mock(async () => "agent response"),
|
||||
}))
|
||||
|
||||
describe("executeSync", () => {
|
||||
test("passes question=false via tools parameter to block question tool", async () => {
|
||||
//#given
|
||||
const { executeSync } = require("./sync-executor")
|
||||
|
||||
let promptArgs: any
|
||||
const promptAsync = mock(async (input: any) => {
|
||||
promptArgs = input
|
||||
return { data: {} }
|
||||
})
|
||||
|
||||
const args = {
|
||||
subagent_type: "explore",
|
||||
description: "test task",
|
||||
prompt: "find something",
|
||||
}
|
||||
|
||||
const toolContext = {
|
||||
sessionID: "parent-session",
|
||||
messageID: "msg-1",
|
||||
agent: "sisyphus",
|
||||
abort: new AbortController().signal,
|
||||
metadata: mock(async () => {}),
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
client: {
|
||||
session: { promptAsync },
|
||||
},
|
||||
}
|
||||
|
||||
//#when
|
||||
await executeSync(args, toolContext, ctx as any)
|
||||
|
||||
//#then
|
||||
expect(promptAsync).toHaveBeenCalled()
|
||||
expect(promptArgs.body.tools.question).toBe(false)
|
||||
})
|
||||
|
||||
test("passes task=false via tools parameter", async () => {
|
||||
//#given
|
||||
const { executeSync } = require("./sync-executor")
|
||||
|
||||
let promptArgs: any
|
||||
const promptAsync = mock(async (input: any) => {
|
||||
promptArgs = input
|
||||
return { data: {} }
|
||||
})
|
||||
|
||||
const args = {
|
||||
subagent_type: "librarian",
|
||||
description: "search docs",
|
||||
prompt: "find docs",
|
||||
}
|
||||
|
||||
const toolContext = {
|
||||
sessionID: "parent-session",
|
||||
messageID: "msg-2",
|
||||
agent: "sisyphus",
|
||||
abort: new AbortController().signal,
|
||||
metadata: mock(async () => {}),
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
client: {
|
||||
session: { promptAsync },
|
||||
},
|
||||
}
|
||||
|
||||
//#when
|
||||
await executeSync(args, toolContext, ctx as any)
|
||||
|
||||
//#then
|
||||
expect(promptAsync).toHaveBeenCalled()
|
||||
expect(promptArgs.body.tools.task).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -35,6 +35,7 @@ export async function executeSync(
|
||||
tools: {
|
||||
...getAgentToolRestrictions(args.subagent_type),
|
||||
task: false,
|
||||
question: false,
|
||||
},
|
||||
parts: [{ type: "text", text: args.prompt }],
|
||||
},
|
||||
|
||||
@@ -1,6 +1,45 @@
|
||||
const { describe, test, expect, mock } = require("bun:test")
|
||||
|
||||
describe("sendSyncPrompt", () => {
|
||||
test("passes question=false via tools parameter", async () => {
|
||||
//#given
|
||||
const { sendSyncPrompt } = require("./sync-prompt-sender")
|
||||
|
||||
let promptArgs: any
|
||||
const promptAsync = mock(async (input: any) => {
|
||||
promptArgs = input
|
||||
return { data: {} }
|
||||
})
|
||||
|
||||
const mockClient = {
|
||||
session: {
|
||||
promptAsync,
|
||||
},
|
||||
}
|
||||
|
||||
const input = {
|
||||
sessionID: "test-session",
|
||||
agentToUse: "sisyphus-junior",
|
||||
args: {
|
||||
description: "test task",
|
||||
prompt: "test prompt",
|
||||
run_in_background: false,
|
||||
load_skills: [],
|
||||
},
|
||||
systemContent: undefined,
|
||||
categoryModel: undefined,
|
||||
toastManager: null,
|
||||
taskId: undefined,
|
||||
}
|
||||
|
||||
//#when
|
||||
await sendSyncPrompt(mockClient as any, input)
|
||||
|
||||
//#then
|
||||
expect(promptAsync).toHaveBeenCalled()
|
||||
expect(promptArgs.body.tools.question).toBe(false)
|
||||
})
|
||||
|
||||
test("applies agent tool restrictions for explore agent", async () => {
|
||||
//#given
|
||||
const { sendSyncPrompt } = require("./sync-prompt-sender")
|
||||
|
||||
Reference in New Issue
Block a user