feat: add a role-scoped GitHub automation gateway - #559
Conversation
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
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.
Taste Rating: 🟢 Good taste
This is a well-designed, security-conscious permission gateway. The role separation is clean, the merge operation is properly fail-closed, and the SHA-guarded squash merge correctly mitigates the TOCTOU race between the eligibility check and the merge PUT. Path validation, constant-time token comparison, and the explicit blocklist of dangerous operations (main writes, force push, admin) are all solid.
No material issues found.
One minor observation (not blocking):
latest_statuses (line 67) fetches /commits/{sha}/statuses?per_page=100 without checking for additional pages. If a commit somehow has >100 statuses, a factory acceptance status on a later page would be missing from the result. The merge gate would still reject (the statuses.get(c, {}).get("state") check returns None != "success"), so this is fail-closed for the critical factory statuses. The broader all(s["state"] == "success" for s in statuses.values()) check on line 149 could theoretically miss a non-factory failing status on a later page, but this is defense-in-depth and 100+ statuses per commit is extremely rare in practice. No action needed unless this gateway is used on very high-traffic repositories.
The test suite (17 tests) exercises real permitted() and merge() code paths with parametrized failure modes, asserting on outputs and write-side effects rather than just mocking calls. Good coverage of the authorization matrix and merge rejection cases.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟢 LOW
The gateway keeps the GitHub credential in the control plane, uses constant-time token comparison, validates all paths and SHAs, and the merge operation is fail-closed with SHA guarding. No role can write to main, force push, or administer the repository. The code is a reference script within a skill directory, not application code.
VERDICT: ✅ Worth merging
KEY INSIGHT: The SHA-guarded squash merge correctly closes the TOCTOU window between eligibility verification and the merge API call, and the fail-closed pagination check on check-runs prevents merging with incomplete CI evidence.
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 is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
all-hands-bot
left a comment
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.
Summary
This PR adds a repository-scoped GitHub gateway script that keeps the host credential out of Docker automation workers by exposing role-specific operations (triage, developer, reviewer, watchdog) through a local HTTP proxy. The merge gate independently rechecks PR state, ancestry, statuses, check-runs, and mergeability before a SHA-guarded squash merge. The authorization model is well-designed: no role can write main, force push, administer the repo, or invoke an unguarded merge. Tests are thorough (17 tests covering authorization, merge fail-closed scenarios, and invalid identity rejection) and all pass without credentials or network access.
Taste Rating: 🟡 Acceptable - Works well, with two issues worth addressing.
Findings
1. Statuses pagination is not validated (inconsistent with check-runs)
The merge function explicitly validates that check-runs pagination is complete (check_page.get("total_count", len(checks)) == len(checks) on line 150), but latest_statuses (lines 65-69) fetches only the first page of statuses with no completeness check. If a commit has >100 commit statuses, the blanket all(s["state"] == "success" for s in statuses.values()) check on line 149 would only see the first 100, potentially missing failing statuses on later pages. The required contexts (software-factory/tests, software-factory/review) are still checked with fail-closed logic (missing context -> statuses.get(c, {}) returns {} -> None != "success" -> rejected), so the practical impact is limited to non-required CI statuses. Still, the inconsistency means the "other CI results" check can be silently bypassed with enough statuses.
2. Force-push guard uses identity check instead of truthiness check
Line 114: body.get("force") is not True blocks the boolean True but allows truthy non-boolean values like the string "true" or the integer 1. In Python, 1 is not True evaluates to True (different objects), so {"sha": "...", "force": 1} would pass the permission check. Whether GitHub's API would honor a non-boolean force value depends on their implementation, but the intent is clearly to prevent force push, and the check doesn't fully achieve that. A truthiness check (not body.get("force")) would be more robust. The practical risk is mitigated by the merge gate's SHA verification, which would reject a merge if the branch head was force-pushed to a different commit.
3. Missing role key in control file crashes handler outside try/except
Lines 166-173: The CONTROL[r] lookup in the role authentication generator expression is outside the try/except block (lines 178-233). If the control file is missing a role key, the handler thread crashes with an unhandled KeyError instead of returning a clean error. Adding a .get(r, "") fallback or validating all four keys in configure() would make this more robust.
Risk Assessment
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟡 MEDIUM
The gateway implements a security-critical permission boundary. The authorization model is sound and the merge gate is thorough (SHA guard, ancestry check, CI verification, pagination check for check-runs). The two findings are low-to-medium severity: the statuses pagination gap could theoretically allow a merge with missed failing non-required CI, and the force-push guard has a theoretical bypass. Neither is easily exploitable given the merge gate's SHA verification, but both should be tightened for a security-critical component.
VERDICT:
✅ Worth merging: Core logic is sound, minor improvements suggested. The authorization model and merge gate are well-designed. The findings are hardening opportunities rather than blocking issues.
KEY INSIGHT:
The merge gate's SHA-guarded squash merge is the critical security invariant - it prevents the TOCTOU race between eligibility check and merge execution, and it makes the force-push guard's theoretical bypass non-exploitable.
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.
all-hands-bot
left a comment
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.
🟡 Acceptable - Works well; two actionable findings around merge-gate completeness and error handling.
Summary
This PR adds a repository-scoped GitHub gateway that exposes role-specific operations (triage, developer, reviewer, watchdog) to Docker automation workers while keeping upstream GitHub credentials in the control plane. The authorization model is well-designed: permitted() uses strict re.fullmatch patterns, path sanitization blocks traversal characters, secrets.compare_digest prevents timing attacks on role tokens, and the merge gate is fail-closed across head SHA, base ancestry, mergeability, CI statuses, and check-runs completeness. Repository placement is correct - this is a skill reference script, not SDK or agent-server behavior.
Findings
1. Statuses pagination not checked for completeness (merge gate asymmetry)
The merge gate carefully verifies check-runs pagination completeness via check_page.get("total_count", len(checks)) == len(checks), but latest_statuses() fetches only the first 100 entries from /commits/{sha}/statuses?per_page=100 with no equivalent completeness check. If a commit has >100 status entries, a failed third-party CI status beyond page 1 would be silently absent from statuses.values(), and the all(s["state"] == "success" for s in statuses.values()) check would pass without seeing it. The required factory contexts would likely be in the first page, but the broader "all statuses must pass" invariant could be violated. Consider adding a total_count comparison for statuses as well, or documenting why only the first page is sufficient.
2. URLError not handled in request handler
The exception handling in do_POST catches HTTPError and (ValueError, KeyError, TypeError) but not urllib.error.URLError. Non-HTTP network failures (DNS resolution failure, connection refused, socket timeout exceeding the 60s/90s urlopen timeout) would propagate as unhandled exceptions in the handler thread. While ThreadingHTTPServer keeps the server alive, the client receives a connection reset with no JSON error response. For a merge service where the watchdog may retry on ambiguous failures, returning a structured 502/504 error would make failure semantics cleaner.
Risk Assessment
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The gateway implements a security-critical permission boundary with a well-structured authorization model and thorough fail-closed merge gate. The two findings above are completeness gaps rather than privilege escalation paths - the core security properties (no main writes, no force push, no admin operations, SHA-guarded merge) hold. Tests are real and exercise actual code paths rather than mock-only assertions.
VERDICT: ✅ Worth merging - core logic is sound, address the two findings as follow-ups.
KEY INSIGHT: The authorization and merge-gate design is solid; the gaps are in edge-case completeness (statuses pagination) and error-path robustness (URLError), not in the permission model itself.
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
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.
Summary
This PR adds a repository-scoped GitHub credential gateway (github_factory_gateway.py) that keeps upstream GitHub tokens in a trusted control-plane process and exposes role-specific operations (triage, developer, reviewer, watchdog) to Docker automation workers via opaque bearer grants. The merge operation independently rechecks PR state, head SHA, base ancestry, mergeability, commit statuses, and check-runs before issuing a SHA-guarded squash merge. 37 tests pass without credentials or network access.
The security design is strong: consistent fail-closed defaults, constant-time token comparison, no credential fallback, explicit permission filtering per role/method/path, SHA-pinned merge to prevent head-update races, and pagination guards that reject rather than silently truncate. The permitted() function correctly denies main writes, force pushes, repository administration, and unguarded merges for every role.
Observations (non-blocking)
-
No TLS on the gateway transport. Worker bearer tokens travel in cleartext over the Docker bridge network (
172.17.0.1:19102). The docs mention restricting the bind address to the Docker network, which is the practical mitigation, but adding a note about TLS or a Unix-domain socket would strengthen the trust-boundary documentation for operators who may not realize the tokens are unencrypted on the wire. -
Repository boundary. This is security-critical executable HTTP server code in a skills registry. The repo's own guidance says scripts under
skills/are allowed, and this script supports theopenhands-automationskill, so it is within scope. However, given the credential-handling responsibility, consider whether this gateway should eventually graduate to a dedicated plugin or theautomationrepo alongside other dispatch-lifecycle infrastructure, or at minimum carry aREADME.mdwith operational hardening guidance beyond the reference doc. -
Check-runs pagination uses a different strategy than status pagination.
latest_statuses()iterates up to 10 pages;merge()fetches one page of check-runs and guards withtotal_count == len(check_runs). Both are sound (the total_count check rejects incomplete pagination), but the inconsistency could surprise future maintainers. Not a bug.
Inline comment
See the inline comment on the /factory/bootstrap path regarding the one code path that writes to main outside the permitted() filter.
[RISK ASSESSMENT]
- [Overall PR] Risk Assessment: 🟡 MEDIUM
The gateway is well-designed with strong fail-closed defaults, but it is security-critical credential-handling infrastructure. The bootstrap endpoint bypasses thepermitted()filter (mitigated by an empty-repo check), and the cleartext transport for bearer tokens depends on network isolation for its security guarantee. No blocking issues found.
VERDICT: ✅ Worth merging. Core logic is sound, security design is thorough, and tests cover the important authorization and merge-gate paths.
KEY INSIGHT: The gateway's defense-in-depth is well-structured: the permitted() function is the primary operation filter, GitHub's fine-grained token permissions are the secondary layer, and the SHA-pinned merge request is the tertiary guard against TOCTOU races.
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
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.
Summary
The gateway is well-designed: fail-closed merge gates, role-separated credentials, constant-time token comparison, path traversal blocking, and thorough pagination/incomplete-scan refusal. The test suite exercises real code paths without mocks-of-the-unit-under-test and covers the important authorization and merge-rejection matrix. The repository placement is correct - a skill script and reference docs in skills/openhands-automation/.
Two findings below, both minor. No blocking issues.
Inline findings
-
/factory/bootstrapwrites tomainvia Contents API, bypassingpermitted()(line 278): Thepermitted()function blocksPUT /contents/*for all roles (explicitly tested), and the PR description states "no role can directly update main." However,/factory/bootstrapcallsPUT /contents/.gitkeepwithbranch: "main"directly, outside thepermitted()filter. The body is hardcoded so it cannot write arbitrary content, making this a deliberate, constrained exception. Worth documenting this exception explicitly in the reference doc's role table, since the current "No role can directly update main" statement has this one carve-out. -
Content-Lengthdirect indexing after.get(line 258): Line 255 usesself.headers.get("Content-Length", "0")(safe default), but line 258 usesself.headers["Content-Length"](direct indexing). If the header is absent, the size check passes (0 bytes) and then the direct index raisesKeyError, caught at line 316 as a 400. Functionally safe but inconsistent - consider using the same.getpattern or reading the value once into a local variable.
Risk assessment
- Overall PR: LOW
The gateway fails closed on every uncertain path (incomplete pagination, missing CI, stale base, network errors). Credential separation is enforced at startup and per-request. The merge TOCTOU is mitigated by including the reviewed SHA in the final merge API call. No secrets are committed. The script is optional and not deployed until credentials are supplied.
Verdict: Worth merging.
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 is merge-ready.Was this review helpful? React with thumbs up or thumbs down 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
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.
Overview
This PR adds a well-structured repository-scoped GitHub gateway that mediates between Docker automation workers and the GitHub API using four role-specific tokens (triage, developer, reviewer, watchdog). The design is deny-by-default, fail-closed on all error paths, and keeps GitHub credentials in the control plane while workers receive only opaque role grants. The merge gate independently re-checks PR state, base ancestry, CI evidence, and commit statuses before a SHA-guarded squash merge. Test coverage is thorough (49 tests covering authorization, merge gates, credential configuration, pagination, and concurrency).
No critical bugs or security holes found. Two design observations below.
Observations
1. checks CI backend does not paginate check-runs
The checks backend (ci_passed, line 127) fetches only the first page of check-runs (per_page=100 without a page parameter). If a repository has more than 100 check-runs for a commit, total_count will exceed len(checks) and the function returns False - fail-closed, but the merge will be permanently blocked for that commit. The actions backend paginates properly (lines 133-151) with a 10-page / 1,000-item safety cap. The documentation recommends FACTORY_CI_BACKEND=actions for fine-grained PATs, but the default is checks. For repos with many CI integrations, this could cause confusing merge failures. Not a bug (it fails safely), but worth being aware of for operators using the default backend on busy repositories.
2. Bearer tokens traverse the Docker bridge in cleartext
The gateway serves plain HTTP on 172.17.0.1:19102. Worker role grants are sent as Authorization: Bearer headers over this unencrypted connection. The documentation says the bind address should be "restricted to that network," and the threat model correctly separates worker grants from upstream GitHub credentials. However, a compromised co-tenant container on the same Docker bridge could sniff a watchdog grant and invoke /factory/merge. Consider documenting this residual risk explicitly, or noting that operators who share Docker networks with untrusted containers should add TLS (e.g., via a reverse proxy).
Risk Assessment
- Overall PR risk: LOW
- The gateway is well-designed with strong defense-in-depth: deny-by-default permission filtering, constant-time token comparison, SHA-guarded merges, fail-closed CI checks, and strict path validation. The bootstrap-to-main carve-out is hardcoded and documented. No SDK documentation is added (correct for this repo). Tests exercise real code paths without mocks of the unit under test.
VERDICT: Worth merging.
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 thumbs up or thumbs down to give feedback.
| def ci_passed(role, sha): | ||
| """Verify the explicitly configured CI source; never infer success from 403.""" | ||
| if CI_BACKEND == "checks": | ||
| result = github(role, "GET", f"/commits/{sha}/check-runs?per_page=100") |
There was a problem hiding this comment.
The checks backend fetches only the first page of check-runs without pagination. If a repo has >100 check-runs for a commit, total_count != len(checks) returns False and the merge is permanently blocked for that SHA. This is fail-closed (safe), but unlike the actions backend (which paginates up to 10 pages), the checks backend has no pagination path. Worth a comment noting this limitation, or adding pagination to match the actions backend's approach.
| and pr["mergeable"] is True | ||
| and comparison["status"] in ("ahead", "identical") | ||
| and all(statuses.get(c, {}).get("state") == "success" for c in contexts) | ||
| and all(s["state"] == "success" for s in statuses.values()) |
There was a problem hiding this comment.
all(s["state"] == "success" for s in statuses.values()) requires every commit status on the SHA to be "success". Any third-party tool that publishes a "pending" status will block the merge even if the factory's own acceptance statuses pass. This is intentionally conservative, but operators should be aware that any external status publisher can gate merges. Consider documenting this stricter-than-expected behavior alongside the existing CI evidence section.
|
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. |
3 participants
Add a repository-scoped GitHub gateway that keeps the host credential out of Docker automation workers. Separate role tokens allow triage, branch publication, exact-commit review evidence, or guarded merge; no role can administer the repository, force push, write main directly, or invoke an unguarded merge.
Private repository snapshots are pinned to a commit. The merge operation rechecks the current head, base ancestry, acceptance statuses, mergeability, and CI before a SHA-guarded squash merge. Includes setup/trust-boundary documentation and authorization/merge rejection tests.
Closes #558. This is the lower, independently reviewable permission boundary for #557; the factory role scripts are in that dependent PR.
Validation: all 17 gateway tests pass without credentials or network access, and current PR CI passes. Live private snapshot retrieval, role-scoped triage writes, and denied-operation checks succeeded. The scheduled watchdog automatically merged a PR only after fresh independent review and all required tests passed at its current head. The watchdog has no comment-write grant; a live check confirmed it can read PRs while comment writes are denied.
Least-privilege credential follow-up
The gateway now requires four distinct upstream GitHub credentials, selects the authenticated role explicitly for ordinary calls, archive downloads, and guarded merges, and refuses missing/shared credentials without falling back to
gh auth token. Reviewer Contents permission is read-only; token permission requirements and the watchdog merge permission limitation are documented. Workers still receive opaque role grants only.Validation: 24 gateway authorization, merge-gate, credential-configuration, and concurrent HTTP-request tests passed; Ruff lint/format passed. This follow-up is not deployed until the four repository-scoped credentials are supplied.