mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 17:51:58 +01:00
fix(runner): preserve deliberate OpenCode approval waits (#18610)
## Thinking Path > - Paperclip coordinates agent work and human approval. > - A saved Plan must wait for the user before work continues. > - The native runner interrupts the provider after it saves that wait. > - OpenCode can report the interruption with an idle event and no final answer. > - Empty-answer recovery treated that intentional pause as an incomplete answer. > - This change stops answer recovery for an interrupted turn. > - The runner still requires a real provider terminal before it settles the wait. ## Linked Issues or Issue Description **What happened?** OpenCode could send a corrective model request after Paperclip interrupted a turn for a saved approval request. The native runner then failed with `governed_wait_provider_terminal_unproven` because it did not receive the required terminal event within the existing drain window. **Expected behavior** An interrupted turn must stop requesting answers. A provider idle or cancelled event must remain available to the native wait settlement. An HTTP abort acknowledgement alone must not count as a terminal. **Steps to reproduce** 1. Start an OpenCode turn that has no final answer. 2. Interrupt that turn after saving a durable approval request. 3. Have the provider return `session.idle` or an idle `session.status`. 4. Observe that the old driver sends a second prompt instead of emitting the terminal event. The controlled regression fails before the fix for both idle event forms: it observes two prompts instead of one. The affected driver blob is identical in the deployed source and the current base. Related: #18364 added the empty-answer recovery path. ## What Changed - Record interruption intent for the active OpenCode turn before asynchronous settlement. - Cancel its answer-recovery timer and prevent corrective requests for that turn. - Preserve provider terminal checks and avoid a delayed abort of a later turn. - Cover both idle event forms, an existing correction, durable question handoff, in-flight tools, missing terminal evidence, and a later authorized turn. - Document the interruption contract. ## Verification - The new regression failed before the fix for the expected extra prompt. A second red test confirmed the sibling durable-question handoff also sent that unwanted prompt. - Interrupted idle events now produce explicit cancellation terminals. Tests reject a failed terminal and require normal completion on the later authorized turn. - All 305 targeted tests passed across the OpenCode driver, normal empty-answer recovery, interruption, backend publication, and native session runtime suites. - Runner TypeScript check passed: `tsc -p packages/paperclip-runner/tsconfig.json --noEmit`. - `git diff --check` passed. - Greptile reviewed the current commit at 5/5 with no findings. Both earlier Paperclip review findings are fixed and their threads are resolved. - The additional Paperclip advisory reviewer could not assess the final commit because its earlier workspace save did not finish. Its check is neutral; this is not a passing assessment. No reviewer configuration or workspace safety gate was changed. - Repository CI passed on run 41470761437, attempt 2. Attempt 1 retained a 15-second timeout in an unchanged voice-report test; only the failed jobs were retried once, on the same commit. The identical test also passed in an isolated baseline check against disposable PostgreSQL with its original timeout. - Live QA acceptance is pending. No eval prompt, model, allowance, timeout, or assertion changed. ## Risks The change affects OpenCode interruption and answer recovery. A silent provider must remain unproven; the negative control checks that case. Ordinary answer recovery remains enabled on authorized turns. No schema, permission, or provider configuration changes are required. ## Model Used OpenAI GPT-6 through Codex, with reasoning, code execution, and browser tools. The runtime does not expose a more specific model identifier or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
aab87ce707
commit
42c64d4ec4
3 files changed
+150
-13
No files matched your search
@@ -391,6 +391,12 @@ runner supplies the pinned executable, selected model, and assigned MCP tools.
|
||||
Provider inference and reasoning still contribute to time to first text. These
|
||||
startup settings do not disable model reasoning.
|
||||
|
||||
An interrupted OpenCode turn does not request a replacement final answer.
|
||||
This also applies when an unanswered native question moves to a durable wait.
|
||||
The driver reports an interrupted idle as a cancelled terminal so the wait can
|
||||
settle. An abort acknowledgement alone does not prove that the provider stopped.
|
||||
A later authorized turn can use normal answer recovery.
|
||||
|
||||
On macOS, the verified OpenCode executable keeps its private launch pathname
|
||||
until the provider process exits. Removing that pathname immediately after
|
||||
spawn can kill or stall the signed binary before its health check succeeds.
|
||||
|
||||
@@ -0,0 +1,124 @@
|
||||
import { chmod, copyFile, mkdtemp, rm } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, resolve } from "node:path";
|
||||
import { expect, it, vi } from "vitest";
|
||||
import type { PrpEvent } from "../../protocol/replay-contract.js";
|
||||
import { OpenCodeServerDriver } from "./opencode-server-driver.js";
|
||||
|
||||
it.each(["idle", "status", "silent", "correcting-idle", "correcting-silent", "busy-idle", "handoff-idle", "handoff-status", "handoff-silent"] as const)("does not restart an interrupted empty OpenCode response: %s", async terminal => {
|
||||
const root = await mkdtemp(join(tmpdir(), "opencode-interrupt-empty-"));
|
||||
const command = join(root, "fake-opencode.mjs");
|
||||
await copyFile(resolve("test/fixtures/fake-opencode-server.mjs"), command);
|
||||
await chmod(command, 0o755);
|
||||
let stream: ReadableStreamDefaultController<Uint8Array> | undefined;
|
||||
let streamOpened!: () => void;
|
||||
const opened = new Promise<void>(resolve => { streamOpened = resolve; });
|
||||
const prompts: unknown[] = [];
|
||||
const observed: PrpEvent[] = [];
|
||||
const correcting = terminal.startsWith("correcting-");
|
||||
const silent = terminal.endsWith("silent");
|
||||
const busy = terminal === "busy-idle";
|
||||
const handoff = terminal.startsWith("handoff-");
|
||||
let ownedSessionId = "";
|
||||
const emit = (value: unknown) => stream!.enqueue(new TextEncoder().encode(`data: ${JSON.stringify(value)}\n\n`));
|
||||
let resumed = false;
|
||||
const driver = new OpenCodeServerDriver({
|
||||
model: "openrouter/controlled-model",
|
||||
runtimeDirectory: join(root, "runtime"), command,
|
||||
environment: { PATH: process.env.PATH, OPENROUTER_API_KEY: "controlled-fixture-key" },
|
||||
fetch: async (input, init) => {
|
||||
const url = new URL(String(input));
|
||||
if (url.pathname === "/event") return new Response(new ReadableStream<Uint8Array>({
|
||||
start(controller) { stream = controller; streamOpened(); },
|
||||
}), { headers: { "Content-Type": "text/event-stream" } });
|
||||
const sessionID = decodeURIComponent(url.pathname.split("/")[2] ?? "");
|
||||
if (url.pathname.endsWith("/prompt_async")) {
|
||||
ownedSessionId = sessionID;
|
||||
prompts.push(JSON.parse(String(init?.body)));
|
||||
if (resumed) queueMicrotask(() => {
|
||||
emit({ type: "message.updated", properties: { info: { id: "resumed-answer", sessionID, role: "assistant", finish: "stop", time: { completed: 2 } } } });
|
||||
emit({ type: "message.part.updated", properties: { part: { id: "resumed-part", messageID: "resumed-answer", sessionID, type: "text", text: "The authorized next turn completed.", time: { start: 1, end: 2 } } } });
|
||||
emit({ type: "session.idle", properties: { sessionID } });
|
||||
});
|
||||
else if (correcting && prompts.length === 1) queueMicrotask(() => {
|
||||
emit({ type: "session.idle", properties: { sessionID } });
|
||||
});
|
||||
else if (busy && prompts.length === 1) queueMicrotask(() => {
|
||||
emit({ type: "message.updated", properties: { info: { id: "tool-message", sessionID, role: "assistant" } } });
|
||||
emit({ type: "message.part.updated", properties: { part: { id: "owned-tool", messageID: "tool-message", sessionID,
|
||||
type: "tool", tool: "bash", callID: "owned-call", state: { status: "running", input: { command: "controlled work" } } } } });
|
||||
});
|
||||
else if (handoff && prompts.length === 1) queueMicrotask(() => {
|
||||
emit({ type: "question.asked", properties: { id: "owned-question", sessionID, questions: [
|
||||
{ question: "Which color should I use?", header: "Color", options: [{ label: "Blue", description: "Use blue." }] },
|
||||
] } });
|
||||
});
|
||||
return new Response(null, { status: 204 });
|
||||
}
|
||||
if (url.pathname.endsWith("/abort")) {
|
||||
if (!silent) queueMicrotask(() => {
|
||||
emit(!terminal.endsWith("status")
|
||||
? { type: "session.idle", properties: { sessionID } }
|
||||
: { type: "session.status", properties: { sessionID, status: { type: "idle" } } });
|
||||
});
|
||||
return Response.json(true);
|
||||
}
|
||||
if (url.pathname === "/question/owned-question/reject") return Response.json(true);
|
||||
return fetch(input, init);
|
||||
},
|
||||
});
|
||||
const session = await driver.openSession({ runId: "owned-run", normalizedSessionId: "owned-session", workingDirectory: root });
|
||||
let collecting: Promise<void> | undefined;
|
||||
try {
|
||||
await opened;
|
||||
vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] });
|
||||
collecting = (async () => { for await (const event of session.events()) observed.push(event); })();
|
||||
const first = await session.startTurn({ message: { role: "user", text: "Save a plan and wait for approval." } });
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
const allowedPrompts = correcting ? 2 : 1;
|
||||
expect(prompts).toHaveLength(allowedPrompts);
|
||||
if (handoff) {
|
||||
const pending = session.pendingRuntimeRequests?.();
|
||||
expect(pending).toHaveLength(1);
|
||||
const result = session.handoffRuntimeRequest?.({ requestId: "owned-question", turnId: first.turnId,
|
||||
reason: "durable_handoff", signal: new AbortController().signal });
|
||||
expect(result?.result).toBe("handed_off");
|
||||
await result?.cleanup;
|
||||
expect(observed.filter(event => event.eventType === "runtime_request.expired")).toHaveLength(1);
|
||||
} else await session.interrupt?.({ turnId: first.turnId, reason: "Durable approval is pending." });
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
// HTTP acknowledgement alone is not a provider terminal. An observed idle
|
||||
// is a terminal, but must never authorize a new corrective model request.
|
||||
expect(prompts).toHaveLength(allowedPrompts);
|
||||
const terminalEvents = () => observed.filter(event => ["turn.completed", "turn.cancelled", "turn.failed"].includes(event.eventType));
|
||||
expect(terminalEvents()).toHaveLength(silent || busy ? 0 : 1);
|
||||
if (!silent && !busy) expect(terminalEvents()[0]?.eventType).toBe("turn.cancelled");
|
||||
await vi.advanceTimersByTimeAsync(20_001);
|
||||
expect(prompts).toHaveLength(allowedPrompts);
|
||||
expect(terminalEvents()).toHaveLength(silent || busy ? 0 : 1);
|
||||
if (busy) {
|
||||
emit({ type: "message.part.updated", properties: { part: { id: "owned-tool", messageID: "tool-message", sessionID: ownedSessionId,
|
||||
type: "tool", tool: "bash", callID: "owned-call", state: { status: "completed", input: { command: "controlled work" }, output: "Work stopped.", time: { start: 1, end: 2 } } } } });
|
||||
emit({ type: "session.idle", properties: { sessionID: ownedSessionId } });
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
expect(terminalEvents()).toHaveLength(1);
|
||||
expect(terminalEvents()[0]?.eventType).toBe("turn.cancelled");
|
||||
expect(prompts).toHaveLength(allowedPrompts);
|
||||
}
|
||||
if (!silent) {
|
||||
resumed = true;
|
||||
const next = await session.startTurn({ message: { role: "user", text: "The plan is approved. Continue." } });
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
expect(prompts).toHaveLength(allowedPrompts + 1);
|
||||
expect(terminalEvents().filter(event => event.turnId === next.turnId)).toHaveLength(1);
|
||||
expect(terminalEvents().find(event => event.turnId === next.turnId)?.eventType).toBe("turn.completed");
|
||||
expect(observed.some(event => event.eventType === "item.completed" && event.payload.kind === "agentMessage"
|
||||
&& event.payload.channel === "final" && event.payload.text === "The authorized next turn completed.")).toBe(true);
|
||||
}
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
await session.close({ reason: "controlled test complete" });
|
||||
await collecting;
|
||||
await rm(root, { recursive: true, force: true });
|
||||
}
|
||||
}, 15_000);
|
||||
@@ -544,6 +544,7 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
#turnReasoningMode: OpenCodeReasoningMode = "default";
|
||||
#emptyResponseRecovery: { turnId: string; priorMessages: Set<string>; responseStarted: boolean; responseFinished: boolean } | null = null;
|
||||
#emptyResponseAckTimer: ReturnType<typeof setTimeout> | undefined;
|
||||
#interruptedTurnId: string | null = null;
|
||||
#pendingToolCalls = 0;
|
||||
readonly #runningNativeToolParts = new Set<string>();
|
||||
#abort = new AbortController();
|
||||
@@ -695,6 +696,7 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
this.#turnStartSourceSeq = this.#sourceSequence;
|
||||
this.#turnReasoningMode = reasoningMode;
|
||||
this.#emptyResponseRecovery = null;
|
||||
this.#interruptedTurnId = null;
|
||||
this.#runningNativeToolParts.clear();
|
||||
this.#emit("turn.submitted", {
|
||||
envelopeSchema: this.#taskEnvelope.schema,
|
||||
@@ -765,7 +767,7 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
}
|
||||
|
||||
#failEmptyResponse(turnId: string, message: string): void {
|
||||
if (this.#closed || this.#activeTurnId !== turnId || this.#result !== null || this.#pendingToolCalls > 0 || this.#runningNativeToolParts.size > 0) return;
|
||||
if (this.#closed || this.#activeTurnId !== turnId || this.#interruptedTurnId === turnId || this.#result !== null || this.#pendingToolCalls > 0 || this.#runningNativeToolParts.size > 0) return;
|
||||
this.#emit("turn.failed", { status: "failed", error: { code: "opencode_empty_response", message, retryable: false } }, { turnId });
|
||||
this.#terminalTurns.set(turnId, canonicalJson({ status: "failed" }));
|
||||
this.#activeTurnId = null;
|
||||
@@ -774,6 +776,9 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
#recoverEmptyResponse(turnId: string): boolean {
|
||||
if (this.#result !== null || this.#pendingRuntimeRequests.size > 0) return false;
|
||||
if (this.#pendingToolCalls > 0 || this.#runningNativeToolParts.size > 0) return true;
|
||||
// An intentional pause needs the provider's idle fact, not another model
|
||||
// request. Keep in-flight tool work gated by the checks above.
|
||||
if (this.#interruptedTurnId === turnId) return false;
|
||||
const recovery = this.#emptyResponseRecovery;
|
||||
const boundary = Math.max(this.#turnStartSourceSeq, this.#lastNonTerminalToolSourceSeq);
|
||||
if (this.#completedTextParts.some(part => part.observedSourceSeq > boundary)) return false;
|
||||
@@ -800,13 +805,18 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
}
|
||||
|
||||
async interrupt(input: { turnId?: string; reason?: string }): Promise<void> {
|
||||
await this.#completionSettlement;
|
||||
const turnId = this.#activeTurnId;
|
||||
if (
|
||||
input.turnId &&
|
||||
this.#activeTurnId &&
|
||||
input.turnId !== this.#activeTurnId
|
||||
turnId &&
|
||||
input.turnId !== turnId
|
||||
)
|
||||
throw new Error("stale OpenCode turn");
|
||||
if (!turnId) return;
|
||||
this.#interruptedTurnId = turnId;
|
||||
this.#clearEmptyResponseAckTimer();
|
||||
await this.#completionSettlement;
|
||||
if (this.#activeTurnId !== turnId) return;
|
||||
await api(
|
||||
this.#fetch,
|
||||
this.#runtime,
|
||||
@@ -982,12 +992,7 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
`/question/${encodeURIComponent(input.requestId)}/reject?${workspace}`,
|
||||
{ method: "POST" },
|
||||
),
|
||||
api(
|
||||
this.#fetch,
|
||||
this.#runtime,
|
||||
`/session/${encodeURIComponent(this.#providerSessionId)}/abort`,
|
||||
{ method: "POST" },
|
||||
),
|
||||
this.interrupt({ turnId: input.turnId, reason: "Durable user input handoff." }),
|
||||
]).then(() => undefined);
|
||||
return { result: "handed_off", cleanup };
|
||||
}
|
||||
@@ -1864,12 +1869,14 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
turnId
|
||||
) {
|
||||
if (this.#recoverEmptyResponse(turnId)) return;
|
||||
const interrupted = this.#interruptedTurnId === turnId;
|
||||
const status = interrupted ? "cancelled" : "completed";
|
||||
const fingerprint = canonicalJson({
|
||||
status: "completed",
|
||||
status,
|
||||
semanticResult: this.#resultFingerprint,
|
||||
});
|
||||
this.#terminalTurns.set(turnId, fingerprint);
|
||||
this.#emitSettledFinalAgentMessage(turnId);
|
||||
if (!interrupted) this.#emitSettledFinalAgentMessage(turnId);
|
||||
const workspace = this.#workspaceChangesByTurn.get(turnId);
|
||||
if (workspace !== undefined)
|
||||
this.#emit(
|
||||
@@ -1884,7 +1891,7 @@ class OpenCodeHarnessSession implements HarnessSession {
|
||||
// Emit while this turn is still `#activeTurnId`; the gate in `#emit`
|
||||
// drops any frame whose turnId is not the active turn, so nulling it
|
||||
// first would make `#emit` drop this very event.
|
||||
this.#emit("turn.completed", { status: "completed" }, { turnId });
|
||||
this.#emit(interrupted ? "turn.cancelled" : "turn.completed", { status }, { turnId });
|
||||
this.#activeTurnId = null;
|
||||
return;
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user