From c847c2ffad08ac1a627df091f9843ae1775e0fde Mon Sep 17 00:00:00 2001 From: Hare Date: Mon, 5 Oct 2026 20:55:17 +0900 Subject: [PATCH] fix: cancel pending uploads when composer chips are removed --- docs/design/feature-chat-invocations.md | 2 +- .../2026-10-05-review-verdict-validation.md | 13 +++ .../console_live_admission_shell_test.ts | 97 ++++++++++++++----- .../console/ComposerInput.browser.spec.ts | 48 ++++++++- .../workspace/console/ComposerInput.svelte | 46 +++++++-- .../lib/workspace/console/composer-draft.ts | 2 + .../workers/[workerRef]/console/+page.svelte | 12 +++ 7 files changed, 186 insertions(+), 34 deletions(-) create mode 100644 docs/report/2026-10-05-review-verdict-validation.md diff --git a/docs/design/feature-chat-invocations.md b/docs/design/feature-chat-invocations.md index 16efb9342..8bfe93554 100644 --- a/docs/design/feature-chat-invocations.md +++ b/docs/design/feature-chat-invocations.md @@ -59,7 +59,7 @@ Completion requests carry a kind, fresh `request_id`, and source-qualified argum - TUI may select a path visible to the TUI process, stage an upload, and replace the invocation intent with an `UploadedFile` chip. - Web opens the existing browser picker; it does not enumerate the browser host filesystem. - The local path is never sent to or resolved by the Worker. -- Uploading is cancellable draft staging, not invocation execution. Removing an unsent uploaded-file chip cancels or deletes staged state, and a late upload completion must not recreate a removed chip. +- Uploading is cancellable draft staging, not invocation execution. Removing an in-progress upload chip by Backspace/Delete, range replacement, or draft abandonment cancels its reservation and adapter upload; the removed upload must no longer block a text-only Submit. Late callbacks and Undo cannot restore that cancelled reservation as sendable input. A completed unsent uploaded-file chip may retain its staged resource while editor Undo can restore it, releasing abandoned resources at the draft boundary; a late completion must never turn a deleted pending reservation into such an Undo-owned resource. - Authoritative `SubmissionAccepted` (not socket-send success) transfers uploaded references to the durable input lifecycle. Draft removal cannot delete an in-flight reference whose admission may already have succeeded, or an accepted attachment. Rejection restores the exact typed intent for retry; retry retains request/invocation IDs. Upload completion alone is not submission acceptance. - TUI standalone currently has no client-local upload staging transport. It reports the unsupported adapter and preserves the draft; it does not reinterpret a local path as a Worker host path. diff --git a/docs/report/2026-10-05-review-verdict-validation.md b/docs/report/2026-10-05-review-verdict-validation.md new file mode 100644 index 000000000..d3cad9724 --- /dev/null +++ b/docs/report/2026-10-05-review-verdict-validation.md @@ -0,0 +1,13 @@ +# Review verdict validation and single-call policy + +During T-696's independent review, `ReviewRequested` captured MR `01a10bc8-59f4-7630-8166-d8cd71943e15`, source `9812e68b4d4021ea778a40d9ead4fe545d09e8bf`, and Ticket revision `00001M4517HE0:5`. The Reviewer reproduced a pending-upload deletion defect and attempted a structured `request_changes` verdict. The request used finding severity `medium`; the Backend returned HTTP 422, listing the accepted values as `blocker`, `major`, `minor`, and `note`. No review ID or verdict event was returned. + +The Reviewer obeyed the role prompt's instruction to call `ReviewMergeRequest` exactly once and did not retry. Its prose recommendation and artifacts are evidence of investigation, not recorded review authority. The parent preserves the finding, fixes the defect, and requests a new independent review bound to the updated source; it does not invent a verdict or bypass the rejected operation. + +Potential improvements: + +- Publish the finding severity vocabulary as a closed tool-schema enum, matching Backend validation, rather than relying on a free-form string. +- Clarify the single-call policy for an unambiguously rejected validation request that committed no event. Keep transport ambiguity, consumed capabilities, and source movement distinct from input validation; any retry permission must come from the authority contract, not the caller's inference. +- Return explicit committed-event/capability-consumption evidence on rejection, so a caller can distinguish an absent verdict from an ambiguous write without inspecting storage. + +These are observed workflow barriers and proposals, not changes to the review authority contract or implementation requirements of T-696. The original rejected request and reproduction are preserved in the parent-observed Reviewer session and `target/review/T-696/review-submission-blocker.md`. diff --git a/tools/web-ux/browser-tests/console_live_admission_shell_test.ts b/tools/web-ux/browser-tests/console_live_admission_shell_test.ts index 4a56a8404..afabf87a8 100644 --- a/tools/web-ux/browser-tests/console_live_admission_shell_test.ts +++ b/tools/web-ux/browser-tests/console_live_admission_shell_test.ts @@ -306,7 +306,7 @@ Deno.test("production attachment upload failure and in-flight cancellation never ); }, true)); -Deno.test("production upload retry retains upload ID and late completion cannot resurrect deleted reservation", () => +Deno.test("production upload retry retains upload ID and deleted pending upload cancels without blocking Submit", () => fixture(async (page, base, errors, caseOutput) => { await control(base, { upload: "fail" }); await attach(page, "retry.md"); @@ -335,41 +335,35 @@ Deno.test("production upload retry retains upload ID and late completion cannot const held = await eventually(base, (value) => value.held_uploads.length === 1); const lateId = held.held_uploads[0]; await page.locator(".composer-typed-chip").filter({ hasText: "Uploading late.md" }).waitFor(); - // Delete only the reservation, without aborting its real pending upload. + // Keyboard deletion cancels staging; a separate Remove action is not needed. await page.locator(".cm-content").focus(); await page.keyboard.press("Control+End"); await page.keyboard.press("Backspace"); assertEquals(await page.locator(".composer-typed-chip").count(), 0); await page.screenshot({ path: join(caseOutput, "late-upload-deleted.png") }); - const response = page.waitForResponse((response) => - response.request().method() === "PUT" && response.url().endsWith(lateId) - ); - await control(base, { release_uploads: true }); - assertEquals((await response).status(), 200); - const late = await eventually( - base, - (value) => value.uploads.includes(lateId) && value.held_uploads.length === 0, - ); - assertEquals( - await page.locator(".composer-typed-chip").count(), - 0, - "late success cannot resurrect deleted atom", - ); - assertEquals( - late.methods.filter((method) => method.method === "submit").length, - 1, - "upload/retry/late completion never submits", - ); - // Deleted resources remain undo-owned until the next accepted draft boundary. - await page.screenshot({ path: join(caseOutput, "late-upload-completed.png") }); - await draft(page, "finish late deletion"); + // Submit immediately, without waiting for upload completion or cancellation HTTP. + await page.keyboard.type("finish late deletion"); await page.keyboard.press("Control+Enter"); - const completed = await eventually(base, (value) => value.deletes.includes(lateId)); - assertEquals(completed.methods.filter((method) => method.method === "submit").length, 2); + const completed = await eventually( + base, + (value) => + value.deletes.includes(lateId) && value.held_uploads.length === 0 && + value.methods.filter((method) => method.method === "submit").length === 2, + ); + assertEquals(await page.locator(".composer-attachments").count(), 0); assertEquals( completed.methods.filter((method) => method.method === "submit").at(-1)?.params?.input, [{ kind: "text", content: "finish late deletion" }], ); + // A delayed fixture release after cancellation cannot revive a resource or intent. + // Component tests separately inject a late completion callback against the tombstone. + await page.waitForFunction(() => document.querySelector(".cm-content")?.textContent === ""); + await control(base, { release_uploads: true }); + const late = await state(base); + assert(!late.uploads.includes(lateId)); + assertEquals(late.methods.filter((method) => method.method === "submit").length, 2); + assertEquals(await page.locator(".composer-typed-chip").count(), 0); + await page.screenshot({ path: join(caseOutput, "late-upload-completed.png") }); assert(errors.every((error) => /503/.test(error)), JSON.stringify(errors)); await Deno.writeTextFile( join(caseOutput, "retry-late-result.json"), @@ -381,6 +375,57 @@ Deno.test("production upload retry retains upload ID and late completion cannot ); }, true)); +Deno.test("production pending-upload Delete, range, cut and replacement cancel staging and allow immediate text Submit", () => + fixture(async (page, base, errors, caseOutput) => { + await control(base, { upload: "hold" }); + const results = []; + for (const [index, operation] of ["Delete", "range", "cut", "replace"].entries()) { + await attach(page, `keyboard-${operation}.md`); + const held = await eventually(base, (value) => value.held_uploads.length === 1); + const uploadId = held.held_uploads[0]; + await page.locator(".composer-typed-chip").filter({ + hasText: `Uploading keyboard-${operation}.md`, + }).waitFor(); + await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-pending.png`) }); + await page.locator(".cm-content").focus(); + if (operation === "Delete") { + await page.keyboard.press("Control+Home"); + await page.keyboard.press("Delete"); + } else { + await page.keyboard.press("Control+a"); + if (operation === "range") await page.keyboard.press("Backspace"); + else if (operation === "cut") await page.keyboard.press("Control+x"); + else await page.keyboard.type("replacement "); + } + assertEquals(await page.locator(".composer-typed-chip").count(), 0); + await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-deleted.png`) }); + const text = `${operation === "replace" ? "replacement " : ""}text after ${operation}`; + await page.keyboard.type(`text after ${operation}`); + await page.keyboard.press("Control+Enter"); + const accepted = await eventually( + base, + (value) => + value.deletes.includes(uploadId) && value.held_uploads.length === 0 && + value.methods.filter((method) => method.method === "submit").length === index + 1, + ); + assertEquals( + accepted.methods.filter((method) => method.method === "submit").at(-1)?.params?.input, + [{ kind: "text", content: text }], + ); + assertEquals(await page.locator(".composer-attachments").count(), 0); + await page.waitForFunction(() => document.querySelector(".cm-content")?.textContent === ""); + await control(base, { release_uploads: true }); + assert(!(await state(base)).uploads.includes(uploadId)); + await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-accepted.png`) }); + results.push({ operation, held, accepted }); + } + assertEquals(errors, []); + await Deno.writeTextFile( + join(caseOutput, "keyboard-cancellation-result.json"), + JSON.stringify({ status: "pass", results, errors }, null, 2), + ); + }, true)); + Deno.test("production selected invocation preserves multiple positional typed arguments", () => fixture(async (page, base, errors, caseOutput) => { await draft(page, "/rev"); diff --git a/web/workspace/src/lib/workspace/console/ComposerInput.browser.spec.ts b/web/workspace/src/lib/workspace/console/ComposerInput.browser.spec.ts index 8e30a1786..355b0da6b 100644 --- a/web/workspace/src/lib/workspace/console/ComposerInput.browser.spec.ts +++ b/web/workspace/src/lib/workspace/console/ComposerInput.browser.spec.ts @@ -3,7 +3,7 @@ import { cleanup, fireEvent, render, waitFor } from "@testing-library/svelte"; import { afterEach, expect, test, vi } from "vitest"; import { EditorView } from "@codemirror/view"; import type { FeatureInvocationDescriptor, Segment } from "#lib/generated/protocol.ts"; -import { undo } from "@codemirror/commands"; +import { redo, undo } from "@codemirror/commands"; import ComposerInput from "./ComposerInput.svelte"; afterEach(cleanup); @@ -188,6 +188,52 @@ test("restored chip descriptor edits reject same-draft cursor ABA", async () => expect(ui.component.snapshot().segments).toEqual([invocationSegment]); }); +test.each(["Backspace", "Delete", "range", "cut", "replace"])("pending upload %s cancels its reservation before late completion and Undo", async (operation) => { + const oncancelupload = vi.fn(); + const ui = render(ComposerInput, { historyScope: `upload-delete-${operation}`, oncancelupload }); + const cm = editor(ui.container); + type(cm, "before after", 7); + const reservation = ui.component.reserveUpload("report.md")!; + const from = 7, to = cm.state.selection.main.head; + if (operation === "Backspace" || operation === "Delete") { + cm.dispatch({ selection: { anchor: operation === "Backspace" ? to : from } }); + await fireEvent.keyDown(cm.contentDOM, { key: operation }); + } else if (operation === "replace") { + cm.dispatch({ changes: { from, to, insert: "replacement" }, userEvent: "input" }); + } else { + // happy-dom emits selectionchange synchronously while CodeMirror updates a + // focused DOM range. Exercise the transaction/cut handler unfocused here; + // the Chromium suite covers focused keyboard range deletion. + cm.contentDOM.blur(); + cm.dispatch({ selection: { anchor: from, head: to } }); + if (operation === "cut") await fireEvent.cut(cm.contentDOM, { clipboardData: { setData: vi.fn() } }); + else await fireEvent.keyDown(cm.contentDOM, { key: "Backspace" }); + } + expect(oncancelupload).toHaveBeenCalledExactlyOnceWith(reservation); + expect(ui.component.completeUpload(reservation, uploadedSegment)).toBe(false); + expect(ui.component.snapshot().segments.some((segment) => segment.kind === "unknown" || segment.kind === "uploaded_file")).toBe(false); + undo(cm); + expect(ui.component.completeUpload(reservation, uploadedSegment)).toBe(false); + expect(ui.component.snapshot().segments.some((segment) => segment.kind === "unknown" || segment.kind === "uploaded_file")).toBe(false); + redo(cm); + ui.component.clear(); + expect(oncancelupload).toHaveBeenCalledOnce(); +}); + +test.each(["clear", "restore", "unmount"])("pending upload %s boundary cancels once without acquiring late staged resources", (boundary) => { + const oncancelupload = vi.fn(); + const onremoveatom = vi.fn(); + const ui = render(ComposerInput, { historyScope: `upload-boundary-${boundary}`, oncancelupload, onremoveatom }); + const reservation = ui.component.reserveUpload("report.md")!; + const complete = ui.component.completeUpload; + if (boundary === "clear") ui.component.clear(); + else if (boundary === "restore") ui.component.restoreSegments([invocationSegment]); + else ui.unmount(); + expect(oncancelupload).toHaveBeenCalledExactlyOnceWith(reservation); + expect(complete(reservation, uploadedSegment)).toBe(false); + expect(onremoveatom).not.toHaveBeenCalled(); +}); + test("cancelled upload cannot become sendable via late completion or Undo", async () => { const onremoveatom = vi.fn(); const ui = render(ComposerInput, { historyScope: "upload-cancel-late", onremoveatom }); diff --git a/web/workspace/src/lib/workspace/console/ComposerInput.svelte b/web/workspace/src/lib/workspace/console/ComposerInput.svelte index ff048e37f..c60639e85 100644 --- a/web/workspace/src/lib/workspace/console/ComposerInput.svelte +++ b/web/workspace/src/lib/workspace/console/ComposerInput.svelte @@ -80,6 +80,7 @@ signal: AbortSignal, ) => Promise; onremoveatom?: (segment: Segment) => void; + oncancelupload?: (reservation: number) => void; onclientadapter?: (descriptor: FeatureInvocationDescriptor) => void; } @@ -98,6 +99,7 @@ resolveFeatureCompletions, resolveFeatureArgumentCompletions, onremoveatom, + oncancelupload, onclientadapter, }: Props = $props(); @@ -105,6 +107,7 @@ let view: EditorView | null = null; let editorExtensions: Extension[] = []; const stagedTypedAtoms = new Map(); + const pendingUploads = new Set(); let composerHistory = new ComposerHistory(); let restoringHistory = false; let nextPasteId = 1; @@ -411,9 +414,19 @@ create: () => new Map(), update(registry, transaction) { const additions = transaction.effects.filter((effect) => effect.is(registerTypedAtom)); - if (!additions.length) return registry; + if (!additions.length && !transaction.docChanged) return registry; const next = new Map(registry); for (const addition of additions) next.set(addition.value.key, addition.value.atom); + if (transaction.docChanged) { + const live = new Set(composerTypedAtoms(transaction.newDoc.toString(), next).map((atom) => atom.key)); + for (const atom of composerTypedAtoms(transaction.startState.doc.toString(), registry)) { + if (atom.uploadReservation && !live.has(atom.key)) { + // Cancellation is part of the editor transaction, not a later callback. + // Undo restores text only; it cannot regain this upload reservation. + next.set(atom.key, { segment: { kind: "text", content: "" }, label: "Removed upload" }); + } + } + } return next; }, }); @@ -457,7 +470,14 @@ EditorView.decorations.of((currentView) => typedDecorations(currentView.state)), EditorView.atomicRanges.of((currentView) => typedDecorations(currentView.state)), ]; - // Deleted uploads remain staged while Undo can still restore their atoms. + function releasePendingUploads(): void { + const cancelled = [...pendingUploads]; + pendingUploads.clear(); + for (const key of cancelled) oncancelupload?.(key); + } + + // Completed uploads remain staged while Undo can still restore their atoms. + // Pending reservations are cancelled immediately, not retained as Undo leases. // Release abandoned resources at a draft boundary; accepted live atoms belong to the host. function releaseStagedAtoms(accepted = false): void { const live = accepted && view ? new Set(composerTypedAtoms( @@ -481,7 +501,7 @@ function insertTypedSegment( segment: Segment, - options: { from?: number; to?: number; cleanup?: boolean; label?: string } = {}, + options: { from?: number; to?: number; cleanup?: boolean; label?: string; uploadReservation?: boolean } = {}, ): number | null { if (!view || view.state.readOnly) return null; const selection = view.state.selection.main; @@ -489,13 +509,14 @@ const to = options.to ?? selection.to; const key = nextTypedKey++; if (options.cleanup) stagedTypedAtoms.set(key, segment); + if (options.uploadReservation) pendingUploads.add(key); const token = composerTypedToken(key); view.dispatch({ changes: { from, to, insert: token }, selection: EditorSelection.cursor(from + token.length), effects: registerTypedAtom.of({ key, - atom: { segment, label: options.label ?? typedAtomLabel(segment), cleanup: options.cleanup }, + atom: { segment, label: options.label ?? typedAtomLabel(segment), cleanup: options.cleanup, uploadReservation: options.uploadReservation }, }), annotations: isolateHistory.of("full"), userEvent: "input.complete", @@ -784,6 +805,14 @@ spellcheck: "true", }), EditorView.updateListener.of((update) => { + // StateField tombstones deleted reservations before notifying the + // adapter. The callback must not dispatch a nested editor update. + for (const key of pendingUploads) { + if (!update.state.field(typedRegistry).get(key)?.uploadReservation) { + pendingUploads.delete(key); + oncancelupload?.(key); + } + } if (update.docChanged || update.selectionSet) { editorGeneration++; // Invalidate synchronously: an A→B→A edit/cursor cycle in one @@ -881,6 +910,7 @@ return () => { closeCompletion(); + releasePendingUploads(); releaseStagedAtoms(); editVersion++; view?.destroy(); @@ -940,11 +970,14 @@ } export function reserveUpload(fileName: string): number | null { - return insertTypedSegment({ kind: "unknown" }, { label: `Uploading ${fileName}` }); + return insertTypedSegment({ kind: "unknown" }, { label: `Uploading ${fileName}`, uploadReservation: true }); } export function completeUpload(key: number, segment: Segment): boolean { - if (!view || view.state.field(typedRegistry).get(key)?.segment.kind !== "unknown") return false; + if (!view || !pendingUploads.has(key) || + !view.state.field(typedRegistry).get(key)?.uploadReservation || + !composerTypedAtoms(view.state.doc.toString(), view.state.field(typedRegistry)).some((atom) => atom.key === key)) return false; + pendingUploads.delete(key); stagedTypedAtoms.set(key, segment); view.dispatch({ effects: registerTypedAtom.of({ key, atom: { segment, label: typedAtomLabel(segment), cleanup: true }, @@ -968,6 +1001,7 @@ export function clear(acceptedTypedResources = false, retainStagedResources = false): void { if (!view) return; + releasePendingUploads(); if (!retainStagedResources) releaseStagedAtoms(acceptedTypedResources); editVersion++; closeCompletion(); diff --git a/web/workspace/src/lib/workspace/console/composer-draft.ts b/web/workspace/src/lib/workspace/console/composer-draft.ts index fd413e791..a1ca24b29 100644 --- a/web/workspace/src/lib/workspace/console/composer-draft.ts +++ b/web/workspace/src/lib/workspace/console/composer-draft.ts @@ -32,6 +32,8 @@ export interface ComposerTypedSegment { label: string; /** Whether deleting this unsent atom must release client-staged state. */ cleanup?: boolean; + /** Client-only reservation whose removal cancels an in-progress upload. */ + uploadReservation?: boolean; } export interface ComposerTypedAtom extends ComposerTypedSegment { diff --git a/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte index 2459cecb9..f46361f8b 100644 --- a/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte @@ -1091,6 +1091,17 @@ ); } + function cancelComposerUpload(reservation: number): void { + const attachment = attachments.find((candidate) => + uploadReservations.get(candidate.id) === reservation); + if (!attachment) return; + // The editor already cancelled/tombstoned its reservation. Do not call + // cancelUpload here: this may run inside a CodeMirror update listener. + uploadReservations.delete(attachment.id); + attachments = attachments.filter((candidate) => candidate.id !== attachment.id); + attachment.request?.abort(); + } + function completeAttachmentUpload( attachment: ComposerAttachment, reference: NonNullable, @@ -2177,6 +2188,7 @@ resolveFeatureArgumentCompletions={(context, prefix, signal) => featureArgumentCompletionLane(context).request(prefix, signal)} onremoveatom={(segment) => void releaseComposerAtom(segment)} + oncancelupload={cancelComposerUpload} onclientadapter={(descriptor) => { if (descriptor.client_adapter === "attachment") fileInput?.click(); }}