feat(robomp): added incoming PR review feature with classify and submit
- Added `review_pr` task that checks out PR head in a detached worktree, classifies rank/type/area, and posts a batched GitHub review as `event=COMMENT`. - Added four new host tools: `fetch_pr`, `classify_pr`, `pr_review_comment`, and `submit_pr_review`; review tools self-gate on `review_mode`, push/open-PR tools refuse when `review_mode` is set. - Added sqlite staging table `pr_review_comments` with `stage_review_comment`, `list_staged_review_comments`, and `clear_staged_review_comments` DAOs. - Routed `pull_request.opened/reopened/ready_for_review` to `review_pr` and extended `pull_request.closed` cleanup to any tracked PR regardless of author.
This commit is contained in:
can1357
@@ -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.<entry>(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.<prompt>(...)
|
||||
├─ 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/<hex>/<slug>` 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=<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/<n>/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/<n>/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:<name> + _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#<pr_number>
|
||||
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.)
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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/<n>/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",
|
||||
|
||||
@@ -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, ...]: ...
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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:<name>`. 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."
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
<critical>
|
||||
- **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.
|
||||
</critical>
|
||||
|
||||
# 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:<name>` (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="<summary>", 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 —
|
||||
<why>" 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.
|
||||
@@ -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.
|
||||
@@ -0,0 +1,11 @@
|
||||
You are **robomp**, reviewing an incoming pull request on `{{repo.full_name}}`.
|
||||
|
||||
<critical>
|
||||
- **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.
|
||||
</critical>
|
||||
|
||||
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.
|
||||
@@ -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 = [
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 ""),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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})
|
||||
|
||||
@@ -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 <ref>` to ensure the base branch is local."""
|
||||
...
|
||||
|
||||
def fetch_pr_head(self, *, repo: str, pool_dir: Path, pr_number: int) -> None:
|
||||
"""Fetch `refs/pull/<n>/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(
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
|
||||
@@ -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",
|
||||
]
|
||||
|
||||
+34
-19
@@ -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,
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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] = []
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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]] = []
|
||||
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user