diff --git a/.github/workflows/comment-pr-artifacts.yml b/.github/workflows/comment-pr-artifacts.yml index b7f0fb2a..20295ede 100644 --- a/.github/workflows/comment-pr-artifacts.yml +++ b/.github/workflows/comment-pr-artifacts.yml @@ -59,8 +59,11 @@ jobs: const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + const maxAttempts = 60; + const pollIntervalMs = 20_000; + let matchedRun = null; - for (let attempt = 1; attempt <= 30; attempt += 1) { + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { const runs = await github.paginate(github.rest.actions.listWorkflowRuns, { owner, repo, @@ -78,8 +81,8 @@ jobs: break; } - core.info(`Waiting for PR Build Validation run for ${headSha} (attempt ${attempt}/30)`); - await sleep(10000); + core.info(`Waiting for PR Build Validation run for ${headSha} (attempt ${attempt}/${maxAttempts})`); + await sleep(pollIntervalMs); } if (!matchedRun) { diff --git a/.github/workflows/pr-build.yml b/.github/workflows/pr-build.yml index cf8a11a2..172d5c4d 100644 --- a/.github/workflows/pr-build.yml +++ b/.github/workflows/pr-build.yml @@ -4,7 +4,6 @@ on: pull_request: types: - opened - - edited - synchronize - reopened - ready_for_review diff --git a/packages/ui/src/components/tool-call.tsx b/packages/ui/src/components/tool-call.tsx index af9be1eb..15eed10d 100644 --- a/packages/ui/src/components/tool-call.tsx +++ b/packages/ui/src/components/tool-call.tsx @@ -13,6 +13,7 @@ import type { QuestionRequest } from "@opencode-ai/sdk/v2" import { useI18n } from "../lib/i18n" import { resolveToolRenderer } from "./tool-call/renderers" import { QuestionToolBlock } from "./tool-call/question-block" +import { isInlineQuestionActive } from "./tool-call/question-active" import { PermissionToolBlock } from "./tool-call/permission-block" import { createAnsiContentRenderer } from "./tool-call/ansi-render" import { createDiffContentRenderer } from "./tool-call/diff-render" @@ -651,11 +652,21 @@ export default function ToolCall(props: ToolCallProps) { return active?.kind === "permission" && active.id === pending.permission.id }) + // Task 059: the inline question block must derive its "active" state from the + // v2 message store only. The legacy `activeInterruption` signal is kept for + // cross-cutting consumers (permission modal, banner) but no longer gates the + // inline prompt — that split was the root cause of the "options render but + // are unclickable" bug. The rule mirrors the order used by the permission + // approval modal: a question is interactive iff it is the head of the v2 + // question queue AND no permission interruption is ahead in the v2 store. const isQuestionActive = createMemo(() => { const pending = pendingQuestion() if (!pending?.request) return false - const active = activeRequest() - return active?.kind === "question" && active.id === pending.request.id + return isInlineQuestionActive({ + requestId: pending.request.id, + questionsActiveRequestId: store().state.questions.active?.request.id ?? null, + permissionsActiveId: store().state.permissions.active?.permission.id ?? null, + }) }) const expanded = () => { diff --git a/packages/ui/src/components/tool-call/question-active.test.ts b/packages/ui/src/components/tool-call/question-active.test.ts new file mode 100644 index 00000000..049c3abc --- /dev/null +++ b/packages/ui/src/components/tool-call/question-active.test.ts @@ -0,0 +1,90 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +import { createInstanceMessageStore } from "../../stores/message-v2/instance-store.ts" +import { isInlineQuestionActive } from "./question-active.ts" + +describe("isInlineQuestionActive (task 059)", () => { + it("returns true when the question is the head of the v2 question queue and no permission is ahead", () => { + const store = createInstanceMessageStore("instance-1") + store.upsertQuestion({ + request: { id: "question-1", questions: [{ header: "Pick", question: "?", options: [{ label: "A", description: "" }] }] } as any, + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 1_000, + }) + + const result = isInlineQuestionActive({ + requestId: "question-1", + questionsActiveRequestId: store.state.questions.active?.request.id ?? null, + permissionsActiveId: store.state.permissions.active?.permission.id ?? null, + }) + + assert.equal(result, true) + }) + + it("returns false when a permission interruption is ahead of the question (F-5 / F-1 reproduction)", () => { + const store = createInstanceMessageStore("instance-1") + + // Permission lands first and takes the v2 active slot. + store.upsertPermission({ + permission: { id: "permission-1", time: { created: 1_000 } } as any, + messageId: "msg-1", + partId: "perm-part-1", + enqueuedAt: 1_000, + }) + + // Then a question arrives — its options will render but the inline block + // must not be interactive because a permission is ahead. + store.upsertQuestion({ + request: { id: "question-1", questions: [{ header: "Pick", question: "?", options: [{ label: "A", description: "" }] }] } as any, + messageId: "msg-1", + partId: "tool-part-1", + enqueuedAt: 2_000, + }) + + const result = isInlineQuestionActive({ + requestId: "question-1", + questionsActiveRequestId: store.state.questions.active?.request.id ?? null, + permissionsActiveId: store.state.permissions.active?.permission.id ?? null, + }) + + assert.equal(result, false, "Question prompt must be inactive while a permission is ahead in the queue") + }) + + it("returns false when another question is ahead in the queue", () => { + const store = createInstanceMessageStore("instance-1") + store.upsertQuestion({ + request: { id: "question-1", questions: [] } as any, + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 1_000, + }) + store.upsertQuestion({ + request: { id: "question-2", questions: [] } as any, + messageId: "msg-1", + partId: "part-2", + enqueuedAt: 2_000, + }) + + // The store keeps the first inserted entry as active. + const activeId = store.state.questions.active?.request.id + assert.equal(activeId, "question-1") + + const queuedResult = isInlineQuestionActive({ + requestId: "question-2", + questionsActiveRequestId: activeId ?? null, + permissionsActiveId: null, + }) + assert.equal(queuedResult, false) + }) + + it("returns false when the request id is missing", () => { + const result = isInlineQuestionActive({ + requestId: undefined, + questionsActiveRequestId: "question-1", + permissionsActiveId: null, + }) + assert.equal(result, false) + }) +}) diff --git a/packages/ui/src/components/tool-call/question-active.ts b/packages/ui/src/components/tool-call/question-active.ts new file mode 100644 index 00000000..5e1ad345 --- /dev/null +++ b/packages/ui/src/components/tool-call/question-active.ts @@ -0,0 +1,32 @@ +/** + * Decide whether an inline question prompt should be interactive. + * + * The legacy `activeInterruption` signal in `instances.ts` and the v2 store's + * `state.questions.active` field used to disagree about which question owned + * the focus, which produced the "options render but cannot be clicked" bug + * tracked in tasks 058 / 059. + * + * This helper now derives the answer from the v2 store only: + * - The question must be the head of the v2 question queue + * (`questionsActiveRequestId === request.id`). + * - No permission interruption may be ahead of the question + * (`permissionsActiveId == null`). + * + * Keeping the rule pure makes it cheap to unit test and removes any + * dependency on the legacy `activeInterruption` signal for the inline + * ``. + */ +export interface QuestionActiveInput { + /** Question request id rendered by the inline block. */ + requestId: string | null | undefined + /** `state.questions.active?.request.id` from the v2 message store. */ + questionsActiveRequestId: string | null | undefined + /** `state.permissions.active?.permission.id` from the v2 message store. */ + permissionsActiveId: string | null | undefined +} + +export function isInlineQuestionActive(input: QuestionActiveInput): boolean { + if (!input.requestId) return false + if (input.permissionsActiveId) return false + return input.questionsActiveRequestId === input.requestId +} diff --git a/packages/ui/src/components/tool-call/question-block.tsx b/packages/ui/src/components/tool-call/question-block.tsx index dee5fee9..79d5d731 100644 --- a/packages/ui/src/components/tool-call/question-block.tsx +++ b/packages/ui/src/components/tool-call/question-block.tsx @@ -188,11 +188,39 @@ export function QuestionToolBlock(props: QuestionToolBlockProps) { updateAnswer(questionIndex, next) } + // Task 059: when the question is pending but not active (e.g. a permission + // interruption is ahead of it, or another question is currently active), + // render an explicit "queued" state instead of a fully-rendered but + // unclickable radio list with no Submit button. This makes the inactive + // state self-explanatory and removes the false impression that the system + // is still loading. + const isQueuedBehindInterruption = createMemo(() => { + if (props.active()) return false + if (hasFinalAnswers()) return false + return Boolean(props.request()) + }) + return ( 0}>
+ +
+
+
{t("toolCall.question.queuedLabel")}
+

+ {t("toolCall.question.queuedHint")} +

+
+
+ +
@@ -340,11 +368,17 @@ export function QuestionToolBlock(props: QuestionToolBlockProps) {
- -

{t("toolCall.question.queuedText")}

- + {/* + Task 059: the previous fallback queuedText was rendered alongside + fully-disabled radios. The dedicated `isQueuedBehindInterruption` + branch above now owns the inactive presentation, so the legacy + hint here would be dead code. If the question is in any other + non-active terminal state (already answered) the answered styling + keeps it readable without an extra hint. + */}
+ ) diff --git a/packages/ui/src/components/tool-call/question-concurrency.test.ts b/packages/ui/src/components/tool-call/question-concurrency.test.ts new file mode 100644 index 00000000..0b7abe0d --- /dev/null +++ b/packages/ui/src/components/tool-call/question-concurrency.test.ts @@ -0,0 +1,221 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +import { createInstanceMessageStore } from "../../stores/message-v2/instance-store.ts" +import { isInlineQuestionActive } from "./question-active.ts" + +/** + * Task 059 — scenario reproductions for the three failure modes called out in + * the investigation (task 058) and observed in the wild on issue #448. + * + * These tests drive the v2 message store the same way the SSE pipeline does + * during a real session and assert that {@link isInlineQuestionActive} — the + * gate that decides whether the inline `` is interactive — + * returns the right answer for every question in the queue. + * + * Reading the v2 store state via the public selector is exactly what + * `tool-call.tsx::isQuestionActive` does after the fix, so a passing assertion + * here means a real user looking at the same question would see an + * interactive prompt (or an honest "Queued" banner) and never a stuck + * options-visible-but-uninteractive state. + * + * Why this lives in `components/tool-call/` rather than `stores/`: the + * scenarios validate the inline component's contract with the store, not the + * store's internal invariants. The store-level invariants are covered by + * `question-optimistic-clear.test.ts`. + */ + +type ActiveSnapshot = { + questionsActiveRequestId: string | null + permissionsActiveId: string | null +} + +function snapshotActive(store: ReturnType): ActiveSnapshot { + return { + questionsActiveRequestId: store.state.questions.active?.request.id ?? null, + permissionsActiveId: store.state.permissions.active?.permission.id ?? null, + } +} + +function gate(snap: ActiveSnapshot, requestId: string) { + return isInlineQuestionActive({ + requestId, + questionsActiveRequestId: snap.questionsActiveRequestId, + permissionsActiveId: snap.permissionsActiveId, + }) +} + +function makeQuestion(id: string) { + return { + id, + questions: [ + { header: "Pick one", question: "?", options: [{ label: "A", description: "" }] }, + ], + } as any +} + +describe("question prompt concurrency scenarios (task 059 / issue #448)", () => { + it("two questions arrive back-to-back: head is interactive, trailing renders queued", () => { + // Scenario: parallel subagents each emit a question into the same session + // within milliseconds. Both tool parts mount and render their options. + // Before the fix the inline gate could leave the head disabled while the + // legacy `activeInterruption` resolved; after the fix the v2 store is the + // single source of truth and the head is always interactive. + const store = createInstanceMessageStore("instance-1") + + store.upsertQuestion({ + request: makeQuestion("q-head"), + messageId: "msg-1", + partId: "part-head", + enqueuedAt: 1_000, + }) + store.upsertQuestion({ + request: makeQuestion("q-tail"), + messageId: "msg-1", + partId: "part-tail", + enqueuedAt: 1_010, + }) + + const snap = snapshotActive(store) + assert.equal(snap.questionsActiveRequestId, "q-head", "first-arrival keeps the active slot") + assert.equal(gate(snap, "q-head"), true, "head question must be interactive") + assert.equal(gate(snap, "q-tail"), false, "trailing question must render the queued banner") + }) + + it("permission interruption arriving alongside a question keeps the prompt non-interactive (issue #448 path 1)", () => { + // Scenario: a tool call requiring permission and a question arrive in the + // same SSE burst. The permission lands first and owns the v2 permission + // slot; the question's tool part still mounts and its options render. + // Before the fix the legacy `activeInterruption` pointed at the permission + // and the inline block disabled every input while leaving Submit hidden — + // visually identical to "loading." After the fix the inline gate explicitly + // sees the permission ahead and the queued banner is rendered instead. + const store = createInstanceMessageStore("instance-1") + + store.upsertPermission({ + permission: { id: "perm-1", time: { created: 1_000 } } as any, + messageId: "msg-1", + partId: "part-perm", + enqueuedAt: 1_000, + }) + store.upsertQuestion({ + request: makeQuestion("q-1"), + messageId: "msg-1", + partId: "part-question", + enqueuedAt: 1_005, + }) + + const blockedSnap = snapshotActive(store) + assert.equal(blockedSnap.permissionsActiveId, "perm-1") + assert.equal(blockedSnap.questionsActiveRequestId, "q-1") + assert.equal( + gate(blockedSnap, "q-1"), + false, + "question must not be interactive while a permission is ahead in the v2 queue", + ) + + // Once the user resolves the permission, the question becomes interactive + // without any further SSE replay — the gate reacts to v2 state only. + store.removePermission("perm-1") + const releasedSnap = snapshotActive(store) + assert.equal(releasedSnap.permissionsActiveId, null) + assert.equal( + gate(releasedSnap, "q-1"), + true, + "question becomes interactive immediately after the permission is cleared", + ) + }) + + it("post-submit lifecycle: optimistic clear + delayed SSE confirmation never re-strands the prompt", () => { + // Scenario: user picks an answer and submits. The HTTP reply succeeds, + // `sendQuestionReply` removes the v2 entry optimistically, and a moment + // later the server's `question.replied` SSE event arrives and triggers a + // second `removeQuestion`. Before the fix the inline gate would briefly + // disagree with the v2 store during this window (legacy queue cleared, v2 + // entry still present, looked stuck). After the fix the gate goes to false + // the instant the v2 entry is removed and stays false even after the + // duplicate SSE clear. + const store = createInstanceMessageStore("instance-1") + store.upsertQuestion({ + request: makeQuestion("q-reply"), + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 2_000, + }) + + const beforeSubmit = snapshotActive(store) + assert.equal(gate(beforeSubmit, "q-reply"), true) + + // sendQuestionReply -> optimistic clear + store.removeQuestion("q-reply") + const afterOptimisticClear = snapshotActive(store) + assert.equal(afterOptimisticClear.questionsActiveRequestId, null) + assert.equal(gate(afterOptimisticClear, "q-reply"), false) + + // Confirming SSE event arrives later — idempotent + assert.doesNotThrow(() => store.removeQuestion("q-reply")) + const afterConfirmingSse = snapshotActive(store) + assert.equal(afterConfirmingSse.questionsActiveRequestId, null) + assert.equal(gate(afterConfirmingSse, "q-reply"), false) + }) + + it("rollback after a failed reply restores interactivity on the same prompt", () => { + // Scenario: optimistic clear runs, the HTTP reply fails, and the v2 entry + // is re-upserted. The gate must return `true` again so the user can retry. + const store = createInstanceMessageStore("instance-1") + const request = makeQuestion("q-retry") + store.upsertQuestion({ + request, + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 3_000, + }) + + // Optimistic clear. + store.removeQuestion("q-retry") + assert.equal(gate(snapshotActive(store), "q-retry"), false) + + // Rollback after network failure. + store.upsertQuestion({ request, messageId: "msg-1", partId: "part-1", enqueuedAt: 3_000 }) + const afterRollback = snapshotActive(store) + assert.equal(afterRollback.questionsActiveRequestId, "q-retry") + assert.equal(gate(afterRollback, "q-retry"), true, "user must be able to retry the answer") + }) + + it("permission ahead of the head only blocks until the permission resolves (queue head stays correct)", () => { + // Defense-in-depth: a permission ahead must not also disable a queued + // (non-head) question's interactivity flag in a way that gets it + // accidentally activated after the permission clears. Only the head + // question becomes interactive when the permission is removed; the + // trailing question remains queued. + const store = createInstanceMessageStore("instance-1") + + store.upsertPermission({ + permission: { id: "perm-1", time: { created: 0 } } as any, + messageId: "msg-1", + partId: "part-perm", + enqueuedAt: 1_000, + }) + store.upsertQuestion({ + request: makeQuestion("q-head"), + messageId: "msg-1", + partId: "part-head", + enqueuedAt: 1_500, + }) + store.upsertQuestion({ + request: makeQuestion("q-tail"), + messageId: "msg-1", + partId: "part-tail", + enqueuedAt: 1_600, + }) + + const blocked = snapshotActive(store) + assert.equal(gate(blocked, "q-head"), false) + assert.equal(gate(blocked, "q-tail"), false) + + store.removePermission("perm-1") + const released = snapshotActive(store) + assert.equal(gate(released, "q-head"), true) + assert.equal(gate(released, "q-tail"), false, "queued question stays queued until head is answered") + }) +}) diff --git a/packages/ui/src/lib/i18n/messages/en/toolCall.ts b/packages/ui/src/lib/i18n/messages/en/toolCall.ts index 6d1736f5..0a14e123 100644 --- a/packages/ui/src/lib/i18n/messages/en/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/en/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "Submit", "toolCall.question.shortcuts.dismiss": "Dismiss", "toolCall.question.queuedText": "Waiting for earlier responses.", + "toolCall.question.queuedLabel": "Queued", + "toolCall.question.queuedHint": "This question is queued behind another interruption. Resolve the active prompt to answer this one.", "toolCall.question.validation.answerAll": "Please answer all questions before submitting.", "toolCall.question.errors.unableToReply": "Unable to reply", "toolCall.question.errors.unableToDismiss": "Unable to dismiss", diff --git a/packages/ui/src/lib/i18n/messages/es/toolCall.ts b/packages/ui/src/lib/i18n/messages/es/toolCall.ts index 52422359..f2488d42 100644 --- a/packages/ui/src/lib/i18n/messages/es/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/es/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "Enviar", "toolCall.question.shortcuts.dismiss": "Descartar", "toolCall.question.queuedText": "Esperando respuestas anteriores.", + "toolCall.question.queuedLabel": "En cola", + "toolCall.question.queuedHint": "Esta pregunta está en cola detrás de otra interrupción. Resuelve la solicitud activa para responder esta.", "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", diff --git a/packages/ui/src/lib/i18n/messages/fr/toolCall.ts b/packages/ui/src/lib/i18n/messages/fr/toolCall.ts index 6cab1b0a..588a01d4 100644 --- a/packages/ui/src/lib/i18n/messages/fr/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/fr/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "Envoyer", "toolCall.question.shortcuts.dismiss": "Ignorer", "toolCall.question.queuedText": "En attente des réponses précédentes.", + "toolCall.question.queuedLabel": "En file d'attente", + "toolCall.question.queuedHint": "Cette question est en file d'attente derrière une autre interruption. Répondez à l'invite active pour pouvoir répondre à celle-ci.", "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", diff --git a/packages/ui/src/lib/i18n/messages/he/toolCall.ts b/packages/ui/src/lib/i18n/messages/he/toolCall.ts index 578dd244..0de319a5 100644 --- a/packages/ui/src/lib/i18n/messages/he/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/he/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "שלח", "toolCall.question.shortcuts.dismiss": "סגור", "toolCall.question.queuedText": "ממתין לתגובות קודמות.", + "toolCall.question.queuedLabel": "בתור", + "toolCall.question.queuedHint": "שאלה זו נמצאת בתור אחרי הפרעה אחרת. השב להודעה הפעילה כדי לענות על שאלה זו.", "toolCall.question.validation.answerAll": "אנא ענה על כל השאלות לפני השליחה.", "toolCall.question.errors.unableToReply": "לא ניתן לשלוח תשובה", "toolCall.question.errors.unableToDismiss": "לא ניתן לסגור", diff --git a/packages/ui/src/lib/i18n/messages/ja/toolCall.ts b/packages/ui/src/lib/i18n/messages/ja/toolCall.ts index 58e6ad1f..376fe16f 100644 --- a/packages/ui/src/lib/i18n/messages/ja/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/ja/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "送信", "toolCall.question.shortcuts.dismiss": "閉じる", "toolCall.question.queuedText": "先行する回答を待機中です。", + "toolCall.question.queuedLabel": "順番待ち", + "toolCall.question.queuedHint": "この質問は別の割り込みの後ろで順番待ちになっています。先にアクティブなプロンプトへ回答してください。", "toolCall.question.validation.answerAll": "送信する前にすべての質問に回答してください。", "toolCall.question.errors.unableToReply": "回答できません", "toolCall.question.errors.unableToDismiss": "閉じられません", diff --git a/packages/ui/src/lib/i18n/messages/ru/toolCall.ts b/packages/ui/src/lib/i18n/messages/ru/toolCall.ts index 54f24020..b437791e 100644 --- a/packages/ui/src/lib/i18n/messages/ru/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/ru/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "Отправить", "toolCall.question.shortcuts.dismiss": "Скрыть", "toolCall.question.queuedText": "Ожидание предыдущих ответов.", + "toolCall.question.queuedLabel": "В очереди", + "toolCall.question.queuedHint": "Этот вопрос находится в очереди за другим запросом. Ответьте на активный запрос, чтобы перейти к этому.", "toolCall.question.validation.answerAll": "Ответьте на все вопросы перед отправкой.", "toolCall.question.errors.unableToReply": "Не удалось ответить", "toolCall.question.errors.unableToDismiss": "Не удалось скрыть", 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 eae5a29b..ab8017f1 100644 --- a/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts +++ b/packages/ui/src/lib/i18n/messages/zh-Hans/toolCall.ts @@ -111,6 +111,8 @@ export const toolCallMessages = { "toolCall.question.shortcuts.submit": "提交", "toolCall.question.shortcuts.dismiss": "忽略", "toolCall.question.queuedText": "正在等待更早的响应。", + "toolCall.question.queuedLabel": "排队中", + "toolCall.question.queuedHint": "此问题正排在另一个中断之后。请先回应当前活动的提示，再回答此问题。", "toolCall.question.validation.answerAll": "请先回答所有问题再提交。", "toolCall.question.errors.unableToReply": "无法回复", "toolCall.question.errors.unableToDismiss": "无法忽略", diff --git a/packages/ui/src/stores/instances.ts b/packages/ui/src/stores/instances.ts index ccf14f16..70bf52f7 100644 --- a/packages/ui/src/stores/instances.ts +++ b/packages/ui/src/stores/instances.ts @@ -696,6 +696,23 @@ function computeActiveInterruption(instanceId: string): ActiveInterruption { } function setActiveInterruptionForInstance(instanceId: string, nextActive: ActiveInterruption): void { + // Task 059 diagnostic: log every change to the per-instance active + // interruption so production telemetry can confirm when a question is + // preempted by a permission (the root cause path documented in task 058). + // Ids and booleans only — no question/answer content is logged. + const prevActive = activeInterruption().get(instanceId) ?? null + const prevKey = prevActive ? `${prevActive.kind}:${prevActive.id}` : null + const nextKey = nextActive ? `${nextActive.kind}:${nextActive.id}` : null + if (prevKey !== nextKey) { + log.info("interruption.active.changed", { + instanceId, + previous: prevKey, + next: nextKey, + previousKind: prevActive?.kind ?? null, + nextKind: nextActive?.kind ?? null, + }) + } + setActiveInterruption((prev) => { const next = new Map(prev) if (!nextActive) { @@ -1026,6 +1043,59 @@ function setActiveQuestionIdForInstance(instanceId: string, requestId: string): setActiveInterruptionForInstance(instanceId, { kind: "question", id: requestId }) } +// Task 059: snapshot a v2 question entry across all `byMessage` slots so we +// can restore it on a network failure during an optimistic clear. The store's +// `removeQuestion` wipes every entry that references the request id, so the +// snapshot must cover all of them; restoring is a sequence of `upsertQuestion` +// calls in the original order. +function snapshotV2QuestionEntries(instanceId: string, requestId: string) { + const store = messageStoreBus.getInstance(instanceId) + if (!store) return [] as Array<{ + messageId: string | undefined + partId: string | undefined + request: QuestionRequest + enqueuedAt: number + }> + const snapshots: Array<{ + messageId: string | undefined + partId: string | undefined + request: QuestionRequest + enqueuedAt: number + }> = [] + const byMessage = store.state.questions.byMessage + for (const messageKey of Object.keys(byMessage)) { + const partEntries = byMessage[messageKey] + if (!partEntries) continue + for (const partKey of Object.keys(partEntries)) { + const entry = partEntries[partKey] + if (!entry || entry.request?.id !== requestId) continue + snapshots.push({ + messageId: entry.messageId, + partId: entry.partId, + request: entry.request, + enqueuedAt: entry.enqueuedAt ?? Date.now(), + }) + } + } + return snapshots +} + +function restoreV2QuestionEntries( + instanceId: string, + snapshots: ReturnType, +): void { + if (snapshots.length === 0) return + const store = messageStoreBus.getOrCreate(instanceId) + for (const snap of snapshots) { + store.upsertQuestion({ + request: snap.request, + messageId: snap.messageId, + partId: snap.partId, + enqueuedAt: snap.enqueuedAt, + }) + } +} + async function sendQuestionReply( instanceId: string, sessionId: string, @@ -1037,6 +1107,23 @@ async function sendQuestionReply( throw new Error("Instance not ready") } + // Task 059 diagnostic: log entry to the reply flow. Ids only — never log + // `answers` (may contain user-supplied text or sensitive content). + log.info("question.reply.start", { instanceId, sessionId, requestId }) + + // Task 059 optimistic clear: remove the v2 entry immediately so the inline + // prompt cannot keep rendering during the post-reply transient window even + // if the server's `question.replied` SSE event is delayed. If the network + // call fails we restore the snapshot and surface the error. + const snapshot = snapshotV2QuestionEntries(instanceId, requestId) + removeQuestionV2(instanceId, requestId) + log.info("question.reply.optimisticClear", { + instanceId, + requestId, + restored: false, + snapshotCount: snapshot.length, + }) + try { const stored = questionWorktreeSlugByInstance.get(instanceId)?.get(requestId) const fallback = sessionId ? getWorktreeSlugForSession(instanceId, sessionId) : "root" @@ -1054,6 +1141,13 @@ async function sendQuestionReply( removeQuestionFromQueue(instanceId, requestId) } catch (error) { log.error("Failed to send question reply", error) + restoreV2QuestionEntries(instanceId, snapshot) + log.info("question.reply.optimisticClear.rollback", { + instanceId, + requestId, + restored: true, + snapshotCount: snapshot.length, + }) throw error } } @@ -1064,6 +1158,18 @@ async function sendQuestionReject(instanceId: string, sessionId: string, request throw new Error("Instance not ready") } + // Task 059 diagnostic: log entry to the reject flow (ids only). + log.info("question.reject.start", { instanceId, sessionId, requestId }) + + const snapshot = snapshotV2QuestionEntries(instanceId, requestId) + removeQuestionV2(instanceId, requestId) + log.info("question.reject.optimisticClear", { + instanceId, + requestId, + restored: false, + snapshotCount: snapshot.length, + }) + try { const stored = questionWorktreeSlugByInstance.get(instanceId)?.get(requestId) const fallback = sessionId ? getWorktreeSlugForSession(instanceId, sessionId) : "root" @@ -1080,6 +1186,13 @@ async function sendQuestionReject(instanceId: string, sessionId: string, request removeQuestionFromQueue(instanceId, requestId) } catch (error) { log.error("Failed to send question reject", error) + restoreV2QuestionEntries(instanceId, snapshot) + log.info("question.reject.optimisticClear.rollback", { + instanceId, + requestId, + restored: true, + snapshotCount: snapshot.length, + }) throw error } } diff --git a/packages/ui/src/stores/question-optimistic-clear.test.ts b/packages/ui/src/stores/question-optimistic-clear.test.ts new file mode 100644 index 00000000..6eb2ff08 --- /dev/null +++ b/packages/ui/src/stores/question-optimistic-clear.test.ts @@ -0,0 +1,80 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" + +import { createInstanceMessageStore } from "./message-v2/instance-store.ts" + +/** + * Task 059: regression test for the post-submit transient window (F-2 in + * task 058's investigation). + * + * Before the fix, the v2 question entry was only removed when the server's + * confirming `question.replied` SSE event arrived. On a slow round-trip or a + * brief SSE disconnect the inline prompt re-rendered with options visible but + * inactive between the HTTP reply resolving and the broadcast event. + * + * After the fix, `sendQuestionReply`/`sendQuestionReject` clear the v2 entry + * optimistically — and restore it on a network failure. We can't exercise the + * full action here without spinning up the network/worktree client, but we + * can verify the underlying invariant: the v2 store correctly removes and + * re-upserts a question entry without leaving stale `byMessage` slots, so + * rollback is always sound. + */ +describe("question v2 entry optimistic clear/restore (task 059)", () => { + it("removeQuestion clears all byMessage references and active slot", () => { + const store = createInstanceMessageStore("instance-1") + const request = { id: "question-1", questions: [] } as any + store.upsertQuestion({ + request, + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 1_000, + }) + + assert.equal(store.state.questions.active?.request.id, "question-1") + assert.equal(store.state.questions.queue.length, 1) + assert.equal(store.getQuestionState("msg-1", "part-1")?.entry.request.id, "question-1") + + store.removeQuestion("question-1") + + assert.equal(store.state.questions.active, null) + assert.equal(store.state.questions.queue.length, 0) + assert.equal(store.getQuestionState("msg-1", "part-1"), null) + }) + + it("re-upserting after a remove restores the entry with the same identity (rollback path)", () => { + const store = createInstanceMessageStore("instance-1") + const request = { id: "question-1", questions: [] } as any + const enqueuedAt = 1_500 + + store.upsertQuestion({ request, messageId: "msg-1", partId: "part-1", enqueuedAt }) + + // Simulate optimistic clear. + store.removeQuestion("question-1") + assert.equal(store.state.questions.queue.length, 0) + + // Simulate rollback after a failed network call. + store.upsertQuestion({ request, messageId: "msg-1", partId: "part-1", enqueuedAt }) + + const restoredActive = store.state.questions.active + assert.ok(restoredActive, "Question should be restored after rollback") + assert.equal(restoredActive.request.id, "question-1") + assert.equal(store.state.questions.queue.length, 1) + assert.equal(store.getQuestionState("msg-1", "part-1")?.active, true) + }) + + it("a second optimistic-clear after the SSE confirmation is idempotent", () => { + const store = createInstanceMessageStore("instance-1") + store.upsertQuestion({ + request: { id: "question-1", questions: [] } as any, + messageId: "msg-1", + partId: "part-1", + enqueuedAt: 1_000, + }) + + // First clear: optimistic (during `sendQuestionReply`). + store.removeQuestion("question-1") + // Second clear: confirming `question.replied` SSE event arrives later. + assert.doesNotThrow(() => store.removeQuestion("question-1")) + assert.equal(store.state.questions.queue.length, 0) + }) +}) diff --git a/packages/ui/src/stores/session-events.ts b/packages/ui/src/stores/session-events.ts index fcb253bf..4d827c40 100644 --- a/packages/ui/src/stores/session-events.ts +++ b/packages/ui/src/stores/session-events.ts @@ -39,6 +39,7 @@ import { hasRepliedPermission, addQuestionToQueue, removeQuestionFromQueue, + getQuestionQueue, } from "./instances" import { showAlertDialog } from "./alerts" import { @@ -728,12 +729,24 @@ 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)}`) + // Task 059 diagnostic: surface duplicate `question.asked` events so we can + // tell whether F-6 (duplicate ask in task 058) is contributing to a + // stuck-prompt report. Ids and booleans only — no question content is + // logged. + const questionId = getQuestionId(request) + const sessionId = getQuestionSessionId(request) ?? null + const duplicate = getQuestionQueue(instanceId).some((q) => q.id === questionId) + + log.info(`[SSE] Question asked: ${questionId}`) + log.info("question.asked", { + instanceId, + sessionId, + questionId, + duplicate, + }) addQuestionToQueue(instanceId, request) upsertQuestionV2(instanceId, request) - const sessionId = getQuestionSessionId(request) - if (shouldSendOsNotificationForSession("needsInput", instanceId, sessionId)) { const title = getInstanceDisplayName(instanceId) const label = getSessionTitle(instanceId, sessionId) @@ -750,7 +763,25 @@ function handleQuestionAnswered( const requestId = getRequestIdFromQuestionReply(properties) if (!requestId) return + // Task 059 diagnostic: log whether the local queue still tracked the + // question id when the confirming SSE event arrived. After the optimistic + // clear in `sendQuestionReply`/`sendQuestionReject` this is normally `false` + // for replies from this client; remaining `true` would point to drift in + // the queue mutation path. Ids and booleans only. + const localQueueHadEntry = getQuestionQueue(instanceId).some((q) => q.id === requestId) + const sessionIdFromEvent = + (properties as any)?.sessionID ?? + (properties as any)?.sessionId ?? + (properties as any)?.session?.id ?? + null + log.info(`[SSE] Question answered: ${requestId}`) + log.info("question.answered", { + instanceId, + sessionId: sessionIdFromEvent, + questionId: requestId, + localStoreHadEntry: localQueueHadEntry, + }) removeQuestionFromQueue(instanceId, requestId) removeQuestionV2(instanceId, requestId) }