From 7dcafb072656c52ee891f24a07ffaa41cf661e00 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Thu, 2 Jul 2026 16:00:15 +0800 Subject: [PATCH] fix: close bug-hunter stale-state paths Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 22 ++ src/app/api/documents/[id]/reindex/route.ts | 23 ++- src/app/api/documents/bulk/reindex/route.ts | 23 ++- .../api/ingestion/jobs/[id]/retry/route.ts | 26 ++- src/app/api/upload/route.ts | 30 ++- src/components/forms/form-detail-page.tsx | 17 +- tests/forms-clipboard-fallback.test.ts | 14 ++ tests/private-access-routes.test.ts | 189 +++++++++++++++++- tests/worker-visual-capture.test.ts | 6 + worker/main.ts | 8 +- 10 files changed, 343 insertions(+), 15 deletions(-) create mode 100644 tests/forms-clipboard-fallback.test.ts diff --git a/AGENTS.md b/AGENTS.md index 35b5468330..e9dcda623f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,28 @@ After setup: + + +## Bug-hunter shortcut + +When the user types exactly `bug-hunter` as the entire task message, after trimming surrounding whitespace, treat it as a shortcut for targeted defect discovery. + +Execution rules: + +- Invoke the `bug-hunter` skill first. +- Prioritize reproducible defects over code style, naming, or formatting feedback. +- Trace realistic failure paths: invalid input, empty states, retries, race/concurrency issues, stale state/cache, network/auth failures, permissions, and boundary values. +- For each finding, include trigger, expected behavior, actual risk, and the smallest proof (or targeted test/check) that would catch it. +- If no high-confidence defect is found, explicitly state that and list the most likely residual risk area. + +Scope and safety: + +- Keep the hunt scoped to code touched by the user request unless the defect clearly crosses module boundaries. +- Do not make broad refactors while hunting; propose minimal fixes for confirmed issues. +- Run the smallest focused verification for each confirmed defect, then expand only if needed. + + + # Local server safety diff --git a/src/app/api/documents/[id]/reindex/route.ts b/src/app/api/documents/[id]/reindex/route.ts index 88c4b85f84..59dcec8bd5 100644 --- a/src/app/api/documents/[id]/reindex/route.ts +++ b/src/app/api/documents/[id]/reindex/route.ts @@ -101,7 +101,7 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: const { data: document, error: documentError } = await supabase .from("documents") - .select("id,owner_id,title,file_name,source_path,import_batch_id,status,metadata") + .select("id,owner_id,title,file_name,source_path,import_batch_id,status,error_message,page_count,chunk_count,image_count,metadata") .eq("id", id) .eq("owner_id", user.id) .maybeSingle(); @@ -170,6 +170,15 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: } const atomicReindex = isAtomicReindexCandidate(document); + const rollbackDocumentPayload = atomicReindex + ? { error_message: document.error_message ?? null } + : { + status: document.status ?? null, + error_message: document.error_message ?? null, + page_count: document.page_count ?? 0, + chunk_count: document.chunk_count ?? 0, + image_count: document.image_count ?? 0, + }; const { error: updateError } = await supabase .from("documents") .update( @@ -194,7 +203,17 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: .select() .single(); - if (jobError) throw new Error(jobError.message); + if (jobError) { + const { error: rollbackError } = await supabase + .from("documents") + .update(rollbackDocumentPayload) + .eq("id", id) + .eq("owner_id", user.id); + if (rollbackError) { + throw new Error(`Failed to enqueue reindex job: ${jobError.message}; rollback failed: ${rollbackError.message}`); + } + throw new Error(jobError.message); + } return NextResponse.json({ job }, { status: 201 }); } catch (error) { if (error instanceof AuthenticationError) return unauthorizedResponse(); diff --git a/src/app/api/documents/bulk/reindex/route.ts b/src/app/api/documents/bulk/reindex/route.ts index 864878678e..28286c58bf 100644 --- a/src/app/api/documents/bulk/reindex/route.ts +++ b/src/app/api/documents/bulk/reindex/route.ts @@ -91,7 +91,7 @@ export async function POST(request: Request) { const documentIds = Array.from(new Set(parsed.documentIds)); const { data: documents, error: documentError } = await supabase .from("documents") - .select("id,owner_id,title,file_name,source_path,import_batch_id,status,metadata") + .select("id,owner_id,title,file_name,source_path,import_batch_id,status,error_message,page_count,chunk_count,image_count,metadata") .eq("owner_id", user.id) .in("id", documentIds); if (documentError) throw new Error(documentError.message); @@ -166,6 +166,15 @@ export async function POST(request: Request) { } const atomicReindex = isAtomicReindexCandidate(document); + const rollbackDocumentPayload = atomicReindex + ? { error_message: document.error_message ?? null } + : { + status: document.status ?? null, + error_message: document.error_message ?? null, + page_count: document.page_count ?? 0, + chunk_count: document.chunk_count ?? 0, + image_count: document.image_count ?? 0, + }; const { error: updateError } = await supabase .from("documents") .update( @@ -188,7 +197,17 @@ export async function POST(request: Request) { }) .select("id") .single(); - if (jobError) throw new Error(jobError.message); + if (jobError) { + const { error: rollbackError } = await supabase + .from("documents") + .update(rollbackDocumentPayload) + .eq("id", document.id) + .eq("owner_id", user.id); + if (rollbackError) { + throw new Error(`Failed to enqueue bulk reindex job: ${jobError.message}; rollback failed: ${rollbackError.message}`); + } + throw new Error(jobError.message); + } results.push({ documentId: document.id, mode: parsed.mode, ok: true, jobId: job.id }); } catch (error) { results.push({ diff --git a/src/app/api/ingestion/jobs/[id]/retry/route.ts b/src/app/api/ingestion/jobs/[id]/retry/route.ts index 5510fd1b24..b1c7e7ff4d 100644 --- a/src/app/api/ingestion/jobs/[id]/retry/route.ts +++ b/src/app/api/ingestion/jobs/[id]/retry/route.ts @@ -16,7 +16,9 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: const { data: job, error: jobError } = await supabase .from("ingestion_jobs") - .select("id,document_id,batch_id,status,locked_at,documents!inner(owner_id)") + .select( + "id,document_id,batch_id,status,stage,progress,error_message,attempt_count,max_attempts,locked_at,locked_by,next_run_at,completed_at,documents!inner(owner_id)", + ) .eq("id", id) .eq("documents.owner_id", user.id) .maybeSingle(); @@ -74,7 +76,27 @@ export async function POST(request: Request, { params }: { params: Promise<{ id: .update({ status: "queued", error_message: null }) .eq("id", job.document_id) .eq("owner_id", user.id); - if (documentError) throw new Error(documentError.message); + if (documentError) { + const { error: rollbackError } = await supabase + .from("ingestion_jobs") + .update({ + status: job.status, + stage: job.stage, + progress: job.progress, + error_message: job.error_message, + attempt_count: job.attempt_count, + max_attempts: job.max_attempts, + locked_at: job.locked_at, + locked_by: job.locked_by, + next_run_at: job.next_run_at, + completed_at: job.completed_at, + }) + .eq("id", id); + if (rollbackError) { + throw new Error(`${documentError.message}; failed to roll back retried job state: ${rollbackError.message}`); + } + throw new Error(documentError.message); + } return NextResponse.json({ job: data }); } catch (error) { diff --git a/src/app/api/upload/route.ts b/src/app/api/upload/route.ts index 3bd1d5150c..e264ffbac0 100644 --- a/src/app/api/upload/route.ts +++ b/src/app/api/upload/route.ts @@ -24,6 +24,8 @@ const uploadMetadataSchema = z export async function POST(request: Request) { let supabase: ReturnType | null = null; let uploadedPath: string | null = null; + let insertedDocumentId: string | null = null; + let insertedDocumentOwnerId: string | null = null; try { supabase = createAdminClient(); @@ -132,6 +134,8 @@ export async function POST(request: Request) { .single(); if (documentError) throw new Error(documentError.message); + insertedDocumentId = documentId; + insertedDocumentOwnerId = user.id; const { data: job, error: jobError } = await supabase .from("ingestion_jobs") @@ -146,7 +150,19 @@ export async function POST(request: Request) { .select() .single(); - if (jobError) throw new Error(jobError.message); + if (jobError) { + const { error: rollbackDocumentError } = await supabase + .from("documents") + .delete() + .eq("id", documentId) + .eq("owner_id", user.id); + if (rollbackDocumentError) { + throw new Error(`Failed to enqueue ingestion job: ${jobError.message}; rollback failed: ${rollbackDocumentError.message}`); + } + insertedDocumentId = null; + insertedDocumentOwnerId = null; + throw new Error(jobError.message); + } await writeAuditLog(supabase, { ownerId: user.id, @@ -158,6 +174,18 @@ export async function POST(request: Request) { return NextResponse.json({ document, job }, { status: 201 }); } catch (error) { + if (insertedDocumentId && insertedDocumentOwnerId && supabase) { + try { + await supabase.from("documents").delete().eq("id", insertedDocumentId).eq("owner_id", insertedDocumentOwnerId); + } catch (cleanupError) { + logger.error("Upload cleanup failed; document row may be orphaned", { + documentId: insertedDocumentId, + ownerId: insertedDocumentOwnerId, + message: cleanupError instanceof Error ? cleanupError.message : String(cleanupError), + }); + } + } + if (uploadedPath && supabase) { try { await supabase.storage.from(env.SUPABASE_DOCUMENT_BUCKET).remove([uploadedPath]); diff --git a/src/components/forms/form-detail-page.tsx b/src/components/forms/form-detail-page.tsx index 7db8180bff..c8c40991f5 100644 --- a/src/components/forms/form-detail-page.tsx +++ b/src/components/forms/form-detail-page.tsx @@ -69,8 +69,12 @@ function readSavedForms() { async function copyText(value: string) { if (navigator.clipboard?.writeText) { - await navigator.clipboard.writeText(value); - return; + try { + await navigator.clipboard.writeText(value); + return; + } catch { + // Fall through to the legacy selection path for restricted browser contexts. + } } const textArea = document.createElement("textarea"); @@ -80,9 +84,12 @@ async function copyText(value: string) { textArea.style.opacity = "0"; document.body.appendChild(textArea); textArea.select(); - const copied = document.execCommand?.("copy"); - document.body.removeChild(textArea); - if (copied === false) throw new Error("copy command rejected"); + try { + const copied = document.execCommand?.("copy"); + if (copied === false) throw new Error("copy command rejected"); + } finally { + document.body.removeChild(textArea); + } } function chipToneClass(tone: ServiceChipTone | null | undefined) { diff --git a/tests/forms-clipboard-fallback.test.ts b/tests/forms-clipboard-fallback.test.ts new file mode 100644 index 0000000000..a41b963293 --- /dev/null +++ b/tests/forms-clipboard-fallback.test.ts @@ -0,0 +1,14 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +const source = readFileSync(new URL("../src/components/forms/form-detail-page.tsx", import.meta.url), "utf8"); + +describe("form detail clipboard fallback", () => { + it("falls back to selection-copy when clipboard.writeText rejects", () => { + expect(source).toContain("if (navigator.clipboard?.writeText)"); + expect(source).toContain("await navigator.clipboard.writeText(value)"); + expect(source).toContain("Fall through to the legacy selection path for restricted browser contexts."); + expect(source).toContain("document.execCommand?.(\"copy\")"); + expect(source).toContain("finally {\n document.body.removeChild(textArea);\n }"); + }); +}); diff --git a/tests/private-access-routes.test.ts b/tests/private-access-routes.test.ts index ce2291fc7d..f6861e1bd5 100644 --- a/tests/private-access-routes.test.ts +++ b/tests/private-access-routes.test.ts @@ -714,7 +714,7 @@ describe("private document API access", () => { id: documentId, title: "Existing guideline", file_name: "guideline.pdf", - status: "indexed", + status: "failed", page_count: 4, chunk_count: 8, image_count: 1, @@ -1047,13 +1047,70 @@ describe("private document API access", () => { expect(client.rpc).not.toHaveBeenCalled(); }); + it("rolls back the job retry when document queue status update fails", async () => { + const previousJob = { + id: "job-1", + document_id: documentId, + batch_id: null, + status: "failed", + stage: "failed", + progress: 42, + error_message: "OCR failed", + attempt_count: 2, + max_attempts: 3, + locked_at: null, + locked_by: null, + next_run_at: null, + completed_at: "2024-01-01T00:00:00.000Z", + }; + + let ingestionUpdateCount = 0; + const client = createSupabaseMock((call) => { + if (call.table === "ingestion_jobs" && call.operation === "select") { + return ok(previousJob); + } + if (call.table === "ingestion_jobs" && call.operation === "update") { + ingestionUpdateCount += 1; + if (ingestionUpdateCount === 1) { + return ok({ id: "job-1", document_id: documentId, status: "pending" }); + } + return ok({ id: "job-1" }); + } + if (call.table === "documents" && call.operation === "update") return fail("documents update failed"); + return ok([]); + }); + mockRuntime(client); + const { POST } = await import("../src/app/api/ingestion/jobs/[id]/retry/route"); + + const response = await POST(authenticatedRequest(`/api/ingestion/jobs/job-1/retry`, { method: "POST" }), { + params: Promise.resolve({ id: "job-1" }), + }); + + expect(response.status).toBe(400); + expect(String((await payload(response)).error)).toBe("Request could not be completed."); + const jobUpdates = client.calls.filter((call) => call.table === "ingestion_jobs" && call.operation === "update"); + expect(jobUpdates).toHaveLength(2); + expect(jobUpdates[1]?.updatePayload).toEqual({ + status: previousJob.status, + stage: previousJob.stage, + progress: previousJob.progress, + error_message: previousJob.error_message, + attempt_count: previousJob.attempt_count, + max_attempts: previousJob.max_attempts, + locked_at: previousJob.locked_at, + locked_by: previousJob.locked_by, + next_run_at: previousJob.next_run_at, + completed_at: previousJob.completed_at, + }); + }); + it("runs enrichment-only reindex for owned indexed documents using generic metadata", async () => { const document = { id: documentId, owner_id: userId, title: "Future Uploaded Protocol", file_name: "future-upload.pdf", - source_path: null, + source_path: "legacy/imports/rollback.pdf", import_batch_id: null, metadata: { existing: true }, }; @@ -1472,6 +1529,125 @@ describe("private document API access", () => { expect(client.calls.some((call) => call.table === "documents" && call.operation === "update")).toBe(false); }); + it("rolls back single-document queue mutation when full reindex job enqueue fails", async () => { + const document = { + id: documentId, + owner_id: userId, + title: "Rollback Protocol", + file_name: "rollback.pdf", + source_path: null, + import_batch_id: null, + status: "failed", + error_message: "older failure", + page_count: 12, + chunk_count: 34, + image_count: 2, + metadata: {}, + }; + const client = createSupabaseMock((call) => { + if (call.table === "documents" && call.operation === "select") return ok(document); + if (call.table === "import_batches") return ok([]); + if (call.table === "ingestion_jobs" && call.operation === "select") return ok([]); + if (call.table === "ingestion_jobs" && call.operation === "insert") return fail("job insert failed"); + if (call.table === "documents" && call.operation === "update") return ok([]); + return ok([]); + }); + mockRuntime(client); + const { POST } = await import("../src/app/api/documents/[id]/reindex/route"); + + const response = await POST( + authenticatedRequest(`/api/documents/${documentId}/reindex`, { + method: "POST", + body: JSON.stringify({ mode: "full" }), + }), + { params: Promise.resolve({ id: documentId }) }, + ); + const body = await payload(response); + const documentUpdates = client.calls.filter((call) => call.table === "documents" && call.operation === "update"); + + expect(response.status).toBe(400); + expect(body).toEqual({ error: "Request could not be completed." }); + expect(documentUpdates).toHaveLength(2); + expect(documentUpdates[0]?.updatePayload).toEqual({ + status: "queued", + error_message: null, + page_count: 0, + chunk_count: 0, + image_count: 0, + }); + expect(documentUpdates[1]?.updatePayload).toEqual({ + status: "failed", + error_message: "older failure", + page_count: 12, + chunk_count: 34, + image_count: 2, + }); + }); + + it("rolls back per-document queue mutation when bulk full reindex enqueue fails", async () => { + const document = { + id: documentId, + owner_id: userId, + title: "Bulk Rollback Protocol", + file_name: "bulk-rollback.pdf", + source_path: "legacy/imports/bulk-rollback.pdf", + import_batch_id: null, + status: "failed", + error_message: "older failure", + page_count: 3, + chunk_count: 8, + image_count: 1, + metadata: {}, + }; + const client = createSupabaseMock((call) => { + if (call.table === "documents" && call.operation === "select") return ok([document]); + if (call.table === "import_batches") return ok([]); + if (call.table === "ingestion_jobs" && call.operation === "select") return ok([]); + if (call.table === "documents" && call.operation === "update") return ok([]); + if (call.table === "ingestion_jobs" && call.operation === "insert") return fail("bulk job insert failed"); + return ok([]); + }); + mockRuntime(client, { invalidateRagCachesForOwner: vi.fn() }); + const { POST } = await import("../src/app/api/documents/bulk/reindex/route"); + + const response = await POST( + authenticatedRequest("/api/documents/bulk/reindex", { + method: "POST", + body: JSON.stringify({ documentIds: [documentId], mode: "full" }), + }), + ); + const body = await payload(response); + const documentUpdates = client.calls.filter((call) => call.table === "documents" && call.operation === "update"); + + expect(response.status).toBe(200); + expect(body).toMatchObject({ + ok: false, + results: [ + { + documentId, + mode: "full", + ok: false, + error: "bulk job insert failed", + }, + ], + }); + expect(documentUpdates).toHaveLength(2); + expect(documentUpdates[0]?.updatePayload).toEqual({ + status: "queued", + error_message: null, + page_count: 0, + chunk_count: 0, + image_count: 0, + }); + expect(documentUpdates[1]?.updatePayload).toEqual({ + status: "failed", + error_message: "older failure", + page_count: 3, + chunk_count: 8, + image_count: 1, + }); + }); + it("cleans up uploaded storage when document insert fails", async () => { const client = createSupabaseMock((call) => call.table === "documents" && call.operation === "insert" ? fail("document insert failed") : ok([]), @@ -1518,6 +1694,15 @@ describe("private document API access", () => { expect(response.status).toBe(400); expect(client.storageMocks.remove).toHaveBeenCalledWith([uploadPath]); + expect( + client.calls.some( + (call) => + call.table === "documents" && + call.operation === "delete" && + call.filters.some((filter) => filter.column === "id" && typeof filter.value === "string") && + call.filters.some((filter) => filter.column === "owner_id" && filter.value === userId), + ), + ).toBe(true); }); it("does not return document details for an unowned document", async () => { diff --git a/tests/worker-visual-capture.test.ts b/tests/worker-visual-capture.test.ts index c910facc30..c882906ede 100644 --- a/tests/worker-visual-capture.test.ts +++ b/tests/worker-visual-capture.test.ts @@ -54,6 +54,12 @@ describe("worker visual capture hardening", () => { expect(workerSource).toContain("indexing_v3_agent_repair_reason: strictCompletionRepairReason"); }); + it("logs fallback import-batch update failures when RPC-based refresh is unavailable", () => { + expect(workerSource).toContain("Import batch status fallback update failed"); + expect(workerSource).toContain("update import batch status fallback"); + expect(workerSource).toContain("const { error: fallbackUpdateError } = await supabase"); + }); + it("invalidates stale image caption cache entries by policy, prompt, and context versions", () => { expect(workerSource).toContain('const imageCaptionCacheVersion = "clinical-image-caption-cache-v2"'); expect(workerSource).toContain('const visionClassificationPromptVersion = "clinical-image-classification-v1"'); diff --git a/worker/main.ts b/worker/main.ts index 4718f1b2ca..c2a3bc1351 100644 --- a/worker/main.ts +++ b/worker/main.ts @@ -171,7 +171,7 @@ async function updateBatch(batchId: string | null) { const failed = data.filter((job) => job.status === "failed").length; const status = terminalBatchStatus({ queued, processing, failed }); - await supabase + const { error: fallbackUpdateError } = await supabase .from("import_batches") .update({ status, @@ -179,6 +179,12 @@ async function updateBatch(batchId: string | null) { completed_at: status === "processing" ? null : new Date().toISOString(), }) .eq("id", batchId); + if (fallbackUpdateError) { + console.warn( + "Import batch status fallback update failed", + safeErrorLogDetails(supabaseStageError("update import batch status fallback", fallbackUpdateError)), + ); + } } async function completeJob(job: JobRow, stage: string) {