feat: run independent factory issues in bounded developer lanes - #564
feat: run independent factory issues in bounded developer lanes#564neubig wants to merge 8 commits into
Conversation
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: Parallel developer lanes, dependency gating, and native branch refresh
Taste Rating: 🟡 Acceptable - The lane partitioning and gateway refactor are clean, but two error-path gaps could cause silent crash loops in production.
Critical Issues
1. dependencies_complete crashes on non-existent dependency references (main.py:316)
dependencies_complete calls gh("GET", f"/issues/{number}") for each #N reference parsed from the issue body. Issue bodies are untrusted user content. If an author writes Depends on: #99999 (typo, deleted issue, or cross-repo reference), GitHub returns 404, the gateway propagates it, and gh() raises HTTPError. This crashes the entire triage() or developer() function - not just the affected issue, but ALL issue processing in that role for that run. The docstring says "unknown/incomplete issues remain blocked," but the actual behavior for unknown issues is an unhandled exception, not a return False.
A one-line fix: catch HTTPError (404) and return False, which matches the documented "unknown issues remain blocked" contract and prevents one bad reference from stalling the whole pipeline.
2. Branch update conflict creates a crash loop with no user-facing signal (main.py:347-352)
When comparison["status"] is "diverged" and software-factory/review == "success", the developer calls gh("POST", "/factory/update-branch", ...). If GitHub rejects the update (merge conflict or stale head), the gateway returns a non-2xx, gh() raises HTTPError, and developer() crashes. On the next automation run, the same PR still has review=success on the same head and the base is still diverged, so the same update is attempted and crashes again - an infinite loop.
The PR description says conflicts "fail closed and require a developer revision," but the implementation provides no mechanism to communicate this to humans: no PR comment, no status update, no log entry beyond the crash trace. The lane is stuck with no signal until someone manually inspects the automation logs. Consider wrapping the update call in try/except, posting a comment on the PR explaining the conflict, and returning gracefully so the lane moves on or waits visibly.
Improvement Opportunities
3. dependencies_complete is called inside list comprehensions with side effects (main.py:168, 381)
dependencies_complete makes N API calls (one per dependency) and is called as a filter predicate inside a list comprehension over all open issues. With 100 open issues each referencing dependencies, this can produce hundreds of sequential gateway calls per run. Not a correctness bug, but worth noting for latency. If the issue count grows, consider batching or caching dependency lookups.
Risk Assessment
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The core lane-partitioning logic and gateway refactor are sound and well-tested. The medium risk comes from the two unhandled error paths that can silently stall automation lanes in production. Neither is a security issue, but both can cause operational outages that are hard to diagnose without log access. The/compare/route addition andfactory_prextraction are correctly scoped.
VERDICT:
❌ Needs rework: The two crash-loop scenarios should be handled gracefully before merge. Both are small fixes (try/except + return False for dependencies; try/except + comment for branch update conflicts) but they prevent real operational problems.
KEY INSIGHT:
The lane and dependency features are well-designed, but the error paths assume all GitHub API calls succeed - a fragile assumption when processing untrusted issue bodies and performing merge operations.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger 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.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- 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
/iterateto automatically drive this PR through CI, review, and QA until it’s merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review: feat: run independent factory issues in bounded developer lanes
Taste Rating: 🟢 Good taste
The PR introduces parallel developer lanes, dependency gating, and native branch-update refresh into the software factory with clean, minimal abstractions. Lane assignment via issue_number % lanes is deterministic and requires no coordination primitive. The gateway's factory_pr extraction correctly shares identity validation between merge and update_branch. The expected_head_sha parameter on the update-branch PUT ensures atomicity against concurrent pushes.
Analysis
Lane isolation is well-scoped: both the issue selection and PR filtering use the same modulo predicate, so a lane never acts on another lane's work. The type() is not int validation correctly rejects booleans and floats as lane config, which is stricter than isinstance and appropriate for config validation.
Dependency gating correctly distinguishes closed + completed from not_planned, and 404 returns False (blocked) while transient errors (503) propagate. The @cache on completed_dependency is process-scoped, which matches the single-sweep-per-process architecture (each role invocation calls one function and exits in __main__).
Branch update flow handles the three outcomes correctly: success returns and waits for re-review, 409 (stale head) returns for next sweep, 422 (conflict) enters the existing developer revision workflow with a visible PR comment and conflict-resolution feedback passed to the implementation prompt.
Gateway security: The new /compare/{sha}...{sha} permission is read-only with strict 40-hex validation, and the developer role already has access to individual commits and refs. The update-branch operation goes through the factory dispatch table (not permitted()), which means it's validated by factory_pr before reaching GitHub. The PUT to /pulls/{n}/update-branch is never directly exposed as a generic permitted route.
Repository boundary
All changes are in skill scripts (skills/github-software-factory/, skills/openhands-automation/) and their tests. No SDK behavior, API endpoints, or scheduling/dispatch code is modified. This belongs in the extensions repo.
Testing
Tests cover lane isolation (own-lane selection, other-lane PR non-blocking), dependency completion states (open/closed-completed/closed-not-planned/404/503), the native branch update path (success, conflict, stale head), and gateway identity validation. Tests exercise real code paths with controlled fakes rather than mocking the unit under test.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟢 LOW
The change adds a new concurrent execution dimension to the factory, but lane assignment is deterministic, the default (developer_lanes=1) preserves existing single-lane behavior, and the branch update is narrowly scoped with exact-head validation. The gateway changes are additive (new operation, new read-only route) and don't modify existing merge logic beyond extracting shared validation.
VERDICT: ✅ Worth merging
KEY INSIGHT: The lane abstraction is the right design: it parallelizes without introducing locks or coordination by making assignment a pure function of issue number, and the branch-update gateway operation reuses the exact-head identity check that already gates merges.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- 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
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
# Conflicts: # tests/test_github_software_factory.py
# Conflicts: # tests/test_github_factory_gateway.py
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Review Summary
This PR adds parallel developer lanes, issue dependency tracking, and a native branch-update flow to the GitHub software factory. The design is clean and the security model is consistent with existing patterns.
Key Strengths
- Lane assignment uses simple modulo-based ownership (
issue_number % developer_lanes), with PR filtering on the same scheme. The validation atdeveloper()rejects non-integer or out-of-range lane config with a clear error. - Dependency tracking correctly distinguishes
closed + completedfromclosed + not_planned, and 404s (missing issues) block rather than crash. The@cacheoncompleted_dependencyis process-scoped, which is correct since each factory run executes a single role (one sweep per process). - Branch update flow handles all three outcomes correctly: 409 (head changed) returns early for re-read on the next sweep; 422 (merge conflict) enters the existing developer revision workflow with conflict context; success returns to await independent re-review.
- Gateway security for
update_branchreuses thefactory_pr()validation gate (open state, exact head SHA, base=main, factory/issue-N ref pattern) before making the GitHub API call, consistent with the existingmergeoperation. Theexpected_head_shaparameter provides optimistic concurrency. - Path traversal fix in
scoped_gh.pycorrectly switches from substring..blocking to per-segment./..blocking, allowing the...separator in compare URLs while still blocking actual traversal. The gateway handler uses a complementary regex-exemption approach for the same goal.
Tests
Tests exercise real code paths: dependencies_complete with various issue states, lane isolation logic, the update-branch gateway operation with identity checks, and path validation. Mocks are used only at the network boundary (gh/github), which is appropriate.
[RISK ASSESSMENT]
- Overall PR: LOW
The change extends the existing factory workflow with well-scoped additions. No new secrets, no new external surfaces beyond a narrowly-scoped/compare/GET and a/factory/update-branchPOST that reuses the existing factory operation dispatch pattern. ThePUT /pulls/{number}/update-branchcall is developer-role-only and gated byfactory_pr()validation. No breaking changes to existing single-lane behavior (defaults preservedeveloper_lane=0,developer_lanes=1).
VERDICT: Worth merging. Core logic is sound, security model is consistent, and tests cover the key behaviors.
KEY INSIGHT: The parallel-lanes design adds no distributed coordination - it relies entirely on the existing dispatcher's per-automation concurrency limit and deterministic modulo-based issue ownership, which is the right pragmatic choice for this architecture.
|
Superseded by the deployed independent automations: #570 (triage), #571 (existing issue-to-PR), #572 (existing reviewer plus QA), and #573 (watchdog). OpenHands/automation#453 stores the selected agent profile on each automation definition. Profiles supply fine-grained PATs directly, so the host gateway and role-dispatched bundle are no longer used. Active Docker probes confirmed triage, developer, and reviewer receive only their selected credential. The replacement stack is native GitHub stack #574. |
Why
One outstanding implementation PR currently blocks every other ready issue in a repository. Independent small issues should progress concurrently within the existing dispatcher and Docker limits.
Summary
Depends on:prerequisites have not completed, during both triage and development.Issue Number
Closes #563.
How to Test
uv run pytest -q tests/test_github_software_factory.py tests/test_github_factory_gateway.py tests/test_factory_extension_workflows.py98 tests passed. Coverage includes lane isolation, prerequisite completion, and the authenticated branch update boundary.
Notes
Stacked after #557 because this extends its canonical workflow composition. Configure a separate automation ID for each lane and keep the lane count stable while runs are active. Global capacity remains controlled by the existing scheduler/runtime. Native branch update conflicts post a visible comment and reuse the canonical developer revision workflow. Missing dependencies block only their issue.