mirror of
https://github.com/logancyang/obsidian-copilot.git
synced 2026-07-22 07:50:24 +00:00
fix(agent-mode): stop cancelled OpenCode retries (#2685)
* fix(agent-mode): stop cancelled OpenCode retries * fix(agent-mode): serialize follow-ups after cancel * fix(agent-mode): drain cancelled event tails
This commit is contained in:
parent
423c456f5f
commit
e89bd562e9
6 changed files with 204 additions and 19 deletions
|
|
@ -317,9 +317,9 @@ export async function buildOpencodeConfig(
|
|||
// native `build` agent this only adds the external_directory key — bash/edit
|
||||
// stay at opencode's permissive defaults, so it does not start asking.
|
||||
//
|
||||
// NOTE (version-sensitive, pinned opencode 1.15.13): the `external_directory`
|
||||
// NOTE (version-sensitive, pinned opencode 1.16.0): the `external_directory`
|
||||
// permission key and the `{ "<glob>": "allow" }` shape are confirmed against
|
||||
// 1.15.13; re-verify when the pinned opencode version changes.
|
||||
// 1.16.0; re-verify when the pinned opencode version changes.
|
||||
const externalDirectoryPermission = cacheRoot
|
||||
? { external_directory: { [`${cacheRoot}/**`]: "allow" } }
|
||||
: undefined;
|
||||
|
|
|
|||
|
|
@ -56,6 +56,7 @@ import * as os from "node:os";
|
|||
import * as path from "node:path";
|
||||
import {
|
||||
computeInstallState,
|
||||
isOpencodeVersionOutdated,
|
||||
legacyVaultDataDir,
|
||||
opencodeManagedDataDir,
|
||||
OpencodeBinaryManager,
|
||||
|
|
@ -65,6 +66,13 @@ import {
|
|||
verifyOpencodeBinary,
|
||||
} from "./OpencodeBinaryManager";
|
||||
|
||||
describe("isOpencodeVersionOutdated", () => {
|
||||
it("requires the ACP cancellation fix shipped in 1.16.0", () => {
|
||||
expect(isOpencodeVersionOutdated("1.15.13")).toBe(true);
|
||||
expect(isOpencodeVersionOutdated("1.16.0")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// Pull the mock helpers off the mocked module without TS complaints.
|
||||
const settingsMock = jest.requireMock("@/settings/model");
|
||||
|
||||
|
|
|
|||
|
|
@ -128,10 +128,10 @@ export function readOpencodeSettings(): OpencodeBackendSettings {
|
|||
|
||||
/**
|
||||
* Whether an installed opencode version predates the minimum the plugin
|
||||
* supports ({@link OPENCODE_MIN_ACP_VERSION}). Older binaries advertise models
|
||||
* through the now-removed ACP `models` state, so the picker can't surface them.
|
||||
* An unknown version isn't flagged — we can't prove it's old, and a missing
|
||||
* install is handled by the install prompt instead.
|
||||
* supports ({@link OPENCODE_MIN_ACP_VERSION}). Older binaries either predate
|
||||
* the current model catalog or leave the backing turn running after ACP
|
||||
* cancellation. An unknown version isn't flagged — we can't prove it's old,
|
||||
* and a missing install is handled by the install prompt instead.
|
||||
*/
|
||||
export function isOpencodeVersionOutdated(version: string | undefined): boolean {
|
||||
if (!version) return false;
|
||||
|
|
|
|||
|
|
@ -1029,6 +1029,97 @@ describe("AgentSession.sendPrompt", () => {
|
|||
resolvePrompt!({ stopReason: "cancelled" });
|
||||
expect(await turn).toBe("cancelled");
|
||||
});
|
||||
|
||||
it("settles locally and drops retry output when backend cancellation fails", async () => {
|
||||
const mock = makeMockBackend();
|
||||
let resolveBackingPrompt: ((value: { stopReason: "cancelled" }) => void) | null = null;
|
||||
let resolveSecondPrompt: ((value: { stopReason: "end_turn" }) => void) | null = null;
|
||||
mock.prompt
|
||||
.mockImplementationOnce(
|
||||
() =>
|
||||
new Promise((resolve) => {
|
||||
resolveBackingPrompt = resolve;
|
||||
})
|
||||
)
|
||||
.mockImplementationOnce(
|
||||
() =>
|
||||
new Promise((resolve) => {
|
||||
resolveSecondPrompt = resolve;
|
||||
})
|
||||
);
|
||||
const session = new AgentSession({
|
||||
backend: mock.asBackend,
|
||||
backendSessionId: "acp-1",
|
||||
internalId: "internal-1",
|
||||
backendId: "opencode",
|
||||
});
|
||||
const emitChunk = (
|
||||
sessionUpdate: "agent_thought_chunk" | "agent_message_chunk",
|
||||
text: string,
|
||||
messageId = "msg-first"
|
||||
) =>
|
||||
mock.emit({
|
||||
sessionId: "acp-1",
|
||||
update: { sessionUpdate, messageId, content: { type: "text", text } },
|
||||
});
|
||||
const { turn } = session.sendPrompt("first");
|
||||
|
||||
emitChunk("agent_thought_chunk", "initial thought");
|
||||
emitChunk("agent_message_chunk", "partial answer");
|
||||
mock.cancel.mockImplementation(() => {
|
||||
emitChunk("agent_thought_chunk", " retry during cancel");
|
||||
mock.emit({
|
||||
sessionId: "acp-1",
|
||||
update: {
|
||||
sessionUpdate: "tool_call",
|
||||
toolCallId: "tc-stale",
|
||||
title: "Stale retry tool",
|
||||
status: "pending",
|
||||
},
|
||||
});
|
||||
return Promise.reject(new Error("cancel unsupported"));
|
||||
});
|
||||
|
||||
await session.cancel();
|
||||
await expect(turn).resolves.toBe("cancelled");
|
||||
expect(session.getStatus()).toBe("idle");
|
||||
|
||||
emitChunk("agent_message_chunk", " retry after cancel");
|
||||
const firstAnswer = session.store
|
||||
.getDisplayMessages()
|
||||
.find((message) => message.sender === AI_SENDER);
|
||||
expect(firstAnswer?.message).toBe("partial answer");
|
||||
expect(firstAnswer?.parts).toEqual(
|
||||
expect.arrayContaining([
|
||||
expect.objectContaining({ kind: "thought", text: "initial thought" }),
|
||||
])
|
||||
);
|
||||
expect(firstAnswer?.parts?.some((part) => part.kind === "tool_call")).toBe(false);
|
||||
expect(firstAnswer?.turnStopReason).toBe("cancelled");
|
||||
|
||||
const next = session.sendPrompt("second");
|
||||
expect(mock.prompt).toHaveBeenCalledTimes(1);
|
||||
emitChunk("agent_message_chunk", " stale retry", "msg-retry");
|
||||
|
||||
jest.useFakeTimers();
|
||||
try {
|
||||
resolveBackingPrompt!({ stopReason: "cancelled" });
|
||||
await jest.advanceTimersByTimeAsync(500);
|
||||
emitChunk("agent_message_chunk", " delayed stale retry", "msg-retry");
|
||||
await jest.advanceTimersByTimeAsync(500);
|
||||
expect(mock.prompt).toHaveBeenCalledTimes(1);
|
||||
|
||||
await jest.advanceTimersByTimeAsync(1_000);
|
||||
expect(mock.prompt).toHaveBeenCalledTimes(2);
|
||||
emitChunk("agent_message_chunk", " stale trailing chunk", "msg-retry");
|
||||
emitChunk("agent_message_chunk", "recovered", "msg-second");
|
||||
resolveSecondPrompt!({ stopReason: "end_turn" });
|
||||
await expect(next.turn).resolves.toBe("end_turn");
|
||||
expect(session.store.getDisplayMessages().at(-1)?.message).toBe("recovered");
|
||||
} finally {
|
||||
jest.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("withReadOnlyPreamble", () => {
|
||||
|
|
|
|||
|
|
@ -25,6 +25,7 @@ import {
|
|||
PlanSummary,
|
||||
PromptContent,
|
||||
PromptInput,
|
||||
PromptOutput,
|
||||
SessionEvent,
|
||||
SessionId,
|
||||
SessionUsage,
|
||||
|
|
@ -84,6 +85,10 @@ export const DEFAULT_TITLE_PREFIX = "New session";
|
|||
* agent's own context always receives the full output regardless.
|
||||
*/
|
||||
const MAX_TOOL_OUTPUT_TEXT_CHARS = 256_000;
|
||||
// ACP prompt results can arrive before the last session/update frames. Require
|
||||
// a quiet interval after a cancelled backing prompt settles before dispatching
|
||||
// a follow-up on the same session.
|
||||
const CANCELLED_TURN_QUIET_MS = 1_000;
|
||||
// Shared sentinel so `getPendingToolPermissions()` returns a stable reference
|
||||
// when nothing is pending — preserves React `useState` setter bail-out
|
||||
// behavior on idle subscription ticks.
|
||||
|
|
@ -376,6 +381,12 @@ export class AgentSession {
|
|||
// a newer turn's placeholder. Replaced on the next completion, cleared on
|
||||
// cancel/dispose.
|
||||
private settledStream: { placeholderId: string; messageIds: Set<string> } | null = null;
|
||||
// A locally-cancelled prompt whose backend promise has not settled yet. A
|
||||
// follow-up may be composed immediately, but its backend prompt waits on this
|
||||
// barrier so output from the cancelled generation cannot target the new turn.
|
||||
private cancelledPromptDrain: Promise<void> | null = null;
|
||||
private cancelledTurnActivity = 0;
|
||||
private cancelledMessageIds = new Set<string>();
|
||||
private abortController: AbortController | null = null;
|
||||
private listeners = new Set<AgentSessionListener>();
|
||||
private unregisterSessionHandler: (() => void) | null = null;
|
||||
|
|
@ -1002,7 +1013,18 @@ export class AgentSession {
|
|||
): Promise<StopReason> {
|
||||
const placeholderId = this.placeholderId;
|
||||
const sessionId = this.backendSessionId!;
|
||||
const signal = this.abortController!.signal;
|
||||
try {
|
||||
const priorPromptDrain = this.cancelledPromptDrain;
|
||||
if (priorPromptDrain) {
|
||||
await Promise.race([
|
||||
priorPromptDrain,
|
||||
new Promise<void>((resolve) => {
|
||||
signal.addEventListener("abort", () => resolve(), { once: true });
|
||||
}),
|
||||
]);
|
||||
}
|
||||
|
||||
// Extract live Web Viewer content (reader-mode markdown, YouTube
|
||||
// transcripts) just before the prompt is built so it reflects the page
|
||||
// at send/flush time, not at compose time. Only take the async hop when
|
||||
|
|
@ -1083,7 +1105,39 @@ export class AgentSession {
|
|||
sessionId,
|
||||
prompt: promptBlocks,
|
||||
};
|
||||
const resp = await this.backend.prompt(req);
|
||||
const promptStarted = !signal.aborted;
|
||||
let resp: PromptOutput = { stopReason: "cancelled" };
|
||||
if (promptStarted) {
|
||||
const backingPrompt = this.backend.prompt(req);
|
||||
resp = await Promise.race([
|
||||
backingPrompt,
|
||||
new Promise<PromptOutput>((resolve) => {
|
||||
signal.addEventListener("abort", () => resolve({ stopReason: "cancelled" }), {
|
||||
once: true,
|
||||
});
|
||||
}),
|
||||
]);
|
||||
if (signal.aborted) {
|
||||
const settledPrompt = backingPrompt.then(
|
||||
() => undefined,
|
||||
() => undefined
|
||||
);
|
||||
const drain = settledPrompt.then(async () => {
|
||||
let activity: number;
|
||||
do {
|
||||
activity = this.cancelledTurnActivity;
|
||||
await new Promise<void>((resolve) =>
|
||||
window.setTimeout(resolve, CANCELLED_TURN_QUIET_MS)
|
||||
);
|
||||
} while (activity !== this.cancelledTurnActivity);
|
||||
});
|
||||
this.cancelledPromptDrain = drain;
|
||||
void drain.then(() => {
|
||||
if (this.cancelledPromptDrain === drain) this.cancelledPromptDrain = null;
|
||||
});
|
||||
resp = { stopReason: "cancelled" };
|
||||
}
|
||||
}
|
||||
// Flush the buffer only on a non-cancelled completion — only then has the
|
||||
// backend durably ingested the `<prior_turns>` block. A cancelled prompt may
|
||||
// have stopped before ingesting it, so keep and re-inject (a duplicate is
|
||||
|
|
@ -1095,13 +1149,13 @@ export class AgentSession {
|
|||
// the turn, so a hard `prompt()` failure (transport/auth) leaves the flag
|
||||
// unset and the user's retry re-delivers the context (a user cancel still
|
||||
// resolves here, and the prompt did reach the backend, so it counts as delivered).
|
||||
if (isFirstTurn) this.firstPromptSent = true;
|
||||
if (isFirstTurn && promptStarted) this.firstPromptSent = true;
|
||||
// Same delivery contract as `firstPromptSent`: the note reached the backend
|
||||
// in this accepted (possibly cancelled) prompt, so advance the session's
|
||||
// cursor. A hard `prompt()` failure throws before here, leaving the cursor
|
||||
// unmoved so the retry re-delivers the note. Fan-out returns earlier and
|
||||
// never reaches this ack.
|
||||
if (projectContextUpdates) {
|
||||
if (projectContextUpdates && promptStarted) {
|
||||
this.markProjectContextUpdatesDeliveredFn?.(projectContextUpdates.epoch);
|
||||
}
|
||||
if (
|
||||
|
|
@ -1266,9 +1320,9 @@ export class AgentSession {
|
|||
}
|
||||
|
||||
/**
|
||||
* Cancel any in-flight turn. The backend may still emit a few trailing
|
||||
* session events before the prompt promise resolves with
|
||||
* `stopReason: "cancelled"` — that's expected.
|
||||
* Cancel any in-flight turn. Local cancellation settles the prompt even if
|
||||
* the backend ignores ACP `session/cancel`; the backend notification still
|
||||
* runs so a compliant agent can stop work and keep its session reusable.
|
||||
*
|
||||
* Flushes any pending tool-permission and AskUserQuestion resolvers (rejects
|
||||
* / empty answers respectively) so the inline cards disappear immediately
|
||||
|
|
@ -1279,6 +1333,10 @@ export class AgentSession {
|
|||
const status = this.getStatus();
|
||||
if (status !== "running" && status !== "awaiting_permission") return;
|
||||
if (!this.backendSessionId) return;
|
||||
this.abortController?.abort();
|
||||
this.settledStream = null;
|
||||
for (const messageId of this.currentMessageIds) this.cancelledMessageIds.add(messageId);
|
||||
this.currentMessageIds = new Set();
|
||||
if (this.pendingToolResolvers.size > 0) {
|
||||
this.flushResolvers(this.pendingToolResolvers);
|
||||
this.notifyMessages();
|
||||
|
|
@ -1292,7 +1350,6 @@ export class AgentSession {
|
|||
} catch (e) {
|
||||
logWarn(`[AgentMode] cancel notification failed`, e);
|
||||
}
|
||||
this.abortController?.abort();
|
||||
}
|
||||
|
||||
/** Detach from the backend. Does not cancel — call `cancel()` first. */
|
||||
|
|
@ -1308,6 +1365,8 @@ export class AgentSession {
|
|||
this.currentTodoList = null;
|
||||
this.currentTodoListSignature = null;
|
||||
this.settledStream = null;
|
||||
this.cancelledPromptDrain = null;
|
||||
this.cancelledMessageIds.clear();
|
||||
this.currentMessageIds = new Set();
|
||||
// Fire the `"closed"` transition before clearing listeners so
|
||||
// subscribers still observe it. `disposed = true` above guarantees
|
||||
|
|
@ -1618,6 +1677,27 @@ export class AgentSession {
|
|||
private handleSessionEvent(event: SessionEvent): void {
|
||||
const update = event.update;
|
||||
|
||||
if (this.cancelledPromptDrain || this.abortController?.signal.aborted) {
|
||||
switch (update.sessionUpdate) {
|
||||
case "agent_message_chunk":
|
||||
case "agent_thought_chunk":
|
||||
this.cancelledTurnActivity += 1;
|
||||
if (update.messageId) this.cancelledMessageIds.add(update.messageId);
|
||||
logWarn(
|
||||
`[AgentMode] dropping ${update.sessionUpdate} from cancelled turn ${this.internalId}`
|
||||
);
|
||||
return;
|
||||
case "tool_call":
|
||||
case "tool_call_update":
|
||||
case "plan":
|
||||
this.cancelledTurnActivity += 1;
|
||||
logWarn(
|
||||
`[AgentMode] dropping ${update.sessionUpdate} from cancelled turn ${this.internalId}`
|
||||
);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
// Refresh the live todo snapshot before any placeholder gating — the
|
||||
// snapshot is session-scoped state, not part of the message trail.
|
||||
if (update.sessionUpdate === "plan" && this.applyCurrentTodoList(update.entries)) {
|
||||
|
|
@ -1742,6 +1822,13 @@ export class AgentSession {
|
|||
* when there's no message to append to (a genuinely stray update).
|
||||
*/
|
||||
private resolveContentTarget(messageId: string | undefined): string | null {
|
||||
if (
|
||||
this.cancelledPromptDrain ||
|
||||
this.abortController?.signal.aborted ||
|
||||
(messageId !== undefined && this.cancelledMessageIds.has(messageId))
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
if (messageId && this.settledStream?.messageIds.has(messageId)) {
|
||||
return this.settledStream.placeholderId;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -935,15 +935,14 @@ export const RESTRICTION_MESSAGES = {
|
|||
`${extension.toUpperCase()} files are not supported in the current mode.`,
|
||||
} as const;
|
||||
|
||||
export const OPENCODE_PINNED_VERSION = "1.15.13";
|
||||
export const OPENCODE_PINNED_VERSION = "1.16.0";
|
||||
export const OPENCODE_RELEASE_TAG = `v${OPENCODE_PINNED_VERSION}`;
|
||||
/**
|
||||
* Minimum opencode the plugin supports. v1.15.13 (PR sst/opencode#29929,
|
||||
* "promote next ACP implementation") dropped the dedicated ACP `models` state
|
||||
* and moved the model catalog to a `category:"model"` config option; older
|
||||
* binaries report no models to our picker, so we force an upgrade.
|
||||
* Minimum opencode the plugin supports. v1.15.13 introduced the config-option
|
||||
* model catalog we consume, and v1.16.0 made ACP `session/cancel` abort the
|
||||
* backing turn so a stopped session remains reusable.
|
||||
*/
|
||||
export const OPENCODE_MIN_ACP_VERSION = "1.15.13";
|
||||
export const OPENCODE_MIN_ACP_VERSION = "1.16.0";
|
||||
export const OPENCODE_RELEASE_URL_TEMPLATE =
|
||||
"https://github.com/sst/opencode/releases/download/v{version}/{asset}";
|
||||
export const OPENCODE_RELEASE_API_URL_TEMPLATE =
|
||||
|
|
|
|||
Loading…
Reference in a new issue