smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #8936: fix(robomp): validate review comment anchors before submitting (@djdembeck)

Browse Source
This commit is contained in:
can1357
2026-08-19 01:39:18 +02:00
parent 8ade6888e6 e599d58f21
commit 2d8837695c
8 changed files with 1025 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
python/robomp/src/github_backend.py
+1
View File
@@ -100,6 +100,7 @@ class GitHubBackend(Protocol):
body: str,
event: str,
comments: list[Mapping[str, Any]],
commit_id: str | None = None,
) -> PullRequestReviewInfo: ...
async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None: ...
python/robomp/src/github_client.py
+37 -6
View File
@@ -69,6 +69,7 @@ class PullRequestInfo:
head_repo: str = ""
title: str = ""
body: str = ""
head_sha: str = ""
@dataclass(slots=True, frozen=True)
@@ -77,6 +78,7 @@ class PullRequestFileInfo:
status: str
additions: int
deletions: int
patch: str = ""
@dataclass(slots=True, frozen=True)
@@ -179,7 +181,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 +196,7 @@ class GitHubClient:
"User-Agent": "robomp/0.1",
}
self._transport = transport
self._platform = platform
def _client(self) -> httpx.Client:
return httpx.Client(
@@ -581,6 +590,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 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 +618,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 +779,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 ""),
)
@@ -769,6 +799,7 @@ def _pr_from_payload(repo: str, data: Mapping[str, Any]) -> PullRequestInfo:
head_repo=str(head_repo.get("full_name") or "") if isinstance(head_repo, Mapping) else "",
title=str(data.get("title") or ""),
body=str(data.get("body") or ""),
head_sha=str(head.get("sha") or "") if isinstance(head, Mapping) else "",
)
python/robomp/src/host_tools.py
+189 -3
View File
@@ -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,85 @@ 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("\\"):
continue
# `+++`/`---` are file headers only before the first hunk. Inside a
# hunk a diff line's *content* may start with `++` (added) or `--`
# (removed), and those lines must advance the counters.
in_hunk = new_line is not None and old_line is not None
if raw.startswith(("+++", "---")) and not in_hunk:
continue
if not in_hunk:
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 +1831,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(
python/robomp/src/proxy/server.py
+3
View File
@@ -671,6 +671,8 @@ def create_proxy_app(settings: Settings) -> FastAPI:
body = _require_str(data.get("body"), "body")
event = str(data.get("event") or "COMMENT")
comments = _require_review_comments(data.get("comments"))
commit_id_raw = data.get("commit_id")
commit_id = commit_id_raw if isinstance(commit_id_raw, str) and commit_id_raw else None
github: GitHubClient = request.app.state.github
try:
review = await github.submit_pr_review(
@@ -679,6 +681,7 @@ def create_proxy_app(settings: Settings) -> FastAPI:
body=body,
event=event,
comments=comments,
commit_id=commit_id,
)
except GitHubError as exc:
return _gh_error_response(exc)
python/robomp/src/proxy_client.py
+13 -7
View File
@@ -334,17 +334,21 @@ class GitHubProxyClient:
body: str,
event: str,
comments: list[Mapping[str, Any]],
commit_id: str | None = None,
) -> PullRequestReviewInfo:
json_body: dict[str, Any] = {
"repo": repo,
"pr_number": pr_number,
"body": body,
"event": event,
"comments": comments,
}
if commit_id:
json_body["commit_id"] = commit_id
data = await self._request(
"POST",
"/gh/v1/submit_pr_review",
json_body={
"repo": repo,
"pr_number": pr_number,
"body": body,
"event": event,
"comments": comments,
},
json_body=json_body,
)
return _pr_review_from(data)
@@ -602,6 +606,7 @@ def _pr_file_from(data: 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 ""),
)
@@ -619,6 +624,7 @@ def _pr_from(data: Any) -> PullRequestInfo:
head_repo=str(data.get("head_repo") or ""),
title=str(data.get("title") or ""),
body=str(data.get("body") or ""),
head_sha=str(data.get("head_sha") or ""),
)
python/robomp/tests/test_github_client.py
+72 -2
View File
@@ -3,6 +3,7 @@
from __future__ import annotations
import asyncio
import json
import httpx
import pytest
@@ -123,7 +124,11 @@ def test_get_pull_request_parses_head_repo_and_author() -> None:
json={
"number": 9,
"html_url": "https://github.com/octo/widget/pull/9",
"head": {"ref": "farm/abc12345/fix", "repo": {"full_name": "octo/widget"}},
"head": {
"ref": "farm/abc12345/fix",
"sha": "abc1234567890123456789012345678901234567",
"repo": {"full_name": "octo/widget"},
},
"base": {"ref": "main"},
"state": "open",
"user": {"login": "robomp-bot"},
@@ -133,6 +138,7 @@ def test_get_pull_request_parses_head_repo_and_author() -> None:
client = GitHubClient("tok", transport=httpx.MockTransport(handler))
pr = _run_async(client.get_pull_request("octo/widget", 9))
assert pr.head_ref == "farm/abc12345/fix"
assert pr.head_sha == "abc1234567890123456789012345678901234567"
assert pr.head_repo == "octo/widget"
assert pr.author == "robomp-bot"
@@ -166,7 +172,15 @@ def test_list_pr_files_parses_changed_file_summary() -> None:
assert request.url.params.get("per_page") == "100"
return httpx.Response(
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))
@@ -175,6 +189,20 @@ def test_list_pr_files_parses_changed_file_summary() -> None:
assert files[0].path == "src/app.py"
assert files[0].additions == 5
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:
@@ -248,6 +276,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:
transport = httpx.MockTransport(lambda r: httpx.Response(204))
client = GitHubClient("tok", transport=transport)
python/robomp/tests/test_host_tools.py
+555 -7
View File
@@ -62,6 +62,20 @@ 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]:
loop = asyncio.new_event_loop()
t = threading.Thread(target=loop.run_forever, daemon=True)
@@ -1112,9 +1126,13 @@ def _pr_bindings(
def _review_bindings(
db: Database, tmp_path: Path, transport: httpx.MockTransport
db: Database,
tmp_path: Path,
transport: httpx.MockTransport,
*,
platform: str = "github",
) -> tuple[ToolBindings, asyncio.AbstractEventLoop, threading.Thread]:
github = GitHubClient("token", transport=transport)
github = GitHubClient("token", transport=transport, platform=platform)
loop, thread = _make_loop_in_background()
issue = IssueInfo(
repo="octo/widget",
@@ -1234,6 +1252,8 @@ def test_pr_review_comment_stages_and_submit_flushes_one_comment_review(db: Data
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["path"] = request.url.path
captured["body"] = json.loads(request.content)
@@ -1315,11 +1335,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:
bindings, loop, t = _review_bindings(
db,
tmp_path,
httpx.MockTransport(lambda _request: httpx.Response(422, json={"message": "Validation failed"})),
)
def handler(request: httpx.Request) -> httpx.Response:
if request.url.path == "/repos/octo/widget/pulls/99/files":
return _pr_files_response(request)
return httpx.Response(403, json={"message": "forbidden"})
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")
@@ -1334,6 +1355,533 @@ def test_submit_pr_review_failure_keeps_staged_comments(db: Database, tmp_path:
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_submit_pr_review_forgejo_fetches_commit_id(db: Database, tmp_path: Path) -> None:
"""Forgejo anchors inline comments by commit: submit must fetch the PR and
include the head sha as commit_id in the reviews POST body."""
captured: dict[str, Any] = {}
def handler(request: httpx.Request) -> httpx.Response:
if request.url.path == "/repos/octo/widget/pulls/99":
return httpx.Response(
200,
json={
"number": 99,
"html_url": "https://x/octo/widget/pull/99",
"head": {"ref": "fix-crash", "sha": "abc123456789"},
"base": {"ref": "main"},
"state": "open",
"user": {"login": "alice"},
},
)
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), platform="forgejo")
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())
result = submit_tool.execute({"body": "summary"}, _ctx())
finally:
_stop_loop(loop, t)
assert "submitted PR review" in result
assert captured["body"]["commit_id"] == "abc123456789"
def test_submit_pr_review_forgejo_commit_id_fetch_failure_is_swallowed(
db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""When the PR fetch fails the commit_id is omitted (fail open) and the
review still submits without it."""
monkeypatch.setattr(GitHubClient, "_TRANSIENT_RETRY_DELAYS", (0.01, 0.01))
captured: dict[str, Any] = {}
def handler(request: httpx.Request) -> httpx.Response:
if request.url.path == "/repos/octo/widget/pulls/99":
return httpx.Response(500, json={"message": "internal error"})
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), platform="forgejo")
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())
result = submit_tool.execute({"body": "summary"}, _ctx())
finally:
_stop_loop(loop, t)
assert "submitted PR review" in result
assert "commit_id" not in captured["body"]
def test_submit_pr_review_422_and_fallback_comment_failure_raises_and_keeps_staged(
db: Database,
tmp_path: Path,
) -> None:
"""When the reviews endpoint AND the issue-comments fallback both fail, the
tool raises and the staged comments survive for a later retry."""
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":
return httpx.Response(500, json={"message": "internal error"})
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())
with pytest.raises(RpcCommandError, match="fallback comment posting failed"):
submit_tool.execute({"body": "summary"}, _ctx())
finally:
_stop_loop(loop, t)
rows = db.list_staged_review_comments(bindings.issue_key)
assert len(rows) == 1
assert rows[0].path == "src/app.py"
def test_submit_pr_review_empty_patch_fails_open(db: Database, tmp_path: Path) -> None:
"""A file whose patch the platform omitted is not a rejection reason: the
comment is kept rather than folded into a 'Not anchored to diff' section."""
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, patch="")
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": 12, "body": "binary finding"}, _ctx())
result = submit_tool.execute({"body": "summary"}, _ctx())
finally:
_stop_loop(loop, t)
assert "submitted PR review" in result
assert "dropped" not in result
assert captured["body"]["comments"] == [
{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "binary finding"}
]
assert "Not anchored to diff" not in captured["body"]["body"]
def test_submit_pr_review_left_side_single_line_anchoring(db: Database, tmp_path: Path) -> None:
"""LEFT-side comments anchor against the old file's hunk lines
(_PATCH: LEFT {9..12}): an in-hunk line is kept, an out-of-hunk line is
dropped and folded into the summary."""
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": 10, "side": "LEFT", "body": "old line finding"}, _ctx())
stage_tool.execute({"path": "src/app.py", "line": 13, "side": "LEFT", "body": "gap finding"}, _ctx())
result = submit_tool.execute({"body": "summary"}, _ctx())
finally:
_stop_loop(loop, t)
assert "dropped=1" in result
assert captured["body"]["comments"] == [
{"path": "src/app.py", "line": 10, "side": "LEFT", "body": "old line finding"}
]
assert "## Not anchored to diff" in captured["body"]["body"]
assert "**`src/app.py:13`** — gap finding" in captured["body"]["body"]
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})
# File-creation hunk boundary: everything is added, nothing exists on the
# LEFT; file-deletion hunk: everything is removed, RIGHT is empty.
right, left = host_tools._diff_anchorable_lines("@@ -0,0 +1,2 @@\n+a\n+b\n")
assert right == frozenset({1, 2})
assert left == frozenset()
right, left = host_tools._diff_anchorable_lines("@@ -5,3 +5,0 @@\n-a\n-b\n-c\n")
assert right == frozenset()
assert left == frozenset({5, 6, 7})
def test_diff_anchorable_lines_in_hunk_plus_minus_content() -> None:
"""Added lines whose content starts with `++` and removed lines starting
with `--` must be treated as diff content, not file headers — otherwise
the line counters desync (the `+++ b/...` skip used to swallow them)."""
patch = "+++ b/src/app.py\n--- a/src/app.py\n@@ -1,2 +1,3 @@\n ctx\n tail\n--- removed\n+++ added\n"
# Old file: ctx(1), tail(2), --- removed(3). New file: ctx(1), tail(2), +++ added(3).
right, left = host_tools._diff_anchorable_lines(patch)
assert right == frozenset({1, 2, 3})
assert left == frozenset({1, 2, 3})
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)))
try:
python/robomp/tests/test_proxy_client.py
+155 -3
View File
@@ -31,7 +31,7 @@ from robomp.github_client import (
)
from robomp.proxy.server import create_proxy_app
from robomp.proxy_client import GitHubProxyClient, ProxyGitTransport
from robomp.proxy_hmac import HEADER_SIGNATURE, HEADER_TIMESTAMP, verify
from robomp.proxy_hmac import HEADER_SIGNATURE, HEADER_TIMESTAMP, sign, verify
from robomp.sandbox import workspace_key
_HMAC = "test-hmac-key-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
@@ -316,7 +316,15 @@ def round_trip_app(proxy_settings: Settings):
if path == "/repos/octo/widget/pulls/2/files":
return httpx.Response(
200,
json=[{"filename": "src/app.py", "status": "modified", "additions": 2, "deletions": 1}],
json=[
{
"filename": "src/app.py",
"status": "modified",
"additions": 2,
"deletions": 1,
"patch": "@@ -8,5 +8,6 @@\n ctx\n-old\n+new\n",
}
],
)
if path == "/repos/octo/widget/pulls/2/reviews" and req.method == "POST":
body = json.loads(req.content)
@@ -340,7 +348,11 @@ def round_trip_app(proxy_settings: Settings):
json={
"number": 4,
"html_url": "https://example/4",
"head": {"ref": "feat", "repo": {"full_name": "octo/widget"}},
"head": {
"ref": "feat",
"sha": "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
"repo": {"full_name": "octo/widget"},
},
"base": {"ref": "main"},
"state": "open",
"user": {"login": "robomp-bot"},
@@ -405,6 +417,7 @@ async def test_round_trip_all_endpoints(round_trip_app) -> None:
files = await client.list_pr_files("octo/widget", 2)
assert len(files) == 1 and isinstance(files[0], PullRequestFileInfo)
assert files[0].path == "src/app.py"
assert files[0].patch == "@@ -8,5 +8,6 @@\n ctx\n-old\n+new\n"
submitted = await client.submit_pr_review(
repo="octo/widget",
@@ -420,6 +433,7 @@ async def test_round_trip_all_endpoints(round_trip_app) -> None:
existing_pr = await client.get_pull_request("octo/widget", 4)
assert isinstance(existing_pr, PullRequestInfo)
assert existing_pr.head_ref == "feat"
assert existing_pr.head_sha == "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"
assert existing_pr.author == "robomp-bot"
posted = await client.post_comment("octo/widget", 1, "hi")
@@ -486,6 +500,144 @@ async def test_close_issue_round_trip(proxy_settings: Settings) -> None:
assert captured["body"] == {"state": "closed", "state_reason": "completed"}
def _capturing_app(app, path: str) -> tuple[Callable, list[dict[str, object]]]:
"""ASGI wrapper recording raw JSON bodies POSTed to `path`."""
bodies: list[dict[str, object]] = []
async def middleware(scope, receive, send):
if scope["type"] == "http" and scope.get("path") == path:
raw = b""
while True:
message = await receive()
raw += message.get("body", b"")
if not message.get("more_body", False):
break
bodies.append(json.loads(raw))
async def replay() -> dict[str, object]:
return {"type": "http.request", "body": raw, "more_body": False}
return await app(scope, replay, send)
return await app(scope, receive, send)
return middleware, bodies
async def test_submit_pr_review_commit_id_reaches_wire(proxy_settings: Settings) -> None:
"""commit_id must appear in the /gh/v1/submit_pr_review wire body the
proxy client POSTs, and the server must forward it to the direct client."""
app = create_proxy_app(proxy_settings)
app.state.settings = proxy_settings
upstream: dict[str, object] = {}
def gh(req: httpx.Request) -> httpx.Response:
if req.url.path == "/repos/octo/widget/pulls/2/reviews" and req.method == "POST":
upstream["body"] = json.loads(req.content)
return httpx.Response(
200,
json={
"id": 55,
"user": {"login": "robomp-bot"},
"body": "summary",
"state": "COMMENTED",
"submitted_at": "t",
},
)
return httpx.Response(404, json={"message": "unrouted"})
_attach_gh(app, gh)
middleware, wire_bodies = _capturing_app(app, "/gh/v1/submit_pr_review")
client = GitHubProxyClient(
base_url="http://proxy.test",
hmac_key=_HMAC,
transport=httpx.ASGITransport(app=middleware),
)
review = await client.submit_pr_review(
repo="octo/widget",
pr_number=2,
body="summary",
event="COMMENT",
comments=[{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "finding"}],
commit_id="abc123",
)
assert review.id == 55
assert wire_bodies == [
{
"repo": "octo/widget",
"pr_number": 2,
"body": "summary",
"event": "COMMENT",
"comments": [{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "finding"}],
"commit_id": "abc123",
}
]
# Server forwarded it to the direct client, which put it on the GitHub wire.
assert upstream["body"]["commit_id"] == "abc123"
# Without commit_id the key is omitted from the proxy wire body.
await client.submit_pr_review(
repo="octo/widget",
pr_number=2,
body="summary",
event="COMMENT",
comments=[],
)
assert "commit_id" not in wire_bodies[1]
@pytest.mark.parametrize("bad_commit_id", [12345, ""])
async def test_submit_pr_review_rejects_non_string_commit_id(proxy_settings: Settings, bad_commit_id: object) -> None:
"""The proxy server must not forward a non-string or empty commit_id
upstream: a raw POST bypasses the proxy client, so assert on the
upstream-captured reviews body — the key must be absent and the request
still succeeds."""
app = create_proxy_app(proxy_settings)
app.state.settings = proxy_settings
upstream: dict[str, object] = {}
def gh(req: httpx.Request) -> httpx.Response:
if req.url.path == "/repos/octo/widget/pulls/3/reviews" and req.method == "POST":
upstream["body"] = json.loads(req.content)
return httpx.Response(
200,
json={
"id": 56,
"user": {"login": "robomp-bot"},
"body": "summary",
"state": "COMMENTED",
"submitted_at": "t",
},
)
return httpx.Response(404, json={"message": "unrouted"})
app.state.github = GitHubClient(_TOKEN, transport=httpx.MockTransport(gh))
payload = {
"repo": "octo/widget",
"pr_number": 3,
"body": "summary",
"event": "COMMENT",
"comments": [],
"commit_id": bad_commit_id,
}
body = json.dumps(payload).encode()
timestamp, sig = sign(method="POST", path="/gh/v1/submit_pr_review", body=body, key=_HMAC_BYTES)
async with httpx.AsyncClient(
transport=httpx.ASGITransport(app=app),
base_url="http://proxy.test",
) as client:
resp = await client.post(
"/gh/v1/submit_pr_review",
content=body,
headers={
HEADER_TIMESTAMP: timestamp,
HEADER_SIGNATURE: sig,
"Content-Type": "application/json",
},
)
assert resp.status_code == 200, resp.text
assert "commit_id" not in upstream["body"]
# ============================================================================
# 3. Error decode
# ============================================================================