WS-XINT-002-01: reconcile ART authority catalogue - #210
Conversation
|
Warning Review limit reached
Next review available in: 25 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 (5)
📝 WalkthroughWalkthroughThe PR reconciles the ART authorization catalogue by removing obsolete upload-session authority, adding planned submission-bundle and review actions, introducing migration 0036 with evidence-safe constraint rewrites, updating tests and counts, and aligning custody, review, operational, and lifecycle documentation. ChangesART authorization reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthorizationCatalogue
participant Alembic0036
participant PostgreSQL
participant AuthorizationTests
AuthorizationCatalogue->>Alembic0036: define reconciled permissions and actions
Alembic0036->>PostgreSQL: lock evidence tables and inspect records
PostgreSQL-->>Alembic0036: permit or reject constraint rewrite
Alembic0036->>PostgreSQL: update authorization constraints
AuthorizationTests->>PostgreSQL: verify catalogue and evidence behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 (4)
backend/tests/test_alembic.py (2)
1777-1787: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTriplicated obsolete-identifier tuple.
The same six-identifier construction is repeated three times here and mirrors
_REMOVED_PERMISSIONSin the migration. Hoist it to a module-level constant so a future catalogue change can't leave one copy stale.Also applies to: 1953-1963, 8294-8304
🤖 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 1777 - 1787, Define one module-level constant for the six obsolete artifact upload identifiers, then replace the repeated tuple constructions at the shown test locations with references to that constant. Keep the constant aligned with the migration’s _REMOVED_PERMISSIONS identifiers so future catalogue changes update a single source.
1981-1982: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBare
pytest.raises(RuntimeError)weakens the per-predicate proof.The migration raises
RuntimeErrorfrom several places — the evidence guard and theunexpected ... definitionguards inside_rewrite_*. Withoutmatch=, these tests pass even if the upgrade failed for an unrelated reason, which is exactly what this test claims to isolate. Same at lines 2110, 2149, and 2189 (with the downgrade message).💚 Pin the expected message
- with pytest.raises(RuntimeError): + with pytest.raises( + RuntimeError, + match="cannot remove non-empty obsolete artifact authority evidence", + ): command.upgrade(config, "head")Also applies to: 2021-2022, 2058-2059
🤖 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 1981 - 1982, Update each bare pytest.raises(RuntimeError) assertion in the migration tests, including the upgrade cases and the downgrade case, to use match= with the exact expected evidence-guard message for that predicate. Ensure the patterns distinguish these failures from the “unexpected ... definition” RuntimeErrors raised inside _rewrite_* while preserving the existing command.upgrade and downgrade calls.backend/alembic/versions/0036_art_auth_catalogue_reconciliation.py (2)
71-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a post-removal assertion instead of relying only on the pre-count.
definition.replace(", " + token, "")silently no-ops when the token happens to be the first element of the list (no leading", "), yet thecount(token) < 1pre-check already passed. The rewritten constraint would then still admit the obsolete permission with no error. Same pattern at lines 94 and 112.♻️ Assert the token is gone after replacement
for value in remove: token = _permission_token(value) if definition.count(token) < 1: raise RuntimeError(f"unexpected {name} removed permission definition") definition = definition.replace(", " + token, "") + if token in definition: + raise RuntimeError(f"unremoved {name} permission token {value}")🤖 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/alembic/versions/0036_art_auth_catalogue_reconciliation.py` around lines 71 - 82, Update the permission-removal loop in the migration, including the corresponding removal logic at the other referenced locations, to assert that each obsolete token no longer appears in definition after definition.replace. Raise the same unexpected removed-permission error if the post-replacement definition still contains the token, covering replacements that silently no-op for first-list elements.
142-144: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winQualify predicate columns in the joined EXISTS.
The linked-evidence query joins
authority_idempotency_records recordwithaudit_events event, but the reused predicate referencesaction_id,permission_id,target_ref_*, andinvalidation_target_*unqualified. It resolves today only because those columns are absent fromauthority_idempotency_records; any future column addition there turns this into an ambiguity error or a silent semantic change. The test helper_art_catalogue_migration_statealready qualifies withevent..Also consider passing the predicate a column prefix rather than string-replacing bind parameter names —
predicate.replace(':actions', ':linked_actions')is easy to break if a future predicate literal contains those substrings.🤖 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/alembic/versions/0036_art_auth_catalogue_reconciliation.py` around lines 142 - 144, Update the linked-evidence EXISTS construction in the reconciliation migration to generate the predicate with all referenced event columns explicitly qualified using the event alias, matching _art_catalogue_migration_state. Replace the fragile bind-parameter string substitutions with a predicate-building approach that accepts the column prefix and linked bind names directly.
🤖 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
@.agent-loop/initiatives/WS-XINT-001-lifecycle-boundary-reconciliation/AUTH_ART_HANDOFF.md:
- Around line 3-5: The handoff’s later sections still present obsolete
upload-session authorities and artifact.upload_session.expire as active
guidance. Reconcile the material after the introduction by either clearly
marking all such content as historical or updating the custody table, service
matrix, and delivery order to match the post-WS-XINT-002-01 22-action catalogue,
ensuring deleted rows and scheduler membership are not treated as live.
In `@backend/alembic/versions/0036_art_auth_catalogue_reconciliation.py`:
- Around line 49-51: Update the _replace helper’s op.drop_constraint call to use
the actual existing audit_events constraint names: authority_registries,
authority_privacy_bounds, and authorization_action_evidence, while preserving
the new ck_audit_events_* names when adding constraints.
In `@docs/spec_authorization_service.md`:
- Line 369: Correct the WS-ART-001-06B entry so artifact.checker_output.write
maps to artifact.checker_output.write, while
artifact.checker_output.binding.create alone maps to artifact.binding.create,
matching the mappings in the authorization catalogue.
---
Nitpick comments:
In `@backend/alembic/versions/0036_art_auth_catalogue_reconciliation.py`:
- Around line 71-82: Update the permission-removal loop in the migration,
including the corresponding removal logic at the other referenced locations, to
assert that each obsolete token no longer appears in definition after
definition.replace. Raise the same unexpected removed-permission error if the
post-replacement definition still contains the token, covering replacements that
silently no-op for first-list elements.
- Around line 142-144: Update the linked-evidence EXISTS construction in the
reconciliation migration to generate the predicate with all referenced event
columns explicitly qualified using the event alias, matching
_art_catalogue_migration_state. Replace the fragile bind-parameter string
substitutions with a predicate-building approach that accepts the column prefix
and linked bind names directly.
In `@backend/tests/test_alembic.py`:
- Around line 1777-1787: Define one module-level constant for the six obsolete
artifact upload identifiers, then replace the repeated tuple constructions at
the shown test locations with references to that constant. Keep the constant
aligned with the migration’s _REMOVED_PERMISSIONS identifiers so future
catalogue changes update a single source.
- Around line 1981-1982: Update each bare pytest.raises(RuntimeError) assertion
in the migration tests, including the upgrade cases and the downgrade case, to
use match= with the exact expected evidence-guard message for that predicate.
Ensure the patterns distinguish these failures from the “unexpected ...
definition” RuntimeErrors raised inside _rewrite_* while preserving the existing
command.upgrade and downgrade calls.
🪄 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: d440c0f3-23e8-4364-83fa-c9d0022ca108
📒 Files selected for processing (18)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md.agent-loop/initiatives/WS-XINT-001-lifecycle-boundary-reconciliation/AUTH_ART_HANDOFF.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/DECISIONS.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/STATUS.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/chunks/WS-XINT-002-01-catalogue-reconciliation.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-01-internal-review.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-01-pr-trust-bundle.md.agent-loop/merge-intents/WS-XINT-002-01.jsonbackend/alembic/versions/0036_art_auth_catalogue_reconciliation.pybackend/app/modules/authorization/admin_schemas.pybackend/app/modules/authorization/catalogue.pybackend/tests/test_alembic.pybackend/tests/test_authorization.pydocs/operations_authorization_service.mddocs/spec_authorization_service.mddocs/spec_review_lifecycle.md
2 participants
PR Trust Bundle: WS-XINT-002-01
Chunk
WS-XINT-002-01— ART Catalogue Reconciliation.Goal and human-approved intent
Front-load the complete v0.1 ART authority catalogue before more ART runtime
work, while keeping every new action planned and unavailable. This removes the
unused multi-step upload design without compatibility aliases.
What changed and why
bundle preparation, reviewer packet materialization, and review evidence
binding authority.
WS-XINT-002-05A/WS-XINT-002-07activation custody.packet; binding gains review evidence. No identity or grant was added.
0036to keep PostgreSQL authority evidence constraintsexactly aligned with the typed catalogue.
Design and alternatives
The chosen design is a clean catalogue cutover with planned-only replacements.
Retained aliases, a second upload path, premature activation, and feature facts
inside catalogue metadata were rejected.
Scope and product behavior
Changed only catalogue/admin schema, audit-constraint migration parity, tests,
and live AUTH/ART/REV handoffs. No route, evaluator, command, worker behavior,
grant, service identity, submission, review, or artifact lifecycle changed.
Acceptance proof
invalidation, and idempotency-linked evidence; refusal preserves revision,
constraints, and evidence counts.
upgrade and downgrade/re-upgrade.
immutable historical artifacts and migration deletion proof.
Tests and checks
app,tests, andscripts: passed.0036_art_auth_cataloguetests, including independentrefusal predicates and round trip: passed with owned-database cleanup.
git diff --check: passed.ceiling; the complete suite and coverage gates are intentionally delegated to
GitHub Actions on the exact PR head.
Test delta and CI integrity
No test was removed, skipped, or weakened. New tests independently cover each
destructive migration predicate and negative post-head SQL acceptance. No CI,
coverage, lint, or runner configuration changed. Hosted gates remain the 78%
repository and 90% authorization-subsystem coverage floors.
Reviewer results
Senior, QA, security, product/ops, architecture, CI integrity, docs,
reuse/dedup, and test-delta tracks all passed after valid findings were fixed.
See
WS-XINT-002-01-internal-review.md.External review
CodeRabbit and exact-head GitHub checks are required after the PR opens. No
external result is claimed in this pre-PR bundle.
Remaining risks and follow-up
Migration
0036takes an access-exclusive audit lock for an atomic vocabularyrewrite; deploy it as a bounded schema migration.
WS-XINT-002-02is the nextsame-initiative explicit-start gate. Later ART activation still requires exact
hidden feature evidence; this chunk grants no executable authority.
Human review focus and merge ownership
Review the exact six-to-three clean cut, planned-only availability, fixed-service
least privilege, and evidence-preserving migration refusal. Only the user may
approve this specific PR for merge.
Summary by CodeRabbit