diff --git a/skills/github-software-factory/README.md b/skills/github-software-factory/README.md index d06b7773..c7407d33 100644 --- a/skills/github-software-factory/README.md +++ b/skills/github-software-factory/README.md @@ -165,3 +165,25 @@ Use `python3 main.py --token-env FACTORY_ROLE_GRANT` as the entrypoint, with the actual name from `config.json` substituted. Naming the secret lets the SDK inject it from the selected profile without shell expansion; the CLI rejects a name that does not match the bundle. The same command works in local and Docker workspaces. + +## Parallel developer lanes + +Install one developer automation per lane, using the existing dispatcher to keep +at most one run active per automation. Set `developer_lanes` to the same positive +count in every developer bundle and `developer_lane` to a distinct zero-based +index. Lane ownership is `issue_number % developer_lanes`. Keep this assignment +fixed while work is active. Each lane permits one outstanding implementation PR; +other lanes can work while it awaits review. Local and Docker bundles are identical. + +Use `Depends on: #1, #2` on an issue to block triage/development until those issues +are closed as completed. Dependency references are repository-local. Configure the +existing service concurrency and Docker memory/CPU limits separately; no extra +scheduler or distributed lock service is introduced. + +An accepted PR whose base has advanced is refreshed with GitHub's native +update-branch endpoint. This produces a new head that must pass independent review +again. Stale expected heads are re-read on the next sweep. Merge conflicts receive +a visible PR comment and enter the existing canonical developer revision workflow +with current-base context. The revised head must pass independent review before +the next native update attempt. Missing dependency references block only their +issue; dependency reads are cached within each sweep. diff --git a/skills/github-software-factory/scripts/main.py b/skills/github-software-factory/scripts/main.py index 81ee3f92..aa78ddc6 100644 --- a/skills/github-software-factory/scripts/main.py +++ b/skills/github-software-factory/scripts/main.py @@ -1,6 +1,7 @@ """One scheduled software-factory role, executed in an isolated runtime.""" import argparse +from functools import cache import base64 import io import importlib.util @@ -165,6 +166,7 @@ def triage(): i for i in open_issues() if "ready-for-dev" not in {label["name"] for label in i["labels"]} + and dependencies_complete(i) ] if not issues: return @@ -304,13 +306,82 @@ def publish(issue, base, branch, existing, local_base): ) +@cache +def completed_dependency(number): + try: + dependency = gh("GET", f"/issues/{number}") + except HTTPError as exc: + if exc.code == 404: + return False + raise + return ( + dependency["state"] == "closed" + and dependency.get("state_reason") == "completed" + ) + + +def dependencies_complete(issue): + """Honor explicit Depends on lines; unknown/incomplete issues remain blocked.""" + for line in re.findall( + r"^Depends on:\s*(.+)$", issue.get("body") or "", re.M | re.I + ): + for number in re.findall(r"#(\d+)", line): + if not completed_dependency(number): + return False + return True + + def developer(): - prs = gh("GET", "/pulls?state=open&per_page=100") - existing = None + lane = CONFIG.get("developer_lane", 0) + lanes = CONFIG.get("developer_lanes", 1) + if type(lane) is not int or type(lanes) is not int or not 0 <= lane < lanes: + raise ValueError("Developer lane must be an integer in [0, developer_lanes)") issues = open_issues() + owned = {i["number"] for i in issues if i["number"] % lanes == lane} + prs = [ + p + for p in gh("GET", "/pulls?state=open&per_page=100") + if (match := re.fullmatch(r"factory/issue-(\d+)", p["head"]["ref"])) + and int(match[1]) % lanes == lane + ] + existing = None + update_conflict = None if prs: for pr in prs: state = statuses(pr["head"]["sha"]) + if state.get("software-factory/review") == "success": + main_sha = gh("GET", "/git/ref/heads/main")["object"]["sha"] + comparison = gh("GET", f"/compare/{main_sha}...{pr['head']['sha']}") + if comparison["status"] not in ("ahead", "identical"): + # GitHub merges the base; the new head gets independent review again. + try: + gh( + "POST", + "/factory/update-branch", + {"number": pr["number"], "sha": pr["head"]["sha"]}, + ) + except HTTPError as exc: + if exc.code == 409: + return # Head changed; re-read it on the next sweep. + if exc.code != 422: + raise + update_conflict = { + "base_sha": main_sha, + "instruction": "The native base update found merge conflicts. " + "Inspect the current main files and PR diff with gh api. " + "Reconcile the conflicting changes in this branch while " + "preserving both completed features and this issue's criteria. " + "Run tests after the revision. The coordinator will publish " + "and retry the base update after independent review.", + } + comment( + pr["number"], + "Automatic base update found conflicts; " + "the canonical developer workflow is revising this PR.", + ) + existing = pr + break + return # Wait for the independent review to publish its findings before # revising, even if the earlier deterministic test phase failed. if state.get("software-factory/review") in ("failure", "error"): @@ -337,7 +408,9 @@ def developer(): ready = [ i for i in issues - if "ready-for-dev" in {label["name"] for label in i["labels"]} + if i["number"] in owned + and "ready-for-dev" in {label["name"] for label in i["labels"]} + and dependencies_complete(i) ] if not ready: return @@ -358,7 +431,7 @@ def developer(): local_base = shell(["git", "rev-list", "--max-parents=0", "HEAD"]) else: base, local_base = clone(branch if existing else "main") - feedback = {} + feedback = {"base_update": update_conflict} if update_conflict else {} if existing: for name, endpoint in ( ("discussion", f"/issues/{existing['number']}/comments"), diff --git a/skills/github-software-factory/scripts/scoped_gh.py b/skills/github-software-factory/scripts/scoped_gh.py index d55dcd4d..9b47c237 100644 --- a/skills/github-software-factory/scripts/scoped_gh.py +++ b/skills/github-software-factory/scripts/scoped_gh.py @@ -48,7 +48,9 @@ def repository_path(endpoint, repository): if not endpoint.startswith(prefix): raise ValueError("Endpoint must be inside the configured repository") path = "/" + endpoint[len(prefix) :] - if any(c in path for c in ("%", "..", "#", "\\")): + if any(c in path for c in ("%", "#", "\\")) or any( + part in (".", "..") for part in urlsplit(path).path.split("/") + ): raise ValueError("Invalid endpoint") return path diff --git a/skills/openhands-automation/scripts/github_factory_gateway.py b/skills/openhands-automation/scripts/github_factory_gateway.py index 1fc359c3..53c80071 100644 --- a/skills/openhands-automation/scripts/github_factory_gateway.py +++ b/skills/openhands-automation/scripts/github_factory_gateway.py @@ -171,6 +171,10 @@ def permitted(role, method, path, body): return True if role in ("developer", "reviewer", "watchdog") and route == "/actions/runs": return True + if role == "developer" and re.fullmatch( + r"/compare/[0-9a-f]{40}\.\.\.[0-9a-f]{40}", route + ): + return True if role in ("developer", "reviewer") and re.fullmatch( r"/git/ref/heads/(?:main|factory/issue-\d+)", route ): @@ -215,22 +219,37 @@ def permitted(role, method, path, body): return False -def merge(number, sha): +def factory_pr(role, number, sha): if number < 1 or not re.fullmatch(r"[0-9a-f]{40}", sha): - raise ValueError("Merge requires a PR number and exact commit SHA") - role = "watchdog" + raise ValueError("Operation requires a PR number and exact commit SHA") pr = github(role, "GET", f"/pulls/{number}") + if not ( + pr["state"] == "open" + and pr["head"]["sha"] == sha + and pr["base"]["ref"] == "main" + and re.fullmatch(r"factory/issue-\d+", pr["head"]["ref"]) + ): + raise ValueError("PR is not an open factory branch at the expected head") + return pr + + +def update_branch(number, sha): + factory_pr("developer", number, sha) + return github( + "developer", "PUT", f"/pulls/{number}/update-branch", {"expected_head_sha": sha} + ) + + +def merge(number, sha): + role = "watchdog" + pr = factory_pr(role, number, sha) statuses = latest_statuses(role, sha) contexts = ("software-factory/tests", "software-factory/review") ci_ok = ci_passed(role, sha) # Refuse a stale base and incomplete pagination rather than overlooking CI. comparison = github(role, "GET", f"/compare/{pr['base']['sha']}...{sha}") eligible = ( - pr["state"] == "open" - and not pr["draft"] - and pr["head"]["sha"] == sha - and pr["base"]["ref"] == "main" - and re.fullmatch(r"factory/issue-\d+", pr["head"]["ref"]) + not pr["draft"] and pr["mergeable"] is True and comparison["status"] in ("ahead", "identical") and all(statuses.get(c, {}).get("state") == "success" for c in contexts) @@ -263,7 +282,17 @@ def do_POST(self): payload = json.loads(self.rfile.read(length)) method, path = payload["method"], payload["path"] body = payload.get("body") - if "%" in path or ".." in path or "#" in path or "\\" in path: + if ( + "%" in path + or ( + ".." in path + and not re.fullmatch( + r"/compare/[0-9a-f]{40}\.\.\.[0-9a-f]{40}", path + ) + ) + or "#" in path + or "\\" in path + ): return self.reply(403, {"error": "Invalid path"}) if path == "/factory/archive" and role in ("developer", "reviewer"): return self.reply(200, archive(role, body["sha"])) @@ -288,9 +317,13 @@ def do_POST(self): }, ), ) - if path == "/factory/merge" and role == "watchdog": + operation = { + ("/factory/merge", "watchdog"): merge, + ("/factory/update-branch", "developer"): update_branch, + }.get((path, role)) + if operation: try: - result = merge(int(body["number"]), body["sha"]) + result = operation(int(body["number"]), body["sha"]) except ValueError as exc: return self.reply(409, {"error": str(exc)}) return self.reply(200, result) diff --git a/tests/test_factory_extension_workflows.py b/tests/test_factory_extension_workflows.py index 17a7daad..5a0ab2a9 100644 --- a/tests/test_factory_extension_workflows.py +++ b/tests/test_factory_extension_workflows.py @@ -169,3 +169,11 @@ def test_coordinator_implementation_prompt_has_no_direct_publication_commands(tm assert "gh pr create" not in prompt assert "coordinator publishes" in prompt assert str(tmp_path / "bin/gh") in prompt + + +def test_scoped_transport_accepts_commit_comparisons(): + path = "/compare/" + "a" * 40 + "..." + "b" * 40 + assert ( + load("scoped_gh").repository_path("repos/owner/repo" + path, "owner/repo") + == path + ) diff --git a/tests/test_github_factory_gateway.py b/tests/test_github_factory_gateway.py index c5d4f97d..279f0193 100644 --- a/tests/test_github_factory_gateway.py +++ b/tests/test_github_factory_gateway.py @@ -510,6 +510,32 @@ def upstream(role, method, path, body=None): assert all(method == "GET" for method, _ in calls) +def test_native_branch_update_checks_identity_and_uses_developer(broker, monkeypatch): + calls = [] + + def github(role, method, path, body=None): + calls.append((role, method, path, body)) + if method == "GET": + return { + "state": "open", + "head": {"sha": "a" * 40, "ref": "factory/issue-1"}, + "base": {"ref": "main"}, + } + return {"message": "Updating branch"} + + monkeypatch.setattr(broker, "github", github) + broker.update_branch(3, "a" * 40) + assert calls[-1] == ( + "developer", + "PUT", + "/pulls/3/update-branch", + {"expected_head_sha": "a" * 40}, + ) + with pytest.raises(ValueError): + broker.update_branch(3, "b" * 40) + assert calls[-1][1] == "GET" + + def test_review_rejection_explains_required_fields(broker, monkeypatch): import io import json @@ -532,3 +558,37 @@ def test_review_rejection_explains_required_fields(broker, monkeypatch): assert responses[0][0] == 403 assert "COMMENT" in responses[0][1]["error"] assert "commit_id" in responses[0][1]["error"] + + +@pytest.mark.parametrize( + "path,expected", + [("/compare/" + "a" * 40 + "..." + "b" * 40, 200), ("/compare/../issues", 403)], +) +def test_handler_allows_exact_commit_comparison_without_path_traversal( + broker, monkeypatch, path, expected +): + import io + import json + from types import SimpleNamespace + + monkeypatch.setattr(broker, "CONTROL", {role: role for role in broker.ROLES}) + upstream = [] + + def github(*args): + upstream.append(args) + return {"status": "ahead"} + + monkeypatch.setattr(broker, "github", github) + payload = json.dumps({"method": "GET", "path": path}).encode() + replies = [] + request = SimpleNamespace( + headers={ + "Authorization": "Bearer developer", + "Content-Length": str(len(payload)), + }, + rfile=io.BytesIO(payload), + reply=lambda code, body: replies.append((code, body)), + ) + broker.Handler.do_POST(request) + assert replies[0][0] == expected + assert bool(upstream) == (expected == 200) diff --git a/tests/test_github_software_factory.py b/tests/test_github_software_factory.py index a3346290..9ff731a1 100644 --- a/tests/test_github_software_factory.py +++ b/tests/test_github_software_factory.py @@ -185,7 +185,11 @@ def request(url, method="GET", body=None): def test_developer_waits_for_review_findings_after_test_failure(monkeypatch, worker): - monkeypatch.setattr(worker, "gh", lambda *args: [{"head": {"sha": "a" * 40}}]) + monkeypatch.setattr( + worker, + "gh", + lambda *args: [{"head": {"sha": "a" * 40, "ref": "factory/issue-1"}}], + ) monkeypatch.setattr(worker, "open_issues", lambda: []) monkeypatch.setattr( worker, "statuses", lambda sha: {"software-factory/tests": "failure"} @@ -224,11 +228,160 @@ def test_developer_skips_closed_source_issue(monkeypatch, worker): monkeypatch.setattr( worker, "gh", - lambda *args: [{"number": 7, "head": {"sha": "a" * 40}, "body": "Closes #1"}], + lambda *args: [ + { + "number": 7, + "head": {"sha": "a" * 40, "ref": "factory/issue-1"}, + "body": "Closes #1", + } + ], ) worker.developer() +@pytest.mark.parametrize( + "state,reason,ready", + [ + ("open", None, False), + ("closed", "not_planned", False), + ("closed", "completed", True), + ], +) +def test_dependencies_require_completed_issues( + worker, monkeypatch, state, reason, ready +): + def gh(method, path, body=None): + assert (method, path) == ("GET", "/issues/1") + return {"state": state, "state_reason": reason} + + monkeypatch.setattr(worker, "gh", gh) + assert ( + worker.dependencies_complete({"body": "Small feature\nDepends on: #1\n"}) + is ready + ) + + +def test_developer_lanes_do_not_block_on_other_lane_pr(worker, monkeypatch): + worker.CONFIG.update(developer_lane=0, developer_lanes=2) + issues = [ + { + "number": 1, + "title": "API", + "body": "", + "labels": [{"name": "ready-for-dev"}], + }, + {"number": 2, "title": "UI", "body": "", "labels": [{"name": "ready-for-dev"}]}, + ] + monkeypatch.setattr(worker, "open_issues", lambda: issues) + + def gh(method, path, body=None): + if path.startswith("/pulls?"): + return [{"number": 9, "head": {"ref": "factory/issue-1", "sha": "a" * 40}}] + assert path == "/factory/bootstrap" + raise LookupError("selected own lane") + + monkeypatch.setattr(worker, "gh", gh) + with pytest.raises(LookupError, match="selected own lane"): + worker.developer() + + +def test_developer_lane_never_takes_another_lanes_issue(worker, monkeypatch): + worker.CONFIG.update(developer_lane=0, developer_lanes=2) + monkeypatch.setattr( + worker, + "open_issues", + lambda: [{"number": 1, "labels": [{"name": "ready-for-dev"}]}], + ) + + def gh(method, path, body=None): + assert path.startswith("/pulls?") + return [] + + monkeypatch.setattr(worker, "gh", gh) + worker.developer() + + +def test_accepted_stale_pr_uses_native_branch_update(worker, monkeypatch): + issue = {"number": 2, "labels": [{"name": "ready-for-dev"}]} + monkeypatch.setattr(worker, "open_issues", lambda: [issue]) + monkeypatch.setattr( + worker, "statuses", lambda sha: {"software-factory/review": "success"} + ) + calls = [] + + def gh(method, path, body=None): + calls.append((method, path, body)) + if path.startswith("/pulls?"): + return [{"number": 3, "head": {"sha": "a" * 40, "ref": "factory/issue-2"}}] + if path == "/git/ref/heads/main": + return {"object": {"sha": "b" * 40}} + if path.startswith("/compare/"): + return {"status": "diverged"} + assert path == "/factory/update-branch" + return {"message": "Updating branch"} + + monkeypatch.setattr(worker, "gh", gh) + worker.developer() + assert calls[-1] == ( + "POST", + "/factory/update-branch", + {"number": 3, "sha": "a" * 40}, + ) + + +@pytest.mark.parametrize("code", [404, 503]) +def test_dependency_lookup_error_isolated_to_missing_issue(worker, monkeypatch, code): + from urllib.error import HTTPError + + def gh(*args): + raise HTTPError("http://gateway", code, "failure", {}, None) + + monkeypatch.setattr(worker, "gh", gh) + if code == 404: + assert not worker.dependencies_complete({"body": "Depends on: #99999"}) + assert worker.dependencies_complete({"body": "Independent issue"}) + else: + with pytest.raises(HTTPError): + worker.dependencies_complete({"body": "Depends on: #1"}) + + +def test_conflicting_base_update_reuses_developer_revision(worker, monkeypatch): + from urllib.error import HTTPError + + issue = {"number": 2, "labels": [{"name": "ready-for-dev"}]} + monkeypatch.setattr(worker, "open_issues", lambda: [issue]) + monkeypatch.setattr( + worker, "statuses", lambda sha: {"software-factory/review": "success"} + ) + comments = [] + monkeypatch.setattr( + worker, "comment", lambda number, body: comments.append((number, body)) + ) + + def gh(method, path, body=None): + if path.startswith("/pulls?"): + return [ + { + "number": 3, + "body": "Closes #2", + "head": {"sha": "a" * 40, "ref": "factory/issue-2"}, + } + ] + if path == "/git/ref/heads/main": + return {"object": {"sha": "b" * 40}} + if path.startswith("/compare/"): + return {"status": "diverged"} + if path == "/factory/update-branch": + raise HTTPError("http://gateway", 422, "conflict", {}, None) + assert path == "/factory/bootstrap" + raise LookupError("entered existing developer revision") + + monkeypatch.setattr(worker, "gh", gh) + with pytest.raises(LookupError, match="entered existing developer revision"): + worker.developer() + assert comments and comments[0][0] == 3 + + def test_publish_rejects_broken_symlink(monkeypatch, worker): worker.PROJECT.mkdir() (worker.PROJECT / "broken").symlink_to("missing")