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

fix(robomp): thread commit_id/head_sha/patch through proxy backend

Browse Source 
Follow-up fix round for 71d608aae6 (validate PR review comment
anchors against the diff before submitting). The validation path
broke in several places the original commit did not cover:

- GitHubBackend.submit_review gained no commit_id parameter, so
  submitting through GitHubProxyClient raised a production
  TypeError on a path the validation code depended on.
- PullRequestInfo lacked head_sha, so the Forgejo commit_id
  fallback raised AttributeError; it is now parsed in
  _pr_from_payload and carried through the proxy round-trip.
- _pr_file_from dropped the file patch, silently no-oping anchor
  validation for anything routed through the proxy; the patch is
  now forwarded.
- The hunk parser treated any +++/--- line as a file header,
  desyncing line counters when added/removed content began with
  those prefixes; file headers are now recognized only before
  the first hunk.
- Reworded the comment to "Forgejo only" to match the actual
  backend behavior.

Adds tests for forgejo commit_id fetch, fallback double-failure,
empty-patch fail-open, LEFT-side anchoring, proxy commit_id
validation, and file-creation hunk boundaries.
This commit is contained in:
djdembeck
2026-08-18 16:25:52 -05:00
parent 71d608aae6
commit e599d58f21
8 changed files with 419 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
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
# ============================================================================