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).
This commit is contained in:
@@ -77,6 +77,7 @@ class PullRequestFileInfo:
|
|||||||
status: str
|
status: str
|
||||||
additions: int
|
additions: int
|
||||||
deletions: int
|
deletions: int
|
||||||
|
patch: str = ""
|
||||||
|
|
||||||
|
|
||||||
@dataclass(slots=True, frozen=True)
|
@dataclass(slots=True, frozen=True)
|
||||||
@@ -179,7 +180,13 @@ def _parse_retry_after(resp: httpx.Response) -> float | None:
|
|||||||
class GitHubClient:
|
class GitHubClient:
|
||||||
"""Async + sync facades over a small slice of the GitHub REST API."""
|
"""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._token = token
|
||||||
self._headers = {
|
self._headers = {
|
||||||
"Authorization": f"Bearer {token}",
|
"Authorization": f"Bearer {token}",
|
||||||
@@ -188,6 +195,7 @@ class GitHubClient:
|
|||||||
"User-Agent": "robomp/0.1",
|
"User-Agent": "robomp/0.1",
|
||||||
}
|
}
|
||||||
self._transport = transport
|
self._transport = transport
|
||||||
|
self._platform = platform
|
||||||
|
|
||||||
def _client(self) -> httpx.Client:
|
def _client(self) -> httpx.Client:
|
||||||
return httpx.Client(
|
return httpx.Client(
|
||||||
@@ -581,6 +589,26 @@ class GitHubClient:
|
|||||||
f"/repos/{repo}/issues/{number}/labels/{encoded}",
|
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(
|
async def submit_pr_review(
|
||||||
self,
|
self,
|
||||||
*,
|
*,
|
||||||
@@ -589,12 +617,12 @@ class GitHubClient:
|
|||||||
body: str,
|
body: str,
|
||||||
event: str,
|
event: str,
|
||||||
comments: list[Mapping[str, Any]],
|
comments: list[Mapping[str, Any]],
|
||||||
|
commit_id: str | None = None,
|
||||||
) -> PullRequestReviewInfo:
|
) -> PullRequestReviewInfo:
|
||||||
data = await self.request(
|
payload: dict[str, Any] = {"body": body, "event": event, "comments": self._review_comments_payload(comments)}
|
||||||
"POST",
|
if commit_id:
|
||||||
f"/repos/{repo}/pulls/{pr_number}/reviews",
|
payload["commit_id"] = commit_id
|
||||||
json={"body": body, "event": event, "comments": comments},
|
data = await self.request("POST", f"/repos/{repo}/pulls/{pr_number}/reviews", json=payload)
|
||||||
)
|
|
||||||
return _pr_review_from_payload(data)
|
return _pr_review_from_payload(data)
|
||||||
|
|
||||||
async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None:
|
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 ""),
|
status=str(data.get("status") or ""),
|
||||||
additions=int(data.get("additions") or 0),
|
additions=int(data.get("additions") or 0),
|
||||||
deletions=int(data.get("deletions") 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_FIX_TIMEOUT_SECONDS = 600.0
|
||||||
_PRE_PR_CHECK_TIMEOUT_SECONDS = 600.0
|
_PRE_PR_CHECK_TIMEOUT_SECONDS = 600.0
|
||||||
_PRE_PR_CHECK_MAX_OUTPUT = 12_000
|
_PRE_PR_CHECK_MAX_OUTPUT = 12_000
|
||||||
|
_DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@")
|
||||||
|
|
||||||
|
|
||||||
@dataclass(slots=True)
|
@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 _build_submit_pr_review(bindings: ToolBindings) -> HostTool[Any, Any]:
|
||||||
def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str:
|
def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str:
|
||||||
_require_review_mode(bindings, "submit_pr_review", args)
|
_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)
|
_raise_command(msg)
|
||||||
staged = bindings.db.list_staged_review_comments(bindings.issue_key)
|
staged = bindings.db.list_staged_review_comments(bindings.issue_key)
|
||||||
comments = [_review_comment_to_payload(comment) for comment in staged]
|
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:
|
try:
|
||||||
review = _run_coro(
|
review = _run_coro(
|
||||||
bindings.loop,
|
bindings.loop,
|
||||||
bindings.github.submit_pr_review(
|
bindings.github.submit_pr_review(
|
||||||
repo=bindings.repo.full_name,
|
repo=bindings.repo.full_name,
|
||||||
pr_number=bindings.default_comment_number,
|
pr_number=bindings.default_comment_number,
|
||||||
body=body.strip(),
|
body=body,
|
||||||
event="COMMENT",
|
event="COMMENT",
|
||||||
comments=comments,
|
comments=comments,
|
||||||
|
commit_id=commit_id,
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
except GitHubError as exc:
|
except GitHubError as exc:
|
||||||
_audit(bindings, "submit_pr_review", args, error=str(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)
|
cleared = bindings.db.clear_staged_review_comments(bindings.issue_key)
|
||||||
_audit(
|
_audit(
|
||||||
bindings,
|
bindings,
|
||||||
"submit_pr_review",
|
"submit_pr_review",
|
||||||
args,
|
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 f"submitted PR review id={review.id}; comments={len(comments)}"
|
||||||
|
|
||||||
return host_tool(
|
return host_tool(
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
|
import json
|
||||||
|
|
||||||
import httpx
|
import httpx
|
||||||
import pytest
|
import pytest
|
||||||
@@ -166,7 +167,15 @@ def test_list_pr_files_parses_changed_file_summary() -> None:
|
|||||||
assert request.url.params.get("per_page") == "100"
|
assert request.url.params.get("per_page") == "100"
|
||||||
return httpx.Response(
|
return httpx.Response(
|
||||||
200,
|
200,
|
||||||
json=[{"filename": "src/app.py", "status": "modified", "additions": 5, "deletions": 2}],
|
json=[
|
||||||
|
{
|
||||||
|
"filename": "src/app.py",
|
||||||
|
"status": "modified",
|
||||||
|
"additions": 5,
|
||||||
|
"deletions": 2,
|
||||||
|
"patch": "@@ -8,3 +8,5 @@\n ctx\n+added\n ctx2",
|
||||||
|
}
|
||||||
|
],
|
||||||
)
|
)
|
||||||
|
|
||||||
client = GitHubClient("tok", transport=httpx.MockTransport(handler))
|
client = GitHubClient("tok", transport=httpx.MockTransport(handler))
|
||||||
@@ -175,6 +184,20 @@ def test_list_pr_files_parses_changed_file_summary() -> None:
|
|||||||
assert files[0].path == "src/app.py"
|
assert files[0].path == "src/app.py"
|
||||||
assert files[0].additions == 5
|
assert files[0].additions == 5
|
||||||
assert files[0].deletions == 2
|
assert files[0].deletions == 2
|
||||||
|
assert files[0].patch.startswith("@@ -8,3 +8,5")
|
||||||
|
|
||||||
|
|
||||||
|
def test_list_pr_files_defaults_missing_patch_to_empty() -> None:
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
assert request.url.path == "/repos/octo/widget/pulls/9/files"
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json=[{"filename": "src/app.py", "status": "modified", "additions": 5, "deletions": 2}],
|
||||||
|
)
|
||||||
|
|
||||||
|
client = GitHubClient("tok", transport=httpx.MockTransport(handler))
|
||||||
|
files = _run_async(client.list_pr_files("octo/widget", 9))
|
||||||
|
assert files[0].patch == ""
|
||||||
|
|
||||||
|
|
||||||
def test_list_pr_files_paginates_past_first_page() -> None:
|
def test_list_pr_files_paginates_past_first_page() -> None:
|
||||||
@@ -248,6 +271,48 @@ def test_submit_pr_review_posts_comment_event_and_inline_comments() -> None:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_forgejo_uses_new_position_payload() -> None:
|
||||||
|
captured: dict[str, object] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
captured["path"] = request.url.path
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": "summary",
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
client = GitHubClient("tok", transport=httpx.MockTransport(handler), platform="forgejo")
|
||||||
|
review = _run_async(
|
||||||
|
client.submit_pr_review(
|
||||||
|
repo="octo/widget",
|
||||||
|
pr_number=9,
|
||||||
|
body="summary",
|
||||||
|
event="COMMENT",
|
||||||
|
comments=[
|
||||||
|
{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "finding"},
|
||||||
|
{"path": "src/old.py", "line": 5, "side": "LEFT", "body": "removed-line finding"},
|
||||||
|
],
|
||||||
|
)
|
||||||
|
)
|
||||||
|
assert review.id == 44
|
||||||
|
assert captured["path"] == "/repos/octo/widget/pulls/9/reviews"
|
||||||
|
assert captured["body"] == {
|
||||||
|
"body": "summary",
|
||||||
|
"event": "COMMENT",
|
||||||
|
"comments": [
|
||||||
|
{"path": "src/app.py", "body": "finding", "new_position": 12},
|
||||||
|
{"path": "src/old.py", "body": "removed-line finding", "old_position": 5},
|
||||||
|
],
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
def test_204_no_content_returns_none() -> None:
|
def test_204_no_content_returns_none() -> None:
|
||||||
transport = httpx.MockTransport(lambda r: httpx.Response(204))
|
transport = httpx.MockTransport(lambda r: httpx.Response(204))
|
||||||
client = GitHubClient("tok", transport=transport)
|
client = GitHubClient("tok", transport=transport)
|
||||||
|
|||||||
@@ -62,6 +62,28 @@ def _stub_repo() -> RepoInfo:
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# Unified diff whose anchorable lines are RIGHT {10..14} / LEFT {9..12};
|
||||||
|
# line 15+ is a gap (unanchorable) and RIGHT line 9 is before the hunk.
|
||||||
|
_PATCH = (
|
||||||
|
"@@ -9,5 +10,6 @@ def f():\n"
|
||||||
|
" ctx1\n"
|
||||||
|
"-old10\n"
|
||||||
|
"+new11\n"
|
||||||
|
" ctx2\n"
|
||||||
|
"+new13\n"
|
||||||
|
" ctx3\n"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _pr_files_response(request: httpx.Request, patch: str = _PATCH, *, status: int = 200) -> httpx.Response:
|
||||||
|
if status != 200:
|
||||||
|
return httpx.Response(status, json={"message": "files fetch failed"})
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json=[{"filename": "src/app.py", "status": "modified", "additions": 1, "deletions": 1, "patch": patch}],
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def _make_loop_in_background() -> tuple[asyncio.AbstractEventLoop, threading.Thread]:
|
def _make_loop_in_background() -> tuple[asyncio.AbstractEventLoop, threading.Thread]:
|
||||||
loop = asyncio.new_event_loop()
|
loop = asyncio.new_event_loop()
|
||||||
t = threading.Thread(target=loop.run_forever, daemon=True)
|
t = threading.Thread(target=loop.run_forever, daemon=True)
|
||||||
@@ -1234,6 +1256,8 @@ def test_pr_review_comment_stages_and_submit_flushes_one_comment_review(db: Data
|
|||||||
captured: dict[str, Any] = {}
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
def handler(request: httpx.Request) -> httpx.Response:
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
if request.url.path.endswith("/reviews"):
|
if request.url.path.endswith("/reviews"):
|
||||||
captured["path"] = request.url.path
|
captured["path"] = request.url.path
|
||||||
captured["body"] = json.loads(request.content)
|
captured["body"] = json.loads(request.content)
|
||||||
@@ -1315,11 +1339,12 @@ def test_submit_pr_review_posts_summary_only_when_no_staged_comments(db: Databas
|
|||||||
|
|
||||||
|
|
||||||
def test_submit_pr_review_failure_keeps_staged_comments(db: Database, tmp_path: Path) -> None:
|
def test_submit_pr_review_failure_keeps_staged_comments(db: Database, tmp_path: Path) -> None:
|
||||||
bindings, loop, t = _review_bindings(
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
db,
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
tmp_path,
|
return _pr_files_response(request)
|
||||||
httpx.MockTransport(lambda _request: httpx.Response(422, json={"message": "Validation failed"})),
|
return httpx.Response(403, json={"message": "forbidden"})
|
||||||
)
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
try:
|
try:
|
||||||
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
@@ -1334,6 +1359,320 @@ def test_submit_pr_review_failure_keeps_staged_comments(db: Database, tmp_path:
|
|||||||
assert rows[0].path == "src/app.py"
|
assert rows[0].path == "src/app.py"
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_drops_unanchorable_comment_and_folds_into_summary(db: Database, tmp_path: Path) -> None:
|
||||||
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": captured["body"]["body"],
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 12, "body": "in-hunk finding"}, _ctx())
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 15, "body": "gap finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert captured["body"]["body"] == "summary\n\n## Not anchored to diff\n- **`src/app.py:15`** — gap finding"
|
||||||
|
assert captured["body"]["comments"] == [
|
||||||
|
{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "in-hunk finding"}
|
||||||
|
]
|
||||||
|
assert "submitted PR review" in result
|
||||||
|
assert "dropped=1" in result
|
||||||
|
assert db.list_staged_review_comments(bindings.issue_key) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_drops_all_comments_submits_summary_only(db: Database, tmp_path: Path) -> None:
|
||||||
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": "ok",
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 15, "body": "gap finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert captured["body"]["comments"] == []
|
||||||
|
assert captured["body"]["body"].endswith("## Not anchored to diff\n- **`src/app.py:15`** — gap finding")
|
||||||
|
assert "comments=0" in result
|
||||||
|
assert "dropped=1" in result
|
||||||
|
assert db.list_staged_review_comments(bindings.issue_key) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_drops_comment_for_path_not_in_diff(db: Database, tmp_path: Path) -> None:
|
||||||
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": "ok",
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/other.py", "line": 3, "body": "stale path finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert captured["body"]["comments"] == []
|
||||||
|
assert captured["body"]["body"].endswith("## Not anchored to diff\n- **`src/other.py:3`** — stale path finding")
|
||||||
|
assert "dropped=1" in result
|
||||||
|
assert db.list_staged_review_comments(bindings.issue_key) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_skips_validation_when_files_fetch_fails(db: Database, tmp_path: Path) -> None:
|
||||||
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request, status=500)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": "ok",
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 15, "body": "gap finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert captured["body"]["comments"] == [
|
||||||
|
{"path": "src/app.py", "line": 15, "side": "RIGHT", "body": "gap finding"}
|
||||||
|
]
|
||||||
|
assert captured["body"]["body"] == "summary"
|
||||||
|
assert "comments=1" in result
|
||||||
|
assert "dropped" not in result
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_422_falls_back_to_issue_comments(db: Database, tmp_path: Path) -> None:
|
||||||
|
comment_bodies: list[str] = []
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
return httpx.Response(422, json={"message": "Validation failed"})
|
||||||
|
if request.url.path == "/repos/octo/widget/issues/99/comments":
|
||||||
|
body = json.loads(request.content)["body"]
|
||||||
|
comment_bodies.append(body)
|
||||||
|
return httpx.Response(200, json={"id": len(comment_bodies), "body": body})
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 12, "body": "finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert "posted summary + 1 inline comment(s) as issue comments" in result
|
||||||
|
assert comment_bodies == ["summary", "**`src/app.py:12`**\n\nfinding"]
|
||||||
|
assert db.list_staged_review_comments(bindings.issue_key) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_500_falls_back_to_issue_comments(db: Database, tmp_path: Path) -> None:
|
||||||
|
"""A 500 from Forgejo's reviews endpoint triggers the same fallback as 422.
|
||||||
|
|
||||||
|
Without this, the 500 propagates to the model, causing a retry-and-degrade
|
||||||
|
loop where the model strips newlines from subsequent review bodies.
|
||||||
|
"""
|
||||||
|
comment_bodies: list[str] = []
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
return httpx.Response(500, json={"message": "github error"})
|
||||||
|
if request.url.path == "/repos/octo/widget/issues/99/comments":
|
||||||
|
body = json.loads(request.content)["body"]
|
||||||
|
comment_bodies.append(body)
|
||||||
|
return httpx.Response(200, json={"id": len(comment_bodies), "body": body})
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
stage_tool.execute({"path": "src/app.py", "line": 12, "body": "finding"}, _ctx())
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert "posted summary + 1 inline comment(s) as issue comments" in result
|
||||||
|
assert comment_bodies == ["summary", "**`src/app.py:12`**\n\nfinding"]
|
||||||
|
assert db.list_staged_review_comments(bindings.issue_key) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_submit_pr_review_range_requires_both_endpoints_anchorable(db: Database, tmp_path: Path) -> None:
|
||||||
|
captured: dict[str, Any] = {}
|
||||||
|
|
||||||
|
def handler(request: httpx.Request) -> httpx.Response:
|
||||||
|
if request.url.path == "/repos/octo/widget/pulls/99/files":
|
||||||
|
return _pr_files_response(request)
|
||||||
|
if request.url.path.endswith("/reviews"):
|
||||||
|
captured["body"] = json.loads(request.content)
|
||||||
|
return httpx.Response(
|
||||||
|
200,
|
||||||
|
json={
|
||||||
|
"id": 44,
|
||||||
|
"user": {"login": "robomp-bot"},
|
||||||
|
"body": "ok",
|
||||||
|
"state": "COMMENTED",
|
||||||
|
"submitted_at": "t",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
return httpx.Response(404, json={"message": "unrouted"})
|
||||||
|
|
||||||
|
bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler))
|
||||||
|
try:
|
||||||
|
stage_tool = next(x for x in build(bindings) if x.name == "pr_review_comment")
|
||||||
|
submit_tool = next(x for x in build(bindings) if x.name == "submit_pr_review")
|
||||||
|
# Reversed range: start_line (15) > line (14) — dropped.
|
||||||
|
stage_tool.execute(
|
||||||
|
{
|
||||||
|
"path": "src/app.py",
|
||||||
|
"line": 14,
|
||||||
|
"start_line": 15,
|
||||||
|
"start_side": "RIGHT",
|
||||||
|
"side": "RIGHT",
|
||||||
|
"body": "reversed range",
|
||||||
|
},
|
||||||
|
_ctx(),
|
||||||
|
)
|
||||||
|
# Valid range: both endpoints (10, 12) are context lines in the hunk — kept.
|
||||||
|
stage_tool.execute(
|
||||||
|
{
|
||||||
|
"path": "src/app.py",
|
||||||
|
"line": 12,
|
||||||
|
"start_line": 10,
|
||||||
|
"start_side": "RIGHT",
|
||||||
|
"side": "RIGHT",
|
||||||
|
"body": "valid range",
|
||||||
|
},
|
||||||
|
_ctx(),
|
||||||
|
)
|
||||||
|
# Cross-side range: start_side LEFT with side RIGHT — dropped.
|
||||||
|
stage_tool.execute(
|
||||||
|
{
|
||||||
|
"path": "src/app.py",
|
||||||
|
"line": 11,
|
||||||
|
"start_line": 10,
|
||||||
|
"start_side": "LEFT",
|
||||||
|
"side": "RIGHT",
|
||||||
|
"body": "cross-side range",
|
||||||
|
},
|
||||||
|
_ctx(),
|
||||||
|
)
|
||||||
|
result = submit_tool.execute({"body": "summary"}, _ctx())
|
||||||
|
finally:
|
||||||
|
_stop_loop(loop, t)
|
||||||
|
|
||||||
|
assert captured["body"]["comments"] == [
|
||||||
|
{
|
||||||
|
"path": "src/app.py",
|
||||||
|
"line": 12,
|
||||||
|
"side": "RIGHT",
|
||||||
|
"body": "valid range",
|
||||||
|
"start_line": 10,
|
||||||
|
"start_side": "RIGHT",
|
||||||
|
}
|
||||||
|
]
|
||||||
|
assert "src/app.py:14" in captured["body"]["body"]
|
||||||
|
assert "src/app.py:11" in captured["body"]["body"]
|
||||||
|
assert "dropped=2" in result
|
||||||
|
|
||||||
|
|
||||||
|
def test_diff_anchorable_lines_parses_hunk_sides() -> None:
|
||||||
|
right, left = host_tools._diff_anchorable_lines(_PATCH)
|
||||||
|
assert right == frozenset({10, 11, 12, 13, 14})
|
||||||
|
assert left == frozenset({9, 10, 11, 12})
|
||||||
|
|
||||||
|
# Gap between hunks: line 30 in the old file / 29..30 in the new file are
|
||||||
|
# unanchorable (the PR 1111 smtp.go:106 failure shape).
|
||||||
|
right, left = host_tools._diff_anchorable_lines(
|
||||||
|
"@@ -1,2 +1,3 @@\n a\n+x\n b\n@@ -30,2 +31,2 @@\n c\n d\n"
|
||||||
|
)
|
||||||
|
assert right >= frozenset({1, 2, 3, 31, 32})
|
||||||
|
assert right.isdisjoint(range(4, 31))
|
||||||
|
|
||||||
|
assert host_tools._diff_anchorable_lines("") == (frozenset(), frozenset())
|
||||||
|
assert host_tools._diff_anchorable_lines("\0Binary files differ") == (frozenset(), frozenset())
|
||||||
|
|
||||||
|
right, left = host_tools._diff_anchorable_lines("@@ -9 +10 @@ x\n+a\n b\n")
|
||||||
|
assert right == frozenset({10, 11})
|
||||||
|
assert left == frozenset({9})
|
||||||
|
|
||||||
|
# `\ No newline at end of file` markers are ignored.
|
||||||
|
right, _ = host_tools._diff_anchorable_lines(
|
||||||
|
"@@ -1,2 +1,3 @@\n a\n+b\n\\ No newline at end of file\n"
|
||||||
|
)
|
||||||
|
assert right == frozenset({1, 2})
|
||||||
|
|
||||||
|
|
||||||
def test_review_tools_reject_outside_review_mode(db: Database, tmp_path: Path) -> None:
|
def test_review_tools_reject_outside_review_mode(db: Database, tmp_path: Path) -> None:
|
||||||
bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500)))
|
bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500)))
|
||||||
try:
|
try:
|
||||||
|
|||||||
Reference in New Issue
Block a user