fix: complete repository audit remediation#715
Conversation
Port the reviewed containment fixes onto current main without changing external API contracts. Drift manifest regeneration remains part of the final remediation verification.
Replace broad public promotion with append-only operator evidence, explicit manifest confirmation, and an atomic service-role publication RPC. Historical public rows remain audit-only.
Move delete safety, cleanup snapshots, trace cleanup, and parent deletion into one RPC. Return normal 409s for delete/reindex races and compensate failed image-row persistence.
Assert effective supabase_admin defaults from pg_default_acl and acldefault, block unsafe applies with operator remediation, and add an explicitly gated read-only verifier.
|
Updates to Preview Branch (codex/audit-remediation-merge-20260717) ↗︎
Tasks are run on every commit but only new migration files are pushed.
❌ Branch Error • Fri, 17 Jul 2026 14:10:57 UTC View logs for this Workflow Run ↗︎. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds manifest-controlled document publication, database-backed deletion and retry coordination, PDF extraction resource budgets, default-ACL verification, committed-generation search filtering, storage compensation, and related migrations, scripts, tests, schema typings, and operational documentation. ChangesPublication governance
Serialized deletion, retry, and reindex races
PDF extraction and storage controls
ACL and search hardening
Operational records
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7fbdb1bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #2769 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
worker/python/extract_pdf_assets.py (1)
850-877: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMark pages when required OCR did not produce text.
When
should_ocr_page()is true but rendering is skipped or OCR fails, the page is returned withoutneedsOcr. Downstream quality checks can therefore treat incomplete clinical content as healthy.Proposed fix
text = page.get_text("text", sort=True) or "" ocr_used = False + needs_ocr = False image_coverage = page_image_coverage_ratio(page) if should_ocr_page(text, page): ocr_text, ocr_warning = maybe_ocr_page(page, budget) if ocr_warning: warnings.append(ocr_warning) if ocr_text.strip(): text = merge_ocr_text(text, ocr_text) ocr_used = True + else: + needs_ocr = True budget.add_text(text) pages.append( { "pageNumber": page_number, "text": text, "ocrUsed": ocr_used, + "needsOcr": needs_ocr,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@worker/python/extract_pdf_assets.py` around lines 850 - 877, Update the page-processing flow around should_ocr_page and maybe_ocr_page to track when OCR is required but produces no usable text, including skipped or failed rendering. Mark that page’s output with needsOcr while preserving the existing merge behavior and leaving pages with successful OCR or no OCR requirement unchanged.src/lib/extractors/document.ts (1)
226-244: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the PDFParse fallback under the same killable boundary. It runs in-process on non-resource Python failures, so a malformed PDF can still bypass the timeout and isolation that protect the main extractor. Run this path in the same killable worker/child, or fail closed outside developer-only setups.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/extractors/document.ts` around lines 226 - 244, The PDFParse fallback in the document extraction flow must not execute in the main process outside developer-only setups. Move the parser creation and the getText/getImage processing around PDFParse into the same killable worker or child boundary used by the primary extractor, or fail closed when that boundary is unavailable; preserve the existing page extraction and budget tracking behavior within the protected path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/migrations/20260717131000_guard_document_publication_approval.sql`:
- Line 14: Remove decision from the unique key for the publication approval
entry in
supabase/migrations/20260717131000_guard_document_publication_approval.sql:14
and apply the same canonical-schema correction in supabase/schema.sql:7542.
Regenerate the corresponding unique-index snapshot at
supabase/drift-manifest.json:5115-5119 and constraint snapshot at
supabase/drift-manifest.json:7391-7394 so they match the corrected key.
- Around line 84-87: Update the documents_require_publication_approval trigger
and guard_document_publication_transition function in
supabase/migrations/20260717131000_guard_document_publication_approval.sql
(lines 84-87) to cover INSERT operations and reject inserts with a NULL owner_id
unless equivalent approval evidence is present; mirror the same trigger/function
correction in supabase/schema.sql (lines 7612-7615), then regenerate the trigger
snapshot in supabase/drift-manifest.json (lines 6270-6274).
In `@supabase/migrations/20260717132000_delete_document_if_idle.sql`:
- Around line 30-55: Update the retry queueing flow to use a transaction/RPC
that acquires the same parent document lock as the deletion routine before
changing an existing job to pending. Perform the job status change and document
update atomically, and verify the document row still exists before returning
success. Align this protocol with the v_active_job handling in the deletion
routine so retries and deletion cannot race.
In
`@supabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sql`:
- Around line 65-75: Update the default-privilege verifier predicate and
remediation in
supabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sql
(lines 65-75) to reject and revoke table:PUBLIC:* and sequence:PUBLIC:* entries,
including corresponding hints. Mirror these predicate and remediation changes in
supabase/schema.sql (lines 7938-7948). In scripts/sql/drift-replay-scaffold.sql
(lines 79-90), revoke PUBLIC table and sequence defaults during replay setup. In
scripts/sql/verify-default-acl-catalog.sql (lines 46-58), add isolated negative
cases covering PUBLIC table and sequence defaults.
---
Outside diff comments:
In `@src/lib/extractors/document.ts`:
- Around line 226-244: The PDFParse fallback in the document extraction flow
must not execute in the main process outside developer-only setups. Move the
parser creation and the getText/getImage processing around PDFParse into the
same killable worker or child boundary used by the primary extractor, or fail
closed when that boundary is unavailable; preserve the existing page extraction
and budget tracking behavior within the protected path.
In `@worker/python/extract_pdf_assets.py`:
- Around line 850-877: Update the page-processing flow around should_ocr_page
and maybe_ocr_page to track when OCR is required but produces no usable text,
including skipped or failed rendering. Mark that page’s output with needsOcr
while preserving the existing merge behavior and leaving pages with successful
OCR or no OCR requirement unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a9b505b-ea3b-4709-91af-950c23a7dff1
📒 Files selected for processing (37)
docs/branch-review-ledger.mddocs/operator-backlog.mddocs/process-hardening.mdpackage.jsonscripts/audit-public-document-approvals.tsscripts/check-default-acl.tsscripts/promote-public-documents-batch.tsscripts/promote-public-documents.tsscripts/sql/drift-replay-scaffold.sqlscripts/sql/verify-default-acl-catalog.sqlscripts/sql/verify-delete-reindex-exclusion.sqlscripts/sql/verify-publication-approval.sqlsrc/app/api/documents/[id]/reindex/route.tssrc/app/api/documents/[id]/route.tssrc/app/api/documents/bulk/reindex/route.tssrc/lib/extractors/document.tssrc/lib/extractors/pdf-extraction-budget.tssrc/lib/ingestion.tssrc/lib/publication-manifest.tssrc/lib/storage-upload-compensation.tssrc/lib/supabase/database.types.tssrc/lib/types.tssupabase/drift-manifest.jsonsupabase/migrations/20260717130000_filter_search_document_chunks_committed_generation.sqlsupabase/migrations/20260717131000_guard_document_publication_approval.sqlsupabase/migrations/20260717132000_delete_document_if_idle.sqlsupabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sqlsupabase/schema.sqltests/pdf-extraction-budget.test.tstests/private-access-routes.test.tstests/publication-manifest.test.tstests/retrieval-owner-filter-guard.test.tstests/storage-upload-compensation.test.tstests/supabase-schema.test.tsworker/main.tsworker/python/extract_pdf_assets.pyworker/python/test_extract_pdf_assets_budget.py
💤 Files with no reviewable changes (2)
- scripts/promote-public-documents.ts
- tests/retrieval-owner-filter-guard.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review follow-up: the Python extractor now reports needsOcr when required OCR produces no text, with focused budget tests passing. The separate suggestion to move the in-process JavaScript PDFParse fallback into a killable worker is deferred: that requires a broader worker-isolation design and is not a safe, scoped change for this remediation merge. Existing fallback aggregate limits remain in place; this residual CPU-preemption risk should be tracked separately. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/sql/verify-publication-approval.sql`:
- Around line 63-77: Update the exception handling around the direct insert
fixture in the publication-guard verification block to assert the trigger’s
specific expected error message, rather than accepting any insertion failure.
Preserve re-raising the success sentinel and make unrelated SQL errors fail the
test, matching the existing specific-error assertion used for unique_violation
below.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 160f2c48-8156-4b71-abe9-2ae079e47abe
📒 Files selected for processing (14)
scripts/sql/verify-default-acl-catalog.sqlscripts/sql/verify-delete-reindex-exclusion.sqlscripts/sql/verify-publication-approval.sqlsrc/app/api/ingestion/jobs/[id]/retry/route.tssrc/lib/supabase/database.types.tssupabase/drift-manifest.jsonsupabase/migrations/20260717131000_guard_document_publication_approval.sqlsupabase/migrations/20260717132000_delete_document_if_idle.sqlsupabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sqlsupabase/roles.sqlsupabase/schema.sqltests/private-access-routes.test.tstests/supabase-schema.test.tsworker/python/extract_pdf_assets.py
🚧 Files skipped from review as they are similar to previous changes (7)
- supabase/roles.sql
- src/lib/supabase/database.types.ts
- supabase/migrations/20260717161000_assert_supabase_admin_default_privileges.sql
- tests/supabase-schema.test.ts
- supabase/drift-manifest.json
- supabase/migrations/20260717131000_guard_document_publication_approval.sql
- worker/python/extract_pdf_assets.py
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…on-merge-20260717
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@supabase/roles.sql`:
- Around line 79-116: Update the v_safe predicate to explicitly allow only
supabase_admin and the permitted service_role entries, rejecting grants to every
other role, including login or inherited roles. Validate the structured exploded
rows rather than relying on parsing v_entries, while preserving the existing
exact service_role table, sequence, and function allowlists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 431cd927-5231-4d2c-8ad7-f80777067bbc
📒 Files selected for processing (3)
.github/workflows/ci.ymldocs/process-hardening.mdsupabase/roles.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/process-hardening.md
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
mainPR-policy hardening without dropping either review-ledger addition.Verification
npm run verify:pr-local— passed on the audit remediation head: formatting, lint, typecheck, 2,628 tests, production build, client-secret scan, and offline RAG fixtures.npm run verify:cheap— passed after review fixes: 276 files and 2,629 tests.npm run check:pr-policy,npm run check:github-actions, andnpm run check:ci-scope— passed after current-main integration.npm run verify:pr-localrerun was not run to completion because the repository lock is held by another worktree Chromium run; hosted checks are running on the combined head.Risk and rollout
Database migrations must deploy before dependent app/worker code. The default-ACL assertion is intentionally last and blocks deployment until the operator-owned Supabase ACL remediation is complete. Historical public rows are audited but are not automatically changed.
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation