You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/10/20 22:08:27 UTC
(magpie) branch main updated: feat(code-review): default to a maintainer fixup for small, mechanical findings (#1743)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie.git
The following commit(s) were added to refs/heads/main by this push:
new e6e5c2e3 feat(code-review): default to a maintainer fixup for small, mechanical findings (#1743)
e6e5c2e3 is described below
commit e6e5c2e36bcab4d0a09d6da5c26e8060caaea3e2
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Wed Oct 21 00:08:22 2026 +0200
feat(code-review): default to a maintainer fixup for small, mechanical findings (#1743)
When every approval gate holds and the only findings left are minor or
nit ones with a mechanical fix, sending the PR back for another round
costs the contributor a cycle for changes the maintainer could make in a
minute. The disposition step now offers FIXUP as the default in that
case: commit exactly those changes to the PR head on the maintainer's
confirmation, wait for CI, and approve with a body that lists what the
fixup changed.
- `disposition` takes `--head-pushable` and a per-finding `fixup` line,
and returns `fixup`, `default` and a `FIXUP` override when eligible.
- A major or blocking finding is never fixed up; no gate is skipped
(CI, threads, standing change requests, open questions, conflicts).
- New classifications/disposition-fixup.md describes the flow; Step 6
of review-flow.md offers `[F]ixup`, and scope.md carves out this one
confirmed path from "never edits the contributor's branch".
- Merging stays a separate, confirmed action.
Generated-by: Claude Opus 5
---
.../classifications/disposition-fixup.md | 23 +++++++++
.../skills/code-review/review-flow.md | 16 +++++-
.../skills/code-review/scope.md | 6 ++-
tools/pr-management/README.md | 2 +-
.../src/pr_management/code_review/cli.py | 8 +++
.../src/pr_management/code_review/disposition.py | 42 ++++++++++++++-
.../tests/code_review/test_decisions.py | 59 ++++++++++++++++++++++
tools/spec-loop/specs/pr-management-family.md | 6 +++
8 files changed, 157 insertions(+), 5 deletions(-)
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 00000000..04936910
--- /dev/null
+++ b/plugins/magpie-pr-management/skills/code-review/classifications/disposition-fixup.md
@@ -0,0 +1,23 @@
+<!-- SPDX-License-Identifier: Apache-2.0
+ https://www.apache.org/licenses/LICENSE-2.0 -->
+
+# 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 <touched 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 <N> --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 bff0292e..836dbd42 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 <framework>/tools/pr-management pr-management code-review disposition --saved-dir <workspace>/saved --pr <N> --viewer <viewer> --findings <file> \
- [--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 <N> --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` — <reason>. 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 <n> small changes on the contributor's branch, then approve: <changes>. 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 a73e3321..4121d760 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
`<project-config>/pr-management-code-review-criteria.md` and
diff --git a/tools/pr-management/README.md b/tools/pr-management/README.md
index d2847ec9..f86dffb4 100644
--- a/tools/pr-management/README.md
+++ b/tools/pr-management/README.md
@@ -226,7 +226,7 @@ uv run --project <framework>/tools/pr-management pr-management code-review <subc
| `context` | the PR headline, the slop signals, the security and AI-disclosure scans, the `AGENTS.md` files that apply, the real-CI result |
| `slop-outcome` | the slop outcome once the agent has judged H1, H5 and S2 |
| `deps` | the dependency-constraint ledger: `broken`, `compatible` or `unknown` |
-| `disposition` | `APPROVE` / `REQUEST_CHANGES` / `COMMENT`, the footer variant and any conflict note |
+| `disposition` | `APPROVE` / `REQUEST_CHANGES` / `COMMENT`, the footer variant, any conflict note, and whether a `FIXUP` (commit the remaining small, mechanical findings to the PR head, then approve) is the default; `--head-pushable` says the maintainer can push there |
| `reviewers` | ranked reviewer suggestions |
| `render` | the review body file, the payload file, inline anchors (line, side, position) and the `shlex.join`-built post command |
| `pick` | the parsed picker choice |
diff --git a/tools/pr-management/src/pr_management/code_review/cli.py b/tools/pr-management/src/pr_management/code_review/cli.py
index 82ed3a03..7df14007 100644
--- a/tools/pr-management/src/pr_management/code_review/cli.py
+++ b/tools/pr-management/src/pr_management/code_review/cli.py
@@ -341,11 +341,14 @@ def _disposition(args: argparse.Namespace, cfg: config.Config) -> 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 328c5957..4d259d5a 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 @@ Rules (one definition; the three the docs carried disagreed):
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 795db167..7aa4590c 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 a56d380a..c5da4791 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