fix(factory): reuse canonical implementation, review, and QA workflows

[neubig]

• A human has tested these changes.

---

Why

The initial factory recipe duplicated the existing implementation/review workflows and posted raw acceptance JSON as reviews. It also failed to read native review bodies and inline findings when revising a PR.

Summary

• Compose the canonical issue-to-PR, GitHub PR Reviewer, and QA Changes workflow sources; package their exact source bytes and hashes with one workspace-independent bundle.
• Supply a repository-scoped gh api transport and retain deterministic test execution and guarded publication in the coordinator. The host GitHub credential remains outside workers.
• Require newly published, commit-pinned native review and QA reports with unambiguous passing verdicts; consume reviews and inline feedback during revisions. Keep complete test evidence in workspace artifacts.

Issue Number

Closes #561

How to Test

python -m pytest tests/test_factory_extension_workflows.py tests/test_github_software_factory.py tests/test_github_factory_gateway.py -q — 46 passed.

Ruff format and checks passed for the changed Python files. Live factory validation is in progress. Identical-bundle local/Docker execution was demonstrated in OpenHands/automation#451; this remains a draft.

Notes

Review after #559 and #557 (native stack #560). The workflow layer does not inspect workspace kind. OpenHands/automation#450 tracks the shared dispatcher contract required to supply the same conversation environment in both modes. Existing standalone automation scheduling is unchanged; this recipe reuses their workflow definitions with coordinator-owned scheduling and publication.

Runtime communication now uses the public SDK client from OpenHands/software-agent-sdk#5010; the worker Python needs that SDK build until its release. The same entrypoint and bundle work in local and Docker environments. The 46 focused tests still pass after migration.

Live canonical review verification: the existing PR Reviewer workflow posted a native Markdown review with three inline findings on the target application PR. The developer consumed those findings and published a revised head. Machine-readable acceptance artifacts stayed in the workspace. Independent acceptance of that revision is still pending.

Clean source installation now pins SDK #5010 by immutable commit for the test group. uv run --group test pytest tests/: 853 passed, 26 skipped. Replace the integration pin with the SDK release before merging.

Current validation

Latest fixes supply gateway grants through profile secret references (SDK #5015), validate publication paths, skip closed source issues, make coordinator publication explicit in the canonical builder, preserve pagination filters, and report review failures visibly. The full suite passed 876 tests with 26 skips plus one stale generated bundle check; that check passed after regenerating bundle-index.js. The SDK test pin remains a release dependency before merge.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 52b3a5338ea9c20b88789c91855d0f0e1faee361
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/20328bcf-d4b6-45c2-93c2-8b5e6d6f04a1

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

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 52b3a5338ea9c20b88789c91855d0f0e1faee361
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/634b579d-2b80-459b-82ae-83b7bebb9938

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

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 52b3a5338ea9c20b88789c91855d0f0e1faee361
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/cde9c30c-47a0-474a-9565-758af2ddd01b

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

This PR replaces the factory's duplicated implementation/review/QA prompts with canonical workflow sources from existing skills (github-issue-to-pr, github-pr-reviewer, qa-changes), introduces a scoped gh api transport for workers, and migrates from raw HTTP calls to AgentServerClient. The architecture is sound: credentials stay on the control plane, workers get scoped access, and evidence matching is fail-closed.

Taste Rating: Good taste - The core design eliminates duplication by composing existing canonical workflows rather than maintaining parallel prompts. The scoped transport, provenance hashes, and fail-closed verdict matching are well-considered.

Blocking Concern

Unreleased SDK git-commit pin. pyproject.toml pins openhands-sdk to an unreleased commit from software-agent-sdk#5010. The PR description acknowledges this should be replaced before merging. This blocks merge-readiness: CI for this repo will depend on an immutable-but-unreleased commit until the SDK ships. Replace with the released package version before merging.

Design Observations (non-blocking)

Coupling to private methods. extension_workflows.py calls _build_implementation_prompt, _build_review_prompt, and _load_repo_review_guide on the canonical scripts. These are underscore-prefixed (internal by convention). If those scripts refactor their internal API, the factory breaks silently at runtime - the provenance hash only verifies file bytes, not API stability. Consider promoting these to public functions in the canonical scripts, or adding a thin compatibility check. Not blocking because the SOURCES tuple and provenance hashes at least make the coupling explicit and verifiable.

complete logic is correct but dense. The complete variable in reviewer()'s finally block correctly distinguishes retryable failures (agent crashed, no report posted) from terminal states (review rejected, QA failed). I traced all paths and they behave as intended. The inline comment helps, though the three-way boolean expression requires careful reading.

Security

The security model is well-designed:

• scoped_gh.py properly validates endpoints are within the configured repository, rejecting path traversal, encoded routes, and foreign repos.
• The gateway maintains role-based access control with the new /pulls/\d+/files route correctly limited to developer/reviewer.
• Workers never receive the GitHub credential directly - they get the scoped gh executable.
• Untrusted content (issue/PR data) is passed as JSON, not as instructions.
• Review/QA verdicts are verified by the coordinator with fail-closed matching.

Testing

Tests are thorough: canonical workflow reuse verification, fail-closed evidence matching, transport security, gateway authorization, and worker continuation logic are all covered with real code paths (not just mock-call assertions). The new test_developer_waits_for_review_findings_after_test_failure test correctly verifies the changed revision-trigger logic.

Repository Boundary

All changes belong in this repo: skills, automation scripts, and their tests. No SDK documentation is added or modified. The AgentServerClient is consumed, not defined.

[RISK ASSESSMENT]

• Overall PR: Medium
• The unreleased SDK dependency is the primary risk. The factory coordinates autonomous code review, testing, and merging, so correctness of the fail-closed evidence matching is critical - and it is well-tested. The scoped transport and gateway changes are security-sensitive but properly validated.

VERDICT: Needs the SDK pin replaced before merge. No material code defects found.

KEY INSIGHT: Composing canonical workflow sources with a workspace-agnostic execution interface is the right abstraction - it eliminates prompt duplication without coupling to workspace kind.

---

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]

Blocking: unreleased SDK pin. The test group depends on openhands-sdk pinned to an unreleased git commit from software-agent-sdk#5010. The PR description acknowledges this should be replaced before merging. Replace with the released package version once available - CI for this repo should not depend on an unreleased commit.

[all-hands-bot]

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

Review Summary

The PR refactors the software factory to compose canonical issue-to-PR, PR-reviewer, and QA workflow sources into a workspace-independent bundle, replacing duplicated prompts with reused canonical implementations. The fail-closed evidence matching (posted_report / report_passed) and scoped gh transport are well-designed with good test coverage. However, there are two material issues that should be resolved before merging.

Material Findings

1. Canonical implementation prompt contradicts the factory's coordinator-owned publication model (extension_workflows.py, implementation_prompt)

implementation_prompt calls _build_implementation_prompt from skills/github-issue-to-pr/scripts/main.py, which returns a prompt containing explicit steps telling the agent to push the branch and create the PR:

• Step 7: git push "https://x-access-token:$GITHUB_PERSONAL_ACCESS_TOKEN@github.com/{repo}.git" HEAD:refs/heads/{branch}
• Step 8: GH_TOKEN=$GITHUB_PERSONAL_ACCESS_TOKEN gh pr create --repo {repo} ...

The factory wrapper then appends: "The coordinator owns remote publication: do not push or create the PR yourself" and "No GitHub token is needed."

These instructions directly conflict. The canonical prompt says push and create the PR using GITHUB_PERSONAL_ACCESS_TOKEN; the wrapper says don't push and no token is needed. The agent receives both and has no GITHUB_PERSONAL_ACCESS_TOKEN in its environment. An LLM following the canonical steps (which appear first and are more detailed) will attempt to push, fail to find the token, and may waste its step budget or leave the workspace in an inconsistent state.

The review and QA prompts don't have this problem because the canonical reviewer/QA workflows publish reviews (which the factory wants the agent to do). But the implementation prompt's publication steps are fundamentally incompatible with the coordinator-owned publication model. Consider either post-processing the canonical prompt to strip/replace the push and PR-creation steps, or not reusing the canonical implementation prompt for this role.

2. Test dependency pinned to an unreleased SDK PR commit (pyproject.toml)

The test group now pins openhands-sdk to git+https://github.com/OpenHands/software-agent-sdk.git@79021c687c63bd1e925cf157f5897ceeaa12f029 - a commit on an unmerged PR (software-agent-sdk#5010). The PR description acknowledges this should be replaced before merging. This commit could disappear if the PR is rebased or force-pushed, breaking CI for this repo and any downstream consumer that runs the test suite. This should be replaced with the released SDK version before merge.

Other Observations (in review body, not blocking)

• scoped_gh.py config path resolution (line 40): config_path defaults to Path(__file__).resolve().parents[1] / "config.json". After prepare_transport() copies the script to WORKSPACE/bin/gh, this resolves to WORKSPACE/config.json. But config.json lives at the bundle root (CWD of main.py), which is only the same as WORKSPACE if WORKSPACE_BASE is set to the bundle unpack directory. If WORKSPACE_BASE is unset (defaulting to /workspace), the scoped transport will fail to find config.json at runtime. Worth verifying that the automation runtime sets WORKSPACE_BASE to the unpack directory, or explicitly copying config.json alongside the transport.

• gh() no longer handles empty response bodies (main.py, line 45): The old request() function returned {} for empty responses; the new gh() uses json.load(response) which raises JSONDecodeError on empty bodies. The gateway currently always returns JSON, so this works in practice, but it is a minor robustness regression.

Risk Assessment

• Overall PR: 🟡 MEDIUM
  • The architecture is sound: composing canonical workflows, fail-closed evidence matching, and a scoped transport are good design choices with solid test coverage.
  • The implementation prompt contradiction is a functional correctness issue that will cause agent confusion or failure in the developer role.
  • The unreleased SDK pin creates a fragile CI dependency that the PR description already flags for replacement.

Verdict: ❌ Needs rework - resolve the implementation prompt contradiction and replace the SDK pin before merging.

Key insight: Reusing canonical workflow prompts for their content while overriding their publication behavior creates a direct instruction conflict in the implementation role; the reviewer and QA roles avoid this because their canonical workflows' publication steps align with what the factory wants.

---

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]

Fragile dependency pin. The test group pins openhands-sdk to a specific commit on the unmerged software-agent-sdk#5010 PR. This commit could disappear if that PR is rebased or force-pushed, breaking CI for this repo and downstream consumers. The PR description acknowledges this should be replaced before merging. Replace with the released SDK version once available.

[all-hands-bot]

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

Review: PR #562 - Reuse canonical implementation, review, and QA workflows

Taste Rating: 🟡 Acceptable - The architectural approach of composing canonical workflow sources rather than duplicating them is sound. The fail-closed evidence matching and scoped transport are well-designed. A few issues warrant attention before merge.

---

Finding 1: Canonical implementation prompt contradicts the factory execution contract

implementation_prompt calls the canonical _build_implementation_prompt which embeds detailed instructions to use GITHUB_PERSONAL_ACCESS_TOKEN, git push, and gh pr create. The factory then appends a contradictory execution contract saying "No GitHub token is needed" and "do not push or create the PR yourself."

The agent receives both sets of instructions in the same prompt. While the appended contract comes last and should take precedence, the canonical prompt contains very specific push/PR commands (including a full git push URL with token substitution) that are now dead instructions in the factory context. An agent following the canonical steps literally would attempt to push using a non-existent GITHUB_PERSONAL_ACCESS_TOKEN environment variable.

Consider either suppressing or clearly overriding the push/PR steps from the canonical prompt, rather than relying on the agent to reconcile contradictory instructions.

Finding 2: gh_pages breaks on endpoints with existing query parameters

gh_pages unconditionally appends ?per_page=100&page={page} to the endpoint string. If an endpoint already contains a query string (e.g., /pulls?state=open), this produces a malformed URL with two ? characters. All current call sites pass bare paths without query parameters, so this is not a live bug, but it is a latent fragility that will silently produce wrong API calls if a future caller passes a parameterized endpoint.

Finding 3: SDK git pin in pyproject.toml must be replaced before merge

The openhands-sdk test dependency is pinned to an unreleased commit from software-agent-sdk#5010 via git URL. The PR description acknowledges this. Confirm this is replaced with the published release before merging, as git-based dependencies are not available on PyPI and will break downstream consumers that install from the wheel.

Finding 4: Reviewer complete logic can leave a PR without a review status

If the review agent crashes (throws an exception) during the review stage before posting any report, reports stays empty, complete evaluates to False, and no software-factory/review status is posted. The developer only revises PRs with software-factory/review in failure/error, so the PR is not picked up for revision. The reviewer will re-run on the next poll (since the status is absent from statuses), which is the intended retry path. However, if the review agent consistently crashes (e.g., a prompt that is too large or a model that rejects it), the PR is stuck in a retry loop with no surface signal on GitHub. Consider posting a failure status after a bounded number of consecutive crashes, or catching the exception in the review loop to post an explicit failure.

---

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM
• The security model is solid: scoped gateway, no credential leakage to workers, fail-closed verdict parsing, and path traversal protection in scoped_gh.py. The main risks are the contradictory prompt instructions (Finding 1) and the temporary SDK pin (Finding 3). The retry-loop edge case (Finding 4) is a liveness concern but not a correctness bug.

VERDICT: ✅ Worth merging after addressing the SDK pin replacement and considering the prompt contradiction. The core architecture - composing canonical workflows with a coordinator-owned transport and publication layer - is a good design that eliminates duplication.

KEY INSIGHT: Composing canonical workflow sources rather than duplicating them is the right approach, but the factory needs to actively suppress or override the canonical prompt's push/PR steps rather than appending contradictory instructions and hoping the agent reconciles them.

---

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]

🤖 OpenHands is reviewing this PR.

Head commit: 55b6fdb938f60772ac2e0e198bfb6d9f73faa194
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/bb676026-49d7-4fe3-ad43-06a719b0318b

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

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 56cb79a5bb436959b02e688e610712b9827ee467
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/46e0d5d7-b7c2-4534-a0b9-2950330ba30b

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

This PR refactors the software factory to compose canonical extension workflow sources (issue-to-PR, PR-reviewer, QA-changes) rather than maintaining duplicated prompts. It introduces a scoped gh api transport, a bundle builder with source provenance, profile-selected gateway grants, and fail-closed evidence matching for native review/QA reports. The design is sound: credentials stay out of bundles, evidence matching rejects stale/ambiguous reports, and the coordinator owns publication.

The changes belong in this repository (skills, plugins, automations). No SDK-specific documentation is introduced.

Findings

Pre-release SDK pin in test dependencies

pyproject.toml line 27 pins openhands-sdk to an unreleased commit (79021c687c...) of software-agent-sdk. The PR description acknowledges this: "Replace the integration pin with the SDK release before merging." This is a first-party package maintained by the same organization, so the 7-day supply-chain waiting rule does not apply. However, this pin must be replaced with the released SDK version before merging - CI and downstream consumers cannot depend on an immutable GitHub commit SHA indefinitely.

No human testing confirmed

The PR description's human-testing checkbox is unchecked ([ ] A human has tested these changes). The PR description provides extensive automated test evidence (853+ tests passing) and describes live factory validation in progress, but the checkbox remains unchecked. Per the repository's contribution guidance, human testing should be confirmed before merge.

complete variable logic in reviewer is subtle but correct

The complete variable (line 542) determines whether a software-factory/review status is posted. The three conditions cover: (1) review explicitly failed, (2) review completed but tests failed, (3) QA ran. When a transport/model exception prevents any report from being posted, complete is False, no status is posted, and the reviewer retries. This is the correct fail-closed behavior - a retryable infrastructure failure should not produce an acceptance decision. The logic is sound but dense; a brief comment explaining the three cases would help future maintainers.

Factory-created PR body lacks the standard AI disclosure format

The publish() function (line 298) creates PRs with body "Produced entirely by the OpenHands software factory." The canonical issue-to-PR workflow (when publish_pr=True) requires the exact disclosure _This pull request was opened by an AI agent (OpenHands)._. The factory's wording conveys the intent but doesn't match the canonical format. Consider aligning the disclosure text for consistency across automation paths.

Developer revision trigger narrowed to review-only

The developer now only revises when software-factory/review is in failure/error (line 317), whereas the previous code also triggered on software-factory/tests failure. The comment explains the rationale: wait for review findings even if tests failed, since the review provides actionable feedback. This is a reasonable design choice - a test failure without review context gives the developer less to act on. Just confirming this is intentional.

Risk Assessment

• Overall PR: 🟡 MEDIUM
• Security: Good practices throughout - no credentials in bundles, scoped gateway with path validation, symlink rejection, O_NOFOLLOW on token materialization, fail-closed evidence matching.
• Correctness: The complete variable logic and posted_report/report_passed matching are correct but subtle. Tests cover the key edge cases.
• Dependency: The unreleased SDK commit pin is the main risk - it must be replaced before merge.
• Testing: Extensive automated test coverage (853+ tests). Human testing not yet confirmed.

VERDICT: Approve with conditions - replace the SDK commit pin with the released version and confirm human testing before merging.

---

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

The test dependency is pinned to an unreleased commit of openhands-sdk. The PR description acknowledges this is temporary. Since this is a first-party package, the 7-day supply-chain waiting rule does not apply, but this pin must be replaced with the released SDK version before merging - CI and downstream consumers should not depend on an immutable GitHub commit SHA.

[all-hands-bot]

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

Summary

This PR refactors the software factory to compose canonical extension workflows (issue-to-PR, PR Reviewer, QA Changes) rather than duplicating their prompts, and migrates runtime communication to the public AgentServerClient from the SDK. It introduces a scoped gh transport (scoped_gh.py) that enforces repository isolation and fail-closed credential handling, a bundle builder that rejects embedded credentials, and a revised reviewer flow that requires commit-pinned native review/QA reports with unambiguous passing verdicts.

The security design is strong: the gateway token is materialized via O_NOFOLLOW with mode 0600, never falls back to GITHUB_TOKEN, and the transport validates all endpoints are within the configured repository. The report_passed regexes are fail-closed for both review and QA stages. The complete disjunction in reviewer() correctly distinguishes retryable transport failures (no status set, reviewer retries) from terminal review outcomes (status set, developer may revise).

Findings

1. Pre-release SDK dependency pin (merge blocker)

pyproject.toml line 27 replaces openhands-sdk>=0.3 with a pin to an unreleased commit (79021c6...) from software-agent-sdk PR #5010. The released SDK (v1.44.1) does not export openhands.sdk.client.AgentServerClient - confirmed by import failure in the current environment. The PR description acknowledges this and says to replace with the SDK release before merging. This is a hard blocker: merging would break uv sync --group test for anyone not using that exact commit. The pin is to an immutable SHA, which is good for reproducibility, but the dependency on an unreviewed, unreleased SDK commit is a supply-chain concern that must be resolved before this can leave draft.

2. Potential duplicate review reports on repeated QA transport failures

When the review agent succeeds (posting a native review) but the QA agent hits a transport error, the complete logic correctly does not set software-factory/review status, so the reviewer retries on the next sweep. However, the already-posted review report is now in the before set, so a new review must be published. If QA keeps failing, this produces duplicate review reports on the PR. This is a fail-closed design trade-off (safer to retry than to accept stale evidence), but consider adding a comment or status that surfaces the retry loop so an operator can intervene before the PR accumulates many duplicate reviews.

3. Repository boundary - correct placement

This PR belongs in the extensions registry: it composes existing skills and workflows from this repo into a factory recipe. No SDK behavior, API endpoints, or UI code is introduced. The AgentServerClient import is a consumer of the SDK, not a modification of it. No SDK-specific documentation is added to this repo.

Risk Assessment

• Overall PR: 🟡 MEDIUM
  • The code is well-structured with strong fail-closed security design. The primary risk is the unreleased SDK dependency pin, which is an acknowledged draft prerequisite. The factory workflow logic is sound but operates in a multi-agent retry loop that could benefit from observability for stuck states.

Verdict: Needs rework before merge - the SDK pin must be replaced with a released version. The code quality itself is good; no blocking code defects were found.

Key insight: The fail-closed evidence matching and scoped gateway transport are the right architectural choices for a multi-agent factory that posts acceptance decisions to GitHub.

---

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]

Merge blocker: unreleased SDK pin. The released openhands-sdk (v1.44.1) does not export openhands.sdk.client.AgentServerClient - the import fails with No module named 'openhands.sdk.client'. This pin to an unreleased commit from software-agent-sdk#5010 must be replaced with a released SDK version before this PR can leave draft. The PR description acknowledges this; flagging here so it is not missed at merge time.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 86fb54fc59f83846dcf9d30c63312c61c69ee70f
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/c675fdd1-50f5-4aa3-be22-32d3f01c3a2e

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: PR #562 - Reuse canonical implementation, review, and QA workflows

Taste Rating: 🟢 Good taste - The refactor cleanly eliminates duplicated workflow logic by composing existing canonical sources. The separation of concerns (coordinator owns publication, workers receive scoped transports) is the right design.

Analysis

The PR replaces the factory's duplicated implementation/review/QA prompts with composable references to the canonical github-issue-to-pr, github-pr-reviewer, and qa-changes workflow definitions. Key strengths:

1. Fail-closed evidence matching: posted_report() requires exactly one newly published review matching the exact head SHA, the run marker, and COMMENTED state. report_passed() uses strict regex matching for verdicts - no inference from agent free text. Tests cover stale, unrelated, and ambiguous verdicts.

2. Scoped gateway transport: scoped_gh.py validates that every endpoint is inside the configured repository, rejects path traversal (.., %, #, \), and never sends credentials to a caller-selected URL. The gateway token is materialized from a profile-selected env var to a mode-0600 file using O_NOFOLLOW, with no fallback to GITHUB_TOKEN.

3. No credentials in bundles: build_bundle.py rejects configs containing token or missing token_env.

4. Coordinator-owned publication: The implementation prompt for factory workers has no git push or gh pr create commands - the coordinator publishes after the run.

5. Review retry semantics: Transport/model failures during review don't set a failure status (allowing retry), while complete-but-rejected reviews do. The complete logic in the finally block correctly distinguishes these cases.

Observations (not blocking)

• Temporary SDK git pin (pyproject.toml): The test dependency is pinned to an unreleased SDK commit. The PR description explicitly acknowledges this must be replaced with the SDK release before merging. This is the correct approach for a draft depending on unreleased SDK features.

• Private function coupling: extension_workflows.py calls underscore-prefixed functions (_build_implementation_prompt, _build_review_prompt, _load_repo_review_guide) from canonical workflow modules. The README documents this as an explicit integration contract covered by tests. This is a pragmatic tradeoff - the alternative would be duplicating the prompt logic, which is what this PR eliminates.

• open_issues() and PR listing use per_page=100 without full pagination: This is pre-existing behavior (not introduced by this PR). The factory targets single-repo workflows with modest issue/PR counts. The new gh_pages() function provides proper pagination where it matters most (reviews, comments, files).

Repository boundary check

This PR belongs in the extensions registry. It modifies factory skill scripts, workflow composition, gateway authorization, and tests - all within this repo's scope. The SDK client import (openhands.sdk.client.AgentServerClient) is a consumer dependency, not SDK implementation code. No SDK documentation is added or modified here.

[RISK ASSESSMENT]

• Overall PR: 🟢 LOW
  • No credential exposure, no injection vectors, fail-closed acceptance gates.
  • The main merge prerequisite (SDK release pin replacement) is already acknowledged.
  • All new code paths have corresponding tests with real assertions (not mock-call verification).

VERDICT: ✅ Worth merging after replacing the temporary SDK git pin with the released package, as noted in the PR description.

KEY INSIGHT: Composing canonical workflow sources with a supplied execution interface - rather than duplicating prompt logic - is the correct factoring. The fail-closed evidence matching and scoped transport design make this a solid foundation for the factory's local/Docker unification.

---

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]

🤖 OpenHands is reviewing this PR.

Head commit: 4c5c1389c8d25c6abd93fc210ff6332e18c3063e
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/a38d922f-6449-41eb-9bb5-33477d3ee5de

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

This PR refactors the software factory to compose canonical issue-to-PR, PR-reviewer, and QA workflow definitions rather than duplicating their prompts. It introduces a repository-scoped gh transport, a workspace-independent bundle builder with source provenance, and migrates the worker to the public AgentServerClient. The design is sound: reusing canonical workflows eliminates drift, the scoped gateway enforces least privilege, and the fail-closed verdict matching prevents ambiguous acceptance.

No critical bugs or security issues found. Two items worth noting:

1. Temporary SDK git pin in pyproject.toml (blocking for merge)

The test dependency pins openhands-sdk to an immutable commit SHA from software-agent-sdk#5010. The PR description explicitly acknowledges this must be replaced with the released package before merge. CI will build the SDK from source on every run until then, which is fragile. Not a blocker for review, but must not merge with the pin in place.

2. finally block in reviewer() can mask exceptions and lose evidence

In main.py lines 365-407, the finally block calls shell(["git", "status", ...]) and gh("GET", ...) to compute clean and current. If either raises (e.g., the gateway is unreachable after a review failure, or git is in a broken state), the exception propagates from finally, masking the original exception from the except block, and acceptance.json is never written. Since the acceptance evidence trail is the primary gate for the watchdog, losing it on a failure path is a real robustness gap. Wrapping the finally operations in their own try/except (defaulting clean=False, current=False on error) would preserve the original exception and still write the evidence file.

Repository boundary

This PR belongs in the extensions registry: it composes existing skills/plugins shipped here and adds factory orchestration code under skills/github-software-factory/. The AgentServerClient import consumes the SDK; it does not implement SDK behavior. No boundary concerns.

Testing

Test coverage is strong: 13 new test cases in test_factory_extension_workflows.py covering transport rejection, gateway grant isolation, bundler credential rejection, and canonical verdict matching. The factory worker tests properly mock the SDK client surface. The fail-closed verdict tests are particularly well-designed - they verify that stale, unrelated, and ambiguous reviews cannot satisfy acceptance.

[RISK ASSESSMENT]

• Overall PR: 🟡 MEDIUM
• The design is solid and security-conscious (scoped transport, no credential fallback, symlink protection, fail-closed verdicts). The medium rating reflects the unreleased SDK dependency, the finally-block evidence gap, and that this is a complex orchestration layer where subtle acceptance-logic bugs could allow incorrect merges. The PR description notes live validation is still in progress and this remains a draft.

VERDICT: Approve with notes. Address the finally-block robustness before merging, and replace the SDK git pin with the release.

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 is merge-ready.

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

[all-hands-bot]

This git-commit pin for openhands-sdk is correctly flagged in the PR description as a temporary integration dependency. It must be replaced with the released package before merge - CI builds the SDK from source on every run until then.

[neubig]

Consolidating this canonical workflow composition directly into #557 during the requested PR audit. The initial custom JSON reviewer must not be merged first and immediately replaced. #557 now carries the final reusable issue-to-PR/reviewer/QA composition and retains the SDK release prerequisite. The new review order is #559 -> #557 -> #564. Closing this intermediate PR without merging it to main.