diff --git a/src/hyperloom/agents/critic/references/verdict_schema.md b/src/hyperloom/agents/critic/references/verdict_schema.md index 1750335948..bb05b54975 100644 --- a/src/hyperloom/agents/critic/references/verdict_schema.md +++ b/src/hyperloom/agents/critic/references/verdict_schema.md @@ -33,7 +33,8 @@ caller explicitly asks for markdown. ], "alternative_action": null, "advice_text": "", - "notes": [] + "notes": [], + "failure_reason_code": "" } ``` @@ -62,6 +63,17 @@ Verdict rules: - `needs_review`: dispatch must not proceed. Use for high-risk mock, timeout, unavailable, or insufficient-evidence cases. +`failure_reason_code` names the review rule a non-`approve` verdict rests on, +copied verbatim from the `failure_reason_code` of the matching rule in +`judge_bundle.review_constraints` (the quantitative-claim rule, a cross-domain +rule, or a safety guard). Leave it empty when the verdict rests on your own +judgement rather than a rule handed to you in the bundle. Several of those +rules declare `advise` as their `failure_verdict` because rejecting on them +costs the round every proposal in the set; naming the rule is how the +Coordinator can tell such a verdict apart from a substantive rejection, so a +verdict that cites a rule must carry its code rather than only mentioning it in +`reasoning`. + Approve example: ```json diff --git a/src/hyperloom/agents/critic/runtime/decision_reviewer.py b/src/hyperloom/agents/critic/runtime/decision_reviewer.py index 54529d510d..00629d6a19 100644 --- a/src/hyperloom/agents/critic/runtime/decision_reviewer.py +++ b/src/hyperloom/agents/critic/runtime/decision_reviewer.py @@ -976,6 +976,7 @@ def _commit_coordinator_inbox( alternative_action=item.get("alternative_action"), advice_text=advice_text, notes=item.get("notes") or [], + failure_reason_code=str(item.get("failure_reason_code") or ""), ) except IntentEnvelopeValidationError as exc: raise ReviewValidationError(str(exc)) from exc @@ -989,6 +990,7 @@ def _commit_coordinator_inbox( "verdict": verdict, "reasoning": item.get("reasoning"), "source": item.get("source", "critic"), + "failure_reason_code": str(item.get("failure_reason_code") or ""), "kb_evidence": item.get("kb_evidence") or [], }, ) diff --git a/src/hyperloom/agents/critic/runtime/intent_envelope.py b/src/hyperloom/agents/critic/runtime/intent_envelope.py index b197459fde..478e8f7975 100644 --- a/src/hyperloom/agents/critic/runtime/intent_envelope.py +++ b/src/hyperloom/agents/critic/runtime/intent_envelope.py @@ -165,6 +165,7 @@ def build_review_verdict_intent( alternative_action: str | None = None, advice_text: str = "", notes: Iterable[str] | None = None, + failure_reason_code: str = "", ) -> Intent: """Build a validated ``review_verdict`` intent. @@ -183,6 +184,9 @@ def build_review_verdict_intent( alternative_action (str | None): Suggested alternative action. advice_text (str): Devil's-advocate advice text. notes (Iterable[str] | None): Additional free-text notes. + failure_reason_code (str): The ``failure_reason_code`` of the review + rule this verdict rests on, as declared in the judge bundle's + ``review_constraints``. Empty when the verdict cites no rule. Returns: Intent: The constructed ``review_verdict`` intent. @@ -210,6 +214,7 @@ def build_review_verdict_intent( "alternative_action": alternative_action, "advice_text": advice_text, "notes": list(notes or []), + "failure_reason_code": failure_reason_code, } if confidence is not None: payload["confidence"] = confidence diff --git a/src/hyperloom/agents/critic/runtime/tests/test_decision_reviewer.py b/src/hyperloom/agents/critic/runtime/tests/test_decision_reviewer.py index 2def11a441..607d8d172f 100644 --- a/src/hyperloom/agents/critic/runtime/tests/test_decision_reviewer.py +++ b/src/hyperloom/agents/critic/runtime/tests/test_decision_reviewer.py @@ -416,6 +416,45 @@ def _verdict_intent_for(intents: list[dict], target: str) -> dict: raise AssertionError(f"no review_verdict intent for {target!r}") +def test_commit_review_carries_the_cited_rule_into_the_intent(reviewer): + """The Coordinator holds a reject to the verdict its rule declared, and it + can only do that if the code the Critic cited survives the commit path.""" + rev, _kb, sm = reviewer + rev.prepare_review(_coordinator_request(_PROMPT_WITH_TWO_PROPOSALS, "sess_code")) + review = { + "review_verdicts": [ + { + "target_proposal_msg_id": "aaa1", + "verdict": "reject", + "reasoning": "proposal carried a self-reported gain", + "failure_reason_code": "specialist_quantitative_claim_violation", + }, + { + "target_proposal_msg_id": "bbb2", + "verdict": "approve", + "reasoning": "evidence is complete", + }, + ] + } + outcome = rev.commit_review( + _coordinator_request(_PROMPT_WITH_TWO_PROPOSALS, "sess_code"), + review, + ) + intents = outcome.intent_envelope["intents"] + + cited = _verdict_intent_for(intents, "aaa1")["payload"] + assert cited["failure_reason_code"] == "specialist_quantitative_claim_violation" + uncited = _verdict_intent_for(intents, "bbb2")["payload"] + assert uncited["failure_reason_code"] == "" + logged = [ + json.loads(line)["decision_review"] + for line in (sm.session_dir("sess_code") / "decisions.jsonl").read_text("utf-8").splitlines() + if line.strip() + ] + codes = {d["target_proposal_msg_id"]: d["failure_reason_code"] for d in logged} + assert codes["aaa1"] == "specialist_quantitative_claim_violation" + + def test_commit_review_backfills_advice_text_from_advice_entry(reviewer): rev, kb, sm = reviewer rev.prepare_review(_coordinator_request(_PROMPT_WITH_TWO_PROPOSALS, "sess_advice")) diff --git a/src/hyperloom/inference_optimizer/tests/test_critic_agent_backend.py b/src/hyperloom/inference_optimizer/tests/test_critic_agent_backend.py index 0f02ad5f7d..6eb256252c 100644 --- a/src/hyperloom/inference_optimizer/tests/test_critic_agent_backend.py +++ b/src/hyperloom/inference_optimizer/tests/test_critic_agent_backend.py @@ -28,10 +28,12 @@ CRITIC_AGENT_LLM_CONNECT_TIMEOUT_SEC, CRITIC_AGENT_LLM_RW_TIMEOUT_SEC, CRITIC_AGENT_MAX_COMPLETION_TOKENS, + _REVIEW_OUTPUT_INSTRUCTIONS, _extract_review_json, _reviewed_msg_ids_from_bundle, _verdict_references_kb, ) +from hyperloom.orchestrator.specialists.patch_safety import FORBIDDEN_PROPOSAL_FIELDS from hyperloom.inference_optimizer.protocol.intent import IntentType @@ -922,6 +924,91 @@ async def test_user_prompt_includes_judge_bundle_and_instructions( assert '"abc"' in user_text # proposal msg_id from judge bundle +def test_the_output_schema_asks_for_the_rule_the_verdict_rests_on(): + """The Critic is told to reply with *exactly* this schema, and the + Coordinator holds a reject to the verdict its cited rule declared by reading + `failure_reason_code`. Documenting the field only in a reference file + nothing loads is why prose-scanning became the only signal in production.""" + schema, _, rules = _REVIEW_OUTPUT_INSTRUCTIONS.partition("Rules (mirror") + + assert '"failure_reason_code"' in schema + assert "failure_reason_code" in rules + + +def _bundle_reviewing(action_name: str) -> dict[str, Any]: + """A judge bundle whose single proposal proposes ``action_name``.""" + return { + "kind": "coordinator_inbox", + "merged_context": {"model": "m", "framework": "sglang"}, + "proposals": [ + { + "msg_id": "abc", + "from_agent": "orchestration", + "action_name": action_name, + "payload": {}, + "predicted_gain_pct": 0.0, + } + ], + "kb_priors_by_proposal": {"abc": []}, + "kb_read_skipped_reason": None, + "review_constraints": {}, + "notes": [], + "missing_context": [], + "required_context": [], + } + + +async def _review_constraints_sent_for( + action_name: str, + fake_critic_root: Path, + fake_session_dir: Path, +) -> dict[str, Any]: + """Run one review turn and return the ``review_constraints`` the model saw.""" + backend, client = _make_backend( + fake_critic_root, + fake_session_dir, + codex_replies=['{"review_verdicts": [{"target_proposal_msg_id": "abc", "verdict": "approve"}]}'], + judge_bundle=_bundle_reviewing(action_name), + ) + await backend.run("ignored", system_prompt="you are critic") + user_text = client.completions.calls[0]["messages"][1]["content"] + match = re.search( + r"==== JUDGE BUNDLE ====\s*(\{.*?\})\s*==== END JUDGE BUNDLE ====", + user_text, + re.DOTALL, + ) + assert match + return json.loads(match.group(1))["review_constraints"] + + +@pytest.mark.asyncio +async def test_the_reviewed_bundle_carries_the_quantitative_claim_rule( + fake_critic_root: Path, + fake_session_dir: Path, +): + """Delivered as data so the Critic's field list stays identical to the one + the runner strips, and so a format slip is advisory rather than a reject + that costs the round every proposal in the set.""" + constraints = await _review_constraints_sent_for("specialist", fake_critic_root, fake_session_dir) + + rule = constraints["quantitative_claim_rule"] + assert rule["failure_verdict"] == "advise" + assert set(rule["forbidden_proposal_fields"]) == set(FORBIDDEN_PROPOSAL_FIELDS) + + +@pytest.mark.asyncio +async def test_a_review_the_rule_cannot_apply_to_is_not_handed_the_rule( + fake_critic_root: Path, + fake_session_dir: Path, +): + """The rule is about ``proposal_set[*]``, which a ``baseline`` proposal has + no room for. Sending it anyway invites a citation the verdict path then has + to read, so it goes only where it can be violated.""" + constraints = await _review_constraints_sent_for("baseline", fake_critic_root, fake_session_dir) + + assert "quantitative_claim_rule" not in constraints + + # Static context propagation — backend sources model/framework from manifest.json or explicit static_context. def _write_manifest(session_dir: Path, payload: dict[str, Any]) -> Path: """Write a minimal manifest.json the backend can ingest.""" diff --git a/src/hyperloom/inference_optimizer/tests/test_critic_verdict_map.py b/src/hyperloom/inference_optimizer/tests/test_critic_verdict_map.py index 9a5d256f5f..4f43cad89b 100644 --- a/src/hyperloom/inference_optimizer/tests/test_critic_verdict_map.py +++ b/src/hyperloom/inference_optimizer/tests/test_critic_verdict_map.py @@ -24,11 +24,22 @@ IntentValidationError, validate_envelope, ) +from hyperloom.orchestrator.loop.coordinator_helpers import ( + collapse_verdicts, + verdict_held_to_its_rule, + verdict_map_entry_grounds, + verdict_map_entry_held_to_its_rule, +) from hyperloom.orchestrator.policy.gate import ( + INTEGRATE_PATCH_PERMISSIVE_VERDICTS, PolicyDenied, PolicyGate, REVIEW_VERDICTS, ) +from hyperloom.orchestrator.specialists.patch_safety import ( + QUANTITATIVE_CLAIM_REASON_CODE, + cross_domain_rule_descriptors, +) # 1. intent_parser — envelope schema accepts verdict OR verdict_map @@ -179,6 +190,16 @@ def test_policy_gate_rejects_when_neither_present(gate): assert exc.value.rule == "payload" +def test_the_per_variant_shape_the_gate_teaches_carries_the_cited_rule(gate): + """The gate's hint is where the per-variant entry shape is spelled out for + the emitter, so it is where a variant learns it can name the rule its + verdict rests on rather than leaving that to prose.""" + with pytest.raises(PolicyDenied) as exc: + gate.validate_intent("critic", _critic_intent(target_proposal_msg_id="msg-1")) + + assert "failure_reason_code" in (exc.value.hint or "") + + def test_policy_gate_rejects_unknown_per_variant_verdict(gate): with pytest.raises(PolicyDenied) as exc: gate.validate_intent( @@ -506,6 +527,1175 @@ async def test_single_verdict_without_advisory_keeps_bare_payload(coord): assert "advice=" not in line +# 3b. A reject on a rule that asked for advice is held to that rule +@pytest.mark.asyncio +async def test_reject_on_an_advisory_only_rule_is_held_to_advise(coord): + """The quantitative-claim rule declares ``advise``; a reject citing it must not end the proposal.""" + pending = PendingProposal( + proposal_msg_id="msg-held", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-1"}}, + ) + coord.state.pending_proposals["msg-held"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-held", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + "reasoning": "proposal carried confidence", + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "advise" + # advise materialises, so the round keeps the proposal. + assert len(coord._materialise_calls) == 1 + assert [m for m in coord.bus.messages if m.topic == "review_verdict"][0].payload["verdict"] == "advise" + + +@pytest.mark.asyncio +async def test_a_held_reject_is_recorded_not_silently_corrected(coord, caplog): + """The downgrade leaves both a log line and an observation, so prompt drift stays visible.""" + import logging + + _seed_explore_proposal(coord, msg_id="msg-audit") + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-audit", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + }, + ) + with caplog.at_level(logging.WARNING, logger="hyperloom.orchestrator.loop.intent_router"): + await coord._handle_review_verdict("critic", intent) + assert any("held to its rule" in r.getMessage() for r in caplog.records) + kinds = [call.args[2].get("kind") for call in coord._record_observation.await_args_list] + assert "verdict_downgraded_to_rule_verdict" in kinds + + +@pytest.mark.asyncio +async def test_a_rule_named_only_in_prose_still_holds_the_verdict(coord): + """Field shape: the Critic names its rule in ``reasoning``, never in ``failure_reason_code``. + + ``failure_reason_code`` is an input descriptor — nothing on the output side + requires it — so a verdict that cites the rule in prose is in contract and + must move the same way one carrying the field does. + """ + pending = PendingProposal( + proposal_msg_id="msg-prose", + from_agent="orchestration", + action_name="framework_agent", + predicted_gain_pct=0.0, + payload={"action_name": "framework_agent", "params": {}}, + ) + coord.state.pending_proposals["msg-prose"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-prose", + "verdict": "reject", + "reasoning": ( + f"{QUANTITATIVE_CLAIM_REASON_CODE}: the proposal payload carries " + "the forbidden predicted_gain_pct field." + ), + "risks": [ + { + "severity": "blocker", + "summary": "Specialist proposal payload contains a prohibited quantitative claim field.", + } + ], + "notes": ["Resubmit without predicted_gain_pct or other prohibited quantitative ranking fields."], + "packet_evidence": ["payload.predicted_gain_pct"], + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + kinds = [call.args[2].get("kind") for call in coord._record_observation.await_args_list] + assert "verdict_downgraded_to_rule_verdict" in kinds + + +@pytest.mark.asyncio +async def test_prose_that_cites_no_rule_leaves_the_reject_alone(coord): + """Prose is scanned for a rule citation, not read for sentiment; an ordinary reject stands.""" + pending = PendingProposal( + proposal_msg_id="msg-prose-plain", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-4"}}, + ) + coord.state.pending_proposals["msg-prose-plain"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-prose-plain", + "verdict": "reject", + "reasoning": "the patch rewrites a kernel with no before/after benchmark to stand on.", + "notes": ["predicted_gain_pct was not the problem here."], + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_declared_reject_code_outranks_an_advisory_one_in_prose(coord): + """The field is the Critic's explicit citation; a citation in prose cannot soften a rule it declared ``reject``.""" + pending = PendingProposal( + proposal_msg_id="msg-both", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-5"}}, + ) + coord.state.pending_proposals["msg-both"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-both", + "verdict": "reject", + "failure_reason_code": "specialist_patch_not_grounded", + "reasoning": f"{QUANTITATIVE_CLAIM_REASON_CODE}: the payload carries predicted_gain_pct.", + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@dataclass +class _PatchVerdictSharedState(_BareSharedState): + """Adds the patch-verdict mirror the integrate_patch gate reads.""" + + patch_verdicts: dict[str, str] = field(default_factory=dict) + + def record_specialist_patch_verdict(self, specialist_task_id: str, verdict: str) -> None: + self.patch_verdicts[specialist_task_id] = verdict.strip().lower() + + +@pytest.mark.asyncio +async def test_a_held_reject_is_not_a_landing_permit(coord): + """``advise`` is an integrate_patch permit, so mirroring the held verdict + turned "the Critic rejected this patch" into "the Critic waved it through" + -- over a formatting rule, and irreversibly.""" + coord.shared_state = _PatchVerdictSharedState() + pending = PendingProposal( + proposal_msg_id="msg-permit", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-permit"}}, + ) + coord.state.pending_proposals["msg-permit"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-permit", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + assert coord.shared_state.patch_verdicts["t-permit"] == "reject" + assert coord.shared_state.patch_verdicts["t-permit"] not in INTEGRATE_PATCH_PERMISSIVE_VERDICTS + + +@pytest.mark.asyncio +async def test_a_held_variant_mirrors_the_verdict_the_critic_wrote(coord): + """The mirror is a landing permit for the specialist's patches, and the + per-variant path reaches it the same way the single one does: what the + Critic wrote is mirrored, whatever the hold made of it for this round.""" + coord.shared_state = _PatchVerdictSharedState() + pending = PendingProposal( + proposal_msg_id="msg-map-permit", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-map"}}, + ) + coord.state.pending_proposals["msg-map-permit"] = pending + entry = {"verdict": "reject", "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE} + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-permit", + "verdict_map": {"v_a": dict(entry), "v_b": dict(entry)}, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + assert coord.shared_state.patch_verdicts["t-map"] == "reject" + + +@pytest.mark.asyncio +async def test_an_unheld_verdict_still_mirrors_itself(coord): + """The mirror only diverges from the acted-on verdict when a hold moved it.""" + coord.shared_state = _PatchVerdictSharedState() + pending = PendingProposal( + proposal_msg_id="msg-plain-permit", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-plain"}}, + ) + coord.state.pending_proposals["msg-plain-permit"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={"target_proposal_msg_id": "msg-plain-permit", "verdict": "advise"}, + ) + await coord._handle_review_verdict("critic", intent) + + assert coord.shared_state.patch_verdicts["t-plain"] == "advise" + + +@pytest.mark.asyncio +async def test_a_reject_that_also_names_a_second_risk_is_not_held(coord): + """The hold answers "the whole reject was this one rule"; a verdict that + also refuses on its own merits keeps both halves of the sentence.""" + pending = PendingProposal( + proposal_msg_id="msg-mixed", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-mixed"}}, + ) + coord.state.pending_proposals["msg-mixed"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-mixed", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + "risks": [ + {"severity": "minor", "summary": "payload carries a self-reported gain."}, + {"severity": "blocker", "summary": "the patch can only be rolled back by hand."}, + ], + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_reject_still_asking_for_evidence_is_not_held(coord): + """An outstanding evidence request is a ground of its own: the Critic is + not complaining about a field, it is saying it cannot judge yet.""" + pending = PendingProposal( + proposal_msg_id="msg-eviden", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-evid"}}, + ) + coord.state.pending_proposals["msg-eviden"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-eviden", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + "required_evidence": ["matched_benchmark"], + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_rule_the_critic_cleared_is_not_the_grounds_for_its_reject(coord): + """The prose scan used to fire on any word-bounded mention, so a Critic that + checked the advisory rule, found it clean, and refused for a real reason had + that reason read as the formatting complaint -- and the proposal ran.""" + pending = PendingProposal( + proposal_msg_id="msg-cleared", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-cleared"}}, + ) + coord.state.pending_proposals["msg-cleared"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-cleared", + "verdict": "reject", + "reasoning": ( + f"Checked {QUANTITATIVE_CLAIM_REASON_CODE}: clean. Rejecting because the " + "patch rewrites a kernel with no before/after benchmark to stand on." + ), + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.parametrize( + "reasoning", + [ + pytest.param( + f"- {QUANTITATIVE_CLAIM_REASON_CODE}: clean.\n- rollback: absent. Rejecting on the rollback.", + id="enumerated_checklist", + ), + pytest.param( + f"> {QUANTITATIVE_CLAIM_REASON_CODE}: proposal_set[*] must not carry a self-reported gain field.\n" + "The proposal is clean on that rule; it is refused for lack of a rollback.", + id="quoted_rule_text", + ), + pytest.param( + f"```\n{QUANTITATIVE_CLAIM_REASON_CODE}: proposal_set[*] must not carry a self-reported gain field.\n" + "```\nRefused for lack of a rollback.", + id="fenced_rule_text", + ), + pytest.param( + f"Refused for lack of a rollback. For reference:\n{QUANTITATIVE_CLAIM_REASON_CODE}: not at issue here.", + id="cited_after_the_grounds", + ), + ], +) +def test_a_rule_a_verdict_enumerates_or_quotes_is_not_the_ground_it_rests_on(reasoning): + """A model that walks the rule list, or quotes a rule to say it does not + apply, writes the code in exactly the shapes a citation would take. Reading + one of those as grounds dispatches a proposal the Critic refused, so the + scan only fires on a citation opening the verdict's own prose.""" + entry = {"verdict": "reject", "reasoning": reasoning} + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ("reject", "") + + +def test_a_citation_opening_the_verdict_still_holds_it(): + """The shape the field verdict used stays readable: the code opens the + prose and a colon introduces the finding.""" + entry = { + "verdict": "reject", + "reasoning": f"`{QUANTITATIVE_CLAIM_REASON_CODE}`: the payload carries predicted_gain_pct.", + } + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ( + "advise", + QUANTITATIVE_CLAIM_REASON_CODE, + ) + + +def test_a_citation_in_either_field_the_entry_states_its_grounds_in_is_read(): + """The two prose keys are the same speaker's grounds for the same verdict -- + ``reasoning`` is how a single verdict spells them and ``rationale`` how a + variant does -- and nothing establishes a priority between them. Stopping at + the first key that says anything let a one-line pointer decide whether the + citation beside it was read at all.""" + entry = { + "verdict": "reject", + "reasoning": "See the per-variant notes.", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: the variant carries a self-reported gain.", + } + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ( + "advise", + QUANTITATIVE_CLAIM_REASON_CODE, + ) + + +def test_a_rule_named_in_a_remediation_note_is_not_a_citation(): + """``notes`` is where a model writes what to do next, including "this is not + a problem"; grounds are stated in ``reasoning``.""" + entry = { + "verdict": "reject", + "reasoning": "the benchmark is not comparable with the baseline.", + "notes": [f"{QUANTITATIVE_CLAIM_REASON_CODE}: nothing to fix on that front."], + } + assert verdict_held_to_its_rule(entry, action_name="specialist") == ("reject", "") + + +@pytest.mark.parametrize( + "reasoning", + [ + pytest.param( + f"{QUANTITATIVE_CLAIM_REASON_CODE}_v2: a successor rule, not this one.", + id="a_longer_identifier_the_code_only_starts", + ), + pytest.param( + f"{QUANTITATIVE_CLAIM_REASON_CODE}\u00a0: a non-breaking space, not the gap a citation leaves.", + id="a_space_that_is_not_the_gap_a_citation_leaves", + ), + ], +) +def test_a_code_the_colon_does_not_follow_is_not_a_citation(reasoning): + """Nothing may sit between the code and the colon but a backtick and an + ASCII gap, so an identifier the code merely starts is not a citation. The + gap stays ASCII on purpose: a line has already been through ``splitlines``, + which leaves only exotic unicode spaces for a wider class to add, and + stretching the scan to reach them would buy a guess at the cost of reading + citations that were never made. + + The other end of the anchor -- that nothing but whitespace or a backtick may + precede the code -- is what + ``test_a_rule_a_verdict_enumerates_or_quotes_is_not_the_ground_it_rests_on`` + covers, through the list and quote markers a model actually writes.""" + entry = {"verdict": "reject", "reasoning": reasoning} + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ("reject", "") + + +@pytest.mark.parametrize( + "risks", + [ + pytest.param("the patch does not apply and there is no rollback plan", id="one_sentence_not_a_list"), + pytest.param({"severity": "blocker", "summary": "the patch does not apply"}, id="one_risk_not_in_a_list"), + ], +) +def test_findings_stated_outside_the_shape_that_counts_them_are_not_one_ground(risks): + """The hold is confined to a reject naming at most one risk, which means + counting the ``risks`` list. A verdict that states its risks in some other + shape has stated grounds the count cannot be read off, and reading them as + one would hold the whole reject to whichever rule the prose cites.""" + entry = { + "verdict": "reject", + "reasoning": f"{QUANTITATIVE_CLAIM_REASON_CODE}: the payload carries predicted_gain_pct.", + "risks": risks, + } + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ("reject", "") + + +@pytest.mark.parametrize( + "findings", + [ + pytest.param( + {"risks": [{"severity": "blocker", "summary": "the payload carries a self-reported gain."}, ""]}, + id="a_ground_stated_beside_an_empty_slot_is_one_ground", + ), + pytest.param({"required_evidence": [""]}, id="an_empty_slot_is_not_evidence_the_verdict_still_wants"), + ], +) +def test_an_empty_findings_slot_states_nothing(findings): + """A verdict serialised with its list slots padded out has stated what is + in them, not how many there are -- the same reading + ``serialize_verdict_advisory`` takes of the field set downstream. Counting + an empty slot as a ground would withhold the downgrade from the shape the + hold was built for.""" + entry = { + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + **findings, + } + + assert verdict_held_to_its_rule(entry, action_name="specialist") == ( + "advise", + QUANTITATIVE_CLAIM_REASON_CODE, + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "findings", + [ + pytest.param({"risks": 1}, id="a_count_where_the_risks_go"), + pytest.param({"required_evidence": 1}, id="a_count_where_the_evidence_requests_go"), + ], +) +async def test_a_verdict_whose_findings_cannot_be_counted_still_decides_its_proposal(coord, findings): + """A number where the schema puts a list used to raise ``TypeError`` out of + the ground count. That reached the router's catch-all, which recorded the + exception and dropped the intent, leaving the proposal undecided for the + rest of the session. A count is not a citation the hold can act on, so the + verdict the Critic wrote is what the proposal is decided on.""" + pending = PendingProposal( + proposal_msg_id="msg-uncountable", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-uncountable"}}, + ) + coord.state.pending_proposals["msg-uncountable"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-uncountable", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + **findings, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.decided, pending.verdict) == (True, "reject") + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_held_reject_never_lands_the_patch_it_rejected(coord): + """The rules the hold enforces are about specialist proposal payloads, and + ``advise`` means "dispatch may proceed": holding an ``integrate_patch`` + reject would execute the patch the Critic refused, with the propose-time + PolicyGate patch gate already behind it.""" + coord.shared_state = _PatchVerdictSharedState() + pending = PendingProposal( + proposal_msg_id="msg-patch", + from_agent="orchestration", + action_name="integrate_patch", + predicted_gain_pct=0.0, + payload={"action_name": "integrate_patch", "params": {"specialist_task_id": "t-patch"}}, + ) + coord.state.pending_proposals["msg-patch"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-patch", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + assert coord.shared_state.patch_verdicts["t-patch"] == "reject" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("action_name", ["integrate_patch", "kernel_opt", "sweep"]) +async def test_a_reject_of_a_proposal_the_rules_do_not_govern_stands(coord, action_name): + """Every advisory rule is about a specialist-authored proposal payload, so a + reject of anything else cannot rest on one however the verdict is worded.""" + pending = PendingProposal( + proposal_msg_id="msg-ungoverned", + from_agent="orchestration", + action_name=action_name, + predicted_gain_pct=0.0, + payload={"action_name": action_name, "params": {}}, + ) + coord.state.pending_proposals["msg-ungoverned"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-ungoverned", + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_cross_domain_hint_reject_is_held_too(coord): + """Every rule declaring ``advise`` is covered, not just the quantitative-claim one.""" + reason_code = cross_domain_rule_descriptors()[0]["failure_reason_code"] + pending = PendingProposal( + proposal_msg_id="msg-xd", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-2"}}, + ) + coord.state.pending_proposals["msg-xd"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-xd", + "verdict": "reject", + "failure_reason_code": reason_code, + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "advise" + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "payload_extra", + [ + pytest.param({}, id="no_reason_code"), + # A code no rule declares as advisory — e.g. the safety hard guard, + # which critic.md keeps at ``reject``. + pytest.param({"failure_reason_code": "specialist_patch_not_grounded"}, id="code_outside_the_advisory_set"), + ], +) +async def test_a_substantive_reject_still_rejects(coord, payload_extra): + """The backstop is scoped to rules that declared ``advise``; every other reject stands.""" + pending = PendingProposal( + proposal_msg_id="msg-real", + from_agent="orchestration", + action_name="specialist", + predicted_gain_pct=0.0, + payload={"action_name": "specialist", "params": {"task_id": "t-3"}}, + ) + coord.state.pending_proposals["msg-real"] = pending + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-real", + "verdict": "reject", + **payload_extra, + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_one_variant_held_to_advise_does_not_out_rank_its_siblings(coord): + """The hold runs per variant before the collapse, so an advisory-only reject cannot discard the set.""" + pending = _seed_explore_proposal(coord, msg_id="msg-grid", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-grid", + "verdict_map": { + "v_a": {"verdict": "advise", "rationale": "worth a look"}, + "v_b": { + "verdict": "reject", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + # Without the per-entry hold, reject out-ranks advise and the set is lost. + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + + +@pytest.mark.asyncio +async def test_a_variant_citing_its_rule_in_the_key_the_entry_carries_is_held(coord): + """A ``verdict_map`` entry is ``{verdict, rationale?}`` — the shape PolicyGate + documents and every fixture uses. Scanning ``reasoning`` there left the + per-variant hold unreachable on the path it was written for.""" + pending = _seed_explore_proposal(coord, msg_id="msg-rationale", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-rationale", + "verdict_map": { + "v_a": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + "v_b": { + "verdict": "reject", + "rationale": (f"{QUANTITATIVE_CLAIM_REASON_CODE}: the variant carries a self-reported gain."), + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + + +@pytest.mark.asyncio +async def test_a_grid_rejected_only_on_advisory_rules_survives(coord): + """A whole map rejected on advisory-only grounds collapses to advise, not reject.""" + pending = _seed_explore_proposal(coord, msg_id="msg-grid-all", variants=["v_a", "v_b"]) + entry = {"verdict": "reject", "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE} + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-grid-all", + "verdict_map": {"v_a": dict(entry), "v_b": dict(entry)}, + }, + ) + await coord._handle_review_verdict("critic", intent) + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + + +@pytest.mark.asyncio +async def test_a_variant_reject_keeps_the_grounds_the_payload_states_for_it(coord): + """A ``verdict_map`` entry is ``{verdict, rationale?, failure_reason_code?}`` + -- it has no slot for ``required_evidence`` or ``risks``, which the schema + puts on the payload. Reading the hold's "rests on one ground" guard against + the entry alone made it structurally unfireable: a reject stating blockers + and asking for evidence was downgraded and its variant dispatched.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-grounds", variants=["v_a"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-grounds", + "required_evidence": ["matched benchmark", "rollback plan"], + "risks": [ + {"severity": "blocker", "summary": "the patch does not apply to the base checkout."}, + {"severity": "blocker", "summary": "there is no rollback plan."}, + {"severity": "minor", "summary": "the variant carries a self-reported gain."}, + ], + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": ( + f"{QUANTITATIVE_CLAIM_REASON_CODE}: the variant carries a self-reported " + "gain, and the patch it rests on does not apply." + ), + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_a_reject_code_the_payload_declares_outranks_a_variants_advisory_prose(coord): + """Declared-code precedence has to reach the batch path too: a + ``failure_reason_code`` naming a rule that asked for a reject is the + Critic's own citation, and a variant naming an advisory rule in prose + cannot soften it.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-declared", variants=["v_a"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-declared", + "failure_reason_code": "specialist_patch_not_grounded", + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: the variant carries a self-reported gain.", + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_the_one_risk_a_batch_states_is_not_the_rule_its_variants_cite(coord): + """A single stated risk is allowed beside a citation because on the single + path both come from one statement by one author. A batch states its risks + for the whole set and the citation belongs to the entry, so counting them + as one ground identifies them -- and a set refused for supplying no + rollback plan anywhere was dispatched on a formatting rule instead.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-one-blocker", variants=["v_a", "v_b"]) + entry = { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: this variant carries a self-reported gain field.", + } + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-one-blocker", + "risks": [{"severity": "blocker", "summary": "no variant in this set supplies a rollback plan."}], + "verdict_map": {"v_a": dict(entry), "v_b": dict(entry)}, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_a_variant_resting_only_on_the_cited_rule_still_gives_up_its_reject(coord): + """A finding is what holds a variant, not the batch's whole advisory + context: remediation notes and evidence pointers say what to do next and + where to look, so a variant resting on nothing but its cited rule still + gives up its reject.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-only", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-only", + "notes": ["Resubmit without predicted_gain_pct."], + "packet_evidence": ["proposal_set[0].predicted_gain_pct"], + "verdict_map": { + "v_a": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + "v_b": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: the variant carries a self-reported gain.", + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "advise" + assert len(coord._materialise_calls) == 1 + kinds = [call.args[2].get("kind") for call in coord._record_observation.await_args_list] + assert "verdict_downgraded_to_rule_verdict" in kinds + + +@pytest.mark.asyncio +async def test_the_verdicts_own_prose_does_not_supply_a_variants_citation(coord): + """Grounds stated once for the whole review are inherited; a citation is + not. The payload's prose speaks for the batch, and reading it as one + variant's grounds would downgrade a reject whose own rationale refuses on + something else entirely.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-prose", variants=["v_a"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-prose", + "reasoning": f"{QUANTITATIVE_CLAIM_REASON_CODE}: two of the variants carry a self-reported gain.", + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": "this variant has no rollback plan and its patch does not apply.", + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +async def test_the_batchs_declared_advisory_code_does_not_supply_a_variants_citation(coord): + """The batch's citation is not the variant's either. A declared code + outranks prose, so inheriting one would not merely compete with the + variant's own rationale — it would stop it being read at all, and downgrade + a reject that refuses on grounds no advisory rule ever asked advice for.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-field", variants=["v_a"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-field", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": "this variant has no rollback plan and its patch does not apply.", + }, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert pending.verdict == "reject" + assert coord._materialise_calls == [] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("rejected", "risks"), + [ + pytest.param( + "v_a", + [ + {"severity": "blocker", "summary": "no variant in this set gives a rollback plan (v_b included)."}, + {"severity": "blocker", "summary": "none name the active path the flag touches; v_b is typical."}, + ], + id="prose_naming_a_sibling_as_its_example", + ), + pytest.param( + "v_a", + [ + {"severity": "blocker", "summary": "v_a: no rollback plan."}, + {"severity": "blocker", "summary": "v_a does not name the active path the flag touches."}, + ], + id="prose_naming_the_rejected_variant", + ), + pytest.param( + "v_a", + [ + {"severity": "blocker", "summary": "the second variant gives no rollback plan; nor do the rest."}, + {"severity": "blocker", "summary": "none name the active path the flag touches."}, + ], + id="prose_naming_no_key_at_all", + ), + pytest.param( + "naïve", + [ + {"severity": "blocker", "summary": "naïve and v_b: neither gives a rollback plan."}, + {"severity": "blocker", "summary": "neither names the active path the flag touches."}, + ], + id="prose_naming_a_key_no_json_escape_survives", + ), + ], +) +async def test_a_batch_blocker_binds_every_variant_whatever_name_its_prose_carries(coord, rejected, risks): + """A batch states its blockers once, in prose written for the whole set, + and nothing asks the Critic to file one under a variant's key. Reading a + name in that prose as "about that one, not you" removed grounds the Critic + stated and dispatched a set it refused; reading it the other way would let + the hold turn on a spelling. Neither is read: the batch's grounds hold + every variant that states none of its own.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-example", variants=[rejected, "v_b", "v_c"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-example", + "risks": risks, + "verdict_map": { + rejected: { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + }, + "v_b": {"verdict": "needs_review", "rationale": "re-run once the patch applies"}, + "v_c": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_a_batch_finding_is_not_disowned_by_a_sibling_named_in_a_neighbouring_field(coord): + """A finding is a dict of several fields, and only one of them states what + the finding *is*. A remediation pointing at how a sibling was fixed, or an + evidence request phrased by example, says nothing about who the finding + binds.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-fields", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-fields", + "required_evidence": ["a rollback plan of the kind v_b supplies"], + "risks": [ + { + "severity": "blocker", + "summary": "the patch does not apply to the base checkout.", + "required_fix": "rebase it the way v_b was rebased.", + }, + ], + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + }, + "v_b": {"verdict": "needs_review", "rationale": "re-run once the patch applies"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_grounds_stated_in_a_shape_the_schema_does_not_use_are_not_read_as_one(coord): + """``risks`` is a list in the schema. A verdict that states it as one + sentence has stated grounds whose count cannot be read off, and "the patch + does not apply and there is no rollback plan" is two of them -- so it + cannot be counted as the single ground the hold is confined to.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-scalar", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-scalar", + "risks": "the patch does not apply and there is no rollback plan", + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + }, + "v_b": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_evidence_the_batch_still_wants_holds_a_variant_resting_on_a_cited_rule(coord): + """``required_evidence`` is the stronger of the two inherited findings -- + one item is a ground where ``risks`` needs two -- and a batch states it in + the same place, for variants whose own shape has no slot for it.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-evidence", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-evidence", + "required_evidence": ["a matched benchmark for every variant here"], + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + }, + "v_b": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_a_variant_that_states_a_finding_of_its_own_still_answers_for_the_batchs(coord): + """Filing a risk under a variant's key says that risk is about it; it does + not say the blocker the batch filed under no key is about someone else. + Letting the entry's own list stand in for the batch's would hand a variant + its downgrade back by writing one minor risk next to it.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-both", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-both", + "risks": [{"severity": "blocker", "summary": "nothing here has a rollback plan."}], + "verdict_map": { + "v_a": { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + "risks": [{"severity": "minor", "summary": "carries a self-reported gain field."}], + }, + "v_b": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_a_variant_stating_no_grounds_does_not_borrow_the_batchs_advisory_citation(coord): + """An entry that states nothing but its verdict has cited no rule. The + batch's advisory code is the batch's citation, and lending it to that + entry would downgrade a reject on grounds nobody wrote down.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-bare", variants=["v_a", "v_b"]) + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-bare", + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + "verdict_map": { + "v_a": {"verdict": "reject"}, + "v_b": {"verdict": "needs_review", "rationale": "no prior on this flag"}, + }, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.asyncio +async def test_a_batch_code_no_rule_declares_withholds_the_downgrade(coord): + """A code the Critic filled in with something no rule defines is not a + citation the hold can act on, and it is not permission to dispatch either: + it withholds the downgrade, at the cost this reading accepts -- the set + keeps the reject it would otherwise have given up.""" + pending = _seed_explore_proposal(coord, msg_id="msg-map-junk", variants=["v_a", "v_b"]) + entry = { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + } + intent = Intent( + type=IntentType.REVIEW_VERDICT, + payload={ + "target_proposal_msg_id": "msg-map-junk", + "failure_reason_code": "N/A", + "verdict_map": {"v_a": dict(entry), "v_b": dict(entry)}, + }, + ) + await coord._handle_review_verdict("critic", intent) + + assert (pending.verdict, len(coord._materialise_calls)) == ("reject", 0) + + +@pytest.mark.parametrize( + ("entry", "payload", "expected"), + [ + pytest.param( + {"verdict": "reject"}, + {"risks": [{"severity": "blocker"}], "required_evidence": ["a bench"]}, + {"verdict": "reject"}, + id="the_batchs_findings_are_not_moved_onto_the_entry", + ), + pytest.param( + {"verdict": "reject", "failure_reason_code": "variant_code"}, + {"failure_reason_code": "payload_code"}, + {"verdict": "reject", "failure_reason_code": "variant_code"}, + id="the_entrys_own_statement_of_a_ground_wins", + ), + pytest.param( + {"verdict": "reject", "rationale": "the variant's own grounds"}, + {"reasoning": "the batch's grounds"}, + {"verdict": "reject", "rationale": "the variant's own grounds"}, + id="prose_is_not_inherited", + ), + pytest.param( + {"verdict": "reject", "rationale": "the variant's own grounds"}, + {"failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE}, + {"verdict": "reject", "rationale": "the variant's own grounds"}, + id="a_code_that_could_soften_the_reject_is_not_inherited_either", + ), + pytest.param( + {"verdict": "reject", "rationale": "the variant's own grounds"}, + {"failure_reason_code": "specialist_patch_not_grounded"}, + { + "verdict": "reject", + "rationale": "the variant's own grounds", + "failure_reason_code": "specialist_patch_not_grounded", + }, + id="a_code_that_can_only_hold_the_reject_is_inherited", + ), + pytest.param( + {"verdict": "reject", "risks": [{"summary": "its own"}]}, + {"risks": [{"summary": "the batch's"}]}, + {"verdict": "reject", "risks": [{"summary": "its own"}]}, + id="an_entrys_own_findings_are_left_as_the_entry_stated_them", + ), + pytest.param("reject", {"failure_reason_code": "payload_code"}, {}, id="a_non_dict_entry_states_nothing"), + ], +) +def test_an_entry_inherits_only_a_code_that_can_hold_its_reject(entry, payload, expected): + """A finding the batch states is read where it is stated, as a hold on the + whole set; moving one onto an entry would put it back in the count the + ``<= 1`` allowance runs on, which is what identified the batch's ground + with the entry's rule.""" + assert verdict_map_entry_grounds(entry, payload) == expected + + +def test_a_batch_that_states_nothing_readable_leaves_its_entry_on_its_own_grounds(): + """Both halves of the batch reading -- what an entry inherits and what + holds it -- ask a payload what it states, so both have to answer for one + that is not a payload at all. It states no findings and no code, which + leaves the entry judged on what it wrote itself.""" + entry = { + "verdict": "reject", + "rationale": f"{QUANTITATIVE_CLAIM_REASON_CODE}: carries a self-reported gain field.", + } + + assert verdict_map_entry_held_to_its_rule(entry, "not a payload", action_name="specialist") == ( + "advise", + QUANTITATIVE_CLAIM_REASON_CODE, + ) + + +def test_a_map_of_verdicts_none_of_which_decides_asks_for_review(): + """``redirect`` is a legal verdict with no place in the collapse order, so a + map carrying only those decides nothing and must fall back to review.""" + assert collapse_verdicts(["redirect", ""]) == "needs_review" + + +def test_an_empty_map_asks_for_review(): + assert collapse_verdicts([]) == "needs_review" + + +@pytest.mark.parametrize( + ("verdicts", "expected"), + [ + pytest.param(["approve", "reject", "advise", "needs_review"], "approve", id="one_approve_carries_it"), + pytest.param(["reject", "advise", "needs_review"], "reject", id="reject_outranks_advice"), + pytest.param(["advise", "needs_review"], "advise", id="advice_outranks_more_review"), + ], +) +def test_the_collapse_keeps_its_priority_order(verdicts, expected): + assert collapse_verdicts(verdicts) == expected + + # 4. _materialize_approved_proposal — filter semantics (unit) @pytest.mark.asyncio async def test_materialize_filter_drops_rejected_variants(tmp_path: Path): diff --git a/src/hyperloom/inference_optimizer/tests/test_specialist_patch_safety_unit.py b/src/hyperloom/inference_optimizer/tests/test_specialist_patch_safety_unit.py index 71fddd4e24..1658149c7e 100644 --- a/src/hyperloom/inference_optimizer/tests/test_specialist_patch_safety_unit.py +++ b/src/hyperloom/inference_optimizer/tests/test_specialist_patch_safety_unit.py @@ -7,6 +7,8 @@ from __future__ import annotations +import pytest + from hyperloom.orchestrator.specialists import patch_safety as ps @@ -163,25 +165,157 @@ def test_patch_safety_report_notes_empty(): assert ps.PatchSafetyReport().notes() == [] -# ---- scan_quantitative_claims --------------------------------------------- -def test_scan_quantitative_claims(): +# ---- scan_numeric_claims --------------------------------------------------- +def test_scan_numeric_claims(): payload = { - "confidence": 0.9, "summary": "gives 20% boost", "proposal_set": [ - {"score": 1, "expected_qualitative_argument": "3x faster"}, + {"expected_qualitative_argument": "3x faster"}, "not-a-dict", ], } - forbidden, warnings = ps.scan_quantitative_claims(payload) - assert "confidence" in forbidden - assert "score" in forbidden + warnings = ps.scan_numeric_claims(payload) assert any("%" in w for w in warnings) assert any("x" in w.lower() for w in warnings) -def test_scan_quantitative_claims_empty(): - assert ps.scan_quantitative_claims({}) == ([], []) +def test_scan_numeric_claims_empty(): + assert ps.scan_numeric_claims({}) == [] + + +def test_the_numeric_scan_answers_only_the_question_no_one_else_answers(): + """It used to return the same ``keys & FORBIDDEN_*`` intersection + ``strip_forbidden_proposal_fields`` computes, for a caller that discarded + it: one question with two implementations, free to drift apart. The numbers + in the prose are what this scan alone finds.""" + payload = { + "expected_gain": 12.0, + "summary": "gives 20% boost", + "proposal_set": [{"score": 1, "confidence": 0.9}], + } + + assert ps.scan_numeric_claims(payload) == ["20%"] + + +# ---- strip_forbidden_proposal_fields -------------------------------------- +def test_round_level_confidence_is_not_a_per_proposal_gain_claim(): + """The output schema asks for a round-level self-assessment and the round + audit records it, so stripping it at the top level only made our own + template a violation. Per proposal it is the ranking claim the guard is + about, and one function now decides both.""" + payload = {"confidence": 0.6, "proposal_set": [{"confidence": 0.4}]} + + assert ps.strip_forbidden_proposal_fields(payload) == ["confidence"] + assert payload["confidence"] == 0.6 + assert payload["proposal_set"][0] == {} + + + +def test_forbidden_fields_are_stripped_so_the_critic_cannot_reject_on_format(): + payload = { + "expected_gain": 9.0, + "confidence": 0.6, + "summary": "keep me", + "proposal_set": [ + {"name": "v1", "confidence": 0.4, "score": 3, "reason": "keep me too"}, + "not-a-dict", + ], + } + + removed = ps.strip_forbidden_proposal_fields(payload) + + assert set(removed) == {"expected_gain", "confidence", "score"} + assert "expected_gain" not in payload + assert payload["confidence"] == 0.6 # round-level self-assessment survives + assert payload["summary"] == "keep me" + assert payload["proposal_set"][0] == {"name": "v1", "reason": "keep me too"} + assert payload["proposal_set"][1] == "not-a-dict" + + +def test_a_gain_claim_under_the_coordinators_own_field_name_is_stripped_too(): + """``predicted_gain_pct`` is the Coordinator's estimate on a propose_action + intent, which is exactly what made it a convenient place for a specialist + to put a number the guard was meant to strip.""" + payload = {"proposal_set": [{"name": "v1", "predicted_gain_pct": 12.0, "reason": "keep me"}]} + + removed = ps.strip_forbidden_proposal_fields(payload) + + assert removed == ["predicted_gain_pct"] + assert payload["proposal_set"][0] == {"name": "v1", "reason": "keep me"} + + +def test_stripping_a_clean_payload_changes_nothing(): + payload = {"proposal_set": [{"name": "v1", "reason": "why"}]} + + assert ps.strip_forbidden_proposal_fields(payload) == [] + assert payload == {"proposal_set": [{"name": "v1", "reason": "why"}]} + + +@pytest.mark.parametrize("payload", [None, [], "", 0]) +def test_stripping_tolerates_a_payload_that_is_not_a_dict(payload): + assert ps.strip_forbidden_proposal_fields(payload) == [] + + +# ---- quantitative_claim_rule_descriptor ------------------------------------ +def test_the_rule_the_critic_gets_lists_exactly_what_the_runner_strips(): + """A hand-copied field list in the prompt is how the Critic came to reject + over a field the runner never enforced.""" + rule = ps.quantitative_claim_rule_descriptor() + + assert set(rule["forbidden_proposal_fields"]) == set(ps.FORBIDDEN_PROPOSAL_FIELDS) + + +def test_a_format_slip_is_advisory_not_a_reject(): + rule = ps.quantitative_claim_rule_descriptor() + + assert rule["failure_verdict"] == "advise" + assert rule["failure_reason_code"] == ps.QUANTITATIVE_CLAIM_REASON_CODE + + +# ---- advisory_only_reason_codes -------------------------------------------- +def test_every_rule_that_asked_for_advice_is_enforceable(): + codes = ps.advisory_only_reason_codes() + + assert ps.QUANTITATIVE_CLAIM_REASON_CODE in codes + for rule in ps.cross_domain_rule_descriptors(): + if rule["failure_verdict"] == ps.ADVISE_VERDICT: + assert rule["failure_reason_code"] in codes + + +def test_a_rule_asking_for_a_reject_is_left_alone(monkeypatch): + """The set is derived from the descriptors, so a rule keeping ``reject`` stays out of it.""" + hard = ps.CrossDomainRule( + rule_id="hard_guard", + description="a violation here is grounds for refusal", + failure_verdict="reject", + failure_reason_code="cross_domain_hard_guard", + ) + monkeypatch.setattr(ps, "CROSS_DOMAIN_RULES", ps.CROSS_DOMAIN_RULES + (hard,)) + + codes = ps.advisory_only_reason_codes() + + assert "cross_domain_hard_guard" not in codes + assert ps.QUANTITATIVE_CLAIM_REASON_CODE in codes + + +def test_the_codes_carry_no_blank_entry(): + assert "" not in ps.advisory_only_reason_codes() + + +# ---- advisory_rules_govern ------------------------------------------------- +@pytest.mark.parametrize("action_name", ["specialist", "explore", "framework_agent"]) +def test_the_rules_govern_the_proposal_kinds_they_are_written_about(action_name): + """``proposal_set[*]`` reaches review as a specialist proposal or the explore + grid it is materialised into; the framework candidate is the one the + quantitative-claim rule names by exception.""" + assert ps.advisory_rules_govern(action_name) is True + + +@pytest.mark.parametrize("action_name", ["integrate_patch", "kernel_opt", "sweep", "baseline", "", None]) +def test_integrate_patch_is_never_governed_by_an_advisory_rule(action_name): + """None of these carries a specialist ``proposal_set``, and holding an + ``integrate_patch`` reject to ``advise`` would land the refused patch.""" + assert ps.advisory_rules_govern(action_name) is False # ---- vet_patches ---------------------------------------------------------- diff --git a/src/hyperloom/inference_optimizer/tests/test_specialist_prompt_builder_coverage_unit.py b/src/hyperloom/inference_optimizer/tests/test_specialist_prompt_builder_coverage_unit.py index 8e0016e031..4da7e7aeff 100644 --- a/src/hyperloom/inference_optimizer/tests/test_specialist_prompt_builder_coverage_unit.py +++ b/src/hyperloom/inference_optimizer/tests/test_specialist_prompt_builder_coverage_unit.py @@ -173,6 +173,21 @@ def test_cold_start_directive(): assert "COLD-START MODE" in user_p +def test_cold_start_does_not_ask_for_a_field_the_safety_gate_forbids(): + """It used to direct the fallback proposals to carry ``confidence: low`` -- + a field in FORBIDDEN_PROPOSAL_FIELDS -- so a compliant specialist tripped + the guard on exactly the round where it was the only source of ideas.""" + inp = SpecialistPromptInputs( + task_id="t", + domain=get_domain("serving_specialist"), + ) + _, user_p = build_specialist_prompts(inp) + + cold_start = user_p[user_p.index("COLD-START MODE") :] + assert "confidence: low" not in cold_start + assert "provenance: domain_focus_default" in cold_start + + def test_research_hints_fallback(): inp = SpecialistPromptInputs( task_id="t", diff --git a/src/hyperloom/inference_optimizer/tests/test_specialist_runner_helpers_unit.py b/src/hyperloom/inference_optimizer/tests/test_specialist_runner_helpers_unit.py index 9a59956988..f04735efca 100644 --- a/src/hyperloom/inference_optimizer/tests/test_specialist_runner_helpers_unit.py +++ b/src/hyperloom/inference_optimizer/tests/test_specialist_runner_helpers_unit.py @@ -220,6 +220,59 @@ def test_write_specialist_done_partial(tmp_path): assert "ts" in payload +def _finalize(r, tmp_path, payload): + """Drive ``_finalize`` far enough to inspect the artifact it writes.""" + prep = sr._PreparedRun( + domain=SimpleNamespace(key="serving_specialist"), + gap="gap-1", + workspace=tmp_path, + ) + ctx = SimpleNamespace(task=SimpleNamespace(task_id="t1", params={}), extra={}) + result = r._finalize( + ctx=ctx, + prep=prep, + specialist_done_payload=payload, + turns_used=1, + tool_violations=[], + backend_error="", + extra_notes=[], + patches_written=[], + ) + return result, json.loads((tmp_path / "specialist_done.json").read_text(encoding="utf-8")) + + +def test_finalize_strips_forbidden_fields_before_the_critic_can_see_them(tmp_path): + """The Critic is told to reject a proposal_set carrying self-reported gain + fields, which costs the round every idea in it. Dropping them makes that + verdict unreachable; the audit note still records what was there.""" + result, written = _finalize( + _runner(), + tmp_path, + { + "proposal_set": [{"name": "v1", "reason": "why", "expected_gain": 8.0, "score": 2}], + "summary": "s", + }, + ) + + proposal = written["proposal_set"][0] + assert "expected_gain" not in proposal + assert "score" not in proposal + assert proposal["name"] == "v1" and proposal["reason"] == "why" + joined = "\n".join(result.notes) + assert "patch_safety_forbidden_fields" in joined + assert "expected_gain" in joined and "score" in joined + + +def test_finalize_keeps_the_round_level_confidence_the_audit_records(tmp_path): + _, written = _finalize( + _runner(), + tmp_path, + {"proposal_set": [{"name": "v1"}], "confidence": 0.6}, + ) + + assert written["confidence"] == 0.6 + + def test_write_specialist_done_partial_noop_none_workspace(): _runner()._write_specialist_done_partial(None, {"a": 1}) # must not raise diff --git a/src/hyperloom/orchestrator/loop/coordinator_helpers.py b/src/hyperloom/orchestrator/loop/coordinator_helpers.py index 186dde663f..9ee3c8592c 100644 --- a/src/hyperloom/orchestrator/loop/coordinator_helpers.py +++ b/src/hyperloom/orchestrator/loop/coordinator_helpers.py @@ -13,11 +13,19 @@ import json import logging import os +import re import shlex +from collections.abc import Iterable from datetime import datetime, timezone from pathlib import Path from typing import Any +from ..specialists.patch_safety import ( + ADVISE_VERDICT, + advisory_only_reason_codes, + advisory_rules_govern, +) + log = logging.getLogger(__name__) # Constants below are read from other modules; listed here to mark them as @@ -497,6 +505,10 @@ def _dedupe_extra_server_args(args_str: str) -> str: "kb_evidence", "packet_evidence", ) +# The verdict that ends a proposal's life; its counterpart ``ADVISE_VERDICT`` +# lets the proposal through. See :func:`verdict_held_to_its_rule`. +_REJECT_VERDICT: str = "reject" + _VERDICT_ADVISORY_TEXT_KEYS: tuple[str, ...] = ( "advice_text", "alternative_action", @@ -541,6 +553,361 @@ def serialize_verdict_advisory(payload: dict[str, Any]) -> dict[str, Any]: return out +# The fields a Critic states its grounds in: ``reasoning`` on a single verdict, +# ``rationale`` on one ``verdict_map`` entry — the per-variant shape PolicyGate +# documents and every fixture uses. ``notes`` is remediation text and +# ``risks[*].summary`` describes the risk, so a rule named in either is being +# discussed rather than invoked; both stay out. +# +# Every key an entry fills is read. They are one speaker's grounds for one +# verdict and nothing ranks them, so trying them in order would let whichever +# happens to come first decide whether the citation in the other is seen. +_VERDICT_PROSE_KEYS: tuple[str, ...] = ("reasoning", "rationale") + +# What a citation looks like: the code opens the verdict's grounds and a colon +# introduces the finding, the shape the field verdict used -- +# ``"specialist_quantitative_claim_violation: the proposal payload carries the +# forbidden predicted_gain_pct field."`` Nothing may precede the code but +# whitespace or a backtick, and only the opening line of each prose field is +# read. +# +# The two mistakes cost different amounts. Missing a citation costs the round +# its proposals, which the next round can re-propose; reading one that was not +# made dispatches a proposal the Critic meant to block. So the scan stays +# deliberately narrow instead of learning every citation format a model might +# use -- a list marker, a quote marker or a fence is how one *enumerates* the +# rules it checked, and "- : clean." must never read as grounds. The +# reliable path is the explicit ``failure_reason_code`` in the Critic's output +# schema; this is the fallback. +_CITATION_OPENER: str = r"[ \t]*`?" + + +def _opening_prose_lines(entry: dict[str, Any]) -> list[str]: + """Return the opening line of each prose field ``entry`` states grounds in. + + Args: + entry: A ``review_verdict`` payload or one ``verdict_map`` entry; the + two spell the field differently (:data:`_VERDICT_PROSE_KEYS`), and + an entry filling both states grounds in both. + + Returns: + The first non-blank line of each field present, in + :data:`_VERDICT_PROSE_KEYS` order; empty when the entry states no + grounds in prose. + """ + openings: list[str] = [] + for key in _VERDICT_PROSE_KEYS: + for line in str(entry.get(key) or "").splitlines(): + if line.strip(): + openings.append(line) + break + return openings + + +def cited_advisory_reason_code(entry: dict[str, Any]) -> str: + """Return the advisory-only rule ``entry`` cites, from the field or its prose. + + ``failure_reason_code`` is the reliable path: the Critic's output schema + asks for the code of the rule its verdict rests on. Prose is read only as a + fallback, for a verdict that names its rule in its grounds text instead — + the shape observed in the field. + + A mention is not a citation: a Critic that clears one rule and refuses on + another names both, and reading the cleared one as the grounds would + materialise a proposal it meant to block. Only an unambiguous citation + counts (see :data:`_CITATION_OPENER`). + + Args: + entry: A ``review_verdict`` payload or one ``verdict_map`` entry. + + Returns: + The cited advisory-only reason code, or ``""`` when the entry cites + none. A code outside the advisory set yields ``""`` too: only rules + that declared ``advise`` can move a verdict. + """ + advisory = advisory_only_reason_codes() + explicit = str(entry.get("failure_reason_code") or "").strip() + if explicit: + return explicit if explicit in advisory else "" + # At most one code can open one line, so the sort only fixes the order the + # candidates are tried in. + for opening in _opening_prose_lines(entry): + for code in sorted(advisory): + if re.match(rf"{_CITATION_OPENER}{re.escape(code)}`?[ \t]*:", opening): + return code + return "" + + +# Priority a batch of per-variant verdicts collapses by: one approved variant +# carries the proposal, otherwise one reject sinks it, and advice outranks a +# request for more review. +_VERDICT_COLLAPSE_ORDER: tuple[str, ...] = ("approve", _REJECT_VERDICT, ADVISE_VERDICT, "needs_review") + + +def collapse_verdicts(verdicts: Iterable[str]) -> str: + """Collapse per-variant verdicts into the one the proposal is decided on. + + Args: + verdicts: The per-variant verdicts of one ``verdict_map``. + + Returns: + The highest-priority verdict present, or ``needs_review`` when none of + the known verdicts appears. + """ + present = set(verdicts) + for candidate in _VERDICT_COLLAPSE_ORDER: + if candidate in present: + return candidate + return "needs_review" + + +def _states_findings(value: Any) -> bool: + """Return whether a findings field states anything at all. + + Args: + value: A ``risks`` or ``required_evidence`` value: the list the schema + documents, or whatever shape a verdict put there instead. + + Returns: + True when a list holds at least one non-empty item, or when a value of + any other shape is non-empty. + """ + if isinstance(value, (list, tuple)): + return any(bool(item) for item in value) + return bool(value) + + +def verdict_rests_on_one_ground(entry: dict[str, Any]) -> bool: + """Return whether ``entry`` refuses for a single reason. + + A verdict can cite an advisory rule *and* refuse on its own merits in the + same breath — "the proposal claims a 12% gain and has no rollback plan". + Holding that verdict to the advisory rule would let the second half of the + sentence disappear, so the hold is confined to a reject that names one + ground and asks for nothing further: at most one risk entry, and no + outstanding evidence request. + + The allowance rests on the citation and the risk being one statement by one + author, which is what makes the risk the cited rule's. Findings a *batch* + states are neither, so they are read where they are stated rather than + counted here (:func:`verdict_map_entry_held_to_its_rule`). + + ``risks`` is a list in the schema. A verdict that states it as one sentence + instead has stated grounds whose number cannot be read off — "the patch + does not apply and there is no rollback plan" is two — so an unlisted + value counts as more than one rather than as the single ground the hold is + confined to. + + Risk *severity* deliberately plays no part. ``references/risk_rules.md`` + reserves ``blocker`` for evidence and correctness failures and lists no + format item, yet the Critic verdict this hold was built for graded its own + format complaint ``blocker`` — so severity separates nothing here, and + reading it would only retire the hold on the case that motivated it. + + Args: + entry: A ``review_verdict`` payload or one ``verdict_map`` entry. + + Returns: + True when the verdict names at most one ground and requests no further + evidence. + """ + if _states_findings(entry.get("required_evidence")): + return False + risks = entry.get("risks") + if not isinstance(risks, (list, tuple)): + return not _states_findings(risks) + return len([risk for risk in risks if risk]) <= 1 + + +# The findings a review lists outside its prose: the evidence it still wants +# and the risks it names. A ``verdict_map`` entry is +# ``{verdict, rationale?, failure_reason_code?}`` -- the shape PolicyGate +# documents -- so these have nowhere to live but the payload, where a batch +# review states them once for every variant it looked at. +_VERDICT_FINDING_KEYS: tuple[str, ...] = ("required_evidence", "risks") + + +def _batch_states_findings(payload: dict[str, Any]) -> bool: + """Return whether a batch review states a finding of its own. + + Args: + payload: The ``review_verdict`` payload a ``verdict_map`` arrived in. + + Returns: + True when the payload names any risk or asks for any evidence. + """ + if not isinstance(payload, dict): + return False + return any(_states_findings(payload.get(key)) for key in _VERDICT_FINDING_KEYS) + + +def _inheritable_reason_code(payload: dict[str, Any]) -> str: + """Return the payload's declared code, when it cannot soften a variant's reject. + + Any code outside the advisory set is inherited, including one no rule + defines. Restricting this to codes a handed rule declares would inherit + nothing at all -- every rule in ``review_constraints`` declares ``advise`` + (:func:`advisory_only_reason_codes`), so the two sets do not intersect -- + and would let a batch that declared a hard code have its variants + downgraded on the advisory rules their rationales cite. An unrecognised + string is not the Critic's permission to dispatch; it withholds the + downgrade, which is what the batch's own findings do. + + Args: + payload: The ``review_verdict`` payload. + + Returns: + The declared ``failure_reason_code``, or ``""`` when it names an + advisory-only rule. + """ + code = str(payload.get("failure_reason_code") or "").strip() + return "" if code in advisory_only_reason_codes() else code + + +def verdict_map_entry_grounds(entry: dict[str, Any], payload: dict[str, Any]) -> dict[str, Any]: + """Return the grounds one ``verdict_map`` entry rests on. + + An entry is ``{verdict, rationale?, failure_reason_code?}`` -- the shape + PolicyGate documents -- so what it states of its own is nearly all it has. + Prose is not inherited, and neither is a ``failure_reason_code`` naming an + advisory rule: the batch's citation is a claim about the batch's verdict, + and a declared code outranks the entry's own rationale rather than + competing with it, so lending one would stop the entry's grounds being read + at all. A code that can only withhold the downgrade is inherited + (:func:`_inheritable_reason_code`), because the two mistakes cost different + amounts (see :data:`_CITATION_OPENER`). + + The findings a batch states are not inherited either. They are read where + they are stated, as a hold on every entry in the set + (:func:`verdict_map_entry_held_to_its_rule`). + + Args: + entry: One ``verdict_map`` entry. + payload: The ``review_verdict`` payload that entry arrived in. + + Returns: + The entry's own keys, plus a declared reason code that can only hold its + reject; ``{}`` when ``entry`` is not a dict. + """ + if not isinstance(entry, dict): + return {} + grounds = dict(entry) + if not isinstance(payload, dict): + return grounds + if not grounds.get("failure_reason_code"): + code = _inheritable_reason_code(payload) + if code: + grounds["failure_reason_code"] = code + return grounds + + +def _stated_verdict(entry: dict[str, Any]) -> str: + """Return the verdict ``entry`` states, whatever a hold makes of it. + + Args: + entry: A ``review_verdict`` payload or one ``verdict_map`` entry. + + Returns: + The stated verdict, stripped; ``""`` when the entry states none. + """ + return str(entry.get("verdict") or "").strip() + + +def verdict_held_to_its_rule(entry: dict[str, Any], *, action_name: str) -> tuple[str, str]: + """Return the verdict a ``review_verdict`` entry carries, and why it moved. + + Several review rules declare ``advise`` as their failure verdict precisely + because rejecting on them costs the round every proposal in the set. That + declaration is prose in the Critic prompt, so a model that rejects anyway + silently gets its way. This holds the verdict to what the cited rule asked + for, which makes the declaration enforceable rather than advisory. + + The hold is narrow by construction: it reaches only the proposal kinds those + rules are about (:func:`advisory_rules_govern`), and only a reject whose + *only* stated ground is a rule that asked for advice (see + :func:`verdict_rests_on_one_ground`). Scoping it by proposal kind is what + keeps ``advise`` — which means "dispatch may proceed" — from executing an + ``integrate_patch`` the Critic refused, since the propose-time + ``PolicyGate`` patch gate does not run a second time on the verdict. + + Args: + entry: A ``review_verdict`` payload or one ``verdict_map`` entry, with + a ``verdict`` and the rule it cites — in ``failure_reason_code`` or + in its own prose (see :func:`cited_advisory_reason_code`). + action_name: The reviewed proposal's action name. + + Returns: + A ``(verdict, reason_code)`` pair: the verdict to act on, and the cited + reason code when it forced a downgrade, else an empty string. + """ + if not isinstance(entry, dict): + return "", "" + verdict = _stated_verdict(entry) + if verdict != _REJECT_VERDICT: + return verdict, "" + if not advisory_rules_govern(action_name): + return verdict, "" + if not verdict_rests_on_one_ground(entry): + return verdict, "" + reason_code = cited_advisory_reason_code(entry) + if reason_code: + return ADVISE_VERDICT, reason_code + return verdict, "" + + +def verdict_map_entry_held_to_its_rule( + entry: dict[str, Any], + payload: dict[str, Any], + *, + action_name: str, +) -> tuple[str, str]: + """Return the verdict one ``verdict_map`` entry carries, and why it moved. + + :func:`verdict_rests_on_one_ground` allows a verdict one stated risk beside + its citation, because on the single path the two are one statement by one + author -- "the proposal claims a 12% gain and has no rollback plan" names + the rule and the risk in the same breath. A batch states its risks once for + the whole set while the citation belongs to the entry, and nothing connects + them: counting them together would identify the batch's one ground with the + entry's rule, which is attribution in the direction that dispatches. A set + refused because no variant in it supplies a rollback plan would run on the + strength of a formatting rule its rationales happen to cite. + + So a finding the batch states holds every reject in the set, whatever its + count, and only an entry's own grounds can support a downgrade. That is the + rule the batch path already had -- what the batch states adds to the grounds + a variant is held on, never supplies the grounds it is softened on -- with + arithmetic that claimed more than it could tell taken out of it. + + Known limitation: the downgrade now needs a payload that states no findings + at all. That is the batch shape the runtime teaches -- + ``{target_proposal_msg_id, verdict_map: {name: {verdict, rationale?, + failure_reason_code?}}}``, the only batch payload spelled out anywhere in + ``src`` (PolicyGate's repair hint) and all + ``references/intent_envelope.md`` asks for. It is not the reject shape + ``references/verdict_schema.md`` documents: that states one risk *and* one + required-evidence item, as both reject exemplars in + ``critic/tests/expected_outputs.json`` do, so a batch written in the + single-verdict style keeps every reject it wrote -- including one resting on + nothing but an advisory rule. Recovering that needs attribution the Critic + is asked for, i.e. a batch shape in the output schema; guessing it from + prose is what this reading gives up. + + Args: + entry: One ``verdict_map`` entry. + payload: The ``review_verdict`` payload that entry arrived in. + action_name: The reviewed proposal's action name. + + Returns: + A ``(verdict, reason_code)`` pair, as :func:`verdict_held_to_its_rule` + returns it. + """ + grounds = verdict_map_entry_grounds(entry, payload) + if _batch_states_findings(payload): + return _stated_verdict(grounds), "" + return verdict_held_to_its_rule(grounds, action_name=action_name) + + # Minimum over-baseline gain a same-harness revalidation must show to count as # "engaged"; detects a collapse back to ~baseline. _MIN_KERNEL_ENGAGED_GAIN_PCT: float = 2.0 diff --git a/src/hyperloom/orchestrator/loop/intent_router.py b/src/hyperloom/orchestrator/loop/intent_router.py index 1ccd7cea3d..467ef5aae0 100644 --- a/src/hyperloom/orchestrator/loop/intent_router.py +++ b/src/hyperloom/orchestrator/loop/intent_router.py @@ -26,8 +26,11 @@ from .coordinator_helpers import ( _parse_iso_unix, coerce_needs_gpu, + collapse_verdicts, format_exc_brief, serialize_verdict_advisory, + verdict_held_to_its_rule, + verdict_map_entry_held_to_its_rule, ) from hyperloom.common.timeutil import now_iso from hyperloom.inference_optimizer.session.session_paths import runs_dir @@ -211,17 +214,27 @@ async def _handle_review_verdict(self, source: str, intent: Intent) -> None: }, ) return - verdict = str(single_verdict or "") + verdict = await self._record_verdict_hold( + verdict_held_to_its_rule(intent.payload, action_name=pending.action_name), + target=target, + ) + authored = str(single_verdict or "").strip() if not verdict and isinstance(verdict_map, dict) and verdict_map: - sub_verdicts = [str((entry or {}).get("verdict") or "").strip() for entry in verdict_map.values()] - verdict = ( - "approve" - if "approve" in sub_verdicts - else "reject" - if "reject" in sub_verdicts - else "advise" - if "advise" in sub_verdicts - else "needs_review" + # Per entry before the collapse below: a variant rejected on an + # advisory-only rule must not out-rank its siblings' advice. Each + # entry is read against the findings the payload states for the + # batch, which hold every reject in the set. + sub_verdicts = [ + await self._record_verdict_hold( + verdict_map_entry_held_to_its_rule(entry, intent.payload, action_name=pending.action_name), + target=target, + variant=str(name), + ) + for name, entry in verdict_map.items() + ] + verdict = collapse_verdicts(sub_verdicts) + authored = collapse_verdicts( + str((entry or {}).get("verdict") or "").strip() for entry in verdict_map.values() ) # Defensive audit (log-only): record verdict_map collapse @@ -240,10 +253,61 @@ async def _handle_review_verdict(self, source: str, intent: Intent) -> None: source=source, pending=pending, verdict=verdict, + authored_verdict=authored, reasoning=str(intent.payload.get("reasoning") or ""), advisory=serialize_verdict_advisory(intent.payload), ) + async def _record_verdict_hold( + self, + held: tuple[str, str], + *, + target: str, + variant: str = "", + ) -> str: + """Return the verdict to act on, recording any hold to a cited rule. + + A rule that declares ``advise`` does so because rejecting on it discards + the whole proposal set over a format or strategy hint. Enforcing the + declaration means the Critic cannot spend a round's proposals on a rule + that never asked for a rejection; the downgrade is recorded so the drift + is visible rather than silently corrected. + + Args: + held: The ``(verdict, reason_code)`` pair + :func:`verdict_held_to_its_rule` returned for a single verdict, + or :func:`verdict_map_entry_held_to_its_rule` for one variant's. + target: The target proposal msg_id, for the audit record. + variant: The ``verdict_map`` key when the verdict is one variant's; + empty for a single verdict. + + Returns: + The verdict to act on. + """ + verdict, downgraded_from_code = held + if not downgraded_from_code: + return verdict + log.warning( + "review_verdict held to its rule: target=%s variant=%s reject -> %s (reason_code=%s)", + target, + variant or "-", + verdict, + downgraded_from_code, + ) + await self._record_observation( + "coordinator", + "observation", + { + "kind": "verdict_downgraded_to_rule_verdict", + "target": target, + "variant": variant, + "from_verdict": "reject", + "to_verdict": verdict, + "failure_reason_code": downgraded_from_code, + }, + ) + return verdict + async def _handle_single_verdict( self, *, @@ -251,6 +315,7 @@ async def _handle_single_verdict( pending: "PendingProposal", # noqa: F821 - deferred ref; imported lazily in handlers to avoid import cycle. verdict: str, reasoning: str, + authored_verdict: str = "", advisory: dict[str, Any] | None = None, ) -> None: """Single-verdict handler (approve/advise materialises proposal as-is); mirrors integrate_patch/specialist verdicts onto specialist_patch_verdicts for PolicyGate. @@ -260,6 +325,9 @@ async def _handle_single_verdict( pending: The pending proposal the verdict targets. verdict: The collapsed verdict (approve / advise / reject / needs_review). reasoning: Free-text reasoning recorded with the verdict. + authored_verdict: The verdict the Critic itself wrote, before any + hold to a cited rule. Mirrored onto ``specialist_patch_verdicts`` + in place of ``verdict``; defaults to ``verdict``. advisory: Pre-serialised advisory fields (``required_evidence`` / ``risks`` / ``advice_text`` / ``alternative_action`` / ``notes`` / ``kb_evidence`` / ``packet_evidence``) to carry on @@ -298,6 +366,13 @@ async def _handle_single_verdict( ) # Mirror specialist / integrate_patch verdicts onto SharedState so # PolicyGate's integrate_patch gate can consult them on the next tick. + # What gets mirrored is what the Critic wrote, never what the hold made + # of it: ``advise`` is a landing permit there, and holding a reject to a + # formatting rule is meant to save the round's ideas from being thrown + # away, not to land a patch the Critic refused. A held proposal that + # still deserves to land gets there through a fresh Critic verdict, + # which overwrites this one. + patch_verdict = str(authored_verdict or verdict).strip() try: pa_params = pending.payload.get("params") or {} except AttributeError: @@ -308,11 +383,11 @@ async def _handle_single_verdict( elif pending.action_name == "specialist": # Critic verdict on the specialist proposal counts as the verdict on its patches; task_id is the key. sid_candidate = str(pa_params.get("task_id") or "").strip() - if sid_candidate and verdict: + if sid_candidate and patch_verdict: try: self.shared_state.record_specialist_patch_verdict( sid_candidate, - verdict, + patch_verdict, ) self.shared_state.save(self.session_dir) except Exception: # noqa: BLE001 — best-effort mirror diff --git a/src/hyperloom/orchestrator/policy/gate.py b/src/hyperloom/orchestrator/policy/gate.py index fc1420a260..bf7ac1f117 100644 --- a/src/hyperloom/orchestrator/policy/gate.py +++ b/src/hyperloom/orchestrator/policy/gate.py @@ -1264,9 +1264,10 @@ def _validate_review_verdict(self, role: "AgentRole", payload: dict[str, Any]) - rule="payload", hint=( "single-proposal review: emit {target_proposal_msg_id, " - "verdict, reasoning}. Explore batch review: emit " - "{target_proposal_msg_id, verdict_map: {variant_name: " - "{verdict, rationale?}}}" + "verdict, reasoning, failure_reason_code?}. Explore batch " + "review: emit {target_proposal_msg_id, verdict_map: " + "{variant_name: {verdict, rationale?, " + "failure_reason_code?}}}" ), ) if has_single: diff --git a/src/hyperloom/orchestrator/prompts/critic.md b/src/hyperloom/orchestrator/prompts/critic.md index a3b5a56d9d..5ffb811e5b 100644 --- a/src/hyperloom/orchestrator/prompts/critic.md +++ b/src/hyperloom/orchestrator/prompts/critic.md @@ -71,19 +71,23 @@ in-phase kernel patch would. * Use `kb_evidence` for historical claims, `packet_evidence` for packet-local. * Never `delegate` / `request` / `propose_action` (PolicyGate rejects). * RCA belongs to Robustness, not you. -* `proposal_set[*]` MUST NOT carry `expected_gain` / `bench_evidence` - / `confidence` / `score` / `rank` / `force_provenance`. Reject with - `reason="specialist_quantitative_claim_violation"`. This guard applies to - every specialist proposal regardless of scope. - Match those six keys exactly, and only on specialist-authored - `proposal_set` entries. It is a guard against a specialist inventing a - performance claim, not a ban on scheduler bookkeeping that happens to be - numeric. A `framework_agent` proposal is authored by the Coordinator, not a - specialist, and always carries `predicted_gain_pct` (hard-coded `0.0`, i.e. - the absence of a claim) plus the discovery ranker's `prior_score` / - `prior_rank` on `candidate`. Rejecting on those denies every framework - candidate before it is ever benchmarked and drains the phase's plateau - counter, so never fire this rule on them. +* `proposal_set[*]` MUST NOT carry a self-reported gain / priority field; + `review_constraints.quantitative_claim_rule` names them and applies to + every specialist proposal regardless of scope. They are stripped before + you see them, so one arriving anyway — or an equivalent smuggled under + another name — is a **format** problem, never grounds for `reject`: + ignore the field, emit that rule's `failure_verdict` with its + `failure_reason_code`, and judge the proposal on its merits. Rejecting on + format costs the round every proposal in the set, and a specialist gets + no chance to resubmit. +* That rule reaches specialist-authored `proposal_set` entries only. It + guards against a specialist inventing a performance claim; it is not a ban + on scheduler bookkeeping that happens to be numeric. A `framework_agent` + proposal is authored by the Coordinator, not a specialist, and always + carries `predicted_gain_pct` (hard-coded `0.0`, i.e. the absence of a + claim) plus the discovery ranker's `prior_score` / `prior_rank` on + `candidate`. Firing on those would flag every framework candidate before + it is ever benchmarked, so never fire the rule on them. ### Cross-domain proposals (scope=domains) diff --git a/src/hyperloom/orchestrator/prompts/specialist_prompt_builder.py b/src/hyperloom/orchestrator/prompts/specialist_prompt_builder.py index ed40618912..805440de02 100644 --- a/src/hyperloom/orchestrator/prompts/specialist_prompt_builder.py +++ b/src/hyperloom/orchestrator/prompts/specialist_prompt_builder.py @@ -1514,9 +1514,12 @@ def _section_kb_subgraph(inp: SpecialistPromptInputs) -> list[str]: + "prior. Pick the **1–2 most conservative, " + "well-attested defaults** from those bullets that are " + "compatible with the hardware (Section 2) and the " - + "gap symptom (Section 3); flag each as " - + "``confidence: low`` and ``provenance: " - + "domain_focus_default`` in the proposal. Use the " + + "gap symptom (Section 3); flag each with " + + "``provenance: domain_focus_default`` in the proposal " + + "and say it is an unvalidated fallback prior in the " + + "proposal's ``reason``. Do NOT add a ``confidence`` " + + "field: self-reported confidence / gain fields are " + + "stripped from your output before review. Use the " + "``residual_questions`` field to record what RecipeKB, " + "research, or ``mcp__pr_monitor__*`` query a future round should pursue.", "", diff --git a/src/hyperloom/orchestrator/roles/critic_agent.py b/src/hyperloom/orchestrator/roles/critic_agent.py index 00365667c3..a1d58d43cd 100644 --- a/src/hyperloom/orchestrator/roles/critic_agent.py +++ b/src/hyperloom/orchestrator/roles/critic_agent.py @@ -87,6 +87,7 @@ "risks": [{"severity": "blocker|major|minor", "summary": "..."}], "required_evidence": ["", ...], "notes": ["..."], + "failure_reason_code": "", "persist_to_kb": false, "topic": "" } @@ -115,6 +116,12 @@ - If `review_constraints.known_actions` is non-empty, any `alternative_action` MUST be drawn from it; otherwise omit `alternative_action`. +- When a verdict rests on a rule from `review_constraints`, copy that + rule's `failure_reason_code` verbatim into the verdict's own + `failure_reason_code`; leave it `""` when the verdict rests on your + own judgement. Some of those rules declare `advise` as their + `failure_verdict`, and naming the rule is how the Coordinator tells + a verdict resting on one apart from a substantive refusal. ==== END OUTPUT FORMAT ==== """.strip() @@ -306,6 +313,38 @@ def _maybe_inject_cross_domain_constraints(judge_bundle: dict[str, Any]) -> None rc["cross_domain_rules"] = cross_domain_rule_descriptors() +def _maybe_inject_quantitative_claim_constraint(judge_bundle: dict[str, Any]) -> None: + """Set ``review_constraints.quantitative_claim_rule`` from the enforced list. + + Delivering the rule as data keeps the Critic's field list identical to the + one the runner strips, instead of a hand-copied prose list that drifts. It + is sent only when the bundle holds a proposal the rule is about, on the same + principle as the cross-domain rules above: a Critic handed a rule that + cannot apply to anything under review can still cite it, and a citation is + what the verdict path reads. + + Args: + judge_bundle: The judge bundle to enrich in place; unchanged when no + proposal is one of the kinds the rule governs. + """ + from ..specialists.patch_safety import ( + advisory_rules_govern, + quantitative_claim_rule_descriptor, + ) + + proposals = judge_bundle.get("proposals") or [] + if not isinstance(proposals, list): + return + governed = any(advisory_rules_govern(str(p.get("action_name") or "")) for p in proposals if isinstance(p, dict)) + if not governed: + return + rc = judge_bundle.setdefault("review_constraints", {}) + if not isinstance(rc, dict): + rc = {} + judge_bundle["review_constraints"] = rc + rc["quantitative_claim_rule"] = quantitative_claim_rule_descriptor() + + @dataclass class CriticAgentBackend: """Real Critic backend that drives the critic-agent runtime. @@ -600,6 +639,7 @@ async def run( _inject_phase_constraints(judge_bundle, self._trace_phase or "") _maybe_inject_cross_domain_constraints(judge_bundle) + _maybe_inject_quantitative_claim_constraint(judge_bundle) # Codex reasoning; short-circuit when there are no proposals. proposals = judge_bundle.get("proposals") or [] diff --git a/src/hyperloom/orchestrator/specialists/patch_safety.py b/src/hyperloom/orchestrator/specialists/patch_safety.py index 881a596567..b1ea27cabe 100644 --- a/src/hyperloom/orchestrator/specialists/patch_safety.py +++ b/src/hyperloom/orchestrator/specialists/patch_safety.py @@ -10,8 +10,8 @@ * unified-diff structural validation (a patch must carry at least one hunk), * git-grounding (``git apply --check`` against a clean checkout so a fabricated patch that does not apply to real source is flagged), -* quantitative-claim guards (forbidden numeric fields + numeric-claim regex on - the qualitative argument), +* quantitative-claim guards (forbidden numeric fields, stripped rather than + merely reported, + numeric-claim regex on the qualitative argument), * the cross-domain Critic rule descriptors, surfaced when ``scope == 'domains'``. Pure / dependency-light: imports only stdlib + git via subprocess so it can be @@ -30,10 +30,18 @@ # Quantitative / priority fields rejected outright on any patch proposal: # throughput / gain numbers are the Coordinator's measured truth, never a # self-reported claim from the worker. +# +# The ban is scoped to specialist-authored output by where it is applied, not +# by what it lists: ``strip_forbidden_proposal_fields`` runs on the specialist +# exit payload alone. ``predicted_gain_pct`` therefore belongs here even though +# it is a *required* field of a ``propose_action`` intent -- there the number +# is the Coordinator's estimate, in a specialist's ``proposal_set`` it is the +# same self-reported claim as ``expected_gain_pct`` under a different name. FORBIDDEN_PROPOSAL_FIELDS: frozenset[str] = frozenset( { "expected_gain", "expected_gain_pct", + "predicted_gain_pct", "bench_evidence", "confidence", "score", @@ -42,6 +50,14 @@ } ) +# The same guard at the payload's top level, where ``confidence`` means +# something else: the output schema asks for a round-level self-assessment and +# the specialist-round audit rows record it. That is not a per-proposal gain +# claim and cannot bias which variant gets benched, so banning it here only +# made the schema contradict itself -- the guard's own scope, per the Critic +# rules, is ``proposal_set[*]``. +FORBIDDEN_PAYLOAD_FIELDS: frozenset[str] = FORBIDDEN_PROPOSAL_FIELDS - {"confidence"} + # Numeric speedup claims smuggled into a qualitative argument / summary. _NUMERIC_CLAIM_PATTERNS: tuple[re.Pattern[str], ...] = ( @@ -162,6 +178,12 @@ def patch_targets_missing( # specialists.profile.SCOPE_DOMAINS to keep this module dependency-light. SCOPE_DOMAINS_LITERAL: str = "domains" +# The verdict a rule declares when its violation is advisory: the proposal still +# reaches the Coordinator, carrying the reason code as a note. Spelled once so a +# typo in one rule cannot quietly drop it from +# :func:`advisory_only_reason_codes` and re-arm the reject it asked to avoid. +ADVISE_VERDICT: str = "advise" + @dataclass(frozen=True) class CrossDomainRule: @@ -182,7 +204,7 @@ class CrossDomainRule: "domain in scope — why this change is necessary within that " "domain's boundary." ), - failure_verdict="advise", + failure_verdict=ADVISE_VERDICT, failure_reason_code="cross_domain_rationale_incomplete", ), CrossDomainRule( @@ -192,7 +214,7 @@ class CrossDomainRule: "(why these changes must happen together) AND at least " "one potential side effect of the combination." ), - failure_verdict="advise", + failure_verdict=ADVISE_VERDICT, failure_reason_code="cross_domain_coupling_unspecified", ), CrossDomainRule( @@ -205,7 +227,7 @@ class CrossDomainRule: "the motivation degenerates so the stack rebench + KEEP " "threshold can adjudicate." ), - failure_verdict="advise", + failure_verdict=ADVISE_VERDICT, failure_reason_code="cross_domain_motivation_invalid", ), ) @@ -229,6 +251,97 @@ def cross_domain_rule_descriptors() -> list[dict[str, str]]: ] +# Audit code the Critic cites when a self-reported gain field reaches review. +QUANTITATIVE_CLAIM_REASON_CODE: str = "specialist_quantitative_claim_violation" + + +def quantitative_claim_rule_descriptor() -> dict[str, Any]: + """Return the self-reported-gain rule in the shape the Critic bundle uses. + + Single-sources the field list from :data:`FORBIDDEN_PROPOSAL_FIELDS` so the + Critic's copy cannot drift from the one the runner enforces, and carries + ``advise`` as the verdict: the fields are stripped before review, so one + arriving anyway is a format problem, and rejecting over format costs the + round every proposal in the set. + + Returns: + A ``rule_id`` / ``description`` / ``forbidden_proposal_fields`` / + ``failure_verdict`` / ``failure_reason_code`` dict. + """ + return { + "rule_id": "no_self_reported_gain", + "description": ( + "proposal_set[*] must not carry a self-reported gain, priority or " + "confidence field: measured gain is the Coordinator's, never the " + "worker's claim. These fields are stripped from specialist output " + "before review, so treat any that still reach you -- including an " + "equivalent smuggled under another name -- as advisory: ignore the " + "field and judge the proposal on its merits." + ), + "forbidden_proposal_fields": sorted(FORBIDDEN_PROPOSAL_FIELDS), + "failure_verdict": ADVISE_VERDICT, + "failure_reason_code": QUANTITATIVE_CLAIM_REASON_CODE, + } + + +def advisory_only_reason_codes() -> frozenset[str]: + """Return the reason codes whose owning rule asked for ``advise``, not ``reject``. + + Derived from the descriptors the Critic is actually handed rather than + restated, so a rule that changes its ``failure_verdict`` cannot leave a + stale entry behind. Lets the verdict path hold a ``reject`` citing one of + these to the verdict its own rule declared: every rule here is a format or + strategy hint, and a reject costs the round every proposal in the set. + + Returns: + The ``failure_reason_code`` of every rule declaring + ``failure_verdict == "advise"``. + """ + descriptors: list[dict[str, Any]] = [quantitative_claim_rule_descriptor()] + descriptors.extend(cross_domain_rule_descriptors()) + codes = { + str(d.get("failure_reason_code") or "").strip() + for d in descriptors + if str(d.get("failure_verdict") or "").strip() == ADVISE_VERDICT + } + codes.discard("") + return frozenset(codes) + + +# The proposal kinds the advisory rules speak about. Both rule families are +# about a specialist-authored payload -- ``proposal_set[*]`` for the +# quantitative-claim rule, ``scope=domains`` for the cross-domain ones -- which +# reaches review as a ``specialist`` proposal or as the ``explore`` grid that +# ``proposal_set`` is materialised into. ``framework_agent`` is here because the +# quantitative-claim rule names it by exception ("never fire the rule on them", +# see prompts/critic.md): its payload always carries ``predicted_gain_pct``, so +# a verdict citing the rule there is a misapplication of the rule itself. +# +# Spelled out rather than derived: no ACTION_CATALOGUE field separates these +# from ``integrate_patch``, which shares their ``exploration`` verdict class, +# ``shallow`` family and ``workspace_write`` side effect while being the one +# action whose materialisation lands the patch under review. +ADVISORY_RULE_PROPOSAL_KINDS: frozenset[str] = frozenset( + { + "explore", + "framework_agent", + "specialist", + } +) + + +def advisory_rules_govern(action_name: str) -> bool: + """Return whether the advisory review rules speak about ``action_name``. + + Args: + action_name: The proposed action's name. + + Returns: + True when the action is one of :data:`ADVISORY_RULE_PROPOSAL_KINDS`. + """ + return str(action_name or "").strip() in ADVISORY_RULE_PROPOSAL_KINDS + + def numeric_claims(text: str) -> list[str]: """Return numeric-speedup claim substrings found in ``text``. @@ -403,37 +516,69 @@ def notes(self) -> list[str]: return out -def scan_quantitative_claims(payload: dict[str, Any]) -> tuple[list[str], list[str]]: - """Return ``(forbidden_fields_present, numeric_warning_strings)``. +def scan_numeric_claims(payload: dict[str, Any]) -> list[str]: + """Return the numeric speedup claims smuggled into ``payload``'s prose. - Forbidden quantitative fields are a hard signal; numeric claims in the - summary / qualitative argument are advisory warnings (the Coordinator's - measured gain is the truth, not the claim). + A number in a summary or qualitative argument is advisory: the Coordinator's + measured gain is the truth, not the claim, and the audit note this feeds is + how a smuggled one stays visible. + + The forbidden *fields* are a different question, and + :func:`strip_forbidden_proposal_fields` is the one place that answers it: it + removes them and returns what it took. Answering it a second time here would + be a copy of that same ``keys & FORBIDDEN_*`` intersection with nothing + holding the two in step. Args: payload: The specialist_done payload to scan. Returns: - A ``(forbidden_fields_present, numeric_warning_strings)`` tuple, each - de-duped with order preserved. + The matched numeric-claim substrings, de-duped with order preserved. """ - forbidden = sorted(set((payload or {}).keys()) & FORBIDDEN_PROPOSAL_FIELDS) warnings: list[str] = [] for key in ("summary", "expected_qualitative_argument", "cross_domain_rationale"): - hits = numeric_claims(str((payload or {}).get(key) or "")) - if hits: - warnings.extend(hits) + warnings.extend(numeric_claims(str((payload or {}).get(key) or ""))) for proposal in (payload or {}).get("proposal_set") or []: if not isinstance(proposal, dict): continue - forbidden.extend(sorted(set(proposal.keys()) & FORBIDDEN_PROPOSAL_FIELDS)) - hits = numeric_claims(str(proposal.get("expected_qualitative_argument") or "")) - if hits: - warnings.extend(hits) - # de-dupe, preserve order - forbidden = list(dict.fromkeys(forbidden)) - warnings = list(dict.fromkeys(warnings)) - return forbidden, warnings + warnings.extend(numeric_claims(str(proposal.get("expected_qualitative_argument") or ""))) + return list(dict.fromkeys(warnings)) + + +def strip_forbidden_proposal_fields(payload: dict[str, Any]) -> list[str]: + """Remove the forbidden quantitative keys from ``payload`` in place. + + Uses :data:`FORBIDDEN_PAYLOAD_FIELDS` at the top level and + :data:`FORBIDDEN_PROPOSAL_FIELDS` on each ``proposal_set`` entry. + + Detecting a self-reported gain number and then forwarding it is what turns a + format slip into a lost round: the Critic is told to reject the whole + ``proposal_set`` over it, so the specialist's ideas never reach a benchmark + and there is rarely budget to resubmit. The claim is worthless either way — + measured gain is the Coordinator's — so dropping it costs nothing and makes + the violation unreachable rather than merely audited. The names returned are + what the caller's audit note records. + + Args: + payload: The ``specialist_done`` payload, mutated in place. Both the + top level and each ``proposal_set`` entry are cleaned. + + Returns: + The removed field names, de-duped with first-seen order preserved. + """ + if not isinstance(payload, dict): + return [] + removed: list[str] = [] + for key in sorted(set(payload.keys()) & FORBIDDEN_PAYLOAD_FIELDS): + payload.pop(key, None) + removed.append(key) + for proposal in payload.get("proposal_set") or []: + if not isinstance(proposal, dict): + continue + for key in sorted(set(proposal.keys()) & FORBIDDEN_PROPOSAL_FIELDS): + proposal.pop(key, None) + removed.append(key) + return list(dict.fromkeys(removed)) def vet_patches( @@ -472,8 +617,11 @@ def vet_patches( __all__ = [ + "ADVISE_VERDICT", + "ADVISORY_RULE_PROPOSAL_KINDS", "CROSS_DOMAIN_RULES", "CrossDomainRule", + "FORBIDDEN_PAYLOAD_FIELDS", "FORBIDDEN_PROPOSAL_FIELDS", "GROUND_APPLIES", "GROUND_MISSING_TARGET", @@ -483,7 +631,10 @@ def vet_patches( "GROUND_UNCHECKED", "PatchGroundingResult", "PatchSafetyReport", + "QUANTITATIVE_CLAIM_REASON_CODE", "SCOPE_DOMAINS_LITERAL", + "advisory_only_reason_codes", + "advisory_rules_govern", "cross_domain_rule_descriptors", "ground_patch_text", "is_unified_diff", @@ -491,6 +642,8 @@ def vet_patches( "patch_escapes_tree", "patch_file_targets", "patch_targets_missing", - "scan_quantitative_claims", + "quantitative_claim_rule_descriptor", + "scan_numeric_claims", + "strip_forbidden_proposal_fields", "vet_patches", ] diff --git a/src/hyperloom/orchestrator/specialists/runner.py b/src/hyperloom/orchestrator/specialists/runner.py index 23187c8230..1c15633de8 100644 --- a/src/hyperloom/orchestrator/specialists/runner.py +++ b/src/hyperloom/orchestrator/specialists/runner.py @@ -1403,9 +1403,11 @@ def _resolve_existing_patch(p: Any) -> str | None: deduped, base_checkout=base_checkout, ) - forbidden_fields, numeric_warnings = _patch_safety.scan_quantitative_claims( - done_payload, - ) + numeric_warnings = _patch_safety.scan_numeric_claims(done_payload) + # Strip, do not forward: the Critic is instructed to reject the whole + # proposal_set over these fields, which costs the round every idea the + # specialist produced. The audit note below records what the strip took. + forbidden_fields = _patch_safety.strip_forbidden_proposal_fields(done_payload) safety = _patch_safety.PatchSafetyReport( kept_patches=kept, dropped=dropped,