mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-19 05:01:15 +00:00
fix: preserve fallback prompt on model fallback for new sessions (#55632) (thanks @openperf)
* fix(agents): preserve original task prompt on model fallback for new sessions * fix(agents): use dynamic transcript check for sessionHasHistory on fallback retry Address Greptile review feedback: replace the static !isNewSession flag with a dynamic sessionFileHasContent() check that reads the on-disk transcript before each fallback retry. This correctly handles the edge case where the primary model completes at least one assistant-response turn (flushing the user message to disk) before failing - the fallback now sends the recovery prompt instead of duplicating the original body. The !isNewSession short-circuit is kept as a fast path so existing sessions skip the file read entirely. * fix(agents): address security vulnerabilities in session fallback logic Fixes three medium-severity security issues identified by Aisle Security Analysis on PR #55632: - CWE-400: Unbounded session transcript read in sessionFileHasContent() - CWE-400: Symlink-following in sessionFileHasContent() - CWE-201: Sensitive prompt replay to a different fallback provider * fix(agents): use JSONL parsing for session history detection (CWE-703) Replace bounded byte-prefix substring matching in sessionFileHasContent() with line-by-line JSONL record parsing. The previous approach could miss an assistant message when the preceding user content exceeded the 256KB read limit, causing a false negative that blocks cross-provider fallback entirely. * fix(agents): preserve fallback prompt across providers --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us>
This commit is contained in:
@@ -57,6 +57,7 @@ import {
|
||||
persistSessionEntry as persistSessionEntryBase,
|
||||
prependInternalEventContext,
|
||||
runAgentAttempt,
|
||||
sessionFileHasContent,
|
||||
} from "./command/attempt-execution.js";
|
||||
import { deliverAgentCommandResult } from "./command/delivery.js";
|
||||
import { resolveAgentRunContext } from "./command/run-context.js";
|
||||
@@ -756,7 +757,7 @@ async function agentCommandInternal(
|
||||
runId,
|
||||
agentDir,
|
||||
fallbacksOverride: effectiveFallbacksOverride,
|
||||
run: (providerOverride, modelOverride, runOptions) => {
|
||||
run: async (providerOverride, modelOverride, runOptions) => {
|
||||
const isFallbackRetry = fallbackAttemptIndex > 0;
|
||||
fallbackAttemptIndex += 1;
|
||||
return runAgentAttempt({
|
||||
@@ -785,6 +786,7 @@ async function agentCommandInternal(
|
||||
sessionStore,
|
||||
storePath,
|
||||
allowTransientCooldownProbe: runOptions?.allowTransientCooldownProbe,
|
||||
sessionHasHistory: !isNewSession || (await sessionFileHasContent(sessionFile)),
|
||||
onAgentEvent: (evt) => {
|
||||
// Track lifecycle end for fallback emission below.
|
||||
if (
|
||||
|
||||
159
src/agents/command/attempt-execution.test.ts
Normal file
159
src/agents/command/attempt-execution.test.ts
Normal file
@@ -0,0 +1,159 @@
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "vitest";
|
||||
import { resolveFallbackRetryPrompt, sessionFileHasContent } from "./attempt-execution.js";
|
||||
|
||||
describe("resolveFallbackRetryPrompt", () => {
|
||||
const originalBody = "Summarize the quarterly earnings report and highlight key trends.";
|
||||
|
||||
it("returns original body on first attempt (isFallbackRetry=false)", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: false,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
});
|
||||
|
||||
it("returns recovery prompt for fallback retry with existing session history", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: true,
|
||||
sessionHasHistory: true,
|
||||
}),
|
||||
).toBe("Continue where you left off. The previous model attempt failed or timed out.");
|
||||
});
|
||||
|
||||
it("preserves original body for fallback retry when session has no history (subagent spawn)", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: true,
|
||||
sessionHasHistory: false,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
});
|
||||
|
||||
it("preserves original body for fallback retry when sessionHasHistory is undefined", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: true,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
});
|
||||
|
||||
it("returns original body on first attempt regardless of sessionHasHistory", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: false,
|
||||
sessionHasHistory: true,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: false,
|
||||
sessionHasHistory: false,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
});
|
||||
|
||||
it("preserves original body on fallback retry without history", () => {
|
||||
expect(
|
||||
resolveFallbackRetryPrompt({
|
||||
body: originalBody,
|
||||
isFallbackRetry: true,
|
||||
sessionHasHistory: false,
|
||||
}),
|
||||
).toBe(originalBody);
|
||||
});
|
||||
});
|
||||
|
||||
describe("sessionFileHasContent", () => {
|
||||
let tmpDir: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "oc-test-"));
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await fs.rm(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("returns false for undefined sessionFile", async () => {
|
||||
expect(await sessionFileHasContent(undefined)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when session file does not exist", async () => {
|
||||
expect(await sessionFileHasContent(path.join(tmpDir, "nonexistent.jsonl"))).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when session file is empty", async () => {
|
||||
const file = path.join(tmpDir, "empty.jsonl");
|
||||
await fs.writeFile(file, "", "utf-8");
|
||||
expect(await sessionFileHasContent(file)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false when session file has only user message (no assistant flush)", async () => {
|
||||
const file = path.join(tmpDir, "user-only.jsonl");
|
||||
await fs.writeFile(
|
||||
file,
|
||||
'{"type":"session","id":"s1"}\n{"type":"message","message":{"role":"user","content":"hello"}}\n',
|
||||
"utf-8",
|
||||
);
|
||||
expect(await sessionFileHasContent(file)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns true when session file has assistant message (flushed)", async () => {
|
||||
const file = path.join(tmpDir, "with-assistant.jsonl");
|
||||
await fs.writeFile(
|
||||
file,
|
||||
'{"type":"session","id":"s1"}\n{"type":"message","message":{"role":"user","content":"hello"}}\n{"type":"message","message":{"role":"assistant","content":"hi"}}\n',
|
||||
"utf-8",
|
||||
);
|
||||
expect(await sessionFileHasContent(file)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true when session file has spaced JSON (role : assistant)", async () => {
|
||||
const file = path.join(tmpDir, "spaced.jsonl");
|
||||
await fs.writeFile(
|
||||
file,
|
||||
'{"type":"message","message":{"role": "assistant","content":"hi"}}\n',
|
||||
"utf-8",
|
||||
);
|
||||
expect(await sessionFileHasContent(file)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true when assistant message appears after large user content", async () => {
|
||||
const file = path.join(tmpDir, "large-user.jsonl");
|
||||
// Create a user message whose JSON line exceeds 256KB to ensure the
|
||||
// JSONL-based parser (CWE-703 fix) finds the assistant record that a
|
||||
// naive byte-prefix approach would miss.
|
||||
const bigContent = "x".repeat(300 * 1024);
|
||||
const lines =
|
||||
[
|
||||
`{"type":"session","id":"s1"}`,
|
||||
`{"type":"message","message":{"role":"user","content":"${bigContent}"}}`,
|
||||
`{"type":"message","message":{"role":"assistant","content":"done"}}`,
|
||||
].join("\n") + "\n";
|
||||
await fs.writeFile(file, lines, "utf-8");
|
||||
expect(await sessionFileHasContent(file)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns false when session file is a symbolic link", async () => {
|
||||
const realFile = path.join(tmpDir, "real.jsonl");
|
||||
await fs.writeFile(
|
||||
realFile,
|
||||
'{"type":"message","message":{"role":"assistant","content":"hi"}}\n',
|
||||
"utf-8",
|
||||
);
|
||||
const link = path.join(tmpDir, "link.jsonl");
|
||||
await fs.symlink(realFile, link);
|
||||
expect(await sessionFileHasContent(link)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,5 @@
|
||||
import fs from "node:fs/promises";
|
||||
import readline from "node:readline";
|
||||
import { SessionManager } from "@mariozechner/pi-coding-agent";
|
||||
import { normalizeReplyPayload } from "../../auto-reply/reply/normalize-reply.js";
|
||||
import type { ThinkLevel, VerboseLevel } from "../../auto-reply/thinking.js";
|
||||
@@ -29,6 +30,67 @@ import type { AgentCommandOpts } from "./types.js";
|
||||
|
||||
const log = createSubsystemLogger("agents/agent-command");
|
||||
|
||||
/** Maximum number of JSONL records to inspect before giving up. */
|
||||
const SESSION_FILE_MAX_RECORDS = 500;
|
||||
|
||||
/**
|
||||
* Check whether a session transcript file exists and contains at least one
|
||||
* assistant message, indicating that the SessionManager has flushed the
|
||||
* initial user+assistant exchange to disk. This is used to decide whether
|
||||
* a fallback retry can rely on the on-disk history or must re-send the
|
||||
* original prompt.
|
||||
*
|
||||
* The check parses JSONL records line-by-line (CWE-703) instead of relying
|
||||
* on a raw substring match against a bounded byte prefix, which could
|
||||
* produce false negatives when the pre-assistant content exceeds the byte
|
||||
* limit.
|
||||
*/
|
||||
export async function sessionFileHasContent(sessionFile: string | undefined): Promise<boolean> {
|
||||
if (!sessionFile) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
// Guard against symlink-following (CWE-400 / arbitrary-file-read vector).
|
||||
const stat = await fs.lstat(sessionFile);
|
||||
if (stat.isSymbolicLink()) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const fh = await fs.open(sessionFile, "r");
|
||||
try {
|
||||
const rl = readline.createInterface({ input: fh.createReadStream({ encoding: "utf-8" }) });
|
||||
let recordCount = 0;
|
||||
for await (const line of rl) {
|
||||
if (!line.trim()) {
|
||||
continue;
|
||||
}
|
||||
recordCount++;
|
||||
if (recordCount > SESSION_FILE_MAX_RECORDS) {
|
||||
break;
|
||||
}
|
||||
let obj: unknown;
|
||||
try {
|
||||
obj = JSON.parse(line);
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
const rec = obj as Record<string, unknown> | null;
|
||||
if (
|
||||
rec?.type === "message" &&
|
||||
(rec.message as Record<string, unknown> | undefined)?.role === "assistant"
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
} finally {
|
||||
await fh.close();
|
||||
}
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
export type PersistSessionEntryParams = {
|
||||
sessionStore: Record<string, SessionEntry>;
|
||||
sessionKey: string;
|
||||
@@ -54,10 +116,19 @@ export async function persistSessionEntry(params: PersistSessionEntryParams): Pr
|
||||
export function resolveFallbackRetryPrompt(params: {
|
||||
body: string;
|
||||
isFallbackRetry: boolean;
|
||||
sessionHasHistory?: boolean;
|
||||
}): string {
|
||||
if (!params.isFallbackRetry) {
|
||||
return params.body;
|
||||
}
|
||||
// When the session has no persisted history (e.g. a freshly-spawned subagent
|
||||
// whose first attempt failed before the SessionManager flushed the user
|
||||
// message to disk), the fallback model would receive only the generic
|
||||
// recovery prompt and lose the original task entirely. Preserve the
|
||||
// original body in that case so the fallback model can execute the task.
|
||||
if (!params.sessionHasHistory) {
|
||||
return params.body;
|
||||
}
|
||||
return "Continue where you left off. The previous model attempt failed or timed out.";
|
||||
}
|
||||
|
||||
@@ -257,10 +328,12 @@ export function runAgentAttempt(params: {
|
||||
sessionStore?: Record<string, SessionEntry>;
|
||||
storePath?: string;
|
||||
allowTransientCooldownProbe?: boolean;
|
||||
sessionHasHistory?: boolean;
|
||||
}) {
|
||||
const effectivePrompt = resolveFallbackRetryPrompt({
|
||||
body: params.body,
|
||||
isFallbackRetry: params.isFallbackRetry,
|
||||
sessionHasHistory: params.sessionHasHistory,
|
||||
});
|
||||
const bootstrapPromptWarningSignaturesSeen = resolveBootstrapWarningSignaturesSeen(
|
||||
params.sessionEntry?.systemPromptReport,
|
||||
|
||||
Reference in New Issue
Block a user