fix: reconcile applied RAG remediation functions#568
Conversation
|
Updates to Preview Branch (codex/live-remediation-apply) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds owner-plus-public retrieval RPCs, reconciles deep-memory and indexing-enrichment functions, restricts execution to ChangesOwner-plus-public retrieval wrappers
RAG remediation reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RetrievalCaller
participant match_documents_for_query_v2
participant match_documents_for_query
participant RetrievalResults
RetrievalCaller->>match_documents_for_query_v2: send query, owner filter, and public flag
match_documents_for_query_v2->>match_documents_for_query: retrieve owner-scoped documents
match_documents_for_query_v2->>match_documents_for_query: retrieve public documents when enabled
match_documents_for_query_v2->>RetrievalResults: deduplicate by document id and rank by text score
RetrievalResults-->>RetrievalCaller: return consolidated documents
sequenceDiagram
participant ServiceRole
participant request_indexing_v3_enrichment
participant IndexingJobs
participant Documents
ServiceRole->>request_indexing_v3_enrichment: request enrichment for document and owner
request_indexing_v3_enrichment->>IndexingJobs: insert or lock visual-core-v3 job
request_indexing_v3_enrichment->>Documents: lock indexed document
request_indexing_v3_enrichment->>IndexingJobs: reset eligible job to pending
request_indexing_v3_enrichment->>Documents: update indexing-agent metadata
request_indexing_v3_enrichment-->>ServiceRole: return job id
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
supabase/schema.sql (5)
7021-7046: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame unnecessary
to_jsonb(fact)->'metadata'indirection;fact.metadataalready exists.
match_document_table_facts_text'sRETURNS TABLE(lines 4130-4150) already declaresmetadata jsonbas a direct output column.coalesce(to_jsonb(fact)->'metadata', '{}'::jsonb)is unnecessary overhead and, like thelexical_scorecase above, trades a compile-time failure mode for a silent fallback if the column is ever removed.🤖 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 `@supabase/schema.sql` around lines 7021 - 7046, Update match_document_table_facts_text_v2 to select fact.metadata directly in both branches of the combined CTE, removing the coalesce(to_jsonb(fact)->'metadata', '{}'::jsonb) indirection while preserving the existing result shape and ranking behavior.
6843-6854: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
retrieval_owner_matches_v2dropsset search_path/parallel safepresent on its v1 sibling.The original
public.retrieval_owner_matches(line 2598) setsset search_path = public, pg_catalogandparallel safe. The newretrieval_owner_matches_v2has neither. The body has no unqualified table/function references today, so there's no active vulnerability, but this deviates from the established convention in this file and would silently lose the hardening/perf attribute if the body is later extended to reference other objects.🔧 Suggested fix
create or replace function public.retrieval_owner_matches_v2( owner_filter uuid, row_owner_id uuid, include_public boolean default true ) -returns boolean language sql immutable as $$ +returns boolean language sql immutable parallel safe +set search_path = public, pg_catalog +as $$ select owner_filter is not null and (🤖 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 `@supabase/schema.sql` around lines 6843 - 6854, Update the public.retrieval_owner_matches_v2 function definition to match retrieval_owner_matches by adding the established SET search_path = public, pg_catalog and PARALLEL SAFE attributes. Preserve the existing SQL body and owner-matching behavior.
7134-7141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTruth-table self-test only exercises
include_public = true.All four assertions pass
include_public = true; thefalsebranch (coalesce(include_public, false) and row_owner_id is null) is never exercised here. Given the function is otherwise unused within this cohort (see earlier comment), adding one more assertion withinclude_public = falseand a nullrow_owner_idwould close the coverage gap cheaply.🤖 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 `@supabase/schema.sql` around lines 7134 - 7141, The retrieval_owner_matches_v2 truth-table self-test only covers include_public = true. Extend the do-block assertions to include a case with include_public = false and a null row_owner_id, verifying the function returns the expected false-branch result while preserving the existing assertions.
6874-6910: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnnecessary
to_jsonb(...)->>round-trip forlexical_score; direct field access already works.
match_document_chunks_text'sRETURNS TABLE(lines 3809-3834) already exposeslexical_score double precisiondirectly on the row.hit.lexical_scorewould be simpler and, unlike the JSON round-trip, would fail loudly atCREATE FUNCTIONtime if that column were ever renamed/dropped — the currentcoalesce(nullif(to_jsonb(hit)->>'lexical_score','')::double precision, hit.hybrid_score)would instead silently substitutehybrid_scorefor every row.♻️ Suggested simplification
- hit.document_labels, hit.document_summary, hit.similarity, hit.text_rank, hit.hybrid_score, - coalesce(nullif(to_jsonb(hit)->>'lexical_score', '')::double precision, hit.hybrid_score) as lexical_score, - hit.images + hit.document_labels, hit.document_summary, hit.similarity, hit.text_rank, hit.hybrid_score, + coalesce(hit.lexical_score, hit.hybrid_score) as lexical_score, + hit.images from public.match_document_chunks_text($1, $2, $3, $4) hit(apply the same change to the second branch)
🤖 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 `@supabase/schema.sql` around lines 6874 - 6910, In both branches of match_document_chunks_text_v2, replace the to_jsonb(hit)->>'lexical_score' conversion and fallback with direct access to hit.lexical_score. Preserve the existing combined, deduplication, and ordering behavior while applying the same simplification to both match_document_chunks_text calls.
6856-7108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated sentinel UUID literal across every wrapper is a DRY smell.
'00000000-0000-0000-0000-000000000000'::uuidis hard-coded roughly 20 times across the 12 new wrapper functions. A singleimmutablehelper (e.g.public.public_owner_sentinel() returns uuid) would remove the duplication and eliminate the risk of a silent typo in one of the literals (a mistyped UUID here wouldn't error, it would just quietly stop matching public rows).🤖 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 `@supabase/schema.sql` around lines 6856 - 7108, Replace the repeated `'00000000-0000-0000-0000-000000000000'::uuid` literals throughout the 12 wrapper functions with a single immutable public helper such as `public.public_owner_sentinel()`. Use that helper for default owner_filter values, public-row calls, and sentinel comparisons in functions including `corpus_topic_term_stats_v2`, `match_document_chunks_text_v2`, and the other `_v2`/`_v3` wrappers, preserving all existing behavior.tests/supabase-schema.test.ts (1)
957-958: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toContain("for update")is a weak assertion; it also matches unrelated text like "for updated_at".Because "updated" starts with "update", any incidental text containing "for updated..." would also satisfy this assertion, so it doesn't reliably confirm the
FOR UPDATErow-locking clause is present in the job-claiming logic. Consider anchoring the check (e.g. a regex requiring a word boundary, or asserting the fuller"for update skip locked"if that's what the migration uses for concurrent job claiming).🧪 Suggested tightening
- expect(ragRemediationFunctionReconciliationMigration).toContain("for update"); + expect(ragRemediationFunctionReconciliationMigration).toMatch(/\bfor update\b/);🤖 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 `@tests/supabase-schema.test.ts` around lines 957 - 958, Strengthen the assertion for the row-locking clause in ragRemediationFunctionReconciliationMigration by matching the complete job-claiming SQL phrase, such as “for update skip locked,” or using a word-boundary-aware regex. Keep the existing conflict assertion unchanged and ensure incidental text like “for updated_at” cannot satisfy the lock assertion.
🤖 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/20260713083000_reconcile_rag_remediation_functions.sql`:
- Around line 210-212: Update the v_expected_index_unit_count calculation to
cast each jsonb_each_text value through numeric before converting to integer,
preserving the existing coalesce and sum behavior. This must align with the
earlier numeric/trunc validation so values such as 1.0 reach the intended 23514
exception instead of causing a raw integer cast error.
---
Nitpick comments:
In `@supabase/schema.sql`:
- Around line 7021-7046: Update match_document_table_facts_text_v2 to select
fact.metadata directly in both branches of the combined CTE, removing the
coalesce(to_jsonb(fact)->'metadata', '{}'::jsonb) indirection while preserving
the existing result shape and ranking behavior.
- Around line 6843-6854: Update the public.retrieval_owner_matches_v2 function
definition to match retrieval_owner_matches by adding the established SET
search_path = public, pg_catalog and PARALLEL SAFE attributes. Preserve the
existing SQL body and owner-matching behavior.
- Around line 7134-7141: The retrieval_owner_matches_v2 truth-table self-test
only covers include_public = true. Extend the do-block assertions to include a
case with include_public = false and a null row_owner_id, verifying the function
returns the expected false-branch result while preserving the existing
assertions.
- Around line 6874-6910: In both branches of match_document_chunks_text_v2,
replace the to_jsonb(hit)->>'lexical_score' conversion and fallback with direct
access to hit.lexical_score. Preserve the existing combined, deduplication, and
ordering behavior while applying the same simplification to both
match_document_chunks_text calls.
- Around line 6856-7108: Replace the repeated
`'00000000-0000-0000-0000-000000000000'::uuid` literals throughout the 12
wrapper functions with a single immutable public helper such as
`public.public_owner_sentinel()`. Use that helper for default owner_filter
values, public-row calls, and sentinel comparisons in functions including
`corpus_topic_term_stats_v2`, `match_document_chunks_text_v2`, and the other
`_v2`/`_v3` wrappers, preserving all existing behavior.
In `@tests/supabase-schema.test.ts`:
- Around line 957-958: Strengthen the assertion for the row-locking clause in
ragRemediationFunctionReconciliationMigration by matching the complete
job-claiming SQL phrase, such as “for update skip locked,” or using a
word-boundary-aware regex. Keep the existing conflict assertion unchanged and
ensure incidental text like “for updated_at” cannot satisfy the lock assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 096a62f8-c5ff-4a85-a8a4-fc6d650a945e
📒 Files selected for processing (4)
supabase/drift-manifest.jsonsupabase/migrations/20260713083000_reconcile_rag_remediation_functions.sqlsupabase/schema.sqltests/supabase-schema.test.ts
| select coalesce(sum(value::integer), 0) | ||
| into v_expected_index_unit_count | ||
| from jsonb_each_text(coalesce(p_index_unit_counts_by_type, '{}'::jsonb)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm jsonb preserves trailing zeros in text output on the target PG version.
rg -nP -C3 'PG_VERSION|postgres.*version|supabase.*postgres' -g '*.{toml,json,md,yml,yaml}'Repository: BigSimmo/Database
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --short
echo
echo "== file length =="
wc -l supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql
echo
echo "== relevant sections =="
sed -n '40,80p' supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql
echo
sed -n '200,225p' supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sqlRepository: BigSimmo/Database
Length of output: 3353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Probe PostgreSQL jsonb/jsonb_each_text semantics based on accessible docs?
# No local DB assumed; this script will only inspect installed client docs if present.
import shutil, subprocess, json, os, sys, textwrap, re
print("psql:", shutil.which("psql"))
print("postgres:", shutil.which("postgres"))
PYRepository: BigSimmo/Database
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for explicit use of jsonb_each_text / numeric casts in nearby migrations for context.
rg -n "jsonb_each_text|::integer|::numeric|trunc\(" supabase/migrationsRepository: BigSimmo/Database
Length of output: 4994
🌐 Web query:
PostgreSQL jsonb_each_text preserves numeric scale trailing zeros numeric output 1.0 cast to integer invalid input syntax official docs
💡 Result:
PostgreSQL's jsonb data type does preserve trailing fractional zeros in numeric values [1][2]. Because the underlying representation uses the numeric type, it maintains these zeros even though they are semantically insignificant for equality checks [1][2]. When using jsonb_each_text, all values are extracted as text [3][4]. If you encounter an "invalid input syntax" error when casting these text values (e.g., to integer), it is generally due to one of two reasons: 1. Non-numeric data: If the JSON object contains keys with non-numeric values (e.g., strings like "Volume"), attempting to cast those values directly to a numeric type will result in an invalid input syntax error [5][6][7]. 2. Query execution planning: In some versions of PostgreSQL, complex queries (such as those involving sub-SELECTs in a WHERE clause) may cause the database to attempt to cast values before the filter is applied [5][6]. Even if the filter is intended to exclude non-numeric rows, the execution plan might evaluate the SELECT list (containing the cast) before the WHERE clause, leading to an error on rows that should have been filtered out [6][8]. To resolve this casting error, you can use a CTE (Common Table Expression) or a subquery to ensure that filtering is performed before the conversion [6]: WITH filtered_data AS ( SELECT value FROM my_table, jsonb_each_text(data) WHERE key IN (SELECT key FROM week_key_table)) SELECT value::integer FROM filtered_data; This approach forces the engine to isolate the rows containing valid numeric-like strings before attempting the type cast [6].
Citations:
- 1: https://www.postgresql.org/docs/19/datatype-json.html
- 2: https://www.postgresql.org/docs/15/datatype-json.html
- 3: https://docs.yugabyte.com/stable/api/ysql/datatypes/type_json/functions-operators/jsonb-each-text/
- 4: https://github.com/neondatabase/website/blob/main/content/postgresql/json-functions/jsonb_each_text.md
- 5: https://www.postgresql.org/message-id/18356-a811d014209fd19c%40postgresql.org
- 6: https://www.postgresql.org/message-id/2df3715aa60088b5d781c6b4b1dc1ec5ab62669b.camel%40cybertec.at
- 7: https://www.postgresql.org/message-id/CAOfL%3Dzr_TEdAeBnCjoONdH5J3zPa%3D6FtB_ORHGOGoNna7vghuA%40mail.gmail.com
- 8: https://www.postgresql.org/message-id/131235.1708547688%40sss.pgh.pa.us
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the function declaration and variable types around the summed count variable.
sed -n '1,45p' supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql
echo
rg -n "v_expected_index_unit_count|v_total_index_unit_count|v_index_unit_unit|v_index_unit_counts_by_type" supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sqlRepository: BigSimmo/Database
Length of output: 2447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the function declaration and variable types around the summed count variable.
sed -n '1,45p' supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql
echo
rg -n "v_expected_index_unit_count|v_total_index_unit_count|v_index_unit_counts_by_type" supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sqlRepository: BigSimmo/Database
Length of output: 2447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,45p' supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql
echo
rg -n "v_expected_index_unit_count|v_total_index_unit_count|v_index_unit_counts_by_type" supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sqlRepository: BigSimmo/Database
Length of output: 2447
Cast the summed values through numeric
The earlier numeric/trunc() gate accepts 1.0, but jsonb_each_text() still feeds text into ::integer, which can raise a raw cast error before the intended 23514 exception. Cast via numeric here to match the validation path.
🩹 Proposed fix
- select coalesce(sum(value::integer), 0)
+ select coalesce(sum((value::numeric)::integer), 0)
into v_expected_index_unit_count
from jsonb_each_text(coalesce(p_index_unit_counts_by_type, '{}'::jsonb));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| select coalesce(sum(value::integer), 0) | |
| into v_expected_index_unit_count | |
| from jsonb_each_text(coalesce(p_index_unit_counts_by_type, '{}'::jsonb)); | |
| select coalesce(sum((value::numeric)::integer), 0) | |
| into v_expected_index_unit_count | |
| from jsonb_each_text(coalesce(p_index_unit_counts_by_type, '{}'::jsonb)); |
🤖 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 `@supabase/migrations/20260713083000_reconcile_rag_remediation_functions.sql`
around lines 210 - 212, Update the v_expected_index_unit_count calculation to
cast each jsonb_each_text value through numeric before converting to integer,
preserving the existing coalesce and sum behavior. This must align with the
earlier numeric/trunc validation so values such as 1.0 reach the intended 23514
exception instead of causing a raw integer cast error.
1 participant
Summary
supabase/schema.sqlWhy
The reviewed remediation migrations were applied to production, but post-apply drift found two integration gaps:
request_indexing_v3_enrichmentandcommit_document_deep_memory_generationdiffered from current main because their original migration versions had already been recorded before the canonical definitions landed.The new migration uses
CREATE OR REPLACEand reasserts service-role-only ACLs. No table rewrite or data backfill is included.Verification
npm run verify:cheap: 209 test files passed, 1 skipped; 1,921 tests passed, 1 skipped; lint and typecheck passednpm run drift:manifest: scratch PostgreSQL schema replay passed; manifest regeneratednpx supabase db push --linked --dry-run: only20260713083000_reconcile_rag_remediation_functions.sqlwould applygit diff --check: passedSafety
Production mutation remains gated until this PR's required CI and migration replay pass. The follow-up apply is a single forward migration; no raw SQL is used.