feat(auth): add submission policy authority foundation - #286
Merged
abiorh-claw merged 11 commits intoAug 5, 2026
Conversation
📝 WalkthroughWalkthroughThis PR adds the submission-policy authorization foundation. It adds typed bindings, replay custody, provenance storage, migration protections, targeted coverage checks, and review records. The planned submission-policy actions remain inactive. ChangesSubmission policy authority
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuthorizationPreparation
participant AuthorizationKernel
participant ReplayService
participant Database
AuthorizationPreparation->>AuthorizationKernel: validate and bind submission-policy mutation
AuthorizationKernel->>ReplayService: validate replay and custody facts
ReplayService->>Database: reserve or complete replay record
AuthorizationKernel->>Database: stage authorization audit evidence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 8
🧹 Nitpick comments (7)
backend/tests/test_projects.py (3)
9466-9474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining
reserveand_find_namespacebranches.Two branches of
SubmissionPolicyMutationReplayRepositorystay unexercised.First, the claimed path always resolves. When
session.scalarreturns arecord_idandsession.getreturnsNone,reserveraisesProjectRepositoryIntegrityError. Settingsession.get_result = Nonewhilesession.scalar_resultholds a UUID covers it.Second,
_find_namespaceis stubbed in every case here, and the PostgreSQL test at Line 9654 uses the human namespace only. The service branch, which filters onservice_identity,setup_run_id,setup_generation,setup_task_id,correlation_id, andaction_id, never runs.The CI step at
.github/workflows/backend.ymlLines 462-478 enforces 90% per file for this module.♻️ Proposed addition
state, record = await repository.reserve(**facts) # type: ignore[arg-type] assert (state, record) == ("claimed", claimed) + session.get_result = None + with pytest.raises(ProjectRepositoryIntegrityError, match="reservation disappeared"): + await repository.reserve(**facts) # type: ignore[arg-type] + session.get_result = claimed + session.scalar_result = claimed🤖 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 9466 - 9474, Add tests in the existing SubmissionPolicyMutationReplayRepository coverage to exercise the claimed reserve integrity branch by setting session.scalar_result to a UUID and session.get_result to None, then asserting ProjectRepositoryIntegrityError. Also configure _find_namespace inputs with a service identity and setup_run_id, setup_generation, setup_task_id, correlation_id, and action_id, and assert the service namespace lookup path is used.
9637-9651: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the accepted service path and the inactive-transaction branch.
The loop proves that three mutated custody values are rejected. It never asserts that the unmodified
service_factsis accepted. A change that rejects every service reservation would still pass this test.
_require_root_transactionhas three failure conditions: the transaction isNone, the transaction is not active, and the session is nested. Lines 9645-9651 coverNoneand nested. Thenot transaction.is_activebranch stays uncovered.♻️ Proposed additions
+ assert await service.reserve_replay(service_facts) == ("claimed", record) for changed in ( replace(service_facts, setup_run_id=str(uuid4())), replace(service_facts, setup_task_id=uuid4()), replace(service_facts, correlation_id=uuid4()), ): with pytest.raises(ValueError, match="service replay custody is invalid"): await service.reserve_replay(changed) session.nested = True with pytest.raises(RuntimeError, match="one root transaction"): await service.reserve_replay(facts) session.nested = False + + class InactiveTransaction: + is_active = False + + session.sync_session.transaction = InactiveTransaction() + with pytest.raises(RuntimeError, match="one root transaction"): + await service.reserve_replay(facts) session.sync_session.transaction = None🤖 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 9637 - 9651, Add assertions in the service replay reservation test around `service.reserve_replay`: verify the unchanged `service_facts` succeeds before testing mutated custody values, and add coverage for `_require_root_transaction` when the root transaction exists but `is_active` is false, preserving the existing `None` and nested transaction assertions.
9743-9753: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed sleep with a deterministic wait for the blocked insert.
await asyncio.sleep(0.05)assumes the competing task reaches its blocking insert within 50 ms. On a loaded CI runner the task can still be starting whenfirst.commit()runs. The competing insert then succeeds instead of conflicting,reservereturns"claimed", and the assertion at Line 9752 fails. The test becomes flaky.Poll
pg_stat_activityuntil the competing backend waits on a lock, then commit.♻️ Proposed fix
competing = asyncio.create_task(reserve_second()) - await asyncio.sleep(0.05) + async with engine.connect() as observer: + for _ in range(200): + waiting = await observer.scalar( + text( + "select count(*) from pg_stat_activity where " + "wait_event_type='Lock' and state='active' " + "and query ilike '%submission_policy_mutation_idempotency_records%'" + ) + ) + if waiting: + break + await asyncio.sleep(0.05) + else: + raise AssertionError("competing reservation never blocked") await first.commit()🤖 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 9743 - 9753, Update the competing insert setup in the reserve test to remove the fixed asyncio.sleep and wait deterministically until the task started by reserve_second is actually blocked on the database lock. Use the existing async test flow around SubmissionPolicyMutationReplayRepository(second).reserve and poll pg_stat_activity for the competing backend’s lock wait state before calling first.commit(), then keep the current assertions on second_result unchanged.backend/app/modules/projects/submission_policy_mutation_service.py (2)
119-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the return annotation to
reserve_replay.The method returns whatever
SubmissionPolicyMutationReplayRepository.reservereturns, which is a typed tuple. Declare it so callers in 12F2 get the classification literal from type checking rather than from reading the repository.♻️ Proposed change
- async def reserve_replay(self, facts: SubmissionPolicyReplayFacts): + async def reserve_replay( + self, facts: SubmissionPolicyReplayFacts + ) -> tuple[ + Literal["claimed", "mismatch", "pending", "replayed"], + SubmissionPolicyMutationIdempotencyRecord, + ]:Add the supporting imports:
from dataclasses import dataclass +from typing import Literal from uuid import UUID+from app.modules.projects.models import SubmissionPolicyMutationIdempotencyRecord from app.modules.projects.submission_policy_mutation_repository import ( SubmissionPolicyMutationReplayRepository, )🤖 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/app/modules/projects/submission_policy_mutation_service.py` around lines 119 - 122, Add the appropriate typed-tuple return annotation to SubmissionPolicyMutationService.reserve_replay, matching the return type of SubmissionPolicyMutationReplayRepository.reserve, and add any required typing imports. Preserve the existing delegation and transaction behavior.
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind submission service identity comparisons to
ServiceIdentity.PROJECT_SETUP.
_replay_values()comparesfacts.service_identityto"workstream.project.setup"for setup-service replay. UseServiceIdentity.PROJECT_SETUPfromapp.modules.actors.service_identitiesat this check so the service identity value cannot drift from the enum.🤖 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/app/modules/projects/submission_policy_mutation_service.py` around lines 79 - 88, Update the service identity comparison in _replay_values() for setup_service replays to use ServiceIdentity.PROJECT_SETUP instead of the hard-coded "workstream.project.setup" string. Import and reuse ServiceIdentity from app.modules.actors.service_identities, leaving the other custody validations unchanged.backend/app/modules/projects/submission_policy_mutation_repository.py (1)
195-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the nullable predicates into a helper.
Five criteria use an inline conditional expression, for example:
Model.service_identity .is_(None) if service_identity is None else Model.service_identity == service_identityThe semantics are correct. Python binds the method call and the
==comparison tighter than the conditional expression, so each criterion produces one clause. The formatting hides that precedence. A future edit that wraps part of the expression in parentheses would change the predicate without an obvious diff signal.Replace the five occurrences with one helper so the intent is explicit.
♻️ Proposed helper
def _matches(column, value): """Return an exact null-safe equality predicate for one nullable column.""" return column.is_(None) if value is None else column == value.where( SubmissionPolicyMutationIdempotencyRecord.operation_id == operation_id, SubmissionPolicyMutationIdempotencyRecord.actor_profile_id == actor_profile_id, SubmissionPolicyMutationIdempotencyRecord.identity_link_id == identity_link_id, - SubmissionPolicyMutationIdempotencyRecord.service_identity - .is_(None) if service_identity is None else - SubmissionPolicyMutationIdempotencyRecord.service_identity == service_identity, + _matches( + SubmissionPolicyMutationIdempotencyRecord.service_identity, service_identity + ), SubmissionPolicyMutationIdempotencyRecord.action_id == action_id, - SubmissionPolicyMutationIdempotencyRecord.idempotency_key - .is_(None) if idempotency_key is None else - SubmissionPolicyMutationIdempotencyRecord.idempotency_key == idempotency_key, + _matches( + SubmissionPolicyMutationIdempotencyRecord.idempotency_key, idempotency_key + ), SubmissionPolicyMutationIdempotencyRecord.request_digest == request_digest, SubmissionPolicyMutationIdempotencyRecord.resource_context_digest == resource_context_digest, - SubmissionPolicyMutationIdempotencyRecord.setup_run_id - .is_(None) if setup_run_id is None else - SubmissionPolicyMutationIdempotencyRecord.setup_run_id == setup_run_id, + _matches(SubmissionPolicyMutationIdempotencyRecord.setup_run_id, setup_run_id), SubmissionPolicyMutationIdempotencyRecord.setup_generation == setup_generation, - SubmissionPolicyMutationIdempotencyRecord.setup_task_id - .is_(None) if setup_task_id is None else - SubmissionPolicyMutationIdempotencyRecord.setup_task_id == setup_task_id, + _matches(SubmissionPolicyMutationIdempotencyRecord.setup_task_id, setup_task_id), - SubmissionPolicyMutationIdempotencyRecord.correlation_id - .is_(None) if correlation_id is None else - SubmissionPolicyMutationIdempotencyRecord.correlation_id == correlation_id, + _matches(SubmissionPolicyMutationIdempotencyRecord.correlation_id, correlation_id), SubmissionPolicyMutationIdempotencyRecord.status == "pending", )Apply the same helper to
_find_namespace, whereidempotency_key,setup_task_id, andcorrelation_idalso use plain==against values that the type signature allows to beNone.🤖 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/app/modules/projects/submission_policy_mutation_repository.py` around lines 195 - 220, Introduce a shared _matches helper that returns a null-safe equality predicate, then replace the five inline nullable conditional expressions in the shown query with calls to it. Apply the same helper in _find_namespace for idempotency_key, setup_task_id, and correlation_id, while leaving non-nullable predicates unchanged.backend/tests/test_authorization.py (1)
5976-6027: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated derive-resource setup into a helper.
Lines 5978-6017 and lines 6169-6208 build the identical custody context, resource context, caller input, and project scope. Extract one module-level helper that returns the
(caller_input, scope)pair forPROJECT_SUBMISSION_ARTIFACT_POLICY_DERIVE, then call it from both tests. This removes about 40 duplicated lines and keeps the two matrix tests in sync when the resource context gains fields in 12F2.♻️ Proposed helper
def _submission_policy_derive_prepare_inputs() -> tuple[ PreparedAuthorizationInput, PreparedAuthorityScope ]: """Build the exact derive caller input and project scope shared by matrix tests.""" project_id, guide_id, snapshot_id, policy_id, operation_id = (uuid4() for _ in range(5)) custody = ProjectSetupServiceCustodyContext( setup_run_id=uuid4(), scope_project_id=project_id, guide_id=guide_id, source_snapshot_id=snapshot_id, setup_generation=1, expected_step="submission_artifact_policy", task_id=uuid4(), correlation_id=uuid4(), stale_output_digest=DIGEST, ) resource = ProjectSubmissionArtifactPolicyMutationResourceContext( resource_type="project_submission_artifact_policy_mutation", resource_id=policy_id, operation_id=operation_id, request_digest=DIGEST, scope_project_id=project_id, guide_id=guide_id, guide_version="1", source_snapshot_id=snapshot_id, source_snapshot_hash=DIGEST, target_kind="derive", execution_kind="setup_service", policy_id=policy_id, policy_version="1", policy_generation=1, setup_generation=1, stale_output_digest=DIGEST, setup_service_custody=custody, ) return ( PreparedAuthorizationInput( idempotency_key=uuid4(), request_value=resource.model_dump(mode="json") ), PreparedAuthorityScope(kind=PreparedAuthorityScopeKind.PROJECT, project_id=project_id), )Then in both tests:
caller_input = PreparedAuthorizationInput(idempotency_key=uuid4(), request_value={}) scope = PreparedAuthorityScope(kind=PreparedAuthorityScopeKind.SYSTEM) if action_id is ActionId.PROJECT_SUBMISSION_ARTIFACT_POLICY_DERIVE: - project_id, guide_id, snapshot_id, policy_id, operation_id = ( - uuid4() for _ in range(5) - ) - ... + caller_input, scope = _submission_policy_derive_prepare_inputs()🤖 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_authorization.py` around lines 5976 - 6027, Extract the duplicated derive setup into a module-level helper named _submission_policy_derive_prepare_inputs that returns the PreparedAuthorizationInput and PreparedAuthorityScope pair, preserving all existing custody and resource fields. Replace the inline PROJECT_SUBMISSION_ARTIFACT_POLICY_DERIVE setup in both matrix tests with calls to this helper, while leaving other action setup unchanged.
🤖 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-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F1-pr-trust-bundle.md:
- Around line 59-60: Update the checklist entries for “Historical null
provenance survives upgrade and empty roundtrip” and “Pending replay and
admitted audit evidence block downgrade” to remain pending until hosted Backend
PostgreSQL/Alembic migration and custody results for the exact pushed head are
available; link those results before marking them complete.
In `@backend/alembic/versions/0057_submission_policy_authority.py`:
- Around line 119-121: The constraint operations around
ck_submission_policy_creation_authority_shape use only the suffix instead of the
generated physical constraint name. Update the migration’s create and drop
operations to consistently use the naming convention’s full name, or drop each
constraint by its definition with ALTER TABLE; ensure every affected constraint
operation targets the actual physical name.
- Around line 418-421: The reservation lookup in the submission policy authority
migration can silently accept multiple committed rows because `select ... into
reservation` in the trigger path only picks the first match. Update the
`submission_policy_mutation_idempotency_records` handling around the committed
reservation check to fail closed by either adding a partial unique index on
`(committed_policy_id, action_id)` where `status='committed'` or switching the
`select ... into reservation` in the relevant trigger/function flow to `into
strict` so duplicate committed reservations cannot be bound.
- Around line 641-665: Update downgrade() to acquire SHARE ROW EXCLUSIVE locks
on submission_policy_mutation_idempotency_records, submission_artifact_policies,
effective_project_submission_artifact_policies, pre_submit_checker_policies, and
audit_events before reading replay_count, provenance_count, or audit_count.
Preserve the existing evidence guard, then remove the later redundant
audit_events lock after the destructive operations begin.
In `@backend/app/modules/authorization/prepared.py`:
- Around line 402-405: Add ActionId._SUBMISSION_POLICY_MUTATIONS and
ProjectSubmissionArtifactPolicyMutationResourceContext to the supported
prepared-denial check in AuthorizationService._complete_prepared_denial() within
kernel.py, so unsupported submit-policy mutations that fail binding validation
are recorded as prepared denials instead of raising TypeError.
- Around line 509-550: Update the authorization preparation validation block
around ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate to
import and catch pydantic.ValidationError alongside the existing exceptions,
preserving the PreparedAuthorizationHandleInvalid conversion for malformed
request values. Apply the same ValidationError handling to every Pydantic
validation path in this block so failures remain fail-closed.
In `@backend/app/modules/projects/models.py`:
- Around line 1282-1312: Bind each scope-project column to the row’s project_id
instead of only checking non-null: update creation_scope_project_id in
ck_submission_policy_creation_authority_shape, approval_scope_project_id in
ck_submission_policy_approval_authority_shape, and creation_scope_project_id in
ck_effective_submission_policy_authority_shape and
ck_pre_submit_policy_authority_shape. Apply the corresponding predicate changes
in backend/alembic/versions/0057_submission_policy_authority.py lines 119-151
and the shared output_shape at lines 152-163; no direct changes are needed at
other sites.
In `@backend/tests/test_projects.py`:
- Line 9371: Replace the `_find_namespace` assignment on `repository` with a
callable stub for `GuideSufficiencyMutationReplayRepository.find`, ensuring
`reserve` receives the intended replay result and exercises its reserve path
against the fake Session.
---
Nitpick comments:
In `@backend/app/modules/projects/submission_policy_mutation_repository.py`:
- Around line 195-220: Introduce a shared _matches helper that returns a
null-safe equality predicate, then replace the five inline nullable conditional
expressions in the shown query with calls to it. Apply the same helper in
_find_namespace for idempotency_key, setup_task_id, and correlation_id, while
leaving non-nullable predicates unchanged.
In `@backend/app/modules/projects/submission_policy_mutation_service.py`:
- Around line 119-122: Add the appropriate typed-tuple return annotation to
SubmissionPolicyMutationService.reserve_replay, matching the return type of
SubmissionPolicyMutationReplayRepository.reserve, and add any required typing
imports. Preserve the existing delegation and transaction behavior.
- Around line 79-88: Update the service identity comparison in _replay_values()
for setup_service replays to use ServiceIdentity.PROJECT_SETUP instead of the
hard-coded "workstream.project.setup" string. Import and reuse ServiceIdentity
from app.modules.actors.service_identities, leaving the other custody
validations unchanged.
In `@backend/tests/test_authorization.py`:
- Around line 5976-6027: Extract the duplicated derive setup into a module-level
helper named _submission_policy_derive_prepare_inputs that returns the
PreparedAuthorizationInput and PreparedAuthorityScope pair, preserving all
existing custody and resource fields. Replace the inline
PROJECT_SUBMISSION_ARTIFACT_POLICY_DERIVE setup in both matrix tests with calls
to this helper, while leaving other action setup unchanged.
In `@backend/tests/test_projects.py`:
- Around line 9466-9474: Add tests in the existing
SubmissionPolicyMutationReplayRepository coverage to exercise the claimed
reserve integrity branch by setting session.scalar_result to a UUID and
session.get_result to None, then asserting ProjectRepositoryIntegrityError. Also
configure _find_namespace inputs with a service identity and setup_run_id,
setup_generation, setup_task_id, correlation_id, and action_id, and assert the
service namespace lookup path is used.
- Around line 9637-9651: Add assertions in the service replay reservation test
around `service.reserve_replay`: verify the unchanged `service_facts` succeeds
before testing mutated custody values, and add coverage for
`_require_root_transaction` when the root transaction exists but `is_active` is
false, preserving the existing `None` and nested transaction assertions.
- Around line 9743-9753: Update the competing insert setup in the reserve test
to remove the fixed asyncio.sleep and wait deterministically until the task
started by reserve_second is actually blocked on the database lock. Use the
existing async test flow around
SubmissionPolicyMutationReplayRepository(second).reserve and poll
pg_stat_activity for the competing backend’s lock wait state before calling
first.commit(), then keep the current assertions on second_result 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28882d53-1960-42ce-935b-d1f97b1a5663
📒 Files selected for processing (18)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12F1-submission-policy-authority-foundation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F1-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F1-pr-trust-bundle.md.github/workflows/backend.ymlbackend/alembic/versions/0057_submission_policy_authority.pybackend/app/modules/audit/schemas.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/models.pybackend/app/modules/projects/submission_policy_mutation_repository.pybackend/app/modules/projects/submission_policy_mutation_service.pybackend/tests/test_alembic.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/operations_authorization_service.mddocs/spec_authorization_service.md
Comment on lines
+509
to
+550
| if action_id in { | ||
| ActionId.PROJECT_SUBMISSION_ARTIFACT_POLICY_CREATE, | ||
| ActionId.PROJECT_SUBMISSION_ARTIFACT_POLICY_DERIVE, | ||
| ActionId.PROJECT_SUBMISSION_ARTIFACT_POLICY_UPDATE, | ||
| ActionId.PROJECT_SUBMISSION_ARTIFACT_POLICY_APPROVE, | ||
| }: | ||
| try: | ||
| value = dict(caller_input.request_value) | ||
| for field in ( | ||
| "resource_id", | ||
| "operation_id", | ||
| "scope_project_id", | ||
| "guide_id", | ||
| "source_snapshot_id", | ||
| "policy_id", | ||
| ): | ||
| value[field] = UUID(str(value[field])) | ||
| raw_custody = value.get("setup_service_custody") | ||
| if raw_custody is not None: | ||
| custody = dict(raw_custody) | ||
| for field in ( | ||
| "setup_run_id", | ||
| "scope_project_id", | ||
| "guide_id", | ||
| "source_snapshot_id", | ||
| "task_id", | ||
| "correlation_id", | ||
| ): | ||
| custody[field] = UUID(str(custody[field])) | ||
| value["setup_service_custody"] = custody | ||
| resource = ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate( | ||
| value | ||
| ) | ||
| except (KeyError, TypeError, ValueError) as exc: | ||
| raise PreparedAuthorizationHandleInvalid( | ||
| "invalid prepared authorization handle" | ||
| ) from exc | ||
| expected_target = PROJECT_SUBMISSION_POLICY_TARGET_KIND_BY_ACTION[action_id] | ||
| if resource.target_kind != expected_target: | ||
| raise PreparedAuthorizationHandleInvalid("invalid prepared authorization handle") | ||
| submission_policy_context = resource.model_dump(mode="json") | ||
| submission_policy_resource_digest = authorization_resource_digest(resource) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the pinned Pydantic version and check whether tests assert the mapped exception.
set -euo pipefail
# Find the declared pydantic constraint.
fd -t f 'pyproject.toml|requirements.*\.txt|constraints.*\.txt' backend . --exec rg -n 'pydantic' {} \;
# Check whether any test asserts PreparedAuthorizationHandleInvalid for malformed submission-policy input.
rg -n -C4 'PreparedAuthorizationHandleInvalid' backend/tests --glob '*.py' | rg -n -C4 -i 'submission'Repository: Flow-Research/workstream
Length of output: 234
🌐 Web query:
Does pydantic v2 ValidationError inherit from ValueError?
💡 Result:
No, Pydantic v2's ValidationError does not inherit from ValueError [1]. It is a distinct exception class [1]. In Pydantic v2, ValidationError is implemented in Rust via pydantic-core, which makes it a separate type that does not follow the standard Python exception hierarchy of inheriting from Exception or ValueError in the way traditional Python classes do [1]. It is important to note that while ValidationError does not inherit from ValueError, you are encouraged to raise ValueError (or AssertionError) within your custom validators; Pydantic will catch these exceptions and automatically wrap them into a Pydantic ValidationError [2][3][4].
Citations:
- 1: [Core] Pydantic ValidationErrors cannot be handled by Ray ray-project/ray#43401
- 2: https://pydantic.dev/docs/validation/latest/errors/errors/
- 3: https://pydantic.dev/docs/validation/2.10/errors/errors/
- 4: https://pydantic.dev/docs/validation/2.10/concepts/validators/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(pyproject\.toml|uv\.lock|poetry\.lock|requirements.*\.txt|constraints.*\.txt)$|backend/app/modules/authorization/prepared\.py|Backend|pydantic'
echo "== pyproject snippets =="
fd -t f 'pyproject.toml|uv.lock|poetry.lock|requirements.*\.txt|constraints.*\.txt' . --exec sh -c '
echo "--- {}"
rg -n "^(include\s*=|packages\s*=|pyproject\.toml|pydantic|pydantic-settings|pydantic-core|# package metadata)" "{}" || true
' \;
echo "== local pydantic availability =="
python3 - <<'PY'
try:
import pydantic
print("pydantic", pydantic.__version__)
print("ValidationError bases", pydantic.ValidationError.__bases__)
print("isinstance check", isinstance(pydantic.ValidationError("x"), (KeyError, TypeError, ValueError)))
except Exception as e:
print(type(e).__name__, e)
PY
echo "== relevant code =="
sed -n '480,570p' backend/app/modules/authorization/prepared.py
echo "== pydantic imports in prepared.py =="
rg -n "import pydantic|from pydantic|ValidationError" backend/app/modules/authorization/prepared.pyRepository: Flow-Research/workstream
Length of output: 4983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend/pyproject.toml =="
cat -n backend/pyproject.toml
echo "== backend/uv.lock pydantic/core occurrences =="
rg -n "pydantic|pydantic-core|Name = \"pydantic\"|Name = \"pydantic-core\"" backend/uv.lock
echo "== mutation requirements =="
cat -n scripts/mutation-requirements.txt
echo "== prepared.py imports and exception definition =="
sed -n '1,80p' backend/app/modules/authorization/prepared.pyRepository: Flow-Research/workstream
Length of output: 50380
Catch pydantic.ValidationError explicitly.
model_validate raises pydantic.ValidationError, and backend/uv.lock pins Pydantic 2.13.4. This exception is not covered by the surrounding except (KeyError, TypeError, ValueError), so malformed request_value escapes instead of raising PreparedAuthorizationHandleInvalid. Import pydantic.ValidationError and add it to this fail-closed handler; apply the same catch to the Pydantic validation paths in this block.
🤖 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/app/modules/authorization/prepared.py` around lines 509 - 550, Update
the authorization preparation validation block around
ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate to import
and catch pydantic.ValidationError alongside the existing exceptions, preserving
the PreparedAuthorizationHandleInvalid conversion for malformed request values.
Apply the same ValidationError handling to every Pydantic validation path in
this block so failures remain fail-closed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/test_authorization.py (1)
6179-6186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest every canonical binding field.
This test changes only
compilation.catalogue_version. A matcher that omitsoperation_id, lineage, output digests, or another compilation field can still pass.Parameterize mutations for every bound top-level and nested field. Assert that each changed resource fails
_submission_policy_binding_matches.As per coding guidelines, “New or materially changed backend subsystems must maintain at least 90% test coverage.”
🤖 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_authorization.py` around lines 6179 - 6186, Expand the test around _submission_policy_binding_matches by parameterizing resource mutations for every canonical binding field, including operation_id, lineage, output digests, and all relevant compilation fields. For each mutation, assert the changed resource does not match the binding, while preserving the existing catalogue_version case. Ensure the added coverage meets the required 90% threshold.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 `@backend/app/modules/projects/models.py`:
- Line 1295: Update the provenance CHECK constraints in
backend/app/modules/projects/models.py at lines 1295-1295, 1315-1315, 1425-1425,
and 1522-1522: require each corresponding scope project ID to be non-null before
evaluating its equality with project_id, using creation_scope_project_id or
approval_scope_project_id as applicable. Add a database-level test covering
otherwise complete provenance with a NULL scope project and assert that
insertion is rejected.
---
Nitpick comments:
In `@backend/tests/test_authorization.py`:
- Around line 6179-6186: Expand the test around
_submission_policy_binding_matches by parameterizing resource mutations for
every canonical binding field, including operation_id, lineage, output digests,
and all relevant compilation fields. For each mutation, assert the changed
resource does not match the binding, while preserving the existing
catalogue_version case. Ensure the added coverage meets the required 90%
threshold.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68cda307-d23c-449c-bda9-9314c9d0029c
📒 Files selected for processing (12)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12F1-submission-policy-authority-foundation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F1-pr-trust-bundle.mdbackend/alembic/versions/0057_submission_policy_authority.pybackend/app/modules/authorization/kernel.pybackend/app/modules/projects/models.pybackend/app/modules/projects/submission_policy_mutation_repository.pybackend/app/modules/projects/submission_policy_mutation_service.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_authorization.pybackend/tests/test_projects.pybackend/tests/test_review_lease_persistence.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F1-pr-trust-bundle.md
- backend/app/modules/projects/submission_policy_mutation_repository.py
- backend/app/modules/projects/submission_policy_mutation_service.py
- backend/tests/test_projects.py
- backend/alembic/versions/0057_submission_policy_authority.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12F1-submission-policy-authority-foundation.md
- backend/app/modules/authorization/kernel.py
abiorh-claw
self-requested a review
August 5, 2026 15:31
abiorh-claw
approved these changes
Aug 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/test_alembic.py (1)
536-555: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
matchpattern to thispytest.raises(IntegrityError).The block asserts only the exception type. Any unrelated integrity failure in the insert (for example a foreign-key violation on
project_id) satisfies the assertion. Add the expected constraint fragment so the test proves the intended attribution rule.♻️ Proposed change
- with pytest.raises(IntegrityError): + with pytest.raises( + IntegrityError, + match="submission_policy_mutation_idempotency_records", + ):🤖 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 536 - 555, Update the pytest.raises(IntegrityError) assertion in the nested transaction test to include a match pattern for the intended idempotency attribution constraint violation. Keep the existing insert and exception type check unchanged, and use the constraint fragment that identifies the duplicate attribution rule rather than accepting unrelated integrity errors such as foreign-key failures.
🤖 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/alembic/versions/0057_submission_policy_authority.py`:
- Around line 617-628: Update submission_policy_mutation_provenance_custody
around the creation_action_id-null branch to enforce immutable linkage for
completed approval outputs. Ensure updates to submission_artifact_policies,
effective_project_submission_artifact_policies, and pre_submit_checker_policies
cannot replace or alter provenance after completion, rather than being routed
through production custody; preserve the existing partial-provenance validation
for non-approval rows.
---
Nitpick comments:
In `@backend/tests/test_alembic.py`:
- Around line 536-555: Update the pytest.raises(IntegrityError) assertion in the
nested transaction test to include a match pattern for the intended idempotency
attribution constraint violation. Keep the existing insert and exception type
check unchanged, and use the constraint fragment that identifies the duplicate
attribution rule rather than accepting unrelated integrity errors such as
foreign-key failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 602fb4c9-f612-43a7-bf9d-b057323c8074
📒 Files selected for processing (5)
backend/alembic/versions/0057_submission_policy_authority.pybackend/app/modules/projects/models.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_authorization.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/conftest.py
- backend/app/modules/projects/models.py
- backend/tests/test_authorization.py
Comment on lines
+617
to
+628
| if new.creation_action_id is null then | ||
| if new.created_by_actor_profile_id is not null | ||
| or new.created_via_identity_link_id is not null | ||
| or new.created_by_admin_role_grant_id is not null | ||
| or new.creation_scope_type is not null | ||
| or new.creation_scope_project_id is not null | ||
| or new.creation_decision_event_id is not null then | ||
| raise exception 'partial effective-policy provenance' | ||
| using errcode='23514'; | ||
| end if; | ||
| return null; | ||
| end if; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the affected model definitions before inspecting constraints.
ast-grep outline backend/app/modules/projects/models.py --items all
# Find constraints, triggers, and tests that protect output linkage and hashes.
rg -n -C5 \
'submission_artifact_policy_id|effective_policy_hash|compiled_bundle_hash|\
committed_effective_policy_id|committed_pre_submit_policy_id|\
protect_submission_policy_output_provenance|submission_policy.*custody' \
backend/alembic/versions backend/app/modules/projects/models.py backend/testsRepository: Flow-Research/workstream
Length of output: 10205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the migration around the cited trigger lines.
sed -n '500,660p' backend/alembic/versions/0057_submission_policy_authority.py | cat -n
# Search without literal newline regex syntax.
rg -n -C5 'submission_artifact_policy_id|effective_policy_hash|compiled_bundle_hash|committed_effective_policy_id|committed_pre_submit_policy_id|protect_submission_policy_output_provenance|submission_policy.*custody|submit_state|committed_submission_artifact' backend/alembic/versions backend/app/modules/projects/models.py backend/tests || true
# Find references to the relevant trigger/function names or completed output fields in migrations/test model definitions.
rg -n -C3 'protect_submission_policy|submission_policy_authority|output_submission_artifact|submission_artifact_policy_id|pre_submit_bundle_hash|effective_policy_hash|compiled_bundle_hash' backend/alembic backend/app backend/tests || trueRepository: Flow-Research/workstream
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the migration around the cited trigger conditions and the replay binding.
sed -n '500,640p' backend/alembic/versions/0057_submission_policy_authority.py | cat -n
# Search migration/test files for constraints, triggers, and tests around submitted setup-run fields.
rg -n -C5 'prevent_submission|output_submission_artifact|output_post_submit_checker|submission_artifact_policy_id|pre_submit_checker_bundle_hash|effective_policy_hash|compiled_bundle_hash|protect|immutable|UPDATE|ON UPDATE|trigger|create_trigger|execute' \
backend/alembic/versions/0057_submission_policy_authority.py backend/tests/backend_alembic_schema_contract tests backend/tests || trueRepository: Flow-Research/workstream
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the migration around the cited replay branch without the broad sed output cap.
python3 - <<'PY'
from pathlib import Path
p = Path('backend/alembic/versions/0057_submission_policy_authority.py')
lines = p.read_text().splitlines()
start, end = 480, 650
for i in range(start, min(end, len(lines)) + 1):
print(f"{i}: {lines[i-1]}")
PY
# Search exact relevant triggers/functions/tests and print only those files.
python3 - <<'PY'
from pathlib import Path
import re
terms = [
'output_submission_artifact_policy_id',
'output_post_submit_checker_policy_id',
'pre_submit_checker_bundle_hash',
'compiled_bundle_hash',
'effective_policy_hash',
'post_submit_checker_policy_hash',
'committed_effective_policy_id',
'prevent_submission',
'submission_policy_custody',
'protect_submission_policy',
'immutable',
'UPDATE',
]
root = Path('backend')
for pat in ['**/*.py']:
matches = []
for p in root.glob(pat):
try:
text = p.read_text()
except UnicodeDecodeError:
continue
if any(t.lower() in text.lower() for t in terms):
lines = text.splitlines()
hits = []
for j,l in enumerate(lines, 1):
if any(t.lower() in l.lower() for t in terms):
hits.append((j,l))
if hits:
matches.append((str(p), hits[:40]))
print('\n'.join(f"{m[0]}" for m in matches))
print("---", sum(len(m[1]) for m in matches), "hits")
PYRepository: Flow-Research/workstream
Length of output: 19899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the effective-policy replay branch continuation and the trigger creation.
python3 - <<'PY'
from pathlib import Path
p = Path('backend/alembic/versions/0057_submission_policy_authority.py')
lines = p.read_text().splitlines()
start, end = 615, 690
for i in range(start, min(end, len(lines)) + 1):
print(f"{i}: {lines[i-1]}")
PY
# Search for project_setup_runs completion-output constraints/triggers in the migration.
python3 - <<'PY'
from pathlib import Path
p = Path('backend/alembic/versions/0057_submission_policy_authority.py')
text = p.read_text()
terms = ["project_setup_runs", "output_submission_artifact_policy_id",
"output_post_submit_checker_policy_id", "effective_policy_hash",
"pre_submit_checker_bundle_hash", "compiled_bundle_hash",
"post_submit_checker_policy_hash", "update", "update on"]
for i, line in enumerate(text.splitlines(), 1):
if any(t.lower() in line.lower() for t in terms):
print(f"{i}: {line}")
PY
# Search schema tests for project_setup_runs contract fields and triggers.
python3 - <<'PY'
from pathlib import Path
p = Path('backend/tests/test_alembic.py')
text = p.read_text().splitlines()
for i in range(50, 220):
if 0 < i <= len(text):
print(f"{i}: {text[i-1]}")
PYRepository: Flow-Research/workstream
Length of output: 13631
Enforce immutable output linkage for completed approval outputs.
The current submission_policy_mutation_provenance_custody only validates approval outputs on the initial approval_action_id; subsequent updates to submission_artifact_policies, effective_project_submission_artifact_policies, or pre_submit_checker_policies use production custody instead. Add a completed-output immutality check in this branch or a separate trigger/constraint for completed outputs.
🤖 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/0057_submission_policy_authority.py` around lines
617 - 628, Update submission_policy_mutation_provenance_custody around the
creation_action_id-null branch to enforce immutable linkage for completed
approval outputs. Ensure updates to submission_artifact_policies,
effective_project_submission_artifact_policies, and pre_submit_checker_policies
cannot replace or alter provenance after completion, rather than being routed
through production custody; preserve the existing partial-provenance validation
for non-approval rows.
abiorh-claw
deleted the
codex/ws-auth-001-12f1-submission-policy-foundation
branch
August 6, 2026 03:37
9 tasks
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.
Workstream PR Trust Bundle
Chunk
WS-AUTH-001-12F1- Submission Policy Authority FoundationGoal
Install exact PREP, replay, provenance, audit, and transaction custody needed by
12F2-12F4 without activating any submission-policy mutation action or changing
route, worker, or product behavior.
What changed
operation, request, policy, setup, compiler, catalogue, and output facts.
completion, deferred product/evidence custody, and guarded downgrade.
exact submission-policy mutation resource.
and per-file coverage proof.
Scope and behavior
approval plus effective/pre-submit output creation.
Evidence
PostgreSQL and Alembic selectors require
WORKSTREAM_TEST_DATABASE_URLand aredelegated to the hosted Backend matrix rather than the user's slow local host.
Acceptance proof
deferred database custody.
Internal review
Architecture, security, QA, senior engineering, test delta, CI integrity,
documentation, and reuse/dedup completed with no blocking findings. Product/ops
review passed after approval/output provenance immutability was added.
Remaining risk and follow-up
coverage must pass on the exact pushed head.
this foundation activates no product writer.
Human review focus
Human merge ownership
Summary by CodeRabbit
New Features
Bug Fixes
Documentation