From 7a35d379a1f3986341e9ca8f91ffdf4ae025e742 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 21:45:01 -0700 Subject: [PATCH] Fix planning repair human requirement acknowledgements --- src/coding_review_agent_loop/orchestrator.py | 16 +++- src/coding_review_agent_loop/repair.py | 77 +++++++++++++++--- tests/test_agent_loop.py | 4 +- tests/test_orchestrator_issue.py | 4 +- tests/test_repair.py | 85 ++++++++++++++++++-- 5 files changed, 165 insertions(+), 21 deletions(-) diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index 5cc353f..cf49b9f 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -2131,7 +2131,14 @@ def _run_validated_agent( repair_kwargs: dict[str, object] = {"expected_kind": repair_expected_kind} if repair_unresolved_item_ids is not None: repair_kwargs["unresolved_item_ids"] = tuple(repair_unresolved_item_ids) - if ( + if repair_expected_kind in {"plan_state", "plan_revision"}: + repair_kwargs["surfaced_requirement_ids"] = tuple( + repair_surfaced_requirement_ids or () + ) + repair_kwargs["requires_direct_discussion_ack"] = ( + repair_requires_direct_discussion_ack + ) + elif ( repair_expected_kind == "coder_followup" and repair_surfaced_requirement_ids is not None ): @@ -3055,6 +3062,11 @@ def _run_plan_first_loop( resume_state = _resume_plan_round(issue_context.comments, configured_reviewers=configured_reviewers) if resume_state is None: log(config, f"Planning issue #{issue_number}: invoking {coder_name} (context mode: full)") + plan_human_requirements_context = render_coder_human_requirements_prompt_context( + issue_context.human_requirements, + requirement_scope="planning requirements", + full_omission_fallback="Fetch the issue discussion directly before finalizing the plan.", + ) plan_response = _run_validated_agent( runner, agent=config.coder, @@ -3071,6 +3083,8 @@ def _run_plan_first_loop( usage_context=usage_context, use_repair=True, repair_expected_kind="plan_state", + repair_surfaced_requirement_ids=plan_human_requirements_context.surfaced_requirement_ids, + repair_requires_direct_discussion_ack=plan_human_requirements_context.requires_direct_discussion_ack, operation_description="planning", ) plan_output = plan_response.text diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index 59436da..d2371cf 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -204,6 +204,8 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st {coder_followup_human_requirements_instruction} +{planning_human_requirements_instruction} + {reviewer_human_requirements_instruction} {prior_item_dispositions_instruction} @@ -417,14 +419,10 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st -- -Only include the optional signed human requirements acknowledgement when it was present in the malformed original. +The active planning human-requirements context above is authoritative. Do not use an acknowledgement marker or section in the malformed response as evidence that a signed requirement exists. ## Valid Format D — Plan Revision: -When the malformed plan_revision did not include a signed human requirements -acknowledgement, omit the `` marker and -the `### Human requirements` section from Format D. - { "schema_version": 1, "kind": "plan_revision", @@ -649,8 +647,8 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st Notes: - When repairing plan_revision, do not output coder_followup even if the original contains a Human requirements section. -- If the original plan revision includes and a ### Human requirements section, preserve both after the JSON and before . -- If the original plan revision does not include both that marker and section, do not fabricate a human requirements acknowledgement. +- Preserve the acknowledgement after the JSON and before the footer only when the active planning context requires it; never infer a requirement from the malformed response. +- The active planning human-requirements context is authoritative: preserve the acknowledgement only when it requires surfaced signed requirements or direct-discussion acknowledgement; otherwise remove both legacy markdown elements. ## WORKED EXAMPLE 5 — coder follow-up with no signed human requirements: @@ -1059,6 +1057,7 @@ def _build_repair_prompt( expected_kind: str | None = None, unresolved_item_ids: Sequence[str] | None = None, surfaced_requirement_ids: Sequence[str] | None = None, + requires_direct_discussion_ack: bool = False, reviewer_requirement_ids: Sequence[str] | None = None, allowed_prior_item_ids: Sequence[str] | None = None, unknown_prior_item_ids: Sequence[str] | None = None, @@ -1066,11 +1065,29 @@ def _build_repair_prompt( ) -> str: if expected_kind is not None and expected_kind not in _SUPPORTED_EXPECTED_KINDS: raise ValueError(f"Unsupported expected repair kind: {expected_kind}") + if ( + (surfaced_requirement_ids is not None or requires_direct_discussion_ack) + and expected_kind not in {"coder_followup", "plan_state", "plan_revision"} + ): + raise ValueError( + "surfaced_requirement_ids may only be used for coder_followup, plan_state, or plan_revision repair" + ) coder_followup_required_items_instruction = _coder_followup_required_items_instruction( expected_kind, unresolved_item_ids ) - coder_followup_human_requirements_instruction = _coder_followup_human_requirements_instruction( - expected_kind, surfaced_requirement_ids + coder_followup_human_requirements_instruction = ( + _coder_followup_human_requirements_instruction(expected_kind, surfaced_requirement_ids) + if expected_kind == "coder_followup" + else "" + ) + planning_human_requirements_instruction = ( + _planning_human_requirements_instruction( + expected_kind, + surfaced_requirement_ids, + requires_direct_discussion_ack, + ) + if expected_kind in {"plan_state", "plan_revision"} + else "" ) reviewer_human_requirements_instr = _reviewer_human_requirements_instruction( expected_kind, reviewer_requirement_ids @@ -1088,6 +1105,7 @@ def _build_repair_prompt( replacements = ( ("{coder_followup_required_items_instruction}", coder_followup_required_items_instruction), ("{coder_followup_human_requirements_instruction}", coder_followup_human_requirements_instruction), + ("{planning_human_requirements_instruction}", planning_human_requirements_instruction), ("{reviewer_human_requirements_instruction}", reviewer_human_requirements_instr), ("{prior_item_dispositions_instruction}", prior_item_dispositions_instruction), ("{raw_response}", raw), @@ -1297,6 +1315,45 @@ def _coder_followup_human_requirements_instruction( ) +def _planning_human_requirements_instruction( + expected_kind: str | None, + surfaced_requirement_ids: Sequence[str] | None, + requires_direct_discussion_ack: bool, +) -> str: + if surfaced_requirement_ids is None and not requires_direct_discussion_ack: + return "" + if expected_kind not in {"plan_state", "plan_revision"}: + raise ValueError( + "surfaced_requirement_ids may only be used for plan_state or plan_revision repair" + ) + ids = tuple(surfaced_requirement_ids or ()) + rendered_ids = ", ".join(ids) or "(none)" + if not ids and not requires_direct_discussion_ack: + return ( + "## Active planning human-requirements context:\n" + "No signed human requirements were surfaced and direct-discussion acknowledgement is not required. " + "This context is authoritative: remove any legacy `` marker " + "and `### Human requirements` section from the repaired output, emit no human-requirement " + "dispositions, and do not treat those malformed-response elements or issue acceptance criteria as evidence; " + "the malformed response cannot establish a requirement.\n" + ) + disposition_rule = ( + "Include one `human_requirement_dispositions` object for every surfaced ID, using the exact ID, " + "a valid disposition (`addressed`, `blocked`, or `not-applicable`), and non-empty evidence." + if ids + else "No surfaced IDs exist, so emit no fabricated requirement dispositions." + ) + return ( + "## Active planning human-requirements context:\n" + f"Surfaced signed requirement IDs: {rendered_ids}\n" + f"Direct-discussion acknowledgement required: {'yes' if requires_direct_discussion_ack else 'no'}\n" + "This active context is authoritative; the malformed response cannot establish a requirement. " + "Preserve or add the `` marker and `### Human requirements` " + "section in the required position after the JSON and before the plan-state footer. " + f"{disposition_rule}\n" + ) + + def _reviewer_human_requirements_instruction( expected_kind: str | None, reviewer_requirement_ids: Sequence[str] | None, @@ -1365,6 +1422,7 @@ def attempt_repair( expected_kind: str | None = None, unresolved_item_ids: Sequence[str] | None = None, surfaced_requirement_ids: Sequence[str] | None = None, + requires_direct_discussion_ack: bool = False, reviewer_requirement_ids: Sequence[str] | None = None, allowed_prior_item_ids: Sequence[str] | None = None, unknown_prior_item_ids: Sequence[str] | None = None, @@ -1381,6 +1439,7 @@ def attempt_repair( expected_kind=expected_kind, unresolved_item_ids=unresolved_item_ids, surfaced_requirement_ids=surfaced_requirement_ids, + requires_direct_discussion_ack=requires_direct_discussion_ack, reviewer_requirement_ids=reviewer_requirement_ids, allowed_prior_item_ids=allowed_prior_item_ids, unknown_prior_item_ids=unknown_prior_item_ids, diff --git a/tests/test_agent_loop.py b/tests/test_agent_loop.py index 2e6229c..306cb55 100644 --- a/tests/test_agent_loop.py +++ b/tests/test_agent_loop.py @@ -402,7 +402,7 @@ def test_gemini_pre_marker_429_does_not_suppress_structured_review_repair(tmp_pa config = make_config(tmp_path, reviewer="gemini", agent_max_retries=0) captured_repairs = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append(raw) assert expected_kind == "pr_review" return repaired_review @@ -446,7 +446,7 @@ def test_gemini_response_file_repair_ignores_raw_stdout_transient_diagnostics(tm config = make_config(tmp_path, reviewer="gemini", agent_max_retries=0) captured_repairs = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append(raw) return repaired_review diff --git a/tests/test_orchestrator_issue.py b/tests/test_orchestrator_issue.py index 57867e5..9440744 100644 --- a/tests/test_orchestrator_issue.py +++ b/tests/test_orchestrator_issue.py @@ -694,7 +694,7 @@ def test_issue_loop_plan_revision_repair_preserves_signed_human_requirements(tmp config = make_config(tmp_path, agent_max_retries=0) captured_repairs = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append((raw, expected_kind)) return repaired_revision @@ -760,7 +760,7 @@ def test_issue_loop_plan_revision_repair_rejects_wrong_kind_from_human_requireme config = make_config(tmp_path, agent_max_retries=0) captured_kinds = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_kinds.append(expected_kind) return wrong_kind_repair diff --git a/tests/test_repair.py b/tests/test_repair.py index 091f0a1..8018ade 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -457,8 +457,76 @@ def test_attempt_repair_includes_expected_kind_instruction(): assert "Output no other `kind` value" in prompt def test_attempt_repair_format_d_marks_human_requirements_optional(): - assert "omit the `` marker" in _REPAIR_PROMPT - assert "the `### Human requirements` section from Format D" in _REPAIR_PROMPT + assert "active planning human-requirements context is authoritative" in _REPAIR_PROMPT + assert "malformed response cannot establish a requirement" in _build_repair_prompt( + "malformed", expected_kind="plan_revision", surfaced_requirement_ids=() + ) + assert "preserve both after the JSON" not in _REPAIR_PROMPT + + +@pytest.mark.parametrize( + ("surfaced_ids", "requires_direct", "must_contain"), + [ + ((), False, "remove any legacy `` marker"), + (("Requirement 1",), False, "Surfaced signed requirement IDs: Requirement 1"), + ((), True, "Direct-discussion acknowledgement required: yes"), + ], +) +def test_planning_repair_prompt_uses_active_context( + surfaced_ids, requires_direct, must_contain +): + prompt = _build_repair_prompt( + "malformed plan revision with Requirement 1 acceptance criteria", + expected_kind="plan_revision", + surfaced_requirement_ids=surfaced_ids, + requires_direct_discussion_ack=requires_direct, + ) + assert must_contain in prompt + assert "malformed response cannot establish a requirement" in prompt + + +def test_plan_revision_repair_strips_fabricated_ack_without_signed_requirement_context(tmp_path): + malformed = structured_plan_revision(reviewer="Anthropic Claude").replace( + "\n", + "\n\n" + "### Human requirements\n" + "- Requirement 1: acceptance criteria mentioned in the issue.\n" + "", + 1, + ) + repaired = structured_plan_revision(reviewer="Anthropic Claude") + runner = FakeRunner(claude_outputs=[malformed]) + config = make_config(tmp_path, coder="claude", agent_max_retries=0) + + with patch("coding_review_agent_loop.orchestrator.attempt_repair", return_value=repaired) as repair_mock: + def validate_without_ack(text): + if HUMAN_REQUIREMENTS_ADDRESSED_MARKER in text or "### Human requirements" in text: + raise AgentLoopError("fabricated human requirements acknowledgement") + return _validate_plan_revision_response(text) + + response = _run_validated_agent( + runner, + agent="claude", + config=config, + prompt="Revise the plan.", + marker_description="", + validate=validate_without_ack, + use_repair=True, + repair_expected_kind="plan_revision", + repair_surfaced_requirement_ids=(), + repair_requires_direct_discussion_ack=False, + ) + + assert response.text == repaired + assert HUMAN_REQUIREMENTS_ADDRESSED_MARKER not in response.text + assert "### Human requirements" not in response.text + repair_mock.assert_called_once_with( + malformed, + config.gemini_cmd, + expected_kind="plan_revision", + surfaced_requirement_ids=(), + requires_direct_discussion_ack=False, + ) def test_repair_prompt_makes_research_intent_conditional_on_active_status(): @@ -903,7 +971,7 @@ def test_run_pr_loop_uses_repair_pass_on_format_failure(tmp_path): captured_repairs = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append(raw) assert expected_kind == "pr_review" return repaired_review @@ -1316,7 +1384,7 @@ def test_run_pr_loop_repairs_format_failure_with_5xx_source_line_reference(tmp_p captured_repairs = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append(raw) assert expected_kind == "pr_review" return repaired_review @@ -1339,7 +1407,7 @@ def test_run_pr_loop_falls_back_to_error_when_repair_also_fails(tmp_path): ) config = make_config(tmp_path, coder="claude", reviewer="codex", agent_max_retries=0) - def fake_attempt_repair_fails(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair_fails(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: assert expected_kind == "pr_review" return "still broken output without valid schema" @@ -1393,7 +1461,7 @@ def test_run_pr_loop_uses_repair_pass_on_coder_followup_format_failure(tmp_path) captured_repairs = [] captured_unresolved_item_ids = [] - def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: + def fake_attempt_repair(raw: str, gemini_cmd: str, *, expected_kind: str | None = None, unresolved_item_ids=None, surfaced_requirement_ids=None, requires_direct_discussion_ack=False, allowed_prior_item_ids=None, unknown_prior_item_ids=None, same_round_context=None) -> str | None: captured_repairs.append(raw) captured_unresolved_item_ids.append(tuple(unresolved_item_ids or ())) assert expected_kind == "coder_followup" @@ -1510,7 +1578,8 @@ def test_repair_prompt_coder_followup_fenced_json_example(): def test_repair_prompt_plan_revision_preserves_human_requirements_acknowledgement(): assert "WORKED EXAMPLE 4" in _REPAIR_PROMPT assert "do not output coder_followup" in _REPAIR_PROMPT - assert "preserve both after the JSON and before " in _REPAIR_PROMPT + assert "preserve the acknowledgement only when it requires surfaced signed requirements" in _REPAIR_PROMPT + assert "If the original plan revision includes " not in _REPAIR_PROMPT def test_repair_prompt_does_not_suggest_ack_pseudo_item_in_addressed_items(): """The ack pseudo-item must never be suggested as a value for addressed_items. @@ -2930,6 +2999,8 @@ def test_structured_plan_revision_transient_terms_before_footer_runs_repair(tmp_ malformed_revision, config.gemini_cmd, expected_kind="plan_revision", + surfaced_requirement_ids=("Requirement 1",), + requires_direct_discussion_ack=False, ) assert not any(cmd[:1] == ["sleep"] for cmd, _cwd in runner.commands)