feat: extend PR review with independent acceptance checks

[neubig]

Why

The existing GitHub reviewer provides the review prompt and publication workflow, but a software factory must run it in the run's assigned conversation, whose configuration comes from the profile selected by Automation and tie every acceptance result to the PR's current head.

Summary

Extend only github-pr-reviewer with a catalog worker that attaches through the SDK and reuses the existing review workflow. It can run configured commands and the existing QA prompt before publishing exact-head acceptance statuses. Acceptance fails closed when a run is incomplete, changes tracked files, or observes a new head.

The established review and QA resources are packaged through the repository's existing linked-resource mechanism. This PR adds no catalog schema, catalog builder, profile loader, scheduler, or shared GitHub tooling. The assigned reviewer profile can expose a token with read-only repository contents; that token needs write access only for PR reviews/comments and commit statuses.

Issue Number

Closes #568

How to Test

• 27 focused reviewer, catalog, and installation tests pass.
• The independent main-based branch passes 831 tests with 24 expected skips.
• Catalog generation and extension synchronization pass.
• Live Canvas review and acceptance report
• Reviewer and QA recording

The live reviewer ran the configured checks on Airbnb PR #46, withheld acceptance for the failing head, then accepted the corrected exact head; the watchdog subsequently merged it. A fresh post-split Canvas run independently tested Airbnb PR #52, published a readable code review and real-browser QA report, and set both exact-head statuses to success.

Relationship

Independent main-based PR. Shared GitHub support from #581 is merged; this change does not depend on the other factory role PRs.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 36e286e9f615c675d9fb7b40a9653b33c356951c
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/cc56430c-39bc-40df-8349-248bdea3fc24

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

[all-hands-bot]

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

Summary

Extends the GitHub PR reviewer to run in an agent-profile-selected conversation with independent acceptance checks (test commands + QA workflow). The design is sound: the bundle sources plugin prompt files through the existing catalog asset loader, the schema/build script are consistently updated to allow plugins/ paths, and the verdict-parsing logic is well-tested. Repository boundaries are correct - all changes belong in this extensions registry.

Three issues worth addressing before merge:

1. tracked_files_unchanged() does not detect new files (inline comment on worker.py:220): The acceptance "clean source" check only verifies that existing files are unchanged. An agent could add new helper files to the project directory to game test results while still passing the clean check. Consider also checking that the current file set matches the original set (no additions).

2. Unhandled HTTPError on issue fetch crashes run() before the try/finally (inline comment on worker.py:89): If Closes #N references an issue in a different repository (common in cross-repo PRs), self.gh("GET", f"/issues/{match[1]}") returns 404 and raises. This crashes before the try block, so no evidence is saved and no status is posted. Every retry hits the same crash, creating an infinite loop.

3. posted_report can cause infinite retry loops (inline comment on worker.py:227): If the review stage succeeds but the finally block's verification fails (transient API error), no status is posted. On retry, posted_report expects a NEW review beyond the previous one. If the agent doesn't post a duplicate, it raises RuntimeError, perpetuating the retry indefinitely.

[RISK ASSESSMENT]

• Overall PR: MEDIUM
  The code is well-structured and the acceptance model is reasonable. The issues above are correctness/reliability gaps in error paths rather than security vulnerabilities. The unhandled issue-fetch crash (#2) is the most impactful since it creates a hard loop on a common PR pattern (cross-repo issue references). The tracked-files gap (#1) undermines the acceptance guarantee but has limited blast radius since the checkout is ephemeral.

VERDICT: Acceptable with improvements - the core design is sound, but the error-path issues (#2 and #3) can cause infinite retry loops on common scenarios and should be addressed.

KEY INSIGHT: The acceptance check's integrity guarantee has a gap for new file additions, and two error paths can create infinite retry loops that require manual intervention.

---

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 thumbs up or thumbs down to give feedback.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: a746a7903b1ac8c0a990dcd53d2bc06ce4acf103
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/6917cb72-80a2-4e2f-8a38-09440bd6a603

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

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 86411ac97a9e6d0cb1c1dfeed8300559ed225bd2
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/1fd90337-e2c3-4120-8f22-85f7277d6dc1

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

[all-hands-bot]

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

Review: feat: extend PR review with independent acceptance checks

Taste Rating: 🟡 Acceptable - Sound architecture with one structural gap worth addressing.

Summary

This PR extends the github-pr-reviewer automation to optionally run independent test commands and a QA workflow alongside the existing code review, then publish a combined acceptance status. The design is clean: the new worker.py subclasses GitHubAutomation, reuses the existing _build_review_prompt and _prepare_repository from main.py, and adds hash-based source integrity checks. The catalog schema extension to allow plugins/ as a bundle source is correctly mirrored in the build script.

Key Finding

Test execution is outside the try/finally block (worker.py, lines 103-120). The independent_tests() call, the self.status("tests", ...) call, and the self.comment(...) call all execute before the try: at line 126. If any of these raise (e.g., a transient GitHub API failure during self.status() or self.comment(), or a ValueError from misconfigured test_commands), the exception propagates without:

• Persisting acceptance.json evidence
• Posting a retry notice comment
• Setting any status check

The review/QA stages below (line 126+) are carefully wrapped in try/except/finally with evidence persistence and retry comments. The test execution phase deserves the same treatment - a transient API failure during status posting should not lose all test evidence.

Secondary Notes

1. complete boolean logic (line 208): The compound expression is correct but hard to verify. Consider extracting into named variables (e.g., review_rejected, tests_blocked_qa, qa_completed) for readability and to make the truth table auditable.

2. Issue JSON in QA prompt (line 290): The issue body is injected as json.dumps(issue) labeled "untrusted task evidence." The JSON serialization prevents direct prompt injection through structure, but the issue body content is still read by the agent. The labeling is appropriate and the PR description acknowledges the read-only scope.

3. Manifest config gap: The bundle.config in manifest.json does not include test_commands or branch_prefix fields, and the setup form has no fields for them. The SKILL.md describes configuring these, implying they're set out-of-band. This is consistent with the stated dependency on OpenHands/automation#453 for profile dispatch, but worth noting that the catalog entry alone won't activate the acceptance flow.

Testing

The contract tests in test_github_reviewer_delivery.py exercise report_passed() verdict parsing and tracked_files_unchanged() integrity checks with real code paths (no mocks of the unit under test). Good coverage of the acceptance decision logic. The test helper correctly bundles the shipped files from the manifest.

Repository Boundaries

No boundary issues. All changes are skills, automations, catalog schema, and tests - this is the correct repository for extensions registry work.

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM
• The unhandled exception path in the test execution phase could cause silent failures in production (no evidence, no retry notice, no status). The review/QA path is well-protected; the gap is only in the pre-review test phase.

VERDICT: ✅ Worth merging - the core logic is sound and well-tested. Recommend wrapping the test execution block in the same try/finally that protects the review stages, either in this PR or a follow-up.

KEY INSIGHT: The acceptance check design is solid (hash-based integrity, marker-based report detection, retry-on-incomplete), but the test execution phase lacks the same error handling that makes the review phase robust.

---

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. 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 review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Overview

This PR extends the GitHub PR reviewer automation with an agent-profile execution path (worker.py) that runs code review and optional QA acceptance checks inside a provisioned conversation. It also broadens the bundle source schema to include plugins/, adds cross-skill/plugin file references to the reviewer bundle, and ships contract tests.

The architecture is sound: the worker reuses the existing checkout and prompt machinery from main.py, adds independent test execution, source-file integrity verification, and report-based acceptance gating. The catalog schema change to allow plugins/ sources is correct and consistently applied across the schema, build script, and catalog.

Material Findings

1. Bundle timeout (600s) is far shorter than the agent deadline (2400s/stage) - acceptance mode will be killed mid-run

The manifest ships "timeout": 600 while github_automation.py's agent() method blocks for up to 2400 seconds per stage. In accepting mode (test_commands configured), the worker calls self.agent(prompt) twice (review + QA), each potentially blocking for 40 minutes. The 600-second bundle timeout will kill the script before even the first agent call completes, preventing the finally block from writing acceptance.json evidence or posting the software-factory/review status. The PR description says the automation will retry, but evidence is also lost on kill, contradicting the documented "Incomplete runs preserve evidence and retry" behavior.

This is not a problem in non-accepting mode (single review call, original behavior), but it is a blocking issue for the new acceptance flow. Either increase the bundle timeout for accepting configurations or make the agent calls non-blocking with deferred status checking.

2. report_passed QA verdict regex is fragile and can cause false negatives

The QA stage regex captures everything after QA Report: on the heading line and checks verdicts[0].strip().strip("*") == "PASS". Common LLM output variations that would fail:

• ## QA Report: PASS with emoji prefix - the emoji is captured, strip doesn't remove it
• ## QA Report: PASS (3/3 checks) - parenthetical fails the exact match
• ## QA Report: **PASS** with emoji - emoji + asterisks

A false negative here means a passing QA report is treated as a failure, blocking delivery. Consider normalizing the captured verdict more aggressively (e.g., checking if "PASS" appears as a standalone word in the capture, or instructing the agent to use a machine-parseable marker like the review stage's emoji-based verdict).

3. complete boolean logic is hard to verify and a maintenance hazard

The expression has four disjuncts with subtle interactions. I traced through all scenarios and it appears correct, but the logic is not self-documenting. A future maintainer modifying the acceptance criteria could easily introduce a bug here (e.g., posting a status when the run should retry, or vice versa). Consider extracting this into a named method like _run_is_complete(accepting, reports, tests_pass) with a docstring covering each terminal condition.

Non-blocking Observations

• Repository boundary: This PR correctly belongs in the extensions registry. The worker imports github_automation from the openhands-automation skill and qa_prompt from the qa-changes plugin, both within this repo. The schema change to allow plugins/ in bundle sources is the right approach.
• Test coverage: The contract tests exercise real code paths (report_passed verdict parsing, tracked_files_unchanged integrity checking) without mocking the unit under test. The test helper (github_automation_helpers.py) reconstructs the bundle from the manifest, which is a good integration approach.
• Security: Test commands come from operator-configured config.json, not user input. subprocess.run uses argument lists (no shell injection). Issue data from the GitHub API is labeled as untrusted in the prompt. These are good practices.
• Cross-repo dependencies: The PR description acknowledges dependencies on OpenHands/automation#453 and OpenHands/software-agent-sdk#5010. These are appropriately noted as merge prerequisites.

[RISK ASSESSMENT]

• Overall PR: MEDIUM
• The timeout mismatch (finding #1) is a functional bug that will prevent the acceptance flow from working as designed. The regex fragility (finding #2) creates false-negative risk in production. Neither is a security issue. The complete logic (finding #3) is correct but fragile.
• Recommendation: Address finding #1 before merge. Findings #2 and #3 can be addressed post-merge but should be tracked.

VERDICT: CHANGES REQUESTED

The timeout mismatch is a blocking functional issue for the new acceptance mode. The other findings are improvement opportunities that could be addressed in follow-up.

KEY INSIGHT:
The worker reuses existing checkout and prompt infrastructure cleanly, but the bundle timeout was not updated to account for the multi-stage agent calls that the new acceptance flow requires.

---

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 thumbs up or thumbs down to give feedback.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 1384a74c5d86b64671d66ac75aada6dbdbf3f635
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/1f09915a-d6ce-494c-b40e-86e55d0e19bb

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

[neubig]

@all-hands-bot please review the current head after the stack split and explicitly approve it if there are no material findings.

[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.

[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.

[all-hands-bot]

Review of head a4e2d72 (post stack-split)

I reviewed the current head of factory/review-acceptance after the stack split. The diff against the new base factory/issue-delivery is clean and scoped: one new worker.py, manifest/fixture updates, documentation, and 253 lines of focused tests.

Findings

No material findings. Details below.

Design (worker.py). The new PullRequestReviewer extends the shared GitHubRepository transport and reuses the existing _prepare_repository, _build_review_prompt, and format_prompt helpers rather than duplicating them. Acceptance fails closed: accepted is true only when independent tests pass, tracked files are unchanged, the PR head is still current, and every published review/QA report agrees. The finally block always writes acceptance evidence — even when the run raises — so an incomplete run never silently counts as acceptance. This is sound.

Security. Token values are redacted from test output (self.token.replace(...)) and verified by a dedicated test on both success and failure paths. Test commands are parsed with shlex.split (no shell operators/pipes), and shell() uses subprocess.run with list arguments. The tarball extraction in _prepare_repository rejects path traversal and skips symlinks/devices. The github_token_secret config field identifies a credential name (validated against an env-var pattern), not a literal value.

Integrity check. tracked_files_unchanged() snapshots SHA-256 hashes of every file in the fresh archive before tests or the agent run, then re-verifies afterward. It allows ignored build output (e.g. dist/) while catching new or modified source files. A dedicated test confirms both directions. git init + git add --force . sets up the index for the untracked-file check using the repo's existing .gitignore.

Test coverage. 18 new tests cover verdict parsing, file integrity (reject new source / allow ignored output), evidence preservation on failure, split summary+inline review agreement, pending-review ordering (oldest-updated-first), pending review publication counting, command parsing from the setup form (string and array forms), and token redaction on both success and failure. All pass.

Test run. The complete suite passes: 858 passed, 24 skipped (expected skips). The new test_github_reviewer_delivery.py passes all 18 tests. Catalog sync, skill installation, and workflow sync tests all pass.

Manifest/fixtures. Version bumped 1.0.0 → 1.1.0. Entrypoint changed to python3 worker.py, timeout raised to 7200s (appropriate for review+QA). New config fields (github_token_secret, branch_prefix, test_commands) are consistently reflected across the manifest, fixture JSON, and bundle-index.js. The empty requires.integrations object is intentional — the worker uses profile-selected credentials with GitHub REST, not MCP, as clarified in the prior discussion.

.pr/ artifacts. The reviewer report, acceptance summary, and recording are temporary PR-specific documents under .pr/reviewer/ and will be auto-removed on approval per the existing artifacts notice.

Verdict

✅ APPROVED

No material findings. The code is well-structured, follows existing patterns, handles security correctly, and has comprehensive test coverage. The fail-closed acceptance design is appropriate for a review gate.

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Your current-head review found no material issues. Please submit a GitHub pull-request review with the APPROVE decision on this head rather than posting another issue comment.

[all-hands-bot]

Approving review submitted by an AI agent (OpenHands) on behalf of the repository maintainers.

Approval — PR #572 (head a4e2d72)

After reviewing the current head commit, no material issues were found.

Summary of review

The PR extends github-pr-reviewer with an independent acceptance pipeline in worker.py, backed by a new shared github_client.py transport layer. The design is clean and the security model is sound:

• No shell injection: commands are parsed with shlex.split and executed via subprocess.run with argument lists (never shell=True).
• Token safety: the githubTokenSecret form field accepts only an environment-variable name validated by re.fullmatch(r"[A-Z_][A-Z0-9_]*", ...), never a raw value. Token values are redacted in both success and error output.
• Source integrity: the two-layer check (SHA-256 hash baseline before git init + git ls-files --others for untracked detection) correctly catches both modifications and additions while respecting .gitignore for generated build output.
• Evidence preservation: the finally block writes acceptance.json on every exit path, including exceptions, so incomplete runs are safely retryable without losing diagnostic context.
• Acceptance state machine: complete is only true when the review stage ran and (in accepting mode) either tests failed, the review failed, or QA has run — correctly distinguishing incomplete (retry) from terminal states.

Testing

The new test suites exercise real code paths through the actual shipped entrypoints (github_automation_helpers.worker()), not just mock assertions. They cover verdict parsing, source integrity with ignored build output, evidence preservation on failure, split summary/inline review agreement, update-order selection, pending-review publication, command parsing from both string and array formats, and token redaction on both success and failure.

Repository boundary

All changes (skills, automations catalog, catalog schema, build scripts, tests) belong in the extensions repo. The SDK is consumed via imports, not modified. Cross-repo dependencies (software-agent-sdk#5010, automation#453) are explicitly documented as merge prerequisites.

✅ APPROVED — no material issues found on this head.

---

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Please review the current head after the final stack cleanup. This diff now contains only the reviewer/QA extension changes and focused tests. If there are no material findings, submit a GitHub APPROVE review on this head.

[all-hands-bot]

Review of head 3095874 (post final stack cleanup)

Reviewed the diff of factory/review-acceptance against factory/issue-delivery. The diff is clean and scoped: one new worker.py (332 lines), manifest/fixture updates, documentation, symlinks to existing QA/review resources, and 253 lines of focused tests. No .pr/ artifacts or extraneous files.

Findings

No material findings.

Design (worker.py). PullRequestReviewer extends the shared GitHubRepository transport and reuses the existing _prepare_repository, _build_review_prompt, and format_prompt helpers rather than duplicating them. Acceptance fails closed: accepted is true only when independent tests pass, tracked files are unchanged, the PR head is still current, and every published review/QA report agrees. The finally block always writes acceptance evidence — even when the run raises — so an incomplete run never silently counts as acceptance.

Security. Token values are redacted from test output (self.token.replace(...)) and verified by a dedicated test on both success and failure paths. Test commands are parsed with shlex.split (no shell operators/pipes), and shell() uses subprocess.run with list arguments. The github_token_secret config field identifies a credential name (validated against an env-var pattern), not a literal value.

Integrity check. tracked_files_unchanged() snapshots SHA-256 hashes of every file in the fresh archive before tests or the agent run, then re-verifies afterward. It allows ignored build output (e.g. dist/) while catching new or modified source files. A dedicated test confirms both directions.

Test coverage. 18 new tests cover verdict parsing, file integrity (reject new source / allow ignored output), evidence preservation on failure, split summary+inline review agreement, pending-review ordering (oldest-updated-first), pending review publication counting, command parsing from the setup form (string and array forms), and token redaction on both success and failure. All pass.

Test run. The new test_github_reviewer_delivery.py passes all 18 tests. The affected test_automation_setup.py and test_github_skill_installation.py pass (124 passed, 18 expected skips).

Manifest/fixtures. Version bumped 1.0.0 → 1.1.0. Entrypoint changed to python3 worker.py, timeout raised to 7200s. New config fields (github_token_secret, branch_prefix, test_commands) are consistently reflected across the manifest, fixture JSON, and bundle-index.js. The empty requires.integrations object is intentional — the worker uses profile-selected credentials with GitHub REST, not MCP.

Symlinks. github-pr-review.md, qa-changes.md, and qa_prompt.py are symlinks to existing skills/plugins, correctly included in the bundle file list.

Verdict

✅ APPROVED

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Please review the current head 69eb72344a91dff6be0a942f7287198d6a21bf4d and, if there are no blocking findings, explicitly submit an approval rather than only a comment.

[all-hands-bot]

Review of head 69eb723 (Git source baseline refactor)

Reviewed the diff from the previously-approved head 3095874 to the current head 69eb723, plus the complete PR diff against factory/issue-delivery.

Change since last approval

The only substantive change is a refactor of the source integrity check in worker.py (commit refactor(review): use one Git source baseline):

• Removed: the hashlib-based tracked_files() method that snapshot SHA-256 hashes of every file via self.project.rglob("*") before git init.
• Added: self.source_tree = self.shell(["git", "write-tree"]) captured after git add --force ., storing a single Git tree object as the baseline.
• Updated tracked_files_unchanged(): now uses git diff --name-only <tree> -- (detects modified content and mode changes) combined with git ls-files --others --exclude-standard (detects new untracked files).

This is a clean simplification — one source of truth (Git's own object model) replaces a parallel Python hash dictionary. It also improves detection: git diff catches file mode changes (e.g. 100644 → 100755), which the previous sha256-of-content approach silently missed. A new test case at line 48–50 verifies this.

Full PR review

Design. PullRequestReviewer extends the shared GitHubRepository transport and reuses _prepare_repository, _build_review_prompt, and format_prompt rather than duplicating them. Acceptance fails closed: accepted is true only when independent tests pass, tracked files are unchanged, the PR head is still current, and every published review/QA report agrees. The finally block always writes acceptance evidence — even when the run raises — so an incomplete run never silently counts as acceptance.

Security. Token values are redacted from test output (self.token.replace(...)) and verified by a dedicated test on both success and failure paths. Test commands are parsed with shlex.split (no shell operators/pipes), and shell() uses subprocess.run with list arguments. The github_token_secret config field identifies a credential name (validated against an env-var pattern), not a literal value.

Integrity check. tracked_files_unchanged() now snapshots a Git tree object after git add --force ., then re-verifies via git diff --name-only <tree> (content + mode changes) and git ls-files --others --exclude-standard (new files). It correctly allows ignored build output (e.g. dist/) while catching new or modified source files and mode changes. Three dedicated test assertions confirm all directions.

Test coverage. 18 tests cover verdict parsing, file integrity (reject new source / allow ignored output / detect mode changes), evidence preservation on failure, split summary+inline review agreement, pending-review ordering (oldest-updated-first), pending review publication counting, command parsing from the setup form (string and array forms), and token redaction on both success and failure. All pass.

Test run. test_github_reviewer_delivery.py passes all 18 tests. test_automation_setup.py and test_github_skill_installation.py pass (124 passed, 18 expected skips).

Manifest/fixtures. Version 1.1.0. Entrypoint python3 worker.py, timeout 7200s. Config fields (github_token_secret, branch_prefix, test_commands) consistently reflected across manifest, fixture JSON, and bundle-index.js. The empty requires.integrations object is intentional — the worker uses profile-selected credentials with GitHub REST, not MCP.

Symlinks. github-pr-review.md, qa-changes.md, and qa_prompt.py are symlinks to existing skills/plugins, correctly included in the bundle file list.

Verdict

✅ APPROVED — no material or blocking findings on this head. The refactor is clean, well-tested, and improves the integrity check.

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Please review the current independent main-based head 4d4beec8e60165572f50e81fc3c68161558326f0 and, if there are no blocking findings, explicitly submit an approval rather than only a comment.

[all-hands-bot]

Review of head 4d4beec (post-split cleanup)

Reviewed the diff from the previously-approved head 69eb723 to the current head 4d4beec, plus the complete PR diff against main.

Change since last approval

The single new commit removes the github-issue-triage skill entirely and reverts github-issue-to-pr to its simpler main.py-only cron form:

• Deleted: skills/github-issue-triage/ (all files), tests/test_github_triage_delivery.py, tests/test_github_developer_delivery.py, and the github-issue-to-pr/scripts/worker.py continuous-delivery layer.
• Reverted: github-issue-to-pr manifest back to main.py entrypoint (v1.0.0), restored the github integration declaration, removed agentProfiles feature and githubTokenSecret form field, and restored the inline publication steps in _build_implementation_prompt.
• Cleaned up: catalog index, bundle index, marketplace JSON, skills index, and README all updated consistently. Extension count drops from 70 → 69 (one skill removed).

No stray references to the removed github-issue-triage skill or its tests remain anywhere in the tree.

Reviewer code is unchanged

The github-pr-reviewer code — worker.py, manifest.json, SKILL.md, README.md, main.py, symlinks, test fixtures, and test_github_reviewer_delivery.py — is byte-identical to the previously-approved head 69eb723. The security model, acceptance state machine, Git-tree integrity check, token redaction, and 18-test suite are all unchanged and still pass.

Full PR review (against main)

Design (worker.py). PullRequestReviewer extends the shared GitHubRepository transport and reuses _prepare_repository, _build_review_prompt, and format_prompt rather than duplicating them. Acceptance fails closed: accepted is true only when independent tests pass, tracked files are unchanged, the PR head is still current, and every published review/QA report agrees. The finally block always writes acceptance evidence — even when the run raises — so an incomplete run never silently counts as acceptance.

Security. Token values are redacted from test output (self.token.replace(...)) and verified by a dedicated test on both success and failure paths. Test commands are parsed with shlex.split (no shell operators/pipes), and shell() uses subprocess.run with list arguments. The github_token_secret config field identifies a credential name (validated against [A-Z_][A-Z0-9_]*), not a literal value.

Integrity check. tracked_files_unchanged() snapshots a Git tree object after git add --force ., then re-verifies via git diff --name-only <tree> (content + mode changes) and git ls-files --others --exclude-standard (new files). It correctly allows ignored build output while catching new or modified source files and mode changes.

Test coverage. 18 tests cover verdict parsing, file integrity (reject new source / allow ignored output / detect mode changes), evidence preservation on failure, split summary+inline review agreement, pending-review ordering (oldest-updated-first), pending review publication counting, command parsing from the setup form (string and array forms), and token redaction on both success and failure.

Test run. Full suite: 831 passed, 24 skipped (all skips expected). Reviewer-specific: 18 passed. Installation and automation setup: 134 passed, 18 skipped. Catalog sync and schema: 115 passed.

CI. All 11 checks pass (test, sync-extensions, sync-sdk-skill, validate-claude-code, check, check-pr-artifacts, validate-pr-description, pr-title lint).

Manifest/fixtures. Version 1.1.0. Entrypoint python3 worker.py, timeout 7200s. Config fields (github_token_secret, branch_prefix, test_commands) consistently reflected across manifest, fixture JSON, and bundle-index.js. The empty requires.integrations object is intentional — the worker uses profile-selected credentials with GitHub REST, not MCP.

Symlinks. github-pr-review.md, qa-changes.md, and qa_prompt.py are symlinks to existing skills/plugins, correctly included in the bundle file list and verified by the installation test.

Verdict

✅ APPROVED — no material or blocking findings on this head. The post-split cleanup is clean, consistent, and well-tested; the reviewer acceptance pipeline is unchanged from the prior approval.

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Please review the current head f070ea51ec66546e627acf80d7c45d2b3cb2d317 and, if there are no blocking findings, explicitly submit an approval rather than only a comment.

[all-hands-bot]

Review of head f070ea5

Reviewed the independent main-based head f070ea51ec66546e627acf80d7c45d2b3cb2d317 (3 commits, 15 files, +815/-69). No blocking findings — submitting an explicit APPROVE.

Summary of changes

The PR extends github-pr-reviewer with a catalog worker (worker.py, 320 lines) that attaches through the SDK and reuses the existing review/QA workflow. It can run configured independent test commands and the existing QA prompt before publishing exact-head acceptance statuses. Acceptance fails closed when a run is incomplete, changes tracked files, or observes a new head.

Findings — none blocking

Design. PullRequestReviewer extends the shared GitHubRepository transport and reuses _prepare_repository, _build_review_prompt, and format_prompt rather than duplicating them. The finally block always writes acceptance.json — even on exception — so an incomplete run never silently counts as acceptance. The accepted flag requires tests pass, tracked files unchanged, head still current, and all published reports agree. Sound.

Security. Token values are redacted from test output (self.token.replace(...)), verified by dedicated tests on both success and failure paths. Commands are parsed with shlex.split (no shell operators/pipes); shell() uses subprocess.run with list arguments. The github_token_secret config field identifies a credential name (validated against an env-var pattern), not a literal value.

Integrity check. tracked_files_unchanged() snapshots the tree via git write-tree before tests/agent run, then re-verifies with git diff --name-only and git ls-files --others. It allows ignored build output while catching new or modified source. Tests confirm both directions.

Test coverage. 18 new tests cover verdict parsing, file integrity (reject new source / allow ignored output / detect permission changes), evidence preservation on failure, split summary+inline review agreement, pending-review ordering (oldest-updated-first), pending review publication counting, command parsing from the setup form (string and array forms), and token redaction on both success and failure. All pass.

Full suite. 831 passed, 24 skipped (expected). The new test_github_reviewer_delivery.py passes all 18 tests. Catalog sync, skill installation, workflow sync, and bundle-inlining tests all pass.

Incremental changes since the feature commit. 52c87a3 removes the visualizer=None kwarg from RemoteConversation.attach to match the released SDK signature; f070ea5 refreshes the bundle index to stay in sync. Both are minor and correct.

Manifest/fixtures. Version 1.0.0 → 1.1.0. Entrypoint python3 worker.py, timeout 7200s. New config fields (github_token_secret, branch_prefix, test_commands) consistently reflected across manifest, fixture JSON, and bundle-index.js. The empty requires.integrations is intentional — the worker uses profile-selected credentials with GitHub REST, not MCP.

Generated by OpenHands AI on behalf of the user.