From b58f3edf6d5623af5a35ceb5a8af27c242020dea Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Fri, 13 Feb 2026 14:55:46 +0900 Subject: [PATCH] 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 --- assets/oh-my-opencode.schema.json | 1 - src/config/schema/hooks.ts | 1 - src/hooks/index.ts | 1 - src/hooks/subagent-question-blocker/hook.ts | 29 ------ .../subagent-question-blocker/index.test.ts | 82 ----------------- src/hooks/subagent-question-blocker/index.ts | 1 - src/plugin/hooks/create-session-hooks.ts | 4 - src/plugin/tool-execute-before.test.ts | 40 +++++++- src/plugin/tool-execute-before.ts | 1 - .../call-omo-agent/sync-executor.test.ts | 91 +++++++++++++++++++ src/tools/call-omo-agent/sync-executor.ts | 1 + .../delegate-task/sync-prompt-sender.test.ts | 39 ++++++++ 12 files changed, 166 insertions(+), 125 deletions(-) delete mode 100644 src/hooks/subagent-question-blocker/hook.ts delete mode 100644 src/hooks/subagent-question-blocker/index.test.ts delete mode 100644 src/hooks/subagent-question-blocker/index.ts create mode 100644 src/tools/call-omo-agent/sync-executor.test.ts diff --git a/assets/oh-my-opencode.schema.json b/assets/oh-my-opencode.schema.json index 6cb00bd7..f2eb3fae 100644 --- a/assets/oh-my-opencode.schema.json +++ b/assets/oh-my-opencode.schema.json @@ -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", diff --git a/src/config/schema/hooks.ts b/src/config/schema/hooks.ts index bb5f6bdb..add67188 100644 --- a/src/config/schema/hooks.ts +++ b/src/config/schema/hooks.ts @@ -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", diff --git a/src/hooks/index.ts b/src/hooks/index.ts index 95447527..fcaf1dad 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -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"; diff --git a/src/hooks/subagent-question-blocker/hook.ts b/src/hooks/subagent-question-blocker/hook.ts deleted file mode 100644 index fc64fae7..00000000 --- a/src/hooks/subagent-question-blocker/hook.ts +++ /dev/null @@ -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." - ) - }, - } -} diff --git a/src/hooks/subagent-question-blocker/index.test.ts b/src/hooks/subagent-question-blocker/index.test.ts deleted file mode 100644 index ea75d3cd..00000000 --- a/src/hooks/subagent-question-blocker/index.test.ts +++ /dev/null @@ -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() - }) - }) -}) diff --git a/src/hooks/subagent-question-blocker/index.ts b/src/hooks/subagent-question-blocker/index.ts deleted file mode 100644 index dbba20b8..00000000 --- a/src/hooks/subagent-question-blocker/index.ts +++ /dev/null @@ -1 +0,0 @@ -export { createSubagentQuestionBlockerHook } from "./hook"; diff --git a/src/plugin/hooks/create-session-hooks.ts b/src/plugin/hooks/create-session-hooks.ts index 28a0ecc3..82a4379f 100644 --- a/src/plugin/hooks/create-session-hooks.ts +++ b/src/plugin/hooks/create-session-hooks.ts @@ -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 | null sisyphusJuniorNotepad: ReturnType | null questionLabelTruncator: ReturnType - subagentQuestionBlocker: ReturnType taskResumeInfo: ReturnType anthropicEffort: ReturnType | 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, } diff --git a/src/plugin/tool-execute-before.test.ts b/src/plugin/tool-execute-before.test.ts index 1b5b8078..95f28de1 100644 --- a/src/plugin/tool-execute-before.test.ts +++ b/src/plugin/tool-execute-before.test.ts @@ -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 } + + //#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[0]["ctx"] + } } test("sets subagent_type to sisyphus-junior when category is provided without subagent_type", async () => { @@ -136,3 +164,5 @@ describe("createToolExecuteBeforeHandler", () => { }) }) }) + +export {} diff --git a/src/plugin/tool-execute-before.ts b/src/plugin/tool-execute-before.ts index 9d07e5ef..0a7bd38e 100644 --- a/src/plugin/tool-execute-before.ts +++ b/src/plugin/tool-execute-before.ts @@ -17,7 +17,6 @@ export function createToolExecuteBeforeHandler(args: { const { ctx, hooks } = args return async (input, output): Promise => { - 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) diff --git a/src/tools/call-omo-agent/sync-executor.test.ts b/src/tools/call-omo-agent/sync-executor.test.ts new file mode 100644 index 00000000..744f08ff --- /dev/null +++ b/src/tools/call-omo-agent/sync-executor.test.ts @@ -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) + }) +}) diff --git a/src/tools/call-omo-agent/sync-executor.ts b/src/tools/call-omo-agent/sync-executor.ts index f64310fb..9bd0f2de 100644 --- a/src/tools/call-omo-agent/sync-executor.ts +++ b/src/tools/call-omo-agent/sync-executor.ts @@ -35,6 +35,7 @@ export async function executeSync( tools: { ...getAgentToolRestrictions(args.subagent_type), task: false, + question: false, }, parts: [{ type: "text", text: args.prompt }], }, diff --git a/src/tools/delegate-task/sync-prompt-sender.test.ts b/src/tools/delegate-task/sync-prompt-sender.test.ts index 7b26ae23..365d796d 100644 --- a/src/tools/delegate-task/sync-prompt-sender.test.ts +++ b/src/tools/delegate-task/sync-prompt-sender.test.ts @@ -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")