diff --git a/plugins/magpie-pr-management/skills/code-review/classifications/disposition-fixup.md b/plugins/magpie-pr-management/skills/code-review/classifications/disposition-fixup.md new file mode 100644 index 000000000..049369102 --- /dev/null +++ b/plugins/magpie-pr-management/skills/code-review/classifications/disposition-fixup.md @@ -0,0 +1,23 @@ + + +# Action `FIXUP` + +[`code-review`](../../../../../tools/pr-management/README.md#code-review--pr-management-code-review) `disposition` offers it, as the default, when the only thing between a `COMMENT` and an `APPROVE` is a set of `minor` / `nit` findings that each carry a `fixup`, and `--head-pushable` says the maintainer can push to the PR head. +Every approval gate still holds: green real CI, no unresolved thread, no standing `CHANGES_REQUESTED`, no unanswered question, a branch that merges cleanly. +A `major` or `blocking` finding is never fixed up — it goes back to the author. + +Instead of asking the contributor for another round over small, mechanical points, the maintainer applies them and approves. + +1. **Check out the PR head** on a local branch. +2. **Apply exactly the `fixup.changes`**, nothing else. No drive-by edits, no behaviour beyond what each finding names. +3. **Run the checks** for what was touched: the affected tool's tests and `prek run --files `. + If a fixup turns out bigger than its one-line description, or a check fails, abandon it and fall back to `COMMENT` with the findings. +4. **Show the diff and the commit message; confirm.** The message lists each change; add the project's attribution trailer per [`AGENTS.md`](../../../../../AGENTS.md#commit-and-pr-conventions). +5. **Push to the PR head on confirmation**: the head repository and branch come from `gh pr view --json headRepository,headRepositoryOwner,headRefName`. Pushing to a contributor's branch is visible to everyone watching the PR. +6. **Wait for CI on the new head** before approving (Golden rule 8), then re-run `guard` with the pushed SHA. +7. **Post an `APPROVE`** whose body says what the fixup changed: one bullet per change, with its file and the fixup commit SHA, so the contributor sees exactly what was done to their branch and why. + No inline comments — the fixup resolved them. + +Merging stays a separate, explicitly confirmed action. +Never push without the maintainer's yes in step 5, and never approve on the strength of a fixup whose CI has not finished. diff --git a/plugins/magpie-pr-management/skills/code-review/review-flow.md b/plugins/magpie-pr-management/skills/code-review/review-flow.md index bff0292e7..836dbd42d 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/plugins/magpie-pr-management/skills/code-review/review-flow.md @@ -161,6 +161,11 @@ For each finding, record: short, concrete fix. If short enough, also include a GitHub `suggestion` block in the eventual review body (see posting.md). + fixup: | + minor / nit only, and only when the fix is small and + mechanical: one line naming the exact change a maintainer + could commit to the PR head (see Step 6 and + classifications/disposition-fixup.md). Omit otherwise. ``` Before recording a correctness finding, verify the claimed @@ -277,13 +282,20 @@ Write the findings list to a file (Step 4 shape) and run: ```bash uv run --project /tools/pr-management pr-management code-review disposition --saved-dir /saved --pr --viewer --findings \ - [--unanswered-question] [--ci-diff-caused] + [--unanswered-question] [--ci-diff-caused] [--head-pushable] ``` -Pass `--unanswered-question` when an author question to the maintainers is open, `--ci-diff-caused` when you judged a CI failure is this diff's. The result's `docs` names the disposition and footer files to read. **Propose**: +Pass `--unanswered-question` when an author question to the maintainers is open, `--ci-diff-caused` when you judged a CI failure is this diff's, and `--head-pushable` when `gh pr view --json maintainerCanModify,isCrossRepository` shows the maintainer can push to the head (`maintainerCanModify`, or a branch in the base repository). +Give each `minor` / `nit` finding whose fix is small and mechanical a `fixup` field: one line naming the exact change. +The result's `docs` names the disposition and footer files to read. **Propose**: > *Suggested disposition: `COMMENT` — . Override? `[A]pprove`, `[R]equest changes`, `[C]omment` (default), `[E]dit findings first`, `[S]kip-for-now`, `[Q]uit`.* +When the result's `fixup.eligible` is true, `default` is `FIXUP` and the prompt leads with it, listing `fixup.changes`: + +> *Suggested: `[F]ixup` (default) — apply these small changes on the contributor's branch, then approve: . Or `[C]omment` with them as findings, `[A]pprove` as-is, `[R]equest changes`, `[E]dit findings first`, `[S]kip-for-now`, `[Q]uit`.* + +`[F]` follows [`classifications/disposition-fixup.md`](classifications/disposition-fixup.md); Steps 7a–7b then compose the `APPROVE` that explains the fixup. `[E]dit` lets the maintainer drop or re-classify findings; re-run `disposition` after. A `conflict_note` is always stated in the body, whatever the disposition. --- diff --git a/plugins/magpie-pr-management/skills/code-review/scope.md b/plugins/magpie-pr-management/skills/code-review/scope.md index a73e3321d..4121d7602 100644 --- a/plugins/magpie-pr-management/skills/code-review/scope.md +++ b/plugins/magpie-pr-management/skills/code-review/scope.md @@ -41,7 +41,11 @@ appeared), the skill says so explicitly and points them at proposes patches via `gh pr review --suggested-changes` beyond the verbatim suggestion blocks in [`posting.md`](posting.md), and never edits the contributor's - branch. + branch — with one exception: the `[F]ixup` action in + [`classifications/disposition-fixup.md`](classifications/disposition-fixup.md), + offered only when every remaining finding is a small, + mechanical `minor` / `nit`, and pushed only on the + maintainer's explicit confirmation. - **Bypassing the project's review criteria.** Findings cite specific rules from the source files declared in `/pr-management-code-review-criteria.md` and diff --git a/tools/pr-management/README.md b/tools/pr-management/README.md index d2847ec98..f86dffb40 100644 --- a/tools/pr-management/README.md +++ b/tools/pr-management/README.md @@ -226,7 +226,7 @@ uv run --project /tools/pr-management pr-management code-review dict[str, Any] mergeable=pr.mergeable, merge_state=pr.merge_state, permission=permission, + head_pushable=args.head_pushable, ) result["docs"] = [ f"classifications/disposition-{result['disposition'].lower().replace('_', '-')}.md", f"classifications/footer-{result['footer']}.md", ] + if result["fixup"]["eligible"]: + result["docs"].append("classifications/disposition-fixup.md") return result @@ -602,6 +605,11 @@ def add_parsers(sub: Any) -> None: dis.add_argument("--permission", default=None) dis.add_argument("--unanswered-question", action="store_true") dis.add_argument("--ci-diff-caused", action="store_true") + dis.add_argument( + "--head-pushable", + action="store_true", + help="the viewer can push to the PR head (maintainerCanModify, or a branch in the base repository)", + ) rev = commands.add_parser("reviewers", help="Step 4.5: grounded reviewer suggestions") rev.add_argument("--saved-dir", type=Path, required=True) diff --git a/tools/pr-management/src/pr_management/code_review/disposition.py b/tools/pr-management/src/pr_management/code_review/disposition.py index 328c59572..4d259d5aa 100644 --- a/tools/pr-management/src/pr_management/code_review/disposition.py +++ b/tools/pr-management/src/pr_management/code_review/disposition.py @@ -27,6 +27,13 @@ no finding above `nit`. * `COMMENT` — everything else. +When the only thing between a `COMMENT` and an `APPROVE` is a set of `minor` / +`nit` findings that each carry a `fixup` (a one-line, mechanical change the +maintainer can make), and the maintainer can push to the PR head, the result +also offers `FIXUP`: apply those changes as a commit on the contributor's +branch, then approve. It becomes the default action, not the disposition — +what is posted after a fixup is still an `APPROVE`. + Conflicts never force `REQUEST_CHANGES`, but a `DIRTY` / `CONFLICTING` / still-`UNKNOWN` branch is always named in the body. """ @@ -65,6 +72,7 @@ def pick( mergeable: str | None = None, merge_state: str | None = None, permission: str | None = None, + head_pushable: bool = False, ) -> dict[str, Any]: counts = Counter(str(f.get("severity") or "").lower() for f in findings) unknown = [s for s in counts if s not in SEVERITIES] @@ -108,6 +116,8 @@ def pick( conflict = "The branch currently conflicts with its base and needs a rebase before this can merge." elif mergeable == "UNKNOWN": conflict = "GitHub has not computed whether this branch merges cleanly yet." + fixup = _fixup(findings, disposition, gates, conflict, head_pushable) + overrides = ["APPROVE", "REQUEST_CHANGES", "COMMENT"] return { "disposition": disposition, "reason": reason, @@ -115,7 +125,37 @@ def pick( "approval_gates": gates, "footer": footer_variant(disposition, permission), "conflict_note": conflict, - "overrides": ["APPROVE", "REQUEST_CHANGES", "COMMENT"], + "fixup": fixup, + "default": "FIXUP" if fixup["eligible"] else disposition, + "overrides": ["FIXUP", *overrides] if fixup["eligible"] else overrides, + } + + +def _fixup( + findings: list[dict[str, Any]], + disposition: str, + gates: list[str], + conflict: str | None, + head_pushable: bool, +) -> dict[str, Any]: + """Whether applying the remaining findings as a commit would earn an `APPROVE`.""" + if disposition != "COMMENT" or not findings: + return {"eligible": False, "reason": f"nothing to fix up on a {disposition}"} + if any(str(f.get("severity") or "").lower() in ("major", "blocking") for f in findings): + return {"eligible": False, "reason": "a major finding needs the author, not a fixup"} + if gates: + return {"eligible": False, "reason": "an approval gate other than the findings is open"} + if conflict: + return {"eligible": False, "reason": "the branch does not merge cleanly"} + if not head_pushable: + return {"eligible": False, "reason": "the maintainer cannot push to the PR head"} + missing = [f for f in findings if not str(f.get("fixup") or "").strip()] + if missing: + return {"eligible": False, "reason": f"{len(missing)} finding(s) have no mechanical fixup"} + return { + "eligible": True, + "reason": "every remaining finding is minor or nit with a mechanical fixup", + "changes": [str(f["fixup"]).strip() for f in findings], } diff --git a/tools/pr-management/tests/code_review/test_decisions.py b/tools/pr-management/tests/code_review/test_decisions.py index 795db1674..7aa4590cc 100644 --- a/tools/pr-management/tests/code_review/test_decisions.py +++ b/tools/pr-management/tests/code_review/test_decisions.py @@ -82,6 +82,65 @@ def test_unknown_severity_is_refused() -> None: _pick(["high"]) +# --- fixup ------------------------------------------------------------------------------------- + + +def _fixable(*severities: str) -> list[dict[str, Any]]: + return [{"severity": s, "fixup": f"fix {i}"} for i, s in enumerate(severities)] + + +def _pick_findings(findings: list[dict[str, Any]], **kw: Any) -> dict[str, Any]: + kw.setdefault("ci_state", "SUCCESS") + kw.setdefault("real_ci", True) + kw.setdefault("unresolved_threads", 0) + kw.setdefault("other_changes_requested", []) + kw.setdefault("unanswered_question", False) + return disposition.pick(findings, **kw) + + +def test_small_mechanical_findings_default_to_fixup() -> None: + result = _pick_findings(_fixable("minor", "nit", "nit"), head_pushable=True) + assert result["disposition"] == "COMMENT" + assert result["default"] == "FIXUP" and result["overrides"][0] == "FIXUP" + assert result["fixup"]["changes"] == ["fix 0", "fix 1", "fix 2"] + + +def test_fixup_needs_push_access_to_the_head() -> None: + result = _pick_findings(_fixable("minor")) + assert result["default"] == "COMMENT" and "FIXUP" not in result["overrides"] + assert "push" in result["fixup"]["reason"] + + +def test_fixup_needs_every_finding_to_be_mechanical() -> None: + findings = [*_fixable("minor"), {"severity": "minor"}] + assert not _pick_findings(findings, head_pushable=True)["fixup"]["eligible"] + + +def test_a_major_is_never_fixed_up() -> None: + result = _pick_findings(_fixable("major", "minor"), head_pushable=True) + assert result["disposition"] == "COMMENT" and not result["fixup"]["eligible"] + + +@pytest.mark.parametrize( + "gate", + [ + {"ci_state": "PENDING"}, + {"real_ci": False}, + {"unresolved_threads": 1}, + {"other_changes_requested": ["alice"]}, + {"unanswered_question": True}, + {"merge_state": "DIRTY"}, + ], +) +def test_fixup_never_skips_an_approval_gate(gate: dict[str, Any]) -> None: + assert not _pick_findings(_fixable("minor"), head_pushable=True, **gate)["fixup"]["eligible"] + + +def test_an_approve_needs_no_fixup() -> None: + result = _pick_findings(_fixable("nit"), head_pushable=True) + assert result["disposition"] == "APPROVE" and result["default"] == "APPROVE" + + # --- review-disposition --------------------------------------------------------------------------- diff --git a/tools/spec-loop/specs/pr-management-family.md b/tools/spec-loop/specs/pr-management-family.md index a56d380a3..c5da47914 100644 --- a/tools/spec-loop/specs/pr-management-family.md +++ b/tools/spec-loop/specs/pr-management-family.md @@ -313,6 +313,12 @@ same directory; the behaviour is unchanged. `CHANGES_REQUESTED` stands, and no maintainer question is unanswered; the maintainer may downgrade it, and upgrading past a named gate needs a stated reason (#1585). + When every gate holds and the only findings left are `minor` / `nit` ones + with a mechanical `fixup`, and the maintainer can push to the PR head, + `FIXUP` is the default action: the maintainer commits those changes to the + contributor's branch on confirmation, waits for CI, and approves with a body + that lists what the fixup changed. A `major` is never fixed up, and merging + stays a separate confirmed action. The "touching mine" selector uses GitHub data rather than `git log --author`, CODEOWNERS team ownership checks team membership rather than organisation membership, and `maintain` permission gets the maintainer