feat(identity): persist data-rights request receipts - #138
Conversation
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughPostgreSQL 데이터 권리 요청 원장과 상태 전환 로직을 추가했다. 원장은 멱등성 키와 SHA-256 다이제스트를 검증하고, 완료 영수증을 보존한다. 단위 테스트와 PostgreSQL 통합 테스트가 재생, 충돌, 입력 검증 및 원본 삭제 후 보존을 확인한다. Changes데이터 권리 요청 원장
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant 요청 처리
participant PostgresDataRightsRequestLedger
participant PostgreSQL
요청 처리->>PostgresDataRightsRequestLedger: beginRequest(input)
PostgresDataRightsRequestLedger->>PostgreSQL: pending 요청 삽입
PostgreSQL-->>PostgresDataRightsRequestLedger: 생성 행 또는 멱등성 충돌
PostgresDataRightsRequestLedger-->>요청 처리: created 또는 replayed
요청 처리->>PostgresDataRightsRequestLedger: completeRequest(input)
PostgresDataRightsRequestLedger->>PostgreSQL: pending 행 완료 처리
PostgreSQL-->>PostgresDataRightsRequestLedger: 완료 행 또는 기존 행
PostgresDataRightsRequestLedger-->>요청 처리: completed 또는 replayed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/identity-service/src/data-rights-request-ledger.integration.test.ts (2)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 데이터베이스 이름이 상수와 리터럴로 이중 정의됩니다.
TEST_DATABASE_NAME은 라인 14에 정의되어 연결 URL 구성(라인 45)에만 사용됩니다.DROP DATABASE와CREATE DATABASE문은 동일한 이름을 리터럴로 반복합니다. 두 값이 어긋나면 테스트는 한 데이터베이스를 만들고 다른 데이터베이스에 연결하거나, 정리에 실패합니다.데이터베이스 이름은 식별자이므로 바인딩 파라미터로 전달할 수 없습니다. 상수를 검증한 뒤 인용 처리하여 삽입하세요.
♻️ 제안 수정
const TEST_DATABASE_NAME = 'life_os_data_rights_ledger_test'; +if (!/^[a-z][a-z0-9_]*$/u.test(TEST_DATABASE_NAME)) { + throw new Error('TEST_DATABASE_NAME must be a safe PostgreSQL identifier'); +} +const DROP_TEST_DATABASE = `DROP DATABASE IF EXISTS "${TEST_DATABASE_NAME}" WITH (FORCE)`; +const CREATE_TEST_DATABASE = `CREATE DATABASE "${TEST_DATABASE_NAME}"`;- await adminPool.query( - 'DROP DATABASE IF EXISTS life_os_data_rights_ledger_test WITH (FORCE)', - ); - await adminPool.query('CREATE DATABASE life_os_data_rights_ledger_test'); + await adminPool.query(DROP_TEST_DATABASE); + await adminPool.query(CREATE_TEST_DATABASE);- await adminPool.query( - 'DROP DATABASE IF EXISTS life_os_data_rights_ledger_test WITH (FORCE)', - ); + await adminPool.query(DROP_TEST_DATABASE);Also applies to: 61-63
🤖 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 `@apps/identity-service/src/data-rights-request-ledger.integration.test.ts` around lines 39 - 42, Update the test database setup and cleanup around TEST_DATABASE_NAME to use the constant instead of duplicated name literals. Validate the constant as a safe database identifier, quote it appropriately, and interpolate the validated identifier into both DROP DATABASE and CREATE DATABASE statements, while preserving the existing connection URL usage.
68-139: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win통합 테스트가 데이터베이스 제약의 강제를 증명하지 않습니다.
이 스위트는 영수증 보존 하나만 검증합니다. 마이그레이션이 선언한 다음 제약은 실제 PostgreSQL에서 검증되지 않습니다.
data_rights_request_completion_consistentdata_rights_request_time_orderdata_rights_request_digest_valid및data_rights_receipt_digest_validdata_rights_request_kind_valid또한
completeRequest의 재생 경로와 충돌 경로가 실제 데이터베이스에서 검증되지 않습니다. 단위 테스트는 SQL 클라이언트를 모사하므로 제약 강제를 증명할 수 없습니다.제약 위반 INSERT가 거부되는지, 완료된 요청의 재생이 동일 영수증을 반환하는지, 다른 영수증이
DataRightsRequestConflictError로 실패하는지 확인하는 테스트를 추가하세요.코딩 가이드라인에 따라: "Tests must prove realistic domain accuracy and failure behavior, not only mocked call counts."
🤖 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 `@apps/identity-service/src/data-rights-request-ledger.integration.test.ts` around lines 68 - 139, Expand the integration coverage around PostgresDataRightsRequestLedger to exercise the database constraints data_rights_request_completion_consistent, data_rights_request_time_order, data_rights_request_digest_valid, data_rights_receipt_digest_valid, and data_rights_request_kind_valid with invalid INSERTs that are rejected. Also verify completeRequest’s real database behavior: replaying an already completed request returns the original receipt, while supplying a different receipt fails with DataRightsRequestConflictError.Source: Coding guidelines
apps/identity-service/src/data-rights-request-ledger.test.ts (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win저장 행 모사가 실제
pg드라이버 반환 형태와 다릅니다.
storedRow는requested_at과completed_at을 ISO 문자열로 제공합니다.pg는timestamptz컬럼을 기본적으로 JavaScriptDate객체로 파싱합니다. 따라서parseInstant의value instanceof Date분기는 이 단위 테스트에서 실행되지 않습니다.
Date값을 사용하는 케이스를 최소 하나 추가하여 실제 드라이버 경로를 검증하세요.♻️ 제안 보강
it('returns an exact durable replay after an idempotency conflict', async () => { - const client = new RecordingSqlClient([[], [storedRow()]]); + const client = new RecordingSqlClient([ + [], + [storedRow({ requested_at: new Date(REQUESTED_AT) })], + ]); const ledger = new PostgresDataRightsRequestLedger(client);코딩 가이드라인에 따라: "Tests must model realistic domain outcomes, not only mocked implementation calls."
🤖 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 `@apps/identity-service/src/data-rights-request-ledger.test.ts` around lines 39 - 53, Update the storedRow test fixture to model pg’s timestamptz output by using JavaScript Date values for the timestamp fields, and add at least one case that explicitly supplies Date values so parseInstant’s value instanceof Date branch is exercised. Keep the existing timestamp behavior and assertions for other cases unchanged.Source: Coding guidelines
apps/identity-service/migrations/0006_data_rights_request_ledger.sql (1)
24-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift완료 후 영수증 불변성을 데이터베이스에서 강제하는 방안을 검토하세요.
현재 불변성은
completeRequest의AND request_status = 'pending'조건에만 의존합니다. 다른 코드 경로나 운영자 세션이 완료된 행의receipt_digest또는completed_at을 직접 갱신하면 감사 증거가 덮어써집니다. 감사 증거 보존이 이 원장의 핵심 계약이므로,BEFORE UPDATE트리거로 완료 상태 전환 후 터미널 필드 변경을 거부하는 방법을 고려하세요.♻️ 트리거 기반 불변성 강제 예시
CREATE FUNCTION identity.reject_completed_data_rights_mutation() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN IF OLD.request_status = 'completed' AND (NEW.receipt_digest IS DISTINCT FROM OLD.receipt_digest OR NEW.completed_at IS DISTINCT FROM OLD.completed_at OR NEW.request_status IS DISTINCT FROM OLD.request_status) THEN RAISE EXCEPTION 'completed data-rights receipt is immutable'; END IF; RETURN NEW; END; $$; CREATE TRIGGER data_rights_receipt_immutable_guard BEFORE UPDATE ON identity.data_rights_requests FOR EACH ROW EXECUTE FUNCTION identity.reject_completed_data_rights_mutation();코딩 가이드라인에 따라: "AI proposals must be inert, explainable suggestions and must not silently mutate user-owned data."
🤖 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 `@apps/identity-service/migrations/0006_data_rights_request_ledger.sql` around lines 24 - 31, 데이터베이스 제약만으로는 완료된 원장의 감사 필드 변경을 막을 수 없으므로, 완료 상태 전환 후 터미널 필드가 수정되지 않도록 `identity.data_rights_requests`에 `BEFORE UPDATE` 트리거와 전용 함수 `identity.reject_completed_data_rights_mutation`을 추가하세요. 기존 행이 `completed`인 경우 `request_status`, `receipt_digest`, `completed_at` 변경을 거부하고, 그 외 업데이트와 완료로의 정상 전환은 그대로 허용해야 합니다.Source: Coding guidelines
🤖 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 `@apps/identity-service/src/data-rights-request-ledger.integration.test.ts`:
- Around line 58-66: Update the afterAll cleanup hook so pool.end() failures
cannot skip adminPool database removal and connection cleanup; use separate
try/finally cleanup stages around pool and adminPool operations while preserving
the existing DROP DATABASE behavior.
- Around line 12-13: Update the migration-path resolution used by this
integration test to derive migrations from the test file’s directory via the
file/directory reference, rather than process.cwd(). Keep the path fixed to the
identity service’s migrations directory so it works regardless of the Vitest or
monorepo working directory; leave DATABASE_URL and describeWithDatabase
unchanged.
In `@apps/identity-service/src/data-rights-request-ledger.ts`:
- Around line 267-297: Update the insert in the request-creation method to use
an unconstrained ON CONFLICT DO NOTHING so request_id primary-key conflicts are
absorbed; extend the follow-up lookup to match both idempotency identity and
request_id, mapping any two-row result from oneOrUndefined to
DataRightsRequestConflictError instead of invalidPersistence. Ensure
requireReplayIdentity also validates requestId consistency where the replay
contract requires it, while preserving the intended new-requestId replay
behavior.
In `@docs/superpowers/plans/2026-08-04-data-rights-orchestration-slice.md`:
- Around line 12-13: 문서의 “PR `#138`” 참조를 제거하고 해당 기능이 구현되었다는 상태만 남기세요. 또한
CHANGELOG.md의 Unreleased 절에 data-rights-request-ledger.ts와
0006_data_rights_request_ledger.sql 변경 사항을 추가하세요.
---
Nitpick comments:
In `@apps/identity-service/migrations/0006_data_rights_request_ledger.sql`:
- Around line 24-31: 데이터베이스 제약만으로는 완료된 원장의 감사 필드 변경을 막을 수 없으므로, 완료 상태 전환 후 터미널
필드가 수정되지 않도록 `identity.data_rights_requests`에 `BEFORE UPDATE` 트리거와 전용 함수
`identity.reject_completed_data_rights_mutation`을 추가하세요. 기존 행이 `completed`인 경우
`request_status`, `receipt_digest`, `completed_at` 변경을 거부하고, 그 외 업데이트와 완료로의 정상
전환은 그대로 허용해야 합니다.
In `@apps/identity-service/src/data-rights-request-ledger.integration.test.ts`:
- Around line 39-42: Update the test database setup and cleanup around
TEST_DATABASE_NAME to use the constant instead of duplicated name literals.
Validate the constant as a safe database identifier, quote it appropriately, and
interpolate the validated identifier into both DROP DATABASE and CREATE DATABASE
statements, while preserving the existing connection URL usage.
- Around line 68-139: Expand the integration coverage around
PostgresDataRightsRequestLedger to exercise the database constraints
data_rights_request_completion_consistent, data_rights_request_time_order,
data_rights_request_digest_valid, data_rights_receipt_digest_valid, and
data_rights_request_kind_valid with invalid INSERTs that are rejected. Also
verify completeRequest’s real database behavior: replaying an already completed
request returns the original receipt, while supplying a different receipt fails
with DataRightsRequestConflictError.
In `@apps/identity-service/src/data-rights-request-ledger.test.ts`:
- Around line 39-53: Update the storedRow test fixture to model pg’s timestamptz
output by using JavaScript Date values for the timestamp fields, and add at
least one case that explicitly supplies Date values so parseInstant’s value
instanceof Date branch is exercised. Keep the existing timestamp behavior and
assertions for other cases unchanged.
🪄 Autofix
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: b5932c5f-035e-49af-a50c-cec756187047
📒 Files selected for processing (5)
apps/identity-service/migrations/0006_data_rights_request_ledger.sqlapps/identity-service/src/data-rights-request-ledger.integration.test.tsapps/identity-service/src/data-rights-request-ledger.test.tsapps/identity-service/src/data-rights-request-ledger.tsdocs/superpowers/plans/2026-08-04-data-rights-orchestration-slice.md
|
@coderabbitai review |
|
* test(planning): define durable Today synchronization contract * feat(planning): implement durable Today domain contract * test(planning): define atomic Today persistence contract * feat(planning): persist Today with atomic optimistic writes * feat(planning): add durable Today persistence schema * feat(planning): compose durable Today runtime * test(planning): define Today HTTP precondition boundary * feat(planning): enforce authenticated Today HTTP preconditions * feat(planning): expose authenticated Today sync API * fix(planning): avoid duplicate migration ordinal * fix(planning): order durable Today migration after repository constraints * test(planning): verify durable Today PostgreSQL behavior * test(web): define authenticated Today sync BFF contract * feat(web): add authenticated durable Today BFF * feat(web): expose same-origin Today sync route * test(web): include durable Today sync in quality gates * test(web): define explicit local-to-workspace Today migration * feat(web): implement explicit local-to-workspace Today migration * test(web): verify workspace Today migration path * feat(web): label local and durable Today states * feat(web): localize durable Today sync states * feat(web): add explicit durable Today controls * feat(web): connect explicit workspace Today controls * test(web): verify explicit durable Today migration * test(ci): require browser journey verification * ci(web): execute Playwright buyer journeys * test(web): verify stale Today reconciliation * docs(research): ground durable Today synchronization * docs(planning): add durable Today operations runbook * docs(planning): design durable Today workspace sync * docs(planning): plan durable Today workspace sync * test(web): preserve local Today across retry * test(web): scope accessibility live-region assertions * test(web): bind durable search acceptance to semantic controls * test(web): follow the current Today capture label * style(planning): restore canonical provider formatting * style(planning): format concurrent Today assertion * fix(web): remove unreachable Today save disabled check * fix(web): narrow optional Today request body * test(web): make Today fetch fixtures exact-optional safe * test(web): preserve explicit optional fetch init in Today sync * style(web): format Today workspace synchronization * fix(planning): acquire Today advisory locks in order * test(planning): enforce deterministic Today lock order * test(planning): stress identical Today replay concurrency * style(web): format Today sync boundary * style(web): format Today workspace client * style(web): format Today sync tests * style(web): format Today workspace tests * test(planning): match SQL client result contract * test(planning): isolate lock-order integration fixture * test(planning): move lock-order fixture out of production source * test(planning): remove dynamic SQL from lock-order fixture * style(web): format Today workspace sync client * style(web): format Today sync boundary * style(web): format Today sync tests * fix(web): parse strong ETags with valid regex syntax * fix(web): use valid strong ETag parser in BFF * feat(identity): enforce recent-authentication policy for data rights (#136) Add a fail-closed recent-authentication policy that uses the preserved authentication provenance from #134, distinguishing authentication age from session rotation. Includes test-first boundary, stale/future/malformed provenance rejection, and exact-head CI/security validation. * test(planning): exercise Today concurrency independently * fix(planning): make Today date constraint DateStyle-independent * fix(planning): type Today SQL parameters explicitly * test(identity): bind data-rights ownership to recent authenticated session (#137) * test(identity): define recent authentication gate for data rights * feat(identity): enforce recent authentication policy * test(identity): define authenticated data-rights context boundary * feat(identity): bind data-rights export to recent authenticated session * fix(planning): serialize Today writes in explicit transactions * fix(planning): pin Today transactions to one PostgreSQL connection * test(planning): cover transactional Today persistence * test(planning): verify transaction lifecycle and cleanup * test(ci): bind Today concurrency to contributor head * test(planning): remove SQL-text lock-order surrogate * ci: capture exact Today prettier patch * test(web): expose in-flight Today save overwrite * fix(web): preserve edits during Today save * ci: apply bounded Today formatting * ci: expose read-only Today format patch * style(web): format Today sync client * style(web): format Today workspace sync * style(web): format Today sync tests * chore(ci): remove Today format diagnostic * test(planning): reject malformed Today lookup scope before SQL * ci: verify Today lookup validation red * ci: apply verified Today lookup validation * fix(ci): compare repair lease to contributor head * fix(ci): include staged self-removal in repair lease * test(planning): reject malformed Today repository lookups * test(planning): distinguish corrupted Today persistence * fix(planning): classify invalid Today persistence separately * fix(planning): validate Today lookup scope before SQL * test(planning): fail explicitly on leaked Today connections * chore(ci): remove superseded Today repair workflow * fix(web): lint complete source globs * test(planning): make Today concurrency cleanup deterministic * docs(today): classify standards publication status * docs(today): align validation and readiness plan * test(planning): define shared Today invariants contract * feat(planning): centralize Today validation invariants * refactor(planning): reuse shared Today invariants * refactor(planning): share Today invariants with persistence * refactor(planning): reuse Today invariants at HTTP boundary * test(planning): import Today persistence error from domain boundary * test(planning): use shared Today persistence error boundary * fix(web): preserve destructive-copy warning in Korean * test(ci): bind browser acceptance to its workflow job * test(web): close Today BFF authority branch gaps * ci(web): stage one-shot canonical formatter * style(web): apply canonical formatter output * feat(identity): persist data-rights request receipts (#138) * test(identity): define durable data-rights request ledger * feat(identity): persist data-rights request state * feat(identity): add data-rights request ledger schema * test(identity): preserve data-rights receipts through erasure * fix(identity): retain data-rights receipts after erasure * chore(identity): sequence data-rights request migration * chore(identity): remove duplicate migration sequence * docs(identity): record durable data-rights ledger boundary * test(identity): expose request-id collision as domain conflict * fix(identity): normalize request ledger conflicts * test(identity): harden data-rights ledger integration harness * test(identity): cover dual request conflict evidence * docs(identity): align data-rights ledger implementation status * docs(changelog): record durable data-rights ledger * fix(identity): resolve ledger migration path portably * test(identity): require immutable terminal receipt storage * test(identity): model pg timestamp rows as Date values * test(identity): satisfy pg parameter mutability contract * fix(identity): enforce immutable terminal data-rights receipts * test(identity): assert immutable receipt no-op at database boundary * test(identity): bind migration fixture lock to one PostgreSQL session * test(web): expose Today media-type and conflict coupling * test(web): execute Today review regressions * fix(web): preserve existing test dependencies * fix(web): normalize Today media types and conflict semantics --------- Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Outcome
Advance #55 with a bounded durable PostgreSQL request/receipt ledger for workspace export/erasure orchestration.
Test-first evidence
Two causal RED→GREEN cycles are preserved in branch history:
aae7e2b9192868abe9ae84811f1669be7e1fd78ffailed CI withTS2307becausedata-rights-request-ledger.tsdid not exist. The branch then added the minimum fixed-SQL request ledger and schema.6e24e07bc3aaf9ce512ade33e71faff66e28ab5eran the real PostgreSQL lifecycle and failed because deleting the source workspace/user removed the completed receipt. The migration was changed so opaque audit references are not foreign keys to rows the operation itself must erase. The subsequent CI test stage passes this regression.The migration is sequenced as
0006_data_rights_request_ledger.sqlafter the existing protected-main session-authentication migrations.Implemented on this Draft
export/erasurerequest kindsSecurity/privacy boundary
The retained workspace/user UUIDs are bounded audit/reconciliation references, not authorization sources and not foreign keys. Their retention duration and disposal remain an explicit future privacy/operability policy. This PR does not weaken the authenticated/recent-auth ownership boundary already on protected main.
Deliberate remaining #55 scope
This PR does not claim complete data-rights UX. #55 remains open for:
Remaining before Ready/Merge
Refs #55 and #21.
Summary by CodeRabbit
새로운 기능
문서
테스트