feat: rebase analysis-run registry onto current #74 head - #107
Conversation
The fail-closed rollback script starts an explicit transaction. On an autocommit connection a RAISE left that transaction aborted, so the empty-registry cleanup could not run.
![opencode-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Buyer gap: after #95 the home Analysis runs row was inert text. Clicking the seeded Demo Corp lineage run now loads GET /api/analysis-runs/{id} and shows cutoff, requested date, and document count. Hidden runs stay not-visible. Synthetic aggregates only -- never a DSN or source SQL.
Buyer gap: after #100 the detail showed cutoff and counts but not the legal lifecycle the registry already stored. GET /api/analysis-runs/{id} now returns labeled status_history (Pending → Running → Succeeded with occurrence times). The list stays latest-status only. Hidden runs still 404 and never leak events. Failure codes stay machine tokens. Synthetic Demo Corp seed only.
Buyer gap: after #102 the run detail showed history but no way to open a post. Detail now lists ABAC-visible titles in the run's scope. Other-corp private posts stay hidden. List payloads stay aggregates-only. Synthetic titles only.
PR #91 landed an adaptive-orchestration ADR 0013 on the #74 base after this slice already used 0013 for the normalized analysis-run registry. Renumber the adaptive record to 0015 so ADR numbers stay unique. Co-authored-by: Seongho Bae <[email protected]>
The #74 changelog fold still called that decision ADR 0013. This stack keeps the analysis-run registry as ADR 0013, so the adaptive record is 0015. Co-authored-by: Seongho Bae <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
|
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. |
f27ecfb
into
feat/role-responsibility-agent-ontology
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
CodeRabbit CLI 0.7.3 is installed in this environment, but coderabbit auth login --agent failed with automatic_login_failed (browser timeout). The GitHub CodeRabbit check on this PR is a skip because the base is not the default branch. This review is the HEAD diff 59cde23...955d0b0.
Request changes before treating this slice as buyer-safe. Do not merge before #74. Do not treat the thin Checks on this non-main base as a pass — Tests and PROV-O only trigger on main.
Change these next
- Read path ignores knowledge cutoff. Write-time guards require
maximum_available_time <= knowledge_cutoff <= requested_at, butfetch_visible_scope_postslists livesource_postrows with nocreated_at <= knowledge_cutoffand no snapshot membership. A buyer opening the 2026-01-12 run can see later posts. Pass the cutoff into that query and add a post created after cutoff that must stay absent. - Re-seed dies on the count-freeze trigger.
enforce_analysis_source_count_freezeisBEFORE INSERTand runs beforeON CONFLICT. The secondmake seedraisesanalysis_source_count_frozen_after_run. Insert counts only when the snapshot is new, orINSERT … SELECT … WHERE NOT EXISTS. Calendar seed is already documented as idempotent; this path should match. - Authorization tests do not exercise production SQL.
_visible_idsomits theanalysis_scope_thread_groupclause and does not import_VISIBLE_RUN_SQL. Hiddenall_visibledetail is never GETted. Drive tests throughfetch_visible_analysis_run/ the exact predicate, GET both hidden ids, and assert 404 bodies contain nostatus_history,source_counts, or run id. - Thread-group visibility matches the ADR, but counts can still leak. One public post in the group opens the whole run, including snapshot-wide
source_countsthat may include private other-tenant posts. Add the missing HTTP case (Other-Corp thread-group run + one public post). Either 404 the Demo analyst or, if you keep the documented “can see a post” rule, omitsource_countsunless the caller is the requester or entity-affiliated.
What already holds
- 3NF registry, two-or-more-word relation names, temporal write guards, immutability, fail-closed rollback.
- Other-corp and outsider
all_visibleomitted from the list; other-corp detail 404; payloads are labels and aggregates; synthetic Demo Corp only. - Stacked rebase hygiene: one app, no second React app, Keyverse identity service, file DB, source SQL, or real organization identifiers.
- ADR 0013 = registry, 0014 = authorized read, 0015 = adaptive (renamed after #74 reused 0013). Keep that rename when retargeting to
main.
After the fixes
Re-run the focused registry, authorization, and frontend tests against local PostgreSQL 16. Retarget onto protected main only after #74.
Non-blocking: retryable is a one-word column; hashlib is imported inline in seed_demo_data.py; doctoring still says the API is deferred.
Sent by Cursor Automation: fix all
| detail["visible_posts"] = await fetch_visible_scope_posts( | ||
| conn, | ||
| row["scope_kind_code"], | ||
| row["corporate_entity_id"], | ||
| row["process_unit_id"], | ||
| row["scope_key"], | ||
| affiliated_entity_ids, | ||
| ) |
There was a problem hiding this comment.
Write-time guards keep maximum_available_time <= knowledge_cutoff <= requested_at, but this call never passes knowledge_cutoff (or snapshot membership) into fetch_visible_scope_posts.
A buyer opening the 2026-01-12 run can then see posts created after the cutoff. Pass row["knowledge_cutoff"] through and filter created_at <= $cutoff. Add an HTTP test with a post created after cutoff that must stay absent from visible_posts.
| or ( | ||
| scope.scope_kind_code = 'analysis_scope_thread_group' | ||
| and exists ( | ||
| select 1 from source_post p | ||
| where p.thread_group_key = scope.scope_key | ||
| and ( | ||
| p.visibility_code = 'public' | ||
| or p.corporate_entity_id = any($2::uuid[]) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This matches ADR 0014 / ARCHITECTURE (“visible when the account can already see a post in that group”), but the run payload still returns snapshot-wide source_counts. Those counts can include private other-tenant posts the caller cannot open.
Add the missing HTTP case: Other-Corp thread-group run + one public post. Either 404 the Demo analyst, or keep this predicate and omit source_counts unless the caller is the requester or entity-affiliated. _visible_ids in tests/test_analysis_run_authorization.py currently drops this clause, so the case cannot fail there.
| def _visible_ids(cursor, account_id: str, entity_ids: list[str]) -> set[str]: | ||
| """Apply the same visibility predicate the product API uses.""" | ||
| cursor.execute( | ||
| """ | ||
| select run.analysis_run_id | ||
| from analysis_run run | ||
| join analysis_run_scope scope on scope.analysis_run_id = run.analysis_run_id | ||
| where | ||
| run.requested_by_account_id = %s | ||
| or ( | ||
| scope.scope_kind_code = 'analysis_scope_corporate_entity' | ||
| and scope.corporate_entity_id = any(%s::uuid[]) | ||
| ) | ||
| or ( | ||
| scope.scope_kind_code = 'analysis_scope_process_unit' | ||
| and exists ( | ||
| select 1 from account_affiliation aff | ||
| where aff.user_account_id = %s | ||
| and aff.process_unit_id = scope.process_unit_id | ||
| ) | ||
| ) | ||
| """, | ||
| (account_id, entity_ids, account_id), | ||
| ) | ||
| return {str(row[0]) for row in cursor.fetchall()} |
There was a problem hiding this comment.
This reimplements a weaker predicate than _VISIBLE_RUN_SQL: no analysis_scope_thread_group clause, no process-unit fixture, and no import of the production string.
A thread-group leak or drift from analysis_run_ingestion.py would still pass. Drive this file through fetch_visible_analysis_runs / fetch_visible_analysis_run (or the exact _VISIBLE_RUN_SQL text) and add process-unit plus thread-group cases, including the public-post cross-tenant case.
| hidden = client.get( | ||
| f"/api/analysis-runs/{seeded_db['hidden_run_id']}", | ||
| headers={"Authorization": f"Bearer {demo_analyst_token}"}, | ||
| ) | ||
| assert hidden.status_code == 404 |
There was a problem hiding this comment.
This asserts other-corp detail is 404, but never GETs hidden_all_visible_id, and the 404 body is not checked.
GET both hidden ids. Expect 404 and assert the body has no status_history, no source_counts, and no run id. A regression that 403s or returns events for a hidden all-visible run would still pass today.
| cur.execute( | ||
| """ | ||
| insert into analysis_source_count | ||
| (analysis_source_snapshot_id, count_type_code, count_value) | ||
| values | ||
| (%s, 'analysis_count_document', 3), | ||
| (%s, 'analysis_count_thread', 1), | ||
| (%s, 'analysis_count_lineage_node', 5), | ||
| (%s, 'analysis_count_lineage_edge', 4) | ||
| on conflict do nothing | ||
| """, | ||
| (snapshot_id, snapshot_id, snapshot_id, snapshot_id), | ||
| ) |
There was a problem hiding this comment.
enforce_analysis_source_count_freeze is BEFORE INSERT and runs before ON CONFLICT. On a second make seed the snapshot and run already exist, so this insert raises analysis_source_count_frozen_after_run.
Insert counts only when the snapshot row was just created, or INSERT … SELECT … WHERE NOT EXISTS. Calendar seed is already documented as idempotent; this path should match.
2 participants
Buyer impact
Keeps Milestone 2 slice 1 (normalized analysis-run registry) stacked on the current PR #74 head so operators still get the durable derivation root without merging before #74.
This is the same additive registry as #89, replayed onto the latest
feat/role-responsibility-agent-ontologyhead. It does not open a path tomain.Refs #79 and #89; does not close them.
Exact stack boundary
feat/role-responsibility-agent-ontology).59cde232fbf2b4876a5acb0c1873a364ac3d8eeb.feat/analysis-run-registry-v079-clean(PR feat: add normalized analysis-run registry (Milestone 2 slice 1) #89).What stayed
0018_analysis_run_registry.sqlsnake_case3NF objectsmaximum_available_time <= knowledge_cutoff <= requested_atADR numbering on this stack
#91 landed adaptive-orchestration as ADR 0013 on the #74 base after this slice already used 0013 for the registry. The registry document remains ADR 0013. The adaptive record is ADR 0015 here so numbers stay unique.
Verification
Tests/PROV-O contractworkflows still trigger only on PRs tomain.Merge gates
59cde232fbf2b4876a5acb0c1873a364ac3d8eeb.main.main.