Skip to content

Commit e6ebd64

Browse files
ualtinokAlfonso
andcommitted
fix: preserve Pi dreamer system prompts on retry
Co-Authored-By: Alfonso <alfonso@cortexkit.io>
1 parent dc605cf commit e6ebd64

7 files changed

Lines changed: 185 additions & 28 deletions

File tree

packages/pi-plugin/src/dreamer/index.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,42 @@ describe("Pi dreamer wiring", () => {
382382
expect(onAdjunctsRefreshNeeded).not.toHaveBeenCalled();
383383
});
384384

385+
test("preserves transient status when a child rejects before producing output", async () => {
386+
db = createDb();
387+
let capturedClient: CapturedDreamClient | null = null;
388+
__test.setStartDreamScheduleTimerFactory(async (registration) => {
389+
capturedClient = registration.client as unknown as CapturedDreamClient;
390+
return mock(() => {});
391+
});
392+
__test.setPiSubagentRunnerFactory(
393+
() =>
394+
({
395+
run: mock(async () => ({
396+
ok: false,
397+
reason: "invalid_prompt",
398+
transient: true,
399+
error: "zero-tool prompt missing",
400+
durationMs: 0,
401+
})),
402+
}) as never,
403+
);
404+
405+
registerPiDreamerProject(
406+
dreamerOptions({
407+
database: db,
408+
projectIdentity: "git:pi-transient-child",
409+
}),
410+
);
411+
const client = requireCapturedClient(capturedClient);
412+
const created = (await client.session.create({})) as { id: string };
413+
await expect(
414+
client.session.prompt({
415+
path: { id: created.id },
416+
body: { system: "system", parts: [{ text: "run dreamer" }] },
417+
}),
418+
).rejects.toMatchObject({ transient: true });
419+
});
420+
385421
test("unregister before timer promise resolves invokes timer cleanup when it eventually resolves", async () => {
386422
db = createDb();
387423
const timerCleanup = mock(() => {});

packages/pi-plugin/src/dreamer/index.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,9 +308,13 @@ function createPiDreamerClient(opts: PiDreamerOptions): DreamTimerClient {
308308
try {
309309
const result = await runPromise;
310310
if (!result.ok) {
311-
throw new Error(
311+
const error = new Error(
312312
`Pi dreamer subagent failed (${result.reason}): ${result.error}`,
313313
);
314+
if (result.transient) {
315+
(error as Error & { transient?: boolean }).transient = true;
316+
}
317+
throw error;
314318
}
315319
dreamSession.messages = [
316320
makeMessage("user", [{ type: "text", text: userMessage }]),

packages/pi-plugin/src/subagent-runner.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,34 @@ describe("subagent-runner pure helpers", () => {
618618
});
619619

620620
describe("PiSubagentRunner spawn lifecycle", () => {
621+
it("refuses to spawn known zero-tool agents without a system prompt", async () => {
622+
const spawnImpl = mock(() => {
623+
throw new Error("spawn must not be reached");
624+
});
625+
// Replace the runner's test seam with a throwing spawn so this assertion
626+
// proves the guard runs before any child process is created.
627+
const guardedRunner = new PiSubagentRunner({
628+
piBinary: "pi-test",
629+
spawnImpl: spawnImpl as never,
630+
});
631+
632+
for (const agent of ["dreamer-classifier", "dreamer-reviewer"]) {
633+
const result = await guardedRunner.run({
634+
...baseOptions,
635+
agent,
636+
systemPrompt: " \n\t",
637+
});
638+
expect(result).toEqual({
639+
ok: false,
640+
reason: "invalid_prompt",
641+
transient: true,
642+
error: `zero-tool Pi subagent "${agent}" requires a non-empty system prompt`,
643+
durationMs: expect.any(Number),
644+
});
645+
}
646+
expect(spawnImpl).not.toHaveBeenCalled();
647+
});
648+
621649
it("treats a terminal stop turn as success even when drain SIGTERM closes the child", async () => {
622650
const child = createMockChild();
623651
const { runner } = runnerWith(child);

packages/pi-plugin/src/subagent-runner.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,12 @@ const STRICT_TOOL_ALLOWLIST: ReadonlyMap<string, readonly string[]> = new Map(
276276
STRICT_TOOL_ALLOWLIST_ENTRIES,
277277
);
278278

279+
const ZERO_TOOL_PROMPT_REQUIRED_AGENTS: ReadonlySet<string> = new Set(
280+
STRICT_TOOL_ALLOWLIST_ENTRIES.filter(([, tools]) => tools.length === 0).map(
281+
([agent]) => agent,
282+
),
283+
);
284+
279285
const KNOWN_PI_SUBAGENT_AGENTS = [
280286
"magic-context-historian",
281287
"historian",
@@ -539,15 +545,18 @@ export class PiSubagentRunner implements SubagentRunner {
539545
}
540546
return result;
541547
}
548+
542549
const failBeforeSpawn = (
543550
reason: Extract<SubagentRunResult, { ok: false }>["reason"],
544551
error: string,
552+
transient = false,
545553
): SubagentRunResult => {
546554
const result: SubagentRunResult = {
547555
ok: false,
548556
reason,
549557
error,
550558
durationMs: Date.now() - startTime,
559+
...(transient ? { transient: true } : {}),
551560
};
552561
try {
553562
recordAccounting(result);
@@ -560,6 +569,20 @@ export class PiSubagentRunner implements SubagentRunner {
560569
return result;
561570
};
562571

572+
// A zero-tool child cannot receive its task instructions unless a system
573+
// prompt is provided. Refuse before spawning so Pi cannot substitute a
574+
// persisted user-mode prompt.
575+
if (
576+
ZERO_TOOL_PROMPT_REQUIRED_AGENTS.has(options.agent) &&
577+
options.systemPrompt.trim().length === 0
578+
) {
579+
return failBeforeSpawn(
580+
"invalid_prompt",
581+
`zero-tool Pi subagent "${options.agent}" requires a non-empty system prompt`,
582+
true,
583+
);
584+
}
585+
563586
// Large prompts (e.g. a ~50K-token historian chunk ≈ 200 KB) overflow
564587
// Linux's per-argv-entry limit (MAX_ARG_STRLEN, 128 KiB) and make spawn()
565588
// fail with E2BIG. Windows is stricter: CreateProcess caps the ENTIRE
@@ -1486,4 +1509,5 @@ export const __test = {
14861509
DREAMER_ACTION_AGENTS,
14871510
KNOWN_PI_SUBAGENT_AGENTS,
14881511
STRICT_TOOL_ALLOWLIST,
1512+
ZERO_TOOL_PROMPT_REQUIRED_AGENTS,
14891513
};

packages/plugin/src/shared/model-suggestion-retry.test.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ import {
66
} from "./model-suggestion-retry";
77

88
type PromptCall = {
9-
body: { model?: { providerID: string; modelID: string } };
9+
body: {
10+
model?: { providerID: string; modelID: string };
11+
system?: string;
12+
[key: string]: unknown;
13+
};
1014
signal?: AbortSignal;
1115
};
1216

@@ -338,6 +342,42 @@ describe("promptSyncWithValidatedOutputRetry", () => {
338342
expect(messages).toHaveBeenCalledTimes(1);
339343
});
340344

345+
test("preserves body.system when a failed Pi-shaped attempt mutates its body", async () => {
346+
const prompt = mock(async (args: PromptCall) => {
347+
if (prompt.mock.calls.length === 1) {
348+
// Reproduce a facade/SDK that consumes the request body before
349+
// rejecting the primary model. The fallback must not inherit
350+
// that mutation and spawn without its task prompt.
351+
delete args.body.system;
352+
throw new Error("primary failed");
353+
}
354+
return {};
355+
});
356+
const client = createClient(prompt);
357+
const systemPrompt = "CLASSIFY_SYSTEM_PROMPT";
358+
359+
await promptSyncWithValidatedOutputRetry(
360+
client,
361+
{
362+
path: { id: "ses-classify" },
363+
body: {
364+
agent: "dreamer-classifier",
365+
system: systemPrompt,
366+
parts: [{ type: "text", text: "classify" }],
367+
},
368+
},
369+
{
370+
fallbackModels: ["anthropic/claude-sonnet-4-6"],
371+
fetchOutput: async () => "fallback-output",
372+
validateOutput: (output: string) => output,
373+
},
374+
);
375+
376+
expect(prompt).toHaveBeenCalledTimes(2);
377+
expect((prompt.mock.calls[0]?.[0] as PromptCall).body.system).toBeUndefined();
378+
expect((prompt.mock.calls[1]?.[0] as PromptCall).body.system).toBe(systemPrompt);
379+
});
380+
341381
test("empty first model tries the next fallback", async () => {
342382
const prompt = mock(async () => ({}));
343383
const messages = mock(async () =>

packages/plugin/src/shared/model-suggestion-retry.ts

Lines changed: 47 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,15 @@ export type PromptArgs = {
2323
[key: string]: unknown;
2424
};
2525

26+
/**
27+
* Keep a prompt body independent from SDK/client mutation. Some prompt facades
28+
* normalize or consume request bodies while handling a failed attempt; fallback
29+
* attempts must start from the original system prompt and request body every time.
30+
*/
31+
function copyPromptArgs(args: PromptArgs, body: PromptBody): PromptArgs {
32+
return { ...args, body: { ...body } };
33+
}
34+
2635
export interface PromptAttemptInfo {
2736
/** Human-readable model label used in logs ("primary" or "provider/model"). */
2837
label: string;
@@ -280,16 +289,21 @@ async function attemptOnce(
280289
callContext: string,
281290
label: string,
282291
): Promise<void> {
292+
// Keep this snapshot separate from the object passed to the client. A
293+
// failed prompt facade may rewrite its request body before rejecting; the
294+
// model-suggestion retry still needs the original system prompt.
295+
const originalBody = { ...args.body };
296+
const attemptArgs = copyPromptArgs(args, originalBody);
283297
try {
284-
await promptWithTimeout(client, args, timeoutMs, signal);
298+
await promptWithTimeout(client, attemptArgs, timeoutMs, signal);
285299
return;
286300
} catch (error) {
287301
// If non-retryable (abort, overflow, timeout), bubble up immediately.
288302
// Don't even try suggestion retry — caller needs the original error.
289303
if (isNonRetryable(error, signal)) throw error;
290304

291305
const suggestion = parseModelSuggestion(error);
292-
if (!suggestion || !args.body.model) {
306+
if (!suggestion || !originalBody.model) {
293307
// No suggestion available — caller's fallback loop will decide
294308
// whether to try the next chain entry.
295309
throw error;
@@ -302,16 +316,13 @@ async function attemptOnce(
302316

303317
await promptWithTimeout(
304318
client,
305-
{
306-
...args,
307-
body: {
308-
...args.body,
309-
model: {
310-
providerID: suggestion.providerID,
311-
modelID: suggestion.suggestion,
312-
},
319+
copyPromptArgs(args, {
320+
...originalBody,
321+
model: {
322+
providerID: suggestion.providerID,
323+
modelID: suggestion.suggestion,
313324
},
314-
},
325+
}),
315326
timeoutMs,
316327
signal,
317328
);
@@ -340,20 +351,25 @@ export async function promptSyncWithModelSuggestionRetry(
340351
const timeoutMs = options.timeoutMs ?? 300_000;
341352
const callContext = options.callContext ?? "subagent";
342353
const fallbacks = options.fallbackModels ?? [];
354+
// Snapshot the body before the first client call. The Pi facade and the
355+
// OpenCode SDK both receive this same shape, and either may mutate it while
356+
// handling a failed request. Fallbacks must never inherit that mutation.
357+
const baseBody = { ...args.body };
358+
const baseArgs = copyPromptArgs(args, baseBody);
343359

344360
// Attempt 0 = whatever the agent or explicit body.model resolves to.
345361
// Subsequent attempts override body.model with each fallback in order.
346362
const explicitPrimaryLabel =
347-
args.body.model?.providerID && args.body.model.modelID
348-
? `${args.body.model.providerID}/${args.body.model.modelID}`
363+
baseBody.model?.providerID && baseBody.model.modelID
364+
? `${baseBody.model.providerID}/${baseBody.model.modelID}`
349365
: "primary";
350366

351367
let lastError: unknown = null;
352368

353369
try {
354370
await attemptOnce(
355371
client,
356-
args,
372+
baseArgs,
357373
timeoutMs,
358374
options.signal,
359375
callContext,
@@ -385,10 +401,10 @@ export async function promptSyncWithModelSuggestionRetry(
385401
}
386402

387403
const label = `${parsed.providerID}/${parsed.modelID}`;
388-
const attemptArgs: PromptArgs = {
389-
...args,
390-
body: { ...args.body, model: parsed },
391-
};
404+
const attemptArgs = copyPromptArgs(baseArgs, {
405+
...baseBody,
406+
model: parsed,
407+
});
392408

393409
try {
394410
await attemptOnce(client, attemptArgs, timeoutMs, options.signal, callContext, label);
@@ -453,10 +469,15 @@ export async function promptSyncWithValidatedOutputRetry<TOutput, TValidated = T
453469
const timeoutMs = options.timeoutMs ?? 300_000;
454470
const callContext = options.callContext ?? "subagent";
455471
const fallbacks = options.fallbackModels ?? [];
472+
// Snapshot the body before the first client call. The Pi facade and the
473+
// OpenCode SDK both receive this same shape, and either may mutate it while
474+
// handling a failed request. Fallbacks must never inherit that mutation.
475+
const baseBody = { ...args.body };
476+
const baseArgs = copyPromptArgs(args, baseBody);
456477

457478
const explicitPrimaryLabel =
458-
args.body.model?.providerID && args.body.model.modelID
459-
? `${args.body.model.providerID}/${args.body.model.modelID}`
479+
baseBody.model?.providerID && baseBody.model.modelID
480+
? `${baseBody.model.providerID}/${baseBody.model.modelID}`
460481
: "primary";
461482
const totalAttempts = fallbacks.length + 1;
462483

@@ -466,7 +487,7 @@ export async function promptSyncWithValidatedOutputRetry<TOutput, TValidated = T
466487
try {
467488
return await attemptAndValidate(
468489
client,
469-
args,
490+
baseArgs,
470491
timeoutMs,
471492
options.signal,
472493
callContext,
@@ -475,7 +496,7 @@ export async function promptSyncWithValidatedOutputRetry<TOutput, TValidated = T
475496
attemptIndex: 0,
476497
isFallback: false,
477498
totalAttempts,
478-
model: args.body.model,
499+
model: baseBody.model,
479500
},
480501
options,
481502
);
@@ -501,10 +522,10 @@ export async function promptSyncWithValidatedOutputRetry<TOutput, TValidated = T
501522
}
502523

503524
const label = `${parsed.providerID}/${parsed.modelID}`;
504-
const attemptArgs: PromptArgs = {
505-
...args,
506-
body: { ...args.body, model: parsed },
507-
};
525+
const attemptArgs = copyPromptArgs(baseArgs, {
526+
...baseBody,
527+
model: parsed,
528+
});
508529
const attempt: PromptAttemptInfo = {
509530
label,
510531
attemptIndex: i + 1,

packages/plugin/src/shared/subagent-runner.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ export type SubagentProgressEvent =
159159
* reported as `ok: false, reason: "no_assistant"` so callers can try fallback
160160
* models instead of accepting an unusable success.
161161
* - `reason`: failure category, one of:
162+
* - `"invalid_prompt"`: a known zero-tool child was given no system prompt
162163
* - `"timeout"`: hit `timeoutMs` before the child finished
163164
* - `"abort"`: caller's `signal` was triggered
164165
* - `"model_failed"`: every configured model + fallback returned an error
@@ -194,6 +195,7 @@ export type SubagentRunResult =
194195
| {
195196
ok: false;
196197
reason:
198+
| "invalid_prompt"
197199
| "timeout"
198200
| "abort"
199201
| "model_failed"
@@ -204,6 +206,8 @@ export type SubagentRunResult =
204206
| "parse_failed";
205207
error: string;
206208
durationMs: number;
209+
/** True when the caller should retry the task rather than advance its schedule. */
210+
transient?: boolean;
207211
meta?: Record<string, unknown>;
208212
};
209213

0 commit comments

Comments
 (0)