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..153715fe4 --- /dev/null +++ b/scripts/check-pr-policy-workflow.mjs @@ -0,0 +1,122 @@ +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"); +const ciWorkflow = fs.readFileSync(ciWorkflowPath, "utf8"); +const githubScriptPin = "3a2844b7e9c422d3c10d287c895573f7108da1b3"; + +const failures = []; + +function collectCheckoutRefs(block) { + return block + .split(/\r?\n/) + .map((line) => line.match(/^\s+ref:\s*(.+?)\s*(?:#.*)?$/)?.[1]?.trim()) + .filter(Boolean); +} + +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.`); + } +} + +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.`); + } +} + +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 = yamlBlock(ciWorkflow, "sync-pr-policy-body:", 2); +if (!syncJob) { + failures.push("ci.yml is missing the sync-pr-policy-body job."); +} else { + 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"); + } + + 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."); + } + } +} + +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/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/DocumentManagementActions.tsx b/src/components/DocumentManagementActions.tsx index 0e9fcc8d8..b7ed68067 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,18 @@ 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 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); @@ -85,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."); @@ -119,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/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index f669cf32e..31572840c 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/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 590efed45..58a751fb7 100644 --- a/src/lib/extractors/document.ts +++ b/src/lib/extractors/document.ts @@ -17,6 +17,7 @@ import { PdfExtractionResourceError, type PdfExtractionBudget, } from "@/lib/extractors/pdf-extraction-budget"; +import { resolvePythonBin } from "@/lib/python-bin"; const extractedPageSchema = z.object({ pageNumber: z.number().int().positive(), @@ -90,16 +91,12 @@ export async function runPythonPdfExtractor( await writeFile(budgetPath, JSON.stringify(limits), "utf8"); return new Promise((resolve, reject) => { - const child = spawn( - process.env.PYTHON_BIN || "python", - [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 = ""; diff --git a/src/lib/python-bin.ts b/src/lib/python-bin.ts new file mode 100644 index 000000000..5ea93a54d --- /dev/null +++ b/src/lib/python-bin.ts @@ -0,0 +1,6 @@ +/** Shared Python executable resolution for extraction and worker prerequisites. */ +export function resolvePythonBin(explicit = process.env.PYTHON_BIN): string { + const trimmed = explicit?.trim(); + if (trimmed) return trimmed; + return process.platform === "win32" ? "python" : "python3"; +} diff --git a/supabase/functions/ingestion-worker/index.ts b/supabase/functions/ingestion-worker/index.ts index a9927baf9..88345df06 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,52 @@ async function markEnrichmentMetadata(documentId: string): Promise { `; } -async function processJob(job: ClaimedJob): Promise { - const summary = await upsertDocumentSummary(job); - await upsertEmbeddingFields(job, summary); - await markEnrichmentMetadata(job.document_id); - - 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, ${job.batch_id}::uuid, - ${"indexed + enrichment backfill"} + ${"indexed + enrichment backfill"}, + ${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; +} + +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); + 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) => { @@ -239,31 +288,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 { - await processJob(job); + const outcome = await processJob(job, workerId); + 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); + + if (isLeaseLost(failure)) { + leaseLost += 1; + continue; + } - 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 - ) - `; + failed += 1; + failures.push({ job_id: job.id, document_id: job.document_id, error: message }); } } @@ -272,6 +319,7 @@ Deno.serve(async (req: Request) => { claimed: claimed.length, processed, failed, + lease_lost: leaseLost, failures, }); } catch (error) { diff --git a/tests/account-access-model.test.ts b/tests/account-access-model.test.ts index b2affd427..aeb2b4e89 100644 --- a/tests/account-access-model.test.ts +++ b/tests/account-access-model.test.ts @@ -48,6 +48,13 @@ 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"'); + expect(documentManagementActions).toContain("assertCanManage()"); + }); + 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"); diff --git a/tests/ingestion-edge-function-auth.test.ts b/tests/ingestion-edge-function-auth.test.ts index 65c735c4f..2108ba8e8 100644 --- a/tests/ingestion-edge-function-auth.test.ts +++ b/tests/ingestion-edge-function-auth.test.ts @@ -28,5 +28,12 @@ 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("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 babba3d51..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 || "python"; +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", () => {