From 7d390f5b3ff6870daf9a48dc056facf068b47ae6 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 01:25:34 -0700 Subject: [PATCH 1/5] Require auditable human requirement dispositions --- .../comment_rendering.py | 28 ++++++ src/coding_review_agent_loop/orchestrator.py | 12 +++ src/coding_review_agent_loop/prompts.py | 35 ++++++- src/coding_review_agent_loop/protocol.py | 92 ++++++++++++++++++- src/coding_review_agent_loop/repair.py | 12 ++- .../unresolved_items.py | 7 ++ tests/test_human_requirement_dispositions.py | 74 +++++++++++++++ 7 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 tests/test_human_requirement_dispositions.py diff --git a/src/coding_review_agent_loop/comment_rendering.py b/src/coding_review_agent_loop/comment_rendering.py index 447f17b..96c602c 100644 --- a/src/coding_review_agent_loop/comment_rendering.py +++ b/src/coding_review_agent_loop/comment_rendering.py @@ -18,6 +18,7 @@ PRIOR_UNRESOLVED_PLAN_ITEM_DISPOSITIONS_HEADING_RE, SIGNATURE_RE, DeferredStage, + HumanRequirementDisposition, ParsedDiscussAgenda, ParsedDiscussAnswer, ParsedFailedDiscussResponse, @@ -212,6 +213,22 @@ def render_canonical_plan_steps(plan_steps: Sequence[str]) -> str: return "\n".join(f"{index}. {step}" for index, step in enumerate(plan_steps, start=1)) +def render_human_requirement_dispositions( + dispositions: Sequence[HumanRequirementDisposition], + *, + heading: str = "### Human requirement dispositions", +) -> str | None: + if not dispositions: + return None + return "\n".join( + [heading] + + [ + f"- **{item.requirement_id}** — `{item.disposition}`: {item.evidence}" + for item in dispositions + ] + ) + + def render_deferred_stages_section(deferred_stages: Sequence[DeferredStage]) -> str | None: """Render declared deferred stages so they carry into the plan's markdown. @@ -290,6 +307,9 @@ def render_canonical_plan_revision( ] ) ) + human_section = render_human_requirement_dispositions(parsed_revision.human_requirement_dispositions) + if human_section: + sections.append(human_section) deferred_section = render_deferred_stages_section(parsed_revision.deferred_stages) if deferred_section: sections.append(deferred_section) @@ -433,6 +453,11 @@ def _render_public_plan_review_comment( config=config, ) ) + human_section = render_human_requirement_dispositions( + parsed_review.human_requirement_dispositions + ) + if human_section: + sections.append(human_section) footer: list[str] = [] if human_requirements_resolved_flag: footer.append("") @@ -568,6 +593,9 @@ def _render_public_plan_state_comment( parsed_plan.summary.strip(), "\n".join(["### Plan steps", render_canonical_plan_steps(parsed_plan.plan_steps)]), ] + human_section = render_human_requirement_dispositions(parsed_plan.human_requirement_dispositions) + if human_section: + sections.append(human_section) deferred_section = render_deferred_stages_section(parsed_plan.deferred_stages) if deferred_section: sections.append(deferred_section) diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index d8da12a..d91eb42 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -148,6 +148,7 @@ review_freeform_summary_text, normalize_response_file_structured_text, validate_human_requirements_acknowledgement, + validate_human_requirement_dispositions, validate_structured_coder_followup, validate_structured_plan_state, validate_structured_plan_revision, @@ -2296,6 +2297,12 @@ def _validate_response_with_human_requirements( surfaced_requirement_ids=prompt_context.surfaced_requirement_ids, requires_direct_discussion_ack=prompt_context.requires_direct_discussion_ack, ) + if hasattr(marker_value, "human_requirement_dispositions"): + validate_human_requirement_dispositions( + marker_value.human_requirement_dispositions, + surfaced_requirement_ids=prompt_context.surfaced_requirement_ids, + context=f"{getattr(marker_value, 'kind', 'structured')}.human_requirement_dispositions", + ) return marker_value @@ -3166,6 +3173,10 @@ def _run_plan_first_loop( reviewer=reviewer_name, unresolved_items=items, current_round_items=round_new_unresolved_items, + surfaced_requirement_ids=_surfaced_reviewer_requirement_ids( + issue_context.human_requirements, + requirement_scope="planning requirements", + ), ), usage_context=usage_context, use_repair=True, @@ -3315,6 +3326,7 @@ def _run_plan_first_loop( reviewer=reviewer_name, unresolved_items=prior_unresolved_items, current_round_items=round_new_unresolved_items, + surfaced_requirement_ids=hr_ids, ), repair_kwargs={ "expected_kind": "plan_review", diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index a97c481..4505f3c 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -442,6 +442,7 @@ def _coder_human_requirements_guidance( HUMAN_REQUIREMENTS_ADDRESSED_MARKER, "", "Then add a `### Human requirements` section.", + "In the structured JSON, also include `human_requirement_dispositions`: one object for every surfaced `Requirement N`, with `requirement_id`, `disposition` (`addressed`, `blocked`, or `not-applicable`), and a concise non-empty `evidence` note.", ] if context.surfaced_requirement_ids: surfaced = ", ".join(f"`{item}`" for item in context.surfaced_requirement_ids) @@ -468,12 +469,19 @@ def _human_requirements_review_guidance( return f"""{requirement_label.capitalize()} override AI reviewer preferences unless they are unsafe, impossible, or contradicted by a later signed human instruction. Verify each requirement in this set before approving. If all surfaced signed -human requirements are addressed or explicitly resolved, an approved +human requirements have complete structured dispositions and the addressed +evidence is concretely covered by the current canonical plan, an approved review must include exactly: If any signed human requirement in this set is unresolved, return blocking. +For an `addressed` disposition, compare the evidence to the canonical plan and +return blocking when it lacks concrete coverage. A named external integration +is distinct from a similarly purposed local UI: a Grafana request requires the +Grafana dashboard/provisioning or another named integration artifact, not merely +an `admin.html` view. Use `blocked` or `not-applicable` only with a visible +reason that you explicitly accept before approving. """ @@ -695,17 +703,27 @@ def _plan_review_schema_and_rules() -> str: "future_followups": ["Consider a later cleanup pass."], "prior_plan_item_dispositions": [ {"item_id": "item-1", "disposition": "resolved", "note": "Covered by the revised tests."} + ], + "human_requirement_dispositions": [ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan names the requested artifact."} ] } -- reviewer signature shown in the volatile tail If signed human requirements are present in the stable prefix and are fully -addressed or explicitly resolved, include exactly this marker before the +addressed or explicitly resolved with complete dispositions and concrete +canonical-plan coverage, include exactly this marker before the `AGENT_PLAN_STATE` footer: +Compare every `addressed` evidence note with the current canonical plan. A +named external integration such as Grafana requires its dashboard, +provisioning, or other named integration artifact in the plan; an `admin.html` +view alone is not coverage. If the plan cannot satisfy the request, record a +visible `blocked` or `not-applicable` reason and approve only if you accept it. + Blocking plan issues and Same-plan follow-ups both prevent approval. Same-plan follow-ups are small current-plan refinements that must be incorporated before implementation starts; they may appear only in blocking plan reviews. Future @@ -965,11 +983,16 @@ def build_issue_plan_prompt( "kind": "plan_state", "state": "blocking", "summary": "", - "plan_steps": [""] + "plan_steps": [""], + "human_requirement_dispositions": [ + {{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Step 1 names the requested deliverable."}} + ] }} `plan_steps` must be a non-empty list of non-empty strings and should cover the intended approach, key files or areas to change, edge cases, and test strategy. +When signed requirements are surfaced, the disposition array must contain every +generated `Requirement N` exactly once; when none are surfaced, it must be empty. The optional `deferred_stages` field is a list of objects with non-empty `title` and `summary` strings. Do not substitute a generic `implementation_plan` object or markdown plan for this @@ -1087,6 +1110,9 @@ def build_plan_review_prompt( "future_followups": ["Consider a later cleanup pass."], "prior_plan_item_dispositions": [ {{"item_id": "item-1", "disposition": "resolved", "note": "Covered by the revised tests."}} + ], + "human_requirement_dispositions": [ + {{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Step 2 names the requested integration artifact."}} ] }} @@ -2468,6 +2494,9 @@ def build_discuss_agenda_prompt( "question_for_next_round": "Would splitting the API boundary into its own issue resolve the scope objection?" }} ], + "human_requirement_dispositions": [ + {{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Step 2 names the requested integration artifact."}} + ], "missing_facts": ["Whether the API boundary is already specified."]{research_example} }} diff --git a/src/coding_review_agent_loop/protocol.py b/src/coding_review_agent_loop/protocol.py index 784ebd6..9e544c2 100644 --- a/src/coding_review_agent_loop/protocol.py +++ b/src/coding_review_agent_loop/protocol.py @@ -150,6 +150,7 @@ class ParsedPlanReview: items: PlanReviewItems dispositions: tuple[ReviewItemDisposition, ...] raw_dispositions_text: str = "" + human_requirement_dispositions: tuple["HumanRequirementDisposition", ...] = () @dataclass(frozen=True) @@ -174,6 +175,7 @@ class StructuredPlanReview: same_plan_followups: tuple[str, ...] future_followups: tuple[str, ...] prior_plan_item_dispositions: tuple[ReviewItemDisposition, ...] + human_requirement_dispositions: tuple["HumanRequirementDisposition", ...] = () @dataclass(frozen=True) @@ -182,6 +184,18 @@ class StructuredHumanRequirementsPayload: checked_discussion_directly: bool +@dataclass(frozen=True) +class HumanRequirementDisposition: + requirement_id: str + disposition: str + evidence: str + + +HUMAN_REQUIREMENT_DISPOSITION_VALUES = frozenset( + {"addressed", "blocked", "not-applicable"} +) + + @dataclass(frozen=True) class StructuredCoderFollowup: schema_version: int @@ -219,6 +233,7 @@ class StructuredPlanRevision: prior_plan_item_dispositions: tuple[ReviewItemDisposition, ...] plan_steps: tuple[str, ...] deferred_stages: tuple[DeferredStage, ...] = () + human_requirement_dispositions: tuple[HumanRequirementDisposition, ...] = () @dataclass(frozen=True) @@ -229,6 +244,7 @@ class StructuredPlanState: summary: str plan_steps: tuple[str, ...] deferred_stages: tuple[DeferredStage, ...] = () + human_requirement_dispositions: tuple[HumanRequirementDisposition, ...] = () @dataclass(frozen=True) @@ -895,6 +911,60 @@ def _expect_requirement_id_list(value: object, *, context: str) -> tuple[str, .. return tuple(rendered) +def _expect_human_requirement_dispositions( + value: object, + *, + context: str, +) -> tuple[HumanRequirementDisposition, ...]: + if not isinstance(value, list): + raise AgentLoopError(f"{context} must be a JSON array.") + result: list[HumanRequirementDisposition] = [] + for index, item in enumerate(value): + item_context = f"{context}[{index}]" + payload = _expect_object(item, context=item_context) + _expect_exact_keys( + payload, + context=item_context, + required={"requirement_id", "disposition", "evidence"}, + ) + requirement_id = _normalize_requirement_label( + _expect_non_empty_string(payload["requirement_id"], context=f"{item_context}.requirement_id") + ) + disposition = _expect_non_empty_string( + payload["disposition"], context=f"{item_context}.disposition" + ) + if disposition not in HUMAN_REQUIREMENT_DISPOSITION_VALUES: + raise AgentLoopError( + f"{item_context}.disposition must be one of: addressed, blocked, not-applicable" + ) + evidence = _expect_non_empty_string( + payload["evidence"], context=f"{item_context}.evidence" + ) + result.append(HumanRequirementDisposition(requirement_id, disposition, evidence)) + return tuple(result) + + +def validate_human_requirement_dispositions( + dispositions: Sequence[HumanRequirementDisposition], + *, + surfaced_requirement_ids: Sequence[str], + context: str = "human_requirement_dispositions", +) -> None: + expected = tuple(_normalize_requirement_label(item) for item in surfaced_requirement_ids) + actual = [item.requirement_id for item in dispositions] + duplicates = sorted({item for item in actual if actual.count(item) > 1}) + if duplicates: + raise AgentLoopError(f"{context} contains duplicate requirement ID(s): {', '.join(duplicates)}") + unknown = sorted(set(actual) - set(expected)) + if unknown: + raise AgentLoopError(f"{context} contains unknown requirement ID(s): {', '.join(unknown)}") + missing = sorted(set(expected) - set(actual)) + if missing: + raise AgentLoopError(f"{context} is missing requirement ID(s): {', '.join(missing)}") + if not expected and actual: + raise AgentLoopError(f"{context} must be empty when no signed human requirements are surfaced.") + + def _extract_structured_response_object(text: str) -> dict[str, object] | None: stripped = text.strip() if not stripped: @@ -1246,6 +1316,7 @@ def _finalize_parsed_plan_review( items: PlanReviewItems, dispositions: tuple[ReviewItemDisposition, ...], raw_dispositions_text: str = "", + human_requirement_dispositions: tuple[HumanRequirementDisposition, ...] = (), ) -> ParsedPlanReview: if state == "blocking" and items.future: items = PlanReviewItems(blocking=items.blocking, same_plan=items.same_plan, future=()) @@ -1269,6 +1340,7 @@ def _finalize_parsed_plan_review( items=items, dispositions=dispositions, raw_dispositions_text=raw_dispositions_text, + human_requirement_dispositions=human_requirement_dispositions, ) @@ -1406,7 +1478,7 @@ def parse_structured_plan_review(text: str, *, reviewer: str) -> ParsedPlanRevie "summary", "prior_plan_item_dispositions", }, - optional={"blocking_plan_issues", "same_plan_followups", "future_followups"}, + optional={"blocking_plan_issues", "same_plan_followups", "future_followups", "human_requirement_dispositions"}, ) state = _expect_state(payload["state"], context="plan_review.state") summary = review_freeform_summary_text( @@ -1437,6 +1509,10 @@ def parse_structured_plan_review(text: str, *, reviewer: str) -> ParsedPlanRevie allowed_same_status="same-plan", is_plan_review=True, ) + human_requirement_dispositions = _expect_human_requirement_dispositions( + payload.get("human_requirement_dispositions", []), + context="plan_review.human_requirement_dispositions", + ) items = _dedupe_plan_review_items( PlanReviewItems( blocking=_structured_followups(blocking_items, reviewer=reviewer), @@ -1449,6 +1525,7 @@ def parse_structured_plan_review(text: str, *, reviewer: str) -> ParsedPlanRevie summary=summary, items=items, dispositions=dispositions, + human_requirement_dispositions=human_requirement_dispositions, ) @@ -1586,7 +1663,7 @@ def validate_structured_plan_revision(text: str) -> StructuredPlanRevision | Non "prior_plan_item_dispositions", "plan_steps", }, - optional={"deferred_stages"}, + optional={"deferred_stages", "human_requirement_dispositions"}, ) state = _expect_non_empty_string(payload["state"], context="plan_revision.state") if state != "blocking": @@ -1608,6 +1685,10 @@ def validate_structured_plan_revision(text: str) -> StructuredPlanRevision | Non deferred_stages = _expect_deferred_stage_list( payload, "deferred_stages", context="plan_revision.deferred_stages" ) + human_requirement_dispositions = _expect_human_requirement_dispositions( + payload.get("human_requirement_dispositions", []), + context="plan_revision.human_requirement_dispositions", + ) return StructuredPlanRevision( schema_version=1, kind="plan_revision", @@ -1616,6 +1697,7 @@ def validate_structured_plan_revision(text: str) -> StructuredPlanRevision | Non prior_plan_item_dispositions=dispositions, plan_steps=plan_steps, deferred_stages=deferred_stages, + human_requirement_dispositions=human_requirement_dispositions, ) @@ -1631,7 +1713,7 @@ def validate_structured_plan_state(text: str) -> StructuredPlanState | None: payload, context="plan_state", required={"schema_version", "kind", "state", "summary", "plan_steps"}, - optional={"deferred_stages"}, + optional={"deferred_stages", "human_requirement_dispositions"}, ) state = _expect_state(payload["state"], context="plan_state.state") if state != "blocking": @@ -1650,6 +1732,10 @@ def validate_structured_plan_state(text: str) -> StructuredPlanState | None: deferred_stages=_expect_deferred_stage_list( payload, "deferred_stages", context="plan_state.deferred_stages" ), + human_requirement_dispositions=_expect_human_requirement_dispositions( + payload.get("human_requirement_dispositions", []), + context="plan_state.human_requirement_dispositions", + ), ) diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index b593000..aa9b740 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -238,6 +238,9 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "future_followups": [], "prior_plan_item_dispositions": [ {"item_id": "item-1", "disposition": "resolved"} + ], + "human_requirement_dispositions": [ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Covered by the current plan."} ] } @@ -401,7 +404,10 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "kind": "plan_state", "state": "blocking", "summary": "", - "plan_steps": ["Update the parser.", "Add regression tests."] + "plan_steps": ["Update the parser.", "Add regression tests."], + "human_requirement_dispositions": [ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Covered by the current plan."} + ] } @@ -428,6 +434,9 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st {"item_id": "item-1", "disposition": "resolved", "note": "covered by the revised plan"} ], "plan_steps": ["Update the parser.", "Add regression tests."] + ,"human_requirement_dispositions": [ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Covered by the revised plan."} + ] } @@ -1268,6 +1277,7 @@ def _reviewer_human_requirements_instruction( "This approved review is missing .\n" f"Surfaced signed human requirements:\n{rendered_ids}\n" "For each listed requirement, confirm whether the current plan/PR satisfies it.\n" + "The JSON must include one `human_requirement_dispositions` object per listed requirement, with exact `Requirement N` ID, disposition `addressed`, `blocked`, or `not-applicable`, and non-empty evidence.\n" "If ALL requirements are satisfied: keep state `approved` and add " f"`` after the JSON and before the `` footer.\n" "If ANY requirement is NOT satisfied: change state to `blocking` and add a concrete " diff --git a/src/coding_review_agent_loop/unresolved_items.py b/src/coding_review_agent_loop/unresolved_items.py index 51c43e9..223ff58 100644 --- a/src/coding_review_agent_loop/unresolved_items.py +++ b/src/coding_review_agent_loop/unresolved_items.py @@ -18,6 +18,7 @@ validate_human_requirements_acknowledgement, validate_structured_coder_followup, validate_structured_human_requirements_acknowledgement, + validate_human_requirement_dispositions, ) HUMAN_REQUIREMENTS_ACK_ITEM_ID = "item-human-requirements-acknowledgement" @@ -443,8 +444,14 @@ def _validate_plan_review_response( reviewer: str, unresolved_items: Sequence[UnresolvedReviewItem], current_round_items: Sequence[UnresolvedReviewItem] = (), + surfaced_requirement_ids: Sequence[str] = (), ) -> ParsedPlanReview: parsed = parse_plan_review(text, reviewer=reviewer) + validate_human_requirement_dispositions( + parsed.human_requirement_dispositions, + surfaced_requirement_ids=surfaced_requirement_ids, + context="plan_review.human_requirement_dispositions", + ) unresolved_by_id = {item.item_id: item for item in unresolved_items} dispositions = _maybe_fill_resolved_dispositions_from_prose( diff --git a/tests/test_human_requirement_dispositions.py b/tests/test_human_requirement_dispositions.py new file mode 100644 index 0000000..b159be8 --- /dev/null +++ b/tests/test_human_requirement_dispositions.py @@ -0,0 +1,74 @@ +import json + +import pytest + +from coding_review_agent_loop.errors import AgentLoopError +from coding_review_agent_loop.protocol import ( + validate_human_requirement_dispositions, + validate_structured_plan_state, +) + + +def _plan(dispositions): + payload = { + "schema_version": 1, + "kind": "plan_state", + "state": "blocking", + "summary": "Plan the requested integration.", + "plan_steps": ["Add the Grafana dashboard provisioning artifact."], + "human_requirement_dispositions": dispositions, + } + return json.dumps(payload) + "\n\n-- coder" + + +def test_signed_requirement_requires_exact_structured_disposition(): + parsed = validate_structured_plan_state( + _plan( + [{ + "requirement_id": "Requirement 1", + "disposition": "addressed", + "evidence": "The plan names Grafana dashboard provisioning.", + }] + ) + ) + validate_human_requirement_dispositions( + parsed.human_requirement_dispositions, + surfaced_requirement_ids=("Requirement 1",), + ) + + +@pytest.mark.parametrize( + "dispositions, message", + [ + ([], "missing"), + ([{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "x"}, + {"requirement_id": "Requirement 1", "disposition": "blocked", "evidence": "y"}], "duplicate"), + ([{"requirement_id": "Requirement 2", "disposition": "addressed", "evidence": "x"}], "unknown"), + ], +) +def test_invalid_requirement_coverage_is_rejected(dispositions, message): + parsed = validate_structured_plan_state(_plan(dispositions)) + with pytest.raises(AgentLoopError, match=message): + validate_human_requirement_dispositions( + parsed.human_requirement_dispositions, + surfaced_requirement_ids=("Requirement 1",), + ) + + +def test_invalid_status_and_empty_evidence_are_rejected_at_schema_boundary(): + with pytest.raises(AgentLoopError, match="disposition"): + validate_structured_plan_state( + _plan([{"requirement_id": "Requirement 1", "disposition": "maybe", "evidence": "x"}]) + ) + with pytest.raises(AgentLoopError, match="evidence"): + validate_structured_plan_state( + _plan([{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": ""}]) + ) + + +def test_no_signed_requirements_requires_empty_collection(): + parsed = validate_structured_plan_state(_plan([])) + validate_human_requirement_dispositions( + parsed.human_requirement_dispositions, + surfaced_requirement_ids=(), + ) From 49c5c5f4e2b0b79f8a5072327d453d6b59093127 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 01:52:23 -0700 Subject: [PATCH 2/5] Fix plan human requirement approval gating --- src/coding_review_agent_loop/orchestrator.py | 99 ++++++++++-- src/coding_review_agent_loop/repair.py | 4 +- tests/test_human_requirement_dispositions.py | 33 ++++ tests/test_orchestrator_issue.py | 150 ++++++++++++++++++- 4 files changed, 271 insertions(+), 15 deletions(-) diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index d91eb42..12053f7 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -2269,7 +2269,7 @@ def _require_pr_number_or_clarification(text: str) -> int | str: ) -def _require_plan_state_or_clarification(text: str) -> str: +def _require_plan_state_or_clarification(text: str) -> StructuredPlanState | str: if is_clarification_request(text): return "clarification" structured_plan = validate_structured_plan_state(text) @@ -2277,7 +2277,7 @@ def _require_plan_state_or_clarification(text: str) -> str: raise AgentLoopError( "Initial planning response must include a structured `plan_state` JSON object." ) - return structured_plan.state + return structured_plan def _validate_response_with_human_requirements( text: str, *, @@ -2306,6 +2306,35 @@ def _validate_response_with_human_requirements( return marker_value +def _current_plan_has_complete_human_requirement_dispositions( + coder_output: str | None, + *, + surfaced_requirement_ids: Sequence[str], +) -> bool: + """Return whether the current coder plan has the required attestation. + + The raw structured coder response is retained in round metadata specifically + so resume can apply this same gate to a canonical markdown revision. + """ + if coder_output is None: + return False + try: + try: + parsed = validate_structured_plan_state(coder_output) + except AgentLoopError: + parsed = validate_structured_plan_revision(coder_output) + if parsed is None: + return False + validate_human_requirement_dispositions( + parsed.human_requirement_dispositions, + surfaced_requirement_ids=surfaced_requirement_ids, + context=f"{parsed.kind}.human_requirement_dispositions", + ) + except AgentLoopError: + return False + return True + + def _merge_human_requirements( issue_context: IssueContext | None, pr_context: PullRequestReviewContext, @@ -3074,8 +3103,10 @@ def _run_plan_first_loop( ) start_round_number = 1 resumed_round: ResumedReviewRound | None = None + current_coder_output = plan_output else: current_plan, resumed_round = resume_state + current_coder_output = resumed_round.coder_output unresolved_items = list(resumed_round.prior_items) compact_prior_summaries = list(resumed_round.compact_prior_summaries) next_unresolved_item_number = resumed_round.next_unresolved_item_number @@ -3297,16 +3328,60 @@ def _run_plan_first_loop( unresolved_items = [*unresolved_items, *round_new_unresolved_items] must_fix_items = [item for item in unresolved_items if item.status in {"blocking", "same-plan"}] if all_approved and not must_fix_items and issue_context.human_requirements: - missing_acknowledgements = [ - reviewer_name - for reviewer_name, review_output in approved_review_outputs - if not human_requirements_resolved(review_output) - ] + hr_ids = _surfaced_reviewer_requirement_ids( + issue_context.human_requirements, + requirement_scope="planning requirements", + ) + coder_dispositions_complete = _current_plan_has_complete_human_requirement_dispositions( + current_coder_output, + surfaced_requirement_ids=hr_ids, + ) + missing_acknowledgements = ( + [reviewer_name for reviewer_name, _review_output in approved_review_outputs] + if not coder_dispositions_complete + else [ + reviewer_name + for reviewer_name, review_output in approved_review_outputs + if not human_requirements_resolved(review_output) + ] + ) if missing_acknowledgements: - hr_ids = _surfaced_reviewer_requirement_ids( - issue_context.human_requirements, - requirement_scope="planning requirements", - ) + if not coder_dispositions_complete: + log( + config, + f"Planning round {round_number}: current coder plan lacks complete " + "human requirement dispositions; re-injecting as blocking plan item", + ) + synthetic_review = ( + "Orchestrator plan review:\n\n" + "The current canonical coder plan lacks complete structured dispositions " + "for the signed human requirements. Coder must provide one valid " + "disposition with evidence for every surfaced requirement before a " + "reviewer approval marker can be accepted." + ) + blocking_reviews.append(("Orchestrator", synthetic_review)) + round_new_unresolved_items.append( + _next_unresolved_item( + item_number=next_unresolved_item_number, + reviewer="Orchestrator", + source_round=round_number, + text=( + "The current canonical coder plan lacks complete structured " + "dispositions for the signed human requirements. Coder must provide " + "one valid disposition with evidence for every surfaced requirement " + "before a reviewer approval marker can be accepted." + ), + status="blocking", + ) + ) + next_unresolved_item_number += 1 + unresolved_items = [*unresolved_items, round_new_unresolved_items[-1]] + must_fix_items = [ + item for item in unresolved_items if item.status in {"blocking", "same-plan"} + ] + all_approved = False + # A reviewer cannot repair a missing coder attestation. + missing_acknowledgements = [] still_missing = [] for reviewer_name, review_output in approved_review_outputs: if human_requirements_resolved(review_output): @@ -3748,6 +3823,7 @@ def _run_plan_first_loop( plan_response.marker_value, must_fix_items, config ) current_plan = canonical_plan + current_coder_output = plan_response.text public_comment = render_public_agent_comment( kind="plan_revision", parsed=plan_response.marker_value, @@ -3759,6 +3835,7 @@ def _run_plan_first_loop( ) else: current_plan = plan_response.text + current_coder_output = plan_response.text public_comment = normalize_freeform_signature( plan_response.text, agent=config.coder, config=config, model_used=plan_response.model_used ) diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index aa9b740..f38e5e6 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -433,8 +433,8 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "prior_plan_item_dispositions": [ {"item_id": "item-1", "disposition": "resolved", "note": "covered by the revised plan"} ], - "plan_steps": ["Update the parser.", "Add regression tests."] - ,"human_requirement_dispositions": [ + "plan_steps": ["Update the parser.", "Add regression tests."], + "human_requirement_dispositions": [ {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Covered by the revised plan."} ] } diff --git a/tests/test_human_requirement_dispositions.py b/tests/test_human_requirement_dispositions.py index b159be8..e1cae35 100644 --- a/tests/test_human_requirement_dispositions.py +++ b/tests/test_human_requirement_dispositions.py @@ -2,7 +2,9 @@ import pytest +import coding_review_agent_loop.orchestrator as orchestrator from coding_review_agent_loop.errors import AgentLoopError +from coding_review_agent_loop.github import HumanReviewRequirement from coding_review_agent_loop.protocol import ( validate_human_requirement_dispositions, validate_structured_plan_state, @@ -72,3 +74,34 @@ def test_no_signed_requirements_requires_empty_collection(): parsed.human_requirement_dispositions, surfaced_requirement_ids=(), ) + + +def test_initial_plan_validator_checks_coder_dispositions(): + with pytest.raises(AgentLoopError, match="missing requirement ID"): + orchestrator._validate_response_with_human_requirements( + _plan([]).replace( + "\n", + "\n\n\n### Human requirements\n" + "- Requirement 1: Grafana is planned.\n" + "", + ), + marker_validator=orchestrator._require_plan_state_or_clarification, + human_requirements=( + HumanReviewRequirement( + source_type="Issue body", + author="maintainer", + created_at=None, + url=None, + body="Provide Grafana.", + ), + ), + requirement_scope="planning requirements", + full_omission_fallback="Fetch the discussion.", + ) + + +def test_current_plan_gate_rejects_missing_coder_dispositions(): + assert not orchestrator._current_plan_has_complete_human_requirement_dispositions( + _plan([]), + surfaced_requirement_ids=("Requirement 1",), + ) diff --git a/tests/test_orchestrator_issue.py b/tests/test_orchestrator_issue.py index fcdc89c..42bb81a 100644 --- a/tests/test_orchestrator_issue.py +++ b/tests/test_orchestrator_issue.py @@ -65,10 +65,42 @@ def __init__(self, **kwargs): else output for output in outputs ] + # Signed-requirement tests written before the typed disposition + # contract focus on their named acknowledgement behavior. Supply the + # otherwise-irrelevant complete attestation to their structured plan + # fixtures, while preserving intentionally missing acknowledgement + # markers for the tests that reject those. + issue_body = str((kwargs.get("issue_payload") or {}).get("body") or "") + if "-- Human Reviewer" in issue_body: + for output_key in ("claude_outputs", "codex_outputs", "gemini_outputs", "antigravity_outputs"): + outputs = kwargs.get(output_key) + if outputs is None: + continue + kwargs[output_key] = [_add_default_requirement_disposition(output) for output in outputs] kwargs.setdefault("pr_payload", {"body": "Fixes #56"}) super().__init__(**kwargs) +def _add_default_requirement_disposition(output: str) -> str: + try: + payload, end = json.JSONDecoder().raw_decode(output.lstrip()) + except (json.JSONDecodeError, ValueError): + return output + if not isinstance(payload, dict) or payload.get("kind") not in { + "plan_state", "plan_revision", "plan_review" + }: + return output + if "human_requirement_dispositions" in payload: + return output + payload["human_requirement_dispositions"] = [{ + "requirement_id": "Requirement 1", + "disposition": "addressed", + "evidence": "The structured plan covers the signed requirement.", + }] + prefix = output[: len(output) - len(output.lstrip())] + return prefix + json.dumps(payload) + output.lstrip()[end:] + + def _initial_plan_state(*, summary: str = "Initial plan.", human_requirements: str = "") -> str: return structured_plan_state(summary=summary).replace( "\n", @@ -77,6 +109,62 @@ def _initial_plan_state(*, summary: str = "Initial plan.", human_requirements: s ) +def _plan_with_requirement_disposition( + *, + kind: str, + summary: str, + plan_steps: list[str], + evidence: str, + state: str = "blocking", +) -> str: + payload = { + "schema_version": 1, + "kind": kind, + "state": state, + "summary": summary, + "plan_steps": plan_steps, + "human_requirement_dispositions": [{ + "requirement_id": "Requirement 1", + "disposition": "addressed", + "evidence": evidence, + }], + } + if kind == "plan_revision": + payload["prior_plan_item_dispositions"] = [] + return ( + json.dumps(payload) + + "\n\n\n### Human requirements\n" + + f"- Requirement 1: {evidence}\n" + + f"\n-- Anthropic Claude" + ) + + +def _plan_review_with_requirement_disposition( + *, state: str, summary: str, marker: bool = False, prior_dispositions: list[dict[str, str]] | None = None +) -> str: + return ( + json.dumps( + { + "schema_version": 1, + "kind": "plan_review", + "state": state, + "summary": summary, + "blocking_plan_issues": [summary] if state == "blocking" else [], + "same_plan_followups": [], + "future_followups": [], + "prior_plan_item_dispositions": prior_dispositions or [], + "human_requirement_dispositions": [{ + "requirement_id": "Requirement 1", + "disposition": "addressed", + "evidence": summary, + }], + } + ) + + ("\n" if marker else "") + + f"\n\n-- OpenAI Codex" + ) + + def test_issue_loop_creates_pr_then_alternates_until_codex_approval(tmp_path): runner = FakeRunner( claude_outputs=[ @@ -560,7 +648,7 @@ def test_issue_loop_plan_revision_repair_preserves_signed_human_requirements(tmp "- Requirement 1: the revised plan preserves backward compatibility.\n\n" "\n-- Anthropic Claude" ) - repaired_revision = structured_plan_revision( + repaired_revision = _add_default_requirement_disposition(structured_plan_revision( summary="Revised plan with compatibility tests.", prior_plan_item_dispositions=[ { @@ -575,7 +663,7 @@ def test_issue_loop_plan_revision_repair_preserves_signed_human_requirements(tmp "### Human requirements\n" "- Requirement 1: the revised plan preserves backward compatibility.\n" ), - ) + )) runner = FakeRunner( issue_payload={ "author": {"login": "maintainer"}, @@ -971,6 +1059,64 @@ def test_issue_loop_plan_first_requires_reviewer_human_requirements_resolution(t captured = capsys.readouterr() assert "approved without acknowledging signed human requirements" in captured.err + +def test_plan_first_grafana_requirement_blocks_admin_html_only_plan_then_accepts_integration(tmp_path): + """A reviewer must reject a lookalike local UI until the named integration is planned.""" + runner = FakeRunner( + issue_payload={ + "body": "Provision a Grafana dashboard for verification attribution.\n\n-- Human Reviewer", + }, + claude_outputs=[ + _plan_with_requirement_disposition( + kind="plan_state", + summary="Add verification attribution to the operations UI.", + plan_steps=["Add verification attribution to admin.html."], + evidence="admin.html will display verification attribution.", + ), + _plan_with_requirement_disposition( + kind="plan_revision", + summary="Provision the requested Grafana dashboard.", + plan_steps=[ + "Add Grafana dashboard provisioning for verification attribution.", + "Test the dashboard provisioning artifact.", + ], + evidence="The plan names Grafana dashboard provisioning and its artifact.", + ), + ], + codex_outputs=[ + _plan_review_with_requirement_disposition( + state="blocking", + summary="admin.html does not cover the requested Grafana dashboard provisioning.", + ), + _plan_review_with_requirement_disposition( + state="approved", + summary="The revised canonical plan covers the Grafana provisioning artifact.", + marker=True, + prior_dispositions=[ + {"item_id": "item-1", "disposition": "resolved", "note": "Grafana is now planned."} + ], + ), + ], + ) + config = make_config( + tmp_path, + coder="claude", + reviewer=("codex",), + max_rounds=2, + plan_execution_mode="plan-only", + ) + + assert run_issue_loop(runner, issue_number=56, config=config, plan_first=True) == 0 + + review_prompts = [ + command[-1] + for command, _cwd in runner.commands + if command[:2] == ["codex", "exec"] + ] + assert any("admin.html" in prompt and "Grafana" in prompt for prompt in review_prompts) + assert "admin.html does not cover the requested Grafana" in runner.comments[1] + assert "Provision the requested Grafana dashboard" in runner.comments[-1] + def test_issue_loop_plan_first_uses_full_context_when_plan_ledger_incomplete(tmp_path, capsys): old_plan = "Old plan.\n\n-- Anthropic Claude" new_plan = "New plan.\n\n-- Anthropic Claude" From 59f78167ee2b73e1a5494434b6f9f4c482c0b22c Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 02:02:59 -0700 Subject: [PATCH 3/5] Fix discuss agenda prompt schema --- src/coding_review_agent_loop/prompts.py | 3 --- tests/test_prompts.py | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index 4505f3c..e236898 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -2494,9 +2494,6 @@ def build_discuss_agenda_prompt( "question_for_next_round": "Would splitting the API boundary into its own issue resolve the scope objection?" }} ], - "human_requirement_dispositions": [ - {{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Step 2 names the requested integration artifact."}} - ], "missing_facts": ["Whether the API boundary is already specified."]{research_example} }} diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 66989b8..9b89578 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -2143,6 +2143,7 @@ def test_build_discuss_agenda_prompt_includes_every_round_of_history(tmp_path): assert "never invent positions" in prompt assert "Carry forward unresolved `missing_facts`" in prompt assert '"kind": "discuss_agenda"' in prompt + assert '"human_requirement_dispositions"' not in prompt def test_build_discuss_agenda_prompt_supports_answer_history(tmp_path): From d6e1af9f73be5815ffe54bdcb572eb11fbb291d4 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 02:16:28 -0700 Subject: [PATCH 4/5] Fix plan disposition acknowledgement recovery --- src/coding_review_agent_loop/orchestrator.py | 30 ++++++++++++++++---- tests/agent_loop_helpers.py | 9 ++++++ tests/test_orchestrator_issue.py | 2 +- tests/test_repair.py | 18 ++++++++++-- 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index 12053f7..5cc353f 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -956,7 +956,7 @@ def _recover_plan_revision_human_requirements_acknowledgement( ) return None - valid_blocks: list[tuple[str, str]] = [] + valid_blocks: list[tuple[str, str, tuple[object, ...]]] = [] invalid_count = 0 incomplete_count = 0 for source, source_text in _candidate_source_texts(result): @@ -971,18 +971,26 @@ def _recover_plan_revision_human_requirements_acknowledgement( surfaced_requirement_ids=context.surfaced_requirement_ids, requires_direct_discussion_ack=context.requires_direct_discussion_ack, ) + source_plan = validate_structured_plan_revision(source_text) + if source_plan is None: + raise AgentLoopError("captured response was not a structured plan revision") + validate_human_requirement_dispositions( + source_plan.human_requirement_dispositions, + surfaced_requirement_ids=context.surfaced_requirement_ids, + context="plan_revision.human_requirement_dispositions", + ) except AgentLoopError: invalid_count += 1 continue - valid_blocks.append((source, block)) + valid_blocks.append((source, block, source_plan.human_requirement_dispositions)) - unique_valid: list[tuple[str, str]] = [] + unique_valid: list[tuple[str, str, tuple[object, ...]]] = [] seen_blocks: set[str] = set() - for source, block in valid_blocks: + for source, block, dispositions in valid_blocks: if block in seen_blocks: continue seen_blocks.add(block) - unique_valid.append((source, block)) + unique_valid.append((source, block, dispositions)) if len(unique_valid) != 1: if len(unique_valid) > 1: @@ -997,7 +1005,17 @@ def _recover_plan_revision_human_requirements_acknowledgement( return None json_prefix, footer_and_signature = split - source, block = unique_valid[0] + source, block, dispositions = unique_valid[0] + payload = json.loads(json_prefix) + payload["human_requirement_dispositions"] = [ + { + "requirement_id": item.requirement_id, + "disposition": item.disposition, + "evidence": item.evidence, + } + for item in dispositions + ] + json_prefix = json.dumps(payload) recovered_text = f"{json_prefix}\n{block}\n{footer_and_signature}" try: marker_value = validate(recovered_text) diff --git a/tests/agent_loop_helpers.py b/tests/agent_loop_helpers.py index fd9677a..e650c7a 100644 --- a/tests/agent_loop_helpers.py +++ b/tests/agent_loop_helpers.py @@ -914,6 +914,7 @@ def structured_plan_review( prior_plan_item_dispositions: list[dict[str, str]] | None = None, reviewer: str = "OpenAI Codex", human_requirements_resolved: bool = False, + human_requirement_dispositions: list[dict[str, str]] | None = None, ) -> str: return ( json.dumps( @@ -926,6 +927,7 @@ def structured_plan_review( "same_plan_followups": same_plan_followups or [], "future_followups": future_followups or [], "prior_plan_item_dispositions": prior_plan_item_dispositions or [], + "human_requirement_dispositions": human_requirement_dispositions or [], } ) + ("\n" if human_requirements_resolved else "") @@ -940,6 +942,7 @@ def structured_plan_revision( plan_steps: list[str] | None = None, reviewer: str = "Anthropic Claude", human_requirements: str = "", + human_requirement_dispositions: list[dict[str, str]] | None = None, deferred_stages: list[dict[str, str]] | None = None, ) -> str: payload = { @@ -949,6 +952,10 @@ def structured_plan_revision( "summary": summary, "prior_plan_item_dispositions": prior_plan_item_dispositions or [], "plan_steps": plan_steps or ["Update the plan.", "Run the relevant tests."], + "human_requirement_dispositions": human_requirement_dispositions or ( + [{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The plan covers the signed requirement."}] + if human_requirements else [] + ), } if deferred_stages is not None: payload["deferred_stages"] = deferred_stages @@ -967,6 +974,7 @@ def structured_plan_state( plan_steps: list[str] | None = None, reviewer: str = "Anthropic Claude", deferred_stages: list[dict[str, str]] | None = None, + human_requirement_dispositions: list[dict[str, str]] | None = None, ) -> str: payload = { "schema_version": 1, @@ -974,6 +982,7 @@ def structured_plan_state( "state": state, "summary": summary, "plan_steps": plan_steps or ["Update the code.", "Run the relevant tests."], + "human_requirement_dispositions": human_requirement_dispositions or [], } if deferred_stages is not None: payload["deferred_stages"] = deferred_stages diff --git a/tests/test_orchestrator_issue.py b/tests/test_orchestrator_issue.py index 42bb81a..57867e5 100644 --- a/tests/test_orchestrator_issue.py +++ b/tests/test_orchestrator_issue.py @@ -90,7 +90,7 @@ def _add_default_requirement_disposition(output: str) -> str: "plan_state", "plan_revision", "plan_review" }: return output - if "human_requirement_dispositions" in payload: + if payload.get("human_requirement_dispositions"): return output payload["human_requirement_dispositions"] = [{ "requirement_id": "Requirement 1", diff --git a/tests/test_repair.py b/tests/test_repair.py index f1a7278..816c258 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -21,7 +21,9 @@ def _initial_plan_with_human_requirements() -> str: "### Human requirements\n" "- Requirement 1: keep the public API unchanged." ) - return structured_plan_state().replace( + return structured_plan_state(human_requirement_dispositions=[ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The plan preserves the public API."} + ]).replace( "\n", acknowledgement + "\n", 1, @@ -1047,7 +1049,9 @@ def test_recover_plan_revision_ack_text_override_uses_stripped_as_base(tmp_path) dirty_text = structured_plan_revision( prior_plan_item_dispositions=[{"item_id": "unknown-prior-item-1", "disposition": "resolved"}], ) - stripped_text = structured_plan_revision() + stripped_text = structured_plan_revision(human_requirement_dispositions=[ + {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The revised plan covers the stripped-base case."} + ]) message_text = structured_plan_revision(human_requirements=ack) result = AgentResult( @@ -1750,11 +1754,13 @@ def test_plan_loop_repair_missing_hr_marker_recovers_approved(tmp_path): state="approved", reviewer="OpenAI Codex", human_requirements_resolved=False, + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan preserves the API."}], ) repaired_with_marker = structured_plan_review( state="approved", reviewer="OpenAI Codex", human_requirements_resolved=True, + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan preserves the API."}], ) runner = FakeRunner( issue_payload=_issue_with_human_requirement(), @@ -1787,12 +1793,14 @@ def test_plan_loop_repair_missing_hr_marker_returns_blocking_not_synthetic(tmp_p state="approved", reviewer="OpenAI Codex", human_requirements_resolved=False, + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan preserves the API."}], ) repaired_blocking = structured_plan_review( state="blocking", summary="Requirement 1 not satisfied: plan changes the public API.", blocking_plan_issues=["Requirement 1 not satisfied: plan changes the public API."], reviewer="OpenAI Codex", + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "blocked", "evidence": "The plan changes the public API."}], ) revision = structured_plan_revision( summary="Revised plan preserving the public API.", @@ -1812,6 +1820,7 @@ def test_plan_loop_repair_missing_hr_marker_returns_blocking_not_synthetic(tmp_p summary="Plan looks sound.", human_requirements_resolved=True, prior_plan_item_dispositions=[{"item_id": "item-1", "disposition": "resolved"}], + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The revised canonical plan preserves the API."}], ), ], ) @@ -1844,6 +1853,7 @@ def test_plan_loop_repair_missing_hr_marker_failure_uses_synthetic(tmp_path): state="approved", reviewer="OpenAI Codex", human_requirements_resolved=False, + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan preserves the API."}], ) revision = structured_plan_revision( summary="Revised plan.", @@ -1863,6 +1873,7 @@ def test_plan_loop_repair_missing_hr_marker_failure_uses_synthetic(tmp_path): summary="Plan looks sound.", human_requirements_resolved=True, prior_plan_item_dispositions=[{"item_id": "item-1", "disposition": "resolved"}], + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The revised canonical plan preserves the API."}], ), ], ) @@ -2036,11 +2047,13 @@ def test_plan_loop_repair_blocking_records_same_plan_followups(tmp_path): state="approved", reviewer="OpenAI Codex", human_requirements_resolved=False, + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The canonical plan preserves the API."}], ) repaired_blocking = structured_plan_review( state="blocking", same_plan_followups=["Add a regression test for the parser edge case."], reviewer="OpenAI Codex", + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "blocked", "evidence": "A parser regression test is required."}], ) revision = structured_plan_revision( summary="Revised plan with regression test.", @@ -2060,6 +2073,7 @@ def test_plan_loop_repair_blocking_records_same_plan_followups(tmp_path): summary="Plan looks sound.", human_requirements_resolved=True, prior_plan_item_dispositions=[{"item_id": "item-1", "disposition": "resolved"}], + human_requirement_dispositions=[{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "The revised canonical plan preserves the API."}], ), ], ) From 4d28168e7c5059196f8377eabdb75366c281a8db Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 11:30:43 -0700 Subject: [PATCH 5/5] Gate plan disposition prompt guidance --- src/coding_review_agent_loop/prompts.py | 23 +++++++++++----- src/coding_review_agent_loop/repair.py | 14 +++++++--- tests/test_prompts.py | 35 +++++++++++++++++++++++++ tests/test_repair.py | 4 +++ 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index e236898..7ab5a64 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -427,6 +427,7 @@ def _coder_human_requirements_guidance( context: CoderHumanRequirementsPromptContext, *, requirement_label: str = "next-revision requirements", + include_disposition_json: bool = False, surfaced_requirement_instruction: str = ( "Each bullet must explain how you addressed that item or why it could not be satisfied safely." ), @@ -442,8 +443,11 @@ def _coder_human_requirements_guidance( HUMAN_REQUIREMENTS_ADDRESSED_MARKER, "", "Then add a `### Human requirements` section.", - "In the structured JSON, also include `human_requirement_dispositions`: one object for every surfaced `Requirement N`, with `requirement_id`, `disposition` (`addressed`, `blocked`, or `not-applicable`), and a concise non-empty `evidence` note.", ] + if include_disposition_json: + lines.append( + "In the structured JSON, also include `human_requirement_dispositions`: one object for every surfaced `Requirement N`, with `requirement_id`, `disposition` (`addressed`, `blocked`, or `not-applicable`), and a concise non-empty `evidence` note." + ) if context.surfaced_requirement_ids: surfaced = ", ".join(f"`{item}`" for item in context.surfaced_requirement_ids) lines.append( @@ -463,26 +467,28 @@ def _human_requirements_review_guidance( human_requirements: Sequence[HumanReviewRequirement] | None, *, requirement_label: str = "signed human reviewer requirements", + require_plan_dispositions: bool = False, ) -> str: if not human_requirements: return "" return f"""{requirement_label.capitalize()} override AI reviewer preferences unless they are unsafe, impossible, or contradicted by a later signed human instruction. -Verify each requirement in this set before approving. If all surfaced signed -human requirements have complete structured dispositions and the addressed -evidence is concretely covered by the current canonical plan, an approved -review must include exactly: +Verify each requirement in this set before approving. An approved review must +include exactly: If any signed human requirement in this set is unresolved, return blocking. +""" + ("""For every surfaced requirement, the JSON must include exactly one +`human_requirement_dispositions` object with its `Requirement N` ID, an +`addressed`, `blocked`, or `not-applicable` disposition, and non-empty evidence. For an `addressed` disposition, compare the evidence to the canonical plan and return blocking when it lacks concrete coverage. A named external integration is distinct from a similarly purposed local UI: a Grafana request requires the Grafana dashboard/provisioning or another named integration artifact, not merely an `admin.html` view. Use `blocked` or `not-applicable` only with a visible reason that you explicitly accept before approving. -""" +""" if require_plan_dispositions else "") def _structured_coder_followup_guidance( @@ -1005,6 +1011,7 @@ def build_issue_plan_prompt( {human_requirements_context.block}{_coder_human_requirements_guidance( human_requirements_context, requirement_label="planning requirements", + include_disposition_json=True, surfaced_requirement_instruction=( "Each bullet must explain how the plan covers that item or what remains risky or blocked." ), @@ -1068,6 +1075,7 @@ def build_plan_review_prompt( human_requirements_guidance = _human_requirements_review_guidance( issue_context.human_requirements if issue_context is not None else (), requirement_label="signed human issue requirements", + require_plan_dispositions=True, ) if unresolved_items: unresolved_items_guidance = """Prior unresolved plan items are present. Disposition every listed item @@ -1190,6 +1198,7 @@ def _build_compact_plan_review_prompt( human_requirements_guidance = _human_requirements_review_guidance( issue_context.human_requirements if issue_context is not None else (), requirement_label="signed human issue requirements", + require_plan_dispositions=True, ) if unresolved_items: unresolved_items_guidance = """Prior unresolved plan items are present. Disposition every listed item @@ -1370,6 +1379,7 @@ def build_plan_revision_prompt( {human_requirements_context.block}{_coder_human_requirements_guidance( human_requirements_context, requirement_label="planning requirements", + include_disposition_json=True, surfaced_requirement_instruction=( "Each bullet must explain how the revised plan covers that item or what remains risky or blocked." ), @@ -1468,6 +1478,7 @@ def _build_compact_plan_revision_prompt( human_requirements_guidance=_coder_human_requirements_guidance( human_requirements_context, requirement_label="planning requirements", + include_disposition_json=True, surfaced_requirement_instruction=( "Each bullet must explain how the revised plan covers that item or what remains risky or blocked." ), diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index f38e5e6..825c1cb 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -1272,13 +1272,21 @@ def _reviewer_human_requirements_instruction( rendered_ids = "- (none)" state_marker = "AGENT_STATE" if expected_kind == "pr_review" else "AGENT_PLAN_STATE" blocking_field = "blocking_items" if expected_kind == "pr_review" else "blocking_plan_issues" + disposition_instruction = "" + if expected_kind == "plan_review": + disposition_instruction = ( + "The JSON must include one `human_requirement_dispositions` object per listed requirement, " + "with exact `Requirement N` ID, disposition `addressed`, `blocked`, or `not-applicable`, " + "and non-empty evidence.\n" + ) + target = "current canonical plan" if expected_kind == "plan_review" else "current PR" return ( "## Signed human requirements missing acknowledgement:\n" "This approved review is missing .\n" f"Surfaced signed human requirements:\n{rendered_ids}\n" - "For each listed requirement, confirm whether the current plan/PR satisfies it.\n" - "The JSON must include one `human_requirement_dispositions` object per listed requirement, with exact `Requirement N` ID, disposition `addressed`, `blocked`, or `not-applicable`, and non-empty evidence.\n" - "If ALL requirements are satisfied: keep state `approved` and add " + f"For each listed requirement, confirm whether the {target} satisfies it.\n" + + disposition_instruction + + "If ALL requirements are satisfied: keep state `approved` and add " f"`` after the JSON and before the `` footer.\n" "If ANY requirement is NOT satisfied: change state to `blocking` and add a concrete " f"`{blocking_field}` entry naming the unresolved requirement. " diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 9b89578..04651eb 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -1258,6 +1258,41 @@ def test_followup_prompt_with_no_human_requirements_guides_empty_addressed_ids(t assert "issue acceptance criteria" in prompt assert "reviewer item IDs" in prompt +def test_non_plan_prompts_do_not_request_plan_disposition_json(tmp_path): + config = make_config(tmp_path, reviewer=("codex",)) + requirements = ( + HumanReviewRequirement( + source_type="Issue comment", + author="maintainer", + created_at="2026-05-17T11:00:00Z", + url="https://github.com/OWNER/REPO/issues/56#issuecomment-1", + body="Preserve backward compatibility.", + ), + ) + issue_context = IssueContext( + number=56, + repo="OWNER/REPO", + title="Support issue comments", + body="Original request.", + url="https://github.com/OWNER/REPO/issues/56", + comments=(), + human_requirements=requirements, + ) + prompts = [ + build_followup_prompt(77, 1, "Needs tests.", config, human_requirements=requirements), + build_same_pr_followup_prompt(77, 1, "Needs tests.", config, human_requirements=requirements), + build_review_prompt(77, 1, config, reviewer="codex", human_requirements=requirements), + build_review_prompt( + 77, 1, config, reviewer="codex", human_requirements=requirements, compact_context=True + ), + build_issue_prompt(56, config, issue_context=issue_context), + build_issue_implementation_prompt(56, "Approved plan.", config, issue_context=issue_context), + ] + + for prompt in prompts: + assert '"human_requirement_dispositions"' not in prompt + + def test_plan_review_prompt_surfaces_signed_issue_requirements_as_approval_critical(tmp_path): config = make_config(tmp_path, reviewer=("codex", "gemini")) issue_context = IssueContext( diff --git a/tests/test_repair.py b/tests/test_repair.py index 816c258..b11a112 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -1516,6 +1516,8 @@ def test_reviewer_human_requirements_instruction_pr_review(): assert "Requirement 2" in result assert "AGENT_STATE" in result assert "blocking_items" in result + assert "human_requirement_dispositions" not in result + assert "canonical plan" not in result def test_reviewer_human_requirements_instruction_plan_review(): result = _reviewer_human_requirements_instruction("plan_review", ["Requirement 1"]) @@ -1523,6 +1525,8 @@ def test_reviewer_human_requirements_instruction_plan_review(): assert "Requirement 1" in result assert "AGENT_PLAN_STATE" in result assert "blocking_plan_issues" in result + assert "human_requirement_dispositions" in result + assert "canonical plan" in result def test_reviewer_human_requirements_instruction_empty_ids(): result = _reviewer_human_requirements_instruction("pr_review", [])