Skip to content

feat(persistence): persist idempotent analysis runs - #287

Draft
seonghobae wants to merge 11 commits into
mainfrom
feat/analysis-run-persistence
Draft

seonghobae wants to merge 11 commits into
mainfrom
feat/analysis-run-persistence

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Scope

Active persistence writer for tenant-scoped idempotent Analysis Run requests/state history and migration 0008_analysis_run_persistence. This PR remains Draft. It is also the required base for the dependent Membership persistence successor #615; #615 must not bypass this writer or copy its migrations.

Current protected base: main@a243f18da4a4ca8a8d068c39922537f1f8ed6ad0.
Current exact head: 9fd492a3b3873d03460c1393308d3c8d1ca73f83.

Current exact-head evidence

Rust Foundation 34863660899 is not GREEN:

  • Production line and branch coverage: success;
  • Live PostgreSQL integration: success;
  • Repository contracts and Python branch coverage: failure in repository-tooling execution;
  • Format/lint/test/rustdoc/dependency-policy job: formatting, Clippy, tests, doctests and warning-free docs succeed, then dependency/license/advisory/source policy fails.

Other exact-head gates:

  • Documentation Quality 34863660918: success;
  • SAST Semgrep 34863660843: success;
  • Security Scan 34863660788: failure;
  • CodeQL PR 34863661066: failure.

There is no qualifying APPROVED review on this exact head; existing submitted reviews are COMMENTED. The previous body statement that checks are not blockers is stale and is withdrawn. Checks, security findings, ruleset requirements and independent approval remain release/merge gates.

Foundation prerequisite

Canonical protected-main successor #538 f335624cea977fea7d7fe6f36871868557fe2bb7 already carries foundation/workflow and rustls >=0.23.45 repairs that this persistence lane must inherit rather than duplicate. #538 itself is not merge-ready yet: its Security/Dependency Graph and CodeQL/GHAS prerequisites remain unresolved. After #538 lands normally, this branch must reconcile ordinary-forward, preserve every valid 0008 delta/test/fixture/contract, and reacquire exact-head evidence.

Downstream persistence stack

#615 is intentionally stacked on this branch. It owns forward 0009/0010 Membership persistence parity and must be restacked ordinary-forward only after this surviving persistence owner lands. Do not renumber or rewrite historical migrations to shortcut the stack.

Acceptance

Require exact-head Rust/rustdoc/test/owned line+branch+edge coverage, live PostgreSQL, repository-tooling coverage, dependency/license/advisory/source policy, Documentation/Semgrep/Security/CodeQL, resolved valid review threads/findings, qualifying independent approval, and normal protected-main landing. Then #615 must inherit the landed result losslessly and reacquire all applicable gates.

No self-approval, --admin/ruleset bypass, force push, destructive rebase, predecessor-check transfer, gate weakening, warning suppression, or historical migration rewrite is authorized.

Refs #166, #538, #615.

Store canonical tenant-bound requests and append-only lifecycle events with database-enforced transitions and artifact provenance.

Refs: #166

Signed-off-by: Seongho Bae <me@seonghobae.me>
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

분석 실행 요청과 상태 이벤트를 PostgreSQL에 영속화하는 기능을 추가했습니다. 정규 요청 JSON과 SHA-256 digest를 저장하고, 테넌트별 멱등 삽입과 append-only 상태 전이를 적용합니다. Rust SQL 생성 API, 마이그레이션, 계약 테스트, 실제 PostgreSQL 테스트와 관련 문서를 추가했습니다.

Changes

분석 실행 영속화

Layer / File(s) Summary
분석 실행 스키마와 마이그레이션
migrations/0008_analysis_run_persistence.*.sql, crates/persistence_postgres/src/migration.rs, docs/ERD.md
analysis_run_requestanalysis_run_state_event 테이블을 추가했습니다. 상태 전이, artifact digest, 테넌트 격리, FORCE RLS, append-only 변경 차단을 정의했습니다. 롤백 SQL과 ERD도 추가했습니다.
Rust SQL 계약과 공개 API
crates/persistence_postgres/src/analysis_run_sql.rs, crates/persistence_postgres/src/error.rs, crates/persistence_postgres/src/lib.rs, crates/persistence_postgres/Cargo.toml
정규 요청 JSON, SHA-256 digest, 실행 ID와 상태 이벤트를 생성하고 검증하는 타입과 함수를 추가했습니다. 멱등 요청 삽입, 상태 이벤트 삽입, 최신 상태 조회 SQL을 공개했습니다.
계약 검증과 실행 테스트
crates/persistence_postgres/tests/analysis_run_persistence_contract.rs, crates/persistence_postgres/tests/live_postgres.rs
SQL 계약과 입력 검증을 테스트했습니다. 실제 PostgreSQL에서 멱등 재시도, 충돌 요청 거부, Failed 이벤트, 중복 이벤트 거부와 UPDATE 차단을 검증했습니다.
계약 및 설계 기록
docs/API_CONTRACT.md, docs/TRACEABILITY.md, docs/adr/0013-bitemporal-persistence-reproducibility-and-split-authority.md, docs/research/analysis-run-idempotent-persistence.md, CHANGELOG.md
API 계약, 요구사항 추적, ADR 성숙도, 설계 연구 문서와 변경 로그에 분석 실행 영속화 내용을 기록했습니다.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller as 호출자
  participant RustAPI as insert_analysis_run_request_sql
  participant PostgreSQL
  Caller->>RustAPI: AnalysisRunRequestRecord 전달
  RustAPI->>RustAPI: 정규 JSON과 SHA-256 digest 검증
  RustAPI-->>Caller: INSERT SQL 반환
  Caller->>PostgreSQL: 멱등 요청 및 accepted 이벤트 삽입
  PostgreSQL-->>Caller: 저장된 실행 ID와 digest 검증 결과
  Caller->>RustAPI: 상태 이벤트 전달
  RustAPI-->>Caller: 상태 이벤트 INSERT SQL 반환
  Caller->>PostgreSQL: append-only 상태 이벤트 삽입
  PostgreSQL-->>Caller: 상태 전이 및 테넌트 제약 결과
Loading

Merge Risk: 🟡 Moderate · up to 61257

This PR adds durable tenant-scoped analysis-run records and stable retries, but the current head still risks failed exact retries, production traffic blocking during migration, and weakened tenant isolation if callers do not bind tenant context correctly. Merge should wait until these issues are fixed or explicitly accepted; the remaining documentation mismatches are non-blocking.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 6 files. (9 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 테넌트 범위의 멱등 분석 실행 영속화라는 PR의 주요 변경을 정확하고 간결하게 설명합니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 6 files. (9 skipped: 9 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/analysis-run-persistence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Exercise digest-only request tampering and cross-tenant lifecycle events so both trust-boundary short-circuit arms remain covered.

Signed-off-by: Seongho Bae <me@seonghobae.me>
devin-ai-integration[bot]

This comment was marked as resolved.

Signed-off-by: Seongho Bae <me@seonghobae.me>
devin-ai-integration[bot]

This comment was marked as resolved.

Signed-off-by: Seongho Bae <me@seonghobae.me>
devin-ai-integration[bot]

This comment was marked as resolved.

Signed-off-by: Seongho Bae <me@seonghobae.me>
devin-ai-integration[bot]

This comment was marked as resolved.

Signed-off-by: Seongho Bae <me@seonghobae.me>
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae enabled auto-merge (squash) August 28, 2026 11:01

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

Devin Review

Comment thread crates/persistence_postgres/src/sqlx_live.rs
Keep both CHANGELOG.md entries and let cargo regenerate Cargo.lock for
the union of both dependency sets.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@seonghobae

Copy link
Copy Markdown
Contributor Author

Restack on protected main (a243f18) + pre-existing lint finding

Non-force merge of origin/main (merge commit 9fd492a3). Two conflicts, both resolved mechanically:

  • CHANGELOG.md: append conflict, both entries kept.
  • Cargo.lock: took main's file and let cargo metadata regenerate it for the union of both dependency sets (3 lines added, no marker left).

Local evidence on the pushed head (toolchain 1.98.0): cargo fmt --all --check clean, documentation/workspace/docstring contracts PASS, git diff --check clean.

Pre-existing lint failure (not introduced by this restack)

cargo clippy -p persistence_postgres --all-targets -- -D warnings fails with 7 dead-code errors, and the same 7 fail on this branch's head before the merge, so Format, lint, test, rustdoc, and dependency policy cannot go green as-is:

crates/persistence_postgres/src/live_pool.rs:66  field `0` is never read      (Execute(String))
crates/persistence_postgres/src/live_pool.rs:67  field `0` is never read      (ExecuteTransaction(Vec<String>))
                                                 fields `tenant_record_id` and `analysis_run_id` are never read
                                                 variants `Executed`, `LockState`, and `AnalysisRun` are never constructed

These look like a recording test double whose captured SQL and identifiers are never asserted on — the repair is to assert the recorded calls (which also closes a real verification gap), not to silence the lint. I did not touch it: choosing what the double must prove is the writer's call. cargo test could not run for the same reason.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor Author

Current exact-head gate refresh for 9fd492a3b3873d03460c1393308d3c8d1ca73f83 on protected main@a243f18da4a4ca8a8d068c39922537f1f8ed6ad0:

  • Documentation Quality 34863660918: SUCCESS.
  • Semgrep 34863660843: SUCCESS.
  • Rust Foundation 34863660899: overall FAILURE, but Production line+branch coverage and Live PostgreSQL integration are SUCCESS. The format/lint/test/rustdoc job reached formatting, Clippy, test suite, doctests, and warning-free docs successfully, then failed specifically at dependency/license/advisory/source policy. Repository-contract/Python branch coverage failed earlier at the repository-tooling exercise. This is not the old pre-restack dead-code lint diagnosis; hosted Clippy on the current head passed.
  • CodeQL PR 34863661066: FAILURE.
  • Security Scan 34863660788: FAILURE.
  • No qualifying APPROVED review exists on the current head.

No blind rerun or wake commit is justified from this evidence. The next repair must identify the current dependency-policy/Python-tooling/CodeQL/Security causes from logs or canonical owner findings rather than inheriting the older local lint diagnosis.

Dependent persistence parity PR #615 is intentionally based on this exact head, uses successor migration 0009, and remains Draft; it must inherit a repaired/landed #287 through ordinary non-force restack rather than bypass these gates.

Copy link
Copy Markdown
Contributor Author

Fresh hosted RCA on actual head 9fd492a3b3873d03460c1393308d3c8d1ca73f83 narrows the Python/repository-contract failure to one inherited foundation mismatch, not #287 persistence source. Job 104041960622 runs 169 quality tests and fails only test_hourly_workflow_schedule_credentials_and_queue_gate: protected main@a243f18... no longer has a local schedule: in .github/workflows/hourly-nim-product-development.yml, while protected-main tests/quality/test_hourly_nim_product_development.py still expects cron: "47 * * * *".

The canonical foundation successor #538 already carries the causal contract repair at f335624cea977fea7d7fe6f36871868557fe2bb7: its same test explicitly accepts central dispatch only and asserts schedule: is absent. #538 exact Rust Foundation run 35389817094 is GREEN, so copying that test into #287 would duplicate a prerequisite and violate the owner stack. Keep #287 Draft and ordinary-forward inherit #538 after normal #538 landing.

The other Rust Foundation failure is likewise in the dependency-policy step after fmt/Clippy/tests/doctests/rustdoc are GREEN; #538 already carries the rustls >=0.23.45 advisory repair. Current #287 causal order is therefore #538 landing -> non-force restack/reconciliation -> fresh exact-head checks, rather than local leaf workarounds or blind reruns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant