From 51f9f94cc3617d64426b2a68534ab93d20951d04 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 23 Apr 2026 14:25:27 -0700 Subject: [PATCH] fix(hooks): harden cli transcript loading (#70786) --- src/agents/cli-runner.reliability.test.ts | 64 +++++++++++++++++ src/agents/cli-runner.ts | 59 ++++++++++------ src/agents/cli-runner/session-history.test.ts | 68 +++++++++++++++++++ src/agents/cli-runner/session-history.ts | 67 +++++++++++------- src/config/types.plugins.ts | 5 ++ src/plugins/config-normalization-shared.ts | 14 +++- src/plugins/config-state.test.ts | 5 +- src/plugins/hook-types.ts | 13 ++++ src/plugins/loader.test.ts | 61 +++++++++++++++++ src/plugins/registry.ts | 25 +++++++ src/plugins/status.test.ts | 4 +- src/plugins/status.ts | 2 + 12 files changed, 338 insertions(+), 49 deletions(-) diff --git a/src/agents/cli-runner.reliability.test.ts b/src/agents/cli-runner.reliability.test.ts index 0e5bea8a454..56d3fd725d2 100644 --- a/src/agents/cli-runner.reliability.test.ts +++ b/src/agents/cli-runner.reliability.test.ts @@ -18,6 +18,7 @@ import { } from "./cli-runner.test-support.js"; import { executePreparedCliRun } from "./cli-runner/execute.js"; import { resolveCliNoOutputTimeoutMs } from "./cli-runner/helpers.js"; +import * as sessionHistoryModule from "./cli-runner/session-history.js"; import { MAX_CLI_SESSION_HISTORY_MESSAGES } from "./cli-runner/session-history.js"; import type { PreparedCliRunContext } from "./cli-runner/types.js"; @@ -510,6 +511,34 @@ describe("runCliAgent reliability", () => { } }); + it("does not emit llm_output when the CLI run returns no assistant text", async () => { + const hookRunner = { + hasHooks: vi.fn((hookName: string) => hookName === "llm_output"), + runLlmInput: vi.fn(async () => undefined), + runLlmOutput: vi.fn(async () => undefined), + runAgentEnd: vi.fn(async () => undefined), + }; + mockGetGlobalHookRunner.mockReturnValue(hookRunner as never); + + supervisorSpawnMock.mockResolvedValueOnce( + createManagedRun({ + reason: "exit", + exitCode: 0, + exitSignal: null, + durationMs: 50, + stdout: " ", + stderr: "", + timedOut: false, + noOutputTimedOut: false, + }), + ); + + const result = await runPreparedCliAgent(buildPreparedContext()); + + expect(result.payloads).toBeUndefined(); + expect(hookRunner.runLlmOutput).not.toHaveBeenCalled(); + }); + it("emits agent_end with failure details when the CLI run fails", async () => { const hookRunner = { hasHooks: vi.fn((hookName: string) => ["llm_input", "agent_end"].includes(hookName)), @@ -634,6 +663,41 @@ describe("runCliAgent reliability", () => { fs.rmSync(dir, { recursive: true, force: true }); } }); + + it("skips transcript loading when only llm_output hooks are active", async () => { + const hookRunner = { + hasHooks: vi.fn((hookName: string) => hookName === "llm_output"), + runLlmInput: vi.fn(async () => undefined), + runLlmOutput: vi.fn(async () => undefined), + runAgentEnd: vi.fn(async () => undefined), + }; + mockGetGlobalHookRunner.mockReturnValue(hookRunner as never); + const historySpy = vi.spyOn(sessionHistoryModule, "loadCliSessionHistoryMessages"); + + supervisorSpawnMock.mockResolvedValueOnce( + createManagedRun({ + reason: "exit", + exitCode: 0, + exitSignal: null, + durationMs: 50, + stdout: "hello from cli", + stderr: "", + timedOut: false, + noOutputTimedOut: false, + }), + ); + + try { + await runPreparedCliAgent(buildPreparedContext()); + + expect(historySpy).not.toHaveBeenCalled(); + await vi.waitFor(() => { + expect(hookRunner.runLlmOutput).toHaveBeenCalledTimes(1); + }); + } finally { + historySpy.mockRestore(); + } + }); }); describe("resolveCliNoOutputTimeoutMs", () => { diff --git a/src/agents/cli-runner.ts b/src/agents/cli-runner.ts index 33771ed65c4..41d28529340 100644 --- a/src/agents/cli-runner.ts +++ b/src/agents/cli-runner.ts @@ -1,7 +1,9 @@ import { formatErrorMessage } from "../infra/errors.js"; +import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { loadCliSessionHistoryMessages } from "./cli-runner/session-history.js"; import type { PreparedCliRunContext, RunCliAgentParams } from "./cli-runner/types.js"; import { FailoverError, isFailoverError, resolveFailoverStatus } from "./failover-error.js"; +import { buildAgentHookConversationMessages } from "./harness/hook-history.js"; import { runAgentHarnessAgentEndHook, runAgentHarnessLlmInputHook, @@ -53,13 +55,20 @@ export async function runPreparedCliAgent( ): Promise { const { executePreparedCliRun } = await import("./cli-runner/execute.runtime.js"); const { params } = context; - const historyMessages = loadCliSessionHistoryMessages({ - sessionId: params.sessionId, - sessionFile: params.sessionFile, - sessionKey: params.sessionKey, - agentId: params.agentId, - config: params.config, - }); + const hookRunner = getGlobalHookRunner(); + const hasLlmInputHooks = hookRunner?.hasHooks("llm_input") === true; + const hasLlmOutputHooks = hookRunner?.hasHooks("llm_output") === true; + const hasAgentEndHooks = hookRunner?.hasHooks("agent_end") === true; + const historyMessages = + hasLlmInputHooks || hasAgentEndHooks + ? loadCliSessionHistoryMessages({ + sessionId: params.sessionId, + sessionFile: params.sessionFile, + sessionKey: params.sessionKey, + agentId: params.agentId, + config: params.config, + }) + : []; const llmInputEvent = { runId: params.runId, sessionId: params.sessionId, @@ -82,9 +91,13 @@ export async function runPreparedCliAgent( } as const; const buildAgentEndMessages = (lastAssistant?: unknown): unknown[] => [ - ...historyMessages, - buildCliHookUserMessage(params.prompt), - ...(lastAssistant ? [lastAssistant] : []), + ...buildAgentHookConversationMessages({ + historyMessages, + currentTurnMessages: [ + buildCliHookUserMessage(params.prompt), + ...(lastAssistant ? [lastAssistant] : []), + ], + }), ]; const buildFailedAgentEndEvent = (error: string) => ({ @@ -125,18 +138,20 @@ export async function runPreparedCliAgent( usage: output.usage, }) : undefined; - runAgentHarnessLlmOutputHook({ - event: { - runId: params.runId, - sessionId: params.sessionId, - provider: params.provider, - model: context.modelId, - assistantTexts, - ...(lastAssistant ? { lastAssistant } : {}), - ...(output.usage ? { usage: output.usage } : {}), - }, - ctx: hookContext, - }); + if (assistantText.length > 0 && hasLlmOutputHooks) { + runAgentHarnessLlmOutputHook({ + event: { + runId: params.runId, + sessionId: params.sessionId, + provider: params.provider, + model: context.modelId, + assistantTexts, + ...(lastAssistant ? { lastAssistant } : {}), + ...(output.usage ? { usage: output.usage } : {}), + }, + ctx: hookContext, + }); + } return { output, assistantText, lastAssistant }; }; diff --git a/src/agents/cli-runner/session-history.test.ts b/src/agents/cli-runner/session-history.test.ts index f1beb4d7db7..b26949730c4 100644 --- a/src/agents/cli-runner/session-history.test.ts +++ b/src/agents/cli-runner/session-history.test.ts @@ -123,6 +123,41 @@ describe("loadCliSessionHistoryMessages", () => { } }); + it("rejects symlinked transcripts instead of following them outside the sessions directory", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-")); + const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-outside-")); + vi.stubEnv("OPENCLAW_STATE_DIR", stateDir); + const canonicalSessionFile = path.join( + stateDir, + "agents", + "main", + "sessions", + "session-symlink.jsonl", + ); + const outsideFile = createSessionTranscript({ + rootDir: outsideDir, + sessionId: "session-symlink", + filePath: path.join(outsideDir, "outside.jsonl"), + messages: ["stolen history"], + }); + fs.mkdirSync(path.dirname(canonicalSessionFile), { recursive: true }); + fs.symlinkSync(outsideFile, canonicalSessionFile); + + try { + expect( + loadCliSessionHistoryMessages({ + sessionId: "session-symlink", + sessionFile: canonicalSessionFile, + sessionKey: "agent:main:main", + agentId: "main", + }), + ).toEqual([]); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + }); + it("drops oversized transcript files instead of loading them into hook payloads", () => { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-")); vi.stubEnv("OPENCLAW_STATE_DIR", stateDir); @@ -149,4 +184,37 @@ describe("loadCliSessionHistoryMessages", () => { fs.rmSync(stateDir, { recursive: true, force: true }); } }); + + it("honors custom session store roots when resolving hook history transcripts", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-")); + const customStoreDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-store-")); + vi.stubEnv("OPENCLAW_STATE_DIR", stateDir); + const storePath = path.join(customStoreDir, "sessions.json"); + fs.writeFileSync(storePath, "{}", "utf-8"); + const sessionFile = createSessionTranscript({ + rootDir: customStoreDir, + sessionId: "session-custom-store", + filePath: path.join(customStoreDir, "session-custom-store.jsonl"), + messages: ["custom store history"], + }); + + try { + expect( + loadCliSessionHistoryMessages({ + sessionId: "session-custom-store", + sessionFile, + sessionKey: "agent:main:main", + agentId: "main", + config: { + session: { + store: storePath, + }, + }, + }), + ).toMatchObject([{ role: "user", content: "custom store history" }]); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + fs.rmSync(customStoreDir, { recursive: true, force: true }); + } + }); }); diff --git a/src/agents/cli-runner/session-history.ts b/src/agents/cli-runner/session-history.ts index 437a2e39563..0879bb222de 100644 --- a/src/agents/cli-runner/session-history.ts +++ b/src/agents/cli-runner/session-history.ts @@ -1,4 +1,5 @@ import fs from "node:fs"; +import path from "node:path"; import { SessionManager } from "@mariozechner/pi-coding-agent"; import { resolveSessionFilePath, @@ -6,9 +7,26 @@ import { } from "../../config/sessions/paths.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { resolveSessionAgentIds } from "../agent-scope.js"; +import { + limitAgentHookHistoryMessages, + MAX_AGENT_HOOK_HISTORY_MESSAGES, +} from "../harness/hook-history.js"; export const MAX_CLI_SESSION_HISTORY_FILE_BYTES = 5 * 1024 * 1024; -export const MAX_CLI_SESSION_HISTORY_MESSAGES = 200; +export const MAX_CLI_SESSION_HISTORY_MESSAGES = MAX_AGENT_HOOK_HISTORY_MESSAGES; + +function safeRealpathSync(filePath: string): string | undefined { + try { + return fs.realpathSync(filePath); + } catch { + return undefined; + } +} + +function isPathWithinBase(basePath: string, targetPath: string): boolean { + const relative = path.relative(basePath, targetPath); + return Boolean(relative) && !relative.startsWith("..") && !path.isAbsolute(relative); +} function resolveSafeCliSessionFile(params: { sessionId: string; @@ -16,19 +34,25 @@ function resolveSafeCliSessionFile(params: { sessionKey?: string; agentId?: string; config?: OpenClawConfig; -}): string { +}): { sessionFile: string; sessionsDir: string } { const { defaultAgentId, sessionAgentId } = resolveSessionAgentIds({ sessionKey: params.sessionKey, config: params.config, agentId: params.agentId, }); - return resolveSessionFilePath( + const pathOptions = resolveSessionFilePathOptions({ + agentId: sessionAgentId ?? defaultAgentId, + storePath: params.config?.session?.store, + }); + const sessionFile = resolveSessionFilePath( params.sessionId, { sessionFile: params.sessionFile }, - resolveSessionFilePathOptions({ - agentId: sessionAgentId ?? defaultAgentId, - }), + pathOptions, ); + return { + sessionFile, + sessionsDir: pathOptions?.sessionsDir ?? path.dirname(sessionFile), + }; } export function loadCliSessionHistoryMessages(params: { @@ -39,28 +63,25 @@ export function loadCliSessionHistoryMessages(params: { config?: OpenClawConfig; }): unknown[] { try { - const sessionFile = resolveSafeCliSessionFile(params); - if (!fs.existsSync(sessionFile)) { + const { sessionFile, sessionsDir } = resolveSafeCliSessionFile(params); + const entryStat = fs.lstatSync(sessionFile); + if (!entryStat.isFile() || entryStat.isSymbolicLink()) { return []; } - const stat = fs.statSync(sessionFile); + const realSessionsDir = safeRealpathSync(sessionsDir) ?? path.resolve(sessionsDir); + const realSessionFile = safeRealpathSync(sessionFile); + if (!realSessionFile || !isPathWithinBase(realSessionsDir, realSessionFile)) { + return []; + } + const stat = fs.statSync(realSessionFile); if (!stat.isFile() || stat.size > MAX_CLI_SESSION_HISTORY_FILE_BYTES) { return []; } - const entries = SessionManager.open(sessionFile).getEntries(); - const history: unknown[] = []; - for (let index = entries.length - 1; index >= 0; index -= 1) { - const entry = entries[index]; - if (entry?.type !== "message") { - continue; - } - history.push(entry.message as unknown); - if (history.length >= MAX_CLI_SESSION_HISTORY_MESSAGES) { - break; - } - } - history.reverse(); - return history; + const entries = SessionManager.open(realSessionFile).getEntries(); + const history = entries.flatMap((entry) => + entry?.type === "message" ? [entry.message as unknown] : [], + ); + return limitAgentHookHistoryMessages(history, MAX_CLI_SESSION_HISTORY_MESSAGES); } catch { return []; } diff --git a/src/config/types.plugins.ts b/src/config/types.plugins.ts index af37ba2020f..86ecb8de78a 100644 --- a/src/config/types.plugins.ts +++ b/src/config/types.plugins.ts @@ -3,6 +3,11 @@ export type PluginEntryConfig = { hooks?: { /** Controls prompt mutation via before_prompt_build and prompt fields from legacy before_agent_start. */ allowPromptInjection?: boolean; + /** + * Controls access to raw conversation content from llm_input/llm_output/agent_end hooks. + * Non-bundled plugins must opt in explicitly; bundled plugins stay allowed unless disabled. + */ + allowConversationAccess?: boolean; }; subagent?: { /** Explicitly allow this plugin to request per-run provider/model overrides for subagent runs. */ diff --git a/src/plugins/config-normalization-shared.ts b/src/plugins/config-normalization-shared.ts index 572c3f7c981..ed61b11cd0b 100644 --- a/src/plugins/config-normalization-shared.ts +++ b/src/plugins/config-normalization-shared.ts @@ -21,6 +21,7 @@ export type NormalizedPluginsConfig = { enabled?: boolean; hooks?: { allowPromptInjection?: boolean; + allowConversationAccess?: boolean; }; subagent?: { allowModelOverride?: boolean; @@ -80,12 +81,21 @@ function normalizePluginEntries( ? { allowPromptInjection: (hooksRaw as { allowPromptInjection?: unknown }) .allowPromptInjection, + allowConversationAccess: (hooksRaw as { allowConversationAccess?: unknown }) + .allowConversationAccess, } : undefined; const normalizedHooks = - hooks && typeof hooks.allowPromptInjection === "boolean" + hooks && + (typeof hooks.allowPromptInjection === "boolean" || + typeof hooks.allowConversationAccess === "boolean") ? { - allowPromptInjection: hooks.allowPromptInjection, + ...(typeof hooks.allowPromptInjection === "boolean" + ? { allowPromptInjection: hooks.allowPromptInjection } + : {}), + ...(typeof hooks.allowConversationAccess === "boolean" + ? { allowConversationAccess: hooks.allowConversationAccess } + : {}), } : undefined; const subagentRaw = entry.subagent; diff --git a/src/plugins/config-state.test.ts b/src/plugins/config-state.test.ts index f153eac0914..5b36dcf0470 100644 --- a/src/plugins/config-state.test.ts +++ b/src/plugins/config-state.test.ts @@ -70,10 +70,12 @@ describe("normalizePluginsConfig", () => { entry: { hooks: { allowPromptInjection: false, + allowConversationAccess: true, }, }, expectedHooks: { allowPromptInjection: false, + allowConversationAccess: true, }, }, { @@ -81,7 +83,8 @@ describe("normalizePluginsConfig", () => { entry: { hooks: { allowPromptInjection: "nope", - } as unknown as { allowPromptInjection: boolean }, + allowConversationAccess: "nope", + } as unknown as { allowPromptInjection: boolean; allowConversationAccess: boolean }, }, expectedHooks: undefined, }, diff --git a/src/plugins/hook-types.ts b/src/plugins/hook-types.ts index ff5e8ae6e54..05e8b567b02 100644 --- a/src/plugins/hook-types.ts +++ b/src/plugins/hook-types.ts @@ -138,6 +138,19 @@ const promptInjectionHookNameSet = new Set(PROMPT_INJECTION_HOOK export const isPromptInjectionHookName = (hookName: PluginHookName): boolean => promptInjectionHookNameSet.has(hookName); +export const CONVERSATION_HOOK_NAMES = [ + "llm_input", + "llm_output", + "agent_end", +] as const satisfies readonly PluginHookName[]; + +export type ConversationHookName = (typeof CONVERSATION_HOOK_NAMES)[number]; + +const conversationHookNameSet = new Set(CONVERSATION_HOOK_NAMES); + +export const isConversationHookName = (hookName: PluginHookName): boolean => + conversationHookNameSet.has(hookName); + export type PluginHookAgentContext = { runId?: string; agentId?: string; diff --git a/src/plugins/loader.test.ts b/src/plugins/loader.test.ts index 5a8291f3974..4412ac9a9f0 100644 --- a/src/plugins/loader.test.ts +++ b/src/plugins/loader.test.ts @@ -5303,6 +5303,67 @@ module.exports = { ]); }); + it("blocks conversation typed hooks for non-bundled plugins unless explicitly allowed", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "conversation-hooks", + filename: "conversation-hooks.cjs", + body: `module.exports = { id: "conversation-hooks", register(api) { + api.on("llm_input", () => undefined); + api.on("llm_output", () => undefined); + api.on("agent_end", () => undefined); +} };`, + }); + + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["conversation-hooks"], + }, + }); + + expect(registry.typedHooks).toEqual([]); + const blockedDiagnostics = registry.diagnostics.filter((diag) => + diag.message.includes( + "non-bundled plugins must set plugins.entries.conversation-hooks.hooks.allowConversationAccess=true", + ), + ); + expect(blockedDiagnostics).toHaveLength(3); + }); + + it("allows conversation typed hooks for non-bundled plugins when explicitly enabled", () => { + useNoBundledPlugins(); + const plugin = writePlugin({ + id: "conversation-hooks-allowed", + filename: "conversation-hooks-allowed.cjs", + body: `module.exports = { id: "conversation-hooks-allowed", register(api) { + api.on("llm_input", () => undefined); + api.on("llm_output", () => undefined); + api.on("agent_end", () => undefined); +} };`, + }); + + const registry = loadRegistryFromSinglePlugin({ + plugin, + pluginConfig: { + allow: ["conversation-hooks-allowed"], + entries: { + "conversation-hooks-allowed": { + hooks: { + allowConversationAccess: true, + }, + }, + }, + }, + }); + + expect(registry.typedHooks.map((entry) => entry.hookName)).toEqual([ + "llm_input", + "llm_output", + "agent_end", + ]); + }); + it("ignores unknown typed hooks from plugins and keeps loading", () => { useNoBundledPlugins(); const plugin = writePlugin({ diff --git a/src/plugins/registry.ts b/src/plugins/registry.ts index 041d741a8c9..150f4ebebe2 100644 --- a/src/plugins/registry.ts +++ b/src/plugins/registry.ts @@ -83,6 +83,7 @@ import { withPluginRuntimePluginIdScope } from "./runtime/gateway-request-scope. import type { PluginRuntime } from "./runtime/types.js"; import { defaultSlotIdForKey, hasKind } from "./slots.js"; import { + isConversationHookName, isPluginHookName, isPromptInjectionHookName, stripPromptMutationFieldsFromLegacyHookResult, @@ -165,6 +166,7 @@ export type { type PluginTypedHookPolicy = { allowPromptInjection?: boolean; + allowConversationAccess?: boolean; }; const constrainLegacyPromptInjectionHook = ( @@ -1210,6 +1212,29 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) { ) as PluginHookHandlerMap[K]; } } + if (isConversationHookName(hookName)) { + const explicitConversationAccess = policy?.allowConversationAccess; + if (record.origin !== "bundled" && explicitConversationAccess !== true) { + pushDiagnostic({ + level: "warn", + pluginId: record.id, + source: record.source, + message: + `typed hook "${hookName}" blocked because non-bundled plugins must set ` + + `plugins.entries.${record.id}.hooks.allowConversationAccess=true`, + }); + return; + } + if (record.origin === "bundled" && explicitConversationAccess === false) { + pushDiagnostic({ + level: "warn", + pluginId: record.id, + source: record.source, + message: `typed hook "${hookName}" blocked by plugins.entries.${record.id}.hooks.allowConversationAccess=false`, + }); + return; + } + } record.hookCount += 1; registry.typedHooks.push({ pluginId: record.id, diff --git a/src/plugins/status.test.ts b/src/plugins/status.test.ts index 258ad6acaca..1f825fffa04 100644 --- a/src/plugins/status.test.ts +++ b/src/plugins/status.test.ts @@ -466,6 +466,7 @@ describe("plugin status reports", () => { expect(inspect).not.toBeNull(); expectInspectPolicy(inspect!, { allowPromptInjection: undefined, + allowConversationAccess: undefined, allowModelOverride: true, allowedModels: ["openai/gpt-5.5"], hasAllowedModelsConfig: true, @@ -583,7 +584,7 @@ describe("plugin status reports", () => { plugins: { entries: { google: { - hooks: { allowPromptInjection: false }, + hooks: { allowPromptInjection: false, allowConversationAccess: true }, subagent: { allowModelOverride: true, allowedModels: ["openai/gpt-5.5"], @@ -623,6 +624,7 @@ describe("plugin status reports", () => { ]); expectInspectPolicy(inspect!, { allowPromptInjection: false, + allowConversationAccess: true, allowModelOverride: true, allowedModels: ["openai/gpt-5.5"], hasAllowedModelsConfig: true, diff --git a/src/plugins/status.ts b/src/plugins/status.ts index 9af274e6662..a239c0fd0b0 100644 --- a/src/plugins/status.ts +++ b/src/plugins/status.ts @@ -82,6 +82,7 @@ export type PluginInspectReport = { diagnostics: PluginDiagnostic[]; policy: { allowPromptInjection?: boolean; + allowConversationAccess?: boolean; allowModelOverride?: boolean; allowedModels: string[]; hasAllowedModelsConfig: boolean; @@ -348,6 +349,7 @@ export function buildPluginInspectReport(params: { diagnostics, policy: { allowPromptInjection: policyEntry?.hooks?.allowPromptInjection, + allowConversationAccess: policyEntry?.hooks?.allowConversationAccess, allowModelOverride: policyEntry?.subagent?.allowModelOverride, allowedModels: [...(policyEntry?.subagent?.allowedModels ?? [])], hasAllowedModelsConfig: policyEntry?.subagent?.hasAllowedModelsConfig === true,