WS-XINT-003-02A: enforce immutable policy identity lineage - #242
Merged
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change introduces immutable review and revision policy identity lineage using ID, generation, and hash triples. It updates persistence, migration, task and checker propagation, activation validation, tests, documentation, and staged 02A/02B policy-mutation contracts. ChangesPolicy lineage and staged activation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/tests/test_projects.py (1)
2033-2064: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the review-policy values once, then reuse them.
The explicit keyword arguments at lines 2057-2062 repeat the defaults used for the digest at lines 2038-2043. If a caller passes
review_policywith any of those keys,ReviewPolicy(...)raisesTypeError: got multiple values for keyword argument. A single merged mapping removes that failure mode and keeps the digest and the row in exact agreement.♻️ Proposed refactor
- values.pop("sla_hours", None) - review_hash = policy_digest( - "review", - ReviewPolicySemantics.model_validate( - { - "review_preference_window_seconds": 3600, - "review_lease_duration_seconds": 1800, - "max_active_review_leases_per_reviewer": 1, - "self_review_allowed": False, - "reject_policy": "close_task", - "finding_evidence_requirement": "optional", - **values, - } - ), - ) + values.pop("sla_hours", None) + values = { + "review_preference_window_seconds": 3600, + "review_lease_duration_seconds": 1800, + "max_active_review_leases_per_reviewer": 1, + "self_review_allowed": False, + "reject_policy": "close_task", + "finding_evidence_requirement": "optional", + **values, + } + review_hash = policy_digest( + "review", ReviewPolicySemantics.model_validate(values) + ) review_id = str(uuid4()) session.add( ReviewPolicy( id=review_id, project_id=project_id, guide_version=guide["version"], policy_generation=1, policy_hash=review_hash, semantics_status="complete", - review_preference_window_seconds=3600, - review_lease_duration_seconds=1800, - max_active_review_leases_per_reviewer=1, - self_review_allowed=False, - reject_policy="close_task", - finding_evidence_requirement="optional", **values, ) )🤖 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 `@backend/tests/test_projects.py` around lines 2033 - 2064, Build one merged review-policy values mapping from the defaults and incoming values before computing the digest, then reuse that mapping for both ReviewPolicySemantics.model_validate and ReviewPolicy in this test setup. Remove the duplicated explicit keyword arguments from ReviewPolicy so caller-provided keys cannot be passed twice and the digest and persisted row remain identical.
🤖 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 `@backend/app/modules/projects/models.py`:
- Around line 423-455: Add SHA-256 hash-shape CheckConstraints for both
ReviewPolicy and RevisionPolicy policy_hash columns. Update
ReviewPolicy.__table_args__ with the established constraint pattern and name it
ck_review_policies_policy_hash_shape; add the equivalent constraint to
RevisionPolicy.__table_args__ named ck_revision_policies_policy_hash_shape,
preserving the required sha256:... format and existing constraints.
In `@backend/app/modules/projects/repository.py`:
- Around line 823-867: Scope the row locks in lock_review_policy and
lock_revision_policy to their respective policy tables by passing
of=ReviewPolicy and of=RevisionPolicy to with_for_update(). Keep the existing
joins and filtering unchanged so only the selected policy row is locked, not the
joined ProjectGuide row.
In `@backend/tests/test_artifact_admission.py`:
- Around line 632-645: The fixture setup around ProjectGuide must avoid flushing
an active guide with null policy selections and must prevent the immutability
trigger from rejecting the later update. Create the guide with draft status,
persist both policies and all selection fields before activation, and update the
row to active in the same final step; alternatively, explicitly suppress the
selection guard for this fixture while retaining valid trigger behavior.
---
Nitpick comments:
In `@backend/tests/test_projects.py`:
- Around line 2033-2064: Build one merged review-policy values mapping from the
defaults and incoming values before computing the digest, then reuse that
mapping for both ReviewPolicySemantics.model_validate and ReviewPolicy in this
test setup. Remove the duplicated explicit keyword arguments from ReviewPolicy
so caller-provided keys cannot be passed twice and the digest and persisted row
remain identical.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d12a4937-0fbd-44e8-896d-ab03ff181604
📒 Files selected for processing (42)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12D2-guide-bound-policy-mutations.md.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/ACTION_CUSTODY.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/CHUNK_MAP.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/PLAN.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/REVIEW_LOG.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/STATUS.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02-policy-mutation-activation.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02A-policy-identity-lineage.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02B-policy-mutation-activation.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-internal-review.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-pr-trust-bundle.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-preimplementation-review.mdbackend/alembic/versions/0046_immutable_review_revision_policy_lineage.pybackend/app/modules/checkers/models.pybackend/app/modules/checkers/runner.pybackend/app/modules/checkers/schemas.pybackend/app/modules/checkers/service.pybackend/app/modules/projects/models.pybackend/app/modules/projects/policy_lineage.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/tasks/models.pybackend/app/modules/tasks/schemas.pybackend/app/modules/tasks/service.pybackend/scripts/api_contract_e2e.pybackend/scripts/week2_api_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_admission.pybackend/tests/test_checkers.pybackend/tests/test_policy_identity_lineage.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/architecture_data_model.mddocs/glossary.mddocs/spec_chunk_5_submission_packet_foundation.mddocs/spec_chunk_6_checker_contract_records.mddocs/spec_review_lifecycle.mddocs/template_project_guide.mdexamples/terminal_benchmark/terminal_benchmark_api_e2e.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_projects.py (1)
173-184: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winStrengthen the schema-contract assertion.
This test checks text fragments only. It would still pass if
ANDchanged toOR, if the hash pattern became broader, or if an unsupported status were added. Compare the normalized compiled expression with the migration contract, or add PostgreSQL cases that reject invalid generations, hashes, and statuses.🤖 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 `@backend/tests/test_projects.py` around lines 173 - 184, Strengthen test_policy_identity_shape_metadata_matches_migration_contract by validating the complete normalized constraint expression rather than independent SQL fragments. Ensure the assertion preserves the migration contract’s AND semantics, exact sha256 hash pattern, positive policy_generation requirement, and only the supported complete and legacy_incomplete statuses; alternatively add PostgreSQL-backed cases that reject invalid generations, hashes, and statuses.
🤖 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.
Nitpick comments:
In `@backend/tests/test_projects.py`:
- Around line 173-184: Strengthen
test_policy_identity_shape_metadata_matches_migration_contract by validating the
complete normalized constraint expression rather than independent SQL fragments.
Ensure the assertion preserves the migration contract’s AND semantics, exact
sha256 hash pattern, positive policy_generation requirement, and only the
supported complete and legacy_incomplete statuses; alternatively add
PostgreSQL-backed cases that reject invalid generations, hashes, and statuses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5849b5ed-9e12-4449-94e9-f64ded2dde4f
📒 Files selected for processing (12)
.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/REVIEW_LOG.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02A-policy-identity-lineage.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-external-review-response.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-internal-review.md.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-pr-trust-bundle.mdbackend/app/modules/projects/models.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/service.pybackend/scripts/run_test_lanes.pybackend/tests/test_artifact_admission.pybackend/tests/test_projects.pybackend/tests/test_tasks.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/reviews/WS-XINT-003-02A-pr-trust-bundle.md
- backend/app/modules/projects/repository.py
- backend/tests/test_tasks.py
- .agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02A-policy-identity-lineage.md
- backend/app/modules/projects/models.py
- backend/tests/test_artifact_admission.py
- backend/app/modules/projects/service.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/test_alembic.py (2)
11867-12082: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore disabled triggers after a failed cleanup.
_remove_xint003_02a_immutable_policiesdisablesusertriggers, then deletes seeded rows with only afinally: engine.dispose(). If an operation fails before the enable-restore loop, the table triggers remain disabled after the test. Add anexcept BaseExceptionblock around the cleanuptry/finallythat rolls back, re-enables the disabled triggers, then re-raises, matchingsuspend_historical_product_custody.🤖 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 `@backend/tests/test_alembic.py` around lines 11867 - 12082, Update _remove_xint003_02a_immutable_policies to recover from cleanup failures: track the tables whose user triggers were disabled, and in an except BaseException handler roll back any active transaction, re-enable those triggers, then re-raise the original exception. Preserve the normal restoration loop and engine disposal, following the recovery pattern used by suspend_historical_product_custody.
11854-11863: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a cross-resource policy lineage test.
The current coverage only asserts existing same-project policy immutability cases. Add an explicit database-level test for rejecting an insert/update linking
$supersedes_policy_id$ to a policy from another project, using both review and revision policy lineage paths.🤖 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 `@backend/tests/test_alembic.py` around lines 11854 - 11863, Add a database-level test alongside the existing immutability assertions that attempts both insert and update operations with supersedes_policy_id referencing a policy from another project, covering both review and revision policy lineage paths. Assert each cross-project link is rejected while preserving the existing same-project immutability expectations.
🤖 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 `@backend/tests/test_guide_bindings.py`:
- Around line 129-183: Move the initial command.downgrade call and engine
creation into the try block so any downgrade failure still triggers restoration
in finally. Initialize engine before try or otherwise guard the finally disposal
because engine may not have been assigned, while preserving the existing
upgrade-to-head cleanup.
---
Outside diff comments:
In `@backend/tests/test_alembic.py`:
- Around line 11867-12082: Update _remove_xint003_02a_immutable_policies to
recover from cleanup failures: track the tables whose user triggers were
disabled, and in an except BaseException handler roll back any active
transaction, re-enable those triggers, then re-raise the original exception.
Preserve the normal restoration loop and engine disposal, following the recovery
pattern used by suspend_historical_product_custody.
- Around line 11854-11863: Add a database-level test alongside the existing
immutability assertions that attempts both insert and update operations with
supersedes_policy_id referencing a policy from another project, covering both
review and revision policy lineage paths. Assert each cross-project link is
rejected while preserving the existing same-project immutability expectations.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 496b493f-be8c-4881-a726-d709bb544ae3
📒 Files selected for processing (10)
.agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02A-policy-identity-lineage.mdbackend/alembic/versions/0047_immutable_review_revision_policy_lineage.pybackend/app/modules/projects/models.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_guide_bindings.pybackend/tests/test_projects.pydocs/architecture_data_model.md
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/app/modules/projects/schemas.py
- .agent-loop/initiatives/WS-XINT-003-rev-auth-end-to-end/chunks/WS-XINT-003-02A-policy-identity-lineage.md
- docs/architecture_data_model.md
- backend/app/modules/projects/models.py
- backend/app/modules/projects/service.py
- backend/tests/test_projects.py
abiorh-claw
self-requested a review
August 2, 2026 08:53
abiorh-claw
previously approved these changes
Aug 2, 2026
abiorh-claw
self-requested a review
August 2, 2026 09:07
abiorh-claw
approved these changes
Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Trust Bundle: WS-XINT-003-02A
Chunk
WS-XINT-003-02A— Immutable Policy Identity And Lineage.Goal and result
ReviewPolicy and RevisionPolicy are now immutable, append-only identities.
ProjectGuide selects exact versions, Task locks those exact facts, and
Submission and CheckerRun copy and database-chain them without treating guide
version as policy version. Both policy mutation actions remain planned and
unavailable.
Design
readiness status, and predecessor lineage.
legacy_incompleterecords; missing lease or preference meaning is neverinvented and readiness denies their future activation.
selections and freezes them.
through downstream foreign keys.
active-selection mutation, and populated downgrade.
activation was added.
Proof
and checker admission: 5 focused cases passed in isolated databases.
git diff --check: passed.targeted 90-percent subsystem gate; no threshold or failure behavior changed.
shared_foundationssemantic-lane custody; the initial fail-closed missing-inventory result was corrected.
Review
Architecture, security/auth, product/operations, QA/test, docs, test-delta, and
CI integrity passed. Senior engineering and reuse/dedup passed with only low,
non-blocking review observations. Every blocking first-round finding was fixed
and re-reviewed.
External review
GitHub Actions and CodeRabbit must review the exact final PR head. Every valid
comment or failing check must be resolved before human merge.
CodeRabbit's complete first-head review produced four valid points across three
inline comments: policy identity metadata, scoped row locking, fixture
activation ordering, and shared test semantics. All are fixed and recorded in
WS-XINT-003-02A-external-review-response.md. Later incremental attempts wererate-limited; exact-head Agent Gates and Backend are the authoritative checks.
Remaining risk and follow-up
This chunk establishes identity and lineage but deliberately activates no
policy writer. WS-XINT-003-02B installs the sole authorized mutation path only
after 02A merges and the user explicitly starts that next chunk.
Human review focus
Confirm deterministic legacy handling, immutable exact identity selection,
complete downstream FK chaining, absence of invented semantics, and zero
runtime policy-action activation.
Human merge ownership
Only the human may merge this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation