From ba8273c24ecc15b6b78bc9c100ae93790c0b7bf0 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:10:40 +0000 Subject: [PATCH 1/7] fix: harden ingestion worker leases and address review follow-ups - Pass workerId through complete/fail ingestion RPCs and skip counters when the lease was reclaimed by another worker. - Generate embedding payloads before delete/insert and swap fields inside a single transaction to avoid partial enrichment gaps. - Check private access before document readiness in DocumentViewer.summarize so signed-out users see the sign-in message. - Return skill descriptions separately from validateSkillCatalog instead of mutating canonical catalog entries during validation. Co-authored-by: BigSimmo --- scripts/list-database-skills.mjs | 7 +- src/components/DocumentViewer.tsx | 14 ++- supabase/functions/ingestion-worker/index.ts | 100 ++++++++++++------- tests/ingestion-edge-function-auth.test.ts | 5 + 4 files changed, 81 insertions(+), 45 deletions(-) diff --git a/scripts/list-database-skills.mjs b/scripts/list-database-skills.mjs index a0d4c766e..46e908bb8 100644 --- a/scripts/list-database-skills.mjs +++ b/scripts/list-database-skills.mjs @@ -77,6 +77,7 @@ export function validateSkillCatalog(catalog = loadSkillCatalog(), discovered = const aliasTargets = new Map(aliases); const discoveredByName = new Map(discovered.map((skill) => [skill.name, skill])); const discoveredNames = discovered.map((skill) => skill.name); + const descriptions = new Map(); for (const [label, names] of [ ["category", categoryNames], @@ -95,7 +96,7 @@ export function validateSkillCatalog(catalog = loadSkillCatalog(), discovered = else if (discoveredSkill.directory !== skill.name) { errors.push(`Canonical skill directory mismatch: ${skill.name} is in ${discoveredSkill.directory}`); } else { - skill.description = discoveredSkill.description; + descriptions.set(skill.name, discoveredSkill.description); } } @@ -146,7 +147,7 @@ export function validateSkillCatalog(catalog = loadSkillCatalog(), discovered = } } - return { errors, canonical, aliases, discovered }; + return { errors, canonical, aliases, discovered, descriptions }; } export function summarizeSkillDescription(description) { @@ -160,7 +161,7 @@ export function renderSkillCatalog(catalog = loadSkillCatalog(), discovered = di const validation = validateSkillCatalog(catalog, discovered); if (validation.errors.length) throw new Error(validation.errors.join("\n")); - const descriptions = new Map(validation.canonical.map((skill) => [skill.name, skill.description])); + const descriptions = validation.descriptions; const lines = [`Database skills (${validation.canonical.length})`, ""]; for (const category of catalog.categories) { lines.push(category.name); diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index 2bf5a660e..91d29e0af 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -2281,14 +2281,14 @@ export function DocumentViewer({ }, []); async function summarize() { - if (!canSummarizeDocument) { - setSummaryError("Load a source document before summarising."); - return; - } if (!canUsePrivateApis) { setSummaryError("Sign in before summarising private documents."); return; } + if (viewerState !== "ready" || loadingSummary) { + setSummaryError("Load a source document before summarising."); + return; + } const summaryMode = sourceSearch.trim().length === 0; const query = summaryMode ? documentSummaryQuestion : sourceSearch.trim(); const controller = new AbortController(); @@ -2408,7 +2408,11 @@ export function DocumentViewer({ : documentHomeHref; const usefulPageHref = (page: number) => documentPageHref(documentId, page); const canSummarizeDocument = viewerState === "ready" && !loadingSummary && canUsePrivateApis; - const summarizeTitle = canSummarizeDocument ? "Answer from this document" : "Load a source document before answering"; + const summarizeTitle = !canUsePrivateApis + ? "Sign in before answering from this document" + : viewerState !== "ready" || loadingSummary + ? "Load a source document before answering" + : "Answer from this document"; const pageByNumber = useMemo(() => new Map(pages.map((page) => [page.page_number, page])), [pages]); const chunkById = useMemo(() => new Map(chunks.map((chunk) => [chunk.id, chunk])), [chunks]); const selectedPage = pageByNumber.get(activePage) ?? pages[0]; diff --git a/supabase/functions/ingestion-worker/index.ts b/supabase/functions/ingestion-worker/index.ts index a9927baf9..b5ceb3f52 100644 --- a/supabase/functions/ingestion-worker/index.ts +++ b/supabase/functions/ingestion-worker/index.ts @@ -25,6 +25,15 @@ type ClaimedJob = { }; }; +type IngestionRpcResult = { + ok?: boolean; + reason?: string; +}; + +function isLeaseLost(result: IngestionRpcResult | null | undefined): boolean { + return result?.ok === false && result.reason === "lease_lost"; +} + const SUPABASE_DB_URL = Deno.env.get("SUPABASE_DB_URL"); if (!SUPABASE_DB_URL) throw new Error("SUPABASE_DB_URL is required"); @@ -151,38 +160,45 @@ async function upsertEmbeddingFields(job: ClaimedJob, summaryText: string): Prom { fieldType: "document_summary", content: summary.length > 0 ? summary : "Summary unavailable" }, ]; - await sql` - delete from public.document_embedding_fields - where document_id = ${docId}::uuid - and field_type = any(${entries.map((e) => e.fieldType)}::text[]) - `; - - for (const entry of entries) { - const embedding = await generateEmbedding(entry.content); - const contentHash = await sha256Hex(entry.content); - - await sql` - insert into public.document_embedding_fields ( - owner_id, - document_id, - source_chunk_id, - field_type, - content, - embedding, - metadata, - content_hash - ) values ( - ${ownerId}::uuid, - ${docId}::uuid, - null, - ${entry.fieldType}, - ${entry.content}, - ${JSON.stringify(embedding)}::vector, - ${JSON.stringify({ generated_by: "ingestion-worker", mode: "backfill" })}::jsonb, - ${contentHash} - ) + const prepared = await Promise.all( + entries.map(async (entry) => ({ + ...entry, + embedding: await generateEmbedding(entry.content), + contentHash: await sha256Hex(entry.content), + })), + ); + + await sql.begin(async (tx) => { + await tx` + delete from public.document_embedding_fields + where document_id = ${docId}::uuid + and field_type = any(${entries.map((entry) => entry.fieldType)}::text[]) `; - } + + for (const entry of prepared) { + await tx` + insert into public.document_embedding_fields ( + owner_id, + document_id, + source_chunk_id, + field_type, + content, + embedding, + metadata, + content_hash + ) values ( + ${ownerId}::uuid, + ${docId}::uuid, + null, + ${entry.fieldType}, + ${entry.content}, + ${JSON.stringify(entry.embedding)}::vector, + ${JSON.stringify({ generated_by: "ingestion-worker", mode: "backfill" })}::jsonb, + ${entry.contentHash} + ) + `; + } + }); } async function markEnrichmentMetadata(documentId: string): Promise { @@ -199,19 +215,22 @@ async function markEnrichmentMetadata(documentId: string): Promise { `; } -async function processJob(job: ClaimedJob): Promise { +async function processJob(job: ClaimedJob, workerId: string): Promise<"completed" | "lease_lost"> { const summary = await upsertDocumentSummary(job); await upsertEmbeddingFields(job, summary); await markEnrichmentMetadata(job.document_id); - await sql` + const [completionRow] = await sql>` select public.complete_ingestion_job( ${job.id}::uuid, ${job.document_id}::uuid, ${job.batch_id}::uuid, - ${"indexed + enrichment backfill"} + ${"indexed + enrichment backfill"}, + ${workerId} ) `; + + return isLeaseLost(completionRow?.complete_ingestion_job) ? "lease_lost" : "completed"; } Deno.serve(async (req: Request) => { @@ -243,7 +262,8 @@ Deno.serve(async (req: Request) => { for (const job of claimed) { try { - await processJob(job); + const outcome = await processJob(job, workerId); + if (outcome === "lease_lost") continue; processed += 1; } catch (error) { failed += 1; @@ -252,7 +272,7 @@ Deno.serve(async (req: Request) => { const shouldRetry = job.attempt_count < job.max_attempts; - await sql` + const [failureRow] = await sql>` select public.fail_or_retry_ingestion_job( ${job.id}::uuid, ${job.document_id}::uuid, @@ -261,9 +281,15 @@ Deno.serve(async (req: Request) => { ${"indexed"}, ${"enrichment backfill failed"}, ${message}, - ${new Date(Date.now() + 60_000).toISOString()}::timestamptz + ${new Date(Date.now() + 60_000).toISOString()}::timestamptz, + ${workerId} ) `; + + if (isLeaseLost(failureRow?.fail_or_retry_ingestion_job)) { + failed -= 1; + failures.pop(); + } } } diff --git a/tests/ingestion-edge-function-auth.test.ts b/tests/ingestion-edge-function-auth.test.ts index 65c735c4f..90f11396b 100644 --- a/tests/ingestion-edge-function-auth.test.ts +++ b/tests/ingestion-edge-function-auth.test.ts @@ -28,5 +28,10 @@ describe("ingestion-worker Edge Function authorization", () => { expect(source).toContain('if (req.method !== "POST")'); expect(source).toContain('hasServiceRoleAuthorization(req.headers.get("authorization"))'); expect(source.indexOf("hasServiceRoleAuthorization")).toBeLessThan(source.indexOf("public.claim_ingestion_jobs")); + expect(source).toContain("complete_ingestion_job("); + expect(source).toContain("fail_or_retry_ingestion_job("); + expect(source).toContain("${workerId}"); + expect(source).toContain('reason === "lease_lost"'); + expect(source).toContain("sql.begin"); }); }); From 97cf7bfd0c27b563405823c109ac1fa8958959e2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:23:50 +0000 Subject: [PATCH 2/7] fix: resolve python3 on Linux and polish ingestion worker lease handling - Use python3 by default on non-Windows when PYTHON_BIN is unset so PDF budget tests invoke the Python extractor instead of falling through to JS - Extract completeIngestionJob/failOrRetryIngestionJob helpers and track lease_lost explicitly in the worker response instead of failed -= 1 - Align pdf-extractor test python bin resolution with production code Co-authored-by: BigSimmo --- src/lib/extractors/document.ts | 7 +- supabase/functions/ingestion-worker/index.ts | 74 ++++++++++++-------- tests/ingestion-edge-function-auth.test.ts | 1 + tests/pdf-extractor.test.ts | 2 +- 4 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/lib/extractors/document.ts b/src/lib/extractors/document.ts index 590efed45..212ad234f 100644 --- a/src/lib/extractors/document.ts +++ b/src/lib/extractors/document.ts @@ -18,6 +18,11 @@ import { type PdfExtractionBudget, } from "@/lib/extractors/pdf-extraction-budget"; +function resolvePythonBin() { + if (process.env.PYTHON_BIN) return process.env.PYTHON_BIN; + return process.platform === "win32" ? "python" : "python3"; +} + const extractedPageSchema = z.object({ pageNumber: z.number().int().positive(), text: z.string(), @@ -91,7 +96,7 @@ export async function runPythonPdfExtractor( return new Promise((resolve, reject) => { const child = spawn( - process.env.PYTHON_BIN || "python", + resolvePythonBin(), [scriptPath, filePath, outputDir, outputJsonPath, budgetPath], { cwd: process.cwd(), diff --git a/supabase/functions/ingestion-worker/index.ts b/supabase/functions/ingestion-worker/index.ts index b5ceb3f52..d44bf3714 100644 --- a/supabase/functions/ingestion-worker/index.ts +++ b/supabase/functions/ingestion-worker/index.ts @@ -215,12 +215,8 @@ async function markEnrichmentMetadata(documentId: string): Promise { `; } -async function processJob(job: ClaimedJob, workerId: string): Promise<"completed" | "lease_lost"> { - const summary = await upsertDocumentSummary(job); - await upsertEmbeddingFields(job, summary); - await markEnrichmentMetadata(job.document_id); - - const [completionRow] = await sql>` +async function completeIngestionJob(job: ClaimedJob, workerId: string): Promise { + const [row] = await sql>` select public.complete_ingestion_job( ${job.id}::uuid, ${job.document_id}::uuid, @@ -229,8 +225,38 @@ async function processJob(job: ClaimedJob, workerId: string): Promise<"completed ${workerId} ) `; + return row?.complete_ingestion_job ?? null; +} + +async function failOrRetryIngestionJob( + job: ClaimedJob, + workerId: string, + shouldRetry: boolean, + message: string, +): Promise { + const [row] = await sql>` + select public.fail_or_retry_ingestion_job( + ${job.id}::uuid, + ${job.document_id}::uuid, + ${job.batch_id}::uuid, + ${shouldRetry}, + ${"indexed"}, + ${"enrichment backfill failed"}, + ${message}, + ${new Date(Date.now() + 60_000).toISOString()}::timestamptz, + ${workerId} + ) + `; + return row?.fail_or_retry_ingestion_job ?? null; +} - return isLeaseLost(completionRow?.complete_ingestion_job) ? "lease_lost" : "completed"; +async function processJob(job: ClaimedJob, workerId: string): Promise<"completed" | "lease_lost"> { + const summary = await upsertDocumentSummary(job); + await upsertEmbeddingFields(job, summary); + await markEnrichmentMetadata(job.document_id); + + const completion = await completeIngestionJob(job, workerId); + return isLeaseLost(completion) ? "lease_lost" : "completed"; } Deno.serve(async (req: Request) => { @@ -258,38 +284,29 @@ Deno.serve(async (req: Request) => { let processed = 0; let failed = 0; + let leaseLost = 0; const failures: Array<{ job_id: string; document_id: string; error: string }> = []; for (const job of claimed) { try { const outcome = await processJob(job, workerId); - if (outcome === "lease_lost") continue; + if (outcome === "lease_lost") { + leaseLost += 1; + continue; + } processed += 1; } catch (error) { - failed += 1; const message = error instanceof Error ? error.message : JSON.stringify(error); - failures.push({ job_id: job.id, document_id: job.document_id, error: message }); - const shouldRetry = job.attempt_count < job.max_attempts; + const failure = await failOrRetryIngestionJob(job, workerId, shouldRetry, message); - const [failureRow] = await sql>` - select public.fail_or_retry_ingestion_job( - ${job.id}::uuid, - ${job.document_id}::uuid, - ${job.batch_id}::uuid, - ${shouldRetry}, - ${"indexed"}, - ${"enrichment backfill failed"}, - ${message}, - ${new Date(Date.now() + 60_000).toISOString()}::timestamptz, - ${workerId} - ) - `; - - if (isLeaseLost(failureRow?.fail_or_retry_ingestion_job)) { - failed -= 1; - failures.pop(); + if (isLeaseLost(failure)) { + leaseLost += 1; + continue; } + + failed += 1; + failures.push({ job_id: job.id, document_id: job.document_id, error: message }); } } @@ -298,6 +315,7 @@ Deno.serve(async (req: Request) => { claimed: claimed.length, processed, failed, + lease_lost: leaseLost, failures, }); } catch (error) { diff --git a/tests/ingestion-edge-function-auth.test.ts b/tests/ingestion-edge-function-auth.test.ts index 90f11396b..7ff9788c0 100644 --- a/tests/ingestion-edge-function-auth.test.ts +++ b/tests/ingestion-edge-function-auth.test.ts @@ -32,6 +32,7 @@ describe("ingestion-worker Edge Function authorization", () => { expect(source).toContain("fail_or_retry_ingestion_job("); expect(source).toContain("${workerId}"); expect(source).toContain('reason === "lease_lost"'); + expect(source).toContain("lease_lost:"); expect(source).toContain("sql.begin"); }); }); diff --git a/tests/pdf-extractor.test.ts b/tests/pdf-extractor.test.ts index babba3d51..d00b49d90 100644 --- a/tests/pdf-extractor.test.ts +++ b/tests/pdf-extractor.test.ts @@ -6,7 +6,7 @@ import path from "node:path"; import PDFDocument from "pdfkit"; import { describe, expect, it } from "vitest"; -const pythonBin = process.env.PYTHON_BIN || "python"; +const pythonBin = process.env.PYTHON_BIN || (process.platform === "win32" ? "python" : "python3"); const hasPyMuPDF = spawnSync(pythonBin, ["-c", "import fitz"], { encoding: "utf8" }).status === 0; describe("Python PDF extraction prerequisite", () => { From 672d06e37ca5a75d5b613e71ffcfe70210ac85e8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:26:15 +0000 Subject: [PATCH 3/7] style: format document extractor after python bin fix Co-authored-by: BigSimmo --- src/lib/extractors/document.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/lib/extractors/document.ts b/src/lib/extractors/document.ts index 212ad234f..355ff3236 100644 --- a/src/lib/extractors/document.ts +++ b/src/lib/extractors/document.ts @@ -95,16 +95,12 @@ export async function runPythonPdfExtractor( await writeFile(budgetPath, JSON.stringify(limits), "utf8"); return new Promise((resolve, reject) => { - const child = spawn( - resolvePythonBin(), - [scriptPath, filePath, outputDir, outputJsonPath, budgetPath], - { - cwd: process.cwd(), - stdio: ["ignore", "pipe", "pipe"], - detached: process.platform !== "win32", - windowsHide: true, - }, - ); + const child = spawn(resolvePythonBin(), [scriptPath, filePath, outputDir, outputJsonPath, budgetPath], { + cwd: process.cwd(), + stdio: ["ignore", "pipe", "pipe"], + detached: process.platform !== "win32", + windowsHide: true, + }); let stdout = ""; let stderr = ""; From 41559d4c9cafe9825a43c4e68b7c8d2a5fd7cae8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:41:40 +0000 Subject: [PATCH 4/7] fix: complete deferred review follow-ups and harden PR policy workflow - Gate DocumentManagementActions on isAdministratorUser for defense in depth - Add check-pr-policy-workflow guard for pull_request_target hardening invariants - Document trusted workflow_sha checkout in process-hardening notes - Extend account-access model test to lock administrator client gate Co-authored-by: BigSimmo --- docs/process-hardening.md | 4 +- package.json | 2 +- scripts/check-pr-policy-workflow.mjs | 103 +++++++++++++++++++ src/components/DocumentManagementActions.tsx | 4 +- tests/account-access-model.test.ts | 6 ++ 5 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 scripts/check-pr-policy-workflow.mjs diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 6256d49cd..bbfbd24dc 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -361,8 +361,8 @@ the durable index for the tooling; `docs/operator-backlog.md` tracks the human-o must use an outcome-focused title, complete Summary and Verification evidence, and provide risk/rollback evidence for clinical or operationally sensitive paths. UI changes require `verify:ui` evidence (or an explicit reason it could not run), while clinical-risk changes must fully disposition the governance - checklist. The `pull_request_target` job checks out the exact base SHA, has read-only permissions, and - never executes PR-head code. Drafts remain non-blocking until marked ready; merge-queue runs emit the + checklist. The `pull_request_target` job checks out the trusted `github.workflow_sha` revision, has + read-only permissions, and never executes PR-head code. Drafts remain non-blocking until marked ready; merge-queue runs emit the same stable `PR policy` check name. - **Default-branch failure attribution** (`scripts/ci-triage.mjs`): triage now compares a failed PR only with the latest completed run of the same workflow on `main`. It no longer samples the latest arbitrary diff --git a/package.json b/package.json index ed261bb60..fad4fa8c0 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "check:maintainability-budgets": "node scripts/check-maintainability-budgets.mjs", "check:ci-scope": "node scripts/ci-change-scope.mjs --self-test", "check:ci-triage": "node scripts/ci-triage.mjs --self-test", - "check:pr-policy": "node scripts/pr-policy.mjs --self-test", + "check:pr-policy": "node scripts/pr-policy.mjs --self-test && node scripts/check-pr-policy-workflow.mjs", "check:env-parity": "node scripts/check-env-parity.mjs", "sweep:branch-ledger": "node scripts/sweep-branch-ledger.mjs", "docs:check-links": "node scripts/check-docs-links.mjs", diff --git a/scripts/check-pr-policy-workflow.mjs b/scripts/check-pr-policy-workflow.mjs new file mode 100644 index 000000000..92c9a1051 --- /dev/null +++ b/scripts/check-pr-policy-workflow.mjs @@ -0,0 +1,103 @@ +import fs from "node:fs"; + +const workflowPath = ".github/workflows/pr-policy.yml"; +const ciWorkflowPath = ".github/workflows/ci.yml"; +const workflow = fs.readFileSync(workflowPath, "utf8"); +const ciWorkflow = fs.readFileSync(ciWorkflowPath, "utf8"); +const githubScriptPin = "3a2844b7e9c422d3c10d287c895573f7108da1b3"; + +const failures = []; + +const forbiddenPatterns = [ + { + pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.head\.sha\s*\}\}/, + message: "PR policy workflow must not checkout the untrusted PR head SHA.", + }, + { + pattern: /ref:\s*\$\{\{\s*github\.base_ref\s*\}\}/, + message: "PR policy workflow must not checkout the moving base branch ref.", + }, + { + pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/, + message: "PR policy workflow must not checkout a potentially stale pull_request.base.sha.", + }, + { + pattern: /persist-credentials:\s*true/, + message: "PR policy workflow must not persist checkout credentials.", + }, + { + pattern: /pull-requests:\s*write/, + message: "PR policy workflow must remain read-only for pull requests.", + }, + { + pattern: /contents:\s*write/, + message: "PR policy workflow must not grant contents write permission.", + }, + { + pattern: /uses:\s*actions\/github-script@v\d+/, + message: "PR policy workflow must pin github-script to an immutable commit.", + }, +]; + +for (const { pattern, message } of forbiddenPatterns) { + if (pattern.test(workflow)) { + failures.push(message); + } +} + +const requiredChecks = [ + "pull_request_target:", + "contents: read", + "pull-requests: read", + "ref: ${{ github.workflow_sha }}", + "persist-credentials: false", + `uses: actions/github-script@${githubScriptPin} # v9.0.0`, + "GITHUB_WORKSPACE}/scripts/pr-policy.mjs", + "github.event_name == 'pull_request_target'", +]; + +for (const requiredCheck of requiredChecks) { + if (!workflow.includes(requiredCheck)) { + failures.push(`PR policy workflow is missing required hardening check: ${requiredCheck}`); + } +} + +const syncJob = ciWorkflow.match(/sync-pr-policy-body:[\s\S]*?(?=\n [a-z0-9-]+:|$)/)?.[0] ?? ""; +if (!syncJob) { + failures.push("ci.yml is missing the sync-pr-policy-body job."); +} else { + const syncForbiddenPatterns = [ + { + pattern: /trusted-policy\/scripts\/pr-policy\.mjs/, + message: "sync-pr-policy-body must import pr-policy.mjs from the trusted base checkout only.", + }, + { + pattern: /existingCheckedItems/, + message: "sync-pr-policy-body must preserve existing governance attestations instead of auto-checking items.", + }, + { + pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}[\s\S]*path:\s*trusted-policy/, + message: "sync-pr-policy-body must checkout trusted policy metadata from the PR base SHA.", + }, + ]; + + for (const { pattern, message } of syncForbiddenPatterns) { + if (!pattern.test(syncJob)) { + failures.push(message); + } + } + + if (/map\(\(item\) => `\s*-\s*\[x\]/i.test(syncJob)) { + failures.push("sync-pr-policy-body must not synthesize completed Clinical Governance Preflight items."); + } +} + +if (failures.length > 0) { + console.error("PR policy workflow guard failed:"); + for (const failure of failures) { + console.error(`- ${failure}`); + } + process.exit(1); +} + +console.log("PR policy workflow guard passed."); diff --git a/src/components/DocumentManagementActions.tsx b/src/components/DocumentManagementActions.tsx index 0e9fcc8d8..db286cd3c 100644 --- a/src/components/DocumentManagementActions.tsx +++ b/src/components/DocumentManagementActions.tsx @@ -14,6 +14,7 @@ import { toolbarButton, } from "@/components/ui-primitives"; import { Sheet } from "@/components/ui/sheet"; +import { isAdministratorUser } from "@/lib/authorization"; import { useAuthSession } from "@/lib/supabase/client"; import type { ClinicalDocument } from "@/lib/types"; @@ -44,6 +45,7 @@ export function DocumentManagementActions({ const inputRef = useRef(null); const { status: authStatus, + session, authorizationHeader, registerAuthRequest, isAuthEpochCurrent, @@ -54,7 +56,7 @@ export function DocumentManagementActions({ const [deleteConfirmation, setDeleteConfirmation] = useState(""); const [error, setError] = useState(null); const [pending, setPending] = useState(false); - const canManage = !disabled && authStatus === "authenticated"; + const canManage = !disabled && authStatus === "authenticated" && isAdministratorUser(session?.user); function resetDialogState() { setTitle(document.title); diff --git a/tests/account-access-model.test.ts b/tests/account-access-model.test.ts index b2affd427..2d8b8cb67 100644 --- a/tests/account-access-model.test.ts +++ b/tests/account-access-model.test.ts @@ -48,6 +48,12 @@ describe("public content and account authorization model", () => { expect(migration).toContain("revoke insert, update, delete on table storage.objects from anon, authenticated"); }); + it("keeps document management actions administrator-gated in the client", () => { + const documentManagementActions = source("src/components/DocumentManagementActions.tsx"); + expect(documentManagementActions).toContain("isAdministratorUser(session?.user)"); + expect(documentManagementActions).toContain('authStatus === "authenticated"'); + }); + it("keeps public document reads separate from administrator mutations", () => { const documentRoute = source("src/app/api/documents/[id]/route.ts"); const tableFactsRoute = source("src/app/api/documents/[id]/table-facts/route.ts"); From 430ee1ec96a176adf9a7ec728829ee50aeff4394 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:51:56 +0000 Subject: [PATCH 5/7] fix: address CodeRabbit review findings before merge - Revalidate administrator access in DocumentManagementActions submit handlers - Centralize resolvePythonBin for extraction, env defaults, and worker prerequisites - Fail ingestion worker conservatively when complete_ingestion_job is ambiguous - Parse PR policy workflow YAML by job/step instead of whole-file string matching Co-authored-by: BigSimmo --- scripts/check-pr-policy-workflow.mjs | 155 +++++++++++-------- src/components/DocumentManagementActions.tsx | 13 ++ src/lib/env.ts | 3 +- src/lib/extractors/document.ts | 6 +- src/lib/python-bin.ts | 5 + supabase/functions/ingestion-worker/index.ts | 8 +- tests/account-access-model.test.ts | 1 + tests/ingestion-edge-function-auth.test.ts | 1 + tests/pdf-extractor.test.ts | 4 +- 9 files changed, 120 insertions(+), 76 deletions(-) create mode 100644 src/lib/python-bin.ts diff --git a/scripts/check-pr-policy-workflow.mjs b/scripts/check-pr-policy-workflow.mjs index 92c9a1051..153715fe4 100644 --- a/scripts/check-pr-policy-workflow.mjs +++ b/scripts/check-pr-policy-workflow.mjs @@ -1,5 +1,7 @@ import fs from "node:fs"; +import { yamlBlock } from "./yaml-contract.mjs"; + const workflowPath = ".github/workflows/pr-policy.yml"; const ciWorkflowPath = ".github/workflows/ci.yml"; const workflow = fs.readFileSync(workflowPath, "utf8"); @@ -8,87 +10,104 @@ const githubScriptPin = "3a2844b7e9c422d3c10d287c895573f7108da1b3"; const failures = []; -const forbiddenPatterns = [ - { - pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.head\.sha\s*\}\}/, - message: "PR policy workflow must not checkout the untrusted PR head SHA.", - }, - { - pattern: /ref:\s*\$\{\{\s*github\.base_ref\s*\}\}/, - message: "PR policy workflow must not checkout the moving base branch ref.", - }, - { - pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/, - message: "PR policy workflow must not checkout a potentially stale pull_request.base.sha.", - }, - { - pattern: /persist-credentials:\s*true/, - message: "PR policy workflow must not persist checkout credentials.", - }, - { - pattern: /pull-requests:\s*write/, - message: "PR policy workflow must remain read-only for pull requests.", - }, - { - pattern: /contents:\s*write/, - message: "PR policy workflow must not grant contents write permission.", - }, - { - pattern: /uses:\s*actions\/github-script@v\d+/, - message: "PR policy workflow must pin github-script to an immutable commit.", - }, -]; +function collectCheckoutRefs(block) { + return block + .split(/\r?\n/) + .map((line) => line.match(/^\s+ref:\s*(.+?)\s*(?:#.*)?$/)?.[1]?.trim()) + .filter(Boolean); +} -for (const { pattern, message } of forbiddenPatterns) { - if (pattern.test(workflow)) { - failures.push(message); +function assertCheckoutRefs(block, label, { allowed = [], forbidden = [] }) { + const refs = collectCheckoutRefs(block); + for (const ref of refs) { + if (forbidden.some((pattern) => pattern.test(ref))) { + failures.push(`${label} must not checkout untrusted ref ${ref}.`); + } + if (allowed.length > 0 && !allowed.includes(ref)) { + failures.push(`${label} checkout ref ${ref} is not in the allowed trusted set.`); + } + } + if (refs.length === 0) { + failures.push(`${label} is missing an actions/checkout ref declaration.`); } } -const requiredChecks = [ - "pull_request_target:", - "contents: read", - "pull-requests: read", - "ref: ${{ github.workflow_sha }}", - "persist-credentials: false", - `uses: actions/github-script@${githubScriptPin} # v9.0.0`, - "GITHUB_WORKSPACE}/scripts/pr-policy.mjs", - "github.event_name == 'pull_request_target'", -]; +function assertPersistCredentialsFalse(block, label) { + if (!/persist-credentials:\s*false/.test(block)) { + failures.push(`${label} must set persist-credentials: false on checkout steps.`); + } + if (/persist-credentials:\s*true/.test(block)) { + failures.push(`${label} must not persist checkout credentials.`); + } +} -for (const requiredCheck of requiredChecks) { - if (!workflow.includes(requiredCheck)) { - failures.push(`PR policy workflow is missing required hardening check: ${requiredCheck}`); +const policyJob = yamlBlock(workflow, "policy:", 2); +if (!policyJob) { + failures.push("pr-policy.yml is missing the policy job."); +} else { + const checkoutStep = yamlBlock(policyJob, "- name: Checkout trusted policy", 6); + if (!checkoutStep) { + failures.push("pr-policy.yml policy job is missing the trusted checkout step."); + } else { + assertCheckoutRefs(checkoutStep, "PR policy trusted checkout", { + allowed: ["${{ github.workflow_sha }}"], + forbidden: [/github\.event\.pull_request\.head/, /github\.base_ref/, /github\.event\.pull_request\.base\.sha/], + }); + assertPersistCredentialsFalse(checkoutStep, "PR policy trusted checkout"); + } + + const validateStep = yamlBlock(policyJob, "- name: Validate pull request evidence", 6); + if (!validateStep) { + failures.push("pr-policy.yml policy job is missing the validation step."); + } else { + if (!validateStep.includes("GITHUB_WORKSPACE}/scripts/pr-policy.mjs")) { + failures.push("PR policy validation must import scripts/pr-policy.mjs from the trusted checkout."); + } + if (!validateStep.includes(`uses: actions/github-script@${githubScriptPin} # v9.0.0`)) { + failures.push("PR policy validation must use the pinned github-script action."); + } + } + + if ( + !/^permissions:\s*$/m.test(workflow) || + !workflow.includes("contents: read") || + !workflow.includes("pull-requests: read") + ) { + failures.push("pr-policy.yml must declare read-only workflow permissions."); + } + if (/pull-requests:\s*write/.test(policyJob) || /contents:\s*write/.test(policyJob)) { + failures.push("pr-policy.yml policy job must not request write permissions."); } } -const syncJob = ciWorkflow.match(/sync-pr-policy-body:[\s\S]*?(?=\n [a-z0-9-]+:|$)/)?.[0] ?? ""; +const syncJob = yamlBlock(ciWorkflow, "sync-pr-policy-body:", 2); if (!syncJob) { failures.push("ci.yml is missing the sync-pr-policy-body job."); } else { - const syncForbiddenPatterns = [ - { - pattern: /trusted-policy\/scripts\/pr-policy\.mjs/, - message: "sync-pr-policy-body must import pr-policy.mjs from the trusted base checkout only.", - }, - { - pattern: /existingCheckedItems/, - message: "sync-pr-policy-body must preserve existing governance attestations instead of auto-checking items.", - }, - { - pattern: /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}[\s\S]*path:\s*trusted-policy/, - message: "sync-pr-policy-body must checkout trusted policy metadata from the PR base SHA.", - }, - ]; - - for (const { pattern, message } of syncForbiddenPatterns) { - if (!pattern.test(syncJob)) { - failures.push(message); - } + const trustedCheckout = yamlBlock(syncJob, "- name: Checkout trusted policy metadata", 6); + if (!trustedCheckout) { + failures.push("sync-pr-policy-body is missing the trusted policy metadata checkout step."); + } else { + assertCheckoutRefs(trustedCheckout, "sync-pr-policy-body trusted policy checkout", { + allowed: ["${{ github.event.pull_request.base.sha }}"], + forbidden: [/github\.event\.pull_request\.head/], + }); + assertPersistCredentialsFalse(trustedCheckout, "sync-pr-policy-body trusted policy checkout"); } - if (/map\(\(item\) => `\s*-\s*\[x\]/i.test(syncJob)) { - failures.push("sync-pr-policy-body must not synthesize completed Clinical Governance Preflight items."); + const applyStep = yamlBlock(syncJob, "- name: Apply PR_POLICY_BODY.md to pull request description", 6); + if (!applyStep) { + failures.push("sync-pr-policy-body is missing the PR body sync step."); + } else { + if (!applyStep.includes("trusted-policy/scripts/pr-policy.mjs")) { + failures.push("sync-pr-policy-body must import pr-policy.mjs from the trusted base checkout only."); + } + if (!applyStep.includes("existingCheckedItems")) { + failures.push("sync-pr-policy-body must preserve existing governance attestations."); + } + if (/map\(\(item\) => `\s*-\s*\[x\]/i.test(applyStep)) { + failures.push("sync-pr-policy-body must not synthesize completed Clinical Governance Preflight items."); + } } } diff --git a/src/components/DocumentManagementActions.tsx b/src/components/DocumentManagementActions.tsx index db286cd3c..b7ed68067 100644 --- a/src/components/DocumentManagementActions.tsx +++ b/src/components/DocumentManagementActions.tsx @@ -58,6 +58,17 @@ export function DocumentManagementActions({ const [pending, setPending] = useState(false); const canManage = !disabled && authStatus === "authenticated" && isAdministratorUser(session?.user); + function assertCanManage(): boolean { + const allowed = !disabled && authStatus === "authenticated" && isAdministratorUser(session?.user); + if (!allowed) { + setMode(null); + resetDialogState(); + setError("Administrator access is required for this action."); + return false; + } + return true; + } + function resetDialogState() { setTitle(document.title); setDeleteConfirmation(""); @@ -87,6 +98,7 @@ export function DocumentManagementActions({ async function submitRename(event: FormEvent) { event.preventDefault(); + if (!assertCanManage()) return; const nextTitle = title.trim(); if (!nextTitle) { setError("Enter a document title."); @@ -121,6 +133,7 @@ export function DocumentManagementActions({ async function submitDelete(event: FormEvent) { event.preventDefault(); + if (!assertCanManage()) return; if (deleteConfirmation !== document.title) { setError("Type the current document title to confirm permanent deletion."); return; diff --git a/src/lib/env.ts b/src/lib/env.ts index 285f7a372..e1b2681ef 100644 --- a/src/lib/env.ts +++ b/src/lib/env.ts @@ -1,6 +1,7 @@ import "server-only"; import { z } from "zod"; +import { resolvePythonBin } from "@/lib/python-bin"; import { assertExpectedSupabaseProjectConfig, checkSupabaseProjectConfig } from "@/lib/supabase/project"; const envSchema = z.object({ @@ -207,7 +208,7 @@ const envSchema = z.object({ .enum(["true", "false"]) .default("false") .transform((value) => value === "true"), - PYTHON_BIN: z.string().default("python"), + PYTHON_BIN: z.string().default(resolvePythonBin()), NEXT_PUBLIC_DEMO_MODE: z.enum(["true", "false"]).optional().default("false"), }); diff --git a/src/lib/extractors/document.ts b/src/lib/extractors/document.ts index 355ff3236..58a751fb7 100644 --- a/src/lib/extractors/document.ts +++ b/src/lib/extractors/document.ts @@ -17,11 +17,7 @@ import { PdfExtractionResourceError, type PdfExtractionBudget, } from "@/lib/extractors/pdf-extraction-budget"; - -function resolvePythonBin() { - if (process.env.PYTHON_BIN) return process.env.PYTHON_BIN; - return process.platform === "win32" ? "python" : "python3"; -} +import { resolvePythonBin } from "@/lib/python-bin"; const extractedPageSchema = z.object({ pageNumber: z.number().int().positive(), diff --git a/src/lib/python-bin.ts b/src/lib/python-bin.ts new file mode 100644 index 000000000..1064d8282 --- /dev/null +++ b/src/lib/python-bin.ts @@ -0,0 +1,5 @@ +/** Shared Python executable resolution for extraction and worker prerequisites. */ +export function resolvePythonBin(explicit = process.env.PYTHON_BIN): string { + if (explicit && explicit.trim().length > 0) return explicit; + return process.platform === "win32" ? "python" : "python3"; +} diff --git a/supabase/functions/ingestion-worker/index.ts b/supabase/functions/ingestion-worker/index.ts index d44bf3714..5c43c206d 100644 --- a/supabase/functions/ingestion-worker/index.ts +++ b/supabase/functions/ingestion-worker/index.ts @@ -256,7 +256,13 @@ async function processJob(job: ClaimedJob, workerId: string): Promise<"completed await markEnrichmentMetadata(job.document_id); const completion = await completeIngestionJob(job, workerId); - return isLeaseLost(completion) ? "lease_lost" : "completed"; + if (isLeaseLost(completion)) return "lease_lost"; + if (!completion || completion.ok === false) { + throw new Error( + `complete_ingestion_job did not confirm success: ${completion?.reason ?? "no result row"}`, + ); + } + return "completed"; } Deno.serve(async (req: Request) => { diff --git a/tests/account-access-model.test.ts b/tests/account-access-model.test.ts index 2d8b8cb67..aeb2b4e89 100644 --- a/tests/account-access-model.test.ts +++ b/tests/account-access-model.test.ts @@ -52,6 +52,7 @@ describe("public content and account authorization model", () => { const documentManagementActions = source("src/components/DocumentManagementActions.tsx"); expect(documentManagementActions).toContain("isAdministratorUser(session?.user)"); expect(documentManagementActions).toContain('authStatus === "authenticated"'); + expect(documentManagementActions).toContain("assertCanManage()"); }); it("keeps public document reads separate from administrator mutations", () => { diff --git a/tests/ingestion-edge-function-auth.test.ts b/tests/ingestion-edge-function-auth.test.ts index 7ff9788c0..2108ba8e8 100644 --- a/tests/ingestion-edge-function-auth.test.ts +++ b/tests/ingestion-edge-function-auth.test.ts @@ -34,5 +34,6 @@ describe("ingestion-worker Edge Function authorization", () => { expect(source).toContain('reason === "lease_lost"'); expect(source).toContain("lease_lost:"); expect(source).toContain("sql.begin"); + expect(source).toContain("complete_ingestion_job did not confirm success"); }); }); diff --git a/tests/pdf-extractor.test.ts b/tests/pdf-extractor.test.ts index d00b49d90..e5c964a70 100644 --- a/tests/pdf-extractor.test.ts +++ b/tests/pdf-extractor.test.ts @@ -6,7 +6,9 @@ import path from "node:path"; import PDFDocument from "pdfkit"; import { describe, expect, it } from "vitest"; -const pythonBin = process.env.PYTHON_BIN || (process.platform === "win32" ? "python" : "python3"); +import { resolvePythonBin } from "@/lib/python-bin"; + +const pythonBin = resolvePythonBin(); const hasPyMuPDF = spawnSync(pythonBin, ["-c", "import fitz"], { encoding: "utf8" }).status === 0; describe("Python PDF extraction prerequisite", () => { From cc784916302c2f9381af5176e0cb10c00df6fe0e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 19:55:49 +0000 Subject: [PATCH 6/7] style: format ingestion worker after conservative completion guard Co-authored-by: BigSimmo --- supabase/functions/ingestion-worker/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/supabase/functions/ingestion-worker/index.ts b/supabase/functions/ingestion-worker/index.ts index 5c43c206d..88345df06 100644 --- a/supabase/functions/ingestion-worker/index.ts +++ b/supabase/functions/ingestion-worker/index.ts @@ -258,9 +258,7 @@ async function processJob(job: ClaimedJob, workerId: string): Promise<"completed const completion = await completeIngestionJob(job, workerId); if (isLeaseLost(completion)) return "lease_lost"; if (!completion || completion.ok === false) { - throw new Error( - `complete_ingestion_job did not confirm success: ${completion?.reason ?? "no result row"}`, - ); + throw new Error(`complete_ingestion_job did not confirm success: ${completion?.reason ?? "no result row"}`); } return "completed"; } From 3aa1c4a2ee908c67009f27ed2ed8c2360e8425e7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 19 Jul 2026 20:11:50 +0000 Subject: [PATCH 7/7] fix: trim PYTHON_BIN before spawning python extractor Co-authored-by: BigSimmo --- src/lib/python-bin.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/python-bin.ts b/src/lib/python-bin.ts index 1064d8282..5ea93a54d 100644 --- a/src/lib/python-bin.ts +++ b/src/lib/python-bin.ts @@ -1,5 +1,6 @@ /** Shared Python executable resolution for extraction and worker prerequisites. */ export function resolvePythonBin(explicit = process.env.PYTHON_BIN): string { - if (explicit && explicit.trim().length > 0) return explicit; + const trimmed = explicit?.trim(); + if (trimmed) return trimmed; return process.platform === "win32" ? "python" : "python3"; }