AUTH-12E: activate guide sufficiency mutations - #263
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change activates guide-sufficiency authorization mutations. It adds prepared authorization, replay custody, provenance persistence, deterministic setup execution, verified material handling, migration safeguards, API wiring, tests, and documentation. ChangesGuide sufficiency authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/modules/projects/setup_queue.py (1)
82-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not dispatch when the setup run row is absent.
Lines 84-86 persist
expected_task_idonly whensetup_runis found. When the row is missing, the function still enqueues the Celery task at lines 88-95. The worker fences deliveries by comparing the delivered task id againstProjectSetupRun.celery_task_id, so that task can never execute. The result is orphaned broker work and no recorded outcome.The caller
backend/app/modules/projects/guide_mutation_router.py:164-171passesoutcome.setup_run_idfrom a committed mutation, so a missing row means a broken invariant upstream. Fail fast instead of dispatching.A second point: when the returned task id differs from
expected_task_idat line 96, the task is already accepted by the broker. The except block then recordsstatus="enqueue_failed"and logs "enqueue failed", which misstates what happened. A distinct error code makes the condition diagnosable.🛡️ Proposed fix
expected_task_id = pre_submit_setup_task_id(setup_run_id, setup_generation) setup_run = await repository.get_project_setup_run(setup_run_id) - if setup_run is not None: - setup_run.celery_task_id = expected_task_id - await session.commit() + if setup_run is None: + logger.warning( + "project setup run missing before dispatch", + extra={"project_id": project_id, "setup_run_id": setup_run_id}, + ) + return None + setup_run.celery_task_id = expected_task_id + await session.commit() try: task_id = await asyncio.to_thread( enqueue_pre_submit_setup_pipeline, project_id=project_id, guide_id=guide_id, source_snapshot_id=source_snapshot_id, setup_run_id=setup_run_id, setup_generation=setup_generation, ) if task_id != expected_task_id: raise ProjectSetupQueueError( - "project setup queue returned the wrong task identity" + "project setup queue returned the wrong task identity; " + "the accepted task cannot execute" )🤖 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/setup_queue.py` around lines 82 - 99, In the setup queue flow around pre_submit_setup_task_id and get_project_setup_run, fail fast with ProjectSetupQueueError when setup_run is absent, before calling enqueue_pre_submit_setup_pipeline. Also handle a returned task ID mismatch as a distinct post-acceptance identity error rather than recording status="enqueue_failed" or logging it as an enqueue failure; preserve normal enqueue failure handling for dispatch exceptions.
🧹 Nitpick comments (12)
backend/alembic/versions/0050_guide_sufficiency_authority.py (2)
252-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the skip condition into one set.
The loop tests
created_by_service_identityseparately from the four scope/action names. One membership test states the same rule.op.drop_columnalso drops the column's foreign key in PostgreSQL, so the explicitdrop_constraintcalls are redundant, but keeping them documents intent.♻️ Proposed simplification
+ _COLUMNS_WITHOUT_FOREIGN_KEYS = { + "created_by_service_identity", + "creation_scope_type", + "creation_action_id", + "warning_acknowledgement_scope_type", + "warning_acknowledgement_action_id", + } for name, _ in reversed((*_CREATION_COLUMNS, *_ACK_COLUMNS)): - if name != "created_by_service_identity" and name not in { - "creation_scope_type", - "creation_action_id", - "warning_acknowledgement_scope_type", - "warning_acknowledgement_action_id", - }: + if name not in _COLUMNS_WITHOUT_FOREIGN_KEYS: op.drop_constraint(foreign_keys[name], "guide_sufficiency_reports", type_="foreignkey") op.drop_column("guide_sufficiency_reports", name)🤖 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/0050_guide_sufficiency_authority.py` around lines 252 - 260, Update the cleanup loop over _CREATION_COLUMNS and _ACK_COLUMNS to use one exclusion set containing created_by_service_identity and the four scope/action names, while preserving the existing foreign-key constraint and column removal behavior.
176-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider blocking
TRUNCATEon the replay table.The row-level trigger covers
UPDATEandDELETEonly. PostgreSQL does not fire row-level triggers forTRUNCATE, so atruncate guide_sufficiency_mutation_idempotency_recordserases all replay evidence without raising. A statement-level trigger closes the gap.♻️ Proposed statement-level guard
op.execute( """ create trigger trg_sufficiency_replay_immutable before update or delete on guide_sufficiency_mutation_idempotency_records for each row execute function reject_sufficiency_replay_mutation() """ ) + op.execute( + """ + create function reject_sufficiency_replay_truncate() returns trigger + language plpgsql as $$ + begin + raise exception 'guide sufficiency replay rows are append-only'; + end $$ + """ + ) + op.execute( + """ + create trigger trg_sufficiency_replay_no_truncate + before truncate on guide_sufficiency_mutation_idempotency_records + for each statement execute function reject_sufficiency_replay_truncate() + """ + )Note:
backend/tests/test_alembic.pyclears evidence withtruncate guide_sufficiency_mutation_idempotency_records. If you add this guard, change that cleanup todelete from.🤖 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/0050_guide_sufficiency_authority.py` around lines 176 - 209, Extend the replay-table protection around reject_sufficiency_replay_mutation by adding a statement-level TRUNCATE trigger that raises the same append-only exception, preventing truncation from removing evidence. Update the test cleanup in backend/tests/test_alembic.py to use DELETE instead of TRUNCATE for guide_sufficiency_mutation_idempotency_records.backend/app/modules/authorization/prepared.py (1)
519-536: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the validated model instead of discarding it.
The block constructs
ProjectGuideSufficiencyMutationResourceContextonly to trigger validation, then discards the instance and carries the rawsufficiencydict into the binding. The two representations hold the same coerced values today, so behavior is correct. Assigning the instance and reading the binding fields from it removes the duplicate mapping and prevents drift if a field is renamed.validated = ProjectGuideSufficiencyMutationResourceContext( resource_type="project_guide_sufficiency_mutation", resource_id=sufficiency["report_id"] or sufficiency["snapshot_id"], ... ) sufficiency["validated"] = validatedThen set
sufficiency_project_id=validated.scope_project_id, and so on, at lines 604-621.🤖 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 519 - 536, Store the constructed ProjectGuideSufficiencyMutationResourceContext instance in a local validated variable instead of discarding it, then update the sufficiency binding fields to read from validated (including scope_project_id and the other mapped attributes) rather than the raw sufficiency dictionary. Keep the existing constructor inputs and validation behavior unchanged.backend/app/modules/authorization/kernel.py (1)
653-662: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated denial resource context for sufficiency mutations.
Every other prepare-time denial family uses a narrow
*PrepareDenialResourceContextmodel, for exampleProjectGuideMutationPrepareDenialResourceContextandProjectPolicyMutationPrepareDenialResourceContext. Those models carry only the requested selectors. This change instead accepts the full canonicalProjectGuideSufficiencyMutationResourceContextfor denial evidence, which also carriessetup_service_custody,material_digest, andstale_output_digest.The stored evidence is limited to hashes and identifiers, so there is no privacy defect today. Aligning with the established pattern keeps the denial surface minimal and consistent.
🤖 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/kernel.py` around lines 653 - 662, Introduce a dedicated prepare-denial resource context for sufficiency mutations containing only the requested selectors, and use it in the authorization check alongside ProjectGuideMutationPrepareDenialResourceContext. Update the corresponding sufficiency denial evidence construction and type references to use this narrower context, while retaining ProjectGuideSufficiencyMutationResourceContext for canonical mutation handling.backend/app/modules/projects/service.py (1)
178-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
REPRESENTATIVE_TASK_SOURCE_KINDSinstead of the literal"example".
_guide_source_materialselects representative items withitem.source_kind in REPRESENTATIVE_TASK_SOURCE_KINDS. This new builder hardcodes"example". If the constant gains another kind, the verified path and the legacy path will disagree about representative material, and the agent prompt digest will differ between paths.♻️ Proposed change
representative_task_material=RepresentativeTaskMaterialContext( items=[ - item for item in verified_items if item.source_kind == "example" + item + for item in verified_items + if item.source_kind in REPRESENTATIVE_TASK_SOURCE_KINDS ] ),🤖 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/service.py` around lines 178 - 182, Update the representative item filter in the RepresentativeTaskMaterialContext builder to check item.source_kind against the existing REPRESENTATIVE_TASK_SOURCE_KINDS constant instead of the literal "example", keeping selection consistent with _guide_source_material.backend/app/modules/projects/sufficiency_mutation_service.py (2)
489-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the return annotations on the async context managers.
run_agentandrun_setup_serviceare decorated with@asynccontextmanagerand useyield. Their annotations declare-> GuideSufficiencyMutationOutcome, which describes the value yielded, not the function result. Type checkers rejectyieldin a function annotated with a non-iterator return type.♻️ Proposed annotation fix
+from collections.abc import AsyncIterator @@ async def run_agent( self, resolved: ResolvedActor, prepared: PreparedAuthorizationService, key: UUID, project_id: UUID, guide_id: UUID, source_snapshot_id: UUID, - ) -> GuideSufficiencyMutationOutcome: + ) -> AsyncIterator[GuideSufficiencyMutationOutcome]: @@ custody: ProjectSetupServiceCustodyContext, - ) -> GuideSufficiencyMutationOutcome: + ) -> AsyncIterator[GuideSufficiencyMutationOutcome]:Also applies to: 515-526
🤖 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/sufficiency_mutation_service.py` around lines 489 - 498, Update the return annotations of the async context manager methods run_agent and run_setup_service from GuideSufficiencyMutationOutcome to the appropriate AsyncIterator or AsyncGenerator type representing the yielded outcome, while preserving their existing yielded values and context-manager behavior.
687-784: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the duplicated replay-recovery block.
The human replay-recovery block and the setup-service replay-recovery block repeat the same sequence: selector comparison, report provenance comparison,
_callerrebuild,_prepare, locked_lineage,consume,_prove_authority, and resource-digest comparison. The two copies already differ in small ways, for example the setup-service copy folds the status and response checks into the mismatch condition while the human copy raisesidempotency_pendingseparately. Divergence in this path weakens replay custody. Extract one helper that takesexecution_kind,material_digest, and the custody context.Also applies to: 900-1000
🤖 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/sufficiency_mutation_service.py` around lines 687 - 784, Extract the duplicated replay-recovery sequence from the human and setup-service paths into one helper, anchored to the existing replay handling around _caller, _prepare, _lineage, consume, and _prove_authority. Have the helper accept execution_kind, material_digest, and the setup-service custody context, while preserving each path’s required selector, report-provenance, pending-state, and resource-digest validations and returning the validated GuideSufficiencyMutationOutcome.backend/tests/test_projects.py (2)
2877-2881: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelect the default material result by identity, not by truthiness.
material_result or GuideSufficiencyMaterialResult(...)replaces a caller-supplied result whenever that result is falsy. A caller that passes a deliberately emptyGuideSufficiencyMaterialResultthen receives the helper's own object instead. Use an identity check so the caller's intent always wins.♻️ Proposed fix
async def load(self, _request: object) -> GuideSufficiencyMaterialResult: type(self).calls += 1 - return material_result or GuideSufficiencyMaterialResult( - source_items=(), provenance=() - ) + if material_result is None: + return GuideSufficiencyMaterialResult(source_items=(), provenance=()) + return material_result🤖 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 2877 - 2881, Update the material-result fallback in the load method to use an identity check against None rather than truthiness, so any caller-supplied GuideSufficiencyMaterialResult—including an intentionally empty one—is returned unchanged; only create the default result when material_result is None.
5970-5975: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the settings cache after the test, not only before it.
Line 5974 patches
WORKSTREAM_CELERY_TASK_ALWAYS_EAGERand line 5975 clears theget_settingscache.monkeypatchrestores the environment variable at teardown, but the cache keeps theSettingsobject that was built while the variable was patched. A later test in the same worker can then read the stale eager-mode setting.
test_project_setup_run_rejects_cross_context_worker_updatesat lines 6415-6417 has the same gap. The file already uses the correct form at lines 8202 and 8218-8219.♻️ Proposed teardown
monkeypatch.setenv("WORKSTREAM_CELERY_TASK_ALWAYS_EAGER", "true") get_settings.cache_clear() + monkeypatch.setattr( + get_settings, "cache_clear", get_settings.cache_clear, raising=False + )A clearer option is a shared autouse fixture that calls
get_settings.cache_clear()after every test, which removes the need for per-test teardown at all call sites.🤖 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 5970 - 5975, Ensure settings cache cleanup runs after these tests, not only before them: update test_verified_worker_composes_fresh_exact_setup_service_authority and test_project_setup_run_rejects_cross_context_worker_updates to clear get_settings after execution, preferably via the file’s shared autouse fixture pattern if available. Preserve the existing pre-test cache clearing where needed.docs/roadmap_status.md (1)
65-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCite the merge evidence for the new ledger entry.
Lines 13-15 require code, migrations, tests, and merged history as evidence for an implemented claim. The adjacent bullet at lines 60-64 follows that rule and names PRs
#242,#248,#255, and#257. The new entry names no pull request.Add the PR reference so the entry carries the same evidence as its neighbors.
📝 Proposed wording
- Project Manager-authorized guide-sufficiency creation, agent-run requests, and warning acknowledgement with UUID replay custody; the fixed project-setup - service has only the internal sufficiency-run capability. + service has only the internal sufficiency-run capability. This merged through + PR `#263`.As per path instructions: "Treat docs/roadmap_status.md as the current capability ledger".
🤖 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 `@docs/roadmap_status.md` around lines 65 - 67, Update the new ledger entry describing Project Manager-authorized guide-sufficiency creation and related capabilities to include the merged pull request reference that evidences its implementation, matching the PR citation style used by the adjacent entries in docs/roadmap_status.md.Source: Path instructions
backend/tests/test_authorization.py (1)
5558-5568: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the handle is single-use.
The docstring claims a single-use handle, and the test proves that a rejected
consumedoes not burn the handle. It does not prove that a second successfulconsumeis refused. Add one trailing call to close that gap:💚 Proposed assertion
assert decision.matched_scope_project_id is None assert len(evidence.events) == 1 + with pytest.raises(PreparedAuthorizationHandleInvalid): + await prepared.consume( + handle, + ActionId.PROJECT_GUIDE_SUFFICIENCY_RUN, + caller, + resource, + ) + assert len(evidence.events) == 1The equivalent replay assertion already exists for the artifact authority at lines 5725-5727.
🤖 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 5558 - 5568, Extend the test around the successful consume of PROJECT_GUIDE_SUFFICIENCY_RUN to call prepared.consume again with the same handle, caller, and resource, then assert the replay is refused as required for a single-use handle. Keep the existing success and evidence assertions intact, matching the equivalent artifact-authority replay assertion.backend/tests/test_alembic.py (1)
235-260: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the provenance branch of the downgrade guard.
clear_evidenceproves only one of the two refusal branches. The seeded report at lines 172-175 sets no provenance columns, soprovenance_countin0050_guide_sufficiency_authority.downgradestays 0 for the whole test. Afterclear_evidencetruncates the replay table,replay_countalso reaches 0, so the second refusal condition never fires on its own.Add one case that clears the replay ledger, sets creation or acknowledgement provenance on
guide_sufficiency_reports, and asserts the same refusal. That proves attributed product provenance alone blocks rollback, which is the guarantee documented indocs/spec_authorization_service.mdlines 1084-1089.🤖 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 235 - 260, The migration downgrade test currently covers only replay evidence; extend the test around clear_evidence and the downgrade assertion to add a separate case that truncates the replay ledger, sets creation or acknowledgement provenance on the seeded guide_sufficiency_reports row, and verifies downgrade raises the same RuntimeError. Ensure the case demonstrates provenance alone blocks rollback, then preserve cleanup and restoration to HEAD_REVISION.
🤖 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/ACTIVATION_CUSTODY.md:
- Line 71: Update the custody counts in ACTIVATION_CUSTODY.md to reflect the
trusted-branch state: keep the three 12E actions counted as planned rather than
active until bounded merge is complete. Alternatively, explicitly label the
current counts as post-merge expected state and move the update to the
post-merge record, consistent with STATUS.md.
In
@.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md:
- Around line 492-494: Update the Celery scope statement near the legacy
role-check replacement to say that 12E changes worker admission and
command-boundary custody, while 12B2 owns the full setup-service call-graph
cutover; remove the blanket claim that 12E does not modify Celery workers.
In `@backend/alembic/versions/0050_guide_sufficiency_authority.py`:
- Around line 72-110: The check-constraint names in the migration are subject to
Alembic naming-convention expansion. Update both create_check_constraint calls
for ck_guide_sufficiency_creation_authority_shape and
ck_guide_sufficiency_ack_authority_shape to pass their names through op.f(...),
keeping the suffixes unchanged so upgrade and downgrade constraint names remain
consistent.
In `@backend/app/modules/authorization/kernel.py`:
- Around line 1017-1023: Update the sufficiency-kind guard in the authorization
kernel to require execution_kind == "human", matching the restriction used by
the fixed-service guard. Keep the existing denial conditions unchanged, and
ensure non-human execution kinds are denied before reaching _prove_authority.
---
Outside diff comments:
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 82-99: In the setup queue flow around pre_submit_setup_task_id and
get_project_setup_run, fail fast with ProjectSetupQueueError when setup_run is
absent, before calling enqueue_pre_submit_setup_pipeline. Also handle a returned
task ID mismatch as a distinct post-acceptance identity error rather than
recording status="enqueue_failed" or logging it as an enqueue failure; preserve
normal enqueue failure handling for dispatch exceptions.
---
Nitpick comments:
In `@backend/alembic/versions/0050_guide_sufficiency_authority.py`:
- Around line 252-260: Update the cleanup loop over _CREATION_COLUMNS and
_ACK_COLUMNS to use one exclusion set containing created_by_service_identity and
the four scope/action names, while preserving the existing foreign-key
constraint and column removal behavior.
- Around line 176-209: Extend the replay-table protection around
reject_sufficiency_replay_mutation by adding a statement-level TRUNCATE trigger
that raises the same append-only exception, preventing truncation from removing
evidence. Update the test cleanup in backend/tests/test_alembic.py to use DELETE
instead of TRUNCATE for guide_sufficiency_mutation_idempotency_records.
In `@backend/app/modules/authorization/kernel.py`:
- Around line 653-662: Introduce a dedicated prepare-denial resource context for
sufficiency mutations containing only the requested selectors, and use it in the
authorization check alongside ProjectGuideMutationPrepareDenialResourceContext.
Update the corresponding sufficiency denial evidence construction and type
references to use this narrower context, while retaining
ProjectGuideSufficiencyMutationResourceContext for canonical mutation handling.
In `@backend/app/modules/authorization/prepared.py`:
- Around line 519-536: Store the constructed
ProjectGuideSufficiencyMutationResourceContext instance in a local validated
variable instead of discarding it, then update the sufficiency binding fields to
read from validated (including scope_project_id and the other mapped attributes)
rather than the raw sufficiency dictionary. Keep the existing constructor inputs
and validation behavior unchanged.
In `@backend/app/modules/projects/service.py`:
- Around line 178-182: Update the representative item filter in the
RepresentativeTaskMaterialContext builder to check item.source_kind against the
existing REPRESENTATIVE_TASK_SOURCE_KINDS constant instead of the literal
"example", keeping selection consistent with _guide_source_material.
In `@backend/app/modules/projects/sufficiency_mutation_service.py`:
- Around line 489-498: Update the return annotations of the async context
manager methods run_agent and run_setup_service from
GuideSufficiencyMutationOutcome to the appropriate AsyncIterator or
AsyncGenerator type representing the yielded outcome, while preserving their
existing yielded values and context-manager behavior.
- Around line 687-784: Extract the duplicated replay-recovery sequence from the
human and setup-service paths into one helper, anchored to the existing replay
handling around _caller, _prepare, _lineage, consume, and _prove_authority. Have
the helper accept execution_kind, material_digest, and the setup-service custody
context, while preserving each path’s required selector, report-provenance,
pending-state, and resource-digest validations and returning the validated
GuideSufficiencyMutationOutcome.
In `@backend/tests/test_alembic.py`:
- Around line 235-260: The migration downgrade test currently covers only replay
evidence; extend the test around clear_evidence and the downgrade assertion to
add a separate case that truncates the replay ledger, sets creation or
acknowledgement provenance on the seeded guide_sufficiency_reports row, and
verifies downgrade raises the same RuntimeError. Ensure the case demonstrates
provenance alone blocks rollback, then preserve cleanup and restoration to
HEAD_REVISION.
In `@backend/tests/test_authorization.py`:
- Around line 5558-5568: Extend the test around the successful consume of
PROJECT_GUIDE_SUFFICIENCY_RUN to call prepared.consume again with the same
handle, caller, and resource, then assert the replay is refused as required for
a single-use handle. Keep the existing success and evidence assertions intact,
matching the equivalent artifact-authority replay assertion.
In `@backend/tests/test_projects.py`:
- Around line 2877-2881: Update the material-result fallback in the load method
to use an identity check against None rather than truthiness, so any
caller-supplied GuideSufficiencyMaterialResult—including an intentionally empty
one—is returned unchanged; only create the default result when material_result
is None.
- Around line 5970-5975: Ensure settings cache cleanup runs after these tests,
not only before them: update
test_verified_worker_composes_fresh_exact_setup_service_authority and
test_project_setup_run_rejects_cross_context_worker_updates to clear
get_settings after execution, preferably via the file’s shared autouse fixture
pattern if available. Preserve the existing pre-test cache clearing where
needed.
In `@docs/roadmap_status.md`:
- Around line 65-67: Update the new ledger entry describing Project
Manager-authorized guide-sufficiency creation and related capabilities to
include the merged pull request reference that evidences its implementation,
matching the PR citation style used by the adjacent entries in
docs/roadmap_status.md.
🪄 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: bbe49ac1-b18f-4b0a-85b9-986174e30107
📒 Files selected for processing (32)
.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-AUTH-001-workstream-authorization-service/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-pr-trust-bundle.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-preimplementation-review-evidence.md.github/workflows/backend.ymlbackend/alembic/versions/0050_guide_sufficiency_authority.pybackend/app/modules/artifacts/authorization.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/guide_mutation_router.pybackend/app/modules/projects/models.pybackend/app/modules/projects/router.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/modules/projects/sufficiency_mutation_repository.pybackend/app/modules/projects/sufficiency_mutation_service.pybackend/app/workers/project_setup.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/operations_authorization_service.mddocs/operations_project_operating_manual.mddocs/roadmap_status.mddocs/spec_authorization_service.md
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/modules/projects/sufficiency_mutation_service.py (1)
342-350: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo mutation entry points carry return annotations that contradict their call protocol.
create_reportis a plain coroutine but is annotated as an async iterator, whilerun_setup_serviceis an@asynccontextmanagerbut is annotated with the plain outcome type. Align each annotation with its actual protocol, usingrun_agentat line 504 as the reference for the context-manager form.
backend/app/modules/projects/sufficiency_mutation_service.py#L342-L350: change the return type toGuideSufficiencyMutationOutcome.backend/app/modules/projects/sufficiency_mutation_service.py#L521-L532: change the return type toAsyncIterator[GuideSufficiencyMutationOutcome].🤖 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/sufficiency_mutation_service.py` around lines 342 - 350, The return annotations in backend/app/modules/projects/sufficiency_mutation_service.py at lines 342-350 and 521-532 contradict the mutation protocols. Update create_report to return GuideSufficiencyMutationOutcome, and update run_setup_service to return AsyncIterator[GuideSufficiencyMutationOutcome], matching the `@asynccontextmanager` pattern used by run_agent; no other behavior changes are needed.backend/app/workers/project_setup.py (1)
294-341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the session before writing a terminal failure state.
If an earlier database operation leaves
sessionrollback-only,service.update_project_setup_run_status(...)can raisePendingRollbackErrorinstead of persistingsetup_blockedorfailed. Roll back both exception branches before the status update.Proposed fix
except ProjectServiceError as exc: + await session.rollback() public_error = safe_project_setup_error_summary(str(exc)) @@ except Exception as exc: + await session.rollback() public_error = "unexpected project setup pipeline failure"🤖 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/workers/project_setup.py` around lines 294 - 341, Roll back the database session at the start of both the ProjectServiceError and generic Exception handlers, before calling service.update_project_setup_run_status. Ensure the rollback resets any rollback-only transaction state while preserving the existing setup_blocked and failed status updates.backend/scripts/api_contract_e2e.py (1)
961-968: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRemove the synchronous setup execution path.
asyncio.to_thread()invokesrun_pre_submit_setup_pipeline()from an async E2E flow. Expose or call a typed async setup-pipeline entry point and await it directly. Keep a synchronous Celery wrapper only at the worker boundary.As per coding guidelines, “Execution is async-first; do not document or implement synchronous-first checkers or jobs.”
🤖 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/scripts/api_contract_e2e.py` around lines 961 - 968, Replace the asyncio.to_thread invocation around run_pre_submit_setup_pipeline with a typed asynchronous setup-pipeline entry point and await it directly in the E2E flow. Keep any synchronous Celery adapter limited to the worker boundary, and update the setup pipeline implementation so the async entry point is the primary execution path.Source: Coding guidelines
backend/app/modules/projects/setup_queue.py (1)
100-115: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe fresh-claim branch builds a non-deterministic task id, so dispatch always fails.
Line 101 builds
f"guide-setup-{setup_run_id}-g{setup_generation}". Line 91 computesexpected_task_idfrompre_submit_setup_task_id, which returns a UUID5 string. The two values can never be equal, so the guard at line 114 raisesProjectSetupQueueErrorfor every run in statusqueuedorenqueue_failed. That is the primary dispatch path, so no setup pipeline is ever enqueued.
backend/tests/test_projects.pyconfirms the intended value:test_verified_setup_enqueue_failure_is_sanitized_and_retryableassertsbody["celery_task_id"] == project_setup_queue_module.pre_submit_setup_task_id(body["id"], body["setup_generation"]).Use
expected_task_idfor the new claim.🐛 Proposed fix for the deterministic task identity
elif setup_run.status in {"queued", "enqueue_failed"}: - deterministic_task_id = f"guide-setup-{setup_run_id}-g{setup_generation}" + deterministic_task_id = expected_task_id setup_run.status = "dispatch_pending" setup_run.current_step = "dispatch" setup_run.celery_task_id = deterministic_task_id🤖 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/setup_queue.py` around lines 100 - 115, Update the fresh-claim branch in the setup queue flow to assign expected_task_id, rather than constructing the "guide-setup-..." value, to setup_run.celery_task_id. Keep the existing identity guard and ensure queued or enqueue_failed runs use the UUID5 value returned by pre_submit_setup_task_id.
🧹 Nitpick comments (2)
backend/alembic/versions/0051_guide_sufficiency_authority.py (1)
188-237: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueVerify trigger and function ownership across repeated upgrade/downgrade cycles.
upgrade()createsreject_sufficiency_replay_mutation()andreject_sufficiency_replay_truncate()withcreate function, notcreate or replace function. If a previous partial upgrade left either function present, the migration fails. Thedowngrade()drops both functions unconditionally, which is only safe while no other object depends on them.Consider
create or replace functionanddrop function if existsfor idempotent re-runs.🤖 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/0051_guide_sufficiency_authority.py` around lines 188 - 237, Make the trigger-function creation in upgrade() safe across repeated or partial runs by replacing both create function statements for reject_sufficiency_replay_mutation and reject_sufficiency_replay_truncate with create or replace function. Update downgrade() to use drop function if exists for both functions, preserving dependency-safe ordering with their associated triggers.backend/app/modules/projects/sufficiency_mutation_service.py (1)
659-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a public accessor over
ProjectService._verified_report_usages.This call reaches into a private method of another class. A public method on
ProjectService, or a shared validation helper, keeps the boundary explicit and prevents silent breakage whenProjectServiceinternals change. The same private call already exists in_run_agentpaths, so one public seam covers both.🤖 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/sufficiency_mutation_service.py` at line 659, Replace the direct call to the private `_verified_report_usages` method in the sufficiency mutation flow with a public accessor on `ProjectService` (or shared validation helper), and update the existing `_run_agent` paths to use the same public seam so all callers avoid reaching into `ProjectService` internals.
🤖 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-12E-pr-trust-bundle.md:
- Around line 24-25: Correct all three references to the authorization migration
in this record from 0050 to 0051: the added-migration summary, the migration
selector result, and the replay/provenance constraints statement. Use the
established migration identifier 0051_guide_sufficiency_authority consistently,
without changing references to the separate 0050_guide_source_v2 migration.
In `@backend/alembic/versions/0051_guide_sufficiency_authority.py`:
- Around line 41-52: Update the status list in the upgrade() check constraint
recreation to include 'dispatch_pending', preserving compatibility with
ProjectSetupRun and setup_queue.py. Leave the downgrade() constraint unchanged
so it continues to use the pre-0051 status values without 'dispatch_pending'.
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 160-167: Move the await session.commit() call in the
identity-mismatch handling after the setup_run status conditional so it executes
unconditionally before returning None. Keep the existing status, step,
error-code, and error-summary updates guarded by the dispatch_pending check,
matching the enqueue-failure commit behavior.
In `@backend/tests/test_projects.py`:
- Around line 2688-2702: Remove the stale pre-cutover assertions in
backend/tests/test_projects.py at lines 2688-2702, 2773-2789, and 3932-3937: at
2688-2702 remove the report/policy provenance block and retain report is None
and policy is None; at 2773-2789 remove enqueued[0] access and the
pre_submit_setup_task_id comparison, retaining enqueued == [] and celery_task_id
is None; at 3932-3937 remove the pre_submit_setup_task_id comparison, retaining
celery_task_id is None and status == "queued".
In `@docs/spec_chunk_3_project_guide_foundation.md`:
- Around line 152-156: Add POST
/api/v1/projects/{project_id}/guides/{guide_id}/source-snapshots/{source_snapshot_id}/run-sufficiency-agent
to the protected-route API inventory, placing it beside the existing
source-snapshot route and keeping the inventory consistent with the OpenAPI
contract.
---
Outside diff comments:
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 100-115: Update the fresh-claim branch in the setup queue flow to
assign expected_task_id, rather than constructing the "guide-setup-..." value,
to setup_run.celery_task_id. Keep the existing identity guard and ensure queued
or enqueue_failed runs use the UUID5 value returned by pre_submit_setup_task_id.
In `@backend/app/modules/projects/sufficiency_mutation_service.py`:
- Around line 342-350: The return annotations in
backend/app/modules/projects/sufficiency_mutation_service.py at lines 342-350
and 521-532 contradict the mutation protocols. Update create_report to return
GuideSufficiencyMutationOutcome, and update run_setup_service to return
AsyncIterator[GuideSufficiencyMutationOutcome], matching the
`@asynccontextmanager` pattern used by run_agent; no other behavior changes are
needed.
In `@backend/app/workers/project_setup.py`:
- Around line 294-341: Roll back the database session at the start of both the
ProjectServiceError and generic Exception handlers, before calling
service.update_project_setup_run_status. Ensure the rollback resets any
rollback-only transaction state while preserving the existing setup_blocked and
failed status updates.
In `@backend/scripts/api_contract_e2e.py`:
- Around line 961-968: Replace the asyncio.to_thread invocation around
run_pre_submit_setup_pipeline with a typed asynchronous setup-pipeline entry
point and await it directly in the E2E flow. Keep any synchronous Celery adapter
limited to the worker boundary, and update the setup pipeline implementation so
the async entry point is the primary execution path.
---
Nitpick comments:
In `@backend/alembic/versions/0051_guide_sufficiency_authority.py`:
- Around line 188-237: Make the trigger-function creation in upgrade() safe
across repeated or partial runs by replacing both create function statements for
reject_sufficiency_replay_mutation and reject_sufficiency_replay_truncate with
create or replace function. Update downgrade() to use drop function if exists
for both functions, preserving dependency-safe ordering with their associated
triggers.
In `@backend/app/modules/projects/sufficiency_mutation_service.py`:
- Line 659: Replace the direct call to the private `_verified_report_usages`
method in the sufficiency mutation flow with a public accessor on
`ProjectService` (or shared validation helper), and update the existing
`_run_agent` paths to use the same public seam so all callers avoid reaching
into `ProjectService` internals.
🪄 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: e35540a5-cf80-4b06-b3e5-d0b2a2e71718
📒 Files selected for processing (28)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-pr-trust-bundle.md.github/workflows/backend.ymlbackend/alembic/versions/0051_guide_sufficiency_authority.pybackend/app/modules/authorization/kernel.pybackend/app/modules/projects/guide_mutation_router.pybackend/app/modules/projects/models.pybackend/app/modules/projects/router.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/modules/projects/sufficiency_mutation_service.pybackend/app/workers/project_setup.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_api_controls.pybackend/tests/test_artifact_internal_authorization.pybackend/tests/test_audit.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/architecture_data_model.mddocs/glossary.mddocs/operations_authorization_service.mddocs/spec_authorization_service.mddocs/spec_chunk_3_project_guide_foundation.md
💤 Files with no reviewable changes (1)
- backend/app/modules/projects/guide_mutation_router.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/backend.yml
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md
- backend/app/modules/authorization/kernel.py
- docs/operations_authorization_service.md
- backend/app/modules/projects/service.py
- backend/app/modules/projects/router.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/modules/projects/sufficiency_mutation_service.py (1)
342-350: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo mutation entry points carry return annotations that contradict their call protocol.
create_reportis a plain coroutine but is annotated as an async iterator, whilerun_setup_serviceis an@asynccontextmanagerbut is annotated with the plain outcome type. Align each annotation with its actual protocol, usingrun_agentat line 504 as the reference for the context-manager form.
backend/app/modules/projects/sufficiency_mutation_service.py#L342-L350: change the return type toGuideSufficiencyMutationOutcome.backend/app/modules/projects/sufficiency_mutation_service.py#L521-L532: change the return type toAsyncIterator[GuideSufficiencyMutationOutcome].🤖 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/sufficiency_mutation_service.py` around lines 342 - 350, The return annotations in backend/app/modules/projects/sufficiency_mutation_service.py at lines 342-350 and 521-532 contradict the mutation protocols. Update create_report to return GuideSufficiencyMutationOutcome, and update run_setup_service to return AsyncIterator[GuideSufficiencyMutationOutcome], matching the `@asynccontextmanager` pattern used by run_agent; no other behavior changes are needed.backend/app/workers/project_setup.py (1)
294-341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the session before writing a terminal failure state.
If an earlier database operation leaves
sessionrollback-only,service.update_project_setup_run_status(...)can raisePendingRollbackErrorinstead of persistingsetup_blockedorfailed. Roll back both exception branches before the status update.Proposed fix
except ProjectServiceError as exc: + await session.rollback() public_error = safe_project_setup_error_summary(str(exc)) @@ except Exception as exc: + await session.rollback() public_error = "unexpected project setup pipeline failure"🤖 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/workers/project_setup.py` around lines 294 - 341, Roll back the database session at the start of both the ProjectServiceError and generic Exception handlers, before calling service.update_project_setup_run_status. Ensure the rollback resets any rollback-only transaction state while preserving the existing setup_blocked and failed status updates.backend/scripts/api_contract_e2e.py (1)
961-968: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRemove the synchronous setup execution path.
asyncio.to_thread()invokesrun_pre_submit_setup_pipeline()from an async E2E flow. Expose or call a typed async setup-pipeline entry point and await it directly. Keep a synchronous Celery wrapper only at the worker boundary.As per coding guidelines, “Execution is async-first; do not document or implement synchronous-first checkers or jobs.”
🤖 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/scripts/api_contract_e2e.py` around lines 961 - 968, Replace the asyncio.to_thread invocation around run_pre_submit_setup_pipeline with a typed asynchronous setup-pipeline entry point and await it directly in the E2E flow. Keep any synchronous Celery adapter limited to the worker boundary, and update the setup pipeline implementation so the async entry point is the primary execution path.Source: Coding guidelines
backend/app/modules/projects/setup_queue.py (1)
100-115: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThe fresh-claim branch builds a non-deterministic task id, so dispatch always fails.
Line 101 builds
f"guide-setup-{setup_run_id}-g{setup_generation}". Line 91 computesexpected_task_idfrompre_submit_setup_task_id, which returns a UUID5 string. The two values can never be equal, so the guard at line 114 raisesProjectSetupQueueErrorfor every run in statusqueuedorenqueue_failed. That is the primary dispatch path, so no setup pipeline is ever enqueued.
backend/tests/test_projects.pyconfirms the intended value:test_verified_setup_enqueue_failure_is_sanitized_and_retryableassertsbody["celery_task_id"] == project_setup_queue_module.pre_submit_setup_task_id(body["id"], body["setup_generation"]).Use
expected_task_idfor the new claim.🐛 Proposed fix for the deterministic task identity
elif setup_run.status in {"queued", "enqueue_failed"}: - deterministic_task_id = f"guide-setup-{setup_run_id}-g{setup_generation}" + deterministic_task_id = expected_task_id setup_run.status = "dispatch_pending" setup_run.current_step = "dispatch" setup_run.celery_task_id = deterministic_task_id🤖 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/setup_queue.py` around lines 100 - 115, Update the fresh-claim branch in the setup queue flow to assign expected_task_id, rather than constructing the "guide-setup-..." value, to setup_run.celery_task_id. Keep the existing identity guard and ensure queued or enqueue_failed runs use the UUID5 value returned by pre_submit_setup_task_id.
🧹 Nitpick comments (2)
backend/alembic/versions/0051_guide_sufficiency_authority.py (1)
188-237: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueVerify trigger and function ownership across repeated upgrade/downgrade cycles.
upgrade()createsreject_sufficiency_replay_mutation()andreject_sufficiency_replay_truncate()withcreate function, notcreate or replace function. If a previous partial upgrade left either function present, the migration fails. Thedowngrade()drops both functions unconditionally, which is only safe while no other object depends on them.Consider
create or replace functionanddrop function if existsfor idempotent re-runs.🤖 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/0051_guide_sufficiency_authority.py` around lines 188 - 237, Make the trigger-function creation in upgrade() safe across repeated or partial runs by replacing both create function statements for reject_sufficiency_replay_mutation and reject_sufficiency_replay_truncate with create or replace function. Update downgrade() to use drop function if exists for both functions, preserving dependency-safe ordering with their associated triggers.backend/app/modules/projects/sufficiency_mutation_service.py (1)
659-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a public accessor over
ProjectService._verified_report_usages.This call reaches into a private method of another class. A public method on
ProjectService, or a shared validation helper, keeps the boundary explicit and prevents silent breakage whenProjectServiceinternals change. The same private call already exists in_run_agentpaths, so one public seam covers both.🤖 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/sufficiency_mutation_service.py` at line 659, Replace the direct call to the private `_verified_report_usages` method in the sufficiency mutation flow with a public accessor on `ProjectService` (or shared validation helper), and update the existing `_run_agent` paths to use the same public seam so all callers avoid reaching into `ProjectService` internals.
🤖 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-12E-pr-trust-bundle.md:
- Around line 24-25: Correct all three references to the authorization migration
in this record from 0050 to 0051: the added-migration summary, the migration
selector result, and the replay/provenance constraints statement. Use the
established migration identifier 0051_guide_sufficiency_authority consistently,
without changing references to the separate 0050_guide_source_v2 migration.
In `@backend/alembic/versions/0051_guide_sufficiency_authority.py`:
- Around line 41-52: Update the status list in the upgrade() check constraint
recreation to include 'dispatch_pending', preserving compatibility with
ProjectSetupRun and setup_queue.py. Leave the downgrade() constraint unchanged
so it continues to use the pre-0051 status values without 'dispatch_pending'.
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 160-167: Move the await session.commit() call in the
identity-mismatch handling after the setup_run status conditional so it executes
unconditionally before returning None. Keep the existing status, step,
error-code, and error-summary updates guarded by the dispatch_pending check,
matching the enqueue-failure commit behavior.
In `@backend/tests/test_projects.py`:
- Around line 2688-2702: Remove the stale pre-cutover assertions in
backend/tests/test_projects.py at lines 2688-2702, 2773-2789, and 3932-3937: at
2688-2702 remove the report/policy provenance block and retain report is None
and policy is None; at 2773-2789 remove enqueued[0] access and the
pre_submit_setup_task_id comparison, retaining enqueued == [] and celery_task_id
is None; at 3932-3937 remove the pre_submit_setup_task_id comparison, retaining
celery_task_id is None and status == "queued".
In `@docs/spec_chunk_3_project_guide_foundation.md`:
- Around line 152-156: Add POST
/api/v1/projects/{project_id}/guides/{guide_id}/source-snapshots/{source_snapshot_id}/run-sufficiency-agent
to the protected-route API inventory, placing it beside the existing
source-snapshot route and keeping the inventory consistent with the OpenAPI
contract.
---
Outside diff comments:
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 100-115: Update the fresh-claim branch in the setup queue flow to
assign expected_task_id, rather than constructing the "guide-setup-..." value,
to setup_run.celery_task_id. Keep the existing identity guard and ensure queued
or enqueue_failed runs use the UUID5 value returned by pre_submit_setup_task_id.
In `@backend/app/modules/projects/sufficiency_mutation_service.py`:
- Around line 342-350: The return annotations in
backend/app/modules/projects/sufficiency_mutation_service.py at lines 342-350
and 521-532 contradict the mutation protocols. Update create_report to return
GuideSufficiencyMutationOutcome, and update run_setup_service to return
AsyncIterator[GuideSufficiencyMutationOutcome], matching the
`@asynccontextmanager` pattern used by run_agent; no other behavior changes are
needed.
In `@backend/app/workers/project_setup.py`:
- Around line 294-341: Roll back the database session at the start of both the
ProjectServiceError and generic Exception handlers, before calling
service.update_project_setup_run_status. Ensure the rollback resets any
rollback-only transaction state while preserving the existing setup_blocked and
failed status updates.
In `@backend/scripts/api_contract_e2e.py`:
- Around line 961-968: Replace the asyncio.to_thread invocation around
run_pre_submit_setup_pipeline with a typed asynchronous setup-pipeline entry
point and await it directly in the E2E flow. Keep any synchronous Celery adapter
limited to the worker boundary, and update the setup pipeline implementation so
the async entry point is the primary execution path.
---
Nitpick comments:
In `@backend/alembic/versions/0051_guide_sufficiency_authority.py`:
- Around line 188-237: Make the trigger-function creation in upgrade() safe
across repeated or partial runs by replacing both create function statements for
reject_sufficiency_replay_mutation and reject_sufficiency_replay_truncate with
create or replace function. Update downgrade() to use drop function if exists
for both functions, preserving dependency-safe ordering with their associated
triggers.
In `@backend/app/modules/projects/sufficiency_mutation_service.py`:
- Line 659: Replace the direct call to the private `_verified_report_usages`
method in the sufficiency mutation flow with a public accessor on
`ProjectService` (or shared validation helper), and update the existing
`_run_agent` paths to use the same public seam so all callers avoid reaching
into `ProjectService` internals.
🪄 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: e35540a5-cf80-4b06-b3e5-d0b2a2e71718
📒 Files selected for processing (28)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-pr-trust-bundle.md.github/workflows/backend.ymlbackend/alembic/versions/0051_guide_sufficiency_authority.pybackend/app/modules/authorization/kernel.pybackend/app/modules/projects/guide_mutation_router.pybackend/app/modules/projects/models.pybackend/app/modules/projects/router.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/modules/projects/sufficiency_mutation_service.pybackend/app/workers/project_setup.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_api_controls.pybackend/tests/test_artifact_internal_authorization.pybackend/tests/test_audit.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/architecture_data_model.mddocs/glossary.mddocs/operations_authorization_service.mddocs/spec_authorization_service.mddocs/spec_chunk_3_project_guide_foundation.md
💤 Files with no reviewable changes (1)
- backend/app/modules/projects/guide_mutation_router.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/backend.yml
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md
- backend/app/modules/authorization/kernel.py
- docs/operations_authorization_service.md
- backend/app/modules/projects/service.py
- backend/app/modules/projects/router.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md
🛑 Comments failed to post (1)
backend/tests/test_projects.py (1)
2688-2702: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Stale pre-cutover assertions remain beside the new assertions in three tests. Each site keeps both the old expectation and the new, opposite expectation for the same variable, so each test fails deterministically. Remove the stale block in each test and keep the assertions that match the post-cutover behavior.
backend/tests/test_projects.py#L2688-L2702: remove thereport is not None/policy is not Noneprovenance block and keepreport is Noneandpolicy is None.backend/tests/test_projects.py#L2773-L2789: remove theenqueued[0]access and thecelery_task_id == pre_submit_setup_task_id(...)assertion, and keepenqueued == []withcelery_task_id is None.backend/tests/test_projects.py#L3932-L3937: remove thecelery_task_id == pre_submit_setup_task_id(...)assertion and keepcelery_task_id is Nonewithstatus == "queued".📍 Affects 1 file
backend/tests/test_projects.py#L2688-L2702(this comment)backend/tests/test_projects.py#L2773-L2789backend/tests/test_projects.py#L3932-L3937🤖 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 2688 - 2702, Remove the stale pre-cutover assertions in backend/tests/test_projects.py at lines 2688-2702, 2773-2789, and 3932-3937: at 2688-2702 remove the report/policy provenance block and retain report is None and policy is None; at 2773-2789 remove enqueued[0] access and the pre_submit_setup_task_id comparison, retaining enqueued == [] and celery_task_id is None; at 3932-3937 remove the pre_submit_setup_task_id comparison, retaining celery_task_id is None and status == "queued".
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_projects.py (1)
2615-2652: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the docstring with the measured contract and cover identity mismatches separately.
This test only shows that a source snapshot with no verified material stays at
status == "queued"withoutcelery_task_id.enqueue_with_wrong_identityis never called because dispatch does not run, so the docstring should describe the verified-material gate instead of broker task-id acceptance. Add a separate test that reaches thedispatch_pendingenqueue path and returns a different task id to exerciseenqueue_identity_mismatch.🤖 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 2615 - 2652, The test docstring should describe the verified-material gate: source snapshots without verified material remain queued without a Celery task ID. Add a separate test that advances setup into the dispatch_pending enqueue path, monkeypatches enqueue_pre_submit_setup_pipeline to return a different task ID, and asserts the enqueue_identity_mismatch behavior and resulting setup-run state.backend/tests/test_alembic.py (1)
477-497: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the head revision in a
finallyblock in every migration-mutating test. These tests change the revision of the shared migrated database without guaranteeing recovery. If an assertion fails, or if a downgrade unexpectedly succeeds instead of raising, the database stays below head and every later test in the session runs against the wrong schema.test_0052_guide_sufficiency_authority_safe_empty_downgrade_and_reupgradeatbackend/tests/test_alembic.pyLine 104 already uses the correcttry/finallypattern.
backend/tests/test_alembic.py#L477-L497: wrap the downgrade, state assertion, and second downgrade intry, and callcommand.upgrade(config, HEAD_REVISION)infinallyinstead of the trailingcommand.upgrade(config, "head").backend/tests/test_review_queue_persistence.py#L563-L596: wrap thepytest.raisesdowngrade attempt intry, and restore the head revision undermigration_lock()infinally.backend/tests/test_review_queue_persistence.py#L599-L628: apply the sametry/finallyrestore around the downgrade attempt in this test.🤖 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 477 - 497, Migration-mutating tests must always restore the shared database to head, even when assertions or downgrade expectations fail. In backend/tests/test_alembic.py:477-497, wrap the downgrade, state assertion, and second downgrade in try/finally and call command.upgrade(config, HEAD_REVISION) in finally; in backend/tests/test_review_queue_persistence.py:563-596 and :599-628, apply the same try/finally restoration around each pytest.raises downgrade attempt while keeping restoration under migration_lock().
🧹 Nitpick comments (1)
backend/tests/test_projects.py (1)
8369-8473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the remaining compared report fields.
_validate_adoptable_verified_reportcompares thirteen report fields. Themismatchestable exercises six of them.project_id,guide_id,guide_version,source_snapshot_id,source_snapshot_hash, andagent_material_byte_countstay unexercised, so a dropped comparison on those fields would not fail this test. Add them to the same table; each entry costs one line.♻️ Proposed additional mismatch cases
mismatches = ( + ("project_id", str(uuid4())), + ("guide_id", str(uuid4())), + ("guide_version", "v2"), + ("source_snapshot_id", str(uuid4())), + ("source_snapshot_hash", sha256_hash("wrong-snapshot")), + ("agent_material_byte_count", 43), ("setup_generation", 3), ("project_setup_run_id", str(uuid4())),🤖 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 8369 - 8473, Expand the mismatches table in test_setup_service_adoption_requires_exact_report_and_source_provenance to include invalid values for project_id, guide_id, guide_version, source_snapshot_id, source_snapshot_hash, and agent_material_byte_count. Keep the existing loop and validation assertions unchanged so each compared report field is independently verified to raise sufficiency_report_provenance_mismatch.
🤖 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_review_queue_persistence.py`:
- Around line 585-591: Update the downgrade assertion around downgrade so it
expects the populated review-queue guard error, “cannot downgrade populated
review queue foundation,” instead of the stale guide-sufficiency-authority
message. Preserve the existing downgrade invocation and alembic_version
assertion, and do not attribute this failure to migration 0052.
---
Outside diff comments:
In `@backend/tests/test_alembic.py`:
- Around line 477-497: Migration-mutating tests must always restore the shared
database to head, even when assertions or downgrade expectations fail. In
backend/tests/test_alembic.py:477-497, wrap the downgrade, state assertion, and
second downgrade in try/finally and call command.upgrade(config, HEAD_REVISION)
in finally; in backend/tests/test_review_queue_persistence.py:563-596 and
:599-628, apply the same try/finally restoration around each pytest.raises
downgrade attempt while keeping restoration under migration_lock().
In `@backend/tests/test_projects.py`:
- Around line 2615-2652: The test docstring should describe the
verified-material gate: source snapshots without verified material remain queued
without a Celery task ID. Add a separate test that advances setup into the
dispatch_pending enqueue path, monkeypatches enqueue_pre_submit_setup_pipeline
to return a different task ID, and asserts the enqueue_identity_mismatch
behavior and resulting setup-run state.
---
Nitpick comments:
In `@backend/tests/test_projects.py`:
- Around line 8369-8473: Expand the mismatches table in
test_setup_service_adoption_requires_exact_report_and_source_provenance to
include invalid values for project_id, guide_id, guide_version,
source_snapshot_id, source_snapshot_hash, and agent_material_byte_count. Keep
the existing loop and validation assertions unchanged so each compared report
field is independently verified to raise sufficiency_report_provenance_mismatch.
🪄 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: 5d55fa52-9668-4aad-a493-db385723f768
📒 Files selected for processing (17)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-pr-trust-bundle.mdbackend/alembic/versions/0052_guide_sufficiency_authority.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/modules/projects/sufficiency_mutation_service.pybackend/app/workers/project_setup.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_projects.pybackend/tests/test_review_queue_persistence.pydocs/architecture_data_model.mddocs/operations_authorization_service.mddocs/operations_project_operating_manual.mddocs/spec_authorization_service.md
💤 Files with no reviewable changes (1)
- backend/app/modules/projects/service.py
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/architecture_data_model.md
- docs/operations_project_operating_manual.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-internal-review-evidence.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md
- backend/app/workers/project_setup.py
- backend/app/modules/projects/setup_queue.py
- docs/spec_authorization_service.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12E-pr-trust-bundle.md
…-guide-sufficiency # Conflicts: # backend/tests/conftest.py # backend/tests/test_alembic.py # backend/tests/test_review_queue_persistence.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
…-guide-sufficiency # Conflicts: # backend/tests/test_projects.py
…-guide-sufficiency
…-guide-sufficiency # Conflicts: # backend/tests/conftest.py # backend/tests/test_alembic.py # backend/tests/test_review_queue_persistence.py
…-guide-sufficiency
|
Final exact-head verification for
The route now commits stable async dispatch custody before broker publication; exact replay survives queue progress; only the fixed setup service materializes guide content and invokes the sufficiency agent. |
2 participants
Workstream PR Trust Bundle
Chunk
WS-AUTH-001-12E- Activate guide-sufficiency mutationsGoal
Activate exactly manual sufficiency-report creation, agent-run requests, and
warning acknowledgement. Keep public mutation Project Manager-only and grant
the fixed
workstream.project.setupservice only the internal run command.Intent And Planning Context
introducing another authorization protocol.
chunks/WS-AUTH-001-12E-guide-sufficiency-mutations.md.What Changed
sufficiency mutations.
provenance.
Why It Changed
The former guide-sufficiency mutations could not safely become live until the
actor, identity link, grant/service identity, guide lineage, material, request,
transaction, and idempotency facts were bound and revalidated atomically.
Design Chosen
PreparedAuthorizationHandle.canonical facts before the protected write.
the worker.
Alternatives Rejected
AuthorizationContextas durable authority: not transaction-bound.Scope Control
Allowed Files Changed
Files Outside Stated Scope
principal resolver; its focused adapter tests prove behavior is unchanged.
Product Behavior
agent run, and acknowledge warnings under exact project authority; only the
fixed project-setup service can execute the internal run command.
Evidence
Commands Run
Result Summary
The repository-wide suite and authoritative coverage gates run in GitHub
Actions; the user's machine is not used for the roughly four-hour local suite.
Acceptance Criteria Proof
operation, request, idempotency, session, and transaction facts.
mid-flight terminal transitions fail closed.
handles, bytes, credentials, or authorization contexts.
Test Delta
Tests Added
mid-flight terminal race, and deterministic broker identity tests.
refusal, schema parity, and API contract tests.
Tests Modified
and project setup fixtures were updated for the exact activated surface.
Tests Removed Or Skipped
Internal Reviewer Results
Reviewed code SHA:
aefec9e3703079744441161ea40356c308cd89fbReviewed at: 2026-08-03
Reviewer run IDs:
12e_arch_final,12e_impl_qa,12e_impl_senior,12e_product_final,12e_security_final,12e_test_delta, plus recorded CI,docs, and reuse tracks.
External Review
CI And Gate Integrity
Remaining Risks
per-file coverage, Agent Gates, and fresh CodeRabbit review.
ProjectServicesufficiency helpers remain a low-risk futureretirement item; live routes and workers use the new orchestrator.
Follow-Up Work
#263to the capability ledger only after human merge.bounded cleanup chunk.
Human Review Focus
Human Merge Ownership
Summary by CodeRabbit