WS-XINT-003-02A: enforce immutable policy identity lineage - #242
Open
Abiorh001 wants to merge 6 commits into
Open
WS-XINT-003-02A: enforce immutable policy identity lineage#242Abiorh001 wants to merge 6 commits into
Abiorh001 wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis change introduces immutable review and revision policy identities based on ID, generation, and hash values. It migrates downstream policy locks, validates exact lineage, updates tests and documentation, and separates immutable persistence in 02A from future mutation activation in 02B. ChangesPolicy lineage and staged activation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectGuide
participant PolicyRepository
participant TaskService
participant CheckerService
ProjectGuide->>PolicyRepository: select exact policy identity
TaskService->>PolicyRepository: resolve and validate policy lineage
TaskService->>CheckerService: persist locked policy identity
CheckerService->>CheckerService: validate and expose authorized provenance
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
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
1 participant
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.