Skip to content

feat(recovery): inspect isolated restore catalog objects - #216

Closed
cursor[bot] wants to merge 6 commits into
mainfrom
cursor/bc-ca28851b-93f5-4723-a387-05d496e18a18-7af3
Closed

feat(recovery): inspect isolated restore catalog objects#216
cursor[bot] wants to merge 6 commits into
mainfrom
cursor/bc-ca28851b-93f5-4723-a387-05d496e18a18-7af3

Conversation

@cursor

@cursor cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Bounded #204 catalog-acceptance slice

This PR starts from protected main d2f1e32271910a6db98a0757d67194ddadca4566. It does not include the #208 pg_dump executor, the #209/#212 pg_restore seam, the #214 five-file overlay, or the #215 receipt binder.

Changed paths:

  • pg_llm_batch/postgres_restore_acceptance.py
  • tests/test_postgres_restore_acceptance.py
  • docs/doctoring/postgres-restore-acceptance.md
  • docs/adr/0018-postgres-restore-catalog-acceptance.md

Test-first history

  • RED 5f3d928 defined the missing isolated-target catalog contract, including a realistic complete catalog that must reappear with the packaged schema.sql SHA-256.
  • GREEN 8f0ef04 adds inspect_postgres_restore_catalog().
  • Branch-coverage close 6ac2670 plus doctoring/ADR e3e0e6f.

Recovery and confidentiality boundary

The probe accepts a caller-owned connection only. It does not parse a DSN, execute pg_dump/pg_restore, or claim restorability. One parameterized pg_class query in current_schema() must show the required packaged-schema tables, the tenant-qualified lifecycle unique index, the tenant-status index, and forced RLS on llm_remote_batch_jobs. A present llm_result_stream_checkpoints table must also be forced. Schema-init targets may omit the checkpoint store.

Evidence is content-free: counts, boolean RLS flags, and the packaged schema hash/size. Lower-layer diagnostics never enter exceptions.

Local evidence

  • 23 acceptance tests passed
  • Related receipt/schema tests: 72 passed with this module
  • coverage run --branch --source=pg_llm_batch.postgres_restore_acceptance: 64 statements, 22 branches, 0 miss, 0 partial = 100%
  • interrogate --fail-under 100 on the new module: 100%

Merge boundary

Merge only the unchanged exact head after every then-live required workflow/check is terminal-success on that head. Do not merge #209 at afbe449. Keep #208/#212 as executor writers and #214 as the canonical overlay.

Refs #204.

Open in Web View Automation 

cursoragent and others added 4 commits August 16, 2026 15:30
Add the failing contract for caller-owned catalog inspection of a
restored PostgreSQL target. Protected main cannot yet prove required
package tables, tenant-status indexes, or forced lifecycle RLS.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Add a caller-owned pg_class probe that accepts a restored target only
when required package tables, tenant-status indexes, and forced
lifecycle RLS are present. The probe does not execute pg_dump or
pg_restore and keeps diagnostics content-free.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Add ADR 0018 and operator doctoring so an isolated restore target is
checked before production traffic is pointed at it.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Cover malformed rows, duplicate relations, missing tenant-status
indexes, and partial RLS flags so the probe stays at 100% branch
coverage.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking exact-head defect on 6ac26702d9ada5bbdca86a738cc12a2464dc85b8: _CATALOG_SQL uses c.relkind = ANY(%s) and c.relname = ANY(%s), but inspect_postgres_restore_catalog() supplies Python tuples for both placeholders. Under Psycopg 3, PostgreSQL arrays are adapted from Python lists; the documented = ANY(%s) pattern requires the sequence argument as a list, not a tuple. The current fake cursor only records parameters, so it cannot expose the real adaptation failure. A real Psycopg/PostgreSQL restore-acceptance call can therefore fail before catalog evaluation even when the isolated target is valid.

Please repair this test-first on this exact lane: add a realistic RED using Psycopg 3 against PostgreSQL (or an equivalent adapter-level proof) that executes the actual ANY() query; pass list-valued array parameters for both relation kinds and relation names while retaining a normal outer execute-parameter sequence; then prove GREEN plus the existing focused/full coverage, confidentiality, and Python 3.14 gates. Keep the caller-owned connection and bounded diagnostics unchanged. Official Psycopg 3 parameter-adaptation documentation explicitly documents Python list → PostgreSQL array and = ANY(%s) with a list.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second exact-head acceptance defect: index presence is currently authenticated by pg_class.relname + relkind='i' only. The probe never joins pg_index back to the indexed table and never verifies key columns, uniqueness, predicate/expression state, or validity/readiness. A restored schema can therefore pass with idx_llm_remote_batch_jobs_tenant_status_observed or uq_llm_remote_batch_jobs_tenant_endpoint_id as a same-named decoy index on the wrong relation or with the wrong semantics, even though this PR claims to prove the tenant-status and tenant-qualified lifecycle indexes.

Add a realistic RED catalog/PostgreSQL case where the expected index name exists but targets the wrong table/columns (and where the uq_ name is non-unique), then bind acceptance to pg_index.indrelid for llm_remote_batch_jobs and the exact protected schema semantics: expected key order/columns, uniqueness where required, valid/ready state, and no unexpected predicate/expression if the packaged schema requires a plain index. Keep evidence content-free and bounded. The earlier Psycopg ANY() adapter defect remains independently blocking.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not merge e74afe8 and do not point production traffic at a restored target from this head.

The two latest commits add a real pg_index + pg_get_indexdef + unique-constraint predicate. By inspection that SQL matches packaged UNIQUE (tenant_scope, endpoint_alias, remote_batch_id) and idx_llm_remote_batch_jobs_tenant_status_observed (tenant_scope, batch_status, last_observed_at). List-valued ANY(%s) parameters also match the Psycopg 3 array adapter.

Those commits do not close the maintainer RED. The new tests only record SQL text and still return a happy-path name snapshot from a fake cursor. A same-name decoy on the wrong table, wrong key order, or a non-unique uq_* index is never created and never rejected. Python-side evaluation still accepts indexes by relname + relkind='i' only.

Keep this PR draft. Do not race #208, #212, #214, #215, or #221. Land the live/adapter decoy proof on a successor that executes the real query, then re-check this exact head only if that successor is unnecessary.

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

Comment on lines +86 to +111
def test_catalog_query_authenticates_index_structure() -> None:
"""Require index ownership, keys, uniqueness, validity, and plain-index shape."""
connection = _RecordingConnection()

inspect_postgres_restore_catalog(connection)

sql = connection.cursor_handle.sql
assert sql is not None
required_fragments = (
"pg_catalog.pg_index",
"indrelid",
"indisunique",
"indisvalid",
"indisready",
"indpred",
"indexprs",
"pg_catalog.pg_get_indexdef",
"llm_remote_batch_jobs",
"tenant_scope",
"endpoint_alias",
"remote_batch_id",
"batch_status",
"last_observed_at",
)
for fragment in required_fragments:
assert fragment in sql

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test never executes _CATALOG_SQL. _RecordingCursor.fetchall() always returns the required names and ignores the query, so a same-name decoy on the wrong table or with the wrong keys cannot fail it.

Add a realistic PostgreSQL (or equivalent adapter) case that creates those decoys and shows inspect_postgres_restore_catalog raise the incomplete catalog error. Keep the evidence content-free.

Comment on lines +43 to +49
Focused tests prove a complete isolated catalog is accepted, a missing
lifecycle table or tenant-status index is incomplete, unforced lifecycle or
checkpoint RLS fails tenant-isolation checks, hostile name subclasses and
lower-layer diagnostics stay out of exceptions, and the probe binds
`current_schema()` with parameters. Coverage for
`pg_llm_batch.postgres_restore_acceptance` is 100% statement and branch.
Public docstrings are complete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification still describes name-presence tests only. The module docstring now claims ownership, key order, uniqueness, validity, readiness, and plain-index shape. Update this section only after a live decoy case exists, so operators are not told the structural claim is already proven.

cursor Bot pushed a commit that referenced this pull request Aug 16, 2026
Succeed the #204 isolated-target gap without racing #212. Operators
must name a live pg_service and a distinct restore-drill service
before pg_restore. DSNs, tenant scope, and same-name reuse fail
closed. Allocate ADR 0021 so the record does not collide with
#216/#219/#221.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
@seonghobae seonghobae closed this Aug 16, 2026
seonghobae added a commit that referenced this pull request Aug 16, 2026
…228)

* test(recovery): require isolated restore-target service names

Add the RED contract for #204 isolated-target identity: a live
pg_service name and a restore-drill name must be exact distinct
libpq service identities. DSNs, tenant scope, subclasses, and
same-name reuse must fail closed before pg_restore.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

* feat(recovery): isolate restore-target libpq service names

Succeed the #204 isolated-target gap without racing #212. Operators
must name a live pg_service and a distinct restore-drill service
before pg_restore. DSNs, tenant scope, and same-name reuse fail
closed. Allocate ADR 0021 so the record does not collide with
#216/#219/#221.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

* fix(recovery): allocate collision-free restore-target ADR 0022

#222 already files ADR 0021 for the workflow-registry audit. Keep the
isolation seam unchanged and retarget this decision, doctoring, and the
documentation contract to 0022 after a fresh open-writer inventory.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

* fix(recovery): require cluster identity for restore-target isolation

Distinct libpq service names are not cluster isolation. Require
caller-owned pg_control_system() identifiers so two aliases for the
same production cluster fail closed before pg_restore.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <me@seonghobae.me>
seonghobae added a commit that referenced this pull request Aug 17, 2026
* test(recovery): require live re-inspection for receipt verification

Add the RED contract for #218's successor: fabricated exact-type
schema/backup evidence objects are not inspection provenance, ADR
numeric prefixes must stay unique, and operator docs must name
backup_artifact_path plus ADR 0020.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

* feat(recovery): verify receipts by re-inspecting current bytes

Succeed #218 by refusing preconstructed evidence objects. The verifier
always hashes the packaged schema and the caller-owned artifact, then
compares those live digests to the stored receipt. Allocate ADR 0020
so the record does not collide with #216's ADR 0018.

Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>

* test(recovery): pin receipt size-only mismatch

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <me@seonghobae.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants