feat: delegate PR review to subject agents

[neubig]

Why

The existing reviewer already provides the established review prompt and native GitHub review workflow. Its host-managed checkout and conversation lifecycle assume a shared filesystem and cannot cleanly fan out several exact PR heads. Review work should run inside each delegated agent's own runtime.

Summary

Keep the established reviewer and add a thin scheduled scanner that:

• selects every labeled exact PR head;
• delegates through the shared KV-backed conversation helper;
• keys conversation identity by immutable repository ID and PR number;
• asks the agent to fetch the exact head, run appropriate tests, publish a readable native review, and record exact-head review and test statuses.

Changed heads remain labeled for a fresh review. The label is removed only after both statuses exist on the reviewed head. The scanner does not parse or repost JSON review artifacts.

Issue Number

Closes #568.

How to Test

uv run --group test pytest -q tests/test_github_reviewer_delivery.py tests/test_automation_setup.py

Result: 127 passed, 18 skipped.

Dependencies

Review and merge #592 first. Running from Canvas with a selected profile additionally requires OpenHands/automation#479. Profile-selected saved secrets require OpenHands/software-agent-sdk#5017; Docker-backed conversations require OpenHands/software-agent-sdk#3403.

Live evidence

The exact head reviewed Airbnb PR #87 in its own Docker conversation. It ran the repository tests and build, posted a readable native review rather than a JSON artifact, published successful exact-head software-factory/tests and software-factory/review statuses, and removed the review label.

[neubig]

@all-hands-bot Please review the current head and explicitly approve it if there are no blocking findings.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: a085b2faee8a73cecb98c275da1c48b390ce8097
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/526de452-e030-472f-987b-cb2ca1ac2a75

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

Review of head a085b2f — ✅ APPROVED

I reviewed the current head of this PR, including all 5 commits, the full diff, and the test suite. Here is a summary of findings.

What this PR does

Refactors the GitHub PR reviewer automation from a monolithic scanner-conversation-manager into a thin scanner + delegated subject agent architecture:

• New worker.py — PullRequestReviewer extends GitHubRepository, polls labeled open PRs, and submits each exact head as an idempotent subject turn ({label_event_id}:{sha}) to the subject-turn API. The scanner never touches a conversation, workspace, runtime credential, or result artifact.
• main.py changes — _build_review_prompt gains two keyword-only parameters (workspace_instructions, github_token_secret) so the worker can inject its own workspace setup instructions (empty repo + gh auth setup-git + detached-HEAD fetch) and the configured token secret name. The old inline behavior is preserved as defaults.
• Manifest v1.1.0 — entrypoint switches to python3 worker.py, timeout raised 600→1800s, agentProfiles capability added, githubTokenSecret form field added, bundle now packs main.py + github_client.py + worker.py.
• Fix commits — non-interactive Git fetch auth (GIT_TERMINAL_PROMPT=0, gh auth setup-git), reviewer validation timeout alignment.

Findings

No blocking findings. Details below:

Area	Assessment
Architecture	Clean separation of concerns. The scanner is stateless w.r.t. conversations and delegates all runtime work to the profile-backed subject agent. Idempotency key ({event_id}:{sha}) and subject key ({repo_id}:pr:{number}) are correctly composed.
Security	The scanner receives only the token secret name (not value) in the prompt text — the actual credential is injected by the profile runtime. test_reviewer_scanner_has_no_conversation_or_runtime_code verifies no conversation/workspace/credential code leaks into the scanner. No hardcoded secrets.
Correctness	PR is re-fetched after listing to get fresh metadata. Latest labeled event is checked to avoid reprocessing. The prompt instructs the agent to re-verify the head SHA before reporting and to leave the label in place if the head changed. Status reporting is keyed to the exact head SHA.
Backward compatibility	_build_review_prompt preserves its original defaults, so the existing main.py standalone path continues to work unchanged.
Tests	All 907 tests pass (24 expected skips). The 3 new delivery tests cover: labeled exact-head submission, unlabeled-PR skipping, and scanner purity (no runtime code). Fixtures and bundle-index are consistent with the manifest.
Bundle consistency	bundle-index.js inlines the correct worker.py, main.py, and github_client.py source; the test_bundle_files_exist_and_are_inlined test validates this.

Minor observations (non-blocking)

1. The main.py module-level docstring (lines 11–14) still describes the old "script owns the repository checkout" model. Since main.py is now primarily a library imported by worker.py, this docstring is slightly stale — but it does not affect runtime behavior.
2. The main.py _CONFIG_TYPES dict does not include github_token_secret, so that key in config.json is silently ignored by load_config. This is intentional (the scanner reads it from self.config directly, and run_repositories resolves it from args/config), but a reader might expect it to be validated alongside the other keys.

Verdict

The code is clean, well-tested, architecturally sound, and introduces no blocking issues. Approving.

Generated by OpenHands AI on behalf of the user.

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR refactors the GitHub PR reviewer automation from a single-process scanner that manages checkouts and conversations directly into a thin scanner that delegates each labeled PR head as an idempotent subject turn to a profile-backed sandboxed agent. The architectural separation is clean: the scanner never touches conversations, workspaces, runtime credentials, or result artifacts. The existing review prompt is reused via _build_review_prompt with new keyword-only parameters for workspace instructions and the token secret name.

Findings

[IMPROVEMENT] Missing per-PR error isolation in the scanner loop

File: skills/github-pr-reviewer/scripts/worker.py, line 88

The run() method iterates over labeled PRs and calls submit_subject_turn for each, but there is no try/except inside the loop. If submit_subject_turn raises (network timeout, 5xx from the subject-turn endpoint, transient DNS), the exception propagates out of run() and is caught by run_repositories at the per-repository level - which means all remaining labeled PRs in that repository are skipped for this scan cycle. The old main.py scanner handled this with per-PR error isolation in _process_repo.

This is not a data-loss issue: labels are not removed until both statuses are visible, so skipped PRs will be picked up on the next scan. But for repositories with many labeled PRs, a single transient failure can delay reviews for all subsequent PRs by one full scan interval (15 minutes by default). Wrapping the per-PR body in a try/except that logs and continues would match the robustness of the previous implementation.

Design Assessment

• Repository boundary: Correct. The scanner, prompt, manifest, and tests all belong in this extensions registry. The subject-turn API and Docker sandbox runtime are in OpenHands/automation and software-agent-sdk respectively, as noted in the PR dependencies.
• Credential handling: The githubTokenSecret form field correctly asks for a secret name (not value), and the prompt references the env var by name (GH_TOKEN=${TOKEN_NAME}) rather than embedding the value. The scanner itself authenticates with self.token for GitHub API reads but never passes the token value into the subject turn - only the name.
• Idempotency: The subject_key ({repository_id}:pr:{pr_number}) and idempotency_key ({label_event_id}:{sha}) are well-chosen: GitHub's immutable repository ID prevents cross-repo collisions, the label event ID prevents duplicate reviews for the same label application, and the SHA ensures a force-pushed head gets a fresh review.
• Test coverage: The delivery tests exercise the real run() code path with mocked I/O boundaries, verify prompt content, and include a static source scan ensuring the scanner has no conversation/runtime code. The test adaptation in test_automation_setup.py (switching the schema-rejection fixture from github-pr-reviewer to github-issue-to-pr) is correct because the reviewer manifest now has empty integrations, which would break the integrations.github.value rejection case.
• Fixture consistency: All fixture entries are internally consistent - version 1.1.0, python3 worker.py entrypoint, 1800s timeout, github_token_secret in config, worker.py in packs.

Risk Assessment

🟢 LOW - The change is well-structured with clean separation of concerns. The scanner is stateless and idempotent. The main robustness concern (per-PR error isolation) is non-blocking: failures delay rather than lose reviews. No security issues found - credential names are used throughout, never values.

Verdict

✅ Worth merging - Core logic is sound. The per-PR error isolation suggestion is an improvement, not a blocker.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

[all-hands-bot]

👋 This PR needs a couple of things fixed before OpenHands can review it:

• the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

[neubig]

@all-hands-bot The current head addresses the per-PR failure-isolation finding and the stale module description. Please review it and submit an APPROVE review if there are no blocking findings.

[all-hands-bot]

Re-review of head 4dbe9c8 — ✅ APPROVED

I re-reviewed the current head, focusing on the two findings from the previous review on a085b2f.

Finding 1: Per-PR failure isolation — ✅ Resolved

worker.py run() now wraps the per-PR body (PR fetch, label-event lookup, submit_subject_turn, and result logging) in a try/except Exception that prints a diagnostic and continues to the next PR. A single transient submission failure no longer propagates to run_repositories, so later labeled PRs in the same repository are still dispatched in the same scan cycle.

The error message uses candidate.get('number', '?') rather than pr["number"], which is correct: if the failure occurs during self.gh("GET", f"/pulls/{...}"), pr may not exist yet, but candidate always does.

A new test, test_reviewer_continues_after_one_submission_fails, exercises the real run() code path with two labeled PRs where the first submission raises RuntimeError and the second succeeds. It asserts submit.call_count == 2, confirming the loop continues past the failure.

Finding 2: Stale module description — ✅ Resolved

The main.py docstring (lines 11–14) was updated to describe the dual role:

This standalone script owns the repository checkout: it downloads the pull request's head commit as a tarball, hands the agent that directory as its workspace, and removes it once the review has finished. Catalog workers may instead reuse its prompt builder with their own workspace instructions.

This correctly reflects that main.py is now both a standalone script and a library whose _build_review_prompt is imported by worker.py. The stale "The agent never clones, checks out, or deletes anything" claim is removed.

Test suite

All 908 tests pass (24 expected skips), including the 4 delivery tests and the full bundle/fixture consistency suite.

Verdict

No blocking findings. Approving.

Generated by OpenHands AI on behalf of the user.