diff --git a/orchestrator/routes/pipelines/_criteria.py b/orchestrator/routes/pipelines/_criteria.py index 6cd91f46a..955990f6b 100644 --- a/orchestrator/routes/pipelines/_criteria.py +++ b/orchestrator/routes/pipelines/_criteria.py @@ -241,6 +241,18 @@ def _get_refine_review_criteria() -> str: "`no_decisions_rationale`), verify the rationale holds: requirements " "genuinely unambiguous, no assumptions made silently. NACK if you " "find a hidden operator-grade choice.\n" + "- **Enumerated candidates required (#3526).** An explicit-none " + "attestation must carry `candidates_considered`: one " + "`{question, disposition, why}` entry per open choice the producer " + "weighed and dispositioned away (`not_operator_grade` = a design " + "call the planner/implementer owns; `deferred_to_plan` = potentially " + "operator-grade, better asked at plan). A bare rationale with no " + "candidates, or candidates whose `why` is vacuous / does not match " + "the choices the analysis actually faced, is a **NACK** — the " + "enumeration is what the operator confirms at the gate, so it must " + "be concrete. Check `deferred_to_plan` items especially: they are " + "carried into the plan prompt as items the planner must close, so a " + "wrongly-deferred operator decision escapes refine's surface.\n" "- **Task-named decisions — NACK an explicit-none ledger (#3462).** " "If the task description names decisions as the operator's to make " "(or directs that decisions be surfaced as HITL questions), each " @@ -685,6 +697,18 @@ def _get_plan_review_criteria() -> str: "- A deliberately empty ledger arrives as a producer's " "`no_decisions_rationale` attestation — verify it holds; NACK if " "the plan hides an operator-grade choice.\n" + "- **Enumerated candidates required (#3526).** An explicit-none " + "attestation must carry `candidates_considered`: one " + "`{question, disposition, why}` entry per open choice the producer " + "weighed. At plan the only valid disposition is `not_operator_grade` " + "(a design call plan legitimately owns) with a concrete why — plan " + "is the last decision surface, so `deferred_to_plan` is rejected at " + "propose time. A bare rationale with no candidates, or a candidate " + "whose `why` is vacuous, is a **NACK**. Cross-check against the " + "refiner's deferred candidates surfaced in the plan prompt: each " + "must be registered as a `cq-N` or dispositioned `not_operator_grade` " + "here — a deferred candidate the plan silently drops is the leak " + "#3526 exists to close.\n" "- **Task-named decisions — NACK an explicit-none ledger (#3462).** " "If the task description or refine analysis names decisions as the " "operator's to make (or directs that decisions be surfaced as HITL " diff --git a/orchestrator/routes/pipelines/_ledger.py b/orchestrator/routes/pipelines/_ledger.py index 4f3ff9416..98304eeff 100644 --- a/orchestrator/routes/pipelines/_ledger.py +++ b/orchestrator/routes/pipelines/_ledger.py @@ -461,6 +461,13 @@ def _collect_decision_ledger_status( ``PhaseExecution.decision_ledger`` (#3526): registered ids, explicit-none flag, and considered candidates, so decisions-surfaced-per-phase is queryable from pipeline state. + The key set is **uniform across all three branches** — + ``registered``, ``resolved``, ``explicit_none``, ``attested_by``, + ``missing``, ``candidates_considered`` are always present (with + ``attested_by=None`` / ``missing=False`` on the branches that + don't set them) so a downstream consumer never has to guess + whether a key is absent because it doesn't apply or because the + branch simply forgot it. """ phase_value = phase.value registered_ids: list[str] = [] @@ -500,6 +507,8 @@ def _collect_decision_ledger_status( "registered": registered_ids, "resolved": resolved, "explicit_none": False, + "attested_by": None, + "missing": False, "candidates_considered": [], }, ) @@ -517,6 +526,7 @@ def _collect_decision_ledger_status( "resolved": 0, "explicit_none": True, "attested_by": role, + "missing": False, "candidates_considered": candidates, }, ) @@ -532,6 +542,7 @@ def _collect_decision_ledger_status( "registered": [], "resolved": 0, "explicit_none": False, + "attested_by": None, "missing": True, "candidates_considered": [], }, diff --git a/orchestrator/tests/test_decision_ledger_gate.py b/orchestrator/tests/test_decision_ledger_gate.py index 99778f261..c0447e6d8 100644 --- a/orchestrator/tests/test_decision_ledger_gate.py +++ b/orchestrator/tests/test_decision_ledger_gate.py @@ -113,6 +113,8 @@ def test_registered_decisions_counted(self, tmp_path: Path): "registered": ["cq-1", "cq-2"], "resolved": 1, "explicit_none": False, + "attested_by": None, + "missing": False, "candidates_considered": [], } @@ -137,6 +139,16 @@ def test_zero_registered_with_explicit_none_attestation(self, tmp_path: Path): assert summary["explicit_none"] is True assert summary["registered"] == [] assert summary["attested_by"] == "refiner" + # Uniform shape across branches (#3526 review): every key present. + assert summary["missing"] is False + assert set(summary) == { + "registered", + "resolved", + "explicit_none", + "attested_by", + "missing", + "candidates_considered", + } def test_zero_registered_no_attestation_is_missing(self, tmp_path: Path): from routes.pipelines import _collect_decision_ledger_status @@ -150,6 +162,16 @@ def test_zero_registered_no_attestation_is_missing(self, tmp_path: Path): assert explicit_none is None assert "MISSING" in note assert summary["missing"] is True + # Uniform shape across branches (#3526 review): every key present. + assert summary["attested_by"] is None + assert set(summary) == { + "registered", + "resolved", + "explicit_none", + "attested_by", + "missing", + "candidates_considered", + } def test_contract_unloadable_falls_back_to_attestation(self, tmp_path: Path): from routes.pipelines import _collect_decision_ledger_status diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 7a77ac44c..83ea1b847 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -4162,12 +4162,22 @@ def test_refine_reviewer_carries_unsurfaced_decision_obligation(self): # Calibration guard so the obligation doesn't over-NACK # legitimate implementation choices. assert "do not over-NACK" in criteria + # The explicit-none contract now requires enumerated candidates + # (#3526); the reviewer rubric must name it so the reviewer checks + # the enumeration, not just the free-form rationale. + assert "candidates_considered" in criteria + assert "deferred_to_plan" in criteria def test_plan_reviewer_carries_unsurfaced_decision_obligation(self): criteria = _get_plan_review_criteria() assert "Un-surfaced decisions — NACK" in criteria assert "no_decisions_rationale" in criteria assert "do not over-NACK" in criteria + # Enumerated-candidate requirement + the plan-is-last-surface rule + # (#3526): the reviewer must cross-check the refiner's deferred + # candidates against the plan attestation. + assert "candidates_considered" in criteria + assert "not_operator_grade" in criteria class TestTaskNamedDecisionRegistrationSurface: