fix(projects): require second-review policy false

.commitrail/changes/enforce-requires-second-review-false.md

@@ -0,0 +1,97 @@
+# Enforce requires_second_review=false
+
+- Initiative: None
+- Durable disposition: Complete
+- Intended merge outcome: ReviewPolicy accepts and persists only `requires_second_review=false`, while existing false-policy hashes and immutable lineage remain unchanged.
+
+## Intent
+
+Adjudication and second-review lifecycle behavior are deferred. The current
+ReviewPolicy surface nevertheless accepts `requires_second_review=true`, and
+the database permits the unsupported value through raw writes. This change
+closes both paths without inventing review behavior or rewriting retained
+policy facts.
+
+## Current behavior
+
+`ReviewPolicyInput`, `ReviewPolicySemantics`, and `ReviewPolicyResponse` type the
+field as an unrestricted boolean. `ReviewPolicy` stores it as a non-null boolean
+without a false-only constraint. The authorized policy writer hashes the field,
+shared locked-policy projection validates it, and the external API drill
+currently submits true on replacement. Migration 0023 is the merged predecessor
+and owns the separate obsolete TASK payment cleanup.
+
+## Bounded change
+
+### Allowed
+
+- PROJECTS review-policy input, immutable semantics/lineage response and ORM
+  constraint metadata.
+- One additive migration 0024 after 0023: lock `review_policies`, refuse any
+  retained true row, then install the false-only check constraint.
+- Alembic accepted-revision registration, graph proof, PostgreSQL 16 schema
+  fingerprint, semantic test-lane registration and the external API drill.
+- Focused input/hash, shared projection, public API, raw-write and real
+  predecessor-upgrade tests, plus the existing payment-cleanup full-head
+  migration proof's current-revision assertion.
+- Current README, architecture, review lifecycle, guide template and roadmap
+  statements affected by the supported ReviewPolicy contract.
+
+### Not allowed
+
+- Second-review, adjudication, reviewer assignment or decision behavior.
+- ContributionPolicy, award, payment, routing, activation or acceptance
+  semantics.
+- Retained-data rewrite/deletion, policy rehashing, migration 0023 changes,
+  compatibility variants or duplicate payment cleanup.
+
+## Design and decisions
+
+Use `Literal[False] = False` at untrusted input and immutable typed lineage
+boundaries. Keep the stored column because it remains part of canonical v1/v2
+policy bodies and removing it would change existing false-policy hashes. Add a
+named PostgreSQL check for raw-write enforcement. Migration 0024 takes an
+`ACCESS EXCLUSIVE` table lock across preflight and constraint installation so a
+writer cannot introduce true between them. A retained true row raises SQLSTATE
+23514 before DDL; the migration does not repair, rewrite, delete or rehash it.
+
+## Acceptance criteria
+
+- [x] Public true input returns 422 without advancing the selected policy.
+- [x] Omitted/false v1 and v2 canonical hashes retain their exact bytes.
+- [x] Typed immutable lineage and shared locked-policy projection reject true.
+- [x] PostgreSQL rejects raw true writes through the named model/migration
+  constraint.
+- [x] Real 0023-to-0024 upgrade refuses a custody-committed retained true policy
+  with its row, hash, guide selector and schema version unchanged.
+- [x] Ordinary false-only upgrade preserves policy rows/hashes/selectors and
+  installs one recognized 0024 head.
+
+## Risk and review routing
+
+- Risk class: L1
+- Required reviewers: architecture, reuse/dedup, security, QA, test delta, documentation
+- Human review focus: retained-fact refusal, unchanged hashes, schema/model/type parity, and no accidental second-review or payment semantics
+
+## Evidence
+
+| Claim | Command or proof | Result | Remaining uncertainty |
+|---|---|---|---|
+| Typed and shared caller behavior | Focused review-policy, locked-policy and identity-lineage pytest modules | False contracts and fixed hash bytes pass; true lineage fails closed | Full hosted suite remains external evidence |
+| PostgreSQL schema identity | Fresh PostgreSQL 16 migration through 0024, canonical fingerprint generation and focused graph/raw-write proof | Head installs the named constraint; fingerprint updated from the real schema; focused boundary nodes pass | Hosted PostgreSQL lane remains external evidence |
+| Retained-data safety | Actual 0023 predecessor migration tests | Custody-committed true facts refuse unchanged; false facts upgrade unchanged | Operational handling of any discovered true row requires a separate preservation decision |
+
+## Review findings
+
+No material finding is recorded in this durable change record.
+
+## Reconciliation
+
+- Current-source reconciliation: current `main` at `235f9e1b` includes the
+  merged obsolete-payment cleanup, local runtime, evaluation-capacity and guide
+  CLI work. Migration 0024 extends the merged 0023 head without changing that
+  migration or cleanup behavior.
+- The existing payment-cleanup proof still upgrades through full Alembic head
+  and now checks that result against the canonical current revision lookup.
+- Next usable boundary: second-review/adjudication behavior remains deferred; other pilot work proceeds only after this pull request is merged.
+- Remaining risks: a database containing retained true policies cannot upgrade until an explicit preservation/disposition design is approved.

README.md

@@ -58,7 +58,11 @@ Pre-submission and post-submission checking are different stages:
 The v0.1 project setting `human_review_required` defaults to `true` in the
 existing locked ReviewPolicy. After required post-submit checks pass, true
 requires human review; false uses an authorized automated FinalAcceptance and
-submitter ContributionRecord, with no reviewer contribution. This branch is
+submitter ContributionRecord, with no reviewer contribution.
+The separate `requires_second_review` policy field is [fixed to `false`](.commitrail/changes/enforce-requires-second-review-false.md)
+at input, immutable lineage, and database boundaries; second-review and
+adjudication behavior remain deferred.
+This branch is
 not live yet: the [policy setting](.commitrail/changes/pre-review-plan-reconciliation.md#delivered-policy-setting-implementation)
 is implemented for configuration, and the hidden submitter participant can
 stage or exactly replay its complete frozen award set. Shared acceptance, TASK

backend/alembic/env.py

@@ -21,7 +21,7 @@
 target_metadata = Base.metadata
 
 _BASELINE_REVISION = "0001_uuid7_v01"
-_CURRENT_HEAD_REVISION = "0023_remove_task_payment_policy"
+_CURRENT_HEAD_REVISION = "0024_require_second_review_false"
 _RECREATE_GUIDANCE = (
     "Workstream v0.1 requires a fresh database; recreate this database before "
     "running the 0001_uuid7_v01 migration"
@@ -52,7 +52,7 @@ def do_run_migrations(connection: Connection) -> None:
             .scalars()
             .all()
         )
-        if revisions not in ((), (_BASELINE_REVISION,), ("0002_task_queue_authority",), ("0003_task_read_authority",), ("0004_task_context_authority",), ("0005_task_evidence_authority",), ("0006_history_read_authority",), ("0007_checker_output_custody",), ("0008_checker_execution",), ("0009_checker_material_lineage",), ("0010_post_submit_authority",), ("0011_task_routing_source",), ("0012_review_packet",), ("0013_review_source",), ("0014_final_acceptance",), ("0015_contribution_awards",), ("0016_review_lifecycle_fence",), ("0017_acceptance_source_contracts",), ("0018_task_routing_request",), ("0019_submitter_awards",), ("0020_review_admission_lock_order",), ("0021_submission_manifest",), ("0022_submission_packet_custody",), (_CURRENT_HEAD_REVISION,)):
+        if revisions not in ((), (_BASELINE_REVISION,), ("0002_task_queue_authority",), ("0003_task_read_authority",), ("0004_task_context_authority",), ("0005_task_evidence_authority",), ("0006_history_read_authority",), ("0007_checker_output_custody",), ("0008_checker_execution",), ("0009_checker_material_lineage",), ("0010_post_submit_authority",), ("0011_task_routing_source",), ("0012_review_packet",), ("0013_review_source",), ("0014_final_acceptance",), ("0015_contribution_awards",), ("0016_review_lifecycle_fence",), ("0017_acceptance_source_contracts",), ("0018_task_routing_request",), ("0019_submitter_awards",), ("0020_review_admission_lock_order",), ("0021_submission_manifest",), ("0022_submission_packet_custody",), ("0023_remove_task_payment_policy",), (_CURRENT_HEAD_REVISION,)):
             raise RuntimeError(_RECREATE_GUIDANCE)
     # The read-only preflight autobegins a SQLAlchemy transaction. End that
     # transaction before Alembic establishes the migration transaction;

backend/alembic/versions/0024_require_second_review_false.py

@@ -0,0 +1,36 @@
+"""Refuse unsupported retained second-review policy before fixing it false."""
+
+from alembic import op
+
+revision = "0024_require_second_review_false"
+down_revision = "0023_remove_task_payment_policy"
+branch_labels = None
+depends_on = None
+
+
+def upgrade() -> None:
+    """Preserve retained policy facts and install the current false-only invariant."""
+    op.execute("LOCK TABLE public.review_policies IN ACCESS EXCLUSIVE MODE")
+    op.execute("""
+DO $$
+BEGIN
+    IF EXISTS (
+        SELECT 1 FROM public.review_policies WHERE requires_second_review
+    ) THEN
+        RAISE EXCEPTION
+            'retained requires_second_review=true policy requires an explicit preservation design'
+            USING ERRCODE='23514';
+    END IF;
+END;
+$$;
+""")
+    op.create_check_constraint(
+        "review_policy_second_review_disabled",
+        "review_policies",
+        "not requires_second_review",
+        schema="public",
+    )
+
+
+def downgrade() -> None:
+    raise RuntimeError("Workstream v0.1 migrations cannot be downgraded; recreate the database")

backend/app/modules/projects/api/policy_lineage.py

@@ -29,7 +29,7 @@ class ReviewPolicySemantics(BaseModel):
     finding_evidence_requirement: Literal[
         "optional", "required_for_blocking", "required_for_all"
     ] = "optional"
-    requires_second_review: bool = False
+    requires_second_review: Literal[False] = False
     allowed_decisions: tuple[Literal["accept", "needs_revision", "reject"], ...]
     minimum_finding_fields: tuple[str, ...] = ()
 

backend/app/modules/projects/models.py

@@ -704,6 +704,10 @@ class ReviewPolicy(Base):
             "semantics_format in ('v1','v2') and (semantics_format <> 'v1' or human_review_required)",
             name="review_policy_semantics_format",
         ),
+        CheckConstraint(
+            "not requires_second_review",
+            name="review_policy_second_review_disabled",
+        ),
         ForeignKeyConstraint(
             ["project_id", "guide_version"],
             ["project_guides.project_id", "project_guides.version"],

backend/app/modules/projects/schemas.py

@@ -26,7 +26,7 @@ class ReviewPolicyInput(BaseModel):
     finding_evidence_requirement: Literal[
         "optional", "required_for_blocking", "required_for_all"
     ] = "optional"
-    requires_second_review: bool = False
+    requires_second_review: Literal[False] = False
     allowed_decisions: list[Literal["accept", "needs_revision", "reject"]] = Field(
         default_factory=lambda: ["accept", "needs_revision", "reject"]
     )
@@ -564,7 +564,7 @@ class ReviewPolicyResponse(BaseModel):
     self_review_allowed: bool | None
     reject_policy: str | None
     finding_evidence_requirement: str | None
-    requires_second_review: bool
+    requires_second_review: Literal[False]
     allowed_decisions: list[str]
     minimum_finding_fields: list[str]
     created_at: datetime

backend/scripts/external_api_drill.py

@@ -686,6 +686,8 @@ async def policy_cases(drill, manager, groute, gpath, outsider):
             invalids.append((field + "_type", body | {field: {"bad": True}}))
         invalids += [(field + "_enum", body | {field: "invalid"}) for field in defaults[suffix]
                      if field not in {"reviewer_reassignment_rule"}]
+        if suffix == "review-policy":
+            invalids.append(("requires_second_review_true", body | {"requires_second_review": True}))
         for label, invalid in invalids:
             await drill.call(suffix + "_" + label, "PUT", groute + "/" + suffix,
                 path=gpath + "/" + suffix, token=manager, payload=invalid,
@@ -698,7 +700,7 @@ async def policy_cases(drill, manager, groute, gpath, outsider):
         # A fresh update with the prior selector proves rejections did not advance it.
         selector = f'"{result["id"]}.{result["policy_generation"]}.{result["policy_hash"].removeprefix("sha256:")}"'
         replacement = (body | {"human_review_required": False, "finding_evidence_requirement": "required_for_all",
-                              "requires_second_review": True, "allowed_decisions": ["accept", "reject"],
+                              "requires_second_review": False, "allowed_decisions": ["accept", "reject"],
                               "minimum_finding_fields": ["summary", "evidence"]}
                        if suffix == "review-policy" else
                        body | {"max_revision_rounds": 1, "revision_deadline_hours": 1,

backend/scripts/test_lane_catalogue.py

@@ -247,6 +247,7 @@ class TestLane:
     "tests/projects/review_policy/test_activation.py",
     "tests/projects/review_policy/test_postgresql.py",
     "tests/projects/review_policy/test_schema.py",
+    "tests/projects/review_policy/test_migration.py",
     "tests/projects/test_compensation_binding_eligibility.py",
     "tests/test_review_authorization_contracts.py",
 )

backend/tests/conftest.py

@@ -23,7 +23,7 @@
 DDL_LOCK_DIRECTORY = Path("/tmp")
 # Match the PostgreSQL 16 engine used by Backend CI. Catalog identity rendering
 # differs across major versions; regenerate only after comparing actual objects.
-EXPECTED_PUBLIC_SCHEMA_SHA256 = "204e6ad818f4f0aac2652e875a4d57d56c8aae83a4d5daf6a807e9a9c362fc79"
+EXPECTED_PUBLIC_SCHEMA_SHA256 = "8ee2b6ba04814fb217471b5c6e40c137bdfd0692e751bfbcb0005f1c936a09db"
 PROTECTED_TEST_TABLES = (
     "joint_lifecycle_release_control",
     "actor_profile_migration_state",