fix(robomp): validate PR review comment anchors against the diff before submitting

A single comment on a line outside a diff hunk 422s the whole review on GitHub, which the 422/500 fallback (ported here from the Forgejo work, including payload shaping and commit_id) then degraded into plain issue comments. Now we fetch /pulls/{n}/files, compute per-hunk anchorable line sets, drop non-anchorable comments, fold them into the review summary, and only fall back to issue comments for genuine 422/500s that survive filtering. Files-fetch failure fails open (submit unfiltered).
2026-08-18 03:54:00 -05:00
parent cc00ab161b
commit 71d608aae6
4 changed files with 630 additions and 15 deletions
@@ -77,6 +77,7 @@ class PullRequestFileInfo:
    status: str
    additions: int
    deletions: int
+    patch: str = ""


@dataclass(slots=True, frozen=True)
@@ -179,7 +180,13 @@ def _parse_retry_after(resp: httpx.Response) -> float | None:
 class GitHubClient:
    """Async + sync facades over a small slice of the GitHub REST API."""

-    def __init__(self, token: str, *, transport: httpx.BaseTransport | None = None) -> None:
+    def __init__(
+        self,
+        token: str,
+        *,
+        transport: httpx.BaseTransport | None = None,
+        platform: str = "github",
+    ) -> None:
        self._token = token
        self._headers = {
            "Authorization": f"Bearer {token}",
@@ -188,6 +195,7 @@ class GitHubClient:
            "User-Agent": "robomp/0.1",
        }
        self._transport = transport
+        self._platform = platform

    def _client(self) -> httpx.Client:
        return httpx.Client(
@@ -581,6 +589,26 @@ class GitHubClient:
            f"/repos/{repo}/issues/{number}/labels/{encoded}",
        )

+    def _review_comments_payload(self, comments: list[Mapping[str, Any]]) -> list[dict[str, Any]]:
+        """Adapt canonical host-tool comment shape to the wire schema for this platform.
+
+        GitHub keeps line/side/start_line/start_side; Forgejo/Gitea only reads
+        path/body/new_position (+old_position), so github-only keys are dropped
+        and `line` is mapped to `new_position` for RIGHT-side comments or
+        `old_position` for LEFT-side (removed-line) comments.
+        """
+        if self._platform != "forgejo":
+            return [dict(c) for c in comments]
+        payload: list[dict[str, Any]] = []
+        for c in comments:
+            entry: dict[str, Any] = {"path": c["path"], "body": c["body"]}
+            if str(c.get("side", "RIGHT")).upper() == "LEFT":
+                entry["old_position"] = c["line"]
+            else:
+                entry["new_position"] = c["line"]
+            payload.append(entry)
+        return payload
+
    async def submit_pr_review(
        self,
        *,
@@ -589,12 +617,12 @@ class GitHubClient:
        body: str,
        event: str,
        comments: list[Mapping[str, Any]],
+        commit_id: str | None = None,
    ) -> PullRequestReviewInfo:
-        data = await self.request(
-            "POST",
-            f"/repos/{repo}/pulls/{pr_number}/reviews",
-            json={"body": body, "event": event, "comments": comments},
-        )
+        payload: dict[str, Any] = {"body": body, "event": event, "comments": self._review_comments_payload(comments)}
+        if commit_id:
+            payload["commit_id"] = commit_id
+        data = await self.request("POST", f"/repos/{repo}/pulls/{pr_number}/reviews", json=payload)
        return _pr_review_from_payload(data)

    async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None:
@@ -750,6 +778,7 @@ def _pr_file_from_payload(data: Mapping[str, Any]) -> PullRequestFileInfo:
        status=str(data.get("status") or ""),
        additions=int(data.get("additions") or 0),
        deletions=int(data.get("deletions") or 0),
+        patch=str(data.get("patch") or ""),
    )


@@ -57,6 +57,7 @@ _AGENT_HOME = Path("/srv/agent-home")
 _PRE_PR_FIX_TIMEOUT_SECONDS = 600.0
 _PRE_PR_CHECK_TIMEOUT_SECONDS = 600.0
 _PRE_PR_CHECK_MAX_OUTPUT = 12_000
+_DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@")


@dataclass(slots=True)
@@ -1741,6 +1742,81 @@ def _build_pr_review_comment(bindings: ToolBindings) -> HostTool[Any, Any]:
    )


+def _diff_anchorable_lines(patch: str) -> tuple[frozenset[int], frozenset[int]]:
+    """Map a unified-diff patch to (RIGHT, LEFT) anchorable line sets.
+
+    RIGHT = new-file line numbers of added (+) lines and in-hunk context
+    lines; LEFT = old-file line numbers of deleted (-) lines and in-hunk
+    context lines. GitHub only anchors review comments on lines inside a
+    hunk; a comment elsewhere 422s the whole review.
+    """
+    right: set[int] = set()
+    left: set[int] = set()
+    new_line: int | None = None
+    old_line: int | None = None
+    for raw in patch.splitlines():
+        m = _DIFF_HUNK_RE.match(raw)
+        if m:
+            old_line = int(m.group(1))
+            new_line = int(m.group(2))
+            continue
+        if raw.startswith("+++") or raw.startswith("---") or raw.startswith("\\"):
+            continue
+        if new_line is None or old_line is None:
+            continue
+        if raw.startswith("+"):
+            right.add(new_line)
+            new_line += 1
+        elif raw.startswith("-"):
+            left.add(old_line)
+            old_line += 1
+        else:
+            right.add(new_line)
+            left.add(old_line)
+            new_line += 1
+            old_line += 1
+    return frozenset(right), frozenset(left)
+
+
+def _filter_anchorable_comments(
+    staged: list[Any], files: list[PullRequestFileInfo]
+) -> tuple[list[Any], list[Any]]:
+    """Partition staged comments into (anchorable, dropped) by diff hunk membership."""
+    by_path = {f.path: f for f in files}
+    cache: dict[str, tuple[frozenset[int], frozenset[int]]] = {}
+    anchorable: list[Any] = []
+    dropped: list[Any] = []
+    for c in staged:
+        entry = by_path.get(c.path)
+        if entry is None:
+            dropped.append(c)  # path not in the PR diff (stale/renamed path)
+            continue
+        if entry.path not in cache:
+            cache[entry.path] = _diff_anchorable_lines(entry.patch)
+        right, left = cache[entry.path]
+        if not entry.patch:
+            # Platform omitted the patch (binary file, no-op, API gap) —
+            # fail open per handoff §3c; a genuine rejection is caught by
+            # the 422/500 fallback after submit.
+            anchorable.append(c)
+            continue
+        side = str(c.side or "RIGHT").upper()
+        lines = right if side == "RIGHT" else left
+        if c.start_line is not None:
+            start_side = str(c.start_side or side).upper()
+            if start_side != side or c.start_line > c.line:
+                dropped.append(c)
+                continue
+            if c.start_line not in lines or c.line not in lines:
+                dropped.append(c)
+                continue
+        elif c.line not in lines:
+            dropped.append(c)
+            continue
+        anchorable.append(c)
+    return anchorable, dropped
+
+
 def _build_submit_pr_review(bindings: ToolBindings) -> HostTool[Any, Any]:
    def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str:
        _require_review_mode(bindings, "submit_pr_review", args)
@@ -1751,27 +1827,133 @@ def _build_submit_pr_review(bindings: ToolBindings) -> HostTool[Any, Any]:
            _raise_command(msg)
        staged = bindings.db.list_staged_review_comments(bindings.issue_key)
        comments = [_review_comment_to_payload(comment) for comment in staged]
+        # commit_id is only needed by Forgejo to anchor inline review comments.
+        # GitHub's reviews endpoint ignores it, so skip the extra API call.
+        commit_id: str | None = None
+        if getattr(bindings.github, "_platform", "github") == "forgejo":
+            try:
+                pr = _run_coro(
+                    bindings.loop,
+                    bindings.github.get_pull_request(
+                        repo=bindings.repo.full_name,
+                        number=bindings.default_comment_number,
+                    ),
+                )
+                commit_id = pr.head_sha or None
+            except GitHubError:
+                commit_id = None
+        body = body.strip()
+        dropped: list[Any] = []
+        if staged:
+            try:
+                pr_files = _run_coro(
+                    bindings.loop,
+                    bindings.github.list_pr_files(
+                        repo=bindings.repo.full_name,
+                        pr_number=bindings.default_comment_number,
+                    ),
+                )
+            except GitHubError as exc:
+                # Fail open: patch fetch failed, submit unfiltered — the
+                # 422/500 fallback below still catches a bad batch.
+                _audit(
+                    bindings,
+                    "submit_pr_review",
+                    args,
+                    error=f"anchor validation skipped: {exc.status} {exc.message}",
+                )
+            else:
+                filtered, dropped = _filter_anchorable_comments(staged, pr_files)
+                if dropped:
+                    _audit(
+                        bindings,
+                        "submit_pr_review",
+                        args,
+                        result={"dropped": [f"{c.path}:{c.line}" for c in dropped]},
+                    )
+                    body += "\n\n## Not anchored to diff"
+                    for c in dropped:
+                        body += f"\n- **`{c.path}:{c.line}`** — {c.body}"
+                    comments = [_review_comment_to_payload(c) for c in filtered]
        try:
            review = _run_coro(
                bindings.loop,
                bindings.github.submit_pr_review(
                    repo=bindings.repo.full_name,
                    pr_number=bindings.default_comment_number,
-                    body=body.strip(),
+                    body=body,
                    event="COMMENT",
                    comments=comments,
+                    commit_id=commit_id,
                ),
            )
        except GitHubError as exc:
            _audit(bindings, "submit_pr_review", args, error=str(exc))
-            _raise_command(f"GitHub rejected PR review: {exc.status} {exc.message}")
+            # 422 = validation rejection (e.g. Forgejo can't anchor inline
+            # comments). 500 = Forgejo internal error on the reviews endpoint
+            # (observed on certain PRs — the server crashes instead of returning
+            # a proper 422). Both mean the batched review can't land as-is.
+            # Degrade to visible issue comments (mirrors mira) so findings still
+            # surface and the model doesn't retry and degrade its own output.
+            # Without this fallback, a 500 propagates to the model as a raw
+            # error, triggering a retry-and-simplify loop where the model
+            # strips newlines from its review body on subsequent attempts.
+            if exc.status not in (422, 500):
+                _raise_command(f"GitHub rejected PR review: {exc.status} {exc.message}")
+
+            def _note(comment: Any) -> str:
+                return f"**`{comment.path}:{comment.line}`**\n\n{comment.body}"
+
+            posted_inline = 0
+            try:
+                _run_coro(
+                    bindings.loop,
+                    bindings.github.post_comment(bindings.repo.full_name, bindings.default_comment_number, body),
+                )
+                for comment in staged:
+                    _run_coro(
+                        bindings.loop,
+                        bindings.github.post_comment(
+                            bindings.repo.full_name, bindings.default_comment_number, _note(comment)
+                        ),
+                    )
+                    posted_inline += 1
+            except GitHubError as fexc:
+                _audit(bindings, "submit_pr_review", args, error=str(fexc))
+                _raise_command(
+                    f"Review rejected ({exc.status}) and fallback comment posting failed: {fexc.status} {fexc.message}"
+                )
+
+            cleared = bindings.db.clear_staged_review_comments(bindings.issue_key)
+            _audit(
+                bindings,
+                "submit_pr_review",
+                args,
+                result={
+                    "fallback": "issue_comments",
+                    "summary": True,
+                    "inline": posted_inline,
+                    "cleared": cleared,
+                },
+            )
+            return (
+                f"review rejected ({exc.status}); posted summary + {posted_inline} inline comment(s) as issue comments"
+            )
        cleared = bindings.db.clear_staged_review_comments(bindings.issue_key)
        _audit(
            bindings,
            "submit_pr_review",
            args,
-            result={"review_id": review.id, "comments": len(comments), "cleared": cleared, "event": "COMMENT"},
+            result={
+                "review_id": review.id,
+                "comments": len(comments),
+                "dropped": len(dropped),
+                "cleared": cleared,
+                "event": "COMMENT",
+            },
        )
+        if dropped:
+            return f"submitted PR review id={review.id}; comments={len(comments)}; dropped={len(dropped)} not anchored to diff"
        return f"submitted PR review id={review.id}; comments={len(comments)}"

    return host_tool(