From d33a04197f6be020211cef054e90b60b92956fea Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Sat, 16 May 2026 13:05:48 +0000 Subject: [PATCH 1/2] fix(ui): unblock stuck question prompt when interruptions race User-visible behavior change: Previously, when an agent issued a question while a permission prompt was ahead in the interruption queue (or during a brief SSE disconnect right after a reply), the inline question block could render with options visible but every input disabled and the Submit button hidden. The state looked indistinguishable from "the system is still loading" and the user could neither pick an option nor dismiss the prompt. After this change, the inline is interactive iff the v2 message store agrees that the question is the current interruption, and otherwise renders an explicit "Queued" banner with a short hint explaining that another interruption is ahead. Submitting or dismissing the prompt clears it from the UI immediately on success, even if the server's confirming SSE event is delayed; on failure the prompt is restored and the error surfaces through the existing path. Implementation approach: - tool-call.tsx::isQuestionActive was rewritten to derive its result from the v2 message store only. The rule lives in a new pure helper at packages/ui/src/components/tool-call/question-active.ts: a question is active when it is the head of the v2 question queue AND no permission interruption is ahead in the v2 store. The legacy activeInterruption signal is preserved for cross-cutting consumers (permission approval modal, banner) but no longer gates the inline prompt; that split was the structural defect causing the symptom. - now renders a dedicated queued-state branch (label + hint, no inputs, no spinner, no Submit) instead of a fully disabled radio list when props.active() === false and the request is still pending. The dead legacy queuedText fallback was removed. - instances.ts::sendQuestionReply and sendQuestionReject snapshot the v2 entry, call removeQuestionV2 before the network request, await the HTTP reply, and restore the snapshot on rejection. This closes the post-submit transient window where the legacy queue had been cleared but the v2 entry was still rendered until the SSE confirmation arrived. - Four diagnostic log points are added using the existing getLogger module (no new logger introduced): interruption.active.changed, question.reply.start / question.reject.start plus their optimistic clear / rollback events, question.asked with a duplicate boolean, and question.answered with a localStoreHadEntry boolean. Payloads contain ids and booleans only; answer text and attachments are never logged. - New i18n keys toolCall.question.queuedLabel and toolCall.question.queuedHint were added to every locale under packages/ui/src/lib/i18n/messages/ (en, es, fr, he, ja, ru, zh-Hans). Edge cases and platform considerations: - Permission interruption ahead of a question: the new helper rule returns false, so the queued banner renders instead of a misleading disabled radio list. - SSE reconnect drops the confirming question.replied event: the optimistic clear has already removed the v2 entry, so the prompt does not redraw in a stuck state. - Multi-question queue within a session: only the head question is active; trailing questions render the queued banner. - Permission approval modal and notification banner are intentionally untouched and continue to read from activeInterruption. Validation: - 28/29 UI tests pass under node:test via tsx (one pre-existing failure in session-status.test.ts on dev, unrelated to this change). Three test files cover task 059: - question-active.test.ts (4 cases, unit-level coverage of the new helper including the permission-ahead branch). - question-optimistic-clear.test.ts (3 cases covering the v2 store remove/restore invariants the rollback path depends on). - question-concurrency.test.ts (5 scenario-level cases reproducing the three failure modes called out in the investigation and observed in issue #448: back-to-back questions, permission ahead of a question, post-submit lifecycle with delayed SSE confirmation, rollback after a failed reply, and permission ahead of a multi-question queue). - tsc --noEmit clean for packages/ui. - vite build clean for packages/ui. - Manual verification on the web build: active prompt unchanged, new queued banner renders when another interruption is ahead, submit clears prompt immediately even when SSE confirmation is delayed. Related: likely overlaps with the user-facing symptom reported in #448. --- packages/ui/src/components/tool-call.tsx | 15 +- .../tool-call/question-active.test.ts | 90 +++++++ .../components/tool-call/question-active.ts | 32 +++ .../components/tool-call/question-block.tsx | 40 +++- .../tool-call/question-concurrency.test.ts | 221 ++++++++++++++++++ .../ui/src/lib/i18n/messages/en/toolCall.ts | 2 + .../ui/src/lib/i18n/messages/es/toolCall.ts | 2 + .../ui/src/lib/i18n/messages/fr/toolCall.ts | 2 + .../ui/src/lib/i18n/messages/he/toolCall.ts | 2 + .../ui/src/lib/i18n/messages/ja/toolCall.ts | 2 + .../ui/src/lib/i18n/messages/ru/toolCall.ts | 2 + .../src/lib/i18n/messages/zh-Hans/toolCall.ts | 2 + packages/ui/src/stores/instances.ts | 113 +++++++++ .../stores/question-optimistic-clear.test.ts | 80 +++++++ packages/ui/src/stores/session-events.ts | 37 ++- 15 files changed, 634 insertions(+), 8 deletions(-) create mode 100644 packages/ui/src/components/tool-call/question-active.test.ts create mode 100644 packages/ui/src/components/tool-call/question-active.ts create mode 100644 packages/ui/src/components/tool-call/question-concurrency.test.ts create mode 100644 packages/ui/src/stores/question-optimistic-clear.test.ts diff --git a/packages/ui/src/components/tool-call.tsx b/packages/ui/src/components/tool-call.tsx index af9be1ebd..15eed10d9 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 000000000..049c3abcf --- /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 000000000..5e1ad3455 --- /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 dee5fee91..79d5d7314 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 000000000..0b7abe0d7 --- /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 6d1736f5c..0a14e1236 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 52422359d..f2488d42b 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 6cab1b0a1..588a01d4a 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 578dd2446..0de319a56 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 58e6ad1f0..376fe16fc 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 54f24020b..b437791e8 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 eae5a29bc..ab8017f15 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 ccf14f160..70bf52f7e 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 000000000..6eb2ff089 --- /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 fcb253bf9..4d827c40c 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) } From e68a053d4e4ec8b6fd9714c8ca9157b364a42671 Mon Sep 17 00:00:00 2001 From: omercnet Date: Sat, 16 May 2026 19:14:36 +0000 Subject: [PATCH 2/2] ci: fix Comment PR Artifacts timeout and avoid duplicate build triggers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Comment PR Artifacts workflow was timing out after ~12 minutes (30 iterations × 10s sleep) while PR Build Validation runs regularly take 17+ minutes, causing most PRs to show a failing CI check. Changes: - Increase comment workflow polling to 60 iterations × 20s sleep (~20 min max) so it reliably waits for the full build to complete. - Remove the 'edited' trigger from pr-build.yml to prevent duplicate build runs when a PR title/body is edited. Editing a PR should not rebuild; only code changes (synchronize) and lifecycle events need builds. This also prevents the comment workflow from finding a newer in-progress run when a completed one already exists. Co-authored-by: openhands --- .github/workflows/comment-pr-artifacts.yml | 9 ++++++--- .github/workflows/pr-build.yml | 1 - 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/comment-pr-artifacts.yml b/.github/workflows/comment-pr-artifacts.yml index b7f0fb2a0..20295ede6 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 cf8a11a23..172d5c4d8 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