From ee5321fde1e2c72461459dfe0e18618e9c3ece5b Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 15:35:59 +0200 Subject: [PATCH] [F047] conventions: reviewer-aware block-finding remediation on pr_pass gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pr_pass (reviewer) conventions guard reused the dev-path block-finding remediation: 'add a waiver to .roboco/conventions.yml in your branch'. A pr_reviewer does not own the assembled cell->root / root->master branch and has no commit verb on it, so the waiver remediation is unreachable — a false positive stranded the gate with no self-recovery (the reviewer could neither commit a waiver nor pr_pass). The fail-open content path is documented precision-over-recall and stays as-is; the actionable gap is the remediation. Fix: _conventions_rejection now branches the block-finding remediation on reviewer=True (mirroring the could_not_run branch from F044). The reviewer path points at pr_fail carrying the findings as issues so the PR returns to needs_revision and the DEV fixes the violation or commits the waiver (the dev CAN commit to the branch); waiver authorship is framed as the dev's action, not the reviewer's. Dev i_am_done path wording unchanged. TDD test_conventions_gate_pr_pass.py (+1). --- .../services/gateway/choreographer/_impl.py | 33 +++++++++++++++---- .../gateway/test_conventions_gate_pr_pass.py | 26 +++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 679b9ac6..61bcb96d 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -2015,17 +2015,38 @@ class Choreographer: listing = "\n".join( f"- {f.get('file')}:{f.get('line')} — {f.get('fix_hint')}" for f in blocks ) + if reviewer: + # F047: the pr_pass gate runs this on the REVIEWER, who does not own + # the assembled cell→root / root→master branch and has no commit + # verb on it. The dev-path remediation ("add a waiver in your + # branch") is unreachable by the reviewer and would strand the gate + # on every false positive with no self-recovery. The reviewer's only + # lever is pr_fail — bounce the PR back to needs_revision carrying + # the findings as issues so the dev fixes the violation or commits + # the waiver (the dev CAN commit to the branch). Waiver authorship is + # framed as the dev's action, not the reviewer's. + remediate = ( + "the assembled PR carries block-level architectural-convention" + " violations. call pr_fail(issues=[, ...])" + " with the findings below so the PR returns to needs_revision and" + " the dev places each definition in the module the architecture" + " map assigns it, or — if a finding is a false positive — commits" + " a waiver to .roboco/conventions.yml in the PR branch for review" + " and re-submits:\n\n" + listing + ) + else: + remediate = ( + "place each definition in the module the architecture map assigns " + "it, then commit and call the verb again. if a finding is a false " + "positive, add a waiver to .roboco/conventions.yml in your branch " + "for the PR to review:\n\n" + listing + ) return Envelope.invalid_state( message=( f"{len(blocks)} architectural-convention violation(s) must be " "fixed before this can proceed" ), - remediate=( - "place each definition in the module the architecture map assigns " - "it, then commit and call the verb again. if a finding is a false " - "positive, add a waiver to .roboco/conventions.yml in your branch " - "for the PR to review:\n\n" + listing - ), + remediate=remediate, context_briefing=briefing, ) diff --git a/tests/unit/gateway/test_conventions_gate_pr_pass.py b/tests/unit/gateway/test_conventions_gate_pr_pass.py index f505c450..fc4d195f 100644 --- a/tests/unit/gateway/test_conventions_gate_pr_pass.py +++ b/tests/unit/gateway/test_conventions_gate_pr_pass.py @@ -94,6 +94,32 @@ async def test_pr_pass_guard_could_not_run_remediation_uses_pr_fail( assert "pr_fail" in body["remediate"] +@pytest.mark.asyncio +async def test_pr_pass_guard_block_remediation_uses_pr_fail_not_reviewer_waiver( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # F047: on the pr_pass (reviewer) path a block-level finding's remediation + # must point at pr_fail (the reviewer's only lever) and frame the waiver as + # the DEV's action — NOT tell the reviewer to "add a waiver to + # .roboco/conventions.yml in your branch". A pr_reviewer does not own the + # assembled cell→root / root→master branch and has no commit verb on it, so + # the shared dev-path waiver remediation is unreachable and would strand the + # gate on every false positive (no self-recovery). + monkeypatch.setattr(settings, "conventions_enabled", True) + c = _make_choreographer(check_result=_BLOCK_RESULT) + env = await c._conventions_guard(uuid4(), MagicMock(), {}) + assert env is not None + body = env.as_dict() + remediate = body["remediate"] + # The reviewer's lever is pr_fail, not committing a waiver themselves. + assert "pr_fail" in remediate + # The offending finding is carried so the reviewer can paste it as an issue. + assert "app/routers/u.py:2" in remediate + # The reviewer must NOT be told to add a waiver "in your branch" — they + # can't commit to the assembled PR branch. The waiver is the dev's job. + assert "in your branch" not in remediate + + @pytest.mark.asyncio async def test_pr_pass_guard_inert_when_flag_off( monkeypatch: pytest.MonkeyPatch,