fix(hooks): harden cli transcript loading (#70786)

This commit is contained in:
Vincent Koc
2026-04-23 14:25:27 -07:00
committed by GitHub
parent bceda6089a
commit 51f9f94cc3
12 changed files with 338 additions and 49 deletions

View File

@@ -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", () => {

View File

@@ -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<EmbeddedPiRunResult> {
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 };
};

View File

@@ -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 });
}
});
});

View File

@@ -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 [];
}

View File

@@ -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. */

View File

@@ -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;

View File

@@ -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,
},

View File

@@ -138,6 +138,19 @@ const promptInjectionHookNameSet = new Set<PluginHookName>(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<PluginHookName>(CONVERSATION_HOOK_NAMES);
export const isConversationHookName = (hookName: PluginHookName): boolean =>
conversationHookNameSet.has(hookName);
export type PluginHookAgentContext = {
runId?: string;
agentId?: string;

View File

@@ -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({

View File

@@ -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,

View File

@@ -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,

View File

@@ -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,