BED-9185: add enterprise GitHub runner support - #29
Open
jaredcatkinson wants to merge 6 commits into
Open
Conversation
Collect enterprise runner groups, runners, memberships, and organization assignments through the PAT-backed enterprise API. Model enterprise and organization runner groups separately while retaining generic runner labels, compose repository access through group relationships, and normalize runner and runner-group identifiers and names across scopes. Add lookup, preprocessing, schema, and focused test coverage for inherited runner groups and enterprise runner resources.
Add node and edge descriptions for enterprise, organization, and repository runner types along with inherited runner-group and access relationships. Update the GH_Contains description to cover the expanded runner containment model.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change adds enterprise and organization runner resources, scoped runner graph models, inheritance and repository-access relationships, lookup utilities, schema updates, and tests for collection and graph resolution. ChangesRunner modeling and access flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SSOClient
participant EnterpriseResources
participant InputTables
participant RunnerModels
SSOClient->>EnterpriseResources: collect enterprise runner data
EnterpriseResources->>InputTables: emit typed runner records
InputTables->>RunnerModels: provide scoped runner data
RunnerModels->>RunnerModels: resolve inheritance and repository access
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/openhound_github/transforms.py (1)
55-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
ensure_optional_input_tablesdocstring.The function now creates enterprise runner tables, but the docstring still describes only zero-row branch-policy inputs. Update the documented contract to include enterprise runner resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhound_github/transforms.py` around lines 55 - 74, Update the docstring for ensure_optional_input_tables to document that it creates the enterprise organization, runner group, runner group organization, and runner group membership input tables in addition to the existing zero-row branch-policy tables.tests/test_runner_models.py (2)
251-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the row-order assertion order-independent.
The query in
enterprise_organization_node_idshas noORDER BY. SQL does not guarantee row order without one. The assertion compares an ordered list, so the test depends on DuckDB scan order. Compare sets instead.💚 Proposed fix
- assert GithubLookup(connection).enterprise_organization_node_ids("ENT_1") == [ - ("ORG_1",), - ("ORG_2",), - ] + assert set(GithubLookup(connection).enterprise_organization_node_ids("ENT_1")) == { + ("ORG_1",), + ("ORG_2",), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runner_models.py` around lines 251 - 254, Update the assertion for GithubLookup.enterprise_organization_node_ids("ENT_1") to compare sets rather than ordered lists, preserving the expected ("ORG_1",) and ("ORG_2",) rows while making the test independent of database row order.
257-283: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for missing enterprise tables.
LookupManager._find_all_objectsreturns[]forduckdb.CatalogException. Add a test with noenterprise_*tables. Assert that group lookup returnsNoneand runner lookup returns[].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runner_models.py` around lines 257 - 283, Add a test alongside test_inherited_org_runner_group_lookup_resolves_all_and_selected_assignments that uses an empty DuckDB schema without creating any enterprise_* tables. Verify the group lookup returns None when tables are absent, while the runner lookup returns an empty list, covering LookupManager._find_all_objects handling of duckdb.CatalogException.Source: Linters/SAST tools
src/openhound_github/lookup.py (1)
93-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog ambiguous inherited runner-group matches.
The method returns
Nonewhen the query matches zero rows or more than one row. The caller insrc/openhound_github/models/runner.pythen drops theGH_InheritedFromedge and the composedGH_CanUseRunneredges without any signal. An operator cannot distinguish "no enterprise data collected" from "duplicate group name across enterprises". Add a debug or warning log for the ambiguous case.🔭 Proposed observability improvement
- if not rows or len(rows) != 1: - return None + if not rows: + return None + if len(rows) != 1: + logger.warning( + "Ambiguous inherited runner group '%s' for organization '%s': " + "%d candidate enterprise groups; skipping inheritance edges.", + group_name, + org_node_id, + len(rows), + ) + return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhound_github/lookup.py` around lines 93 - 97, Update the lookup method containing the rows validation to log a debug or warning message when multiple rows are returned, identifying the ambiguous runner-group match before returning None. Keep the no-row case silent and preserve the existing return behavior for both zero and duplicate matches.src/openhound_github/models/runner.py (1)
481-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared repository-visibility resolution.
OrgRunnerGroupAccess.repository_node_idsandOrgRunnerGroupMembership._can_use_runner_edges(Lines 602-611) resolve repository node IDs with identical logic:allmaps torepository_node_ids_for_org,privatemaps toprivate_repository_node_ids_for_org, and any other value maps toaccessible_repo_node_ids. The two copies can diverge when GitHub adds a visibility value. Move the logic into one shared helper or a mixin.♻️ Proposed refactor
+def _repository_node_ids_for_visibility( + lookup, visibility: str | None, org_login: str, accessible_repo_node_ids: list[str] +): + if visibility == "all": + return lookup.repository_node_ids_for_org(org_login) + if visibility == "private": + return lookup.private_repository_node_ids_for_org(org_login) + return [(repo_node_id,) for repo_node_id in accessible_repo_node_ids]`@property` def repository_node_ids(self): - if self.runner_group_visibility == "all": - return self._lookup.repository_node_ids_for_org(self.org_login) - if self.runner_group_visibility == "private": - return self._lookup.private_repository_node_ids_for_org(self.org_login) - return [(repo_node_id,) for repo_node_id in self.accessible_repo_node_ids] + return _repository_node_ids_for_visibility( + self._lookup, + self.runner_group_visibility, + self.org_login, + self.accessible_repo_node_ids, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhound_github/models/runner.py` around lines 481 - 487, Extract the runner-group visibility mapping from OrgRunnerGroupAccess.repository_node_ids and OrgRunnerGroupMembership._can_use_runner_edges into one shared helper or mixin. Preserve the existing all, private, and fallback accessible_repo_node_ids behavior, and update both callers to use the shared implementation so future visibility values are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhound_github/models/__init__.py`:
- Around line 41-51: Restore the deprecated RunnerGroup compatibility export in
the models package, including the corresponding symbol in __all__ if present, so
existing from openhound_github.models import RunnerGroup imports continue to
work. Keep the new runner-group types unchanged and expose RunnerGroup as an
adapter or alias to the replacement API.
In `@src/openhound_github/resources/organization.py`:
- Around line 1238-1250: Wrap the client.paginate flow in the runner-group
membership transformer with request-failure handling. Log the pagination error
using the existing logging approach, then stop processing that runner group and
yield no membership rows when the endpoint request fails; preserve normal
pagination and row generation on success.
In `@src/openhound_github/runner_ids.py`:
- Around line 1-6: Update runner_group_node_id and runner_node_id to reject a
None scope_node_id before constructing identifiers, either by making the
parameter required or raising an appropriate error. Preserve valid scoped
identifier formatting and prevent any None_runner_* values from being returned.
---
Nitpick comments:
In `@src/openhound_github/lookup.py`:
- Around line 93-97: Update the lookup method containing the rows validation to
log a debug or warning message when multiple rows are returned, identifying the
ambiguous runner-group match before returning None. Keep the no-row case silent
and preserve the existing return behavior for both zero and duplicate matches.
In `@src/openhound_github/models/runner.py`:
- Around line 481-487: Extract the runner-group visibility mapping from
OrgRunnerGroupAccess.repository_node_ids and
OrgRunnerGroupMembership._can_use_runner_edges into one shared helper or mixin.
Preserve the existing all, private, and fallback accessible_repo_node_ids
behavior, and update both callers to use the shared implementation so future
visibility values are handled consistently.
In `@src/openhound_github/transforms.py`:
- Around line 55-74: Update the docstring for ensure_optional_input_tables to
document that it creates the enterprise organization, runner group, runner group
organization, and runner group membership input tables in addition to the
existing zero-row branch-policy tables.
In `@tests/test_runner_models.py`:
- Around line 251-254: Update the assertion for
GithubLookup.enterprise_organization_node_ids("ENT_1") to compare sets rather
than ordered lists, preserving the expected ("ORG_1",) and ("ORG_2",) rows while
making the test independent of database row order.
- Around line 257-283: Add a test alongside
test_inherited_org_runner_group_lookup_resolves_all_and_selected_assignments
that uses an empty DuckDB schema without creating any enterprise_* tables.
Verify the group lookup returns None when tables are absent, while the runner
lookup returns an empty list, covering LookupManager._find_all_objects handling
of duckdb.CatalogException.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 473bfc7f-1a3a-49fc-8f3b-4f6d2193a5f9
📒 Files selected for processing (24)
descriptions/edges/GH_AssignedTo.mddescriptions/edges/GH_CanUseRunner.mddescriptions/edges/GH_Contains.mddescriptions/edges/GH_GrantsAccessTo.mddescriptions/edges/GH_InheritedFrom.mddescriptions/nodes/GH_EnterpriseRunner.mddescriptions/nodes/GH_EnterpriseRunnerGroup.mddescriptions/nodes/GH_OrgRunner.mddescriptions/nodes/GH_OrgRunnerGroup.mddescriptions/nodes/GH_RepoRunner.mdextension/schema.jsonsrc/openhound_github/kinds/edges.pysrc/openhound_github/kinds/nodes.pysrc/openhound_github/lookup.pysrc/openhound_github/main.pysrc/openhound_github/models/__init__.pysrc/openhound_github/models/runner.pysrc/openhound_github/resources/enterprise.pysrc/openhound_github/resources/organization.pysrc/openhound_github/runner_ids.pysrc/openhound_github/transforms.pytests/test_enterprise_resources.pytests/test_runner_models.pytests/test_runner_resources.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhound_github/resources/organization.py`:
- Around line 1238-1261: In the runner pagination loop for
org_runner_group_memberships, narrow the broad exception handler around
client.paginate to catch only requests.RequestException. Keep the existing
request-error logging and early return for transport failures, while allowing
KeyError, ValueError, and paginator/data-processing errors to propagate visibly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4ce5949-d047-4637-ba45-cd34b8d59028
📒 Files selected for processing (2)
src/openhound_github/resources/organization.pytests/test_runner_resources.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GH_EnterpriseRunnerGroupandGH_EnterpriseRunnerGH_OrgRunnerGroupwhile preserving the genericGH_RunnerGroupandGH_Runnerlabels across scopesGH_AssignedTo,GH_InheritedFrom,GH_GrantsAccessTo, and composedGH_CanUseRunneredgesValidation
uv run pytest tests/test_runner_models.py tests/test_enterprise_resources.py tests/test_runner_resources.pyScope
Summary by CodeRabbit