diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/REVIEW_LOG.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/REVIEW_LOG.md index 877ce163..da56846b 100644 --- a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/REVIEW_LOG.md +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/REVIEW_LOG.md @@ -1123,3 +1123,43 @@ AUTH/ART/CON owner plan, or frozen reference source changed. The repaired candidate requires fresh exact-SHA internal review, deterministic gates, evidence rebinding, push, CodeRabbit re-review, and GitHub CI before PR #150 is ready for the user's merge decision. + +## WS-REV-001-03P Internal Implementation Review - 2026-07-23 + +Signed run `30014647556` authorized 03P on exact main `bcf1292e`. The bounded +implementation persists immutable ReviewPolicy and RevisionPolicy facts only. +It preserves migrated legacy facts privately, permits only an atomic legacy to +canonical conversion, serializes policy writes with guide publication, refuses +lossy downgrade, and exposes no retired archival fields. + +Review iterations repaired legacy activation compatibility, stamped Task +compensation isolation, deterministic PostgreSQL waiter proof, all-five-field +archival denial/non-exposure, and distinct-context direct-insert failures. +Exact code candidate `35531df254c6b25726d666a5e89eda997b97d792` +passes senior engineering, QA/test, security/auth, product/ops, architecture, +docs, reuse/dedup, test-delta, CI-integrity, and circuit-breaker review. No +finding remains. Full backend tests and coverage are intentionally deferred to +GitHub Actions; 03A has not started. + +## WS-REV-001-03P API E2E Repair - 2026-07-23 + +PR #195's first GitHub run passed preflight and Agent Gates but API E2E rejected +the stale Project Guide policy fixture. A prospective contract amendment +authorized only `backend/scripts/api_contract_e2e.py::guide_payload`; all +lifecycle, endpoint, authorization, response, and non-policy behavior remained +prohibited. Candidate `b68a1e22b3bf373d15479784d545fcc9b5737f64` +removes the five retired inputs, uses canonical finding fields, and supplies the +two reviewed durations. Ruff and diff integrity pass, and all required internal +tracks pass on the exact candidate. + +The completed Backend run then exposed shard 1's six stale 0033 head/rollback +expectations and shard 3's ART fixture policy-after-publication ordering. The +six assertions now name 0034 without changing historical targets. The ART +fixture repair was reverted after an internal scope finding, prospectively +authorized, and reapplied byte-identically: policies flush while the same guide +is draft, then its prior active/approval/effective values are restored before +Task, Submission, or CheckerRun setup. Final code candidate +`b9501b31565370306aea168ff4c218a2880e6c0b` passes the four exact Alembic tests, +the exact ART test, Ruff, diff integrity, and all internal tracks. CodeRabbit +did not perform a review because its limit was reached. Fresh GitHub and actual +CodeRabbit proof remain required. diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/STATUS.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/STATUS.md index 6d18ec34..36380cdb 100644 --- a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/STATUS.md +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/STATUS.md @@ -2,19 +2,20 @@ ## Current status -On 2026-07-22 a complete reread of the canonical Markdown and PDF specification -identified that the 02A family crossed the REV ownership boundary. REV begins -only when the current finalized checker result is `allow_review`; it consumes -the existing Submission and the same submitted/verified artifact set. Project -Guide setup, activation, Task intake, and upstream publication fencing belong to -their owning subsystems. REV reports gaps in those handoffs and does not repair -them. The attempted 02A1 runtime candidate was reverted in full. - -`WS-REV-001-PLAN3` is proposed planning-only work, not canonically active. It retires -02A/02A1/02A2/02A3/02A4/02B/02C as REV implementation authorization. The first -future REV chunk is 03P: REV-owned ReviewPolicy/RevisionPolicy persistence. -Canonical start requires the signed `Loop Memory Explicit Event` workflow on -exact current main; chat and local work are not start evidence. +`WS-REV-001-03P` is active through signed `Loop Memory Explicit Event` run +`30014647556` on exact trusted main +`bcf1292e1a591e3e84bf8ee212ee7191d80741fa`. Reviewed CodeRabbit-repair +implementation candidate `f19b2d798e49e7c3ebb05a19eaa53638c61237a1` +persists only REV-owned immutable ReviewPolicy and RevisionPolicy facts and +passed every required internal review track plus focused local evidence. Candidate +`35531df254c6b25726d666a5e89eda997b97d792` is retained only as historical +pre-external-repair authorization context. GitHub Actions full-suite/coverage, +incremental CodeRabbit confirmation, and human review remain before merge. + +The chunk does not implement Project Guide ownership, Task transitions, +Submission or artifact behavior, review queues or leases, Review decisions, +revision execution, FinalAcceptance, or contribution records. `03A` remains +stopped and requires a separate signed start after 03P merges. The historical text below records the superseded PLAN2/02A state and is not current implementation authority. Trusted main at that time was `44f2467c`, which additionally @@ -26,11 +27,11 @@ database/concurrency boundaries. That attempted split is now retired. ## Historical dependency snapshot -Current trusted main is `14fa4316f7d984f2176657bfafd2a2dae56f944e` +At the PLAN3 snapshot, trusted main was `14fa4316f7d984f2176657bfafd2a2dae56f944e` with sole Alembic head `0033_authorization_read_rate_control`. The bullets below describe the older PLAN2 snapshot and are not current runtime proof. -Canonical signed loop memory currently remains stopped after merged 02A and +At that snapshot, canonical signed loop memory remained stopped after merged 02A and names retired 02A1 as next. That projection is accurate historical automation state but no longer a valid REV scope choice. PLAN3 is proposed—not active—and its reviewed merge intent changes the same-initiative successor to 03P. No diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md index b1ae04b7..eb06bac4 100644 --- a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md @@ -2,8 +2,15 @@ ## Status -Proposed only. It may start only through a signed `Loop Memory Explicit Event` -on exact current main after PLAN3 merges and this contract is refreshed/reviewed. +Active through signed `Loop Memory Explicit Event` run `30014647556` on exact +trusted main `bcf1292e1a591e3e84bf8ee212ee7191d80741fa`. Superseded history SHA +`94702ae492341402b725ffd6751f8f81ac68100b` +records the content-equivalent CodeRabbit-repair tree that passed every required +internal review track and focused local gate; canonical exact-SHA binding lives +in the internal review evidence after the prospective commit sequence is final. +`35531df254c6b25726d666a5e89eda997b97d792` is historical pre-external-repair +authorization context only. Publication remains gated on final evidence review +and current-head GitHub Actions, CodeRabbit, and human review. ## Goal @@ -16,8 +23,54 @@ L1: policy immutability, duration/limit semantics, and later decision authority. ## Allowed files -To be fixed at signed start: REV-owned policy models, migration, schemas, -focused tests, and this initiative's evidence/merge-intent files only. +```text +backend/app/modules/projects/models.py +backend/app/modules/projects/schemas.py +backend/app/modules/projects/repository.py +backend/app/modules/projects/service.py +backend/alembic/versions/0034_review_revision_policy_persistence.py +backend/tests/test_alembic.py +backend/tests/test_projects.py +backend/tests/test_tasks.py +backend/tests/test_artifact_admission.py +backend/tests/conftest.py +backend/scripts/api_contract_e2e.py +docs/architecture_data_model.md +docs/template_project_guide.md +.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/{DISCOVERY,PLAN,STATUS,REVIEW_LOG}.md +.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/chunks/WS-REV-001-03P-review-revision-policy-persistence.md +.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-*.md +.agent-loop/merge-intents/WS-REV-001-03P.json +``` + +The following cross-owner files are proof-only exceptions: + +- `backend/tests/conftest.py`: update only the deterministic public-schema + fingerprint for migration 0034 and register `review_policies` plus + `revision_policies` in the existing `TRUNCATE_GUARDED_TABLES` reset path so + isolated test cleanup can temporarily disable only their user triggers before + truncation and restore them immediately afterward. Do not weaken the schema + fingerprint check or reset ownership checks. +- `backend/tests/test_artifact_admission.py`: update ReviewPolicy and + RevisionPolicy fixture construction for schema compatibility. Where 0034 + requires those immutable policies before publication, the owning helper may + construct the same guide as draft, flush the policies, and then assign its + previously existing active/approval/effective values before Task, Submission, + or CheckerRun setup. The final fixture state and every ART assertion and + behavior must remain unchanged. +- `backend/tests/test_tasks.py`: update only policy fixture construction and add + immutable-policy regression proof. No Task behavior may change, and the + existing stamped review, revision, and compensation-policy isolation + regression—including every compensation assertion—must remain present without + weakening. +- `backend/tests/test_projects.py`: adapt canonical policy requests/responses + while preserving every existing Project Guide activation outcome. Retired + request fields may become schema-denial tests, but activation denial may not + be removed or broadened by 03P. +- `backend/scripts/api_contract_e2e.py`: update only the existing Project Guide + policy request fixture for the canonical 03P schema. No API lifecycle step, + endpoint expectation, authorization assertion, or non-policy fixture may + change. ## Not allowed @@ -32,15 +85,144 @@ focused tests, and this initiative's evidence/merge-intent files only. the exact upstream context they govern without mutating that context. - Review preference/lease duration and human revision limits/deadlines have explicit typed semantics and are never inferred from unrelated SLA fields. +- Migration revision `0034_review_revision_policy` descends only from + `0033_authorization_read_rate` and creates no second head. +- `ReviewPolicy` adds nullable positive integer + `review_preference_window_seconds` and `review_lease_duration_seconds`, + non-null integer `max_active_review_leases_per_reviewer`, non-null boolean + `self_review_allowed`, non-null varchar(30) `reject_policy`, non-null + varchar(30) `finding_evidence_requirement`, non-null boolean + `legacy_incomplete`, nullable varchar(100) `configured_by_actor`, and nullable + timestamptz `configured_at`. +- A non-legacy ReviewPolicy requires both positive durations, capacity exactly + `1`, self-review exactly `false`, reject policy exactly `close_task`, evidence + policy exactly `optional`, `required_for_blocking`, or `required_for_all`, + decisions exactly the ordered JSON array `accept`, `needs_revision`, + `reject`, required finding fields exactly the ordered JSON array + `description`, `severity`, and non-empty configuring actor/time provenance. + `finding_evidence_requirement` defaults to `optional` when omitted while + accepting either other canonical token explicitly. Finding severity + semantics are only `blocking` and `advisory` and are documented here for the + later ReviewFinding owner; this chunk persists no finding. +- Existing ReviewPolicy rows are marked `legacy_incomplete=true`, receive no + invented duration or actor, and preserve retired values losslessly by + renaming `requires_second_review` to `legacy_requires_second_review` and + `sla_hours` to `legacy_sla_hours`. New rows are database-constrained to + `legacy_incomplete=false` and null archival fields. Only migration code may + create a legacy row: the database role loses no runtime bypass because the + trigger rejects inserts claiming `legacy_incomplete=true` after migration. +- `RevisionPolicy` retains positive integer `max_revision_rounds` and + `revision_deadline_hours`, and adds non-null `legacy_incomplete`, nullable + `configured_by_actor`, and nullable `configured_at`. Existing rows preserve + retired values losslessly as `legacy_auto_reject_after_limit`, + `legacy_allowed_resubmission_states`, and + `legacy_reviewer_reassignment_rule`; new rows require those archival fields + to be null. The runtime/API no longer accepts or exposes them. Exhaustion + cannot synthesize a Review or reject. +- All five renamed archival columns are database/migration-only: request schemas + reject them and responses never expose them. Responses expose + `legacy_incomplete`; legacy ReviewPolicy durations/configuration provenance + are nullable, while legacy RevisionPolicy retains its positive active + limit/deadline plus nullable configuration provenance. Project create/update + and active/detail reads never reinterpret an archival value as active policy. +- The migration adds these exact checks: + `ck_review_policies_fixed_v01`, `ck_review_policies_decisions_v01`, + `ck_review_policies_finding_fields_v01`, + `ck_review_policies_evidence_requirement`, + `ck_review_policies_complete_or_legacy`, + `ck_review_policies_archival_shape`, + `ck_revision_policies_positive_limits`, + `ck_revision_policies_complete_or_legacy`, and + `ck_revision_policies_archival_shape`. The JSON checks require the exact + canonical arrays, thereby rejecting missing, extra, duplicated, reordered, + null, blank, number, or object values. Direct SQL cannot insert an incomplete + new policy or arbitrary decision/state token. +- No index, unique constraint, or foreign key changes. Existing project/guide + uniqueness and composite guide-context foreign keys remain authoritative. +- Table-typed trigger functions `guard_review_policy_write` and + `guard_revision_policy_write` plus triggers + `trg_review_policies_guard_write` and + `trg_revision_policies_guard_write` lock the matching `project_guides` row + `FOR UPDATE` for insert and update. Those writes are permitted only while the + guide remains an unactivated draft (`status='draft'`, `effective_at is null`, + and `superseded_at is null`). Delete is rejected before dereferencing `NEW` + and therefore never waits on guide state. +- Statement-level `BEFORE TRUNCATE` triggers + `trg_review_policies_reject_truncate` and + `trg_revision_policies_reject_truncate` use those table-typed functions to + reject bulk erasure before dereferencing `NEW`; downgrade removes both + truncate triggers before dropping their functions. Direct-SQL proof must + exercise the rejection independently for each policy table. +- Policy `id`, `project_id`, `guide_version`, and `created_at` are immutable from + insert. Draft replacement updates the existing row only and may change only + canonical policy fields plus configuring provenance. The trigger overwrites + `configured_at` with database transaction time. It rejects moving a policy + between guide/project contexts or editing archival fields independently. + This consumes guide state read-only and changes no Guide route, activation + rule, state, or response. +- A legacy draft may convert exactly once and atomically to a complete policy: + `legacy_incomplete` becomes false, every canonical field becomes complete, + all archival fields become null, and verified configuring actor/database time + become non-null. Partial conversion, conversion after publication, or a + complete-to-legacy transition is rejected. This deliberate replacement makes + downgrade refuse because the original archival truth no longer exists. +- A migrated row is fully immutable while `legacy_incomplete` remains true. + The triggers reject every legacy-to-legacy update, including ReviewPolicy + decision/finding/fixed/evidence changes and RevisionPolicy round/deadline + changes. Direct-SQL negatives prove both tables refuse those writes and that + untouched legacy rows downgrade to their exact originals. +- Draft replacement updates `configured_by_actor` and `configured_at` to the + verified Flow `ActorContext.actor_id` and database time. Current project setup + does not expose canonical `actor_profile_id`; 03P records this exact upstream + attribution gap and does not add an AUTH lookup, compatibility path, or + ActorProfile lifecycle rule. +- Two-session PostgreSQL tests exercise ReviewPolicy and RevisionPolicy insert + and update against direct guide publication in both lock orders. The + guide-row lock must serialize the operations: either the draft policy write + commits before publication, or the post-publication write is rejected. Direct + delete refusal is proved independently for both tables because delete never + waits on guide state. +- Downgrade is lossless only for migration-existing legacy rows. It locks + `project_guides`, `review_policies`, then `revision_policies` in that exact + order with `ACCESS EXCLUSIVE`, and refuses before DDL if any non-legacy policy exists, + any archival field/provenance invariant drifted, or any protected row cannot + reconstruct the exact 0033 columns. Refusal preserves schema, head, and data; + clean prior-head upgrade/downgrade/re-upgrade restores every legacy value. - Missing upstream Task/Assignment compatibility is reported to its owner and cannot be repaired in this chunk. +- 03P does not change Project Guide activation. Legacy policy completeness is + exposed deterministically for the later REV queue-admission owner to reject; + no activation test may assert new behavior. - No review lifecycle transition is activated. ## Verification -Fix exact commands at start: focused model/migration tests, PostgreSQL -immutability/refusal proof, downgrade/re-upgrade proof, agent gates, and full -coverage through GitHub Actions. +```text +cd backend && .venv/bin/alembic heads +cd backend && .venv/bin/pytest -q tests/test_alembic.py -k review_revision_policy +cd backend && .venv/bin/pytest -q tests/test_projects.py -k 'review_policy or revision_policy' +cd backend && .venv/bin/pytest -q tests/test_tasks.py -k 'published_review_policy or stamped_policy_values' +cd backend && .venv/bin/pytest -q tests/test_artifact_admission.py::test_committed_put_and_independent_verification_are_fenced +cd backend && .venv/bin/ruff check app/modules/projects tests/conftest.py tests/test_alembic.py tests/test_projects.py tests/test_tasks.py tests/test_artifact_admission.py alembic/versions/0034_review_revision_policy_persistence.py +python3 scripts/check_stale_workstream_wording.py +python3 scripts/check_markdown_links.py +PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 backend/.venv/bin/python scripts/test_agent_gates.py +git diff --check +``` + +Migration proof must cover prior-head upgrade, all direct-SQL constraints, +legacy-incomplete preservation, update/delete refusal, both-order independent +transaction publication races, every downgrade refusal predicate, lossless +clean-head downgrade/re-upgrade, and restoration of the sole head. +The full backend suite and repository/subsystem coverage gates run in GitHub +Actions, not locally. + +The migration, application mapping, direct-SQL/concurrency proof, and active +contract documentation form one atomic L1 review unit. Splitting them would +leave either an unproved database boundary or application models incompatible +with the sole Alembic head. Reviewers use those four sections as explicit human +focus areas even when the resulting safety proof exceeds the preferred soft +diff size. ## Required reviewers @@ -49,4 +231,5 @@ reuse/dedup, docs, test-delta, and CI integrity. ## Stop -Do not implement without a signed start on exact current main. +After focused proof, required internal reviews, trust-bundle publication, and +PR creation, stop. Do not begin 03A. diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-external-review-response.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-external-review-response.md new file mode 100644 index 00000000..91b94f13 --- /dev/null +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-external-review-response.md @@ -0,0 +1,70 @@ +# External Review Response: WS-REV-001-03P + +## Comments addressed + +- GitHub Backend run `30025075528`, job `89267629866`, failed API E2E because + `guide_payload()` still sent five retired policy fields, used obsolete finding + names, and omitted the two canonical review durations. +- A prospectively reviewed scope amendment authorized only the policy request + fixture. Candidate `b68a1e22b3bf373d15479784d545fcc9b5737f64` + removes those retired inputs, uses `description` and `severity`, and supplies + 900-second preference and 1800-second lease durations. +- Shard 1 jobs exposed six stale current-head/atomic-downgrade expectations in + `tests/test_alembic.py`. They now expect sole head + `0034_review_revision_policy`; historical downgrade targets are unchanged. +- Shard 3 job `89267629913` exposed an ART proof fixture that inserted immutable + policies after guide publication. After prospective scope review, the same + guide is constructed draft, the same policies are flushed, and its prior + active/approval/effective values are assigned before Task, Submission, or + CheckerRun setup. Every ART assertion and final fixture fact is unchanged. +- Aggregate `test` failed only because API E2E and shards 1 and 3 failed. + Shards 2 and 4 passed. +- CodeRabbit run `9747dead-4c6c-4474-a989-4035a1292ec3` found three valid + current-head issues. The evidence and status artifacts now bind their + reviewed and post-repair candidates consistently, and migration 0034 now + rejects `TRUNCATE` on both immutable policy tables with statement-level + triggers that downgrade removes explicitly. The migration regression test + exercises both rejection paths. +- Backend run `30029324884`, shard 4, rejected the intentionally changed public + schema fingerprint after the authorized truncate triggers and their reused + guard-function definitions changed. The + proof-only `tests/conftest.py` constant now binds the observed deterministic + fingerprint `756d2c281c873d5b61e96d89e1f855d8e63fc8ed70380bf231c0325d437d7fe7`; + both policy tables use the existing guarded-reset path so test cleanup can + temporarily disable and immediately restore their user triggers. No reset + ownership check changed. +- Backend run `30029973617`, shard 1, showed PostgreSQL's foreign-key precheck + rejecting the single-table `TRUNCATE` test before the new trigger could fire. + Each independent negative now uses `TRUNCATE ... CASCADE`, which reaches the + intended table's `BEFORE TRUNCATE` guard and still rolls back without erasing + policy or related rows. + +## Comments deferred + +None. + +## Human decisions needed + +Only the normal explicit approval to merge PR #195 after every current-head +external check passes. The repair does not start 03A. + +## Commands rerun + +- Ruff on the API fixture and both affected test files: PASS. +- Four exact failed Alembic tests: PASS in 341.07 seconds. +- Exact failed ART test: PASS in 6.71 seconds. +- `git diff --check`: PASS. +- Senior/architecture/reuse, QA/product/test-delta, and security/docs/CI + exact-SHA review: PASS. +- Fresh API E2E, all shards, aggregate test, and coverage evidence: pending + GitHub Actions after push. +- Ruff and `git diff --check` for the CodeRabbit repair: PASS. The exact + database-backed migration test could not start locally because + `WORKSTREAM_TEST_DATABASE_URL` is unset; it remains required in GitHub + Actions rather than substituting a weaker local database. + +## Remaining risks + +The CodeRabbit repair requires current-head GitHub Actions and incremental +CodeRabbit confirmation after push. No lifecycle, authorization, or ART +assertion was changed to obtain the repairs. diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-internal-review-evidence.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-internal-review-evidence.md new file mode 100644 index 00000000..c7cb16c1 --- /dev/null +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-internal-review-evidence.md @@ -0,0 +1,70 @@ +# Internal Review Evidence: WS-REV-001-03P + +## Candidate + +- Trusted base: `bcf1292e1a591e3e84bf8ee212ee7191d80741fa` +- Final reviewed code candidate: `f19b2d798e49e7c3ebb05a19eaa53638c61237a1` +- Pre-external-repair implementation candidate: `35531df254c6b25726d666a5e89eda997b97d792` +- Signed start: workflow run `30014647556` +- Risk: L1 / P2 +- Boundary: ReviewPolicy and RevisionPolicy persistence only + +Open sub-agent sessions: none + +Valid findings addressed: yes + +## Reviewed Revision + +Reviewed code SHA: `f19b2d798e49e7c3ebb05a19eaa53638c61237a1` + +Reviewed at: 2026-07-23T17:19:00Z + +Reviewer run IDs: /root/plan_arch_review@f19b2d798e49e7c3ebb05a19eaa53638c61237a1; /root/qa_product_review@f19b2d798e49e7c3ebb05a19eaa53638c61237a1; /root/security_docs_ci_review@f19b2d798e49e7c3ebb05a19eaa53638c61237a1 + +## Reviewer results + +| Reviewer | Result | Blocking findings | Notes | +|---|---:|---|---| +| Senior engineering | PASS | None | Maintainable table-specific guards and bounded atomic L1 scope. | +| QA/test | PASS | None | Focused migration, API, snapshot, and compatibility proof is complete. | +| Security/auth | PASS | None | Provenance and immutability fail closed without absorbing AUTH ownership. | +| Product/ops | PASS | None | Policy persistence remains separate from later lifecycle execution. | +| Architecture | PASS | None | No Project Guide, Task, Submission, ART, AUTH, or CON boundary drift. | +| CI integrity | PASS | None | No workflow, coverage floor, script, skip, or assertion was weakened. | +| Docs | PASS | None | Active docs, schema, status, and lifecycle terminology agree. | +| Reuse/dedup | PASS | None | Existing Project policy mapping and locking conventions are reused. | +| Test delta | PASS | None | Compensation and ART assertions remain intact; proof only increased. | + +No Critical, High, Medium, or Low finding remains. All reviewer sessions +completed before publication. + +## Deterministic evidence + +- Alembic sole head: `0034_review_revision_policy`. +- Migration/direct-SQL/lossless downgrade test: PASS, including both policy + tables, exact archival reconstruction, insert/update/delete refusal, and + legacy-to-canonical conversion. +- Publication race test: PASS for review/revision, insert/update, and both lock + orders; every second backend was observed waiting on a PostgreSQL lock before + the first transaction released it. +- Project policy selector: PASS, 13 tests; migrated missing, empty, invalid, and + valid resubmission-state outcomes are preserved and all five archives remain + private. +- Task policy selector: PASS, 2 tests; immutable review/revision snapshots and + all compensation assertions remain intact. +- Artifact compatibility node: PASS, 1 test; no ART assertion changed. +- Ruff: PASS for every changed backend/test/migration path. +- Stale wording and Markdown links: PASS. +- Agent gates: PASS, 100 tests. +- `git diff --check`: PASS. + +The full backend suite and repository/subsystem coverage gates were not run +locally. They must run in GitHub Actions per the user's scaling instruction. + +## Scope and disposition + +No Task lifecycle, Project Guide behavior, Submission, checker, AUTH, ART, CON, +queue, lease, Review decision, revision execution, FinalAcceptance, or +contribution implementation was added. The reviewed atomic L1 size exception +is documented in the chunk contract. PASS for publication after the +evidence-only delta is reviewed; 03A remains stopped. diff --git a/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-pr-trust-bundle.md b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-pr-trust-bundle.md new file mode 100644 index 00000000..f1980d81 --- /dev/null +++ b/.agent-loop/initiatives/WS-REV-001-review-revision-lifecycle/reviews/WS-REV-001-03P-pr-trust-bundle.md @@ -0,0 +1,109 @@ +# PR Trust Bundle: WS-REV-001-03P + +## Chunk + +`WS-REV-001-03P` - Review And Revision Policy Persistence. + +## Goal + +Persist the immutable REV-owned policy facts required by later review routing, +leases, decisions, and human revision work, without implementing those later +lifecycle stages. + +## Human-approved intent + +REV starts only after `allow_review`, consumes the existing Submission and +artifact set, records complete traceable review/revision history in later +chunks, and never absorbs Project Guide, Task, AUTH, ART, or CON ownership. + +## What changed and why + +Migration 0034 replaces ambiguous legacy policy columns with explicit review +preference/lease, finding-evidence, revision-limit/deadline, and provenance +facts. Legacy values remain private and losslessly downgradeable until one +atomic conversion. Models, schemas, repository/service mapping, active docs, +and focused tests now agree with that database contract. + +## Design chosen + +Separate table-specific triggers make policy rows immutable, serialize every +insert/update with the exact guide row, reject runtime legacy claims, and permit +only atomic legacy-to-canonical conversion. Downgrade locks all three involved +tables and refuses canonical facts or unreconstructible archives before DDL. + +## Alternatives rejected + +- No mutable published policy or delete path. +- No runtime fallback defaults for migrated incomplete rows. +- No combined queue, lease, Review, decision, revision, or contribution work. +- No AUTH-owned actor schema or ART/Submission behavior. +- No lossy downgrade. + +## Scope control and product behavior + +The change is limited to the contract's policy persistence, mapping, docs, and +proof-only compatibility exceptions. Canonical decisions remain exactly +`accept`, `needs_revision`, and `reject`. No review lifecycle transition is +activated by this PR. + +## Acceptance criteria proof + +Direct SQL proves canonical shape, provenance, private archives, immutable +identity/context, update/delete refusal, runtime-insert denial, atomic +conversion, downgrade refusal, and lossless legacy round-trip. Eight real +PostgreSQL races prove both policy tables and both write operations serialize +with guide publication in both lock orders. + +## Tests/checks run + +Focused migration, Project, Task, and artifact tests passed. Alembic has one +head, Ruff passed all changed backend paths, stale wording and Markdown links +passed, 100 agent-gate tests passed, and diff integrity passed. Full backend +tests and coverage are delegated to GitHub Actions as instructed by the user. + +## Test delta and CI integrity + +Tests add constraints, downgrade, deterministic waiter, legacy compatibility, +archive privacy, and immutable snapshot proof. Existing compensation and ART +assertions remain intact. No workflow, package script, skip, xfail, coverage +floor, or assertion was weakened; CI must preserve 78 percent repository-wide +coverage and prove at least 90 percent for the materially changed subsystem. + +## Reviewer results + +Senior engineering, QA/test, security/auth, product/ops, architecture, docs, +reuse/dedup, test delta, CI integrity, and circuit breaker all PASS on code +candidate `b9501b31565370306aea168ff4c218a2880e6c0b`. The earlier +`35531df254c6b25726d666a5e89eda997b97d792` candidate predates external CI +fixture repairs. + +## External review + +PR #195's first run passed preflight, Agent Gates, and shards 2 and 4. It failed +API E2E, shards 1 and 3, and therefore the aggregate test gate. Repairs align +the API policy request fixture, six migration-head expectations, and one ART +fixture's policy-before-publication ordering without changing assertions or +product behavior. Focused failing tests, Ruff, diff integrity, and exact-SHA +internal review pass. CodeRabbit's green context explicitly said it could not +start a review because its review limit was reached; it is not review evidence. +Fresh current-head GitHub Actions and an actual CodeRabbit review remain +required before merge. + +## Remaining risks and follow-up work + +Later queue, lease, Review chain, decision, revision, FinalAcceptance, and CON +composition remain deliberately unimplemented. After merge, loop memory may +name same-initiative successor 03A, but 03A requires a separate explicit signed +start and a fresh current-main dependency check. + +## Human review focus + +Review the trigger invariants, downgrade refusal/reconstruction, eight race +cases, legacy API privacy, strict REV boundary, and preservation of Task and +ART compatibility assertions. + +## Human merge ownership + +Only the user may approve and merge this specific PR. Do not merge with a +pending/failed current-head GitHub or CodeRabbit check or an unresolved +actionable comment. Merge does not start 03A. diff --git a/.agent-loop/merge-intents/WS-REV-001-03P.json b/.agent-loop/merge-intents/WS-REV-001-03P.json new file mode 100644 index 00000000..e8cc629c --- /dev/null +++ b/.agent-loop/merge-intents/WS-REV-001-03P.json @@ -0,0 +1,9 @@ +{ + "chunk_id": "WS-REV-001-03P", + "chunk_title": "Review And Revision Policy Persistence", + "initiative_id": "WS-REV-001", + "next_chunk_id": "WS-REV-001-03A", + "next_chunk_title": "Queue And Lease Base Persistence", + "next_requires_explicit_start": true, + "schema_version": 2 +} diff --git a/backend/alembic/versions/0034_review_revision_policy_persistence.py b/backend/alembic/versions/0034_review_revision_policy_persistence.py new file mode 100644 index 00000000..b63e1ae9 --- /dev/null +++ b/backend/alembic/versions/0034_review_revision_policy_persistence.py @@ -0,0 +1,398 @@ +"""make review and revision policy persistence explicit and immutable + +Revision ID: 0034_review_revision_policy +Revises: 0033_authorization_read_rate +Create Date: 2026-07-23 +""" + +from __future__ import annotations + +from alembic import op +import sqlalchemy as sa + + +revision = "0034_review_revision_policy" +down_revision = "0033_authorization_read_rate" +branch_labels = depends_on = None + + +def _lock_policy_context() -> None: + for table in ("project_guides", "review_policies", "revision_policies"): + op.execute(sa.text(f"lock table {table} in access exclusive mode")) + + +def _create_review_columns() -> None: + op.alter_column( + "review_policies", + "requires_second_review", + new_column_name="legacy_requires_second_review", + nullable=True, + ) + op.alter_column("review_policies", "sla_hours", new_column_name="legacy_sla_hours") + op.add_column( + "review_policies", sa.Column("review_preference_window_seconds", sa.Integer()) + ) + op.add_column( + "review_policies", sa.Column("review_lease_duration_seconds", sa.Integer()) + ) + op.add_column( + "review_policies", + sa.Column( + "max_active_review_leases_per_reviewer", + sa.Integer(), + nullable=False, + server_default="1", + ), + ) + op.add_column( + "review_policies", + sa.Column("self_review_allowed", sa.Boolean(), nullable=False, server_default=sa.false()), + ) + op.add_column( + "review_policies", + sa.Column("reject_policy", sa.String(30), nullable=False, server_default="close_task"), + ) + op.add_column( + "review_policies", + sa.Column( + "finding_evidence_requirement", + sa.String(30), + nullable=False, + server_default="optional", + ), + ) + op.add_column( + "review_policies", + sa.Column("legacy_incomplete", sa.Boolean(), nullable=False, server_default=sa.true()), + ) + op.add_column("review_policies", sa.Column("configured_by_actor", sa.String(100))) + op.add_column( + "review_policies", sa.Column("configured_at", sa.DateTime(timezone=True)) + ) + op.alter_column("review_policies", "legacy_incomplete", server_default=sa.false()) + + +def _create_revision_columns() -> None: + op.alter_column( + "revision_policies", + "auto_reject_after_limit", + new_column_name="legacy_auto_reject_after_limit", + nullable=True, + ) + op.alter_column( + "revision_policies", + "allowed_resubmission_states", + new_column_name="legacy_allowed_resubmission_states", + nullable=True, + ) + op.alter_column( + "revision_policies", + "reviewer_reassignment_rule", + new_column_name="legacy_reviewer_reassignment_rule", + ) + op.add_column( + "revision_policies", + sa.Column("legacy_incomplete", sa.Boolean(), nullable=False, server_default=sa.true()), + ) + op.add_column("revision_policies", sa.Column("configured_by_actor", sa.String(100))) + op.add_column( + "revision_policies", sa.Column("configured_at", sa.DateTime(timezone=True)) + ) + op.alter_column("revision_policies", "legacy_incomplete", server_default=sa.false()) + + +def _create_constraints() -> None: + op.create_check_constraint( + "ck_review_policies_fixed_v01", + "review_policies", + "legacy_incomplete or (max_active_review_leases_per_reviewer = 1 and " + "self_review_allowed = false and reject_policy = 'close_task')", + ) + op.create_check_constraint( + "ck_review_policies_decisions_v01", + "review_policies", + "legacy_incomplete or allowed_decisions::jsonb = " + "'[\"accept\",\"needs_revision\",\"reject\"]'::jsonb", + ) + op.create_check_constraint( + "ck_review_policies_finding_fields_v01", + "review_policies", + "legacy_incomplete or minimum_finding_fields::jsonb = " + "'[\"description\",\"severity\"]'::jsonb", + ) + op.create_check_constraint( + "ck_review_policies_evidence_requirement", + "review_policies", + "legacy_incomplete or finding_evidence_requirement in " + "('optional','required_for_blocking','required_for_all')", + ) + op.create_check_constraint( + "ck_review_policies_complete_or_legacy", + "review_policies", + "legacy_incomplete or (review_preference_window_seconds > 0 and " + "review_lease_duration_seconds > 0 and configured_by_actor is not null and " + "btrim(configured_by_actor) <> '' and configured_at is not null)", + ) + op.create_check_constraint( + "ck_review_policies_archival_shape", + "review_policies", + "(legacy_incomplete and review_preference_window_seconds is null and " + "review_lease_duration_seconds is null and configured_by_actor is null and " + "configured_at is null) or (not legacy_incomplete and " + "legacy_requires_second_review is null and legacy_sla_hours is null)", + ) + op.create_check_constraint( + "ck_revision_policies_positive_limits", + "revision_policies", + "max_revision_rounds > 0 and revision_deadline_hours > 0", + ) + op.create_check_constraint( + "ck_revision_policies_complete_or_legacy", + "revision_policies", + "legacy_incomplete or (configured_by_actor is not null and " + "btrim(configured_by_actor) <> '' and configured_at is not null)", + ) + op.create_check_constraint( + "ck_revision_policies_archival_shape", + "revision_policies", + "(legacy_incomplete and configured_by_actor is null and configured_at is null) " + "or (not legacy_incomplete and legacy_auto_reject_after_limit is null and " + "legacy_allowed_resubmission_states is null and " + "legacy_reviewer_reassignment_rule is null)", + ) + + +def _create_write_guards() -> None: + common_prefix = """ + declare + guide_status varchar(30); + guide_effective_at timestamptz; + guide_superseded_at timestamptz; + begin + if tg_op = 'TRUNCATE' then + raise exception 'review and revision policies cannot be truncated' + using errcode = '23514'; + end if; + if tg_op = 'DELETE' then + raise exception 'review and revision policies cannot be deleted' + using errcode = '23514'; + end if; + select status, effective_at, superseded_at + into guide_status, guide_effective_at, guide_superseded_at + from project_guides + where project_id = new.project_id and version = new.guide_version + for update; + if not found then + raise exception 'policy guide context does not exist' using errcode = '23503'; + end if; + if guide_status <> 'draft' or guide_effective_at is not null + or guide_superseded_at is not null then + raise exception 'published review and revision policies are immutable' + using errcode = '23514'; + end if; + if tg_op = 'INSERT' and new.legacy_incomplete then + raise exception 'new policies cannot claim legacy state' using errcode = '23514'; + end if; + if tg_op = 'UPDATE' then + if (new.id, new.project_id, new.guide_version, new.created_at) + is distinct from + (old.id, old.project_id, old.guide_version, old.created_at) then + raise exception 'policy identity and context are immutable' + using errcode = '23514'; + end if; + if not old.legacy_incomplete and new.legacy_incomplete then + raise exception 'complete policy cannot become legacy' using errcode = '23514'; + end if; + if old.legacy_incomplete and new.legacy_incomplete then + raise exception 'legacy policy is immutable until atomic conversion' + using errcode = '23514'; + end if; + """ + review_suffix = """ + if old.legacy_incomplete and not new.legacy_incomplete then + if new.legacy_requires_second_review is not null + or new.legacy_sla_hours is not null then + raise exception 'legacy review policy conversion must clear archives' + using errcode = '23514'; + end if; + elsif (new.legacy_requires_second_review, new.legacy_sla_hours) + is distinct from + (old.legacy_requires_second_review, old.legacy_sla_hours) then + raise exception 'review policy archives are immutable' using errcode = '23514'; + end if; + end if; + if not new.legacy_incomplete then + new.configured_at := transaction_timestamp(); + end if; + return new; + end + """ + revision_suffix = """ + if old.legacy_incomplete and not new.legacy_incomplete then + if new.legacy_auto_reject_after_limit is not null + or new.legacy_allowed_resubmission_states is not null + or new.legacy_reviewer_reassignment_rule is not null then + raise exception 'legacy revision policy conversion must clear archives' + using errcode = '23514'; + end if; + elsif (new.legacy_auto_reject_after_limit, + new.legacy_allowed_resubmission_states::jsonb, + new.legacy_reviewer_reassignment_rule) + is distinct from + (old.legacy_auto_reject_after_limit, + old.legacy_allowed_resubmission_states::jsonb, + old.legacy_reviewer_reassignment_rule) then + raise exception 'revision policy archives are immutable' using errcode = '23514'; + end if; + end if; + if not new.legacy_incomplete then + new.configured_at := transaction_timestamp(); + end if; + return new; + end + """ + op.execute( + sa.text( + "create function guard_review_policy_write() returns trigger language plpgsql as $$" + + common_prefix + + review_suffix + + "$$" + ) + ) + op.execute( + sa.text( + "create function guard_revision_policy_write() returns trigger language plpgsql as $$" + + common_prefix + + revision_suffix + + "$$" + ) + ) + op.execute( + sa.text( + "create trigger trg_review_policies_guard_write before insert or update or delete " + "on review_policies for each row execute function " + "guard_review_policy_write()" + ) + ) + op.execute( + sa.text( + "create trigger trg_revision_policies_guard_write before insert or update or delete " + "on revision_policies for each row execute function " + "guard_revision_policy_write()" + ) + ) + op.execute( + sa.text( + "create trigger trg_review_policies_reject_truncate before truncate " + "on review_policies for each statement execute function " + "guard_review_policy_write()" + ) + ) + op.execute( + sa.text( + "create trigger trg_revision_policies_reject_truncate before truncate " + "on revision_policies for each statement execute function " + "guard_revision_policy_write()" + ) + ) + + +def upgrade() -> None: + """Preserve legacy facts and install the canonical policy boundary.""" + _lock_policy_context() + _create_review_columns() + _create_revision_columns() + _create_constraints() + _create_write_guards() + + +def _require_lossless_downgrade() -> None: + bind = op.get_bind() + unsafe_review = bind.execute( + sa.text( + "select exists(select 1 from review_policies where not legacy_incomplete " + "or review_preference_window_seconds is not null " + "or review_lease_duration_seconds is not null or configured_by_actor is not null " + "or configured_at is not null or legacy_requires_second_review is null)" + ) + ).scalar_one() + unsafe_revision = bind.execute( + sa.text( + "select exists(select 1 from revision_policies where not legacy_incomplete " + "or configured_by_actor is not null or configured_at is not null " + "or legacy_auto_reject_after_limit is null " + "or legacy_allowed_resubmission_states is null)" + ) + ).scalar_one() + if unsafe_review or unsafe_revision: + raise RuntimeError("cannot downgrade canonical review or revision policy facts") + + +def downgrade() -> None: + """Restore 0033 only when every policy is an untouched migrated legacy row.""" + _lock_policy_context() + _require_lossless_downgrade() + op.execute( + sa.text("drop trigger trg_revision_policies_reject_truncate on revision_policies") + ) + op.execute(sa.text("drop trigger trg_review_policies_reject_truncate on review_policies")) + op.execute(sa.text("drop trigger trg_revision_policies_guard_write on revision_policies")) + op.execute(sa.text("drop trigger trg_review_policies_guard_write on review_policies")) + op.execute(sa.text("drop function guard_revision_policy_write()")) + op.execute(sa.text("drop function guard_review_policy_write()")) + + for name in ( + "ck_revision_policies_archival_shape", + "ck_revision_policies_complete_or_legacy", + "ck_revision_policies_positive_limits", + ): + op.drop_constraint(name, "revision_policies", type_="check") + for name in ( + "ck_review_policies_archival_shape", + "ck_review_policies_complete_or_legacy", + "ck_review_policies_evidence_requirement", + "ck_review_policies_finding_fields_v01", + "ck_review_policies_decisions_v01", + "ck_review_policies_fixed_v01", + ): + op.drop_constraint(name, "review_policies", type_="check") + + for column in ("configured_at", "configured_by_actor", "legacy_incomplete"): + op.drop_column("revision_policies", column) + op.alter_column( + "revision_policies", + "legacy_reviewer_reassignment_rule", + new_column_name="reviewer_reassignment_rule", + ) + op.alter_column( + "revision_policies", + "legacy_allowed_resubmission_states", + new_column_name="allowed_resubmission_states", + nullable=False, + ) + op.alter_column( + "revision_policies", + "legacy_auto_reject_after_limit", + new_column_name="auto_reject_after_limit", + nullable=False, + ) + + for column in ( + "configured_at", + "configured_by_actor", + "legacy_incomplete", + "finding_evidence_requirement", + "reject_policy", + "self_review_allowed", + "max_active_review_leases_per_reviewer", + "review_lease_duration_seconds", + "review_preference_window_seconds", + ): + op.drop_column("review_policies", column) + op.alter_column("review_policies", "legacy_sla_hours", new_column_name="sla_hours") + op.alter_column( + "review_policies", + "legacy_requires_second_review", + new_column_name="requires_second_review", + nullable=False, + ) diff --git a/backend/app/modules/projects/models.py b/backend/app/modules/projects/models.py index 9a06c5f7..f6806b9b 100644 --- a/backend/app/modules/projects/models.py +++ b/backend/app/modules/projects/models.py @@ -217,15 +217,61 @@ class ReviewPolicy(Base): name="fk_review_policies_project_guide", ), UniqueConstraint("project_id", "guide_version", name="uq_review_policies_project_version"), + CheckConstraint( + "legacy_incomplete or (max_active_review_leases_per_reviewer = 1 and " + "self_review_allowed = false and reject_policy = 'close_task')", + name="fixed_v01", + ), + CheckConstraint( + "legacy_incomplete or allowed_decisions::jsonb = " + "'[\"accept\",\"needs_revision\",\"reject\"]'::jsonb", + name="decisions_v01", + ), + CheckConstraint( + "legacy_incomplete or minimum_finding_fields::jsonb = " + "'[\"description\",\"severity\"]'::jsonb", + name="finding_fields_v01", + ), + CheckConstraint( + "legacy_incomplete or finding_evidence_requirement in " + "('optional','required_for_blocking','required_for_all')", + name="evidence_requirement", + ), + CheckConstraint( + "legacy_incomplete or (review_preference_window_seconds > 0 and " + "review_lease_duration_seconds > 0 and configured_by_actor is not null and " + "btrim(configured_by_actor) <> '' and configured_at is not null)", + name="complete_or_legacy", + ), + CheckConstraint( + "(legacy_incomplete and review_preference_window_seconds is null and " + "review_lease_duration_seconds is null and configured_by_actor is null and " + "configured_at is null) or (not legacy_incomplete and " + "legacy_requires_second_review is null and legacy_sla_hours is null)", + name="archival_shape", + ), ) id: Mapped[str] = mapped_column(String(36), primary_key=True) project_id: Mapped[str] = mapped_column(ForeignKey("projects.id"), nullable=False, index=True) guide_version: Mapped[str] = mapped_column(String(50), nullable=False) - requires_second_review: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) allowed_decisions: Mapped[list[str]] = mapped_column(JSON, nullable=False, default=list) minimum_finding_fields: Mapped[list[str]] = mapped_column(JSON, nullable=False, default=list) - sla_hours: Mapped[int | None] = mapped_column(Integer) + review_preference_window_seconds: Mapped[int | None] = mapped_column(Integer) + review_lease_duration_seconds: Mapped[int | None] = mapped_column(Integer) + max_active_review_leases_per_reviewer: Mapped[int] = mapped_column(Integer, nullable=False, default=1) + self_review_allowed: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) + reject_policy: Mapped[str] = mapped_column(String(30), nullable=False, default="close_task") + finding_evidence_requirement: Mapped[str] = mapped_column( + String(30), nullable=False, default="optional" + ) + legacy_incomplete: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) + configured_by_actor: Mapped[str | None] = mapped_column(String(100)) + configured_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) + _legacy_requires_second_review: Mapped[bool | None] = mapped_column( + "legacy_requires_second_review", Boolean + ) + _legacy_sla_hours: Mapped[int | None] = mapped_column("legacy_sla_hours", Integer) created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now()) @@ -244,6 +290,22 @@ class RevisionPolicy(Base): "guide_version", name="uq_revision_policies_project_version", ), + CheckConstraint( + "max_revision_rounds > 0 and revision_deadline_hours > 0", + name="positive_limits", + ), + CheckConstraint( + "legacy_incomplete or (configured_by_actor is not null and " + "btrim(configured_by_actor) <> '' and configured_at is not null)", + name="complete_or_legacy", + ), + CheckConstraint( + "(legacy_incomplete and configured_by_actor is null and configured_at is null) " + "or (not legacy_incomplete and legacy_auto_reject_after_limit is null and " + "legacy_allowed_resubmission_states is null and " + "legacy_reviewer_reassignment_rule is null)", + name="archival_shape", + ), ) id: Mapped[str] = mapped_column(String(36), primary_key=True) @@ -251,13 +313,18 @@ class RevisionPolicy(Base): guide_version: Mapped[str] = mapped_column(String(50), nullable=False) max_revision_rounds: Mapped[int] = mapped_column(Integer, nullable=False) revision_deadline_hours: Mapped[int] = mapped_column(Integer, nullable=False) - auto_reject_after_limit: Mapped[bool] = mapped_column(Boolean, nullable=False, default=True) - allowed_resubmission_states: Mapped[list[str]] = mapped_column( - JSON, - nullable=False, - default=list, + legacy_incomplete: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) + configured_by_actor: Mapped[str | None] = mapped_column(String(100)) + configured_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) + _legacy_auto_reject_after_limit: Mapped[bool | None] = mapped_column( + "legacy_auto_reject_after_limit", Boolean + ) + _legacy_allowed_resubmission_states: Mapped[list[str] | None] = mapped_column( + "legacy_allowed_resubmission_states", JSON + ) + _legacy_reviewer_reassignment_rule: Mapped[str | None] = mapped_column( + "legacy_reviewer_reassignment_rule", Text ) - reviewer_reassignment_rule: Mapped[str | None] = mapped_column(Text) created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now()) diff --git a/backend/app/modules/projects/repository.py b/backend/app/modules/projects/repository.py index a35556c1..c383aab6 100644 --- a/backend/app/modules/projects/repository.py +++ b/backend/app/modules/projects/repository.py @@ -797,10 +797,21 @@ async def upsert_review_policy(self, policy: ReviewPolicy) -> ReviewPolicy: await self._session.flush() await self._session.refresh(policy) return policy - existing.requires_second_review = policy.requires_second_review existing.allowed_decisions = policy.allowed_decisions existing.minimum_finding_fields = policy.minimum_finding_fields - existing.sla_hours = policy.sla_hours + existing.review_preference_window_seconds = policy.review_preference_window_seconds + existing.review_lease_duration_seconds = policy.review_lease_duration_seconds + existing.max_active_review_leases_per_reviewer = ( + policy.max_active_review_leases_per_reviewer + ) + existing.self_review_allowed = policy.self_review_allowed + existing.reject_policy = policy.reject_policy + existing.finding_evidence_requirement = policy.finding_evidence_requirement + existing.legacy_incomplete = False + existing.configured_by_actor = policy.configured_by_actor + existing.configured_at = policy.configured_at + existing._legacy_requires_second_review = None + existing._legacy_sla_hours = None await self._session.flush() await self._session.refresh(existing) return existing @@ -840,9 +851,12 @@ async def upsert_revision_policy(self, policy: RevisionPolicy) -> RevisionPolicy return policy existing.max_revision_rounds = policy.max_revision_rounds existing.revision_deadline_hours = policy.revision_deadline_hours - existing.auto_reject_after_limit = policy.auto_reject_after_limit - existing.allowed_resubmission_states = policy.allowed_resubmission_states - existing.reviewer_reassignment_rule = policy.reviewer_reassignment_rule + existing.legacy_incomplete = False + existing.configured_by_actor = policy.configured_by_actor + existing.configured_at = policy.configured_at + existing._legacy_auto_reject_after_limit = None + existing._legacy_allowed_resubmission_states = None + existing._legacy_reviewer_reassignment_rule = None await self._session.flush() await self._session.refresh(existing) return existing diff --git a/backend/app/modules/projects/schemas.py b/backend/app/modules/projects/schemas.py index 433234ad..ffe307a3 100644 --- a/backend/app/modules/projects/schemas.py +++ b/backend/app/modules/projects/schemas.py @@ -14,12 +14,36 @@ class ReviewPolicyInput(BaseModel): model_config = ConfigDict(extra="forbid") - requires_second_review: bool = False allowed_decisions: list[Literal["accept", "needs_revision", "reject"]] = Field( default_factory=lambda: ["accept", "needs_revision", "reject"] ) - minimum_finding_fields: list[str] = Field(default_factory=list) - sla_hours: int | None = None + minimum_finding_fields: list[Literal["description", "severity"]] = Field( + default_factory=lambda: ["description", "severity"] + ) + review_preference_window_seconds: int = Field(ge=1) + review_lease_duration_seconds: int = Field(ge=1) + max_active_review_leases_per_reviewer: Literal[1] = 1 + self_review_allowed: Literal[False] = False + reject_policy: Literal["close_task"] = "close_task" + finding_evidence_requirement: Literal[ + "optional", "required_for_blocking", "required_for_all" + ] = "optional" + + @field_validator("allowed_decisions") + @classmethod + def decisions_are_canonical(cls, value: list[str]) -> list[str]: + """Require the single v0.1 product-decision sequence.""" + if value != ["accept", "needs_revision", "reject"]: + raise ValueError("allowed_decisions must be the canonical v0.1 sequence") + return value + + @field_validator("minimum_finding_fields") + @classmethod + def finding_fields_are_canonical(cls, value: list[str]) -> list[str]: + """Require the single v0.1 finding-field sequence.""" + if value != ["description", "severity"]: + raise ValueError("minimum_finding_fields must be the canonical v0.1 sequence") + return value class RevisionPolicyInput(BaseModel): @@ -29,9 +53,6 @@ class RevisionPolicyInput(BaseModel): max_revision_rounds: int = Field(ge=1) revision_deadline_hours: int = Field(ge=1) - auto_reject_after_limit: bool = True - allowed_resubmission_states: list[str] = Field(default_factory=lambda: ["needs_revision"]) - reviewer_reassignment_rule: str | None = None class PaymentPolicyInput(BaseModel): @@ -569,10 +590,17 @@ class ReviewPolicyResponse(BaseModel): id: str project_id: str guide_version: str - requires_second_review: bool allowed_decisions: list[str] minimum_finding_fields: list[str] - sla_hours: int | None + review_preference_window_seconds: int | None + review_lease_duration_seconds: int | None + max_active_review_leases_per_reviewer: int + self_review_allowed: bool + reject_policy: str + finding_evidence_requirement: str + legacy_incomplete: bool + configured_by_actor: str | None + configured_at: datetime | None created_at: datetime @@ -586,9 +614,9 @@ class RevisionPolicyResponse(BaseModel): guide_version: str max_revision_rounds: int revision_deadline_hours: int - auto_reject_after_limit: bool - allowed_resubmission_states: list[str] - reviewer_reassignment_rule: str | None + legacy_incomplete: bool + configured_by_actor: str | None + configured_at: datetime | None created_at: datetime diff --git a/backend/app/modules/projects/service.py b/backend/app/modules/projects/service.py index 4fbbb9a7..194bd17f 100644 --- a/backend/app/modules/projects/service.py +++ b/backend/app/modules/projects/service.py @@ -559,7 +559,7 @@ async def create_guide( except IntegrityError as exc: await self._session.rollback() raise GuideVersionConflict("guide version already exists for project") from exc - await self._upsert_optional_policies(project_id, payload.version, payload) + await self._upsert_optional_policies(actor, project_id, payload.version, payload) if ( source_snapshot_payload is not None or get_settings().project_setup_pipeline_autostart @@ -634,7 +634,7 @@ async def update_draft_guide( }: continue setattr(guide, field, value) - await self._upsert_optional_policies(project_id, guide.version, payload) + await self._upsert_optional_policies(actor, project_id, guide.version, payload) await self._session.commit() await self._session.refresh(guide) return ProjectGuideResponse.model_validate(guide) @@ -2157,6 +2157,7 @@ async def _lock_project_guide_for_setup( async def _upsert_optional_policies( self, + actor: ActorContext, project_id: str, guide_version: str, payload: ProjectGuideCreate | ProjectGuideUpdate, @@ -2164,17 +2165,18 @@ async def _upsert_optional_policies( """Create or replace policy records supplied with guide payloads. Args: + actor: Verified Flow actor configuring the policies. project_id: Project that owns the policies. guide_version: Guide version the policies apply to. payload: Guide create or update payload carrying optional policies. """ if payload.review_policy is not None: await self._repo.upsert_review_policy( - self._review_policy_model(project_id, guide_version, payload.review_policy) + self._review_policy_model(actor, project_id, guide_version, payload.review_policy) ) if payload.revision_policy is not None: await self._repo.upsert_revision_policy( - self._revision_policy_model(project_id, guide_version, payload.revision_policy) + self._revision_policy_model(actor, project_id, guide_version, payload.revision_policy) ) if payload.payment_policy is not None: await self._repo.upsert_payment_policy( @@ -4293,13 +4295,14 @@ def _validate_activation_ready( if ( revision_policy.max_revision_rounds < 1 or revision_policy.revision_deadline_hours < 1 - or not revision_policy.allowed_resubmission_states ): raise GuideActivationBlocked("revision policy is incomplete") - if not set(revision_policy.allowed_resubmission_states).issubset( - ALLOWED_REVISION_RESUBMISSION_STATES - ): - raise GuideActivationBlocked("revision policy contains invalid resubmission states") + if revision_policy.legacy_incomplete: + legacy_states = revision_policy._legacy_allowed_resubmission_states + if not legacy_states: + raise GuideActivationBlocked("revision policy is incomplete") + if not set(legacy_states).issubset(ALLOWED_REVISION_RESUBMISSION_STATES): + raise GuideActivationBlocked("revision policy contains invalid resubmission states") if payment_policy is None: raise GuideActivationBlocked("payment policy is required") if ( @@ -4329,6 +4332,7 @@ def _validate_sufficiency_warning_acknowledgement( def _review_policy_model( self, + actor: ActorContext, project_id: str, guide_version: str, payload: ReviewPolicyInput, @@ -4336,6 +4340,7 @@ def _review_policy_model( """Build a review policy model from API input. Args: + actor: Verified Flow actor configuring the policy. project_id: Project that owns the policy. guide_version: Guide version the policy applies to. payload: Validated review policy input. @@ -4347,14 +4352,22 @@ def _review_policy_model( id=str(uuid4()), project_id=project_id, guide_version=guide_version, - requires_second_review=payload.requires_second_review, allowed_decisions=payload.allowed_decisions, minimum_finding_fields=payload.minimum_finding_fields, - sla_hours=payload.sla_hours, + review_preference_window_seconds=payload.review_preference_window_seconds, + review_lease_duration_seconds=payload.review_lease_duration_seconds, + max_active_review_leases_per_reviewer=payload.max_active_review_leases_per_reviewer, + self_review_allowed=payload.self_review_allowed, + reject_policy=payload.reject_policy, + finding_evidence_requirement=payload.finding_evidence_requirement, + legacy_incomplete=False, + configured_by_actor=actor.actor_id, + configured_at=datetime.now(UTC), ) def _revision_policy_model( self, + actor: ActorContext, project_id: str, guide_version: str, payload: RevisionPolicyInput, @@ -4362,6 +4375,7 @@ def _revision_policy_model( """Build a revision policy model from API input. Args: + actor: Verified Flow actor configuring the policy. project_id: Project that owns the policy. guide_version: Guide version the policy applies to. payload: Validated revision policy input. @@ -4375,9 +4389,9 @@ def _revision_policy_model( guide_version=guide_version, max_revision_rounds=payload.max_revision_rounds, revision_deadline_hours=payload.revision_deadline_hours, - auto_reject_after_limit=payload.auto_reject_after_limit, - allowed_resubmission_states=payload.allowed_resubmission_states, - reviewer_reassignment_rule=payload.reviewer_reassignment_rule, + legacy_incomplete=False, + configured_by_actor=actor.actor_id, + configured_at=datetime.now(UTC), ) def _payment_policy_model( diff --git a/backend/scripts/api_contract_e2e.py b/backend/scripts/api_contract_e2e.py index 60dfc298..5dba947b 100644 --- a/backend/scripts/api_contract_e2e.py +++ b/backend/scripts/api_contract_e2e.py @@ -641,17 +641,14 @@ def guide_payload(run_id: str) -> dict: ), "change_summary": "Initial real API guide", "review_policy": { - "requires_second_review": False, "allowed_decisions": ["accept", "needs_revision", "reject"], - "minimum_finding_fields": ["issue", "required_fix"], - "sla_hours": 24, + "minimum_finding_fields": ["description", "severity"], + "review_preference_window_seconds": 900, + "review_lease_duration_seconds": 1800, }, "revision_policy": { "max_revision_rounds": 7, "revision_deadline_hours": 48, - "auto_reject_after_limit": True, - "allowed_resubmission_states": ["needs_revision"], - "reviewer_reassignment_rule": "same reviewer preferred", }, "payment_policy": { "base_amount": "25.00", diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 19f99f99..e0a0672a 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -22,7 +22,7 @@ DDL_LOCK_DIRECTORY = Path("/tmp") EXPECTED_PUBLIC_SCHEMA_SHA256 = ( - "57c0f4b00709fb1c865277d9cac07ea2bc34d22ef26f36f93a7214986cb1388b" + "756d2c281c873d5b61e96d89e1f855d8e63fc8ed70380bf231c0325d437d7fe7" ) PROTECTED_TEST_TABLES = ( "actor_profile_migration_state", @@ -84,6 +84,8 @@ "outbox_events", "project_role_grants", "project_role_qualification_snapshots", + "review_policies", + "revision_policies", ) TestDatabaseReset = Callable[..., Awaitable[None]] DatabaseLock = Callable[[], AbstractContextManager[None]] diff --git a/backend/tests/test_alembic.py b/backend/tests/test_alembic.py index 94ed6925..b6d2e2d4 100644 --- a/backend/tests/test_alembic.py +++ b/backend/tests/test_alembic.py @@ -135,7 +135,7 @@ def test_artifact_recovery_schema_and_empty_downgrade( command.downgrade(config, "base") command.upgrade(config, "head") assert asyncio.run(_artifact_recovery_schema(isolated_database_env)) == { - "revision": "0033_authorization_read_rate", + "revision": "0034_review_revision_policy", "constraints": { "artifact_recovery_attempt_custody", "artifact_verification_lineage_custody", @@ -201,7 +201,7 @@ def test_project_role_migration_constraints_and_immutable_history( command.upgrade(config, "head") result = asyncio.run(_exercise_project_role_migration(isolated_database_env)) assert result == { - "revision": "0033_authorization_read_rate", + "revision": "0034_review_revision_policy", "role_count": 3, "invalid_availability": "23514", "duplicate_role": "23505", @@ -409,7 +409,7 @@ def test_project_role_downgrade_refuses_each_reserved_evidence_predicate( ): command.downgrade(config, "0030_artifact_verification") assert asyncio.run(_project_role_refusal_state(isolated_database_env))[:3] == ( - "0033_authorization_read_rate", + "0034_review_revision_policy", True, True, ) @@ -436,7 +436,7 @@ def test_project_role_downgrade_refuses_each_reserved_evidence_predicate( ): command.downgrade(config, "0030_artifact_verification") assert asyncio.run(_project_role_refusal_state(isolated_database_env))[:3] == ( - "0033_authorization_read_rate", + "0034_review_revision_policy", True, True, ) @@ -464,7 +464,7 @@ def test_outbox_migration_schema_and_downgrade_writer_guard( command.upgrade(config, "head") schema = asyncio.run(_outbox_schema(isolated_database_env)) assert schema == { - "revision": "0033_authorization_read_rate", + "revision": "0034_review_revision_policy", "columns": { "aggregate_id", "aggregate_type", @@ -524,7 +524,7 @@ def test_outbox_migration_schema_and_downgrade_writer_guard( ) assert committed == "refused_after_commit" assert asyncio.run(_current_revision(isolated_database_env)) == ( - "0033_authorization_read_rate" + "0034_review_revision_policy" ) asyncio.run(_remove_outbox_migration_row(isolated_database_env, committed_project_id)) command.downgrade(config, "0028_artifact_admission") @@ -2745,6 +2745,456 @@ def guarded_downgrade() -> None: } +def test_review_revision_policy_migration_is_lossless_and_guarded( + isolated_database_env: str, + migration_lock, +) -> None: + """Prove 0034 preserves legacy truth and guards canonical policy facts.""" + config = _alembic_config() + project_id = str(uuid4()) + review_id = str(uuid4()) + revision_id = str(uuid4()) + + async def execute(statement: str, parameters: dict | None = None) -> None: + engine = create_async_engine(isolated_database_env) + try: + async with engine.begin() as connection: + await connection.execute(text(statement), parameters or {}) + finally: + await engine.dispose() + + async def row(table: str, row_id: str) -> dict: + engine = create_async_engine(isolated_database_env) + try: + async with engine.connect() as connection: + value = await connection.scalar( + text( + f"select row_to_json(selected) from " + f"(select * from {table} where id=:id) selected" + ), + {"id": row_id}, + ) + assert isinstance(value, dict) + return value + finally: + await engine.dispose() + + with migration_lock(): + try: + command.downgrade(config, "base") + command.upgrade(config, "0033_authorization_read_rate") + asyncio.run( + execute( + "insert into projects (id,name,slug,status) " + "values (:id,'Policy migration','policy-migration','draft')", + {"id": project_id}, + ) + ) + asyncio.run( + execute( + "insert into project_guides " + "(id,project_id,version,status,content_markdown,created_by) " + "values (:id,:project_id,'v1','draft','# Guide','actor-old')", + {"id": str(uuid4()), "project_id": project_id}, + ) + ) + asyncio.run( + execute( + "insert into review_policies " + "(id,project_id,guide_version,requires_second_review," + "allowed_decisions,minimum_finding_fields,sla_hours) values " + "(:id,:project_id,'v1',false," + "'[\"accept\",\"needs_revision\",\"reject\"]'::json," + "'[\"legacy_issue\"]'::json,24)", + {"id": review_id, "project_id": project_id}, + ) + ) + asyncio.run( + execute( + "insert into revision_policies " + "(id,project_id,guide_version,max_revision_rounds," + "revision_deadline_hours,auto_reject_after_limit," + "allowed_resubmission_states,reviewer_reassignment_rule) values " + "(:id,:project_id,'v1',7,48,true," + "'[\"needs_revision\"]'::json,'same reviewer preferred')", + {"id": revision_id, "project_id": project_id}, + ) + ) + old_review = asyncio.run(row("review_policies", review_id)) + old_revision = asyncio.run(row("revision_policies", revision_id)) + + command.upgrade(config, "0034_review_revision_policy") + migrated_review = asyncio.run(row("review_policies", review_id)) + migrated_revision = asyncio.run(row("revision_policies", revision_id)) + assert migrated_review["legacy_incomplete"] is True + assert migrated_review["review_preference_window_seconds"] is None + assert migrated_review["legacy_sla_hours"] == 24 + assert migrated_revision["legacy_incomplete"] is True + assert migrated_revision["legacy_auto_reject_after_limit"] is True + + for version in ("v2", "v3", "v4", "v5"): + asyncio.run( + execute( + "insert into project_guides " + "(id,project_id,version,status,content_markdown,created_by) " + "values (:id,:project_id,:version,'draft','# Guide','actor-old')", + { + "id": str(uuid4()), + "project_id": project_id, + "version": version, + }, + ) + ) + forbidden_inserts = ( + ( + "insert into review_policies " + "(id,project_id,guide_version,allowed_decisions,minimum_finding_fields," + "legacy_incomplete) values " + "(:id,:project_id,'v2','[\"accept\",\"needs_revision\",\"reject\"]'::json," + "'[\"description\",\"severity\"]'::json,true)", + "new policies cannot claim legacy state", + ), + ( + "insert into review_policies " + "(id,project_id,guide_version,allowed_decisions,minimum_finding_fields," + "legacy_incomplete) values " + "(:id,:project_id,'v3','[\"accept\",\"needs_revision\",\"reject\"]'::json," + "'[\"description\",\"severity\"]'::json,false)", + "ck_review_policies_complete_or_legacy", + ), + ( + "insert into revision_policies " + "(id,project_id,guide_version,max_revision_rounds," + "revision_deadline_hours,legacy_incomplete) " + "values (:id,:project_id,'v4',7,48,true)", + "new policies cannot claim legacy state", + ), + ( + "insert into revision_policies " + "(id,project_id,guide_version,max_revision_rounds," + "revision_deadline_hours,legacy_incomplete) " + "values (:id,:project_id,'v5',7,48,false)", + "ck_revision_policies_complete_or_legacy", + ), + ) + for statement, expected_error in forbidden_inserts: + with pytest.raises(IntegrityError, match=expected_error): + asyncio.run( + execute( + statement, + {"id": str(uuid4()), "project_id": project_id}, + ) + ) + + command.downgrade(config, "0033_authorization_read_rate") + assert asyncio.run(row("review_policies", review_id)) == old_review + assert asyncio.run(row("revision_policies", revision_id)) == old_revision + + command.upgrade(config, "0034_review_revision_policy") + with pytest.raises(IntegrityError, match="legacy policy is immutable"): + asyncio.run( + execute( + "update review_policies set allowed_decisions='[]'::json where id=:id", + {"id": review_id}, + ) + ) + with pytest.raises(IntegrityError, match="legacy policy is immutable"): + asyncio.run( + execute( + "update revision_policies set max_revision_rounds=8 where id=:id", + {"id": revision_id}, + ) + ) + for table, trigger, column, restore in ( + ( + "review_policies", + "trg_review_policies_guard_write", + "legacy_requires_second_review", + "false", + ), + ( + "revision_policies", + "trg_revision_policies_guard_write", + "legacy_auto_reject_after_limit", + "true", + ), + ): + asyncio.run(execute(f"alter table {table} disable trigger {trigger}")) + asyncio.run(execute(f"update {table} set {column}=null")) + asyncio.run(execute(f"alter table {table} enable trigger {trigger}")) + with pytest.raises( + RuntimeError, + match="cannot downgrade canonical review or revision policy facts", + ): + command.downgrade(config, "0033_authorization_read_rate") + asyncio.run(execute(f"alter table {table} disable trigger {trigger}")) + asyncio.run(execute(f"update {table} set {column}={restore}")) + asyncio.run(execute(f"alter table {table} enable trigger {trigger}")) + + asyncio.run( + execute( + "update review_policies set legacy_incomplete=false," + "legacy_requires_second_review=null,legacy_sla_hours=null," + "allowed_decisions='[\"accept\",\"needs_revision\",\"reject\"]'::json," + "minimum_finding_fields='[\"description\",\"severity\"]'::json," + "review_preference_window_seconds=900,review_lease_duration_seconds=1800," + "max_active_review_leases_per_reviewer=1,self_review_allowed=false," + "reject_policy='close_task',finding_evidence_requirement='optional'," + "configured_by_actor='actor-new' where id=:id", + {"id": review_id}, + ) + ) + converted = asyncio.run(row("review_policies", review_id)) + assert converted["legacy_incomplete"] is False + assert converted["configured_at"] is not None + invalid_finding_fields = ( + "[]", + '["description"]', + '["description","severity","extra"]', + '["severity","description"]', + '["description","severity","severity"]', + '["description","severity",null]', + '["description","severity",1]', + '["description","severity",{}]', + ) + for invalid_value in invalid_finding_fields: + with pytest.raises(IntegrityError): + asyncio.run( + execute( + "update review_policies set minimum_finding_fields=" + "cast(:value as json) where id=:id", + {"id": review_id, "value": invalid_value}, + ) + ) + with pytest.raises(IntegrityError): + asyncio.run( + execute( + "update review_policies set review_lease_duration_seconds=0 " + "where id=:id", + {"id": review_id}, + ) + ) + invalid_review_updates = ( + "allowed_decisions='[]'::json", + "max_active_review_leases_per_reviewer=2", + "self_review_allowed=true", + "reject_policy='reopen_to_pool'", + "finding_evidence_requirement='invalid'", + "configured_by_actor=''", + "legacy_incomplete=true", + ) + for mutation in invalid_review_updates: + with pytest.raises(IntegrityError): + asyncio.run( + execute( + f"update review_policies set {mutation} where id=:id", + {"id": review_id}, + ) + ) + asyncio.run( + execute( + "update revision_policies set legacy_incomplete=false," + "legacy_auto_reject_after_limit=null," + "legacy_allowed_resubmission_states=null," + "legacy_reviewer_reassignment_rule=null,configured_by_actor='actor-new' " + "where id=:id", + {"id": revision_id}, + ) + ) + for mutation in ("max_revision_rounds=0", "revision_deadline_hours=0"): + with pytest.raises(IntegrityError): + asyncio.run( + execute( + f"update revision_policies set {mutation} where id=:id", + {"id": revision_id}, + ) + ) + with pytest.raises( + RuntimeError, + match="cannot downgrade canonical review or revision policy facts", + ): + command.downgrade(config, "0033_authorization_read_rate") + assert asyncio.run(row("review_policies", review_id)) == converted + + asyncio.run( + execute( + "update project_guides set status='active',effective_at=statement_timestamp() " + "where project_id=:project_id and version='v1'", + {"project_id": project_id}, + ) + ) + with pytest.raises(IntegrityError, match="published review and revision policies"): + asyncio.run( + execute( + "update review_policies set review_lease_duration_seconds=1900 " + "where id=:id", + {"id": review_id}, + ) + ) + with pytest.raises(IntegrityError, match="cannot be deleted"): + asyncio.run( + execute("delete from review_policies where id=:id", {"id": review_id}) + ) + with pytest.raises(IntegrityError, match="cannot be deleted"): + asyncio.run( + execute("delete from revision_policies where id=:id", {"id": revision_id}) + ) + for table in ("review_policies", "revision_policies"): + with pytest.raises(IntegrityError, match="cannot be truncated"): + asyncio.run(execute(f"truncate table {table} cascade")) + finally: + command.upgrade(config, "head") + + +def test_review_revision_policy_writes_serialize_with_publication( + isolated_database_env: str, +) -> None: + """Prove both policy tables serialize inserts/updates in both lock orders.""" + + async def seed(marker: str, policy_kind: str, create_policy: bool) -> tuple[str, str]: + project_id = str(uuid4()) + policy_id = str(uuid4()) + engine = create_async_engine(isolated_database_env) + try: + async with engine.begin() as connection: + await connection.execute( + text( + "insert into projects (id,name,slug,status) " + "values (:id,:name,:slug,'draft')" + ), + {"id": project_id, "name": marker, "slug": marker}, + ) + await connection.execute( + text( + "insert into project_guides " + "(id,project_id,version,status,content_markdown,created_by) " + "values (:id,:project_id,'v1','draft','# Guide','actor')" + ), + {"id": str(uuid4()), "project_id": project_id}, + ) + if create_policy: + await connection.execute( + text(policy_insert_sql(policy_kind)), + {"id": policy_id, "project_id": project_id}, + ) + return project_id, policy_id + finally: + await engine.dispose() + + def policy_insert_sql(policy_kind: str) -> str: + if policy_kind == "review": + return ( + "insert into review_policies " + "(id,project_id,guide_version,allowed_decisions,minimum_finding_fields," + "review_preference_window_seconds,review_lease_duration_seconds," + "max_active_review_leases_per_reviewer,self_review_allowed,reject_policy," + "finding_evidence_requirement,legacy_incomplete,configured_by_actor) values " + "(:id,:project_id,'v1','[\"accept\",\"needs_revision\",\"reject\"]'::json," + "'[\"description\",\"severity\"]'::json,900,1800,1,false," + "'close_task','optional',false,'actor')" + ) + return ( + "insert into revision_policies " + "(id,project_id,guide_version,max_revision_rounds,revision_deadline_hours," + "legacy_incomplete,configured_by_actor) " + "values (:id,:project_id,'v1',7,48,false,'actor')" + ) + + def policy_write_sql(policy_kind: str, operation: str) -> str: + if operation == "insert": + return policy_insert_sql(policy_kind) + if policy_kind == "review": + return "update review_policies set review_lease_duration_seconds=1900 where id=:id" + return "update revision_policies set revision_deadline_hours=49 where id=:id" + + async def race(policy_kind: str, operation: str, policy_first: bool) -> None: + project_id, policy_id = await seed( + f"{policy_kind}-{operation}-{policy_first}-{uuid4()}", + policy_kind, + create_policy=operation == "update", + ) + engine = create_async_engine(isolated_database_env) + policy_connection = None + publication_connection = None + monitor_connection = None + try: + policy_connection = await engine.connect() + publication_connection = await engine.connect() + monitor_connection = await engine.connect() + policy_pid = await policy_connection.scalar(text("select pg_backend_pid()")) + publication_pid = await publication_connection.scalar( + text("select pg_backend_pid()") + ) + await policy_connection.commit() + await publication_connection.commit() + + async def require_lock_wait(pid: int) -> None: + for _ in range(100): + waiting = await monitor_connection.scalar( + text( + "select exists(select 1 from pg_stat_activity " + "where pid=:pid and state='active' and wait_event_type='Lock')" + ), + {"pid": pid}, + ) + if waiting: + return + await asyncio.sleep(0.01) + pytest.fail(f"backend {pid} never entered a database lock wait") + + publication_sql = text( + "update project_guides set status='active',effective_at=statement_timestamp() " + "where project_id=:project_id and version='v1'" + ) + policy_sql = text(policy_write_sql(policy_kind, operation)) + parameters = {"id": policy_id, "project_id": project_id} + if policy_first: + first_tx = await policy_connection.begin() + await policy_connection.execute(policy_sql, parameters) + + async def publish() -> None: + async with publication_connection.begin(): + await publication_connection.execute( + publication_sql, {"project_id": project_id} + ) + + second = asyncio.create_task(publish()) + await require_lock_wait(publication_pid) + assert not second.done() + await first_tx.commit() + await second + else: + first_tx = await publication_connection.begin() + await publication_connection.execute( + publication_sql, {"project_id": project_id} + ) + + async def write_policy() -> None: + async with policy_connection.begin(): + await policy_connection.execute(policy_sql, parameters) + + second = asyncio.create_task(write_policy()) + await require_lock_wait(policy_pid) + assert not second.done() + await first_tx.commit() + with pytest.raises(DBAPIError, match="published review and revision policies"): + await second + finally: + for connection in ( + monitor_connection, + publication_connection, + policy_connection, + ): + if connection is not None: + await connection.close() + await engine.dispose() + + for policy_kind in ("review", "revision"): + for operation in ("insert", "update"): + for policy_first in (True, False): + asyncio.run(race(policy_kind, operation, policy_first)) + + def test_authorization_read_rate_scope_upgrade_and_downgrade_refusal( isolated_database_env: str, migration_lock, diff --git a/backend/tests/test_artifact_admission.py b/backend/tests/test_artifact_admission.py index df781d35..f253237b 100644 --- a/backend/tests/test_artifact_admission.py +++ b/backend/tests/test_artifact_admission.py @@ -414,18 +414,15 @@ async def _seed_checker_output_relationships(session) -> tuple[str, str, str]: session.add(Project(id=project_id, name="Checker project", slug=f"checker-{project_id}")) await session.flush() - session.add( - ProjectGuide( - id=guide_id, - project_id=project_id, - version=guide_version, - status="active", - content_markdown="# Checker guide", - approved_by="setup-actor", - effective_at=now, - created_by="setup-actor", - ) + guide = ProjectGuide( + id=guide_id, + project_id=project_id, + version=guide_version, + status="draft", + content_markdown="# Checker guide", + created_by="setup-actor", ) + session.add(guide) await session.flush() session.add( GuideSourceSnapshot( @@ -527,9 +524,17 @@ async def _seed_checker_output_relationships(session) -> tuple[str, str, str]: id=str(uuid4()), project_id=project_id, guide_version=guide_version, - requires_second_review=False, allowed_decisions=["accept", "needs_revision", "reject"], - minimum_finding_fields=[], + minimum_finding_fields=["description", "severity"], + review_preference_window_seconds=900, + review_lease_duration_seconds=1800, + max_active_review_leases_per_reviewer=1, + self_review_allowed=False, + reject_policy="close_task", + finding_evidence_requirement="optional", + legacy_incomplete=False, + configured_by_actor="artifact-admission-test", + configured_at=datetime.now(UTC), ), RevisionPolicy( id=str(uuid4()), @@ -537,8 +542,9 @@ async def _seed_checker_output_relationships(session) -> tuple[str, str, str]: guide_version=guide_version, max_revision_rounds=1, revision_deadline_hours=24, - auto_reject_after_limit=True, - allowed_resubmission_states=["needs_revision"], + legacy_incomplete=False, + configured_by_actor="artifact-admission-test", + configured_at=datetime.now(UTC), ), PaymentPolicy( id=str(uuid4()), @@ -548,6 +554,10 @@ async def _seed_checker_output_relationships(session) -> tuple[str, str, str]: ] ) await session.flush() + guide.status = "active" + guide.approved_by = "setup-actor" + guide.effective_at = now + await session.flush() session.add( WorkstreamTask( id=task_id, diff --git a/backend/tests/test_projects.py b/backend/tests/test_projects.py index 777f5203..825a04b9 100644 --- a/backend/tests/test_projects.py +++ b/backend/tests/test_projects.py @@ -13,7 +13,7 @@ import pytest # type: ignore[import-not-found] from httpx import ASGITransport, AsyncClient -from sqlalchemy import event, select, update +from sqlalchemy import event, select, text, update from sqlalchemy.dialects import postgresql from sqlalchemy.exc import IntegrityError from sqlalchemy.schema import CreateIndex @@ -657,17 +657,14 @@ def complete_guide_payload(version: str = "v1") -> dict: ), "change_summary": f"Initial {version}", "review_policy": { - "requires_second_review": False, "allowed_decisions": ["accept", "needs_revision", "reject"], - "minimum_finding_fields": ["issue", "required_fix"], - "sla_hours": 24, + "minimum_finding_fields": ["description", "severity"], + "review_preference_window_seconds": 900, + "review_lease_duration_seconds": 1800, }, "revision_policy": { "max_revision_rounds": 7, "revision_deadline_hours": 48, - "auto_reject_after_limit": True, - "allowed_resubmission_states": ["needs_revision"], - "reviewer_reassignment_rule": "same reviewer preferred", }, "payment_policy": { "base_amount": "25.00", @@ -7208,20 +7205,33 @@ async def test_activation_requires_complete_payment_policy(project_client: Async assert "payment policy is incomplete" in response.json()["detail"] -async def test_activation_requires_complete_revision_policy(project_client: AsyncClient) -> None: +@pytest.mark.parametrize( + ("policy_name", "field_name", "field_value"), + ( + ("review_policy", "requires_second_review", False), + ("review_policy", "sla_hours", 24), + ("revision_policy", "auto_reject_after_limit", True), + ("revision_policy", "allowed_resubmission_states", ["needs_revision"]), + ("revision_policy", "reviewer_reassignment_rule", "same reviewer preferred"), + ), +) +async def test_policy_schema_rejects_retired_archival_inputs( + project_client: AsyncClient, + policy_name: str, + field_name: str, + field_value: object, +) -> None: project = await create_project(project_client) payload = complete_guide_payload() - payload["revision_policy"]["allowed_resubmission_states"] = [] - guide = await create_guide(project_client, project["id"], payload) - await create_approved_policy_bundle(project_client, project["id"], guide["id"]) - + payload[policy_name][field_name] = field_value response = await project_client.post( - f"/api/v1/projects/{project['id']}/guides/{guide['id']}/activate", + f"/api/v1/projects/{project['id']}/guides", headers=auth_headers(), + json=payload, ) assert response.status_code == 422 - assert "revision policy is incomplete" in response.json()["detail"] + assert field_name in response.text async def test_revision_policy_requires_deadline(project_client: AsyncClient) -> None: @@ -7261,22 +7271,85 @@ async def test_guide_update_rejects_manual_post_submit_checker_policy( assert "post_submit_checker_policy" in response.text -async def test_activation_rejects_unsupported_revision_resubmission_states( +@pytest.mark.parametrize( + ("legacy_states", "expected_status", "expected_detail"), + ( + (None, 422, "revision policy is incomplete"), + ([], 422, "revision policy is incomplete"), + (["submitted"], 422, "revision policy contains invalid resubmission states"), + (["needs_revision"], 200, None), + ), +) +async def test_activation_preserves_migrated_revision_policy_state_rules( project_client: AsyncClient, + legacy_states: list[str] | None, + expected_status: int, + expected_detail: str | None, ) -> None: project = await create_project(project_client) - payload = complete_guide_payload() - payload["revision_policy"]["allowed_resubmission_states"] = ["random_state"] - guide = await create_guide(project_client, project["id"], payload) + guide = await create_guide(project_client, project["id"], complete_guide_payload()) await create_approved_policy_bundle(project_client, project["id"], guide["id"]) + async with db_session.get_session_factory()() as session: + await session.execute( + text("alter table review_policies disable trigger trg_review_policies_guard_write") + ) + await session.execute( + text("alter table revision_policies disable trigger trg_revision_policies_guard_write") + ) + await session.execute( + text( + "update review_policies set legacy_incomplete=true," + "review_preference_window_seconds=null,review_lease_duration_seconds=null," + "configured_by_actor=null,configured_at=null," + "legacy_requires_second_review=false,legacy_sla_hours=24 " + "where project_id=:project_id and guide_version='v1'" + ), + {"project_id": project["id"]}, + ) + await session.execute( + text( + "update revision_policies set legacy_incomplete=true," + "configured_by_actor=null,configured_at=null," + "legacy_auto_reject_after_limit=true," + "legacy_allowed_resubmission_states=cast(:states as json)," + "legacy_reviewer_reassignment_rule='same reviewer preferred' " + "where project_id=:project_id and guide_version='v1'" + ), + { + "project_id": project["id"], + "states": json.dumps(legacy_states) if legacy_states is not None else None, + }, + ) + await session.execute( + text("alter table review_policies enable trigger trg_review_policies_guard_write") + ) + await session.execute( + text("alter table revision_policies enable trigger trg_revision_policies_guard_write") + ) + await session.commit() + response = await project_client.post( f"/api/v1/projects/{project['id']}/guides/{guide['id']}/activate", headers=auth_headers(), ) - assert response.status_code == 422 - assert "invalid resubmission states" in response.json()["detail"] + assert response.status_code == expected_status, response.text + if expected_detail is not None: + assert expected_detail in response.json()["detail"] + else: + review_policy = response.json()["review_policy"] + assert review_policy["legacy_incomplete"] is True + for archival_field in ("requires_second_review", "sla_hours"): + assert archival_field not in review_policy + revision_policy = response.json()["revision_policy"] + assert revision_policy["legacy_incomplete"] is True + for archival_field in ( + "auto_reject_after_limit", + "allowed_resubmission_states", + "reviewer_reassignment_rule", + ): + assert archival_field not in revision_policy async def test_activation_rejects_pending_pre_submit_checker_policy( @@ -7484,8 +7557,17 @@ async def test_guide_activation_and_active_guide_retrieval(project_client: Async active.json()["pre_submit_checker_policy"]["checker_configs"] == (bundle["pre_submit_checker_policy"]["checker_configs"]) ) + assert active.json()["review_policy"]["review_preference_window_seconds"] == 900 + assert active.json()["review_policy"]["review_lease_duration_seconds"] == 1800 + assert active.json()["review_policy"]["finding_evidence_requirement"] == "optional" + assert active.json()["review_policy"]["legacy_incomplete"] is False + assert "requires_second_review" not in active.json()["review_policy"] + assert "sla_hours" not in active.json()["review_policy"] assert active.json()["revision_policy"]["max_revision_rounds"] == 7 - assert active.json()["revision_policy"]["auto_reject_after_limit"] is True + assert active.json()["revision_policy"]["legacy_incomplete"] is False + assert "auto_reject_after_limit" not in active.json()["revision_policy"] + assert "allowed_resubmission_states" not in active.json()["revision_policy"] + assert "reviewer_reassignment_rule" not in active.json()["revision_policy"] assert active.json()["payment_policy"]["base_amount"] == "25.00" diff --git a/backend/tests/test_tasks.py b/backend/tests/test_tasks.py index 288d1d3b..5da951d7 100644 --- a/backend/tests/test_tasks.py +++ b/backend/tests/test_tasks.py @@ -697,17 +697,14 @@ def complete_guide_payload(version: str = "v1") -> dict: ), "change_summary": f"Initial {version}", "review_policy": { - "requires_second_review": False, "allowed_decisions": ["accept", "needs_revision", "reject"], - "minimum_finding_fields": ["issue", "required_fix"], - "sla_hours": 24, + "minimum_finding_fields": ["description", "severity"], + "review_preference_window_seconds": 900, + "review_lease_duration_seconds": 1800, }, "revision_policy": { "max_revision_rounds": 7, "revision_deadline_hours": 48, - "auto_reject_after_limit": True, - "allowed_resubmission_states": ["needs_revision"], - "reviewer_reassignment_rule": "same reviewer preferred", }, "payment_policy": { "base_amount": "25.00", @@ -2671,7 +2668,7 @@ async def test_ready_worker_work_context_omits_private_task_source_fields( assert private_field not in body["task"] -async def test_work_context_uses_stamped_policy_values_after_same_version_policy_mutation( +async def test_published_review_policy_rejects_same_version_mutation( task_client: AsyncClient, monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -2697,19 +2694,44 @@ async def test_work_context_uses_stamped_policy_values_after_same_version_policy RevisionPolicy.guide_version == "v1", ) ) + assert review_policy is not None + assert revision_policy is not None + review_policy.allowed_decisions = ["reject"] + with pytest.raises(IntegrityError, match="published review and revision policies"): + await session.commit() + await session.rollback() + + after_response = await task_client.get( + f"/api/v1/tasks/{started_task['id']}/work-context", + headers=auth_headers(), + ) + + assert after_response.status_code == 200, after_response.text + after = after_response.json() + assert after == before + + +async def test_work_context_uses_stamped_policy_values_after_payment_policy_mutation( + task_client: AsyncClient, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = await create_active_project(task_client) + started_task = await create_started_task(task_client, project["id"], monkeypatch) + before_response = await task_client.get( + f"/api/v1/tasks/{started_task['id']}/work-context", + headers=auth_headers(), + ) + assert before_response.status_code == 200, before_response.text + before = before_response.json() + + async with db_session.get_session_factory()() as session: payment_policy = await session.scalar( select(PaymentPolicy).where( PaymentPolicy.project_id == project["id"], PaymentPolicy.guide_version == "v1", ) ) - assert review_policy is not None - assert revision_policy is not None assert payment_policy is not None - review_policy.requires_second_review = True - review_policy.allowed_decisions = ["reject"] - revision_policy.max_revision_rounds = 1 - revision_policy.revision_deadline_hours = 1 payment_policy.base_amount = Decimal("999.00") payment_policy.currency = "EUR" payment_policy.payout_type = "manual" @@ -2719,7 +2741,6 @@ async def test_work_context_uses_stamped_policy_values_after_same_version_policy f"/api/v1/tasks/{started_task['id']}/work-context", headers=auth_headers(), ) - assert after_response.status_code == 200, after_response.text after = after_response.json() assert after["review_policy"] == before["review_policy"] diff --git a/docs/architecture_data_model.md b/docs/architecture_data_model.md index a7f7a9e0..11b87ab6 100644 --- a/docs/architecture_data_model.md +++ b/docs/architecture_data_model.md @@ -928,9 +928,22 @@ Fields: - `guide_version` - `allowed_decisions` - `minimum_finding_fields` -- `sla_hours` +- `review_preference_window_seconds` +- `review_lease_duration_seconds` +- `max_active_review_leases_per_reviewer` (fixed to `1` in v0.1) +- `self_review_allowed` (fixed to `false` in v0.1) +- `reject_policy` (fixed to `close_task` in v0.1) +- `finding_evidence_requirement` +- `legacy_incomplete` +- `configured_by_actor` +- `configured_at` - `created_at` +The decision array is exactly `accept`, `needs_revision`, `reject`; minimum +finding fields are exactly `description`, `severity`. Migrated rows retain +retired second-review/SLA facts only in database-only archival columns and are +explicitly legacy-incomplete. No routing duration is inferred from an SLA. + ## RevisionPolicy Fields: @@ -940,10 +953,16 @@ Fields: - `guide_version` - `max_revision_rounds` - `revision_deadline_hours` -- `allowed_resubmission_states` -- `reviewer_reassignment_rule` +- `legacy_incomplete` +- `configured_by_actor` +- `configured_at` - `created_at` +Migrated automatic-reject, resubmission-state, and reassignment values are +preserved only in database-only archival columns. They are not active policy +and are not exposed by the API. New policy rows contain only positive revision +round and deadline inputs. + Limit or deadline exhaustion blocks further preparation and submission; it does not synthesize a reject Review. Project Guide context selection is deterministic, not policy-selected: exact prior Submission identity/activation-sequence match diff --git a/docs/template_project_guide.md b/docs/template_project_guide.md index 6ff1aed9..854b8849 100644 --- a/docs/template_project_guide.md +++ b/docs/template_project_guide.md @@ -110,8 +110,12 @@ Every active guide version must have: - EffectiveProjectSubmissionArtifactPolicy hash: - project PreSubmitCheckerPolicy compiled bundle hash: - PostSubmitCheckerPolicy: -- ReviewPolicy: -- RevisionPolicy: +- ReviewPolicy: preference window seconds, lease duration seconds, evidence + requirement (`optional`, `required_for_blocking`, or `required_for_all`), + canonical decisions, capacity `1`, self-review `false`, reject policy + `close_task`, and configuring actor +- RevisionPolicy: positive maximum human revision rounds, positive deadline + hours, and configuring actor; exhaustion never creates an automatic reject - ContributionPolicy and active published version: ContributionPolicyVersion is the source of truth for exact @@ -171,18 +175,13 @@ Define: - maximum revision rounds: - revision deadline hours: -- allowed resubmission states: -- `RevisionPolicyInput.auto_reject_after_limit`: `false` (required explicitly - on project create/update; the backend schema default is not the v0.1 REV - contract) - limit/deadline exhaustion behavior: block preparation and submission pending reason-bound covered-manager closure; never synthesize reject -- reviewer reassignment rule: -Revision-policy activation and task screening must reject an effective policy -whose `auto_reject_after_limit` value is not `false`. That runtime enforcement -belongs to `WS-REV-001-02`; until it activates, this template is a required -configuration precondition and does not claim the guard is available. +Automatic reject, arbitrary resubmission-state lists, and reviewer-reassignment +strings are not active `RevisionPolicy` input. Migrated values remain archival +database evidence only. Later REV admission and revision execution consume the +positive round/deadline facts without synthesizing a Review or decision. ## Acceptance Policy