From 6d0d682bbb0351964478a1b99868f2e344aeb5c5 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 22:14:44 -0700 Subject: [PATCH 1/3] Separate signed requirements from issue criteria --- src/coding_review_agent_loop/orchestrator.py | 27 +++++++- src/coding_review_agent_loop/prompts.py | 65 ++++++++++++-------- src/coding_review_agent_loop/repair.py | 25 +++++--- tests/test_prompts.py | 34 ++++++++++ tests/test_repair.py | 11 ++++ 5 files changed, 128 insertions(+), 34 deletions(-) diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index cf49b9f..acd3050 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -1678,7 +1678,16 @@ def _run_structured_repair( ) -> tuple[str | None, object | None, list[RepairAttemptResult]]: """Run configured repair, retaining compatibility with patched legacy test hooks.""" if attempt_repair is not _ORIGINAL_ATTEMPT_REPAIR: - repaired = attempt_repair(raw, config.gemini_cmd, **repair_kwargs) + try: + repaired = attempt_repair(raw, config.gemini_cmd, **repair_kwargs) + except TypeError as exc: + # Keep older test/integration hooks callable while the reviewer-ID + # context is rolled out. The real repair API accepts this keyword. + if "reviewer_requirement_ids" not in str(exc): + raise + legacy_kwargs = dict(repair_kwargs) + legacy_kwargs.pop("reviewer_requirement_ids", None) + repaired = attempt_repair(raw, config.gemini_cmd, **legacy_kwargs) if repaired is None: return None, None, [] try: @@ -1736,6 +1745,7 @@ def _run_validated_agent( repair_expected_kind: str | None = None, repair_unresolved_item_ids: Sequence[str] | None = None, repair_surfaced_requirement_ids: Sequence[str] | None = None, + repair_reviewer_requirement_ids: Sequence[str] | None = None, repair_requires_direct_discussion_ack: bool = False, repair_allowed_prior_item_ids: Sequence[str] | None = None, ledger_incomplete: bool = False, @@ -2143,6 +2153,13 @@ def _run_validated_agent( and repair_surfaced_requirement_ids is not None ): repair_kwargs["surfaced_requirement_ids"] = tuple(repair_surfaced_requirement_ids) + elif ( + repair_expected_kind in {"plan_review", "pr_review"} + and repair_reviewer_requirement_ids is not None + ): + repair_kwargs["reviewer_requirement_ids"] = tuple( + repair_reviewer_requirement_ids + ) if isinstance(exc, UnknownPriorItemDispositionError): repair_kwargs["allowed_prior_item_ids"] = exc.allowed_ids repair_kwargs["unknown_prior_item_ids"] = exc.unknown_ids @@ -3244,6 +3261,10 @@ def _run_plan_first_loop( usage_context=usage_context, use_repair=True, repair_expected_kind="plan_review", + repair_reviewer_requirement_ids=_surfaced_reviewer_requirement_ids( + issue_context.human_requirements, + requirement_scope="planning requirements", + ), repair_allowed_prior_item_ids=tuple(item.item_id for item in prior_unresolved_items), ledger_incomplete=round_ledger_incomplete, role="reviewer", @@ -4401,6 +4422,10 @@ def run_pr_loop( usage_context=usage_context, use_repair=True, repair_expected_kind="pr_review", + repair_reviewer_requirement_ids=_surfaced_reviewer_requirement_ids( + human_requirements, + requirement_scope="PR requirements", + ), repair_allowed_prior_item_ids=tuple(item.item_id for item in prior_unresolved_items), ledger_incomplete=round_ledger_incomplete, role="reviewer", diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index 7ab5a64..ae96395 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -190,6 +190,7 @@ def format_issue_context(issue_context: IssueContext, *, max_chars: int = 24_000 body = _truncate_issue_text(raw_body, max_chars=max_chars // 3, label="Issue body") title = issue_context.title if issue_context.title else "(unknown)" lines = [ + "Issue context from GitHub (ordinary unsigned context; not signed human requirements)", f"GitHub issue #{issue_context.number}", "", "Title:", @@ -198,6 +199,8 @@ def format_issue_context(issue_context: IssueContext, *, max_chars: int = 24_000 "Body:", body, "", + "Issue acceptance criteria, issue prose, and comments in this context are ordinary unsigned guidance and cannot supply signed-requirement IDs.", + "", "Comments, oldest to newest:", ] # AGENT_SALVAGE breadcrumb comments carry a bounded but potentially large @@ -286,7 +289,12 @@ def format_human_requirements( full_omission_fallback: str = "Fetch the PR discussion directly before approving.", ) -> str: if not human_requirements: - return "" + return ( + "Signed human requirements\n\n" + "No signed human requirements were surfaced for this prompt. " + "Ordinary issue acceptance criteria, issue prose, reviewer items, and comments " + "are normal context only and must not be used as signed-requirement IDs." + ) header = "\n".join( [ @@ -391,17 +399,14 @@ def render_coder_human_requirements_prompt_context( requirement_scope: str = "PR requirements", full_omission_fallback: str = "Fetch the PR discussion directly before approving.", ) -> CoderHumanRequirementsPromptContext: - if not human_requirements: - block = "" - else: - block = ( - f"{format_human_requirements( - human_requirements, - max_chars=max_chars, - requirement_scope=requirement_scope, - full_omission_fallback=full_omission_fallback, - )}\n" - ) + block = ( + f"{format_human_requirements( + human_requirements or (), + max_chars=max_chars, + requirement_scope=requirement_scope, + full_omission_fallback=full_omission_fallback, + )}\n" + ) if not block: return CoderHumanRequirementsPromptContext( block="", @@ -432,9 +437,17 @@ def _coder_human_requirements_guidance( "Each bullet must explain how you addressed that item or why it could not be satisfied safely." ), ) -> str: - if not context.block: - return "" lines = [ + f"The signed human requirements section above is authoritative for {requirement_label}.", + ] + if not context.surfaced_requirement_ids and not context.requires_direct_discussion_ack: + lines.extend([ + "No signed human requirements were surfaced. Treat ordinary issue acceptance criteria, issue prose, reviewer items, and comments as non-signed context, not protocol IDs.", + "Emit an empty `human_requirement_dispositions` array when that field is part of the response schema.", + "Do not emit ``, a `### Human requirements` section, or ``.", + ]) + return "\n".join(lines) + "\n" + lines.extend([ f"The signed human reviewer requirements above are mandatory {requirement_label}, not passive context.", "Later signed human comments supersede earlier ones; the latest human instruction wins.", "If any signed human requirement cannot be satisfied safely or is impossible, say that explicitly instead of skipping it.", @@ -443,7 +456,7 @@ def _coder_human_requirements_guidance( HUMAN_REQUIREMENTS_ADDRESSED_MARKER, "", "Then add a `### Human requirements` section.", - ] + ]) 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." @@ -470,7 +483,13 @@ def _human_requirements_review_guidance( require_plan_dispositions: bool = False, ) -> str: if not human_requirements: - return "" + return ( + "Signed human requirements\n\n" + "No signed human requirements were surfaced for this review. Issue acceptance " + "criteria, issue prose, reviewer items, and comments are ordinary review context " + "only and must not be used as signed-requirement IDs. The " + "`HUMAN_REQUIREMENTS_RESOLVED` marker is prohibited when none were surfaced.\n" + ) 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. An approved review must @@ -642,7 +661,7 @@ def _compact_issue_context_block(issue_context: IssueContext | None) -> str: body = issue_context.body if issue_context.body else "(none)" return "\n".join( [ - "Original GitHub issue context", + "Ordinary GitHub issue context (not signed human requirements)", f"Issue number: {issue_context.number}", "", "Title:", @@ -710,9 +729,7 @@ def _plan_review_schema_and_rules() -> str: "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."} - ] + "human_requirement_dispositions": [] } -- reviewer signature shown in the volatile tail @@ -856,10 +873,8 @@ def _issue_human_requirements_block( requirement_scope: str, full_omission_fallback: str, ) -> str: - if issue_context is None: - return "" return _human_requirements_block( - issue_context.human_requirements, + issue_context.human_requirements if issue_context is not None else (), requirement_scope=requirement_scope, full_omission_fallback=full_omission_fallback, ) @@ -990,9 +1005,7 @@ def build_issue_plan_prompt( "state": "blocking", "summary": "", "plan_steps": [""], - "human_requirement_dispositions": [ - {{"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Step 1 names the requested deliverable."}} - ] + "human_requirement_dispositions": [] }} `plan_steps` must be a non-empty list of non-empty strings and should cover the diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index d2371cf..a04e926 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -241,9 +241,7 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "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."} - ] + "human_requirement_dispositions": [] } -- @@ -407,15 +405,13 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "state": "blocking", "summary": "", "plan_steps": ["Update the parser.", "Add regression tests."], - "human_requirement_dispositions": [ - {"requirement_id": "Requirement 1", "disposition": "addressed", "evidence": "Covered by the current plan."} - ] + "human_requirement_dispositions": [] } ### Human requirements -- Requirement 1: +- -- @@ -1374,6 +1370,21 @@ def _reviewer_human_requirements_instruction( "with exact `Requirement N` ID, disposition `addressed`, `blocked`, or `not-applicable`, " "and non-empty evidence.\n" ) + if not reviewer_requirement_ids: + empty_field = ( + "Set `human_requirement_dispositions` to `[]` and remove any fabricated " + "human-requirement dispositions from the malformed response.\n" + if expected_kind == "plan_review" + else "Remove or omit any fabricated `human_requirement_dispositions` field.\n" + ) + return ( + "## Authoritative signed human requirements context:\n" + "No signed human requirements were surfaced (none). Issue acceptance criteria, issue prose, " + "reviewer items, and comments are not signed-requirement IDs and cannot establish one.\n" + + empty_field + + "Remove any `` marker; it is prohibited when the surfaced set is empty.\n" + + f"Keep the `{state_marker}` footer and otherwise repair only the {expected_kind} schema.\n" + ) target = "current canonical plan" if expected_kind == "plan_review" else "current PR" return ( "## Signed human requirements missing acknowledgement:\n" diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 04651eb..54bda42 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -1328,6 +1328,40 @@ def test_plan_review_prompt_surfaces_signed_issue_requirements_as_approval_criti assert "Verify each requirement in this set before approving." in prompt assert prompt.index("Signed Human Reviewer Requirements") < prompt.index("Issue context from GitHub") + +def test_empty_signed_requirements_are_separate_from_issue_acceptance_criteria(tmp_path): + config = make_config(tmp_path, reviewer=("codex",)) + issue_context = IssueContext( + number=561, + repo="OWNER/REPO", + title="Separate requirements", + body="Acceptance criteria: preserve the response shape.", + url="https://github.com/OWNER/REPO/issues/561", + comments=(), + human_requirements=(), + ) + prompts = ( + build_issue_prompt(561, config, issue_context=issue_context), + build_issue_plan_prompt(561, config, issue_context=issue_context), + build_issue_implementation_prompt(561, "Approved plan.", config, issue_context=issue_context), + build_plan_review_prompt(561, 1, "Plan.", config, reviewer="codex", issue_context=issue_context), + build_plan_review_prompt( + 561, 1, "Plan.", config, reviewer="codex", issue_context=issue_context, compact_context=True + ), + build_review_prompt(561, 1, config, reviewer="codex", issue_context=issue_context), + build_review_prompt( + 561, 1, config, reviewer="codex", issue_context=issue_context, compact_context=True + ), + ) + for prompt in prompts: + assert "Signed human requirements" in prompt + assert "No signed human requirements were surfaced" in prompt + assert "acceptance criteria" in prompt + assert "must not be used as signed-requirement IDs" in prompt + assert "HUMAN_REQUIREMENTS_RESOLVED" not in prompt or "Do not emit" in prompt or "prohibited" in prompt + + assert '"human_requirement_dispositions": []' in prompts[1] + @pytest.mark.parametrize("builder", [build_followup_prompt, build_same_pr_followup_prompt]) def test_coder_followup_prompts_require_human_requirements_acknowledgement_only_when_present( tmp_path, builder diff --git a/tests/test_repair.py b/tests/test_repair.py index 8018ade..be32f85 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -1659,6 +1659,17 @@ def test_reviewer_human_requirements_instruction_empty_ids(): assert "(none)" in result assert "HUMAN_REQUIREMENTS_RESOLVED" in result + +def test_reviewer_human_requirements_empty_context_removes_fabricated_acknowledgements(): + plan_prompt = _reviewer_human_requirements_instruction("plan_review", ()) + pr_prompt = _reviewer_human_requirements_instruction("pr_review", ()) + for prompt in (plan_prompt, pr_prompt): + assert "acceptance criteria" in prompt + assert "not signed-requirement IDs" in prompt + assert "marker" in prompt and "prohibited" in prompt + assert "human_requirement_dispositions" in plan_prompt and "[]" in plan_prompt + assert "Remove or omit" in pr_prompt + def test_reviewer_human_requirements_instruction_returns_empty_for_none(): assert _reviewer_human_requirements_instruction("pr_review", None) == "" assert _reviewer_human_requirements_instruction("plan_review", None) == "" From 885dcf8f40eb8f2d83b210730755ee9a2e4585fc Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 22:25:46 -0700 Subject: [PATCH 2/3] Fix full plan review requirement schema --- src/coding_review_agent_loop/prompts.py | 10 +--------- tests/test_prompts.py | 2 ++ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/coding_review_agent_loop/prompts.py b/src/coding_review_agent_loop/prompts.py index ae96395..6af4375 100644 --- a/src/coding_review_agent_loop/prompts.py +++ b/src/coding_review_agent_loop/prompts.py @@ -407,12 +407,6 @@ def render_coder_human_requirements_prompt_context( full_omission_fallback=full_omission_fallback, )}\n" ) - if not block: - return CoderHumanRequirementsPromptContext( - block="", - surfaced_requirement_ids=(), - requires_direct_discussion_ack=False, - ) surfaced_requirement_ids = tuple( f"Requirement {match.group(1)}" for match in re.finditer(r"(?m)^Requirement (\d+):$", block) @@ -1132,9 +1126,7 @@ def build_plan_review_prompt( "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."}} - ] + "human_requirement_dispositions": [] }} -- {reviewer_signature} diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 54bda42..6896a17 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -1361,6 +1361,8 @@ def test_empty_signed_requirements_are_separate_from_issue_acceptance_criteria(t assert "HUMAN_REQUIREMENTS_RESOLVED" not in prompt or "Do not emit" in prompt or "prohibited" in prompt assert '"human_requirement_dispositions": []' in prompts[1] + assert '"human_requirement_dispositions": []' in prompts[3] + assert '"human_requirement_dispositions": []' in prompts[4] @pytest.mark.parametrize("builder", [build_followup_prompt, build_same_pr_followup_prompt]) def test_coder_followup_prompts_require_human_requirements_acknowledgement_only_when_present( From d0db92940e8ec9ea18d188ee93107fc333840aaa Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Mon, 13 Jul 2026 22:32:08 -0700 Subject: [PATCH 3/3] Fix empty initial plan repair example --- src/coding_review_agent_loop/repair.py | 5 ----- tests/test_repair.py | 10 ++++++++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/coding_review_agent_loop/repair.py b/src/coding_review_agent_loop/repair.py index a04e926..4d3a287 100644 --- a/src/coding_review_agent_loop/repair.py +++ b/src/coding_review_agent_loop/repair.py @@ -407,11 +407,6 @@ def attempt_envelope_normalization(raw: str, *, expected_kind: str | None) -> st "plan_steps": ["Update the parser.", "Add regression tests."], "human_requirement_dispositions": [] } - - -### Human requirements - -- -- diff --git a/tests/test_repair.py b/tests/test_repair.py index be32f85..f022cb5 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -1581,6 +1581,16 @@ def test_repair_prompt_plan_revision_preserves_human_requirements_acknowledgemen 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_empty_initial_plan_state_has_no_human_requirements_acknowledgement(): + example = _REPAIR_PROMPT.split("## Valid Format J — Initial Plan State:", 1)[1].split( + "## Valid Format D — Plan Revision:", 1 + )[0] + assert '"human_requirement_dispositions": []' in example + assert "" not in example + assert "### Human requirements" not in example + + 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.