From f7618722fbc94518371e09e8f2f6af5d008579c9 Mon Sep 17 00:00:00 2001 From: Omer Date: Mon, 1 Jun 2026 11:01:12 +0000 Subject: [PATCH 1/2] fix: 058 port permission idempotency model to question replies Eliminate opencode's "reply for unknown request" warning caused by orphaned/stale question replies in the CodeNomad UI. Previously the question request lifecycle lacked the idempotency and stale-guard layer that permissions already had, so a question popup answered after a reconnect, restart, or rapid double-submit could POST a reply for a request the backend no longer tracked. User-visible behavior change: - Answering a question prompt that has already expired no longer fails silently or mis-routes; the UI now surfaces a localized "this prompt expired, send your answer as a message instead" notice (new i18n key toolCall.question.errors.expired added to all 7 locales). - Rapid double-submit (Enter + button) can no longer fire two replies. - Reconnect/rehydrate no longer re-surfaces an already-answered question. Implementation: - New stores/question-replies.ts ledger (markQuestionReplied / hasRepliedQuestion / pruneRepliedQuestions / clearRepliedQuestions), a structural mirror of permission-replies.ts (no shared abstraction, per YAGNI; prune semantics may diverge). - sendQuestionReply/sendQuestionReject reconcile against question.list() before POSTing (no error-string parsing); on absent requestID they take the expired-prompt path instead of POSTing to a "root" fallback, and resolve the owning worktree when the stored slug is missing. markQuestionReplied is recorded on success before removeQuestionFromQueue. - syncPendingQuestions adopts the full timestamp-based prune+filter pattern so replied questions are never rehydrated. - The question ledger is cleared only on instance removal, never on rehydrate, so in-flight stale replies cannot slip through. - session-events stale guards on handleQuestionAsked/handleQuestionAnswered. - tool-call.tsx synchronous double-submit guard before the await boundary. Also fixes two pre-existing module-load crashes uncovered while making the UI test suite green (ownership policy): client-identity.ts dereferenced window storage before its typeof-window guard, and server-events.ts opened an EventSource at module load without an EventSource guard. Both fixes are minimal and production-safe. Validation: UI suite 49/49 pass (bun test --conditions browser), UI typecheck clean (tsc --noEmit). Evidence packet under evidences/058-question-reply-idempotency/. --- packages/ui/src/components/tool-call.tsx | 37 +++- packages/ui/src/lib/client-identity.ts | 8 +- .../ui/src/lib/i18n/messages/en/toolCall.ts | 1 + .../ui/src/lib/i18n/messages/es/toolCall.ts | 1 + .../ui/src/lib/i18n/messages/fr/toolCall.ts | 1 + .../ui/src/lib/i18n/messages/he/toolCall.ts | 1 + .../ui/src/lib/i18n/messages/ja/toolCall.ts | 1 + .../ui/src/lib/i18n/messages/ru/toolCall.ts | 1 + .../src/lib/i18n/messages/zh-Hans/toolCall.ts | 1 + packages/ui/src/lib/server-events.ts | 5 + packages/ui/src/stores/instances.ts | 121 ++++++++++++-- .../ui/src/stores/question-replies.test.ts | 74 ++++++++ packages/ui/src/stores/question-replies.ts | 42 +++++ .../src/stores/question-reply-exports.test.ts | 40 +++++ .../src/stores/question-stale-guard.test.ts | 158 ++++++++++++++++++ packages/ui/src/stores/session-events.ts | 17 +- 16 files changed, 489 insertions(+), 20 deletions(-) create mode 100644 packages/ui/src/stores/question-replies.test.ts create mode 100644 packages/ui/src/stores/question-replies.ts create mode 100644 packages/ui/src/stores/question-reply-exports.test.ts create mode 100644 packages/ui/src/stores/question-stale-guard.test.ts 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..c19df28f2 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, @@ -752,7 +754,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 +782,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) From d0c0a7225f91a08acd0fc1c439088509f19ce5bf Mon Sep 17 00:00:00 2001 From: Omer Date: Mon, 1 Jun 2026 12:30:47 +0000 Subject: [PATCH 2/2] fix(ui): refresh + auto-switch worktrees on session idle The opencode agent can run `git worktree add` mid-session. CodeNomad cached its worktree list with no invalidation trigger for out-of-band git changes, so the files view stayed bound to a stale list and the session remained mapped to its old worktree slug. Wire a debounced refresh into the existing onSessionIdle path (sse-manager -> handleSessionIdle) via a new refreshWorktreesOnIdle in the worktrees store. On idle we reload the live server enumeration (no server cache added) and apply a constrained auto-switch: - Per-instance trailing debounce (~600ms) keyed by instanceId, deduped on the parent session id, so idle storms coalesce into one fetch. - Add a reloadLoads in-flight guard to reloadWorktrees so it no longer overlaps with rehydrateInstance's reload. - Diff the slug set before/after reload. Auto-switch only when exactly one worktree was added AND the idle session is the active session AND the parent session's slug has not changed since the snapshot (user has not manually switched). Zero or multiple additions refresh only. - The switch persists via setWorktreeSlugForParentSession, which writes through OpenCode session metadata (per #514) so it survives reload. Delete/prune paths are untouched. Adds co-located tests covering idle reload, debounce coalescing, the in-flight guard, single-new auto-switch, zero/multi-new no-switch, and the active-session guard. --- packages/ui/src/stores/session-events.ts | 8 +- .../src/stores/worktrees-idle-refresh.test.ts | 231 ++++++++++++++++++ packages/ui/src/stores/worktrees.ts | 98 +++++++- 3 files changed, 334 insertions(+), 3 deletions(-) create mode 100644 packages/ui/src/stores/worktrees-idle-refresh.test.ts diff --git a/packages/ui/src/stores/session-events.ts b/packages/ui/src/stores/session-events.ts index c19df28f2..040ec6e5d 100644 --- a/packages/ui/src/stores/session-events.ts +++ b/packages/ui/src/stores/session-events.ts @@ -59,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, @@ -594,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}`) } 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,