feat: add normalized analysis-run registry (Milestone 2.1, v0.76.0) - #105
feat: add normalized analysis-run registry (Milestone 2.1, v0.76.0)#105seonghobae wants to merge 8 commits into
Conversation
Add migration 0012 as an additive Milestone 2.1 persistence boundary on protected main: immutable source snapshots, aggregate counts, account-scoped runs, product scope, append-only status events, and a current-status view. Wire TEPP AnalysisRunRequest to snapshot_id and knowledge_cutoff with a fail-closed transport, keep new orchestrator helpers on mode=auto, and record ADR 0014 plus beginner ERD and APA 7th traceability. Refs #87 #79 Co-authored-by: Seongho Bae <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Use a 64-character SHA-256-shaped snapshot digest, accept the fail-closed missing-snapshot trigger, skip binary files in the public-content walk, and match the HTTPS transport error case-insensitively. Co-authored-by: Seongho Bae <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Reject inverted snapshot clocks, prove status events cannot be deleted, and show concurrent count writes fail after the first run. List the new null analysis-run orchestration client in AGENTS.md. Co-authored-by: Seongho Bae <[email protected]>
There was a problem hiding this comment.
Stale comment
Review of
214588fevsmain(c93d449)Milestone 2.1 as an additive persistence boundary on protected
mainis the right product cut: migration0012, ADR 0014, package 0.72.0. Fail-closed TEPP helpers,NullAnalysisRunOrchestrationClient.available = False, exclusive-shape scope, append-only status as a view, and two-or-more-wordsnake_caseobjects look sound in the SQL. Stacked #107/#89 stay off this merge path — this PR is themainslice.The merge blockers are tests and buyer copy that do not prove what the PR claims. Required
Full test suiteis already green on this head because the live registry fixture skips without Postgres (same pattern astests/test_schema.py). Do not treat that green as proof of0012.Requested changes
- Dead-code constraint tests in
test_registry_rejects_orphans_and_missing_actor—snapshot_not_foundswallows the negative-count, bad-digest, missing-actor, and dangling-entity asserts. Split to onepytest.raisesper statement.- Add a live reject for
knowledge_cutoff > requested_at(table CHECK exists; no runtime insert).- Add a live reject for
pending → failed(pending → succeededis already tested).- Soften CHANGELOG / beginner copy — this slice is schema only. A run can persist with no scope, no counts, and no pending event until the atomic create in ADR follow-up #1 exists.
Residual risk (not a CI-architecture demand in this slice)
The required pytest job has no Postgres service. Keep the local ephemeral suite mandatory, and do not claim CI covered the legal/clock contract.
Next action
Fix the four items above on this branch, re-run the ephemeral Postgres registry suite, and push. Independent review is still required; this comment does not approve or merge.
Sent by Cursor Automation: Fix Issues
Keep one pytest.raises per statement so negative-count, digest, actor, and dangling-entity checks actually run. Reject knowledge_cutoff after requested_at and pending-to-failed. Soften buyer copy: this slice is schema only until the atomic create exists. Co-authored-by: Seongho Bae <[email protected]>
Strix job 95180714827This required Strix run failed with org-scanner infrastructure, not a product finding: The gate then treated that as a hard failure ( I am not suppressing the required Strix gate and not weakening the product to make the scanner pass. Independent review is still required; this comment does not approve or merge. Review 4946471344Pushed
Local ephemeral Postgres registry suite plus doc hygiene: 24 passed. |
There was a problem hiding this comment.
Stale comment
Review of
f0713edvsmain(c93d449)The four items from review 4946471344 are fixed on this head. Those threads are closed.
Verified on
f0713ed
test_registry_rejects_orphans_and_missing_actornow uses onepytest.raisesper statement. Aftersnapshot_not_found, the negative-count CHECK, bad-digest CHECK, missing-actor NOT NULL, and dangling-entity FK actually run.- Live
CheckViolationforknowledge_cutoff > requested_at(00:50cutoff /00:45request) sits next to the capture-after-request and availability-after-capture rejects.- Live reject for
pending → failed(legal failure shape).pending → succeededstays rejected.- CHANGELOG, beginner registry doc, and ARCHITECTURE name the schema-only hole: a raw insert can store a run with no scope, no counts, and no pending event until ADR 0014 follow-up 1.
SQL still looks sound: additive
0012, ADR 0014, package 0.72.0, fail-closed TEPP,NullAnalysisRunOrchestrationClient.available = False, exclusive-shape scope, append-only status as a view.Do not merge this PR onto protected
mainThe maintainer already said this
main-targeted0012/ 0.72.0 slice is superseded by #89+#107 on #74. Landing it now creates a second schema bridge when that stack rebases. Leave this PR; do not treat a green required pytest job as proof of0012(no Postgres service; the live registry suite self-skips).Next action
Continue the canonical stack: #74 → #110 → #111. Close or idle this PR after #74 carries the registry onto
main. Independent review is still required; this comment does not approve or merge.Sent by Cursor Automation: Fix Issues
Pull request was converted to draft
|
Idling this |
There was a problem hiding this comment.
Review of f0713ed vs main (c93d449)
The four items from review 4946471344 stay fixed. Those threads remain closed.
Verified again on this head:
- One
pytest.raisesper orphan/CHECK statement. - Live
CheckViolationforknowledge_cutoff > requested_at. - Live reject for
pending → failed(andpending → succeeded). - CHANGELOG / beginner doc / ARCHITECTURE name the schema-only hole until ADR 0014 follow-up 1.
SQL still looks sound: additive 0012, ADR 0014, package 0.72.0, fail-closed TEPP, NullAnalysisRunOrchestrationClient.available = False, exclusive-shape scope, append-only status as a view.
Do not merge this PR onto protected main
This 0012 / 0.72.0 slice is a second schema bridge. The canonical stack is #74 (already carries merged #89/#107 as 0018 plus authorized read, click-through, cutoff, and TEPP seed). Leave this draft idle. A green required pytest job is not proof of 0012 (no Postgres service; the live registry suite self-skips).
If this slice is ever revived
Two residual honesty gaps remain (inline):
- Rollback comments promise an approved retention delete, but after the first
analysis_runinsert there is no SQL path that can empty the registry. test_concurrent_count_writes_freeze_once_a_run_existsoverclaims a count-vs-first-run lock share.
Next action
Keep landing #74. Do not open another main-targeted registry. Independent review is still required; this comment does not approve or merge.
Sent by Cursor Automation: Fix Issues
| @@ -0,0 +1,70 @@ | |||
| -- Fail-closed rollback for migration 0012. | |||
| -- | |||
| -- Registry evidence must be exported or explicitly deleted under an approved | |||
There was a problem hiding this comment.
This comment tells operators to export or explicitly delete registry evidence under an approved retention procedure. After any analysis_run insert, reject_analysis_run_mutation blocks DELETE, and the snapshot FK then blocks snapshot delete. test_rollback_refuses_data_loss_then_removes_an_empty_registry only inserts a snapshot (which can still be deleted), so it never proves a run-bearing registry can be emptied.
Add a privileged, audited retention procedure that deletes in FK order after an explicit approval token, or rewrite this comment and ADR 0014 to say rollback after the first run requires a superuser trigger disable and is not a supported product path. Until one of those exists, an operator following the documented procedure cannot satisfy analysis_run_registry_not_empty.
|
|
||
|
|
||
| def test_concurrent_count_writes_freeze_once_a_run_exists(registry_db) -> None: | ||
| """Count insert and first run share the snapshot lock; later counts fail.""" |
There was a problem hiding this comment.
The docstring says count insert and first run share the snapshot lock. The body inserts the run first, then races two later count inserts. Both freeze, which only proves post-run freeze under concurrency. A later edit that drops FOR UPDATE on first-run insert can stay green while the lock-order race this comment describes is untested.
Race a count insert against first-run insert on the same snapshot, or narrow this docstring to the post-run freeze the test actually proves.
|
Superseded by the #74 stack, which already carries the analysis-run registry, start, outbox, period-report seed, and Event Lineage landing through v1.4.0. Closing this older main-targeted v0.72.0 vehicle. Do not merge onto main. |
Protected main now owns leftover-pair migration 0012 and package 0.75.0. Keep that chain and land the Milestone 2.1 registry as additive 0013 / ADR 0014 so this slice is no longer a competing 0012 number. Co-authored-by: Seongho Bae <[email protected]>
Strix HIGH vuln-0001: missing kid previously matched the first published key. Require a non-empty kid and an exact JWKS match. Co-authored-by: Seongho Bae <[email protected]>
Strix flagged the curl | sh rustup install in tests.yml as remote script execution. CI now uses a SHA-pinned dtolnay/rust-toolchain action for 1.97.1. The backend image fetches rustup-init 1.28.2 from the versioned archive and verifies its SHA-256 before running it. Co-authored-by: Seongho Bae <[email protected]>
Strix HIGH vuln-0001 flagged missing HEALTHCHECK instructions after the rustup Dockerfile change pulled the full image set into scan scope. Backend and frontend now probe GET /healthz (frontend is a static nginx 200, not the SPA fallback). Postgres uses pg_isready; SearXNG uses its existing /healthz. Raise locked pyjwt and fastapi floors to the versions already in uv.lock. Co-authored-by: Seongho Bae <[email protected]>
| # the SPA -- a missing index.html must not look like a healthy probe. | ||
| location = /healthz { | ||
| default_type text/plain; | ||
| add_header Cache-Control "no-store"; |
|
Superseded by the protected analysis-run lineage already on |


Buyer impact
The product gains a durable, migration-safe schema for a real analysis run: which frozen source capture was used, how large it was in aggregate, who requested it, which Demo Corp product scope applied, which knowledge cutoff governed the work, and which legal lifecycle steps occurred.
This slice is schema only. A raw insert can store a run with no scope, no counts, and no pending event. Treat a run as recorded only after a later write API stores snapshot, counts, run, scope, and pending in one transaction (ADR 0014 follow-up 1).
Refs #87 and #79. Does not close #79.
Rebase onto current main
Protected
mainnow owns leftover-pair migration0012and package0.75.0. This PR keeps that chain and lands the registry as additive0013/ ADR 0014 / 0.76.0. It does not reuse stacked0018or 0.78.0 numbers.What shipped
Migration
0013_analysis_run_registry.sqladds:analysis_source_snapshot— immutable capture digest, source-contract version,maximum_available_time,captured_atanalysis_source_count— one non-negative aggregate per snapshot × count vocabularyanalysis_run— authenticateduser_account, account-scoped idempotency, run-ownedknowledge_cutoff, reproducibility digestsanalysis_run_scope— all-visible | corporate-entity | process-unit | thread-groupanalysis_run_status_event— append-only legal transitionsanalysis_run_current_status— a view, not a second mutable tableLeakage guard (complements TEPP; does not replace TEPP’s six clocks):
CI fixes on this head
vuln-0001onbackend/app/auth.py: a JWT that omittedkidpreviously fell back to the first JWKS key. Verification now requires a non-emptykidand an exact JWKS match. Covered bybackend/tests/test_auth_jwks.py..github/workflows/tests.yml: CI no longer pipeshttps://sh.rustup.rsinto a shell. The pytest job installs rustc 1.97.1 through a SHA-pinneddtolnay/rust-toolchainaction. The backend image fetches rustup-init 1.28.2 from the versioned archive and checks its SHA-256 before running it.vuln-0001on Docker images: product Dockerfiles now declare a realHEALTHCHECKagainst an existing probe (GET /healthzon backend and frontend,pg_isreadyon Postgres, SearXNG/healthz). The frontend probe is a static nginx 200, not the SPA fallback. Locked floors arepyjwt>=2.13.0andfastapi>=0.141.1;react-oidc-contextwas already^3.3.1.eb6ee80failed with GitHub HTTP 503 (gh: No server is currently available) before a verdict. That is infrastructure, not a product finding. A new push retriggers the gate; the gate is not skipped.Verification
Local
pytest: 251 passed, 87 skipped. JWKS kid unit test: 1 passed. The required CI pytest job has no Postgres service; do not treat that green as proof of0013. Independent review is required; this PR does not claim self-approval.Out of scope
Authorized read APIs, Analysis Run Console UI, Valkey outbox, live TEPP measurement, and actual-data acceptance remain later #79 slices.