From f8ab31f42f6d7add2239d55526c5388993ed9609 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 10 Nov 2026 10:19:11 -0700 Subject: [PATCH] feat(pr-management): record FIXUP as a code-review session outcome `code-review session record --outcome` accepted only the three GitHub review events plus skip / slop / dry-run outcomes, so a PR that ended on the FIXUP disposition (maintainer fixup pushed to the branch, then an APPROVE that explains it) could not be recorded, and the session summary under-counted the pass. Add `FIXUP` to the outcomes, count it as a review in the summary line, and say in review-flow Step 9 and the session-summary doc which dispositions map to which outcome. Generated-by: Claude Opus 5 --- .../skills/code-review/review-flow.md | 2 + .../skills/code-review/session-summary.md | 2 +- .../src/pr_management/code_review/session.py | 19 ++++++++-- .../src/pr_management/code_review/session.py | 19 ++++++++-- .../tests/code_review/test_cli.py | 37 +++++++++++++++++++ 5 files changed, 72 insertions(+), 7 deletions(-) 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 b5f981a24..a8c1bd378 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/plugins/magpie-pr-management/skills/code-review/review-flow.md @@ -374,6 +374,8 @@ Record the outcome and move on: uv run --project /tools/pr-management pr-management code-review session record --session /cr-session.json --pr --outcome [--reason ] [--adversarial yes|no] ``` +`` is the review that was posted: `APPROVE`, `REQUEST_CHANGES` or `COMMENT`, or `FIXUP` when the maintainer pushed a fixup and then approved ([`classifications/disposition-fixup.md`](classifications/disposition-fixup.md)), with or without auto-merge. + To keep wall-clock time low on a long queue, fire background analysis subagents on the next PRs while the maintainer works on this one — below. --- diff --git a/plugins/magpie-pr-management/skills/code-review/session-summary.md b/plugins/magpie-pr-management/skills/code-review/session-summary.md index 9aced90ab..6d57132af 100644 --- a/plugins/magpie-pr-management/skills/code-review/session-summary.md +++ b/plugins/magpie-pr-management/skills/code-review/session-summary.md @@ -6,7 +6,7 @@ Each PR's outcome is recorded with `uv run --project /tools/pr-management pr-management code-review session record` as the loop goes (Step 9 of [`review-flow.md`](review-flow.md)). On exit, print the `text` of `uv run --project /tools/pr-management pr-management code-review session summary --session /cr-session.json --untouched `: -reviews per disposition, skips with the maintainer's reasons, PRs left +reviews per disposition (a `FIXUP` counts as a review), skips with the maintainer's reasons, PRs left untouched, which PRs had adversarial findings folded in, time, throughput and the GitHub calls spent. The summary is for the maintainer's records — this skill never writes a session log anywhere else. diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/session.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/session.py index 6089d5c32..973c0f94e 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/session.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/session.py @@ -35,7 +35,20 @@ #: A pass that crosses this many calls is mis-batching: stop and fix the call pattern. BUDGET_ALARM = 100 -OUTCOMES = ("APPROVE", "REQUEST_CHANGES", "COMMENT", "skipped", "slop-comment", "slop-close", "dry-run") +#: ``FIXUP`` is a maintainer fixup pushed to the PR branch followed by an +#: ``APPROVE`` that explains it (classifications/disposition-fixup.md); it counts +#: as a review, like the three GitHub review events. +OUTCOMES = ( + "APPROVE", + "REQUEST_CHANGES", + "COMMENT", + "FIXUP", + "skipped", + "slop-comment", + "slop-close", + "dry-run", +) +REVIEWED = ("APPROVE", "REQUEST_CHANGES", "COMMENT", "FIXUP") def _load(path: Path) -> dict[str, Any]: @@ -83,13 +96,13 @@ def summary(path: Path, *, untouched: int = 0, now: dt.datetime) -> dict[str, An counts = Counter(v["outcome"] for v in prs.values()) started = dt.datetime.fromisoformat(data["started_at"]) if data.get("started_at") else now minutes = max(1, int((now - started).total_seconds() // 60)) - reviewed = sum(counts[k] for k in ("APPROVE", "REQUEST_CHANGES", "COMMENT")) + reviewed = sum(counts[k] for k in REVIEWED) skipped = [{"pr": int(k), "reason": v.get("reason")} for k, v in prs.items() if v["outcome"] == "skipped"] covered = sorted(int(k) for k, v in prs.items() if v.get("adversarial")) uncovered = sorted(int(k) for k, v in prs.items() if v.get("adversarial") is False) lines = [ f"Reviewed: {reviewed} (APPROVE {counts['APPROVE']}, REQUEST_CHANGES {counts['REQUEST_CHANGES']}, " - f"COMMENT {counts['COMMENT']})", + f"COMMENT {counts['COMMENT']}, FIXUP {counts['FIXUP']})", f"Skipped: {len(skipped)}" + ( "" diff --git a/tools/pr-management/src/pr_management/code_review/session.py b/tools/pr-management/src/pr_management/code_review/session.py index 6089d5c32..973c0f94e 100644 --- a/tools/pr-management/src/pr_management/code_review/session.py +++ b/tools/pr-management/src/pr_management/code_review/session.py @@ -35,7 +35,20 @@ #: A pass that crosses this many calls is mis-batching: stop and fix the call pattern. BUDGET_ALARM = 100 -OUTCOMES = ("APPROVE", "REQUEST_CHANGES", "COMMENT", "skipped", "slop-comment", "slop-close", "dry-run") +#: ``FIXUP`` is a maintainer fixup pushed to the PR branch followed by an +#: ``APPROVE`` that explains it (classifications/disposition-fixup.md); it counts +#: as a review, like the three GitHub review events. +OUTCOMES = ( + "APPROVE", + "REQUEST_CHANGES", + "COMMENT", + "FIXUP", + "skipped", + "slop-comment", + "slop-close", + "dry-run", +) +REVIEWED = ("APPROVE", "REQUEST_CHANGES", "COMMENT", "FIXUP") def _load(path: Path) -> dict[str, Any]: @@ -83,13 +96,13 @@ def summary(path: Path, *, untouched: int = 0, now: dt.datetime) -> dict[str, An counts = Counter(v["outcome"] for v in prs.values()) started = dt.datetime.fromisoformat(data["started_at"]) if data.get("started_at") else now minutes = max(1, int((now - started).total_seconds() // 60)) - reviewed = sum(counts[k] for k in ("APPROVE", "REQUEST_CHANGES", "COMMENT")) + reviewed = sum(counts[k] for k in REVIEWED) skipped = [{"pr": int(k), "reason": v.get("reason")} for k, v in prs.items() if v["outcome"] == "skipped"] covered = sorted(int(k) for k, v in prs.items() if v.get("adversarial")) uncovered = sorted(int(k) for k, v in prs.items() if v.get("adversarial") is False) lines = [ f"Reviewed: {reviewed} (APPROVE {counts['APPROVE']}, REQUEST_CHANGES {counts['REQUEST_CHANGES']}, " - f"COMMENT {counts['COMMENT']})", + f"COMMENT {counts['COMMENT']}, FIXUP {counts['FIXUP']})", f"Skipped: {len(skipped)}" + ( "" diff --git a/tools/pr-management/tests/code_review/test_cli.py b/tools/pr-management/tests/code_review/test_cli.py index 9a96f5c72..4e78bf29e 100644 --- a/tools/pr-management/tests/code_review/test_cli.py +++ b/tools/pr-management/tests/code_review/test_cli.py @@ -324,6 +324,43 @@ def test_session_summary(capsys: pytest.CaptureFixture[str], project: Path, tmp_ assert out["counts"] == {"APPROVE": 1, "COMMENT": 1, "skipped": 1} and "Reviewed: 2" in out["text"] +def test_session_records_fixup_as_a_review( + capsys: pytest.CaptureFixture[str], project: Path, tmp_path: Path +) -> None: + ses = tmp_path / "session.json" + for pr, outcome in ((1, "FIXUP"), (2, "APPROVE")): + _run( + capsys, + "--config-dir", + str(project), + "code-review", + "session", + "record", + "--session", + str(ses), + "--pr", + str(pr), + "--outcome", + outcome, + "--now", + NOW.isoformat(), + ) + out = _run( + capsys, + "--config-dir", + str(project), + "code-review", + "session", + "summary", + "--session", + str(ses), + "--now", + NOW.isoformat(), + ) + assert out["counts"] == {"FIXUP": 1, "APPROVE": 1} + assert "Reviewed: 2 (APPROVE 1, REQUEST_CHANGES 0, COMMENT 0, FIXUP 1)" in out["text"] + + def test_slop_comment_builds_quoted_commands( capsys: pytest.CaptureFixture[str], project: Path, tmp_path: Path ) -> None: