feat(code-review): default to a maintainer fixup for small, mechanical findings

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.

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.
 
 ---

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

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 |

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)

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,14 +116,46 @@ 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,
         "counts": {"blocking": blocking, "major": major, "minor": minor, "nit": nit},
         "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],
     }
 
 

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 ---------------------------------------------------------------------------
 
 

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