mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F047] conventions: reviewer-aware block-finding remediation on pr_pass gate
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).
This commit is contained in:
@@ -2015,17 +2015,38 @@ class Choreographer:
|
|||||||
listing = "\n".join(
|
listing = "\n".join(
|
||||||
f"- {f.get('file')}:{f.get('line')} — {f.get('fix_hint')}" for f in blocks
|
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=[<file:line — fix_hint>, ...])"
|
||||||
|
" 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(
|
return Envelope.invalid_state(
|
||||||
message=(
|
message=(
|
||||||
f"{len(blocks)} architectural-convention violation(s) must be "
|
f"{len(blocks)} architectural-convention violation(s) must be "
|
||||||
"fixed before this can proceed"
|
"fixed before this can proceed"
|
||||||
),
|
),
|
||||||
remediate=(
|
remediate=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
|
|
||||||
),
|
|
||||||
context_briefing=briefing,
|
context_briefing=briefing,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -94,6 +94,32 @@ async def test_pr_pass_guard_could_not_run_remediation_uses_pr_fail(
|
|||||||
assert "pr_fail" in body["remediate"]
|
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
|
@pytest.mark.asyncio
|
||||||
async def test_pr_pass_guard_inert_when_flag_off(
|
async def test_pr_pass_guard_inert_when_flag_off(
|
||||||
monkeypatch: pytest.MonkeyPatch,
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
|||||||
Reference in New Issue
Block a user