diff --git a/python/robomp/docs/pr-review-handoff.md b/python/robomp/docs/pr-review-handoff.md new file mode 100644 index 000000000..a70da8543 --- /dev/null +++ b/python/robomp/docs/pr-review-handoff.md @@ -0,0 +1,373 @@ +# Handoff: incoming-PR review feature + +Wire robomp to **review pull requests opened by contributors** (and other bots), in two +phases: (1) classify + rank, (2) a real line-by-line review posted as one GitHub review. +robomp **never merges, closes, approves, or pushes** — the rank label is the verdict; the +maintainer acts on it. + +Confirmed decisions: +- **COMMENT-only.** `submit_pr_review` always uses `event="COMMENT"`. Never `APPROVE` / + `REQUEST_CHANGES` (those gate merge — the maintainer's call). +- **SQLite staging.** Inline comments are staged in a sqlite table, flushed in one review. + Survives `--continue` resume; honours "DB is the only source of truth, in-memory state is + just `_inflight`." +- **Reuse the issue isolation verbatim.** The PR head is checked out into a per-PR worktree + (clone pool + slot uid + natives cache + scrubbed env) **before** the agent starts, and + that worktree is the agent's cwd. Review is read-only on that checkout. + +The agent prompt already exists: `src/prompts/pr_review_rubric.md` (rename to +`kickoff_pr_review.md` — see §5). Everything below is the wiring around it. + +--- + +## 1. How the existing flows work (the substrate to mirror) + +End-to-end, every task today follows the same spine: + +``` +GitHub webhook + └─ server.py POST /webhook/github (HMAC verify; 401 on bad sig) + └─ github_events.route(event_type, payload, …) → RouteDecision(queue|skip, task, …) + └─ db.record_event(...) INSERT OR IGNORE on X-GitHub-Delivery → 202 + WorkerPool._dispatch_loop (BEGIN IMMEDIATE claim; _inflight set keyed by (owner,repo,number)) + └─ WorkerPool._dispatch(row) re-derives handler from (event_type, action) + └─ tasks.(settings, db, github, sandbox, git_transport, payload, delivery_id, …) + ├─ resolve RepoInfo + IssueInfo (PRs are issues) + ├─ sandbox.ensure_workspace(...) → per-issue worktree (clone pool, slot, natives) + ├─ db.upsert_issue(...) → state + branch + session_dir + └─ worker.run_task(task_kind=..., inputs=TaskInputs, …) + ├─ ToolBindings(inbound_thread_number=pr_number, inbound_is_pr=…) + ├─ _build_prompt(task_kind, …) → persona.(...) + ├─ RpcClient(omp --mode rpc, cwd=worktree, custom_tools=host_tools.build(bindings)) + └─ _drive_turn(...) → completion/dirty reminders until terminal tool / clean +``` + +Existing task kinds and their analogues for us: + +| Task | Trigger (`route`) | Workspace | Terminal action | Notes | +|---|---|---|---|---| +| `triage_issue` | `issues.opened` | fresh `farm//` worktree | `gh_open_pr` / `mark_unable_to_reproduce` / `abort_task` | **The fresh-entry template** for `review_pr`. | +| `handle_comment` | `issue_comment.created` on an issue | resume existing | one `gh_post_comment` | | +| `handle_pr_conversation` | `issue_comment.created` on a PR | resume bot-PR branch | `gh_post_comment` / push | bot-owned PRs only. | +| `handle_review` | `pull_request_review_comment.created` on a **bot-authored** PR | resume via `existing_branch=pr.head_ref` | reply / push | **The PR-context template** — shows `ensure_workspace(existing_branch=…)` and `inbound_is_pr`. | +| `cleanup_workspace` | `issues.closed` / bot `pull_request.merged` | removes worktree | — | | + +Two facts that shape the wiring: +- `route()` and `WorkerPool._dispatch()` **both** branch on `(event_type, action)`. `route` + decides queue/skip + carries `submitter`/`directive`; `_dispatch` re-derives the handler. + **A new task kind must be added in both.** +- `host_tools.build(bindings)` returns an **identical tuple for every task kind** (to keep the + LLM prompt cache warm); tools **self-gate at execution time** (e.g. `classify_issue` rejects + when `bindings.inbound_is_pr`). We follow the same pattern: add the new tools to `build()` + unconditionally and gate them on a `review_mode` flag. + +--- + +## 2. Routing (`src/github_events.py`) + +### 2a. New entry: incoming PR opened + +Add a `pull_request` branch **before** the existing `pull_request`/`closed` block. Trigger a +one-shot review on `opened`, `reopened`, and `ready_for_review`; **skip everything else** +(notably `synchronize` already falls through to the final skip — keep it that way: do **not** +re-review on new commits). + +```python +if event_type == "pull_request" and action in ("opened", "reopened", "ready_for_review"): + pr = payload.get("pull_request") or {} + if bool(pr.get("draft")): + return RouteDecision("skip", None, repo, None, "draft PR") + pr_user = pr.get("user") or {} + if _is_bot_account(pr_user, bot_login): + return RouteDecision("skip", None, repo, None, "bot-authored PR") # our own farm PRs + number = pr.get("number") + if not isinstance(number, int): + return RouteDecision("skip", None, repo, None, "PR missing number") + login, assoc = _submitter_info(pr) # PR author = rate-limit subject + return RouteDecision("queue", "review_pr", repo, issue_key(repo, number), + f"pull_request.{action}", submitter=login, association=assoc) +``` + +Use the PR's **own** key (`issue_key(repo, number)`), not `_resolve_pr_key` — an incoming PR +has no originating bot issue. + +### 2b. Gate incoming-PR comments ("don't run on comments unless I ask") + +Today `issue_comment.created` on **any** PR queues `handle_pr_conversation`. For incoming +(non-bot) PRs that would make the bot respond to every comment. Change the PR branch of the +`issue_comment` handler to: + +- PR author **is** the bot → `handle_pr_conversation` (unchanged). +- PR author is **not** the bot → **skip**, *unless* `_directive_kwargs(...)` is non-empty + (a maintainer `@bot` mention or a configured reviewer bot). A directive routes to the + existing directive path; only an explicit "re-review" directive re-runs the review. + +The PR author is on `payload.issue.user.login` for `issue_comment` events. `synchronize`, +`edited`, etc. need no change (they already skip). + +### 2c. Cleanup for incoming PRs + +`pull_request.closed` currently requires a bot-authored, merged PR. Incoming-PR review +worktrees would otherwise leak. Extend the close branch (or add a TTL sweep) so an incoming +PR's worktree is GC'd on close. Minimal: when the PR has a workspace row, route +`pull_request.closed` → `cleanup_workspace` regardless of author/merge. + +--- + +## 3. Dispatch (`src/queue.py` `_dispatch`) + +Add a branch mirroring `triage_issue`: + +```python +elif event == "pull_request" and action in ("opened", "reopened", "ready_for_review"): + await tasks.review_pr( + settings=self.settings, db=self.db, github=self.github, + sandbox=self.sandbox, git_transport=self.git_transport, + payload=row.payload, delivery_id=row.delivery_id, + attempts=row.attempts, slot_uid=slot_uid, + ) +``` + +Idempotency: `record_event` dedups on delivery id and `_inflight` serializes per +`(owner,repo,number)`. Add one guard in `tasks.review_pr`: if the PR already carries a +`triaged`/`review:*` label, skip the re-review (a `reopened` shouldn't redo work) unless a +directive forces it. + +--- + +## 4. Task dispatcher (`src/tasks.py` `review_pr`) + +New entry point — structurally `triage_issue` (fresh worktree) crossed with `handle_review` +(PR context). Key differences: it checks out the **PR head**, and it never opens an issue row +for an originating issue (the PR is the unit). + +```python +async def review_pr(*, settings, db, github, sandbox, git_transport, + payload, delivery_id, attempts=0, slot_uid=None) -> None: + pr_node = payload.get("pull_request") or {} + pr_number = int(pr_node.get("number") or 0) + repo_full = str((payload.get("repository") or {}).get("full_name") or "") + if pr_number <= 0 or not repo_full: + return + repo = await github.get_repo(repo_full) + issue = await github.get_issue(repo_full, pr_number) # PR-as-issue → title/body/labels + pr = await github.get_pull_request(repo_full, pr_number) + + # idempotency: already triaged? bail (see §3) + key = issue_key(repo_full, pr_number) + db.upsert_issue(key=key, repo=repo_full, number=pr_number, state="reviewing", pr_number=pr_number) + + workspace = sandbox.ensure_workspace( + repo=repo.full_name, number=pr_number, title=issue.title, + clone_url=repo.clone_url, default_branch=repo.default_branch, + pr_head=pr_number, # ← NEW: check out the PR head (see §6) + author_name=settings.resolved_author_name, author_email=settings.git_author_email, + slot_uid=slot_uid, + ) + db.upsert_issue(key=key, repo=repo_full, number=pr_number, state="reviewing", + branch=workspace.branch, session_dir=str(workspace.session_dir), pr_number=pr_number) + + inputs = TaskInputs(settings=settings, db=db, github=github, git_transport=git_transport, + repo=repo, issue=issue, workspace=workspace, delivery_id=delivery_id, + attempts=attempts, slot_uid=slot_uid, natives_cache=sandbox.natives_cache) + await run_task(task_kind="review_pr", inputs=inputs, pr_number=pr_number) +``` + +`run_task(..., pr_number=pr_number)` makes `ToolBindings.inbound_is_pr=True` and points the +comment tools at the PR thread (existing behavior). Add `review_pr` to `tasks.__all__`. + +--- + +## 5. Prompt + persona (`src/persona.py`, `src/prompts/`) + +- **Rename** `src/prompts/pr_review_rubric.md` → `src/prompts/kickoff_pr_review.md` (it's the + full kickoff now, not just a rubric). +- Add the loader, mirroring `kickoff`: + + ```python + def kickoff_pr_review(*, repo: RepoInfo, pr: PullRequestInfo, workspace: Workspace) -> str: + return render(_load("kickoff_pr_review.md"), {"repo": repo, "pr": pr, "workspace": workspace}) + ``` + + The template references `{{repo.*}}`, `{{pr.number|author|head_ref|base_ref|head_repo|html_url}}`, + `{{workspace.branch}}`. Title/body/diff come from the `fetch_pr` tool, not template vars + (`PullRequestInfo` has no title/body). `_lookup` returns `""` for any missing field — safe. +- `_build_prompt` (`worker.py`): add a `task_kind == "review_pr"` branch calling + `persona.kickoff_pr_review(repo=inputs.repo, pr=, workspace=inputs.workspace)`. The `pr` + object must reach `_build_prompt` — simplest is to add an optional `pr: PullRequestInfo | None` + param to `run_task`/`_build_prompt` (parallel to `comment`/`review_payload`), or rebuild it + from `inputs.issue` (number/author) + a `get_pull_request` call inside the branch. +- **`todo_phases.toml`**: add a `review_pr` table (Phase 0 orient / Phase 1 classify / Phase 2 + review) so `seed_phases("review_pr")` seeds the todo list, like `triage_issue`. +- **`host_tools.toml`**: add descriptions for the four new tools (see §7). + +--- + +## 6. Sandbox: check out the PR head (`src/sandbox.py`) + +This is the load-bearing isolation change. Reuse the entire worktree machinery; only the +**checkout source** differs. The PR head may live on a fork, so it is fetched via +`refs/pull//head` on the base repo's remote (not a branch on origin). + +- **`GitTransport` protocol** — add: + ```python + def fetch_pr_head(self, *, repo: str, pool_dir: Path, pr_number: int) -> None: ... + ``` + `LocalGitTransport`: `git fetch origin pull//head` (PAT injected per-call, as + `fetch_base_ref` does). `ProxyGitTransport`: add the matching gh-proxy git op (mirror its + `fetch_base_ref` path over the HMAC channel + a proxy-server handler). +- **`ensure_workspace`** — add `pr_head: int | None = None`. When set, in the `not repo_exists` + branch: + ```python + self.transport.fetch_pr_head(repo=repo, pool_dir=pool, pr_number=pr_head) + _run(["git", "worktree", "add", "--detach", str(repo_dir), "FETCH_HEAD"], cwd=pool) + ``` + Detached HEAD (never a pushable branch — review is read-only). Set + `workspace.branch = f"review/pr-{pr_head}"` for bookkeeping/logging only. Everything after + (slot chown, `_share_git_metadata_with_slots`, `_provision_runtime_dirs`, natives-cache + hardlink, identity config) runs unchanged, so the review worktree gets the **same isolation + and the warm native cache** as a fix worktree (`bun check`/lsp stay fast). + +Result: the agent's cwd is the PR head, fully isolated, read-only. No credentialed push remote +is configured for review worktrees. + +--- + +## 7. Host tools (`src/host_tools.py`) + +Add a `review_mode: bool = False` field to `ToolBindings`; set it from `run_task` +(`review_mode = task_kind == "review_pr"`). Self-gating pattern (consistent with how +`classify_issue` gates on `inbound_is_pr`): + +- New review tools require `review_mode` → reject otherwise. +- `gh_push_branch` / `gh_open_pr` **refuse** when `review_mode` (read-only review; never push to + a contributor's branch). + +Four new tools, registered unconditionally in `build()`: + +| Tool | Params | Behavior | Audit | +|---|---|---|---| +| `fetch_pr` | — (defaults to inbound PR) | `get_pull_request` + `list_pr_files`; returns title, body, `Fixes #N` links, changed-file list (path/status/+−). The premise read. | yes | +| `classify_pr` | `rank`(req `review:p0..p3`), `type`(one of `_PR_TYPES`), `area[]`(⊆ `_FUNCTIONAL`), `provider?`, `rationale` | Validate (drop unknowns silently, like `classify_issue`); `github.add_issue_labels(repo, pr.number, ["triaged", rank, type, *area, provider?])` (issues-labels API works on PRs); persist rank in the issue row. | yes | +| `pr_review_comment` | `path`(req), `line`(req int), `body`(req), `side`="RIGHT", `start_line?`, `start_side?` | **Stage only** — append to sqlite (§9). Validate path/line/body. Return staged count. No GitHub call. | yes | +| `submit_pr_review` | `body`(req), `event`="COMMENT" (forced) | Read staged rows → `github.submit_pr_review(repo, pr.number, body, "COMMENT", comments)` → `clear_staged_review_comments` on success. | yes | + +New allowlists next to the existing ones: +```python +_PR_RANKS = ("review:p0", "review:p1", "review:p2", "review:p3") +_PR_TYPES = ("feat", "fix", "docs", "refactor", "perf", "test", "chore", "ci", "build") +# area reuses _FUNCTIONAL; provider: + _PLATFORMS as for classify_issue +``` + +`classify_pr` mirrors `_build_classify_issue` (validation + label apply + persist + audit). +`submit_pr_review` clears the buffer only after a 2xx so a failed post is retryable. + +--- + +## 8. Backend (`github_backend.py` + `github_client.py` + `proxy_client.py` + `proxy/server.py`) + +- `PullRequestInfo`: add `title: str = ""`, `body: str = ""`. Populate in `_pr_from_payload` + (REST `/pulls/{n}` carries both) and proxy `_pr_from`. +- `GitHubBackend` protocol + both impls: + - `list_pr_files(repo, pr_number) -> list[PullRequestFileInfo]` → `GET /pulls/{n}/files` + (new small frozen dataclass: `path`, `status`, `additions`, `deletions`). + - `submit_pr_review(*, repo, pr_number, body, event, comments) -> PullRequestReviewInfo` + → `POST /pulls/{n}/reviews` with `comments=[{path, line, side, body, start_line?, start_side?}]`. +- gh-proxy mode (`proxy_client.py` + `src/proxy/server.py`): add `/gh/v1/pr_files` (GET) and + `/gh/v1/submit_pr_review` (POST) endpoints + client wrappers. HMAC signing is generic — no + protocol change. Validate inputs server-side with the existing `_require_*` helpers. + +--- + +## 9. DB (`src/db.py`) + +One staging table (schema block near `events`/`issues`/`tool_calls`): + +```sql +CREATE TABLE IF NOT EXISTS pr_review_comments ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + issue_key TEXT NOT NULL, -- repo__owner# + path TEXT NOT NULL, + line INTEGER NOT NULL, + side TEXT NOT NULL DEFAULT 'RIGHT', + start_line INTEGER, + start_side TEXT, + body TEXT NOT NULL, + created_at TEXT NOT NULL +); +CREATE INDEX IF NOT EXISTS idx_pr_review_comments_key ON pr_review_comments(issue_key); +``` + +DAOs (thread-safe via the existing `_lock`): `stage_review_comment(...)`, +`list_staged_review_comments(issue_key) -> list[...]`, `clear_staged_review_comments(issue_key)`. +Rank persistence reuses the `issues` row (`classification`/a new `pr_rank` column) keyed by the +PR's `issue_key`. + +--- + +## 10. Worker completion gate (`src/worker.py`) + +- `_needs_completion_reminder`: extend so `task_kind == "review_pr"` reminds until + `submit_pr_review` is in `tools_called` (the terminal action), mirroring the + `_TERMINAL_TRIAGE_TOOLS` logic. Add a `_TERMINAL_REVIEW_TOOLS = {"submit_pr_review", "abort_task"}`. + Review worktrees are read-only, so the dirty-state reminder is irrelevant — skip it for + `review_pr` (or it'll be clean anyway). +- `run_task`: thread `review_mode`/`pr` through to `ToolBindings`/`_build_prompt` (§5, §7). + +--- + +## 11. Config (`src/config.py`) — optional + +Add `pr_review_enabled: bool = True` (`ROBOMP_PR_REVIEW_ENABLED`) so the whole flow can be +killed without a redeploy; check it in `route()`'s new branch. Reuse the existing +`repo_allowlist`, `maintainers`, `reviewer_bots`. No new auth. + +--- + +## 12. Routing truth table + +| Event | Condition | Result | +|---|---|---| +| `pull_request.opened` / `reopened` / `ready_for_review` | non-draft, author ≠ bot, allowlisted, enabled | **`review_pr`** | +| `pull_request.opened` | draft / bot-authored | skip | +| `pull_request.synchronize` (new commits) | — | **skip** (no re-review) | +| `pull_request.edited` / others | — | skip | +| `issue_comment.created` on incoming PR | not a directive | **skip** | +| `issue_comment.created` on incoming PR | maintainer `@bot` / reviewer bot | directive path (may re-review) | +| `issue_comment.created` on bot PR | — | `handle_pr_conversation` (unchanged) | +| `pull_request.closed` | has review workspace | `cleanup_workspace` | + +--- + +## 13. Test plan (`tests/`, `pytest`, `httpx.MockTransport`) + +Mirror existing test style; assert observable contracts, never internals. + +- **Routing** (`test_github_events.py`): `pull_request.opened` → `review_pr`; draft/bot/non-allowlist + → skip; `synchronize` → skip; incoming-PR comment → skip unless directive. +- **classify_pr** (`test_host_tools.py`): happy path applies `triaged`+`review:pN`+type+area + (assert the labels in the mocked `add_issue_labels` call); bad rank → validation error; unknown + area dropped silently. +- **Staging + submit**: `pr_review_comment` writes rows (assert via DB); `submit_pr_review` posts + one review with all staged comments + `event="COMMENT"` (assert the mocked POST body) and clears + the buffer; second submit with empty buffer posts summary-only / no-ops. +- **review_mode gating**: `gh_push_branch`/`gh_open_pr` refuse under `review_mode`; review tools + refuse outside it. +- **Sandbox** (`test_sandbox.py`, real local bare repo as upstream): `pr_head` checkout yields a + detached worktree at the PR head commit; no push remote configured. +- **Completion gate**: a `review_pr` turn ending before `submit_pr_review` triggers the reminder. + +Do **not** enable the integration smoke (`ROBOMP_INTEGRATION=1`) in the default suite. + +--- + +## 14. Open questions for @can1357 + +1. **Rank label namespace**: `review:p0..p3` (proposed, avoids colliding with issue `prio:p0..p3`) + — or reuse `prio:`? These must exist (or be auto-creatable) as repo labels. +2. **`type` labels**: introduce `feat`/`fix`/`docs`/… as bare labels, or namespace `type:feat`? + The repo's current label set should be checked before `classify_pr` writes them. +3. **Re-review trigger phrasing**: which directive text re-runs Phase 2 vs. just answers a + question? (Routed through the existing directive path.) +4. **Cleanup**: GC incoming-PR review worktrees on `pull_request.closed` (any author), or a TTL + sweep? (§2c.) diff --git a/python/robomp/src/config.py b/python/robomp/src/config.py index bec0815d5..eb7179a4b 100644 --- a/python/robomp/src/config.py +++ b/python/robomp/src/config.py @@ -37,6 +37,7 @@ class Settings(BaseSettings): git_author_name: str | None = Field(None, alias="ROBOMP_GIT_AUTHOR_NAME") git_author_email: str = Field(..., alias="ROBOMP_GIT_AUTHOR_EMAIL") repo_allowlist_raw: str = Field("", alias="ROBOMP_REPO_ALLOWLIST") + pr_review_enabled: bool = Field(True, alias="ROBOMP_PR_REVIEW_ENABLED") # gh-proxy. Set BOTH to route GitHub through the proxy; leave both empty # to keep PAT-on-orchestrator behavior. Mixing the two (PAT + proxy) is diff --git a/python/robomp/src/db.py b/python/robomp/src/db.py index a3d27aee7..0e5762ba9 100644 --- a/python/robomp/src/db.py +++ b/python/robomp/src/db.py @@ -19,6 +19,7 @@ IssueState = Literal[ "new", "reproducing", "fixing", + "reviewing", "opened", "merged", "closed", @@ -72,6 +73,20 @@ CREATE TABLE IF NOT EXISTS tool_calls ( ); CREATE INDEX IF NOT EXISTS tool_calls_issue ON tool_calls(issue_key, ts); +CREATE TABLE IF NOT EXISTS pr_review_comments ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + issue_key TEXT NOT NULL, + path TEXT NOT NULL, + line INTEGER NOT NULL, + side TEXT NOT NULL DEFAULT 'RIGHT', + start_line INTEGER, + start_side TEXT, + body TEXT NOT NULL, + created_at TEXT NOT NULL +); +CREATE INDEX IF NOT EXISTS idx_pr_review_comments_key + ON pr_review_comments(issue_key); + CREATE TABLE IF NOT EXISTS submissions ( delivery_id TEXT PRIMARY KEY, login TEXT NOT NULL, @@ -132,6 +147,19 @@ class IssueRow: classification: str | None = None +@dataclass(slots=True, frozen=True) +class StagedReviewComment: + id: int + issue_key: str + path: str + line: int + side: str + body: str + created_at: str + start_line: int | None = None + start_side: str | None = None + + def _event_row_from_db_row(row: sqlite3.Row) -> EventRow: return EventRow( delivery_id=row["delivery_id"], @@ -778,6 +806,93 @@ class Database: ) return int(cur.lastrowid or 0) + def has_successful_tool_call(self, issue_key: str, tool: str) -> bool: + with self._lock: + row = self._conn.execute( + """ + SELECT 1 + FROM tool_calls + WHERE issue_key=? AND tool=? AND error IS NULL + ORDER BY id DESC + LIMIT 1 + """, + (issue_key, tool), + ).fetchone() + return row is not None + + # ---- PR review comment staging ---- + def stage_review_comment( + self, + *, + issue_key: str, + path: str, + line: int, + body: str, + side: str = "RIGHT", + start_line: int | None = None, + start_side: str | None = None, + ) -> StagedReviewComment: + with self._lock: + cur = self._conn.execute( + """ + INSERT INTO pr_review_comments + (issue_key, path, line, side, start_line, start_side, body, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + (issue_key, path, line, side, start_line, start_side, body, _utcnow()), + ) + row = self._conn.execute( + """ + SELECT id, issue_key, path, line, side, start_line, start_side, body, created_at + FROM pr_review_comments + WHERE id=? + """, + (int(cur.lastrowid or 0),), + ).fetchone() + assert row is not None + return StagedReviewComment( + id=int(row["id"]), + issue_key=row["issue_key"], + path=row["path"], + line=int(row["line"]), + side=row["side"], + body=row["body"], + created_at=row["created_at"], + start_line=int(row["start_line"]) if row["start_line"] is not None else None, + start_side=row["start_side"], + ) + + def list_staged_review_comments(self, issue_key: str) -> list[StagedReviewComment]: + with self._lock: + rows = self._conn.execute( + """ + SELECT id, issue_key, path, line, side, start_line, start_side, body, created_at + FROM pr_review_comments + WHERE issue_key=? + ORDER BY id + """, + (issue_key,), + ).fetchall() + return [ + StagedReviewComment( + id=int(row["id"]), + issue_key=row["issue_key"], + path=row["path"], + line=int(row["line"]), + side=row["side"], + body=row["body"], + created_at=row["created_at"], + start_line=int(row["start_line"]) if row["start_line"] is not None else None, + start_side=row["start_side"], + ) + for row in rows + ] + + def clear_staged_review_comments(self, issue_key: str) -> int: + with self._lock: + cur = self._conn.execute("DELETE FROM pr_review_comments WHERE issue_key=?", (issue_key,)) + return int(cur.rowcount or 0) + # ---- submissions (per-user rate limiting) ---- def admit_submission( self, diff --git a/python/robomp/src/git_ops.py b/python/robomp/src/git_ops.py index bf6e23480..312319f2d 100644 --- a/python/robomp/src/git_ops.py +++ b/python/robomp/src/git_ops.py @@ -439,6 +439,20 @@ def fetch_ref(repo_dir: Path, ref: str, *, token: str | None, safe_directory: Pa ) +def fetch_pr_head( + repo_dir: Path, + pr_number: int, + *, + token: str | None, + safe_directory: Path | None = None, +) -> None: + """Fetch `refs/pull//head` into FETCH_HEAD for detached PR review worktrees.""" + if pr_number <= 0: + raise ValueError(f"invalid PR number: {pr_number!r}") + args = ["fetch", "origin", f"pull/{pr_number}/head"] + _check(_run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory), ["git", *args]) + + @dataclass(slots=True, frozen=True) class PushResult: head: str @@ -646,6 +660,7 @@ __all__ = [ "HeadDriftError", "PushResult", "clone", + "fetch_pr_head", "fetch_prune", "fetch_ref", "inspect_dirty_state", diff --git a/python/robomp/src/github_backend.py b/python/robomp/src/github_backend.py index e22a92514..b8ced989f 100644 --- a/python/robomp/src/github_backend.py +++ b/python/robomp/src/github_backend.py @@ -8,12 +8,14 @@ dataclasses (`IssueInfo`, `RepoInfo`, …) defined in `github_client`. from __future__ import annotations -from typing import Protocol +from collections.abc import Mapping +from typing import Any, Protocol from robomp.github_client import ( CommentInfo, IssueInfo, IssueSummary, + PullRequestFileInfo, PullRequestInfo, PullRequestReviewInfo, ReactionInfo, @@ -34,6 +36,8 @@ class GitHubBackend(Protocol): async def get_pull_request(self, repo: str, number: int) -> PullRequestInfo: ... + async def list_pr_files(self, repo: str, pr_number: int) -> list[PullRequestFileInfo]: ... + async def list_issues( self, repo: str, @@ -76,6 +80,16 @@ class GitHubBackend(Protocol): async def add_issue_labels(self, repo: str, number: int, labels: list[str]) -> tuple[str, ...]: ... + async def submit_pr_review( + self, + *, + repo: str, + pr_number: int, + body: str, + event: str, + comments: list[Mapping[str, Any]], + ) -> PullRequestReviewInfo: ... + async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None: ... async def list_comment_reactions(self, repo: str, comment_id: int) -> tuple[ReactionInfo, ...]: ... diff --git a/python/robomp/src/github_client.py b/python/robomp/src/github_client.py index 1a4f429b0..48e3002bb 100644 --- a/python/robomp/src/github_client.py +++ b/python/robomp/src/github_client.py @@ -65,6 +65,16 @@ class PullRequestInfo: state: str author: str = "" head_repo: str = "" + title: str = "" + body: str = "" + + +@dataclass(slots=True, frozen=True) +class PullRequestFileInfo: + path: str + status: str + additions: int + deletions: int @dataclass(slots=True, frozen=True) @@ -254,6 +264,21 @@ class GitHubClient: data = await self.request("GET", f"/repos/{repo}/pulls/{number}") return _pr_from_payload(repo, data) + async def list_pr_files(self, repo: str, pr_number: int) -> list[PullRequestFileInfo]: + files: list[PullRequestFileInfo] = [] + page = 1 + while True: + data = await self.request( + "GET", + f"/repos/{repo}/pulls/{pr_number}/files", + params={"per_page": 100, "page": page}, + ) + batch = [_pr_file_from_payload(item) for item in (data or [])] + files.extend(batch) + if len(batch) < 100: + return files + page += 1 + async def list_issues( self, repo: str, @@ -421,6 +446,22 @@ class GitHubClient: ) return tuple(str(lbl["name"]) if isinstance(lbl, dict) else str(lbl) for lbl in (data or [])) + async def submit_pr_review( + self, + *, + repo: str, + pr_number: int, + body: str, + event: str, + comments: list[Mapping[str, Any]], + ) -> PullRequestReviewInfo: + data = await self.request( + "POST", + f"/repos/{repo}/pulls/{pr_number}/reviews", + json={"body": body, "event": event, "comments": comments}, + ) + return _pr_review_from_payload(data) + async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None: if not assignees: return @@ -482,6 +523,27 @@ def _issue_from_payload(repo: str, data: Mapping[str, Any]) -> IssueInfo: ) +def _pr_review_from_payload(data: Mapping[str, Any]) -> PullRequestReviewInfo: + user = data.get("user") or {} + body = str(data.get("body") or "").strip() + return PullRequestReviewInfo( + id=int(data.get("id") or 0), + author=str(user.get("login") or "") if isinstance(user, Mapping) else "", + body=body, + state=str(data.get("state") or ""), + submitted_at=str(data.get("submitted_at") or data.get("created_at") or ""), + ) + + +def _pr_file_from_payload(data: Mapping[str, Any]) -> PullRequestFileInfo: + return PullRequestFileInfo( + path=str(data.get("filename") or data.get("path") or ""), + status=str(data.get("status") or ""), + additions=int(data.get("additions") or 0), + deletions=int(data.get("deletions") or 0), + ) + + def _pr_from_payload(repo: str, data: Mapping[str, Any]) -> PullRequestInfo: head = data.get("head") or {} base = data.get("base") or {} @@ -496,6 +558,8 @@ def _pr_from_payload(repo: str, data: Mapping[str, Any]) -> PullRequestInfo: state=str(data.get("state") or "open"), author=str(user.get("login") or "") if isinstance(user, Mapping) else "", 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 ""), ) @@ -534,6 +598,7 @@ __all__ = [ "GitHubError", "IssueInfo", "IssueSummary", + "PullRequestFileInfo", "PullRequestInfo", "PullRequestReviewInfo", "ReactionInfo", diff --git a/python/robomp/src/github_events.py b/python/robomp/src/github_events.py index e6a185a61..13435908c 100644 --- a/python/robomp/src/github_events.py +++ b/python/robomp/src/github_events.py @@ -134,6 +134,7 @@ def route( maintainers: frozenset[str] = frozenset(), reviewer_bots: frozenset[str] = frozenset(), resolve_issue_from_pr: PrIssueResolver = None, + pr_review_enabled: bool = True, ) -> RouteDecision: """Decide whether and how to handle a webhook event. @@ -225,23 +226,26 @@ def route( if not isinstance(number, int): return RouteDecision("skip", None, repo, None, "comment missing issue number") if "pull_request" in issue: - # Conversation comment on a PR. The PR number lives at issue.number - # on this payload type. Prefer the originating issue key when the - # DB has it, but do not drop bot-authored follow-ups just because - # the PR mapping was lost; the worker can recover from the PR - # branch or handle the PR directly. + # Conversation comments on incoming contributor PRs are intentionally + # ignored for now: the one-shot review runs on open, and re-review + # directives are not wired yet. Only bot-authored PRs resume a live + # amend-and-push workflow. key = _resolve_pr_key(number) login, assoc = _submitter_info(comment) - return RouteDecision( - "queue", - "handle_pr_conversation", - repo, - key, - f"issue_comment.created on PR #{number}", - submitter=login, - association=assoc, - **_directive_kwargs(comment, login, assoc), - ) + issue_user_raw = issue.get("user") + issue_user = issue_user_raw if isinstance(issue_user_raw, Mapping) else {} + if str(issue_user.get("login") or "") == bot_login: + return RouteDecision( + "queue", + "handle_pr_conversation", + repo, + key, + f"issue_comment.created on PR #{number}", + submitter=login, + association=assoc, + **_directive_kwargs(comment, login, assoc), + ) + return RouteDecision("skip", None, repo, issue_key(repo, number), "incoming PR comments ignored") key = issue_key(repo, number) login, assoc = _submitter_info(comment) return RouteDecision( @@ -255,6 +259,29 @@ def route( **_directive_kwargs(comment, login, assoc), ) + if event_type == "pull_request" and action in ("opened", "reopened", "ready_for_review"): + if not pr_review_enabled: + return RouteDecision("skip", None, repo, None, "PR review disabled") + pr = payload.get("pull_request") or {} + if bool(pr.get("draft")): + return RouteDecision("skip", None, repo, None, "draft PR") + pr_user = pr.get("user") or {} + if _is_bot_account(pr_user, bot_login): + return RouteDecision("skip", None, repo, None, "bot-authored PR") + number = pr.get("number") + if not isinstance(number, int): + return RouteDecision("skip", None, repo, None, "PR missing number") + login, assoc = _submitter_info(pr) + return RouteDecision( + "queue", + "review_pr", + repo, + issue_key(repo, number), + f"pull_request.{action}", + submitter=login, + association=assoc, + ) + if event_type == "pull_request_review_comment" and action == "created": comment = payload.get("comment") or {} rb_login = _reviewer_bot_login(comment.get("user")) @@ -282,15 +309,11 @@ def route( if event_type == "pull_request" and action == "closed": pr = payload.get("pull_request") or {} - pr_user = pr.get("user") or {} - if str(pr_user.get("login") or "") != bot_login: - return RouteDecision("skip", None, repo, None, "PR not bot-authored") - if not bool(pr.get("merged")): - return RouteDecision("skip", None, repo, None, "PR closed without merge") number = pr.get("number") if not isinstance(number, int): return RouteDecision("skip", None, repo, None, "PR missing number") - return RouteDecision("queue", "cleanup_workspace", repo, _resolve_pr_key(number), "pull_request.merged") + reason = "pull_request.merged" if bool(pr.get("merged")) else "pull_request.closed" + return RouteDecision("queue", "cleanup_workspace", repo, _resolve_pr_key(number), reason) return RouteDecision("skip", None, repo, None, f"{event_type}.{action} not handled") diff --git a/python/robomp/src/host_tools.py b/python/robomp/src/host_tools.py index aeee25c76..713518c3f 100644 --- a/python/robomp/src/host_tools.py +++ b/python/robomp/src/host_tools.py @@ -10,6 +10,7 @@ import asyncio import json import logging import os +import re import subprocess import time from collections.abc import Callable, Mapping @@ -25,7 +26,7 @@ from robomp.config import Settings from robomp.db import Database, issue_key from robomp.git_ops import GitCommandError, HeadDriftError from robomp.github_backend import GitHubBackend -from robomp.github_client import GitHubError, IssueInfo, RepoInfo +from robomp.github_client import GitHubError, IssueInfo, PullRequestFileInfo, RepoInfo from robomp.sandbox import ( GitTransport, Workspace, @@ -109,6 +110,9 @@ class ToolBindings: # — the originating issue has already been classified and the PR # itself does not carry triage labels. inbound_is_pr: bool = False + # True only for incoming-PR review tasks. Review tools require it; mutating + # branch/PR publication tools reject when it is set. + review_mode: bool = False slot_uid: int | None = None # Set by the worker before launching omp. Carries the abort-task signal # back out to the worker; `None` for unit tests that exercise tools @@ -514,6 +518,10 @@ def _build_post_comment(bindings: ToolBindings) -> HostTool[Any, Any]: def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_name: str, branch: str) -> str: + if bindings.review_mode: + msg = "refusing to push: PR review worktrees are read-only." + _audit(bindings, tool_name, args, error=msg) + _raise_command(msg) if branch != bindings.workspace.branch: _raise_command( f"refusing to push: branch={branch!r} does not match workspace branch {bindings.workspace.branch!r}." @@ -611,6 +619,10 @@ def _guarded_push_branch(bindings: ToolBindings, args: Mapping[str, Any], tool_n # ---------- gh_push_branch ---------- def _build_push_branch(bindings: ToolBindings) -> HostTool[Any, Any]: def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str: + if bindings.review_mode: + msg = "refusing to push: PR review worktrees are read-only." + _audit(bindings, "gh_push_branch", args, error=msg) + _raise_command(msg) branch = str(args.get("branch") or bindings.workspace.branch) skip = bool(args.get("skip_checks", False)) # Same gate as gh_open_pr — formatter + check before bytes leave the @@ -648,6 +660,10 @@ def _build_push_branch(bindings: ToolBindings) -> HostTool[Any, Any]: # ---------- gh_open_pr ---------- def _build_open_pr(bindings: ToolBindings) -> HostTool[Any, Any]: def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str: + if bindings.review_mode: + msg = "refusing to open PR: PR review tasks are read-only." + _audit(bindings, "gh_open_pr", args, error=msg) + _raise_command(msg) title = args.get("title") body = args.get("body") if not isinstance(title, str) or not title.strip(): @@ -969,6 +985,301 @@ _PRIMARY_TYPES = ("bug", "enhancement", "question", "proposal", "documentation", _PRIORITIES = ("prio:p0", "prio:p1", "prio:p2", "prio:p3") _FUNCTIONAL = ("agent", "tool", "tui", "cli", "prompting", "sdk", "auth", "setup", "ux", "providers") _PLATFORMS = ("platform:linux", "platform:macos", "platform:windows", "platform:wsl") +_PR_RANKS = ("review:p0", "review:p1", "review:p2", "review:p3") +_PR_TYPES = ("feat", "fix", "docs", "refactor", "perf", "test", "chore", "ci", "build") +_CLOSING_ISSUE_RE = re.compile(r"\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+#(\d+)", re.IGNORECASE) + + +def _require_review_mode(bindings: ToolBindings, name: str, args: Mapping[str, Any]) -> None: + if bindings.review_mode: + return + msg = f"{name} is only available during incoming PR review tasks." + _audit(bindings, name, args, error=msg) + _raise_command(msg) + + +def _format_pr_file(file: PullRequestFileInfo) -> str: + return f"- `{file.path}` ({file.status}, +{file.additions}/-{file.deletions})" + + +def _build_fetch_pr(bindings: ToolBindings) -> HostTool[Any, Any]: + def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str: + _require_review_mode(bindings, "fetch_pr", args) + pr_number = bindings.default_comment_number + try: + pr = _run_coro(bindings.loop, bindings.github.get_pull_request(bindings.repo.full_name, pr_number)) + files = _run_coro(bindings.loop, bindings.github.list_pr_files(bindings.repo.full_name, pr_number)) + except GitHubError as exc: + _audit(bindings, "fetch_pr", args, error=str(exc)) + _raise_command(f"GitHub fetch failed: {exc.status} {exc.message}") + linked = tuple(sorted({int(match.group(1)) for match in _CLOSING_ISSUE_RE.finditer(pr.body)})) + lines = [ + f"# {pr.repo}#{pr.number} ({pr.state})", + f"title: {pr.title or '(untitled)'}", + f"author: @{pr.author}", + f"head: {pr.head_repo or pr.repo}:{pr.head_ref}", + f"base: {pr.base_ref}", + f"url: {pr.html_url}", + "", + "## Body", + pr.body.strip() or "(empty)", + "", + "## Linked issues", + ", ".join(f"#{n}" for n in linked) if linked else "(none found in PR body)", + "", + f"## Changed files ({len(files)})", + ] + lines.extend(_format_pr_file(file) for file in files) + rendered = "\n".join(lines) + _audit(bindings, "fetch_pr", args, result={"files": len(files), "linked_issues": list(linked)}) + return rendered + + return host_tool( + name="fetch_pr", + description=persona.host_tool_description("fetch_pr"), + parameters={"type": "object", "properties": {}, "additionalProperties": False}, + execute=execute, + ) + + +def _build_classify_pr(bindings: ToolBindings) -> HostTool[Any, Any]: + def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str: + _require_review_mode(bindings, "classify_pr", args) + rank = args.get("rank") + if rank not in _PR_RANKS: + msg = f"classify_pr 'rank' must be one of {_PR_RANKS}; got {rank!r}." + _audit(bindings, "classify_pr", args, error=msg) + _raise_command(msg) + pr_type = args.get("type") + if pr_type not in _PR_TYPES: + msg = f"classify_pr 'type' must be one of {_PR_TYPES}; got {pr_type!r}." + _audit(bindings, "classify_pr", args, error=msg) + _raise_command(msg) + rationale = args.get("rationale") + if not isinstance(rationale, str) or not rationale.strip(): + msg = "classify_pr requires a one-sentence 'rationale'." + _audit(bindings, "classify_pr", args, error=msg) + _raise_command(msg) + + labels: list[str] = ["triaged", str(rank), str(pr_type)] + for area in args.get("area") or (): + if isinstance(area, str) and area in _FUNCTIONAL: + labels.append(area) + provider = args.get("provider") + if isinstance(provider, str) and provider.strip() and provider.startswith("provider:"): + labels.append("providers") + labels.append(provider) + try: + applied = _run_coro( + bindings.loop, + bindings.github.add_issue_labels(bindings.repo.full_name, bindings.default_comment_number, labels), + ) + except GitHubError as exc: + _audit(bindings, "classify_pr", args, error=str(exc)) + _raise_command(f"GitHub rejected labels: {exc.status} {exc.message}") + bindings.db.set_issue_classification(bindings.issue_key, str(rank)) + _audit( + bindings, + "classify_pr", + args, + result={"rank": rank, "type": pr_type, "labels": list(applied), "rationale": rationale}, + ) + return f"classified PR as {rank}; labels applied: {', '.join(applied)}." + + return host_tool( + name="classify_pr", + description=persona.host_tool_description("classify_pr"), + parameters={ + "type": "object", + "properties": { + "rank": { + "type": "string", + "enum": list(_PR_RANKS), + "description": persona.host_tool_parameter_description("classify_pr", "rank"), + }, + "type": { + "type": "string", + "enum": list(_PR_TYPES), + "description": persona.host_tool_parameter_description("classify_pr", "type"), + }, + "area": { + "type": "array", + "items": {"type": "string", "enum": list(_FUNCTIONAL)}, + "description": persona.host_tool_parameter_description("classify_pr", "area"), + }, + "provider": { + "type": "string", + "description": persona.host_tool_parameter_description("classify_pr", "provider"), + }, + "rationale": { + "type": "string", + "description": persona.host_tool_parameter_description("classify_pr", "rationale"), + }, + }, + "required": ["rank", "type", "rationale"], + "additionalProperties": False, + }, + execute=execute, + ) + + +def _review_comment_to_payload(comment: Any) -> dict[str, Any]: + payload: dict[str, Any] = { + "path": comment.path, + "line": comment.line, + "side": comment.side, + "body": comment.body, + } + if comment.start_line is not None: + payload["start_line"] = comment.start_line + if comment.start_side is not None: + payload["start_side"] = comment.start_side + return payload + + +def _build_pr_review_comment(bindings: ToolBindings) -> HostTool[Any, Any]: + def execute(args: dict[str, Any], _ctx: HostToolContext[Any]) -> str: + _require_review_mode(bindings, "pr_review_comment", args) + path = args.get("path") + line = args.get("line") + body = args.get("body") + if not isinstance(path, str) or not path.strip(): + msg = "pr_review_comment requires a non-empty 'path'." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + if not isinstance(line, int) or line <= 0: + msg = "pr_review_comment requires a positive integer 'line'." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + if not isinstance(body, str) or not body.strip(): + msg = "pr_review_comment requires a non-empty 'body'." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + side = str(args.get("side") or "RIGHT") + if side not in ("RIGHT", "LEFT"): + msg = "pr_review_comment 'side' must be RIGHT or LEFT." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + start_line = args.get("start_line") + if start_line is not None and (not isinstance(start_line, int) or start_line <= 0): + msg = "pr_review_comment 'start_line' must be a positive integer when provided." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + start_side_raw = args.get("start_side") + start_side = str(start_side_raw) if start_side_raw is not None else None + if start_side is not None and start_side not in ("RIGHT", "LEFT"): + msg = "pr_review_comment 'start_side' must be RIGHT or LEFT when provided." + _audit(bindings, "pr_review_comment", args, error=msg) + _raise_command(msg) + staged = bindings.db.stage_review_comment( + issue_key=bindings.issue_key, + path=path.strip(), + line=line, + side=side, + start_line=start_line, + start_side=start_side, + body=body.strip(), + ) + count = len(bindings.db.list_staged_review_comments(bindings.issue_key)) + _audit(bindings, "pr_review_comment", args, result={"id": staged.id, "staged": count}) + return f"staged review comment #{staged.id}; staged_count={count}" + + return host_tool( + name="pr_review_comment", + description=persona.host_tool_description("pr_review_comment"), + parameters={ + "type": "object", + "properties": { + "path": { + "type": "string", + "description": persona.host_tool_parameter_description("pr_review_comment", "path"), + }, + "line": { + "type": "integer", + "description": persona.host_tool_parameter_description("pr_review_comment", "line"), + }, + "body": { + "type": "string", + "description": persona.host_tool_parameter_description("pr_review_comment", "body"), + }, + "side": { + "type": "string", + "enum": ["RIGHT", "LEFT"], + "default": "RIGHT", + "description": persona.host_tool_parameter_description("pr_review_comment", "side"), + }, + "start_line": { + "type": "integer", + "description": persona.host_tool_parameter_description("pr_review_comment", "start_line"), + }, + "start_side": { + "type": "string", + "enum": ["RIGHT", "LEFT"], + "description": persona.host_tool_parameter_description("pr_review_comment", "start_side"), + }, + }, + "required": ["path", "line", "body"], + "additionalProperties": False, + }, + execute=execute, + ) + + +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) + body = args.get("body") + if not isinstance(body, str) or not body.strip(): + msg = "submit_pr_review requires a non-empty 'body'." + _audit(bindings, "submit_pr_review", args, error=msg) + _raise_command(msg) + staged = bindings.db.list_staged_review_comments(bindings.issue_key) + comments = [_review_comment_to_payload(comment) for comment in staged] + 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(), + event="COMMENT", + comments=comments, + ), + ) + except GitHubError as exc: + _audit(bindings, "submit_pr_review", args, error=str(exc)) + _raise_command(f"GitHub rejected PR review: {exc.status} {exc.message}") + 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"}, + ) + return f"submitted PR review id={review.id}; comments={len(comments)}" + + return host_tool( + name="submit_pr_review", + description=persona.host_tool_description("submit_pr_review"), + parameters={ + "type": "object", + "properties": { + "body": { + "type": "string", + "description": persona.host_tool_parameter_description("submit_pr_review", "body"), + }, + "event": { + "type": "string", + "enum": ["COMMENT"], + "default": "COMMENT", + "description": persona.host_tool_parameter_description("submit_pr_review", "event"), + }, + }, + "required": ["body"], + "additionalProperties": False, + }, + execute=execute, + ) def _build_set_issue_labels(bindings: ToolBindings) -> HostTool[Any, Any]: @@ -1205,6 +1516,10 @@ def build(bindings: ToolBindings) -> tuple[HostTool[Any, Any], ...]: return ( _build_classify_issue(bindings), _build_set_issue_labels(bindings), + _build_fetch_pr(bindings), + _build_classify_pr(bindings), + _build_pr_review_comment(bindings), + _build_submit_pr_review(bindings), _build_post_comment(bindings), _build_push_branch(bindings), _build_open_pr(bindings), diff --git a/python/robomp/src/persona.py b/python/robomp/src/persona.py index 074a949c8..7119857ef 100644 --- a/python/robomp/src/persona.py +++ b/python/robomp/src/persona.py @@ -16,7 +16,7 @@ from importlib import resources from typing import Any from robomp.git_ops import DirtyState -from robomp.github_client import CommentInfo, IssueInfo, RepoInfo +from robomp.github_client import CommentInfo, IssueInfo, PullRequestInfo, RepoInfo from robomp.sandbox import Workspace _PLACEHOLDER = re.compile(r"\{\{\s*([a-zA-Z0-9_.]+)\s*\}\}") @@ -132,10 +132,18 @@ def system_append(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspace) -> return render(_load("system_append.md"), {"repo": repo, "issue": issue, "workspace": workspace}) +def system_append_pr_review(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspace) -> str: + return render(_load("system_append_pr_review.md"), {"repo": repo, "issue": issue, "workspace": workspace}) + + def kickoff(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspace) -> str: return render(_load("kickoff_issue.md"), {"repo": repo, "issue": issue, "workspace": workspace}) +def kickoff_pr_review(*, repo: RepoInfo, pr: PullRequestInfo, workspace: Workspace) -> str: + return render(_load("kickoff_pr_review.md"), {"repo": repo, "pr": pr, "workspace": workspace}) + + def resume_triage(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspace) -> str: """Resume prompt for a `triage_issue` task whose omp session already exists.""" return render(_load("resume_triage.md"), {"repo": repo, "issue": issue, "workspace": workspace}) @@ -146,6 +154,11 @@ def completion_reminder(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspac return render(_load("completion_reminder.md"), {"repo": repo, "issue": issue, "workspace": workspace}) +def review_completion_reminder(*, repo: RepoInfo, issue: IssueInfo, workspace: Workspace) -> str: + """Reminder injected when a PR review turn ends before submission.""" + return render(_load("review_completion_reminder.md"), {"repo": repo, "issue": issue, "workspace": workspace}) + + def dirty_state_reminder( *, repo: RepoInfo, @@ -376,8 +389,11 @@ __all__ = [ "host_tool_parameter_description", "kickoff", "kickoff_directive", + "kickoff_pr_review", "render", "completion_reminder", + "review_completion_reminder", + "system_append_pr_review", "dirty_state_reminder", "resume_triage", "seed_phases", diff --git a/python/robomp/src/prompts/host_tools.toml b/python/robomp/src/prompts/host_tools.toml index 77b4af592..54b5203a7 100644 --- a/python/robomp/src/prompts/host_tools.toml +++ b/python/robomp/src/prompts/host_tools.toml @@ -1,3 +1,34 @@ +[fetch_pr] +description = "Fetch the inbound PR premise and changed-file list for review. Review-mode only." + +[classify_pr] +description = "Classify and rank an incoming PR, apply triaged/review labels, and persist the rank. Review-mode only." + +[classify_pr.parameters] +rank = "Required review verdict label: one of `review:p0..p3`." +type = "Exactly one PR type: feat, fix, docs, refactor, perf, test, chore, ci, or build." +area = "Zero or more functional labels. Unknown values are dropped silently; omit when none apply." +provider = "Only when provider-scoped; format `provider:`. Omit otherwise." +rationale = "One sentence explaining what the PR changes and why it earns this rank." + +[pr_review_comment] +description = "Stage one inline PR review comment in sqlite. Does not call GitHub until submit_pr_review. Review-mode only." + +[pr_review_comment.parameters] +path = "Changed file path to comment on." +line = "Line number in the PR diff." +body = "Markdown body for one concrete finding." +side = "`RIGHT` for added/changed lines, `LEFT` for removed lines. Defaults to `RIGHT`." +start_line = "Optional first line for a multi-line comment range." +start_side = "Optional side for start_line; `RIGHT` or `LEFT`." + +[submit_pr_review] +description = "Submit one GitHub PR review with all staged inline comments. Always posts event=`COMMENT`; never approves or requests changes. Review-mode only." + +[submit_pr_review.parameters] +body = "Markdown review summary. Required even when there are no inline comments." +event = "Ignored except for schema compatibility; the orchestrator always sends `COMMENT`." + [gh_post_comment] description = "Post a comment on the inbound thread (PR for PR conversations/reviews, originating issue otherwise). Pass `number` ONLY to post elsewhere." diff --git a/python/robomp/src/prompts/kickoff_pr_review.md b/python/robomp/src/prompts/kickoff_pr_review.md new file mode 100644 index 000000000..3d965cdc5 --- /dev/null +++ b/python/robomp/src/prompts/kickoff_pr_review.md @@ -0,0 +1,136 @@ +# Reviewing pull request {{repo.full_name}}#{{pr.number}} + +**Author:** @{{pr.author}} +**Head:** `{{pr.head_ref}}` from `{{pr.head_repo}}` → **Base:** `{{pr.base_ref}}` +**PR:** {{pr.html_url}} + +The PR's head is checked out in the worktree at cwd. This is a **read-only review**: +you classify, rank, and comment. You NEVER merge, close, approve, push, or edit the +PR's code. The maintainer decides what happens to the PR — your job is to make that +decision a one-glance call. + +Run two phases in order. Phase 1 is cheap and always happens; Phase 2 is the real review. + + +- **Read-only.** No `gh_push_branch`, no `gh_open_pr`, no commits, no `git push`. The only + side effects are `classify_pr`, `pr_review_comment`, `submit_pr_review`, and (if a + maintainer must decide something) one `gh_post_comment`. +- **Phase 1 before Phase 2.** `classify_pr` is the first side effect. Rank and tag before + you write a single inline comment. +- **One review, batched.** Stage every inline finding with `pr_review_comment`, then flush + them all in ONE `submit_pr_review`. NEVER post inline findings as standalone comments. +- **Evidence first.** Cite file + line + symbol. "This looks risky" is not a review; + "`foo()` at `x.ts:42` dereferences `cfg` before the null guard on line 40" is. +- **Stay in scope.** Review THIS diff. Do not demand unrelated refactors, re-architecture, + or features the PR never claimed to deliver. + + +# Phase 0 — orient + +1. **Read the premise.** Call `fetch_pr` for the title, body, and any linked issue + (`Fixes #N`). Understand what the PR *claims* to do before judging whether it does it. +2. **Read the diff.** Prefer `git diff origin/{{pr.base_ref}}...HEAD` for the full changed-file set. If + `origin/{{pr.base_ref}}` is not present locally, fall back to `fetch_pr`'s file list plus + targeted `read`/`search` on the changed files. Note size, number of files, and whether the + changes are coherent or a grab-bag. +3. **Check it isn't already done.** Skim `git log origin/{{repo.default_branch}}` and open + PRs for the same fix. Already landed or superseded → still review, but it ranks **P3** + and your summary says so with a pointer to the commit/PR. + +# Phase 1 — classify & rank + +Call **`classify_pr`** exactly once. It applies the `triaged` tag plus the labels below. + +## Rank — one of `review:p0` … `review:p3` + +Rank by **value × scope discipline × maintainer confidence**, weighted heavily by how +closely the PR follows repo conventions (see Conventions). Higher convention adherence +and tighter scope rank up; sprawl and sloppiness rank down. + +- **P0** — lgtm / must-fix / a truly incremental, nicely scoped change. Correct, follows + conventions, nothing blocking. The maintainer can merge on a glance. + *(e.g. a small root-cause bug fix with a regression test.)* +- **P1** — mergeable after a touch. Minor nits, or an architectural concern worth raising + before it merges. + *(e.g. the fix is right but ships a verbose hardcoded list, or a cleaner placement exists.)* +- **P2** — needs an explicit maintainer call. A feature addition, or anything that changes + default behaviour without fixing a break. Don't treat "small" as "safe". + *(e.g. flips a default, adds a setting, or changes an existing contract.)* +- **P3** — deprioritize. Badly scoped (grab-bag of unrelated edits), carries irrelevant + changes, a large implementation with no confirmed maintainer intent, broken/off-spec, + or already resolved/superseded. + *(e.g. a 200-file PR standing up a mechanism the repo already has.)* + +## Categories + +- **type** — exactly one: `feat` `fix` `docs` `refactor` `perf` `test` `chore` `ci` `build`. +- **area** — zero or more, reusing the issue taxonomy: `agent` `tool` `tui` `cli` + `prompting` `sdk` `auth` `setup` `ux` `providers`. +- **provider** — only when provider-scoped: `provider:` (adds `providers`). Never + speculative. +- **rationale** — one sentence: what the PR does and why it earns its rank. + +# Phase 2 — review the diff + +Read the changed files in detail — not just the diff hunks, the surrounding code they +touch. Review with the lens of someone who will own this code: + +- **Correctness** — does it do what the premise claims? Off-by-one, wrong branch, inverted + condition, mishandled async, swallowed errors. +- **Introduced bugs / regressions** — does the change break a path that worked? Null/empty + conflated with error? Resource left open? Concurrency or shared-mutable-state hazard + (a global singleton mutated across sessions is a hard blocker)? +- **Security / safety** — injection, unsanitized input, credential leakage, sandbox escape, + unbounded execution. +- **Breaking changes** — changed defaults, renamed/removed public API, altered output that + something downstream parses. +- **Test coverage** — does every new branch have a test that defends an observable + contract? Tautological or default-value-only tests don't count. +- **Conventions** — see below. A convention breach is a real finding, not a nit to wave + through. +- **Silent contract violations** — does it advertise behavior (validation, caching, + isolation) it doesn't actually implement? + +For each concrete finding, stage an inline comment: + +``` +pr_review_comment(path="src/foo.ts", line=42, body="...", side="RIGHT", start_line=optional) +``` + +- `line` is the line in the diff you're commenting on; `side="RIGHT"` for added/changed + lines (the default), `"LEFT"` for removed lines. `start_line` for a multi-line range. +- One finding per comment. Lead with severity: **blocking** (correctness/security/contract), + **should-fix** (conventions, missing tests, regressions), **nit** (style/naming — sparingly). +- Ask, don't assume: if intent is unclear, phrase it as a question on the line. + +When done, flush everything in one review: + +``` +submit_pr_review(body="", event="COMMENT") +``` + +- `event` is always `COMMENT`. You do NOT `APPROVE` or `REQUEST_CHANGES` — those gate the + merge, which is the maintainer's call. The rank label carries your recommendation. +- The `body` summary: 2–5 lines. The rank and why, the headline findings grouped, and any + open question the maintainer must answer. Thank the contributor. No emoji. +- If the diff is clean and you found nothing, still submit a review: a one-line "lgtm — + " body with no inline comments. A clean P0 deserves an explicit green light. + +# Conventions (the bar; see `AGENTS.md`) + +Adherence is a first-class ranking signal. Flag violations as findings: + +- `CHANGELOG.md` entry under `## [Unreleased]` in each touched package. +- No prompts built in code — prompts live in `.md` files, dynamic content via Handlebars. +- No dynamic / inline `import()`; top-level imports only. +- Bun APIs over `node:*` where Bun covers it; never shell out for things with an API. +- TUI text sanitized (tabs→spaces, truncate, shorten paths) on EVERY render path, errors included. +- `#private` fields; no TS access keywords on members; no `any`; no `ReturnType<>`; star barrel exports. +- Tests assert observable contracts, never `mock.module()`, full-suite-safe. +- **No default-behaviour changes without explicit maintainer sign-off** — this alone caps a PR at P2. + +# Tone + +Terse. Technical. Evidence first, opinion last. Cite files/symbols/commits in backticks, +not vibes. Mirror the contributor's vocabulary. No filler, no emoji. Always thank the +contributor — in the review body, regardless of rank. diff --git a/python/robomp/src/prompts/review_completion_reminder.md b/python/robomp/src/prompts/review_completion_reminder.md new file mode 100644 index 000000000..725b062a0 --- /dev/null +++ b/python/robomp/src/prompts/review_completion_reminder.md @@ -0,0 +1,14 @@ +You ended your turn before finishing the PR review. + +PR: {{repo.full_name}}#{{issue.number}} — {{issue.title}} +Review workspace: `{{workspace.branch}}` + +You already started the review, but you did NOT reach the terminal action. +The acceptable terminal actions for an incoming PR review are exactly one of: + +1. `submit_pr_review` — submit the batched review summary plus any staged inline comments. +2. `abort_task` — unrecoverable environment failure. + +Review the staged comments, your TodoList, and the prior tool calls, then continue from where you stopped. Do NOT re-classify unless the earlier classify call failed. Do NOT post standalone inline findings. If you already staged comments, call `submit_pr_review` now. If you found no inline issues, still call `submit_pr_review` with the summary-only verdict. + +You MUST end this turn by calling one of the two terminal tools listed above. diff --git a/python/robomp/src/prompts/system_append_pr_review.md b/python/robomp/src/prompts/system_append_pr_review.md new file mode 100644 index 000000000..34cbf2b46 --- /dev/null +++ b/python/robomp/src/prompts/system_append_pr_review.md @@ -0,0 +1,11 @@ +You are **robomp**, reviewing an incoming pull request on `{{repo.full_name}}`. + + +- **Read-only PR review.** Never edit files, commit, push, open a PR, approve, request changes, merge, or close. +- **Review tools only.** Side effects are limited to `classify_pr`, staged `pr_review_comment` calls, one `submit_pr_review(event="COMMENT")`, and at most one `gh_post_comment` when maintainer context is required. +- **No issue triage workflow.** Do not call `classify_issue`, `set_issue_labels`, `repro_record`, `gh_push_branch`, `gh_open_pr`, or `mark_unable_to_reproduce`. +- **Classify before review comments.** Call `fetch_pr`, inspect the diff, then call `classify_pr` before staging inline comments. +- **One batched review.** Stage inline findings in sqlite and flush once with `submit_pr_review`. Submit even when there are zero inline findings. + + +Review only the PR diff and surrounding code needed to judge it. Findings must cite concrete files, lines, symbols, and failure modes. No filler, no emoji. diff --git a/python/robomp/src/prompts/todo_phases.toml b/python/robomp/src/prompts/todo_phases.toml index 3ade224cd..660eb8039 100644 --- a/python/robomp/src/prompts/todo_phases.toml +++ b/python/robomp/src/prompts/todo_phases.toml @@ -12,6 +12,26 @@ tasks = [ "Bug: repro_record, fix, open PR. Else: one gh_post_comment, stop.", ] +[[review_pr]] +name = "Orient" +tasks = [ + "Call fetch_pr and read the changed files", + "Compare the PR head against its base", +] + +[[review_pr]] +name = "Classify" +tasks = [ + "Call classify_pr with rank, type, and areas", +] + +[[review_pr]] +name = "Review" +tasks = [ + "Stage inline findings with pr_review_comment", + "Call submit_pr_review once with the summary", +] + [[handle_comment]] name = "Follow up" tasks = [ diff --git a/python/robomp/src/proxy/server.py b/python/robomp/src/proxy/server.py index 1fe210c63..6246382ae 100644 --- a/python/robomp/src/proxy/server.py +++ b/python/robomp/src/proxy/server.py @@ -33,6 +33,9 @@ from robomp.git_ops import ( from robomp.git_ops import ( clone as git_clone, ) +from robomp.git_ops import ( + fetch_pr_head as git_fetch_pr_head, +) from robomp.git_ops import ( fetch_prune as git_fetch_prune, ) @@ -120,6 +123,35 @@ def _optional_str_list(value: Any, field: str) -> list[str] | None: return list(value) +def _require_review_comments(value: Any) -> list[dict[str, Any]]: + if value is None: + return [] + if not isinstance(value, list): + raise HTTPException(400, "missing/invalid 'comments'") + comments: list[dict[str, Any]] = [] + for idx, item in enumerate(value): + if not isinstance(item, dict): + raise HTTPException(400, f"comments[{idx}] must be an object") + path = _require_str(item.get("path"), f"comments[{idx}].path") + line = _require_int(item.get("line"), f"comments[{idx}].line") + body = _require_str(item.get("body"), f"comments[{idx}].body") + side = str(item.get("side") or "RIGHT") + if side not in ("RIGHT", "LEFT"): + raise HTTPException(400, f"comments[{idx}].side must be RIGHT or LEFT") + comment: dict[str, Any] = {"path": path, "line": line, "side": side, "body": body} + start_line = item.get("start_line") + if start_line is not None: + comment["start_line"] = _require_int(start_line, f"comments[{idx}].start_line") + start_side = item.get("start_side") + if start_side is not None: + start_side_str = _require_str(start_side, f"comments[{idx}].start_side") + if start_side_str not in ("RIGHT", "LEFT"): + raise HTTPException(400, f"comments[{idx}].start_side must be RIGHT or LEFT") + comment["start_side"] = start_side_str + comments.append(comment) + return comments + + def _pool_dir(cfg: Settings, repo: str) -> Path: if "/" not in repo or repo.startswith("/") or ".." in repo.split("/"): raise HTTPException(400, f"invalid repo {repo!r}") @@ -346,6 +378,16 @@ def create_proxy_app(settings: Settings) -> FastAPI: return _gh_error_response(exc) return JSONResponse(_serialize(info)) + @app.get("/gh/v1/pr_files") + async def list_pr_files(request: Request, repo: str, pr_number: int) -> JSONResponse: + await _authenticate(request) + github: GitHubClient = request.app.state.github + try: + items = await github.list_pr_files(repo, pr_number) + except GitHubError as exc: + return _gh_error_response(exc) + return JSONResponse({"items": [_serialize(item) for item in items]}) + @app.get("/gh/v1/issues") async def list_issues(request: Request, repo: str, state: str = "open", limit: int = 30) -> JSONResponse: await _authenticate(request) @@ -467,6 +509,27 @@ def create_proxy_app(settings: Settings) -> FastAPI: return _gh_error_response(exc) return JSONResponse({"labels": list(applied)}) + @app.post("/gh/v1/submit_pr_review") + async def submit_pr_review(request: Request) -> JSONResponse: + data = await _json_body(request) + repo = _require_str(data.get("repo"), "repo") + pr_number = _require_int(data.get("pr_number"), "pr_number") + body = _require_str(data.get("body"), "body") + event = str(data.get("event") or "COMMENT") + comments = _require_review_comments(data.get("comments")) + github: GitHubClient = request.app.state.github + try: + review = await github.submit_pr_review( + repo=repo, + pr_number=pr_number, + body=body, + event=event, + comments=comments, + ) + except GitHubError as exc: + return _gh_error_response(exc) + return JSONResponse(_serialize(review)) + @app.post("/gh/v1/add_assignees") async def add_assignees(request: Request) -> JSONResponse: data = await _json_body(request) @@ -568,6 +631,18 @@ def create_proxy_app(settings: Settings) -> FastAPI: await _run_git_op(git_fetch_ref, target, ref, token=_resolve_token(settings)) return JSONResponse({"pool_dir": str(target)}) + @app.post("/gh/v1/git/fetch_pr_head") + async def git_fetch_pr_head_endpoint(request: Request) -> JSONResponse: + data = await _json_body(request) + repo = _require_str(data.get("repo"), "repo") + pr_number = _require_int(data.get("pr_number"), "pr_number") + target = _pool_dir(settings, repo) + try: + await _run_git_op(git_fetch_pr_head, target, pr_number, token=_resolve_token(settings)) + except GitCommandError as exc: + return _git_error_response(exc) + return JSONResponse({"pool_dir": str(target)}) + @app.post("/gh/v1/git/push") async def git_push_endpoint(request: Request) -> JSONResponse: data = await _json_body(request) diff --git a/python/robomp/src/proxy_client.py b/python/robomp/src/proxy_client.py index 5d8e8429b..ca539b5e9 100644 --- a/python/robomp/src/proxy_client.py +++ b/python/robomp/src/proxy_client.py @@ -25,6 +25,7 @@ from robomp.github_client import ( GitHubError, IssueInfo, IssueSummary, + PullRequestFileInfo, PullRequestInfo, PullRequestReviewInfo, ReactionInfo, @@ -169,6 +170,14 @@ class GitHubProxyClient: data = await self._request("GET", "/gh/v1/pull_request", params={"repo": repo, "number": number}) return _pr_from(data) + async def list_pr_files(self, repo: str, pr_number: int) -> list[PullRequestFileInfo]: + data = await self._request( + "GET", + "/gh/v1/pr_files", + params={"repo": repo, "pr_number": pr_number}, + ) + return [_pr_file_from(item) for item in (data.get("items") if isinstance(data, dict) else None) or []] + async def list_issues( self, repo: str, @@ -273,6 +282,28 @@ class GitHubProxyClient: ) return tuple(str(lbl) for lbl in (data.get("labels") if isinstance(data, dict) else None) or []) + async def submit_pr_review( + self, + *, + repo: str, + pr_number: int, + body: str, + event: str, + comments: list[Mapping[str, Any]], + ) -> PullRequestReviewInfo: + data = await self._request( + "POST", + "/gh/v1/submit_pr_review", + json_body={ + "repo": repo, + "pr_number": pr_number, + "body": body, + "event": event, + "comments": comments, + }, + ) + return _pr_review_from(data) + async def add_assignees(self, repo: str, number: int, assignees: list[str]) -> None: if not assignees: return @@ -360,6 +391,10 @@ class ProxyGitTransport: del pool_dir self._post("/gh/v1/git/fetch_ref", {"repo": repo, "ref": ref}) + def fetch_pr_head(self, *, repo: str, pool_dir: Path, pr_number: int) -> None: + del pool_dir + self._post("/gh/v1/git/fetch_pr_head", {"repo": repo, "pr_number": pr_number}) + def push_branch( self, *, @@ -477,6 +512,17 @@ def _pr_review_from(data: Any) -> PullRequestReviewInfo: ) +def _pr_file_from(data: Any) -> PullRequestFileInfo: + if not isinstance(data, dict): + raise GitHubError(500, "proxy returned malformed pr_file payload") + return PullRequestFileInfo( + path=str(data.get("path") or ""), + status=str(data.get("status") or ""), + additions=int(data.get("additions") or 0), + deletions=int(data.get("deletions") or 0), + ) + + def _pr_from(data: Any) -> PullRequestInfo: if not isinstance(data, dict): raise GitHubError(500, "proxy returned malformed pr payload") @@ -489,6 +535,8 @@ def _pr_from(data: Any) -> PullRequestInfo: state=str(data.get("state") or "open"), author=str(data.get("author") or ""), head_repo=str(data.get("head_repo") or ""), + title=str(data.get("title") or ""), + body=str(data.get("body") or ""), ) diff --git a/python/robomp/src/queue.py b/python/robomp/src/queue.py index 4b90a96d7..96ff5c1c2 100644 --- a/python/robomp/src/queue.py +++ b/python/robomp/src/queue.py @@ -374,6 +374,18 @@ class WorkerPool: attempts=row.attempts, slot_uid=slot_uid, ) + elif event == "pull_request" and action in ("opened", "reopened", "ready_for_review"): + await tasks.review_pr( + settings=self.settings, + db=self.db, + github=self.github, + sandbox=self.sandbox, + git_transport=self.git_transport, + payload=row.payload, + delivery_id=row.delivery_id, + attempts=row.attempts, + slot_uid=slot_uid, + ) elif event == "pull_request_review_comment" and action == "created": await tasks.handle_review( settings=self.settings, @@ -395,12 +407,14 @@ class WorkerPool: target_state="closed", ) elif event == "pull_request" and action == "closed": + pr = row.payload.get("pull_request") or {} + target_state = "merged" if bool(pr.get("merged")) else "closed" await tasks.cleanup_workspace( settings=self.settings, db=self.db, sandbox=self.sandbox, payload=row.payload, - target_state="merged", + target_state=target_state, ) else: log.info("no-op dispatch", extra={"event": event, "action": action}) diff --git a/python/robomp/src/sandbox.py b/python/robomp/src/sandbox.py index 15a671b0e..4e15b28e7 100644 --- a/python/robomp/src/sandbox.py +++ b/python/robomp/src/sandbox.py @@ -59,6 +59,9 @@ from robomp.git_ops import ( from robomp.git_ops import ( clone as git_clone, ) +from robomp.git_ops import ( + fetch_pr_head as git_fetch_pr_head, +) from robomp.git_ops import ( fetch_prune as git_fetch_prune, ) @@ -231,6 +234,10 @@ class GitTransport(Protocol): """Best-effort `git fetch origin ` to ensure the base branch is local.""" ... + def fetch_pr_head(self, *, repo: str, pool_dir: Path, pr_number: int) -> None: + """Fetch `refs/pull//head` into FETCH_HEAD for detached PR review checkouts.""" + ... + def push_branch( self, *, @@ -270,6 +277,10 @@ class LocalGitTransport: del repo git_fetch_ref(pool_dir, ref, token=self._token) + def fetch_pr_head(self, *, repo: str, pool_dir: Path, pr_number: int) -> None: + del repo + git_fetch_pr_head(pool_dir, pr_number, token=self._token) + def push_branch( self, *, @@ -679,11 +690,14 @@ class SandboxManager: clone_url: str, default_branch: str, existing_branch: str | None = None, + pr_head: int | None = None, author_name: str, author_email: str, slot_uid: int | None = None, ) -> Workspace: """Create or resume a per-issue worktree.""" + if pr_head is not None and existing_branch is not None: + raise ValueError("ensure_workspace accepts either pr_head or existing_branch, not both") pool = self.ensure_clone(repo=repo, clone_url=clone_url, default_branch=default_branch) ws_root = self.workspace_root(repo, number) repo_dir = ws_root / "repo" @@ -693,10 +707,15 @@ class SandboxManager: for path in (ws_root, session_dir, context_dir, context_dir / "repro", artifacts_dir): path.mkdir(parents=True, exist_ok=True) - branch = existing_branch or make_branch( - issue_number=number, - title=title, - seed=f"{repo}#{number}", + branch = ( + f"review/pr-{pr_head}" + if pr_head is not None + else existing_branch + or make_branch( + issue_number=number, + title=title, + seed=f"{repo}#{number}", + ) ) repo_exists = (repo_dir / ".git").exists() @@ -713,35 +732,39 @@ class SandboxManager: _chown_workspace(ws_root, slot_uid) workspace_prepared = True if not repo_exists: - # Make sure the requested start point exists locally (best-effort). - # For follow-ups on an existing PR, `existing_branch` is the remote - # head branch we need to amend; starting from default would silently - # lose the PR's current commits if the local pool branch is absent. - self.transport.fetch_base_ref(repo=repo, pool_dir=pool, ref=existing_branch or default_branch) - check = _safe_run(["git", "rev-parse", "--verify", f"refs/heads/{branch}"], cwd=pool) - if check.returncode == 0: - _run(["git", "worktree", "add", str(repo_dir), branch], cwd=pool) + if pr_head is not None: + self.transport.fetch_pr_head(repo=repo, pool_dir=pool, pr_number=pr_head) + _run(["git", "worktree", "add", "--detach", str(repo_dir), "FETCH_HEAD"], cwd=pool) else: - start_point = f"origin/{default_branch}" - if existing_branch: - remote = _safe_run( - ["git", "rev-parse", "--verify", f"refs/remotes/origin/{existing_branch}"], + # Make sure the requested start point exists locally (best-effort). + # For follow-ups on an existing PR, `existing_branch` is the remote + # head branch we need to amend; starting from default would silently + # lose the PR's current commits if the local pool branch is absent. + self.transport.fetch_base_ref(repo=repo, pool_dir=pool, ref=existing_branch or default_branch) + check = _safe_run(["git", "rev-parse", "--verify", f"refs/heads/{branch}"], cwd=pool) + if check.returncode == 0: + _run(["git", "worktree", "add", str(repo_dir), branch], cwd=pool) + else: + start_point = f"origin/{default_branch}" + if existing_branch: + remote = _safe_run( + ["git", "rev-parse", "--verify", f"refs/remotes/origin/{existing_branch}"], + cwd=pool, + ) + if remote.returncode == 0: + start_point = f"origin/{existing_branch}" + _run( + [ + "git", + "worktree", + "add", + "-b", + branch, + str(repo_dir), + start_point, + ], cwd=pool, ) - if remote.returncode == 0: - start_point = f"origin/{existing_branch}" - _run( - [ - "git", - "worktree", - "add", - "-b", - branch, - str(repo_dir), - start_point, - ], - cwd=pool, - ) else: slot_git_env = _git_env_for_repo(repo_dir) current = _safe_run( diff --git a/python/robomp/src/server.py b/python/robomp/src/server.py index 2cab9702f..ff1833c3c 100644 --- a/python/robomp/src/server.py +++ b/python/robomp/src/server.py @@ -340,6 +340,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: bot_login=cfg.bot_login, maintainers=cfg.maintainer_logins, reviewer_bots=cfg.reviewer_bots, + pr_review_enabled=cfg.pr_review_enabled, resolve_issue_from_pr=_resolve, ) diff --git a/python/robomp/src/tasks.py b/python/robomp/src/tasks.py index 2b9ba5739..00b898661 100644 --- a/python/robomp/src/tasks.py +++ b/python/robomp/src/tasks.py @@ -290,6 +290,83 @@ async def triage_issue( await run_task(task_kind="triage_issue", inputs=inputs) +async def review_pr( + *, + settings: Settings, + db: Database, + github: GitHubBackend, + sandbox: SandboxManager, + git_transport: GitTransport, + payload: Mapping[str, Any], + delivery_id: str, + attempts: int = 0, + slot_uid: int | None = None, +) -> None: + pr_node = payload.get("pull_request") or {} + pr_number = int(pr_node.get("number") or 0) + repo_payload = payload.get("repository") or {} + repo_full = str(repo_payload.get("full_name") or "") + if pr_number <= 0 or not repo_full: + log.info("skip: review_pr missing repo/number") + return + try: + repo = await github.get_repo(repo_full) + issue = await github.get_issue(repo_full, pr_number) + pr = await github.get_pull_request(repo_full, pr_number) + except GitHubError as exc: + log.warning("review_pr fetch failed", extra={"repo": repo_full, "pr": pr_number, "err": str(exc)}) + return + + labels = {label.lower() for label in issue.labels} + key = issue_key(repo.full_name, pr_number) + review_labeled = "triaged" in labels or any(label.startswith("review:") for label in labels) + if db.has_successful_tool_call(key, "submit_pr_review"): + log.info("skip: PR review already submitted", extra={"repo": repo_full, "pr": pr_number}) + return + if review_labeled: + log.info( + "review labels present without submitted review; retrying", + extra={"repo": repo_full, "pr": pr_number, "labels": sorted(labels)}, + ) + + db.upsert_issue(key=key, repo=repo.full_name, number=pr_number, state="reviewing", pr_number=pr_number) + workspace = sandbox.ensure_workspace( + repo=repo.full_name, + number=pr_number, + title=issue.title, + clone_url=repo.clone_url, + default_branch=repo.default_branch, + pr_head=pr_number, + author_name=settings.resolved_author_name, + author_email=settings.git_author_email, + slot_uid=slot_uid, + ) + db.upsert_issue( + key=key, + repo=repo.full_name, + number=pr_number, + state="reviewing", + branch=workspace.branch, + session_dir=str(workspace.session_dir), + pr_number=pr_number, + ) + inputs = TaskInputs( + settings=settings, + db=db, + github=github, + git_transport=git_transport, + repo=repo, + issue=issue, + workspace=workspace, + delivery_id=delivery_id, + attempts=attempts, + slot_uid=slot_uid, + natives_cache=sandbox.natives_cache, + ) + await run_task(task_kind="review_pr", inputs=inputs, pr_number=pr_number, pr=pr) + return + + async def handle_comment( *, settings: Settings, @@ -566,6 +643,9 @@ async def handle_pr_conversation( if pr_info is None or not _can_handle_pr_directly(settings=settings, repo_full=repo_full, pr=pr_info): return directive = _directive_from_payload(payload) + if issue_row is not None and issue_row.state == "reviewing": + log.info("skip: incoming PR conversation unsupported", extra={"key": issue_row.key, "pr": pr_number}) + return if issue_row is not None and issue_row.state in ("merged", "closed", "abandoned"): if directive is None: log.info("skip: pr-conversation on finalized issue", extra={"key": issue_row.key, "state": issue_row.state}) @@ -716,5 +796,6 @@ __all__ = [ "handle_comment", "handle_pr_conversation", "handle_review", + "review_pr", "triage_issue", ] diff --git a/python/robomp/src/worker.py b/python/robomp/src/worker.py index ea8326ebc..eb763ad0a 100644 --- a/python/robomp/src/worker.py +++ b/python/robomp/src/worker.py @@ -35,7 +35,7 @@ from robomp.config import Settings from robomp.db import Database, issue_key from robomp.git_ops import DirtyState, inspect_dirty_state from robomp.github_backend import GitHubBackend -from robomp.github_client import CommentInfo, IssueInfo, RepoInfo +from robomp.github_client import CommentInfo, IssueInfo, PullRequestInfo, RepoInfo from robomp.host_tools import AbortController, ToolBindings, _git_identity_env from robomp.natives_cache import NativesCache from robomp.natives_cache import compute_key as natives_compute_key @@ -190,6 +190,7 @@ def _build_extra_env(settings: Settings) -> dict[str, str]: _TERMINAL_TRIAGE_TOOLS: frozenset[str] = frozenset({"gh_open_pr", "mark_unable_to_reproduce", "abort_task"}) +_TERMINAL_REVIEW_TOOLS: frozenset[str] = frozenset({"submit_pr_review", "abort_task"}) _PR_REQUIRING_CLASSIFICATIONS: frozenset[str] = frozenset({"bug", "documentation"}) @@ -200,16 +201,13 @@ def _needs_completion_reminder( bindings: ToolBindings, tools_called: set[str], ) -> bool: - """True iff a `triage_issue` turn ended before reaching a terminal tool. - - Only enforced for `bug` / `documentation` classifications — `question`, - `enhancement`, `proposal`, `invalid`, `duplicate` terminate on a single - `gh_post_comment` which we can't reliably distinguish from a preamble. - """ - if task_kind != "triage_issue": - return False + """True iff a task turn ended before reaching its terminal tool.""" if bindings.abort is not None and bindings.abort.triggered: return False + if task_kind == "review_pr": + return not (tools_called & _TERMINAL_REVIEW_TOOLS) + if task_kind != "triage_issue": + return False row = inputs.db.get_issue(bindings.issue_key) if row is None or row.classification not in _PR_REQUIRING_CLASSIFICATIONS: return False @@ -278,8 +276,9 @@ def _drive_turn( needs_completion = _needs_completion_reminder( task_kind=task_kind, inputs=inputs, bindings=bindings, tools_called=tools_called ) - dirty: DirtyState | None = None if not needs_completion: + if task_kind == "review_pr": + break dirty = _probe_workspace_dirty(inputs.workspace, inputs.slot_uid) if not dirty.is_dirty: break @@ -294,7 +293,11 @@ def _drive_turn( "max": max_reminders, }, ) - reminder = persona.completion_reminder(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + reminder = ( + persona.review_completion_reminder(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + if task_kind == "review_pr" + else persona.completion_reminder(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + ) else: assert dirty is not None log.warning( @@ -331,7 +334,7 @@ def _drive_turn( "tools_called": sorted(tools_called), }, ) - if reminders_used: + if reminders_used and task_kind != "review_pr": final_dirty = _probe_workspace_dirty(inputs.workspace, inputs.slot_uid) if final_dirty.is_dirty: log.warning( @@ -368,6 +371,7 @@ def _build_prompt( comment: CommentInfo | None, pr_number: int | None, review_payload: dict[str, Any] | None, + pr: PullRequestInfo | None = None, directive: DirectiveInfo | None = None, thread: tuple[ThreadMessage, ...] = (), resuming: bool = False, @@ -383,6 +387,9 @@ def _build_prompt( directive=directive, ) return persona.kickoff(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + if task_kind == "review_pr": + assert pr is not None + return persona.kickoff_pr_review(repo=inputs.repo, pr=pr, workspace=inputs.workspace) if task_kind == "handle_comment": assert comment is not None issue_row = inputs.db.get_issue(issue_key(inputs.repo.full_name, inputs.issue.number)) @@ -503,6 +510,11 @@ def _run_rpc_blocking( }, ) inputs.db.set_event_model(inputs.delivery_id, chosen_model) + append_system_prompt = ( + persona.system_append_pr_review(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + if task_kind == "review_pr" + else persona.system_append(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace) + ) with RpcClient( executable=settings.omp_command, @@ -514,7 +526,7 @@ def _run_rpc_blocking( model=chosen_model, provider=settings.provider, thinking=chosen_thinking if chosen_thinking != "off" else None, - append_system_prompt=persona.system_append(repo=inputs.repo, issue=inputs.issue, workspace=inputs.workspace), + append_system_prompt=append_system_prompt, custom_tools=host_tools.build(bindings), request_timeout=settings.request_timeout_seconds, startup_timeout=60.0, @@ -557,13 +569,12 @@ def _run_rpc_blocking( phases = persona.seed_phases(task_kind) if phases: try: - if task_kind == "triage_issue" and not resuming: - # Fresh triage: seed the full plan. + if task_kind in ("triage_issue", "review_pr") and not resuming: + # Fresh kickoff tasks seed the full plan. client.set_todos(phases) - elif task_kind == "triage_issue": - # Resumed triage: prior phases are intact in the - # JSONL transcript — re-seeding would clobber any - # in-progress task statuses. Trust the loaded state. + elif task_kind in ("triage_issue", "review_pr"): + # Resumed kickoff tasks keep prior todo state from the + # JSONL transcript; re-seeding would clobber progress. log.info( "set_todos skipped (resume)", extra={"issue": bindings.issue_key, "task": task_kind}, @@ -653,10 +664,12 @@ async def run_task( comment: CommentInfo | None = None, pr_number: int | None = None, review_payload: dict[str, Any] | None = None, + pr: PullRequestInfo | None = None, directive: DirectiveInfo | None = None, thread: tuple[ThreadMessage, ...] = (), ) -> str | None: """Async wrapper that runs the synchronous RPC driver on a worker thread.""" + review_mode = task_kind == "review_pr" or inputs.workspace.branch.startswith("review/pr-") loop = asyncio.get_running_loop() bindings = ToolBindings( db=inputs.db, @@ -671,6 +684,7 @@ async def run_task( author_email=inputs.settings.git_author_email, inbound_thread_number=pr_number, inbound_is_pr=pr_number is not None, + review_mode=review_mode, slot_uid=inputs.slot_uid, abort=AbortController(), ) @@ -681,6 +695,7 @@ async def run_task( comment=comment, pr_number=pr_number, review_payload=review_payload, + pr=pr, directive=directive, thread=thread, resuming=resuming, diff --git a/python/robomp/tests/test_db.py b/python/robomp/tests/test_db.py index b46bf54cd..55aa05584 100644 --- a/python/robomp/tests/test_db.py +++ b/python/robomp/tests/test_db.py @@ -204,6 +204,36 @@ def test_log_tool_call(db: Database) -> None: assert row_id > 0 +def test_pr_review_comment_staging_round_trip(db: Database) -> None: + first = db.stage_review_comment( + issue_key="octo/widget#9", + path="src/app.py", + line=12, + side="RIGHT", + start_line=10, + start_side="RIGHT", + body="blocking finding", + ) + db.stage_review_comment( + issue_key="octo/widget#9", + path="src/other.py", + line=3, + body="nit", + ) + db.stage_review_comment(issue_key="octo/widget#10", path="x.py", line=1, body="other") + + rows = db.list_staged_review_comments("octo/widget#9") + assert [row.id for row in rows] == [first.id, first.id + 1] + assert rows[0].path == "src/app.py" + assert rows[0].start_line == 10 + assert rows[0].start_side == "RIGHT" + assert rows[1].side == "RIGHT" + + assert db.clear_staged_review_comments("octo/widget#9") == 2 + assert db.list_staged_review_comments("octo/widget#9") == [] + assert len(db.list_staged_review_comments("octo/widget#10")) == 1 + + def test_processed_issue_keys_returns_only_known(db: Database) -> None: db.upsert_issue(key=issue_key("octo/widget", 1), repo="octo/widget", number=1, state="new") db.upsert_issue(key=issue_key("octo/widget", 2), repo="octo/widget", number=2, state="reproducing") diff --git a/python/robomp/tests/test_github_client.py b/python/robomp/tests/test_github_client.py index 9da1d4ce4..c055eee72 100644 --- a/python/robomp/tests/test_github_client.py +++ b/python/robomp/tests/test_github_client.py @@ -109,6 +109,117 @@ def test_get_pull_request_parses_head_repo_and_author() -> None: assert pr.author == "robomp-bot" +def test_get_pull_request_parses_title_and_body() -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.path == "/repos/octo/widget/pulls/9" + return httpx.Response( + 200, + json={ + "number": 9, + "html_url": "https://github.com/octo/widget/pull/9", + "title": "Fix crash", + "body": "Fixes #1", + "head": {"ref": "fix", "repo": {"full_name": "fork/widget"}}, + "base": {"ref": "main"}, + "state": "open", + "user": {"login": "alice"}, + }, + ) + + client = GitHubClient("tok", transport=httpx.MockTransport(handler)) + pr = _run_async(client.get_pull_request("octo/widget", 9)) + assert pr.title == "Fix crash" + assert pr.body == "Fixes #1" + + +def test_list_pr_files_parses_changed_file_summary() -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.path == "/repos/octo/widget/pulls/9/files" + assert request.url.params.get("per_page") == "100" + 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 len(files) == 1 + assert files[0].path == "src/app.py" + assert files[0].additions == 5 + assert files[0].deletions == 2 + + +def test_list_pr_files_paginates_past_first_page() -> None: + seen_pages: list[str | None] = [] + + def handler(request: httpx.Request) -> httpx.Response: + assert request.url.path == "/repos/octo/widget/pulls/9/files" + page = request.url.params.get("page") + seen_pages.append(page) + if page == "1": + return httpx.Response( + 200, + json=[ + { + "filename": f"src/file-{idx}.py", + "status": "modified", + "additions": 1, + "deletions": 0, + } + for idx in range(100) + ], + ) + assert page == "2" + return httpx.Response( + 200, + json=[{"filename": "src/final.py", "status": "added", "additions": 2, "deletions": 0}], + ) + + client = GitHubClient("tok", transport=httpx.MockTransport(handler)) + files = _run_async(client.list_pr_files("octo/widget", 9)) + assert seen_pages == ["1", "2"] + assert len(files) == 101 + assert files[-1].path == "src/final.py" + + +def test_submit_pr_review_posts_comment_event_and_inline_comments() -> None: + captured: dict[str, object] = {} + + def handler(request: httpx.Request) -> httpx.Response: + import json + + 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)) + 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"}], + ) + ) + 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", "line": 12, "side": "RIGHT", "body": "finding"}], + } + + def test_204_no_content_returns_none() -> None: transport = httpx.MockTransport(lambda r: httpx.Response(204)) client = GitHubClient("tok", transport=transport) diff --git a/python/robomp/tests/test_github_events.py b/python/robomp/tests/test_github_events.py index 0d32bba2e..5ac17811e 100644 --- a/python/robomp/tests/test_github_events.py +++ b/python/robomp/tests/test_github_events.py @@ -131,7 +131,7 @@ def test_route_pr_conversation_uses_handle_pr_conversation() -> None: { "action": "created", "comment": {"user": {"login": "alice"}, "body": "looks good"}, - "issue": {"number": 9, "pull_request": {"url": "x"}}, + "issue": {"number": 9, "user": {"login": BOT}, "pull_request": {"url": "x"}}, "repository": {"full_name": "octo/widget"}, }, allowlist=ALLOWLIST, @@ -155,7 +155,7 @@ def test_route_pr_conversation_uses_resolver_for_inflight_key() -> None: { "action": "created", "comment": {"user": {"login": "alice"}, "body": "looks good"}, - "issue": {"number": 9, "pull_request": {"url": "x"}}, + "issue": {"number": 9, "user": {"login": BOT}, "pull_request": {"url": "x"}}, "repository": {"full_name": "octo/widget"}, }, allowlist=ALLOWLIST, @@ -175,7 +175,7 @@ def test_route_pr_conversation_falls_back_to_pr_key_when_resolver_misses() -> No { "action": "created", "comment": {"user": {"login": "alice"}, "body": "hi"}, - "issue": {"number": 9, "pull_request": {"url": "x"}}, + "issue": {"number": 9, "user": {"login": BOT}, "pull_request": {"url": "x"}}, "repository": {"full_name": "octo/widget"}, }, allowlist=ALLOWLIST, @@ -188,6 +188,98 @@ def test_route_pr_conversation_falls_back_to_pr_key_when_resolver_misses() -> No assert decision.issue_key == "octo/widget#9" +def test_route_incoming_pr_opened_queues_review_pr() -> None: + decision = route( + "pull_request", + { + "action": "opened", + "pull_request": { + "number": 9, + "draft": False, + "user": {"login": "alice", "type": "User"}, + "author_association": "CONTRIBUTOR", + }, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert decision.should_queue + assert decision.task == "review_pr" + assert decision.issue_key == "octo/widget#9" + assert decision.submitter == "alice" + assert decision.association == "CONTRIBUTOR" + + +def test_route_incoming_pr_opened_skips_draft_bot_and_disabled() -> None: + payload = { + "action": "opened", + "pull_request": {"number": 9, "draft": True, "user": {"login": "alice", "type": "User"}}, + "repository": {"full_name": "octo/widget"}, + } + assert not route("pull_request", payload, allowlist=ALLOWLIST, bot_login=BOT).should_queue + + payload["pull_request"]["draft"] = False # type: ignore[index] + payload["pull_request"]["user"] = {"login": BOT, "type": "Bot"} # type: ignore[index] + assert not route("pull_request", payload, allowlist=ALLOWLIST, bot_login=BOT).should_queue + + payload["pull_request"]["user"] = {"login": "alice", "type": "User"} # type: ignore[index] + disabled = route("pull_request", payload, allowlist=ALLOWLIST, bot_login=BOT, pr_review_enabled=False) + assert not disabled.should_queue + assert "disabled" in disabled.reason + + +def test_route_pull_request_synchronize_stays_skipped() -> None: + decision = route( + "pull_request", + { + "action": "synchronize", + "pull_request": {"number": 9, "user": {"login": "alice"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert not decision.should_queue + + +def test_route_incoming_pr_comment_skips() -> None: + decision = route( + "issue_comment", + { + "action": "created", + "comment": {"user": {"login": "alice"}, "body": "ping"}, + "issue": {"number": 9, "user": {"login": "contributor"}, "pull_request": {"url": "x"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert not decision.should_queue + assert "incoming PR comments ignored" == decision.reason + + +def test_route_incoming_pr_comment_with_maintainer_mention_still_skips() -> None: + decision = route( + "issue_comment", + { + "action": "created", + "comment": { + "user": {"login": "can1357"}, + "author_association": "OWNER", + "body": "@robomp-bot please re-review", + }, + "issue": {"number": 9, "user": {"login": "contributor"}, "pull_request": {"url": "x"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert not decision.should_queue + assert decision.issue_key == "octo/widget#9" + assert decision.reason == "incoming PR comments ignored" + + def test_route_review_only_for_bot_authored_pr() -> None: decision = route( "pull_request_review_comment", @@ -238,10 +330,10 @@ def test_route_review_comment_falls_back_to_pr_key_when_resolver_misses() -> Non assert decision.issue_key == "octo/widget#9" -def test_route_pr_closed_only_when_merged_by_bot() -> None: +def test_route_pr_closed_cleans_up_any_tracked_pr() -> None: payload = { "action": "closed", - "pull_request": {"number": 9, "user": {"login": BOT}, "merged": True}, + "pull_request": {"number": 9, "user": {"login": "alice"}, "merged": False}, "repository": {"full_name": "octo/widget"}, } decision = route( @@ -254,21 +346,21 @@ def test_route_pr_closed_only_when_merged_by_bot() -> None: assert decision.should_queue assert decision.task == "cleanup_workspace" assert decision.issue_key == "octo/widget#42" + assert decision.reason == "pull_request.closed" - fallback = route( + payload["pull_request"]["merged"] = True # type: ignore[index] + merged = route( "pull_request", payload, allowlist=ALLOWLIST, bot_login=BOT, resolve_issue_from_pr=lambda _r, _n: None, ) - assert fallback.should_queue - assert fallback.task == "cleanup_workspace" - assert fallback.issue_key == "octo/widget#9" - assert fallback.submitter is None - - payload["pull_request"]["merged"] = False # type: ignore[index] - assert not route("pull_request", payload, allowlist=ALLOWLIST, bot_login=BOT).should_queue + assert merged.should_queue + assert merged.task == "cleanup_workspace" + assert merged.issue_key == "octo/widget#9" + assert merged.reason == "pull_request.merged" + assert merged.submitter is None def test_route_skips_pull_request_issues_event() -> None: @@ -537,7 +629,7 @@ def test_route_directive_unset_for_maintainer_without_mention() -> None: assert decision.directive is False -def test_route_directive_set_on_pr_conversation() -> None: +def test_route_directive_on_incoming_pr_conversation_is_ignored() -> None: decision = route( "issue_comment", { @@ -554,10 +646,8 @@ def test_route_directive_set_on_pr_conversation() -> None: bot_login=BOT, resolve_issue_from_pr=lambda _r, _n: "octo/widget#42", ) - assert decision.should_queue - assert decision.task == "handle_pr_conversation" - assert decision.directive is True - assert decision.directive_body == "change the indentation in foo.py" + assert not decision.should_queue + assert decision.reason == "incoming PR comments ignored" def test_route_directive_set_on_review_comment() -> None: @@ -586,7 +676,7 @@ def test_route_directive_set_on_review_comment() -> None: # ---------- reviewer bots ---------- -def test_route_reviewer_bot_comment_is_directive_without_mention() -> None: +def test_route_reviewer_bot_comment_on_incoming_pr_is_ignored() -> None: decision = route( "issue_comment", { @@ -603,11 +693,8 @@ def test_route_reviewer_bot_comment_is_directive_without_mention() -> None: reviewer_bots=frozenset({"chatgpt-codex-connector"}), resolve_issue_from_pr=lambda _r, _n: "octo/widget#42", ) - assert decision.should_queue - assert decision.task == "handle_pr_conversation" - assert decision.directive is True - assert decision.directive_body == "Found two issues in the diff: ..." - assert decision.directive_author == "chatgpt-codex-connector" + assert not decision.should_queue + assert decision.reason == "incoming PR comments ignored" def test_route_reviewer_bot_review_comment_is_directive() -> None: @@ -651,16 +738,16 @@ def test_route_random_bot_still_skipped_when_not_in_reviewer_list() -> None: assert "bot" in decision.reason -def test_route_reviewer_bot_login_case_insensitive() -> None: +def test_route_reviewer_bot_login_case_insensitive_for_review_comments() -> None: decision = route( - "issue_comment", + "pull_request_review_comment", { "action": "created", "comment": { "user": {"login": "ChatGPT-Codex-Connector", "type": "Bot"}, "body": "feedback", }, - "issue": {"number": 9, "pull_request": {"url": "x"}}, + "pull_request": {"number": 9, "user": {"login": BOT}}, "repository": {"full_name": "octo/widget"}, }, allowlist=ALLOWLIST, @@ -693,16 +780,16 @@ def test_route_directive_strips_pragmas_from_maintainer_comment() -> None: assert decision.directive_pragmas == (("model", "gpt"), ("thinking", "low")) -def test_route_directive_strips_pragmas_from_reviewer_bot_comment() -> None: +def test_route_directive_strips_pragmas_from_reviewer_bot_review_comment() -> None: decision = route( - "issue_comment", + "pull_request_review_comment", { "action": "created", "comment": { "user": {"login": "chatgpt-codex-connector", "type": "Bot"}, "body": "/model claude\nLeak in foo()", }, - "issue": {"number": 9, "pull_request": {"url": "x"}}, + "pull_request": {"number": 9, "user": {"login": BOT}}, "repository": {"full_name": "octo/widget"}, }, allowlist=ALLOWLIST, diff --git a/python/robomp/tests/test_host_tools.py b/python/robomp/tests/test_host_tools.py index 37fa4225b..1c2756c01 100644 --- a/python/robomp/tests/test_host_tools.py +++ b/python/robomp/tests/test_host_tools.py @@ -684,6 +684,264 @@ def _pr_bindings( return bindings, loop, thread +def _review_bindings( + db: Database, tmp_path: Path, transport: httpx.MockTransport +) -> tuple[ToolBindings, asyncio.AbstractEventLoop, threading.Thread]: + github = GitHubClient("token", transport=transport) + loop, thread = _make_loop_in_background() + issue = IssueInfo( + repo="octo/widget", + number=99, + title="contributor PR", + body="body", + state="open", + author="alice", + labels=(), + is_pull_request=True, + ) + workspace = _stub_workspace(tmp_path) + workspace.issue_number = 99 + bindings = ToolBindings( + db=db, + github=github, + git_transport=LocalGitTransport(token=None), + repo=_stub_repo(), + issue=issue, + workspace=workspace, + loop=loop, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + inbound_thread_number=99, + inbound_is_pr=True, + review_mode=True, + ) + db.upsert_issue( + key=bindings.issue_key, + repo="octo/widget", + number=99, + state="reviewing", + branch=bindings.workspace.branch, + session_dir=str(bindings.workspace.session_dir), + pr_number=99, + ) + return bindings, loop, thread + + +def test_fetch_pr_returns_premise_and_changed_files(db: Database, tmp_path: Path) -> None: + 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://github.com/octo/widget/pull/99", + "title": "Fix crash", + "body": "Fixes #42", + "head": {"ref": "fix-crash", "repo": {"full_name": "alice/widget"}}, + "base": {"ref": "main"}, + "state": "open", + "user": {"login": "alice"}, + }, + ) + if request.url.path == "/repos/octo/widget/pulls/99/files": + return httpx.Response( + 200, + json=[{"filename": "src/app.py", "status": "modified", "additions": 5, "deletions": 2}], + ) + return httpx.Response(404, json={"message": "unrouted"}) + + bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler)) + try: + tool = next(x for x in build(bindings) if x.name == "fetch_pr") + result = tool.execute({}, _ctx()) + finally: + _stop_loop(loop, t) + + assert "Fix crash" in result + assert "#42" in result + assert "`src/app.py` (modified, +5/-2)" in result + + +def test_classify_pr_applies_review_labels_and_persists_rank(db: Database, tmp_path: Path) -> None: + captured: dict[str, Any] = {} + + def handler(request: httpx.Request) -> httpx.Response: + captured["path"] = request.url.path + captured["body"] = json.loads(request.content) + return httpx.Response(200, json=[{"name": label} for label in captured["body"]["labels"]]) + + bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler)) + try: + tool = next(x for x in build(bindings) if x.name == "classify_pr") + result = tool.execute( + { + "rank": "review:p1", + "type": "fix", + "area": ["tool", "unknown"], + "provider": "provider:openai", + "rationale": "fixes the tool crash with a scoped guard", + }, + _ctx(), + ) + finally: + _stop_loop(loop, t) + + assert "review:p1" in result + assert captured["path"].endswith("/issues/99/labels") + assert captured["body"]["labels"] == ["triaged", "review:p1", "fix", "tool", "providers", "provider:openai"] + row = db.get_issue(bindings.issue_key) + assert row is not None and row.classification == "review:p1" + + +def test_classify_pr_rejects_bad_rank(db: Database, tmp_path: Path) -> None: + bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) + try: + tool = next(x for x in build(bindings) if x.name == "classify_pr") + with pytest.raises(RpcCommandError): + tool.execute({"rank": "prio:p1", "type": "fix", "rationale": "wrong namespace"}, _ctx()) + finally: + _stop_loop(loop, t) + + +def test_pr_review_comment_stages_and_submit_flushes_one_comment_review(db: Database, tmp_path: Path) -> None: + captured: dict[str, Any] = {} + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/reviews"): + captured["path"] = request.url.path + 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") + staged = stage_tool.execute( + { + "path": "src/app.py", + "line": 12, + "side": "RIGHT", + "start_line": 10, + "start_side": "RIGHT", + "body": "blocking: this dereferences cfg before the guard.", + }, + _ctx(), + ) + assert "staged_count=1" in staged + rows = db.list_staged_review_comments(bindings.issue_key) + assert len(rows) == 1 + assert rows[0].path == "src/app.py" + + result = submit_tool.execute({"body": "review:p1 — one blocking issue", "event": "APPROVE"}, _ctx()) + finally: + _stop_loop(loop, t) + + assert "submitted PR review" in result + assert captured["path"].endswith("/pulls/99/reviews") + assert captured["body"] == { + "body": "review:p1 — one blocking issue", + "event": "COMMENT", + "comments": [ + { + "path": "src/app.py", + "line": 12, + "side": "RIGHT", + "body": "blocking: this dereferences cfg before the guard.", + "start_line": 10, + "start_side": "RIGHT", + } + ], + } + assert db.list_staged_review_comments(bindings.issue_key) == [] + + +def test_submit_pr_review_posts_summary_only_when_no_staged_comments(db: Database, tmp_path: Path) -> None: + captured: dict[str, Any] = {} + + def handler(request: httpx.Request) -> httpx.Response: + captured["body"] = json.loads(request.content) + return httpx.Response( + 200, + json={"id": 45, "user": {"login": "robomp-bot"}, "body": "ok", "state": "COMMENTED", "submitted_at": "t"}, + ) + + bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(handler)) + try: + tool = next(x for x in build(bindings) if x.name == "submit_pr_review") + result = tool.execute({"body": "lgtm — scoped fix"}, _ctx()) + finally: + _stop_loop(loop, t) + + assert "comments=0" in result + assert captured["body"]["event"] == "COMMENT" + assert captured["body"]["comments"] == [] + + +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"})), + ) + 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): + 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_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: + tool = next(x for x in build(bindings) if x.name == "pr_review_comment") + with pytest.raises(RpcCommandError): + tool.execute({"path": "x.py", "line": 1, "body": "nit"}, _ctx()) + finally: + _stop_loop(loop, t) + + +def test_review_mode_rejects_push_and_open_pr_before_repo_commands(db: Database, tmp_path: Path) -> None: + calls: list[list[str] | tuple[str, ...]] = [] + + def record_repo_command(_bindings: ToolBindings, cmd: list[str] | tuple[str, ...], *, timeout: float | None = None): + del timeout + calls.append(cmd) + raise AssertionError("repo command must not run in review mode") + + bindings, loop, t = _review_bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) + try: + original = host_tools._run_repo_command + host_tools._run_repo_command = record_repo_command # type: ignore[assignment] + push = next(x for x in build(bindings) if x.name == "gh_push_branch") + open_pr = next(x for x in build(bindings) if x.name == "gh_open_pr") + with pytest.raises(RpcCommandError): + push.execute({}, _ctx()) + with pytest.raises(RpcCommandError): + open_pr.execute({"title": "t", "body": "invalid"}, _ctx()) + finally: + host_tools._run_repo_command = original # type: ignore[assignment] + _stop_loop(loop, t) + + assert calls == [] + + def test_classify_issue_on_pr_thread_is_noop(db: Database, tmp_path: Path) -> None: """On PR threads the tool must not hit GitHub and must not raise.""" calls: list[str] = [] diff --git a/python/robomp/tests/test_persona.py b/python/robomp/tests/test_persona.py index 094f9c835..6d9f1e2a3 100644 --- a/python/robomp/tests/test_persona.py +++ b/python/robomp/tests/test_persona.py @@ -36,6 +36,16 @@ class _Workspace: repo_dir: str = "/tmp/repo" +@dataclass(slots=True, frozen=True) +class _Pr: + number: int = 99 + author: str = "alice" + head_ref: str = "fix-crash" + base_ref: str = "main" + head_repo: str = "alice/widget" + html_url: str = "https://github.com/octo/widget/pull/99" + + @dataclass(slots=True, frozen=True) class _Comment: id: int = 1 @@ -151,3 +161,23 @@ def test_resume_triage_renders_branch_and_issue() -> None: assert "broken thing" in out # The prompt instructs the agent to reconcile drift via fetch_issue_thread. assert "fetch_issue_thread" in out + + +def test_kickoff_pr_review_formats_head_repo_and_origin_base() -> None: + out = persona.kickoff_pr_review( + repo=_Repo(), + pr=_Pr(), + workspace=_Workspace(), + ) + assert "`fix-crash` from `alice/widget`" in out + assert "git diff origin/main...HEAD" in out + + +def test_review_completion_reminder_mentions_submit_only() -> None: + out = persona.review_completion_reminder( + repo=_Repo(), + issue=_Issue(number=99, title="Fix parser"), + workspace=_Workspace(branch="review/pr-99"), + ) + assert "submit_pr_review" in out + assert "gh_open_pr" not in out diff --git a/python/robomp/tests/test_proxy_client.py b/python/robomp/tests/test_proxy_client.py index 62e3e1516..691ba5519 100644 --- a/python/robomp/tests/test_proxy_client.py +++ b/python/robomp/tests/test_proxy_client.py @@ -22,6 +22,7 @@ from robomp.github_client import ( GitHubError, IssueInfo, IssueSummary, + PullRequestFileInfo, PullRequestInfo, PullRequestReviewInfo, ReactionInfo, @@ -276,7 +277,7 @@ def round_trip_app(proxy_settings: Settings): } ], ) - if path == "/repos/octo/widget/pulls/2/reviews": + if path == "/repos/octo/widget/pulls/2/reviews" and req.method == "GET": return httpx.Response( 200, json=[ @@ -289,6 +290,25 @@ 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}], + ) + if path == "/repos/octo/widget/pulls/2/reviews" and req.method == "POST": + body = json.loads(req.content) + assert body["event"] == "COMMENT" + assert body["comments"] == [{"path": "src/app.py", "line": 12, "side": "RIGHT", "body": "finding"}] + return httpx.Response( + 200, + json={ + "id": 55, + "user": {"login": "robomp-bot"}, + "body": body["body"], + "state": "COMMENTED", + "submitted_at": "2026-01-01T00:00:00Z", + }, + ) if path == "/user": return httpx.Response(200, json={"login": "robomp-bot"}) if path == "/repos/octo/widget/pulls/4" and req.method == "GET": @@ -353,6 +373,19 @@ async def test_round_trip_all_endpoints(round_trip_app) -> None: prs = await client.list_pr_reviews("octo/widget", 2) assert len(prs) == 1 and isinstance(prs[0], PullRequestReviewInfo) + 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" + + submitted = 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"}], + ) + assert submitted.id == 55 + assert await client.get_authenticated_login() == "robomp-bot" existing_pr = await client.get_pull_request("octo/widget", 4) diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index d80f6beea..896a30098 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/python/robomp/tests/test_sandbox.py @@ -393,6 +393,73 @@ def test_ensure_workspace_creates_worktree(tmp_path: Path, upstream_repo: Path) assert ws.artifacts_dir.is_dir() +def test_ensure_workspace_pr_head_uses_detached_pr_ref(tmp_path: Path, upstream_repo: Path) -> None: + contributor = tmp_path / "contributor" + _git(["clone", str(upstream_repo), str(contributor)], cwd=tmp_path) + (contributor / "README.md").write_text("hello from pr\n", encoding="utf-8") + _git(["-C", str(contributor), "add", "README.md"], cwd=tmp_path) + subprocess.run( + ["git", "commit", "-m", "pr change"], + cwd=str(contributor), + check=True, + capture_output=True, + text=True, + env=os.environ + | { + "GIT_AUTHOR_NAME": "c", + "GIT_AUTHOR_EMAIL": "c@t", + "GIT_COMMITTER_NAME": "c", + "GIT_COMMITTER_EMAIL": "c@t", + }, + ) + pr_head = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(contributor), + check=True, + capture_output=True, + text=True, + ).stdout.strip() + _git(["-C", str(contributor), "push", "origin", "HEAD:refs/pull/9/head"], cwd=tmp_path) + + mgr = SandboxManager(tmp_path / "workspaces") + ws = mgr.ensure_workspace( + repo="octo/widget", + number=9, + title="incoming PR", + clone_url=str(upstream_repo), + default_branch="main", + pr_head=9, + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + + head = subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=str(ws.repo_dir), + check=True, + capture_output=True, + text=True, + ).stdout.strip() + symbolic = subprocess.run( + ["git", "symbolic-ref", "--quiet", "--short", "HEAD"], + cwd=str(ws.repo_dir), + capture_output=True, + text=True, + check=False, + ) + pushurl = subprocess.run( + ["git", "config", "--get", "remote.origin.pushurl"], + cwd=str(ws.repo_dir), + capture_output=True, + text=True, + check=False, + ) + assert head == pr_head + assert symbolic.returncode != 0 + assert ws.branch == "review/pr-9" + assert pushurl.returncode != 0 + + def test_chown_workspace_noops_when_not_root(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: calls: list[tuple[list[str], bool]] = [] diff --git a/python/robomp/tests/test_server.py b/python/robomp/tests/test_server.py index 7c1d58678..95678f6d1 100644 --- a/python/robomp/tests/test_server.py +++ b/python/robomp/tests/test_server.py @@ -888,20 +888,20 @@ def test_webhook_rate_limits_unknown_submitter_at_default_cap(rate_limited_setti assert states == ["queued", "queued", "skipped"] -def test_webhook_unmapped_pr_comment_queues_with_pr_key_and_counts_budget( +def test_webhook_incoming_pr_comment_without_directive_skips_without_counting_budget( rate_limited_settings: Settings, ) -> None: app = create_app(rate_limited_settings) with TestClient(app) as client: - queued = _post_pr_issue_comment( + skipped = _post_pr_issue_comment( client, delivery="pr-unmapped", user="stranger", pr_number=900, association="NONE", ) - assert queued.status_code == 202 - assert queued.json()["state"] == "queued" + assert skipped.status_code == 202 + assert skipped.json()["state"] == "skipped" states = [] for i in range(3): @@ -921,8 +921,8 @@ def test_webhook_unmapped_pr_comment_queues_with_pr_key_and_counts_budget( assert unmapped is not None assert unmapped.issue_key == "octo/widget#900" - assert unmapped.last_error is None - assert states == ["queued", "skipped", "skipped"] + assert "incoming PR comments ignored" in (unmapped.last_error or "") + assert states == ["queued", "queued", "skipped"] def test_webhook_contributor_gets_higher_cap(rate_limited_settings: Settings) -> None: @@ -1554,6 +1554,7 @@ class _RecordingSandbox: clone_url: str, default_branch: str, existing_branch=None, + pr_head: int | None = None, author_name: str = "", author_email: str = "", slot_uid: int | None = None, @@ -1565,6 +1566,7 @@ class _RecordingSandbox: "title": title, "default_branch": default_branch, "existing_branch": existing_branch, + "pr_head": pr_head, "slot_uid": slot_uid, } ) @@ -1581,7 +1583,7 @@ class _RecordingSandbox: wid = f"{repo.replace('/', '__')}__{number}" return _W( - branch=existing_branch or f"farm/auto/{wid}", + branch=existing_branch or (f"review/pr-{pr_head}" if pr_head is not None else f"farm/auto/{wid}"), session_dir=self.tmp_root / wid / "session", context_dir=self.tmp_root / wid / "context", repo_dir=self.tmp_root / wid / "repo", @@ -1823,6 +1825,198 @@ async def test_handle_pr_conversation_repairs_missing_pr_mapping_from_branch( close_database() +async def test_handle_pr_conversation_skips_review_workspace_rows( + settings: Settings, tmp_path: Path, stub_run_task, monkeypatch +) -> None: + from robomp import tasks + from robomp.github_client import GitHubClient, PullRequestInfo + + sandbox = _RecordingSandbox(tmp_path) + db = get_database(settings.sqlite_path) + db.upsert_issue( + key="octo/widget#900", + repo="octo/widget", + number=900, + state="reviewing", + branch="review/pr-900", + pr_number=900, + ) + + async def _get_pull_request(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return PullRequestInfo( + repo="octo/widget", + number=900, + html_url="https://github.com/octo/widget/pull/900", + head_ref="contrib/fix", + base_ref="main", + state="open", + author="alice", + head_repo="alice/widget", + ) + + monkeypatch.setattr(GitHubClient, "get_pull_request", _get_pull_request) + + payload = { + "action": "created", + "issue": { + "number": 900, + "user": {"login": "alice"}, + "pull_request": {"url": "https://api.github.com/repos/octo/widget/pulls/900"}, + }, + "comment": {"user": {"login": "can1357"}, "body": "@robomp-bot please re-review", "id": 12}, + "repository": {"full_name": "octo/widget"}, + "_robomp_directive": {"body": "please re-review", "author": "can1357"}, + } + await tasks.handle_pr_conversation( + settings=settings, + db=db, + github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), + sandbox=sandbox, + payload=payload, + delivery_id="test-pr-review-row", + ) + + assert stub_run_task == [] + assert sandbox.ensure_calls == [] + close_database() + + +async def test_review_pr_retries_when_ranked_but_not_submitted( + settings: Settings, tmp_path: Path, stub_run_task, monkeypatch +) -> None: + from robomp import tasks + from robomp.github_client import GitHubClient, IssueInfo, PullRequestInfo, RepoInfo + + sandbox = _RecordingSandbox(tmp_path) + db = get_database(settings.sqlite_path) + repo = RepoInfo( + full_name="octo/widget", default_branch="main", clone_url="https://github.com/octo/widget.git", private=False + ) + issue = IssueInfo( + repo="octo/widget", + number=900, + title="Fix parser", + body="body", + state="open", + author="alice", + labels=("triaged", "review:p1"), + is_pull_request=True, + ) + pr = PullRequestInfo( + repo="octo/widget", + number=900, + html_url="https://github.com/octo/widget/pull/900", + head_ref="alice/fix-parser", + base_ref="main", + state="open", + author="alice", + head_repo="alice/widget", + ) + + async def _get_repo(self, repo_full: str): + assert repo_full == "octo/widget" + return repo + + async def _get_issue(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return issue + + async def _get_pull_request(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return pr + + monkeypatch.setattr(GitHubClient, "get_repo", _get_repo) + monkeypatch.setattr(GitHubClient, "get_issue", _get_issue) + monkeypatch.setattr(GitHubClient, "get_pull_request", _get_pull_request) + + await tasks.review_pr( + settings=settings, + db=db, + github=GitHubClient("t"), + sandbox=sandbox, + git_transport=LocalGitTransport(token=None), + payload={"pull_request": {"number": 900}, "repository": {"full_name": "octo/widget"}}, + delivery_id="d-review-retry", + ) + + assert len(stub_run_task) == 1 + assert stub_run_task[0]["task_kind"] == "review_pr" + assert sandbox.ensure_calls[0]["pr_head"] == 900 + close_database() + + +async def test_review_pr_skips_after_submitted_review( + settings: Settings, tmp_path: Path, stub_run_task, monkeypatch +) -> None: + from robomp import tasks + from robomp.github_client import GitHubClient, IssueInfo, PullRequestInfo, RepoInfo + + sandbox = _RecordingSandbox(tmp_path) + db = get_database(settings.sqlite_path) + key = issue_key("octo/widget", 900) + db.log_tool_call(issue_key=key, tool="submit_pr_review", args={"body": "done"}, result={"review_id": 12}) + repo = RepoInfo( + full_name="octo/widget", default_branch="main", clone_url="https://github.com/octo/widget.git", private=False + ) + issue = IssueInfo( + repo="octo/widget", + number=900, + title="Fix parser", + body="body", + state="open", + author="alice", + labels=("triaged", "review:p1"), + is_pull_request=True, + ) + pr = PullRequestInfo( + repo="octo/widget", + number=900, + html_url="https://github.com/octo/widget/pull/900", + head_ref="alice/fix-parser", + base_ref="main", + state="open", + author="alice", + head_repo="alice/widget", + ) + + async def _get_repo(self, repo_full: str): + assert repo_full == "octo/widget" + return repo + + async def _get_issue(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return issue + + async def _get_pull_request(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return pr + + monkeypatch.setattr(GitHubClient, "get_repo", _get_repo) + monkeypatch.setattr(GitHubClient, "get_issue", _get_issue) + monkeypatch.setattr(GitHubClient, "get_pull_request", _get_pull_request) + + await tasks.review_pr( + settings=settings, + db=db, + github=GitHubClient("t"), + sandbox=sandbox, + git_transport=LocalGitTransport(token=None), + payload={"pull_request": {"number": 900}, "repository": {"full_name": "octo/widget"}}, + delivery_id="d-review-skip", + ) + + assert stub_run_task == [] + assert sandbox.ensure_calls == [] + close_database() + + async def test_handle_comment_directive_bootstraps_untriaged_issue( settings: Settings, tmp_path: Path, stub_run_task, monkeypatch ) -> None: diff --git a/python/robomp/tests/test_worker.py b/python/robomp/tests/test_worker.py index cbdb1ccc6..28108c22b 100644 --- a/python/robomp/tests/test_worker.py +++ b/python/robomp/tests/test_worker.py @@ -659,10 +659,74 @@ async def test_run_rpc_skips_reminder_when_unclassified(tmp_path: Path, settings assert len(fake.prompts) == 1 +@pytest.mark.asyncio +async def test_run_rpc_review_pr_reminds_until_submit_pr_review(tmp_path: Path, settings: Settings) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False) + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="review_pr", + prompt="kickoff", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + fake = _FakeRpcClient.instances[0] + assert len(fake.prompts) == 1 + settings.task_completion_max_reminders + assert fake.prompts[0] == "kickoff" + assert all("submit_pr_review" in p for p in fake.prompts[1:]) + assert all("gh_open_pr" not in p for p in fake.prompts[1:]) + + +@pytest.mark.asyncio +async def test_run_rpc_review_pr_stops_after_submit_without_dirty_probe( + tmp_path: Path, settings: Settings, monkeypatch: pytest.MonkeyPatch +) -> None: + inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False) + + def _probe(_workspace, _slot_uid): # type: ignore[no-untyped-def] + raise AssertionError("review_pr must not run dirty-state probes") + + monkeypatch.setattr(worker, "_probe_workspace_dirty", _probe) + original_on_tool_end = _FakeRpcClient.on_tool_execution_end + + def _record_tool_end(self, cb) -> None: + self._tool_end_callbacks = getattr(self, "_tool_end_callbacks", []) + self._tool_end_callbacks.append(cb) + + def _on_prompt(client: _FakeRpcClient, _prompt: str) -> None: + for cb in client._tool_end_callbacks: + cb(SimpleNamespace(tool_name="submit_pr_review", result={})) + + _FakeRpcClient.on_tool_execution_end = _record_tool_end # type: ignore[assignment] + try: + _FakeRpcClient.on_prompt = staticmethod(_on_prompt) # type: ignore[attr-defined] + loop = asyncio.new_event_loop() + try: + worker._run_rpc_blocking( + inputs, + task_kind="review_pr", + prompt="kickoff", + loop=loop, + bindings=bindings, # type: ignore[arg-type] + ) + finally: + loop.close() + finally: + _FakeRpcClient.on_tool_execution_end = original_on_tool_end # type: ignore[assignment] + delattr(_FakeRpcClient, "on_prompt") + + fake = _FakeRpcClient.instances[0] + assert fake.prompts == ["kickoff"] + + # --------------------------------------------------------------------------- # Dirty-state watchdog # --------------------------------------------------------------------------- + @pytest.mark.asyncio async def test_run_rpc_sends_dirty_state_reminder_when_worktree_has_unpushed_work( tmp_path: Path, settings: Settings, monkeypatch: pytest.MonkeyPatch