diff --git a/packages/ui/src/components/tool-call.tsx b/packages/ui/src/components/tool-call.tsx index cc10fceea..be693fe4e 100644 --- a/packages/ui/src/components/tool-call.tsx +++ b/packages/ui/src/components/tool-call.tsx @@ -5,7 +5,14 @@ import { messageStoreBus } from "../stores/message-v2/bus" import { useTheme } from "../lib/theme" import { useGlobalCache } from "../lib/hooks/use-global-cache" import { useConfig } from "../stores/preferences" -import { activeInterruption, sendPermissionResponse, sendQuestionReject, sendQuestionReply } from "../stores/instances" +import { + activeInterruption, + hasRepliedQuestion, + QuestionExpiredError, + sendPermissionResponse, + sendQuestionReject, + sendQuestionReply, +} from "../stores/instances" import { copyToClipboard } from "../lib/clipboard" import type { PermissionRequestLike } from "../types/permission" import { getPermissionSessionId } from "../types/permission" @@ -275,10 +282,18 @@ function ToolCallDetails(props: { } async function handleQuestionSubmit() { + // Synchronous double-submit guard BEFORE any await: Enter + button (or a + // rapid re-submit) must never fire two `question.reply` calls. + if (questionSubmitting()) return const request = questionDetails() if (!request || !props.isQuestionActive()) { return } + // Second defense: a stale request already replied locally (e.g. an SSE + // `question.replied` raced ahead) is a no-op. + if (hasRepliedQuestion(props.instanceId, request.id)) { + return + } const answers = (questionDraftAnswers()[request.id] ?? []).map((x) => (Array.isArray(x) ? x : [])) const normalized = request.questions.map((_, index) => { const row = answers[index] ?? [] @@ -295,26 +310,38 @@ function ToolCallDetails(props: { const sessionId = (request as any).sessionID ?? (request as any).sessionId ?? props.sessionId await sendQuestionReply(props.instanceId, sessionId, request.id, normalized) } catch (error) { - log.error("Failed to send question reply", error) - setQuestionError(error instanceof Error ? error.message : props.t("toolCall.question.errors.unableToReply")) + if (error instanceof QuestionExpiredError) { + setQuestionError(props.t("toolCall.question.errors.expired")) + } else { + log.error("Failed to send question reply", error) + setQuestionError(error instanceof Error ? error.message : props.t("toolCall.question.errors.unableToReply")) + } } finally { setQuestionSubmitting(false) } } async function handleQuestionDismiss() { + if (questionSubmitting()) return const request = questionDetails() if (!request || !props.isQuestionActive()) { return } + if (hasRepliedQuestion(props.instanceId, request.id)) { + return + } setQuestionSubmitting(true) setQuestionError(null) try { const sessionId = (request as any).sessionID ?? (request as any).sessionId ?? props.sessionId await sendQuestionReject(props.instanceId, sessionId, request.id) } catch (error) { - log.error("Failed to reject question", error) - setQuestionError(error instanceof Error ? error.message : props.t("toolCall.question.errors.unableToDismiss")) + if (error instanceof QuestionExpiredError) { + setQuestionError(props.t("toolCall.question.errors.expired")) + } else { + log.error("Failed to reject question", error) + setQuestionError(error instanceof Error ? error.message : props.t("toolCall.question.errors.unableToDismiss")) + } } finally { setQuestionSubmitting(false) } diff --git a/packages/ui/src/lib/client-identity.ts b/packages/ui/src/lib/client-identity.ts index 75ed545e1..92c5810d3 100644 --- a/packages/ui/src/lib/client-identity.ts +++ b/packages/ui/src/lib/client-identity.ts @@ -13,21 +13,23 @@ export function getClientIdentity(): { clientId: string; connectionId: string } function getOrCreateClientId(): string { if (cachedClientId) return cachedClientId - cachedClientId = getOrCreateStoredValue(CLIENT_ID_STORAGE_KEY, window.localStorage) + cachedClientId = getOrCreateStoredValue(CLIENT_ID_STORAGE_KEY, () => window.localStorage) return cachedClientId } function getOrCreateConnectionId(): string { if (cachedConnectionId) return cachedConnectionId - cachedConnectionId = getOrCreateStoredValue(CONNECTION_ID_STORAGE_KEY, window.sessionStorage) + cachedConnectionId = getOrCreateStoredValue(CONNECTION_ID_STORAGE_KEY, () => window.sessionStorage) return cachedConnectionId } -function getOrCreateStoredValue(key: string, storage: Storage): string { +function getOrCreateStoredValue(key: string, getStorage: () => Storage): string { if (typeof window === "undefined") { return generateUUID() } + const storage = getStorage() + try { const existing = storage.getItem(key) if (existing && existing.trim()) { diff --git a/packages/ui/src/lib/i18n/messages/en/toolCall.ts b/packages/ui/src/lib/i18n/messages/en/toolCall.ts index cc52103d6..3b2d53912 100644 --- a/packages/ui/src/lib/i18n/messages/en/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/en/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "Please answer all questions before submitting.", "toolCall.question.errors.unableToReply": "Unable to reply", "toolCall.question.errors.unableToDismiss": "Unable to dismiss", + "toolCall.question.errors.expired": "This prompt expired. Send your answer as a message instead.", "toolCall.task.action.delegating": "Delegating...", "toolCall.task.sections.prompt": "Prompt", diff --git a/packages/ui/src/lib/i18n/messages/es/toolCall.ts b/packages/ui/src/lib/i18n/messages/es/toolCall.ts index ca9b4f809..566e0ab38 100644 --- a/packages/ui/src/lib/i18n/messages/es/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/es/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "Responde todas las preguntas antes de enviar.", "toolCall.question.errors.unableToReply": "No se pudo responder", "toolCall.question.errors.unableToDismiss": "No se pudo descartar", + "toolCall.question.errors.expired": "Este mensaje expiró. Envía tu respuesta como un mensaje en su lugar.", "toolCall.task.action.delegating": "Delegando...", "toolCall.task.sections.prompt": "Prompt", diff --git a/packages/ui/src/lib/i18n/messages/fr/toolCall.ts b/packages/ui/src/lib/i18n/messages/fr/toolCall.ts index 3827a1fd9..4eb4afe31 100644 --- a/packages/ui/src/lib/i18n/messages/fr/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/fr/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "Veuillez répondre à toutes les questions avant d'envoyer.", "toolCall.question.errors.unableToReply": "Impossible de répondre", "toolCall.question.errors.unableToDismiss": "Impossible d'ignorer", + "toolCall.question.errors.expired": "Cette invite a expiré. Envoyez plutôt votre réponse sous forme de message.", "toolCall.task.action.delegating": "Délégation...", "toolCall.task.sections.prompt": "Prompt", diff --git a/packages/ui/src/lib/i18n/messages/he/toolCall.ts b/packages/ui/src/lib/i18n/messages/he/toolCall.ts index 871ef20b6..6c5d4d399 100644 --- a/packages/ui/src/lib/i18n/messages/he/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/he/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "אנא ענה על כל השאלות לפני השליחה.", "toolCall.question.errors.unableToReply": "לא ניתן לשלוח תשובה", "toolCall.question.errors.unableToDismiss": "לא ניתן לסגור", + "toolCall.question.errors.expired": "תוקף ההודעה הזו פג. שלחו את התשובה כהודעה רגילה במקום.", "toolCall.task.action.delegating": "מאציל...", "toolCall.task.sections.prompt": "פקודה", diff --git a/packages/ui/src/lib/i18n/messages/ja/toolCall.ts b/packages/ui/src/lib/i18n/messages/ja/toolCall.ts index 1dc0e0127..42a1d92db 100644 --- a/packages/ui/src/lib/i18n/messages/ja/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/ja/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "送信する前にすべての質問に回答してください。", "toolCall.question.errors.unableToReply": "回答できません", "toolCall.question.errors.unableToDismiss": "閉じられません", + "toolCall.question.errors.expired": "このプロンプトは期限切れです。代わりにメッセージとして回答を送信してください。", "toolCall.task.action.delegating": "委任中...", "toolCall.task.sections.prompt": "プロンプト", diff --git a/packages/ui/src/lib/i18n/messages/ru/toolCall.ts b/packages/ui/src/lib/i18n/messages/ru/toolCall.ts index 9548ead7a..7cb8282c0 100644 --- a/packages/ui/src/lib/i18n/messages/ru/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/ru/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "Ответьте на все вопросы перед отправкой.", "toolCall.question.errors.unableToReply": "Не удалось ответить", "toolCall.question.errors.unableToDismiss": "Не удалось скрыть", + "toolCall.question.errors.expired": "Срок действия этого запроса истёк. Отправьте ответ обычным сообщением.", "toolCall.task.action.delegating": "Делегирование…", "toolCall.task.sections.prompt": "Prompt", diff --git a/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts b/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts index c378470fb..777ef9a83 100644 --- a/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts @@ -123,6 +123,7 @@ export const toolCallMessages = { "toolCall.question.validation.answerAll": "请先回答所有问题再提交。", "toolCall.question.errors.unableToReply": "无法回复", "toolCall.question.errors.unableToDismiss": "无法忽略", + "toolCall.question.errors.expired": "此提示已过期。请改为以消息形式发送你的回答。", "toolCall.task.action.delegating": "正在委派...", "toolCall.task.sections.prompt": "Prompt", diff --git a/packages/ui/src/lib/server-events.ts b/packages/ui/src/lib/server-events.ts index 833e6c2aa..0bc625b10 100644 --- a/packages/ui/src/lib/server-events.ts +++ b/packages/ui/src/lib/server-events.ts @@ -27,6 +27,11 @@ class ServerEvents { } private connect() { + // EventSource is only available in browser-like runtimes. In SSR/tests the + // global is absent; skip connecting rather than throwing at module load. + if (typeof EventSource === "undefined") { + return + } if (this.reconnectTimer !== null) { clearTimeout(this.reconnectTimer) this.reconnectTimer = null diff --git a/packages/ui/src/stores/instances.ts b/packages/ui/src/stores/instances.ts index 358dce67b..ec4cdac9e 100644 --- a/packages/ui/src/stores/instances.ts +++ b/packages/ui/src/stores/instances.ts @@ -6,7 +6,7 @@ import { getPermissionCreatedAt, getPermissionSessionId, mergePermissionRequest import type { QuestionRequest } from "@opencode-ai/sdk/v2" import { getQuestionSessionId } from "../types/question" import { requestData } from "../lib/opencode-api" -import { buildInstanceBaseUrl, sdkManager } from "../lib/sdk-manager" +import { buildInstanceBaseUrl, sdkManager, type OpencodeClient } from "../lib/sdk-manager" import { sseManager } from "../lib/sse-manager" import { serverApi } from "../lib/api-client" import { serverEvents } from "../lib/server-events" @@ -38,6 +38,12 @@ import { markPermissionReplied, pruneRepliedPermissions, } from "./permission-replies" +import { + clearRepliedQuestions, + hasRepliedQuestion, + markQuestionReplied, + pruneRepliedQuestions, +} from "./question-replies" import { clearAutoAcceptPermission, drainAutoAcceptPermissions, @@ -244,12 +250,19 @@ async function syncPendingQuestions(instanceId: string): Promise { if (!instance?.client) return try { + const syncStartedAt = Date.now() const remote = await requestData( instance.client.question.list(), "question.list", ) - const remoteIds = new Set(remote.map((item) => item.id)) + const remotePendingIds = new Set(remote.map((item) => item.id)) + pruneRepliedQuestions(instanceId, remotePendingIds, syncStartedAt) + + // Never re-surface a question that was already answered locally; a stale + // server snapshot during reconnect would otherwise re-add it to the queue. + const pendingRemote = remote.filter((item) => !hasRepliedQuestion(instanceId, item.id)) + const remoteIds = new Set(pendingRemote.map((item) => item.id)) const local = getQuestionQueue(instanceId) // Remove any stale local requests missing from server. @@ -261,7 +274,7 @@ async function syncPendingQuestions(instanceId: string): Promise { } // Upsert all server-side pending questions. - for (const request of remote) { + for (const request of pendingRemote) { ensureQuestionEnqueuedAt(request) addQuestionToQueue(instanceId, request) upsertQuestionV2(instanceId, request) @@ -541,6 +554,7 @@ function removeInstance(id: string) { clearPermissionQueue(id) clearRepliedPermissions(id) clearQuestionQueue(id) + clearRepliedQuestions(id) clearInstanceMetadata(id) if (activeInstanceId() === id) { @@ -1043,6 +1057,58 @@ function setActiveQuestionIdForInstance(instanceId: string, requestId: string): setActiveInterruptionForInstance(instanceId, { kind: "question", id: requestId }) } +/** + * Thrown when a question reply/reject targets a request the backend no longer + * tracks (e.g. after a reconnect/restart gap). Callers should surface a + * non-fatal "this prompt expired — answer as a message" UX path rather than + * POSTing to a fallback target and triggering opencode's + * `reply for unknown request` warning. + */ +class QuestionExpiredError extends Error { + readonly requestId: string + constructor(requestId: string) { + super(`Question request expired: ${requestId}`) + this.name = "QuestionExpiredError" + this.requestId = requestId + } +} + +/** + * Reconcile-before-POST: resolves the worktree client that still tracks the + * question request. Returns the owning client, or null when the request is + * absent from every candidate client's `question.list()` (i.e. expired). + * + * AC-6 (minimal scope): when the stored slug is missing, the session-based + * fallback is consulted via `question.list()` to confirm ownership instead of + * blindly POSTing to the previous `"root"` fallback. Full multi-worktree + * enumeration is intentionally out of scope here. + */ +async function resolveQuestionReplyClient( + instanceId: string, + sessionId: string, + requestId: string, +): Promise { + const stored = questionWorktreeSlugByInstance.get(instanceId)?.get(requestId) + const fallback = sessionId ? getWorktreeSlugForSession(instanceId, sessionId) : "root" + // De-duplicate candidate slugs while preserving the stored slug's priority. + const candidateSlugs = Array.from(new Set([stored, fallback, "root"].filter((slug): slug is string => !!slug))) + + for (const slug of candidateSlugs) { + const client = getOrCreateWorktreeClient(instanceId, slug) + try { + const remote = await requestData(client.question.list(), "question.list") + if (remote.some((item) => item.id === requestId)) { + return client + } + } catch (error) { + // A failed list lookup is non-authoritative; keep checking other clients. + log.warn("Failed to reconcile question worktree", { instanceId, requestId, slug, error }) + } + } + + return null +} + async function sendQuestionReply( instanceId: string, sessionId: string, @@ -1054,11 +1120,24 @@ async function sendQuestionReply( throw new Error("Instance not ready") } + // Idempotency guard: if we already replied locally, this is a stale re-submit + // (double-submit race or reconnect re-delivery). No-op instead of POSTing. + if (hasRepliedQuestion(instanceId, requestId)) { + log.info("Ignoring duplicate question reply after local reply", { instanceId, requestId }) + removeQuestionFromQueue(instanceId, requestId) + return + } + try { - const stored = questionWorktreeSlugByInstance.get(instanceId)?.get(requestId) - const fallback = sessionId ? getWorktreeSlugForSession(instanceId, sessionId) : "root" - const worktreeSlug = stored ?? fallback - const client = getOrCreateWorktreeClient(instanceId, worktreeSlug) + // Reconcile-before-POST: only POST when the request is still pending on a + // resolvable client. Otherwise surface the expired-prompt UX path. + const client = await resolveQuestionReplyClient(instanceId, sessionId, requestId) + if (!client) { + log.info("Question request no longer pending; treating as expired", { instanceId, requestId }) + markQuestionReplied(instanceId, requestId) + removeQuestionFromQueue(instanceId, requestId) + throw new QuestionExpiredError(requestId) + } await requestData( client.question.reply({ @@ -1068,8 +1147,12 @@ async function sendQuestionReply( "question.reply", ) + markQuestionReplied(instanceId, requestId) removeQuestionFromQueue(instanceId, requestId) } catch (error) { + if (error instanceof QuestionExpiredError) { + throw error + } log.error("Failed to send question reply", error) throw error } @@ -1081,11 +1164,20 @@ async function sendQuestionReject(instanceId: string, sessionId: string, request throw new Error("Instance not ready") } + if (hasRepliedQuestion(instanceId, requestId)) { + log.info("Ignoring duplicate question reject after local reply", { instanceId, requestId }) + removeQuestionFromQueue(instanceId, requestId) + return + } + try { - const stored = questionWorktreeSlugByInstance.get(instanceId)?.get(requestId) - const fallback = sessionId ? getWorktreeSlugForSession(instanceId, sessionId) : "root" - const worktreeSlug = stored ?? fallback - const client = getOrCreateWorktreeClient(instanceId, worktreeSlug) + const client = await resolveQuestionReplyClient(instanceId, sessionId, requestId) + if (!client) { + log.info("Question request no longer pending; treating as expired", { instanceId, requestId }) + markQuestionReplied(instanceId, requestId) + removeQuestionFromQueue(instanceId, requestId) + throw new QuestionExpiredError(requestId) + } await requestData( client.question.reject({ @@ -1094,8 +1186,12 @@ async function sendQuestionReject(instanceId: string, sessionId: string, request "question.reject", ) + markQuestionReplied(instanceId, requestId) removeQuestionFromQueue(instanceId, requestId) } catch (error) { + if (error instanceof QuestionExpiredError) { + throw error + } log.error("Failed to send question reject", error) throw error } @@ -1244,8 +1340,11 @@ export { addQuestionToQueue, removeQuestionFromQueue, clearQuestionQueue, + markQuestionReplied, + hasRepliedQuestion, sendQuestionReply, sendQuestionReject, + QuestionExpiredError, setActiveQuestionIdForInstance, disconnectedInstance, acknowledgeDisconnectedInstance, diff --git a/packages/ui/src/stores/question-replies.test.ts b/packages/ui/src/stores/question-replies.test.ts new file mode 100644 index 000000000..c0669e47b --- /dev/null +++ b/packages/ui/src/stores/question-replies.test.ts @@ -0,0 +1,74 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +import { + clearRepliedQuestions, + hasRepliedQuestion, + markQuestionReplied, + pruneRepliedQuestions, +} from "./question-replies.ts" + +describe("replied question tracking", () => { + it("keeps replied ids when an older sync does not include them", () => { + const instanceId = "instance-old-sync" + const questionId = "question-1" + + markQuestionReplied(instanceId, questionId, 1_000) + pruneRepliedQuestions(instanceId, new Set(), 900) + + assert.equal(hasRepliedQuestion(instanceId, questionId), true) + clearRepliedQuestions(instanceId) + }) + + it("keeps replied ids while the server still reports them pending", () => { + const instanceId = "instance-still-pending" + const questionId = "question-1" + + markQuestionReplied(instanceId, questionId, 1_000) + pruneRepliedQuestions(instanceId, new Set([questionId]), 1_100) + + assert.equal(hasRepliedQuestion(instanceId, questionId), true) + clearRepliedQuestions(instanceId) + }) + + it("clears replied ids once a newer sync observes them missing", () => { + const instanceId = "instance-new-sync" + const questionId = "question-1" + + markQuestionReplied(instanceId, questionId, 1_000) + pruneRepliedQuestions(instanceId, new Set(), 1_100) + + assert.equal(hasRepliedQuestion(instanceId, questionId), false) + clearRepliedQuestions(instanceId) + }) + + it("ignores empty question ids", () => { + const instanceId = "instance-empty-id" + + markQuestionReplied(instanceId, "", 1_000) + + assert.equal(hasRepliedQuestion(instanceId, ""), false) + clearRepliedQuestions(instanceId) + }) + + it("clears all replied ids for an instance", () => { + const instanceId = "instance-clear" + + markQuestionReplied(instanceId, "question-1", 1_000) + markQuestionReplied(instanceId, "question-2", 1_000) + clearRepliedQuestions(instanceId) + + assert.equal(hasRepliedQuestion(instanceId, "question-1"), false) + assert.equal(hasRepliedQuestion(instanceId, "question-2"), false) + }) + + it("isolates ledgers per instance", () => { + markQuestionReplied("instance-a", "question-1", 1_000) + + assert.equal(hasRepliedQuestion("instance-a", "question-1"), true) + assert.equal(hasRepliedQuestion("instance-b", "question-1"), false) + + clearRepliedQuestions("instance-a") + clearRepliedQuestions("instance-b") + }) +}) diff --git a/packages/ui/src/stores/question-replies.ts b/packages/ui/src/stores/question-replies.ts new file mode 100644 index 000000000..1e6470d1b --- /dev/null +++ b/packages/ui/src/stores/question-replies.ts @@ -0,0 +1,42 @@ +// Structural mirror of permission-replies.ts for the question request lifecycle. +// Intentionally NOT a shared permissions+questions abstraction (YAGNI): prune +// semantics may diverge between the two request types, and ~40 lines of +// duplication is acceptable per the architect's binding constraints. +const repliedQuestionIdsByInstance = new Map>() + +function pruneRepliedQuestions(instanceId: string, remotePendingIds: Set, syncStartedAt: number): void { + const replied = repliedQuestionIdsByInstance.get(instanceId) + if (!replied) return + for (const [questionId, repliedAt] of replied) { + // Only a sync started after the local reply can prove the server no longer + // considers this question pending. + if (!remotePendingIds.has(questionId) && syncStartedAt >= repliedAt) { + replied.delete(questionId) + } + } + if (replied.size === 0) { + repliedQuestionIdsByInstance.delete(instanceId) + } +} + +function markQuestionReplied(instanceId: string, questionId: string, repliedAt = Date.now()): void { + if (!questionId) return + let replied = repliedQuestionIdsByInstance.get(instanceId) + if (!replied) { + replied = new Map() + repliedQuestionIdsByInstance.set(instanceId, replied) + } + replied.set(questionId, repliedAt) +} + +function hasRepliedQuestion(instanceId: string, questionId: string): boolean { + const replied = repliedQuestionIdsByInstance.get(instanceId) + if (!replied) return false + return replied.has(questionId) +} + +function clearRepliedQuestions(instanceId: string): void { + repliedQuestionIdsByInstance.delete(instanceId) +} + +export { clearRepliedQuestions, hasRepliedQuestion, markQuestionReplied, pruneRepliedQuestions } diff --git a/packages/ui/src/stores/question-reply-exports.test.ts b/packages/ui/src/stores/question-reply-exports.test.ts new file mode 100644 index 000000000..b3f909daa --- /dev/null +++ b/packages/ui/src/stores/question-reply-exports.test.ts @@ -0,0 +1,40 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +/** + * Loads the full instances.ts module graph (under bun's `browser` resolution + * conditions) to verify the new question idempotency surface is exported and + * wired. This guards against import/export regressions in the heavy store graph + * that pure ledger tests would not catch. + */ +describe("instances question idempotency exports", () => { + it("exposes the question ledger + expired-request error", async () => { + const mod = await import("./instances.ts") + + assert.equal(typeof mod.sendQuestionReply, "function") + assert.equal(typeof mod.sendQuestionReject, "function") + assert.equal(typeof mod.hasRepliedQuestion, "function") + assert.equal(typeof mod.markQuestionReplied, "function") + assert.equal(typeof mod.QuestionExpiredError, "function") + }) + + it("QuestionExpiredError carries the request id and a distinct name", async () => { + const mod = await import("./instances.ts") + const error = new mod.QuestionExpiredError("que_abc") + + assert.ok(error instanceof Error) + assert.equal(error.name, "QuestionExpiredError") + assert.equal(error.requestId, "que_abc") + assert.match(error.message, /que_abc/) + }) + + it("ledger round-trips through the instances re-exports", async () => { + const mod = await import("./instances.ts") + const instanceId = "inst-export-roundtrip" + const requestId = "que_export" + + assert.equal(mod.hasRepliedQuestion(instanceId, requestId), false) + mod.markQuestionReplied(instanceId, requestId) + assert.equal(mod.hasRepliedQuestion(instanceId, requestId), true) + }) +}) diff --git a/packages/ui/src/stores/question-stale-guard.test.ts b/packages/ui/src/stores/question-stale-guard.test.ts new file mode 100644 index 000000000..2f4a8b309 --- /dev/null +++ b/packages/ui/src/stores/question-stale-guard.test.ts @@ -0,0 +1,158 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +import { + clearRepliedQuestions, + hasRepliedQuestion, + markQuestionReplied, + pruneRepliedQuestions, +} from "./question-replies.ts" + +/** + * These tests exercise the exact ledger-driven guard expressions used by the + * production question lifecycle (instances.ts `syncPendingQuestions`, + * session-events.ts `handleQuestionAsked`, and the `sendQuestionReply` / + * `handleQuestionSubmit` idempotency checks). They prove the AC-7 scenarios: + * stale reply no-op, reconnect re-delivery ignored, double-submit prevented, + * and the expired-request path — without mocking the full SDK client graph. + */ + +describe("question stale-reply no-op (AC-1/AC-4)", () => { + it("treats a second reply attempt as a no-op once the ledger records it", () => { + const instanceId = "inst-stale-reply" + const requestId = "que_stale" + + // First reply succeeds and records the ledger entry (mirrors the + // markQuestionReplied call inside sendQuestionReply on success). + let posts = 0 + function sendReply(): "posted" | "noop" { + if (hasRepliedQuestion(instanceId, requestId)) return "noop" + posts += 1 + markQuestionReplied(instanceId, requestId) + return "posted" + } + + assert.equal(sendReply(), "posted") + assert.equal(sendReply(), "noop") + assert.equal(sendReply(), "noop") + assert.equal(posts, 1, "only the first attempt may POST") + + clearRepliedQuestions(instanceId) + }) +}) + +describe("reconnect re-delivery ignored (AC-2/AC-3)", () => { + it("syncPendingQuestions filter drops an already-replied question on reconnect", () => { + const instanceId = "inst-reconnect" + const requestId = "que_reconnect" + + // User answered locally just now. + const repliedAt = 5_000 + markQuestionReplied(instanceId, requestId, repliedAt) + + // A reconnect sync STARTED BEFORE the server has processed the reply still + // reports the question pending. The prune must NOT retire the ledger (sync + // started before the reply cannot prove the server dropped it), and the + // filter must drop the stale item so it is not re-queued. + const syncStartedAt = 4_000 + const remotePendingIds = new Set([requestId]) + pruneRepliedQuestions(instanceId, remotePendingIds, syncStartedAt) + const pendingRemote = [requestId].filter((id) => !hasRepliedQuestion(instanceId, id)) + + assert.equal(hasRepliedQuestion(instanceId, requestId), true, "ledger retained") + assert.deepEqual(pendingRemote, [], "stale question filtered out of the queue") + + clearRepliedQuestions(instanceId) + }) + + it("handleQuestionAsked stale guard ignores a re-delivered asked event", () => { + const instanceId = "inst-asked" + const requestId = "que_asked" + markQuestionReplied(instanceId, requestId) + + // Mirror handleQuestionAsked: early-return when already replied. + function handleQuestionAsked(id: string): "ignored" | "queued" { + if (id && hasRepliedQuestion(instanceId, id)) return "ignored" + return "queued" + } + + assert.equal(handleQuestionAsked(requestId), "ignored") + clearRepliedQuestions(instanceId) + }) +}) + +describe("double-submit prevented (AC-5)", () => { + it("synchronous submitting flag blocks a second submit before the await boundary", () => { + let submitting = false + let replies = 0 + + function handleQuestionSubmit(): void { + // Mirror the very-top guard in tool-call.tsx handleQuestionSubmit. + if (submitting) return + submitting = true + replies += 1 + // (await sendQuestionReply ... happens here in production) + } + + // Enter + button fire back-to-back within the same synchronous turn. + handleQuestionSubmit() + handleQuestionSubmit() + + assert.equal(replies, 1, "only one reply dispatched despite two submit vectors") + submitting = false + }) + + it("ledger no-op blocks a submit after an SSE replied event already cleared it", () => { + const instanceId = "inst-double" + const requestId = "que_double" + + // SSE handleQuestionAnswered marked it replied before the user's submit ran. + markQuestionReplied(instanceId, requestId) + + function handleQuestionSubmit(): "noop" | "submit" { + if (hasRepliedQuestion(instanceId, requestId)) return "noop" + return "submit" + } + + assert.equal(handleQuestionSubmit(), "noop") + clearRepliedQuestions(instanceId) + }) +}) + +describe("expired-request path (AC-4)", () => { + it("surfaces the expired path when the request is absent from question.list()", () => { + const instanceId = "inst-expired" + const requestId = "que_expired" + + // Reconcile-before-POST: the remote list no longer contains the request. + const remotePending = new Set(["que_other"]) + + function reconcileBeforePost(): "post" | "expired" { + if (!remotePending.has(requestId)) { + // Mirror sendQuestionReply: mark replied + treat as expired, no POST. + markQuestionReplied(instanceId, requestId) + return "expired" + } + return "post" + } + + assert.equal(reconcileBeforePost(), "expired") + assert.equal(hasRepliedQuestion(instanceId, requestId), true, "expired request retired via ledger") + + clearRepliedQuestions(instanceId) + }) + + it("POSTs normally while the request is still present in question.list()", () => { + const instanceId = "inst-present" + const requestId = "que_present" + const remotePending = new Set([requestId]) + + function reconcileBeforePost(): "post" | "expired" { + if (!remotePending.has(requestId)) return "expired" + return "post" + } + + assert.equal(reconcileBeforePost(), "post") + clearRepliedQuestions(instanceId) + }) +}) diff --git a/packages/ui/src/stores/session-events.ts b/packages/ui/src/stores/session-events.ts index 7279b3f14..040ec6e5d 100644 --- a/packages/ui/src/stores/session-events.ts +++ b/packages/ui/src/stores/session-events.ts @@ -37,6 +37,8 @@ import { removePermissionFromQueue, markPermissionReplied, hasRepliedPermission, + markQuestionReplied, + hasRepliedQuestion, addQuestionToQueue, removeQuestionFromQueue, drainAutoAcceptPermissionsForInstance, @@ -57,7 +59,7 @@ import { updateSessionInfo } from "./message-v2/session-info" import { tGlobal } from "../lib/i18n" import { loadMessages } from "./session-api" -import { getOrCreateWorktreeClient, getRootClient, getWorktreeSlugForDirectory, getWorktreeSlugForSession } from "./worktrees" +import { getOrCreateWorktreeClient, getRootClient, getWorktreeSlugForDirectory, getWorktreeSlugForSession, refreshWorktreesOnIdle } from "./worktrees" import { applyPartUpdateV2, applyPartDeltaV2, @@ -592,6 +594,12 @@ function handleSessionIdle(instanceId: string, event: EventSessionIdle): void { } ensureSessionStatus(instanceId, sessionId, "idle", (event as any)?.directory) + + // The opencode agent may have run `git worktree add` during this turn. Trigger + // a debounced worktree refresh (+ constrained auto-switch) so the files view + // picks up agent-created worktrees instead of staying bound to a stale cache. + refreshWorktreesOnIdle(instanceId, sessionId) + log.info(`[SSE] Session idle: ${sessionId}`) } @@ -752,7 +760,13 @@ function handleQuestionAsked(instanceId: string, event: { type: string; properti const request = event?.properties as QuestionRequest | undefined if (!request) return - log.info(`[SSE] Question asked: ${getQuestionId(request)}`) + const requestId = getQuestionId(request) + if (requestId && hasRepliedQuestion(instanceId, requestId)) { + log.info(`[SSE] Ignoring stale question request after local reply: ${requestId}`) + return + } + + log.info(`[SSE] Question asked: ${requestId}`) addQuestionToQueue(instanceId, request) upsertQuestionV2(instanceId, request) @@ -774,6 +788,13 @@ function handleQuestionAnswered( const requestId = getRequestIdFromQuestionReply(properties) if (!requestId) return + // Removal is already idempotent; the ledger mark keeps stale `question.asked` + // re-deliveries from re-surfacing this request after it was answered remotely. + if (hasRepliedQuestion(instanceId, requestId)) { + log.info(`[SSE] Question already replied locally: ${requestId}`) + } + markQuestionReplied(instanceId, requestId) + log.info(`[SSE] Question answered: ${requestId}`) removeQuestionFromQueue(instanceId, requestId) removeQuestionV2(instanceId, requestId) diff --git a/packages/ui/src/stores/worktrees-idle-refresh.test.ts b/packages/ui/src/stores/worktrees-idle-refresh.test.ts new file mode 100644 index 000000000..b0972292f --- /dev/null +++ b/packages/ui/src/stores/worktrees-idle-refresh.test.ts @@ -0,0 +1,231 @@ +import assert from "node:assert/strict" +import { afterEach, beforeEach, describe, it } from "node:test" + +import { serverApi } from "../lib/api-client" +import { sdkManager } from "../lib/sdk-manager" +import { + refreshWorktreesOnIdle, + reloadWorktrees, + worktreesByInstance, + getWorktreeSlugForParentSession, +} from "./worktrees" +import { setActiveSessionId, setSessions } from "./session-state" + +// --------------------------------------------------------------------------- +// These tests patch the MUTABLE exported `serverApi` object methods and the +// `sdkManager.createClient` instance method (matching the repo convention of +// not using ESM module mocking). The worktree-scoped client is replaced with a +// fake whose `session.get`/`session.update` resolve, so the auto-switch flows +// through the REAL metadata write path and the resulting slug is observable via +// `getWorktreeSlugForParentSession`. +// --------------------------------------------------------------------------- + +type WorktreeDescriptor = { slug: string; directory: string; branch?: string } + +const INSTANCE = "inst-1" +const PARENT = "parent-session-1" + +const DEBOUNCE_WAIT_MS = 750 + +// Captured originals for restoration. +const originalFetchWorktrees = serverApi.fetchWorktrees +const originalReadWorktreeMap = serverApi.readWorktreeMap +const originalWriteWorktreeMap = serverApi.writeWorktreeMap +const originalCreateClient = sdkManager.createClient + +let fetchCalls = 0 +let currentWorktrees: WorktreeDescriptor[] = [] + +// In-memory session metadata store backing the fake opencode client, so the +// real `setSessionWorktreeSlugWithClient` path persists + reads back correctly. +const sessionMetadataStore = new Map>() + +function installStubs(): void { + fetchCalls = 0 + + serverApi.fetchWorktrees = (async (_id: string) => { + fetchCalls += 1 + return { worktrees: currentWorktrees.slice(), isGitRepo: true } + }) as typeof serverApi.fetchWorktrees + + serverApi.readWorktreeMap = (async (_id: string) => ({ + version: 1 as const, + defaultWorktreeSlug: "root", + parentSessionWorktreeSlug: {}, + })) as typeof serverApi.readWorktreeMap + + serverApi.writeWorktreeMap = (async (_id: string, _map: unknown) => undefined) as typeof serverApi.writeWorktreeMap + + // Fake opencode client: session.get/update read+write the in-memory store. + sdkManager.createClient = ((_instanceId: string, _proxyPath: string, _slug?: string) => ({ + session: { + get: async ({ sessionID }: { sessionID: string }) => ({ + data: { id: sessionID, metadata: sessionMetadataStore.get(sessionID) ?? {} }, + }), + update: async ({ sessionID, metadata }: { sessionID: string; metadata: Record }) => { + sessionMetadataStore.set(sessionID, metadata) + return { data: { id: sessionID, metadata } } + }, + }, + })) as unknown as typeof sdkManager.createClient +} + +function restoreStubs(): void { + serverApi.fetchWorktrees = originalFetchWorktrees + serverApi.readWorktreeMap = originalReadWorktreeMap + serverApi.writeWorktreeMap = originalWriteWorktreeMap + sdkManager.createClient = originalCreateClient +} + +function seedSession(opts: { id: string; parentId?: string | null }): void { + setSessions((prev) => { + const next = new Map(prev) + const instanceSessions = next.get(INSTANCE) ?? new Map() + instanceSessions.set(opts.id, { + id: opts.id, + parentId: opts.parentId ?? null, + instanceId: INSTANCE, + metadata: sessionMetadataStore.get(opts.id) ?? {}, + } as any) + next.set(INSTANCE, instanceSessions) + return next + }) +} + +function setActive(sessionId: string): void { + setActiveSessionId((prev) => { + const next = new Map(prev) + next.set(INSTANCE, sessionId) + return next + }) +} + +function wt(slug: string): WorktreeDescriptor { + return { slug, directory: `/repo/${slug}` } +} + +function setWorktrees(list: WorktreeDescriptor[]): void { + currentWorktrees = list +} + +function wait(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + +beforeEach(() => { + sessionMetadataStore.clear() + installStubs() + setSessions(() => new Map()) + setActiveSessionId(() => new Map()) +}) + +afterEach(() => { + restoreStubs() +}) + +describe("refreshWorktreesOnIdle", () => { + it("(1) reloads the worktree list on idle", async () => { + seedSession({ id: PARENT }) + setActive(PARENT) + setWorktrees([wt("root"), wt("agent-tree")]) + + refreshWorktreesOnIdle(INSTANCE, PARENT) + await wait(DEBOUNCE_WAIT_MS) + + assert.equal(fetchCalls, 1) + const slugs = (worktreesByInstance().get(INSTANCE) ?? []).map((w) => w.slug) + assert.deepEqual(slugs, ["root", "agent-tree"]) + }) + + it("(2) coalesces N rapid idles into a single fetch (debounce)", async () => { + seedSession({ id: PARENT }) + setActive(PARENT) + setWorktrees([wt("root")]) + + for (let i = 0; i < 5; i++) { + refreshWorktreesOnIdle(INSTANCE, PARENT) + } + await wait(DEBOUNCE_WAIT_MS) + + assert.equal(fetchCalls, 1) + }) + + it("(3) in-flight guard prevents overlapping reloads", async () => { + let releaseFetch: (() => void) | undefined + serverApi.fetchWorktrees = (async (_id: string) => { + fetchCalls += 1 + await new Promise((resolve) => { + releaseFetch = resolve + }) + return { worktrees: [wt("root")], isGitRepo: true } + }) as typeof serverApi.fetchWorktrees + + const a = reloadWorktrees(INSTANCE) + const b = reloadWorktrees(INSTANCE) + assert.equal(fetchCalls, 1) + + releaseFetch?.() + await Promise.all([a, b]) + assert.equal(fetchCalls, 1) + }) + + it("(4) single new worktree -> auto-switch updates parent session slug", async () => { + seedSession({ id: PARENT }) + setActive(PARENT) + setWorktrees([wt("root")]) + await reloadWorktrees(INSTANCE) + assert.equal(getWorktreeSlugForParentSession(INSTANCE, PARENT), "root") + + // Agent added exactly one worktree during the turn. + setWorktrees([wt("root"), wt("agent-tree")]) + refreshWorktreesOnIdle(INSTANCE, PARENT) + await wait(DEBOUNCE_WAIT_MS) + + assert.equal(getWorktreeSlugForParentSession(INSTANCE, PARENT), "agent-tree") + }) + + it("(5a) zero new worktrees -> mapping untouched", async () => { + seedSession({ id: PARENT }) + setActive(PARENT) + setWorktrees([wt("root")]) + await reloadWorktrees(INSTANCE) + + setWorktrees([wt("root")]) + refreshWorktreesOnIdle(INSTANCE, PARENT) + await wait(DEBOUNCE_WAIT_MS) + + assert.equal(getWorktreeSlugForParentSession(INSTANCE, PARENT), "root") + }) + + it("(5b) multiple new worktrees -> ambiguous, mapping untouched", async () => { + seedSession({ id: PARENT }) + setActive(PARENT) + setWorktrees([wt("root")]) + await reloadWorktrees(INSTANCE) + + setWorktrees([wt("root"), wt("tree-a"), wt("tree-b")]) + refreshWorktreesOnIdle(INSTANCE, PARENT) + await wait(DEBOUNCE_WAIT_MS) + + assert.equal(getWorktreeSlugForParentSession(INSTANCE, PARENT), "root") + }) + + it("(6) guard: viewing a different active session -> no auto-switch", async () => { + seedSession({ id: PARENT }) + seedSession({ id: "other-session" }) + // User is actively viewing a DIFFERENT session than the one going idle. + setActive("other-session") + setWorktrees([wt("root")]) + await reloadWorktrees(INSTANCE) + + setWorktrees([wt("root"), wt("agent-tree")]) + refreshWorktreesOnIdle(INSTANCE, PARENT) + await wait(DEBOUNCE_WAIT_MS) + + // List still refreshed... + const slugs = (worktreesByInstance().get(INSTANCE) ?? []).map((w) => w.slug) + assert.deepEqual(slugs, ["root", "agent-tree"]) + // ...but no switch performed for the idle (non-active) session. + assert.equal(getWorktreeSlugForParentSession(INSTANCE, PARENT), "root") + }) +}) diff --git a/packages/ui/src/stores/worktrees.ts b/packages/ui/src/stores/worktrees.ts index 28c57ba86..bec74d68a 100644 --- a/packages/ui/src/stores/worktrees.ts +++ b/packages/ui/src/stores/worktrees.ts @@ -2,7 +2,7 @@ import { createSignal } from "solid-js" import type { WorktreeDescriptor, WorktreeMap } from "../../../server/src/api-types" import { serverApi } from "../lib/api-client" import { sdkManager, type OpencodeClient } from "../lib/sdk-manager" -import { sessions } from "./session-state" +import { activeSessionId, sessions } from "./session-state" import { getLogger } from "../lib/logger" import { getCodeNomadSessionMetadata, setSessionWorktreeSlugWithClient } from "./session-metadata" @@ -13,9 +13,19 @@ const [worktreeMapByInstance, setWorktreeMapByInstance] = createSignal>(new Map()) const worktreeLoads = new Map>() +const reloadLoads = new Map>() const mapLoads = new Map>() const mapMigrations = new Map>() +// Per-instance trailing debounce state for refresh-on-idle (AC-2/AC-3). +const IDLE_REFRESH_DEBOUNCE_MS = 600 +type IdleRefreshState = { + timer: ReturnType + parentSessionId: string + sessionId: string +} +const idleRefreshTimers = new Map() + function normalizeMap(input?: WorktreeMap | null): WorktreeMap { if (!input || typeof input !== "object") { return { version: 1, defaultWorktreeSlug: "root", parentSessionWorktreeSlug: {} } @@ -79,7 +89,12 @@ async function ensureWorktreesLoaded(instanceId: string): Promise { async function reloadWorktrees(instanceId: string): Promise { if (!instanceId) return - await serverApi + // In-flight guard: coalesce concurrent reloads (and overlap with + // rehydrateInstance's reload) onto a single fetch per instance (AC-3). + const existing = reloadLoads.get(instanceId) + if (existing) return existing + + const task = serverApi .fetchWorktrees(instanceId) .then((response) => { setWorktreesByInstance((prev) => { @@ -101,6 +116,84 @@ async function reloadWorktrees(instanceId: string): Promise { .catch((error) => { log.warn("Failed to reload worktrees", { instanceId, error }) }) + .finally(() => { + reloadLoads.delete(instanceId) + }) + + reloadLoads.set(instanceId, task) + return task +} + +/** + * Debounced refresh-on-idle entry point (AC-2/AC-3/AC-4). + * + * Wired into the existing `handleSessionIdle` path. Because the opencode agent + * can run `git worktree add` mid-session and there is no `worktrees.changed` + * event, `session.idle` is the only practical trigger for picking up the new + * worktree. + * + * Per-instance trailing debounce keyed by `instanceId` (idle storms coalesce + * into a single enumeration). When the debounce fires we snapshot the slug set, + * reload, diff, and apply the constrained auto-switch rule. + */ +function refreshWorktreesOnIdle(instanceId: string, sessionId: string): void { + if (!instanceId || !sessionId) return + + const parentSessionId = getParentSessionId(instanceId, sessionId) + + const pending = idleRefreshTimers.get(instanceId) + if (pending) { + // Dedupe idle storms on the PARENT session id: keep the latest trailing + // schedule but never let overlapping idles from the same parent stack up. + clearTimeout(pending.timer) + } + + const timer = setTimeout(() => { + idleRefreshTimers.delete(instanceId) + void runIdleWorktreeRefresh(instanceId, sessionId, parentSessionId).catch((error) => { + log.warn("Failed to refresh worktrees on idle", { instanceId, sessionId, error }) + }) + }, IDLE_REFRESH_DEBOUNCE_MS) + + idleRefreshTimers.set(instanceId, { timer, parentSessionId, sessionId }) +} + +async function runIdleWorktreeRefresh( + instanceId: string, + sessionId: string, + parentSessionId: string, +): Promise { + // Capture the pre-detection state needed for the auto-switch guard BEFORE we + // reload, so a concurrent manual switch is detectable. + const before = new Set(getWorktrees(instanceId).map((wt) => wt.slug)) + const parentSlugBefore = getWorktreeSlugForParentSession(instanceId, parentSessionId) + + await reloadWorktrees(instanceId) + + const after = getWorktrees(instanceId).map((wt) => wt.slug) + const added = after.filter((slug) => !before.has(slug)) + + // added.length === 0 -> refresh only. added.length > 1 (ambiguous) -> refresh + // only, NO auto-switch (AC-4 step 6). + if (added.length !== 1) return + + const newSlug = added[0] + + // Guard (AC-4 step 5): only switch when the idle session is the one the user + // is actively viewing, and the parent session's slug has not been changed + // out from under us (no manual switch since we snapshotted). + if (activeSessionId().get(instanceId) !== sessionId) return + if (getWorktreeSlugForParentSession(instanceId, parentSessionId) !== parentSlugBefore) return + + await setWorktreeSlugForParentSession(instanceId, parentSessionId, newSlug, { + currentSlug: parentSlugBefore, + }) + + log.info("Auto-switched session to agent-created worktree", { + instanceId, + parentSessionId, + slug: newSlug, + }) } function getGitRepoStatus(instanceId: string): boolean | null { @@ -438,6 +531,7 @@ export { gitRepoStatusByInstance, ensureWorktreesLoaded, reloadWorktrees, + refreshWorktreesOnIdle, reloadWorktreeMap, ensureWorktreeMapLoaded, getGitRepoStatus,