Skip to content

Commit 51f9f94

Browse files
vincentkocvincentkoc
authored
fix(hooks): harden cli transcript loading (#70786)
1 parent bceda60 commit 51f9f94

12 files changed

Lines changed: 338 additions & 49 deletions

‎src/agents/cli-runner.reliability.test.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
} from "./cli-runner.test-support.js";
1919
import { executePreparedCliRun } from "./cli-runner/execute.js";
2020
import { resolveCliNoOutputTimeoutMs } from "./cli-runner/helpers.js";
21+
import * as sessionHistoryModule from "./cli-runner/session-history.js";
2122
import { MAX_CLI_SESSION_HISTORY_MESSAGES } from "./cli-runner/session-history.js";
2223
import type { PreparedCliRunContext } from "./cli-runner/types.js";
2324

@@ -510,6 +511,34 @@ describe("runCliAgent reliability", () => {
510511
}
511512
});
512513

514+
it("does not emit llm_output when the CLI run returns no assistant text", async () => {
515+
const hookRunner = {
516+
hasHooks: vi.fn((hookName: string) => hookName === "llm_output"),
517+
runLlmInput: vi.fn(async () => undefined),
518+
runLlmOutput: vi.fn(async () => undefined),
519+
runAgentEnd: vi.fn(async () => undefined),
520+
};
521+
mockGetGlobalHookRunner.mockReturnValue(hookRunner as never);
522+
523+
supervisorSpawnMock.mockResolvedValueOnce(
524+
createManagedRun({
525+
reason: "exit",
526+
exitCode: 0,
527+
exitSignal: null,
528+
durationMs: 50,
529+
stdout: " ",
530+
stderr: "",
531+
timedOut: false,
532+
noOutputTimedOut: false,
533+
}),
534+
);
535+
536+
const result = await runPreparedCliAgent(buildPreparedContext());
537+
538+
expect(result.payloads).toBeUndefined();
539+
expect(hookRunner.runLlmOutput).not.toHaveBeenCalled();
540+
});
541+
513542
it("emits agent_end with failure details when the CLI run fails", async () => {
514543
const hookRunner = {
515544
hasHooks: vi.fn((hookName: string) => ["llm_input", "agent_end"].includes(hookName)),
@@ -634,6 +663,41 @@ describe("runCliAgent reliability", () => {
634663
fs.rmSync(dir, { recursive: true, force: true });
635664
}
636665
});
666+
667+
it("skips transcript loading when only llm_output hooks are active", async () => {
668+
const hookRunner = {
669+
hasHooks: vi.fn((hookName: string) => hookName === "llm_output"),
670+
runLlmInput: vi.fn(async () => undefined),
671+
runLlmOutput: vi.fn(async () => undefined),
672+
runAgentEnd: vi.fn(async () => undefined),
673+
};
674+
mockGetGlobalHookRunner.mockReturnValue(hookRunner as never);
675+
const historySpy = vi.spyOn(sessionHistoryModule, "loadCliSessionHistoryMessages");
676+
677+
supervisorSpawnMock.mockResolvedValueOnce(
678+
createManagedRun({
679+
reason: "exit",
680+
exitCode: 0,
681+
exitSignal: null,
682+
durationMs: 50,
683+
stdout: "hello from cli",
684+
stderr: "",
685+
timedOut: false,
686+
noOutputTimedOut: false,
687+
}),
688+
);
689+
690+
try {
691+
await runPreparedCliAgent(buildPreparedContext());
692+
693+
expect(historySpy).not.toHaveBeenCalled();
694+
await vi.waitFor(() => {
695+
expect(hookRunner.runLlmOutput).toHaveBeenCalledTimes(1);
696+
});
697+
} finally {
698+
historySpy.mockRestore();
699+
}
700+
});
637701
});
638702

639703
describe("resolveCliNoOutputTimeoutMs", () => {

‎src/agents/cli-runner.ts‎

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { formatErrorMessage } from "../infra/errors.js";
2+
import { getGlobalHookRunner } from "../plugins/hook-runner-global.js";
23
import { loadCliSessionHistoryMessages } from "./cli-runner/session-history.js";
34
import type { PreparedCliRunContext, RunCliAgentParams } from "./cli-runner/types.js";
45
import { FailoverError, isFailoverError, resolveFailoverStatus } from "./failover-error.js";
6+
import { buildAgentHookConversationMessages } from "./harness/hook-history.js";
57
import {
68
runAgentHarnessAgentEndHook,
79
runAgentHarnessLlmInputHook,
@@ -53,13 +55,20 @@ export async function runPreparedCliAgent(
5355
): Promise<EmbeddedPiRunResult> {
5456
const { executePreparedCliRun } = await import("./cli-runner/execute.runtime.js");
5557
const { params } = context;
56-
const historyMessages = loadCliSessionHistoryMessages({
57-
sessionId: params.sessionId,
58-
sessionFile: params.sessionFile,
59-
sessionKey: params.sessionKey,
60-
agentId: params.agentId,
61-
config: params.config,
62-
});
58+
const hookRunner = getGlobalHookRunner();
59+
const hasLlmInputHooks = hookRunner?.hasHooks("llm_input") === true;
60+
const hasLlmOutputHooks = hookRunner?.hasHooks("llm_output") === true;
61+
const hasAgentEndHooks = hookRunner?.hasHooks("agent_end") === true;
62+
const historyMessages =
63+
hasLlmInputHooks || hasAgentEndHooks
64+
? loadCliSessionHistoryMessages({
65+
sessionId: params.sessionId,
66+
sessionFile: params.sessionFile,
67+
sessionKey: params.sessionKey,
68+
agentId: params.agentId,
69+
config: params.config,
70+
})
71+
: [];
6372
const llmInputEvent = {
6473
runId: params.runId,
6574
sessionId: params.sessionId,
@@ -82,9 +91,13 @@ export async function runPreparedCliAgent(
8291
} as const;
8392

8493
const buildAgentEndMessages = (lastAssistant?: unknown): unknown[] => [
85-
...historyMessages,
86-
buildCliHookUserMessage(params.prompt),
87-
...(lastAssistant ? [lastAssistant] : []),
94+
...buildAgentHookConversationMessages({
95+
historyMessages,
96+
currentTurnMessages: [
97+
buildCliHookUserMessage(params.prompt),
98+
...(lastAssistant ? [lastAssistant] : []),
99+
],
100+
}),
88101
];
89102

90103
const buildFailedAgentEndEvent = (error: string) => ({
@@ -125,18 +138,20 @@ export async function runPreparedCliAgent(
125138
usage: output.usage,
126139
})
127140
: undefined;
128-
runAgentHarnessLlmOutputHook({
129-
event: {
130-
runId: params.runId,
131-
sessionId: params.sessionId,
132-
provider: params.provider,
133-
model: context.modelId,
134-
assistantTexts,
135-
...(lastAssistant ? { lastAssistant } : {}),
136-
...(output.usage ? { usage: output.usage } : {}),
137-
},
138-
ctx: hookContext,
139-
});
141+
if (assistantText.length > 0 && hasLlmOutputHooks) {
142+
runAgentHarnessLlmOutputHook({
143+
event: {
144+
runId: params.runId,
145+
sessionId: params.sessionId,
146+
provider: params.provider,
147+
model: context.modelId,
148+
assistantTexts,
149+
...(lastAssistant ? { lastAssistant } : {}),
150+
...(output.usage ? { usage: output.usage } : {}),
151+
},
152+
ctx: hookContext,
153+
});
154+
}
140155
return { output, assistantText, lastAssistant };
141156
};
142157

‎src/agents/cli-runner/session-history.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,41 @@ describe("loadCliSessionHistoryMessages", () => {
123123
}
124124
});
125125

126+
it("rejects symlinked transcripts instead of following them outside the sessions directory", () => {
127+
const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-"));
128+
const outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-outside-"));
129+
vi.stubEnv("OPENCLAW_STATE_DIR", stateDir);
130+
const canonicalSessionFile = path.join(
131+
stateDir,
132+
"agents",
133+
"main",
134+
"sessions",
135+
"session-symlink.jsonl",
136+
);
137+
const outsideFile = createSessionTranscript({
138+
rootDir: outsideDir,
139+
sessionId: "session-symlink",
140+
filePath: path.join(outsideDir, "outside.jsonl"),
141+
messages: ["stolen history"],
142+
});
143+
fs.mkdirSync(path.dirname(canonicalSessionFile), { recursive: true });
144+
fs.symlinkSync(outsideFile, canonicalSessionFile);
145+
146+
try {
147+
expect(
148+
loadCliSessionHistoryMessages({
149+
sessionId: "session-symlink",
150+
sessionFile: canonicalSessionFile,
151+
sessionKey: "agent:main:main",
152+
agentId: "main",
153+
}),
154+
).toEqual([]);
155+
} finally {
156+
fs.rmSync(stateDir, { recursive: true, force: true });
157+
fs.rmSync(outsideDir, { recursive: true, force: true });
158+
}
159+
});
160+
126161
it("drops oversized transcript files instead of loading them into hook payloads", () => {
127162
const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-"));
128163
vi.stubEnv("OPENCLAW_STATE_DIR", stateDir);
@@ -149,4 +184,37 @@ describe("loadCliSessionHistoryMessages", () => {
149184
fs.rmSync(stateDir, { recursive: true, force: true });
150185
}
151186
});
187+
188+
it("honors custom session store roots when resolving hook history transcripts", () => {
189+
const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-state-"));
190+
const customStoreDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-cli-store-"));
191+
vi.stubEnv("OPENCLAW_STATE_DIR", stateDir);
192+
const storePath = path.join(customStoreDir, "sessions.json");
193+
fs.writeFileSync(storePath, "{}", "utf-8");
194+
const sessionFile = createSessionTranscript({
195+
rootDir: customStoreDir,
196+
sessionId: "session-custom-store",
197+
filePath: path.join(customStoreDir, "session-custom-store.jsonl"),
198+
messages: ["custom store history"],
199+
});
200+
201+
try {
202+
expect(
203+
loadCliSessionHistoryMessages({
204+
sessionId: "session-custom-store",
205+
sessionFile,
206+
sessionKey: "agent:main:main",
207+
agentId: "main",
208+
config: {
209+
session: {
210+
store: storePath,
211+
},
212+
},
213+
}),
214+
).toMatchObject([{ role: "user", content: "custom store history" }]);
215+
} finally {
216+
fs.rmSync(stateDir, { recursive: true, force: true });
217+
fs.rmSync(customStoreDir, { recursive: true, force: true });
218+
}
219+
});
152220
});

‎src/agents/cli-runner/session-history.ts‎

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,58 @@
11
import fs from "node:fs";
2+
import path from "node:path";
23
import { SessionManager } from "@mariozechner/pi-coding-agent";
34
import {
45
resolveSessionFilePath,
56
resolveSessionFilePathOptions,
67
} from "../../config/sessions/paths.js";
78
import type { OpenClawConfig } from "../../config/types.openclaw.js";
89
import { resolveSessionAgentIds } from "../agent-scope.js";
10+
import {
11+
limitAgentHookHistoryMessages,
12+
MAX_AGENT_HOOK_HISTORY_MESSAGES,
13+
} from "../harness/hook-history.js";
914

1015
export const MAX_CLI_SESSION_HISTORY_FILE_BYTES = 5 * 1024 * 1024;
11-
export const MAX_CLI_SESSION_HISTORY_MESSAGES = 200;
16+
export const MAX_CLI_SESSION_HISTORY_MESSAGES = MAX_AGENT_HOOK_HISTORY_MESSAGES;
17+
18+
function safeRealpathSync(filePath: string): string | undefined {
19+
try {
20+
return fs.realpathSync(filePath);
21+
} catch {
22+
return undefined;
23+
}
24+
}
25+
26+
function isPathWithinBase(basePath: string, targetPath: string): boolean {
27+
const relative = path.relative(basePath, targetPath);
28+
return Boolean(relative) && !relative.startsWith("..") && !path.isAbsolute(relative);
29+
}
1230

1331
function resolveSafeCliSessionFile(params: {
1432
sessionId: string;
1533
sessionFile: string;
1634
sessionKey?: string;
1735
agentId?: string;
1836
config?: OpenClawConfig;
19-
}): string {
37+
}): { sessionFile: string; sessionsDir: string } {
2038
const { defaultAgentId, sessionAgentId } = resolveSessionAgentIds({
2139
sessionKey: params.sessionKey,
2240
config: params.config,
2341
agentId: params.agentId,
2442
});
25-
return resolveSessionFilePath(
43+
const pathOptions = resolveSessionFilePathOptions({
44+
agentId: sessionAgentId ?? defaultAgentId,
45+
storePath: params.config?.session?.store,
46+
});
47+
const sessionFile = resolveSessionFilePath(
2648
params.sessionId,
2749
{ sessionFile: params.sessionFile },
28-
resolveSessionFilePathOptions({
29-
agentId: sessionAgentId ?? defaultAgentId,
30-
}),
50+
pathOptions,
3151
);
52+
return {
53+
sessionFile,
54+
sessionsDir: pathOptions?.sessionsDir ?? path.dirname(sessionFile),
55+
};
3256
}
3357

3458
export function loadCliSessionHistoryMessages(params: {
@@ -39,28 +63,25 @@ export function loadCliSessionHistoryMessages(params: {
3963
config?: OpenClawConfig;
4064
}): unknown[] {
4165
try {
42-
const sessionFile = resolveSafeCliSessionFile(params);
43-
if (!fs.existsSync(sessionFile)) {
66+
const { sessionFile, sessionsDir } = resolveSafeCliSessionFile(params);
67+
const entryStat = fs.lstatSync(sessionFile);
68+
if (!entryStat.isFile() || entryStat.isSymbolicLink()) {
4469
return [];
4570
}
46-
const stat = fs.statSync(sessionFile);
47-
if (!stat.isFile() || stat.size > MAX_CLI_SESSION_HISTORY_FILE_BYTES) {
71+
const realSessionsDir = safeRealpathSync(sessionsDir) ?? path.resolve(sessionsDir);
72+
const realSessionFile = safeRealpathSync(sessionFile);
73+
if (!realSessionFile || !isPathWithinBase(realSessionsDir, realSessionFile)) {
4874
return [];
4975
}
50-
const entries = SessionManager.open(sessionFile).getEntries();
51-
const history: unknown[] = [];
52-
for (let index = entries.length - 1; index >= 0; index -= 1) {
53-
const entry = entries[index];
54-
if (entry?.type !== "message") {
55-
continue;
56-
}
57-
history.push(entry.message as unknown);
58-
if (history.length >= MAX_CLI_SESSION_HISTORY_MESSAGES) {
59-
break;
60-
}
76+
const stat = fs.statSync(realSessionFile);
77+
if (!stat.isFile() || stat.size > MAX_CLI_SESSION_HISTORY_FILE_BYTES) {
78+
return [];
6179
}
62-
history.reverse();
63-
return history;
80+
const entries = SessionManager.open(realSessionFile).getEntries();
81+
const history = entries.flatMap((entry) =>
82+
entry?.type === "message" ? [entry.message as unknown] : [],
83+
);
84+
return limitAgentHookHistoryMessages(history, MAX_CLI_SESSION_HISTORY_MESSAGES);
6485
} catch {
6586
return [];
6687
}

‎src/config/types.plugins.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@ export type PluginEntryConfig = {
33
hooks?: {
44
/** Controls prompt mutation via before_prompt_build and prompt fields from legacy before_agent_start. */
55
allowPromptInjection?: boolean;
6+
/**
7+
* Controls access to raw conversation content from llm_input/llm_output/agent_end hooks.
8+
* Non-bundled plugins must opt in explicitly; bundled plugins stay allowed unless disabled.
9+
*/
10+
allowConversationAccess?: boolean;
611
};
712
subagent?: {
813
/** Explicitly allow this plugin to request per-run provider/model overrides for subagent runs. */

0 commit comments

Comments
 (0)