BED-8788: add PAT-backed enterprise SSO collection and fix repeated enterprise SSO pagination#22
Open
jaredcatkinson wants to merge 14 commits into
Open
Conversation
…nterprise SSO pagination - add optional pat_token support for enterprise app sources - create a dedicated SSO client/context when a PAT is provided - gate enterprise SSO collections on the presence of that client - run enterprise SAML provider and external identity collection with the PAT client - fix enterprise resource iteration so SSO collection does not repeat per org page - give PAT-backed requests the same retry/backoff behavior as the app client - add enterprise resource coverage tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds PAT-backed SSO, environment-scoped SAML graph assets and identity mapping, environment deployment relationships, workflow containment queries, empty-input table initialization, and related tests and documentation. ChangesEnterprise SAML and graph integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Source as enterprise_app
participant Resources as enterprise_resources
participant SSO as sso_client
participant API as GraphQL API
participant Models as SAML and environment graph assets
Source->>Resources: provide optional PAT-backed sso_client
Resources->>SSO: request EnterpriseSAMLProvider
SSO->>API: send GraphQL request
API-->>SSO: return provider data
SSO-->>Resources: yield environment-scoped provider
Resources->>Models: build SAML assets and deployment relationships
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (5)
tests/test_enterprise_resources.py (4)
55-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the post-call assertion.
Only the call count is checked; the actual GraphQL variables (e.g.,
enterprise_name) sent in the post body aren't verified. A regression that sends the wrong enterprise slug/name would pass this test silently.Suggested strengthening
assert len(rows) == 1 assert rows[0].id == "E_1" assert len(client.post_calls) == 1 + _, posted_json = client.post_calls[0] + assert posted_json["variables"]["slug"] == "acme"🤖 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_enterprise_resources.py` at line 55, Strengthen the enterprise resources test by verifying the actual GraphQL payload, not just the number of post calls. In the test around client.post_calls, assert the posted body/variables include the expected enterprise_name (and any other key GraphQL variables) so a wrong slug/name in the request will fail. Use the existing client.post_calls structure in tests/test_enterprise_resources.py to inspect the single post call and validate its variables.
89-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the pagination call arguments.
Only
len(client.paginate_calls) == 1is checked (line 93); the kwargs passed topaginate(query, variables including the enterprise node id/cursor) aren't verified. Given the PR's stated fix is specifically about correct enterprise resource iteration/pagination variables, asserting onclient.paginate_calls[0][1]would give stronger regression protection for the exact bug this PR fixes.🤖 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_enterprise_resources.py` around lines 89 - 93, The test for enterprise resource iteration only checks that paginate was called once, but it does not verify the query or variables used. Update the test around the paginate call in the enterprise resources flow to assert the captured arguments from client.paginate_calls[0][1], including the enterprise node id and cursor-related variables, so the regression coverage matches the pagination fix.
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReliance on
__wrapped__couples the test to decorator internals.Accessing
enterprise_organizations.__wrapped__assumes the resource/transformer decorator preserves the original callable viafunctools.wraps(or equivalent). If the decorator implementation changes (e.g., wraps differently or omits__wrapped__), this test breaks with an unrelatedAttributeErrorrather than a meaningful assertion failure. Consider invoking the resource through its public/documented test surface if the underlying decorator (likely dlt's@dlt.transformer) exposes one, or add a comment explaining why__wrapped__is required here.🤖 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_enterprise_resources.py` at line 87, The test is reaching into `enterprise_organizations.__wrapped__`, which ties it to decorator internals and can fail with an `AttributeError` if the wrapper implementation changes. Update the test to use the public/documented way to invoke the `enterprise_organizations` resource/transformer in `tests/test_enterprise_resources.py`, or add a short justification comment if calling the underlying callable directly is truly required. Keep the assertion focused on the emitted rows rather than the decorator behavior.
58-93: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMissing coverage for the SSO-gated resources.
The PR's core change is gating
enterprise_saml_provider/enterprise_external_identitiesonsso_clientpresence, but this test file only coversenterpriseandenterprise_organizations. No test verifies the skip/warning behavior whensso_clientis absent, nor the happy path when it is present.🤖 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_enterprise_resources.py` around lines 58 - 93, Add test coverage for the SSO-gated enterprise resources so the new gating behavior is verified. In the enterprise resources test module, add one case for the absence of sso_client that confirms enterprise_saml_provider and enterprise_external_identities are skipped or warn as intended, and another case for the present sso_client path that confirms both resources execute normally through the enterprise_saml_provider and enterprise_external_identities entry points. Use the existing SourceContext, _FakeClient, and enterprise_data setup patterns to keep the tests aligned with the current enterprise_organizations coverage.src/openhound_github/graphql.py (1)
169-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate field list between the two SAML queries.
ENTERPRISE_SAML_PROVIDER_QUERY'ssamlIdentityProviderfield selection is byte-for-byte duplicated fromENTERPRISE_SAML_QUERY(Lines 125-131). Consider extracting a shared GraphQL fragment for the provider fields to keep both queries in sync as the schema evolves.♻️ Example using a shared fragment
fragment SamlIdentityProviderFields on SamlIdentityProvider { id issuer ssoUrl digestMethod signatureMethod idpCertificate }Then reference
...SamlIdentityProviderFieldsin both queries instead of repeating the field list.🤖 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/graphql.py` around lines 169 - 188, The SAML provider field selection is duplicated between ENTERPRISE_SAML_QUERY and ENTERPRISE_SAML_PROVIDER_QUERY, so extract the repeated samlIdentityProvider fields into a shared GraphQL fragment and reuse it in both query strings. Update the query definitions in graphql.py to reference the new fragment from the existing ENTERPRISE_SAML_QUERY and ENTERPRISE_SAML_PROVIDER_QUERY symbols so future schema changes only need one edit.
🤖 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/enterprise.py`:
- Around line 70-96: Add the same graceful error handling used in enterprise()
around the new enterprise_organizations GraphQL pagination flow. Wrap the
ctx.client.paginate("/graphql", ...) loop in try/except (including HTTPError or
a broader request exception if consistent with the module), log the failure with
enough context, and return/stop cleanly instead of letting the exception escape.
Use the enterprise_organizations paginator setup and the ctx.client.paginate
call as the key locations to update.
- Around line 345-372: Add error handling around the SSO GraphQL request in
enterprise_saml_provider: the unguarded client.post("/graphql",
json=data).json() call can raise on non-2xx or network failures and should
degrade gracefully like enterprise(). Wrap the POST/JSON fetch in a try/except,
log a warning or error with ctx.enterprise_name and the exception details, then
return early when the SSO-backed lookup fails; keep the existing saml_provider
parsing and yield logic unchanged when the request succeeds.
---
Nitpick comments:
In `@src/openhound_github/graphql.py`:
- Around line 169-188: The SAML provider field selection is duplicated between
ENTERPRISE_SAML_QUERY and ENTERPRISE_SAML_PROVIDER_QUERY, so extract the
repeated samlIdentityProvider fields into a shared GraphQL fragment and reuse it
in both query strings. Update the query definitions in graphql.py to reference
the new fragment from the existing ENTERPRISE_SAML_QUERY and
ENTERPRISE_SAML_PROVIDER_QUERY symbols so future schema changes only need one
edit.
In `@tests/test_enterprise_resources.py`:
- Line 55: Strengthen the enterprise resources test by verifying the actual
GraphQL payload, not just the number of post calls. In the test around
client.post_calls, assert the posted body/variables include the expected
enterprise_name (and any other key GraphQL variables) so a wrong slug/name in
the request will fail. Use the existing client.post_calls structure in
tests/test_enterprise_resources.py to inspect the single post call and validate
its variables.
- Around line 89-93: The test for enterprise resource iteration only checks that
paginate was called once, but it does not verify the query or variables used.
Update the test around the paginate call in the enterprise resources flow to
assert the captured arguments from client.paginate_calls[0][1], including the
enterprise node id and cursor-related variables, so the regression coverage
matches the pagination fix.
- Line 87: The test is reaching into `enterprise_organizations.__wrapped__`,
which ties it to decorator internals and can fail with an `AttributeError` if
the wrapper implementation changes. Update the test to use the public/documented
way to invoke the `enterprise_organizations` resource/transformer in
`tests/test_enterprise_resources.py`, or add a short justification comment if
calling the underlying callable directly is truly required. Keep the assertion
focused on the emitted rows rather than the decorator behavior.
- Around line 58-93: Add test coverage for the SSO-gated enterprise resources so
the new gating behavior is verified. In the enterprise resources test module,
add one case for the absence of sso_client that confirms
enterprise_saml_provider and enterprise_external_identities are skipped or warn
as intended, and another case for the present sso_client path that confirms both
resources execute normally through the enterprise_saml_provider and
enterprise_external_identities entry points. Use the existing SourceContext,
_FakeClient, and enterprise_data setup patterns to keep the tests aligned with
the current enterprise_organizations coverage.
🪄 Autofix (Beta)
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: 6b1f1ac8-1163-4a8d-9e47-6bb356ee1893
📒 Files selected for processing (5)
src/openhound_github/graphql.pysrc/openhound_github/helpers.pysrc/openhound_github/resources/enterprise.pysrc/openhound_github/source.pytests/test_enterprise_resources.py
Comment on lines
+70
to
+96
| paginator = GraphQLCursorPaginator( | ||
| page_info_path="data.enterprise.organizations.pageInfo", | ||
| cursor_variable="after", | ||
| cursor_field="endCursor", | ||
| has_next_field="hasNextPage", | ||
| ) | ||
| data = { | ||
| "query": ENTERPRISE_QUERY, | ||
| "variables": {"slug": ctx.enterprise_name, "after": None}, | ||
| } | ||
|
|
||
| for page_data in ctx.client.paginate( | ||
| "/graphql", | ||
| method="POST", | ||
| json=data, | ||
| paginator=paginator, | ||
| data_selector="data", | ||
| ): | ||
| for enterprise_object in page_data: | ||
| es_data = enterprise_object.get("enterprise", {}) | ||
| orgs = (es_data.get("organizations") or {}).get("nodes", []) | ||
| for org in orgs: | ||
| yield { | ||
| **org, | ||
| "enterprise_node_id": enterprise_data.id, | ||
| "enterprise_slug": ctx.enterprise_name, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add error handling around the new enterprise_organizations GraphQL pagination call.
Unlike enterprise() (Lines 53-62), which wraps its GraphQL POST in try/except with graceful logging, this newly-added independent pagination call has no error handling. Since client.paginate()/.post() raise HTTPError on non-2xx responses by default, a transient or permission error here will propagate as an unhandled exception and fail this resource entirely instead of degrading gracefully.
🛡️ Suggested fix
def enterprise_organizations(enterprise_data: Enterprise, ctx: SourceContext):
paginator = GraphQLCursorPaginator(
page_info_path="data.enterprise.organizations.pageInfo",
cursor_variable="after",
cursor_field="endCursor",
has_next_field="hasNextPage",
)
data = {
"query": ENTERPRISE_QUERY,
"variables": {"slug": ctx.enterprise_name, "after": None},
}
- for page_data in ctx.client.paginate(
- "/graphql",
- method="POST",
- json=data,
- paginator=paginator,
- data_selector="data",
- ):
- for enterprise_object in page_data:
- es_data = enterprise_object.get("enterprise", {})
- orgs = (es_data.get("organizations") or {}).get("nodes", [])
- for org in orgs:
- yield {
- **org,
- "enterprise_node_id": enterprise_data.id,
- "enterprise_slug": ctx.enterprise_name,
- }
+ try:
+ for page_data in ctx.client.paginate(
+ "/graphql",
+ method="POST",
+ json=data,
+ paginator=paginator,
+ data_selector="data",
+ ):
+ for enterprise_object in page_data:
+ es_data = enterprise_object.get("enterprise", {})
+ orgs = (es_data.get("organizations") or {}).get("nodes", [])
+ for org in orgs:
+ yield {
+ **org,
+ "enterprise_node_id": enterprise_data.id,
+ "enterprise_slug": ctx.enterprise_name,
+ }
+ except Exception as e:
+ logger.error(
+ f"Error in resource 'enterprise_organizations' processing enterprise '{ctx.enterprise_name}': {e}",
+ extra={"resource": "enterprise_organizations", "phase": "resource_iteration"},
+ )
+ return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| paginator = GraphQLCursorPaginator( | |
| page_info_path="data.enterprise.organizations.pageInfo", | |
| cursor_variable="after", | |
| cursor_field="endCursor", | |
| has_next_field="hasNextPage", | |
| ) | |
| data = { | |
| "query": ENTERPRISE_QUERY, | |
| "variables": {"slug": ctx.enterprise_name, "after": None}, | |
| } | |
| for page_data in ctx.client.paginate( | |
| "/graphql", | |
| method="POST", | |
| json=data, | |
| paginator=paginator, | |
| data_selector="data", | |
| ): | |
| for enterprise_object in page_data: | |
| es_data = enterprise_object.get("enterprise", {}) | |
| orgs = (es_data.get("organizations") or {}).get("nodes", []) | |
| for org in orgs: | |
| yield { | |
| **org, | |
| "enterprise_node_id": enterprise_data.id, | |
| "enterprise_slug": ctx.enterprise_name, | |
| } | |
| paginator = GraphQLCursorPaginator( | |
| page_info_path="data.enterprise.organizations.pageInfo", | |
| cursor_variable="after", | |
| cursor_field="endCursor", | |
| has_next_field="hasNextPage", | |
| ) | |
| data = { | |
| "query": ENTERPRISE_QUERY, | |
| "variables": {"slug": ctx.enterprise_name, "after": None}, | |
| } | |
| try: | |
| for page_data in ctx.client.paginate( | |
| "/graphql", | |
| method="POST", | |
| json=data, | |
| paginator=paginator, | |
| data_selector="data", | |
| ): | |
| for enterprise_object in page_data: | |
| es_data = enterprise_object.get("enterprise", {}) | |
| orgs = (es_data.get("organizations") or {}).get("nodes", []) | |
| for org in orgs: | |
| yield { | |
| **org, | |
| "enterprise_node_id": enterprise_data.id, | |
| "enterprise_slug": ctx.enterprise_name, | |
| } | |
| except Exception as e: | |
| logger.error( | |
| f"Error in resource 'enterprise_organizations' processing enterprise '{ctx.enterprise_name}': {e}", | |
| extra={"resource": "enterprise_organizations", "phase": "resource_iteration"}, | |
| ) | |
| return |
🤖 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/resources/enterprise.py` around lines 70 - 96, Add the
same graceful error handling used in enterprise() around the new
enterprise_organizations GraphQL pagination flow. Wrap the
ctx.client.paginate("/graphql", ...) loop in try/except (including HTTPError or
a broader request exception if consistent with the module), log the failure with
enough context, and return/stop cleanly instead of letting the exception escape.
Use the enterprise_organizations paginator setup and the ctx.client.paginate
call as the key locations to update.
Comment on lines
+345
to
+372
| client = ctx.sso_client | ||
| if not client: | ||
| logger.info( | ||
| "Skipping enterprise_saml_provider for enterprise '%s': no SSO client configured", | ||
| ctx.enterprise_name, | ||
| ) | ||
| return | ||
|
|
||
| data = { | ||
| "query": ENTERPRISE_SAML_QUERY, | ||
| "variables": {"slug": ctx.enterprise_name, "count": 1, "after": None}, | ||
| "query": ENTERPRISE_SAML_PROVIDER_QUERY, | ||
| "variables": {"slug": ctx.enterprise_name}, | ||
| } | ||
|
|
||
| for page_data in ctx.client.paginate( | ||
| "/graphql", | ||
| method="POST", | ||
| json=data, | ||
| paginator=paginator, | ||
| data_selector="data", | ||
| ): | ||
| for enterprise_object in page_data: | ||
| es_data = enterprise_object.get("enterprise", {}) | ||
| saml_provider = (es_data.get("ownerInfo") or {}).get("samlIdentityProvider") | ||
| if not saml_provider: | ||
| return | ||
| yield { | ||
| **{k: v for k, v in saml_provider.items() if k != "externalIdentities"}, | ||
| "enterprise_node_id": enterprise_data.id, | ||
| "enterprise_slug": ctx.enterprise_name, | ||
| } | ||
| response = client.post("/graphql", json=data).json() | ||
| enterprise_object = (response.get("data") or {}).get("enterprise", {}) | ||
| saml_provider = (enterprise_object.get("ownerInfo") or {}).get("samlIdentityProvider") | ||
| if not saml_provider: | ||
| logger.warning( | ||
| "No enterprise SAML provider returned for enterprise '%s'", | ||
| ctx.enterprise_name, | ||
| ) | ||
| return | ||
|
|
||
| yield { | ||
| **saml_provider, | ||
| "enterprise_node_id": enterprise_data.id, | ||
| "enterprise_slug": ctx.enterprise_name, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing error handling around the SSO-backed GraphQL POST.
client.post("/graphql", json=data).json() at Line 358 is unwrapped, unlike enterprise()'s equivalent call in the same file/diff which catches exceptions and logs gracefully (Lines 53-62). A non-2xx response (e.g., insufficient PAT scope, transient network failure) raises HTTPError by default and will crash this resource instead of degrading gracefully like its sibling.
🛡️ Suggested fix
- response = client.post("/graphql", json=data).json()
- enterprise_object = (response.get("data") or {}).get("enterprise", {})
- saml_provider = (enterprise_object.get("ownerInfo") or {}).get("samlIdentityProvider")
+ try:
+ response = client.post("/graphql", json=data).json()
+ except Exception as e:
+ logger.error(
+ f"Error in resource 'enterprise_saml_provider' processing enterprise '{ctx.enterprise_name}': {e}",
+ extra={"resource": "enterprise_saml_provider", "phase": "resource_iteration"},
+ )
+ return
+ enterprise_object = (response.get("data") or {}).get("enterprise", {})
+ saml_provider = (enterprise_object.get("ownerInfo") or {}).get("samlIdentityProvider")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| client = ctx.sso_client | |
| if not client: | |
| logger.info( | |
| "Skipping enterprise_saml_provider for enterprise '%s': no SSO client configured", | |
| ctx.enterprise_name, | |
| ) | |
| return | |
| data = { | |
| "query": ENTERPRISE_SAML_QUERY, | |
| "variables": {"slug": ctx.enterprise_name, "count": 1, "after": None}, | |
| "query": ENTERPRISE_SAML_PROVIDER_QUERY, | |
| "variables": {"slug": ctx.enterprise_name}, | |
| } | |
| for page_data in ctx.client.paginate( | |
| "/graphql", | |
| method="POST", | |
| json=data, | |
| paginator=paginator, | |
| data_selector="data", | |
| ): | |
| for enterprise_object in page_data: | |
| es_data = enterprise_object.get("enterprise", {}) | |
| saml_provider = (es_data.get("ownerInfo") or {}).get("samlIdentityProvider") | |
| if not saml_provider: | |
| return | |
| yield { | |
| **{k: v for k, v in saml_provider.items() if k != "externalIdentities"}, | |
| "enterprise_node_id": enterprise_data.id, | |
| "enterprise_slug": ctx.enterprise_name, | |
| } | |
| response = client.post("/graphql", json=data).json() | |
| enterprise_object = (response.get("data") or {}).get("enterprise", {}) | |
| saml_provider = (enterprise_object.get("ownerInfo") or {}).get("samlIdentityProvider") | |
| if not saml_provider: | |
| logger.warning( | |
| "No enterprise SAML provider returned for enterprise '%s'", | |
| ctx.enterprise_name, | |
| ) | |
| return | |
| yield { | |
| **saml_provider, | |
| "enterprise_node_id": enterprise_data.id, | |
| "enterprise_slug": ctx.enterprise_name, | |
| } | |
| client = ctx.sso_client | |
| if not client: | |
| logger.info( | |
| "Skipping enterprise_saml_provider for enterprise '%s': no SSO client configured", | |
| ctx.enterprise_name, | |
| ) | |
| return | |
| data = { | |
| "query": ENTERPRISE_SAML_PROVIDER_QUERY, | |
| "variables": {"slug": ctx.enterprise_name}, | |
| } | |
| try: | |
| response = client.post("/graphql", json=data).json() | |
| except Exception as e: | |
| logger.error( | |
| f"Error in resource 'enterprise_saml_provider' processing enterprise '{ctx.enterprise_name}': {e}", | |
| extra={"resource": "enterprise_saml_provider", "phase": "resource_iteration"}, | |
| ) | |
| return | |
| enterprise_object = (response.get("data") or {}).get("enterprise", {}) | |
| saml_provider = (enterprise_object.get("ownerInfo") or {}).get("samlIdentityProvider") | |
| if not saml_provider: | |
| logger.warning( | |
| "No enterprise SAML provider returned for enterprise '%s'", | |
| ctx.enterprise_name, | |
| ) | |
| return | |
| yield { | |
| **saml_provider, | |
| "enterprise_node_id": enterprise_data.id, | |
| "enterprise_slug": ctx.enterprise_name, | |
| } |
🤖 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/resources/enterprise.py` around lines 345 - 372, Add
error handling around the SSO GraphQL request in enterprise_saml_provider: the
unguarded client.post("/graphql", json=data).json() call can raise on non-2xx or
network failures and should degrade gracefully like enterprise(). Wrap the
POST/JSON fetch in a try/except, log a warning or error with ctx.enterprise_name
and the exception details, then return early when the SSO-backed lookup fails;
keep the existing saml_provider parsing and yield logic unchanged when the
request succeeds.
- add normalized SAML models for: - `SAML_ServiceProvider` - `SAML_Issuer` - `SAML_AssertionConsumerService` - wire normalized SAML resources into both organization and enterprise collection flows - unify enterprise and organization SAML handling onto shared models instead of separate enterprise-only variants - emit `SAML_Implements` edges from `GH_SamlIdentityProvider` to `SAML_ServiceProvider` - emit `SAML_TrustsIssuer` and `SAML_HasAssertionConsumerService` from dedicated normalized models - add `SAML_HasAccount` edges from the GitHub SAML service provider to mapped GitHub users when SAML account linkage exists - enrich `GH_SamlIdentityProvider` output with provider details such as issuer, SSO URL, signing configuration, certificate data, and foreign environment context - improve `GH_ExternalIdentity` mapping logic so foreign IdP matches can be constrained by provider-specific tenant/environment properties - support both organization and enterprise scope metadata on normalized SAML nodes - remove duplicated enterprise-specific SAML and external identity model implementations in favor of the shared path
jaredcatkinson
force-pushed
the
fix/BED-8788-enterprise-sso-pat
branch
from
July 14, 2026 05:20
c6970d1 to
2ef1c87
Compare
- replace alert-number-only naming with repository-aware names - set alert display names using the secret type when available - make secret scanning alert nodes easier to distinguish in graph views and search results
- move extension metadata into the package so OpenHound can discover it correctly - remove the repo-root `extension.yaml` - update SAML provider lookup behavior to align with the current collected schema - keep external identity resolution dependent on issuer and SSO URL derived context
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhound_github/resources/enterprise.py (1)
439-477: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd error handling around the
enterprise_external_identitypagination call.Like the previously-flagged
enterprise_organizationsandenterprise_saml_providergaps, this new pagination call (client.paginate(...)) has no try/except. A transient/permission error on the SSO client will propagate unhandled and fail the whole resource, unlike the graceful-degradation pattern used elsewhere in this file and inorganization.py'ssaml_provider/external_identities.🛡️ Suggested fix
- for page_data in client.paginate( - "/graphql", - method="POST", - json=data, - paginator=paginator, - data_selector="data", - ): - for enterprise_object in page_data: + try: + pages = client.paginate( + "/graphql", + method="POST", + json=data, + paginator=paginator, + data_selector="data", + ) + except Exception as e: + logger.error( + f"Error in resource 'enterprise_external_identity' processing enterprise '{ctx.enterprise_name}': {e}", + extra={"resource": "enterprise_external_identity", "phase": "resource_iteration"}, + ) + return + + for page_data in pages: + for enterprise_object in page_data:🤖 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/resources/enterprise.py` around lines 439 - 477, Wrap the `client.paginate(...)` iteration in `enterprise_external_identity` with the same targeted exception handling used by the existing `enterprise_organizations`, `enterprise_saml_provider`, or `organization.py` SAML flows. Log the pagination failure with enterprise context and return gracefully so transient or permission errors do not fail the entire resource, while preserving normal page processing.
🧹 Nitpick comments (7)
src/openhound_github/resources/enterprise.py (1)
478-489: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused SAML-provider fields
ExternalIdentityonly consumesenvironment_slug; the other keys are ignored here and never surface in the graph. Drop them unless you plan to add matching fields to the model/schema.🤖 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/resources/enterprise.py` around lines 478 - 489, Update the external identity construction loop to yield only the SAML-provider field consumed by ExternalIdentity: retain environment_slug and remove environment_node_id, environment_name, saml_provider_id, saml_provider_issuer, and saml_provider_sso_url.src/openhound_github/models/__init__.py (1)
96-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
"ExternalIdentity"entry in__all__.Harmless but confusing — appears at both line 99 and line 101.
🧹 Proposed fix
"SamlAssertionConsumerService", "SamlProvider", "SamlServiceProvider", - "ExternalIdentity", "SamlIssuer", "ExternalIdentity",🤖 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/__init__.py` around lines 96 - 101, Remove the duplicate "ExternalIdentity" entry from the __all__ list in src/openhound_github/models/__init__.py, keeping a single export entry alongside the other model symbols.src/openhound_github/models/external_identity.py (2)
202-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
idshadows the Python builtin.Minor style nit flagged by Ruff.
🤖 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/external_identity.py` at line 202, Rename the local variable id in the external identity unpacking assignment to a non-shadowing name, and update its uses within the surrounding function accordingly.Source: Linters/SAST tools
293-301: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThread
environment_typethrough here instead of inferring it fromenvironment_node_id.idp_for_environment(...)already has the SAML provider row; include the explicitenvironment_typecolumn and reuse it soservice_provider_node_idstays aligned with the normalized SAML nodes andSAML_HAS_ACCOUNTdoesn’t point at the wrong target.🤖 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/external_identity.py` around lines 293 - 301, Update ExternalIdentity.service_provider_node_id to obtain and reuse the explicit environment_type from idp_for_environment(...)’s SAML provider row, rather than inferring it from environment_node_id. Thread the environment_type column through the provider lookup and preserve the existing None behavior when required environment data is unavailable, ensuring the generated node ID matches normalized SAML nodes.src/openhound_github/models/saml_provider.py (1)
89-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate foreign-IdP detection logic, and its result is discarded.
detect_foreign_environmentduplicatesExternalIdentity.detect_foreign_idp(inexternal_identity.py) with divergent naming for the same IdPs ("PingOneUser"/"AZUser"/"Okta_User"here vs"pingone"/"entra"/"okta"there). Inas_node(line 105) the returnedforeign_idp_typeis discarded (_), so this duplication currently has no functional effect, but the divergence is a maintenance hazard if either side is later relied upon. Consider extracting a single shared helper.🤖 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/saml_provider.py` around lines 89 - 101, Remove the duplicate IdP detection logic from SAMLProvider.detect_foreign_environment and reuse the shared ExternalIdentity.detect_foreign_idp helper instead. Update as_node to preserve and use the helper’s canonical IdP values rather than discarding the returned foreign_idp_type, or remove the unused wrapper if no caller requires it.src/openhound_github/models/saml_service_provider.py (1)
90-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused unpacked variable
acs_url.Ruff flags
acs_urlas unused; onlysp_entity_idis consumed.🧹 Proposed fix
- acs_url, sp_entity_id = self.saml_route + _acs_url, sp_entity_id = self.saml_route🤖 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/saml_service_provider.py` around lines 90 - 93, Update the as_node property’s saml_route unpacking to avoid binding the unused acs_url value while preserving the existing use of sp_entity_id and subsequent scope_type logic.Source: Linters/SAST tools
src/openhound_github/kinds/edges.py (1)
143-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
SAML_TRUSTS_ISSUERassignment.Line 148 duplicates line 147.
🧹 Proposed fix
SAML_TRUSTS_ISSUER = "SAML_TrustsIssuer" -SAML_TRUSTS_ISSUER = "SAML_TrustsIssuer" SAML_ISSUES_ASSERTIONS_TO = "SAML_IssuesAssertionsTo"🤖 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/kinds/edges.py` around lines 143 - 155, Remove the duplicate SAML_TRUSTS_ISSUER assignment in the SAML edges constants, retaining a single definition with the existing value.
🤖 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/external_identity.py`:
- Around line 211-292: Update the match_key selection in the SYNCED_TO_GH_USER
block of _maps_to_user_edges to use "login" for Okta, matching the MAPS_TO_USER
block, while preserving "email" for PingOne and "name" as the default. Prefer
reusing a shared match-key computation if practical so both edge blocks cannot
diverge.
- Around line 273-278: Escape the IdP-supplied foreign_username before
interpolating it into the Cypher query assigned to q, while preserving the
existing uppercase comparison and query behavior. Apply the escaping at the
value used by the WHERE n.name condition so embedded single quotes cannot alter
the composed query.
---
Outside diff comments:
In `@src/openhound_github/resources/enterprise.py`:
- Around line 439-477: Wrap the `client.paginate(...)` iteration in
`enterprise_external_identity` with the same targeted exception handling used by
the existing `enterprise_organizations`, `enterprise_saml_provider`, or
`organization.py` SAML flows. Log the pagination failure with enterprise context
and return gracefully so transient or permission errors do not fail the entire
resource, while preserving normal page processing.
---
Nitpick comments:
In `@src/openhound_github/kinds/edges.py`:
- Around line 143-155: Remove the duplicate SAML_TRUSTS_ISSUER assignment in the
SAML edges constants, retaining a single definition with the existing value.
In `@src/openhound_github/models/__init__.py`:
- Around line 96-101: Remove the duplicate "ExternalIdentity" entry from the
__all__ list in src/openhound_github/models/__init__.py, keeping a single export
entry alongside the other model symbols.
In `@src/openhound_github/models/external_identity.py`:
- Line 202: Rename the local variable id in the external identity unpacking
assignment to a non-shadowing name, and update its uses within the surrounding
function accordingly.
- Around line 293-301: Update ExternalIdentity.service_provider_node_id to
obtain and reuse the explicit environment_type from idp_for_environment(...)’s
SAML provider row, rather than inferring it from environment_node_id. Thread the
environment_type column through the provider lookup and preserve the existing
None behavior when required environment data is unavailable, ensuring the
generated node ID matches normalized SAML nodes.
In `@src/openhound_github/models/saml_provider.py`:
- Around line 89-101: Remove the duplicate IdP detection logic from
SAMLProvider.detect_foreign_environment and reuse the shared
ExternalIdentity.detect_foreign_idp helper instead. Update as_node to preserve
and use the helper’s canonical IdP values rather than discarding the returned
foreign_idp_type, or remove the unused wrapper if no caller requires it.
In `@src/openhound_github/models/saml_service_provider.py`:
- Around line 90-93: Update the as_node property’s saml_route unpacking to avoid
binding the unused acs_url value while preserving the existing use of
sp_entity_id and subsequent scope_type logic.
In `@src/openhound_github/resources/enterprise.py`:
- Around line 478-489: Update the external identity construction loop to yield
only the SAML-provider field consumed by ExternalIdentity: retain
environment_slug and remove environment_node_id, environment_name,
saml_provider_id, saml_provider_issuer, and saml_provider_sso_url.
🪄 Autofix (Beta)
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: 25442157-24b2-4c32-833e-a43fb94ded43
📒 Files selected for processing (12)
src/openhound_github/kinds/edges.pysrc/openhound_github/kinds/nodes.pysrc/openhound_github/models/__init__.pysrc/openhound_github/models/enterprise_external_identity.pysrc/openhound_github/models/enterprise_saml_provider.pysrc/openhound_github/models/external_identity.pysrc/openhound_github/models/saml_assertion_consumer_service.pysrc/openhound_github/models/saml_issuer.pysrc/openhound_github/models/saml_provider.pysrc/openhound_github/models/saml_service_provider.pysrc/openhound_github/resources/enterprise.pysrc/openhound_github/resources/organization.py
💤 Files with no reviewable changes (2)
- src/openhound_github/models/enterprise_saml_provider.py
- src/openhound_github/models/enterprise_external_identity.py
Comment on lines
+273
to
+278
| q = ( | ||
| f"MATCH p=()<-[:GH_SyncedToEnvironment]-(:GH_SamlIdentityProvider)" | ||
| f"-[:GH_HasExternalIdentity]->(:GH_ExternalIdentity)" | ||
| f"-[:GH_MapsToUser]->(n) " | ||
| f"WHERE n.objectid = '{gh_id}' OR n.name = '{foreign_username.upper()}' RETURN p" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other query_composition/query_* string builders in this codebase escape single quotes.
rg -n "query_composition|query_mapped_users|query_environments" --type=py -C2Repository: SpecterOps/openhound-github
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file structure first, then inspect the relevant section.
ast-grep outline src/openhound_github/models/external_identity.py --view expanded || true
echo '--- lines around the reported query ---'
sed -n '240,320p' src/openhound_github/models/external_identity.py
echo '--- search for similar Cypher query construction in this file and related models ---'
rg -n "MATCH p=|RETURN p|foreign_username|gh_id|Cypher|cypher|session\.run|query_" src/openhound_github/models -C 2 || trueRepository: SpecterOps/openhound-github
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- find query_composition definition and usages ---'
rg -n "query_composition" src -C 2
echo '--- inspect the edge/property types around query_composition ---'
sed -n '1,240p' src/openhound_github/models/edge.py
echo '--- inspect the external identity model around the composed query ---'
sed -n '210,300p' src/openhound_github/models/external_identity.py
echo '--- inspect any code that executes composed queries ---'
rg -n "query_composition|composed=True|RETURN p" src/openhound_github -C 2 | sed -n '1,240p'Repository: SpecterOps/openhound-github
Length of output: 2824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- graph.py definition of edge properties ---'
sed -n '1,120p' src/openhound_github/graph.py
echo '--- locate all query_composition field reads and composed-edge handling ---'
rg -n "\.query_composition|query_composition\s*=|composed\s*=\s*True|composed" src/openhound_github -C 3
echo '--- inspect the query builder helpers used in other composed edges for escaping/parameterization patterns ---'
sed -n '1,260p' src/openhound_github/models/org_secret.py
sed -n '200,310p' src/openhound_github/models/secret_scanning_alert.pyRepository: SpecterOps/openhound-github
Length of output: 16302
Escape foreign_username before building this composed Cypher query. foreign_username.upper() is interpolated directly into WHERE n.name = '...', so an IdP-supplied value containing ' can alter the query. src/openhound_github/models/external_identity.py:273-277
🤖 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/external_identity.py` around lines 273 - 278,
Escape the IdP-supplied foreign_username before interpolating it into the Cypher
query assigned to q, while preserving the existing uppercase comparison and
query behavior. Apply the escaping at the value used by the WHERE n.name
condition so embedded single quotes cannot alter the composed query.
… mapping - move shared SAML parsing and node-id construction into saml_helpers - reuse shared helpers across SAML provider, issuer, ACS, and external identity models - stop inferring service provider environment type from node id prefixes - remove duplicated foreign IdP matching logic in external identity edges - escape composed Cypher username matching for synced identity queries
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/openhound_github/resources/enterprise.py (1)
350-372: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing error handling around PAT-backed SSO GraphQL calls.
client.post(...)(Line 364) andclient.paginate(...)(Lines 462-468) — both usingctx.sso_client— are unwrapped, unlikeenterprise()in this same file, which catches exceptions and logs gracefully. A non-2xx response (insufficient PAT scope, transient failure) will raise and crash the whole enterprise SAML/identity pipeline instead of degrading gracefully. Theclient.postcase at Line 364 was already flagged in a prior review round and remains unresolved in this version; theclient.paginatecall at Lines 462-468 has the identical hazard.🛡️ Suggested fix
- response = client.post("/graphql", json=data).json() + try: + response = client.post("/graphql", json=data).json() + except Exception as e: + logger.error( + f"Error in resource 'enterprise_saml_provider' processing enterprise '{ctx.enterprise_name}': {e}", + extra={"resource": "enterprise_saml_provider", "phase": "resource_iteration"}, + ) + return enterprise_object = (response.get("data") or {}).get("enterprise", {})- for page_data in client.paginate( - "/graphql", - method="POST", - json=data, - paginator=paginator, - data_selector="data", - ): + try: + pages = list(client.paginate( + "/graphql", + method="POST", + json=data, + paginator=paginator, + data_selector="data", + )) + except Exception as e: + logger.error( + f"Error in resource 'enterprise_external_identity' processing enterprise '{ctx.enterprise_name}': {e}", + extra={"resource": "enterprise_external_identity", "phase": "resource_iteration"}, + ) + return + for page_data in pages:Also applies to: 462-468
🤖 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/resources/enterprise.py` around lines 350 - 372, Wrap the PAT-backed SSO requests in enterprise_saml_provider and the corresponding identity-provider pagination flow around client.post and client.paginate with the same exception handling pattern used by enterprise(). Log the failure with enterprise context and return safely so missing scopes or transient GraphQL errors skip these resources without crashing the pipeline.
🧹 Nitpick comments (2)
src/openhound_github/resources/organization.py (1)
1736-1738: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the nonexistent argument from the docstring.
The docstring documents a
saml (SamlProvider)argument, but the function signature only acceptsctx.♻️ Proposed fix
Args: - saml (SamlProvider): The SAML provider to extract identities for. ctx (SourceContext): The shared context containing the REST client and organization name.🤖 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/resources/organization.py` around lines 1736 - 1738, Remove the `saml (SamlProvider)` entry from the function docstring’s Args section, leaving only documentation for the actual `ctx` parameter.src/openhound_github/models/saml_helpers.py (1)
20-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
urllib.parseto safely extract the domain.Extracting the domain via
.split("/")[2]will throw anIndexErrorif thesso_urlis malformed and lacks a protocol scheme likehttps://. Usingurllib.parse.urlparseis safer and more idiomatic.♻️ Proposed refactor
Add the import at the top of the file:
+from urllib.parse import urlparseAnd update the Okta domain extraction logic:
if issuer.startswith("http://www.okta.com/"): - domain = (sso_url or "").split("/")[2] if sso_url else None + domain = urlparse(sso_url).netloc if sso_url else None return "okta", domain🤖 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/saml_helpers.py` around lines 20 - 22, Update the Okta handling in the issuer-detection function to use urllib.parse.urlparse for extracting the SSO hostname instead of indexing sso_url.split("/"). Preserve the existing None behavior when sso_url is absent and safely handle malformed URLs without raising IndexError.
🤖 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.
Duplicate comments:
In `@src/openhound_github/resources/enterprise.py`:
- Around line 350-372: Wrap the PAT-backed SSO requests in
enterprise_saml_provider and the corresponding identity-provider pagination flow
around client.post and client.paginate with the same exception handling pattern
used by enterprise(). Log the failure with enterprise context and return safely
so missing scopes or transient GraphQL errors skip these resources without
crashing the pipeline.
---
Nitpick comments:
In `@src/openhound_github/models/saml_helpers.py`:
- Around line 20-22: Update the Okta handling in the issuer-detection function
to use urllib.parse.urlparse for extracting the SSO hostname instead of indexing
sso_url.split("/"). Preserve the existing None behavior when sso_url is absent
and safely handle malformed URLs without raising IndexError.
In `@src/openhound_github/resources/organization.py`:
- Around line 1736-1738: Remove the `saml (SamlProvider)` entry from the
function docstring’s Args section, leaving only documentation for the actual
`ctx` parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c192d69-44cd-4a99-bb72-3b887c5b11c9
📒 Files selected for processing (22)
src/openhound_github/extension.yamlsrc/openhound_github/graphql.pysrc/openhound_github/kinds/edges.pysrc/openhound_github/kinds/nodes.pysrc/openhound_github/lookup.pysrc/openhound_github/models/__init__.pysrc/openhound_github/models/enterprise_external_identity.pysrc/openhound_github/models/enterprise_saml_provider.pysrc/openhound_github/models/external_identity.pysrc/openhound_github/models/repository.pysrc/openhound_github/models/saml_assertion_consumer_service.pysrc/openhound_github/models/saml_helpers.pysrc/openhound_github/models/saml_issuer.pysrc/openhound_github/models/saml_provider.pysrc/openhound_github/models/saml_service_provider.pysrc/openhound_github/models/secret_scanning_alert.pysrc/openhound_github/models/workflow.pysrc/openhound_github/models/workflow_job.pysrc/openhound_github/models/workflow_step.pysrc/openhound_github/resources/enterprise.pysrc/openhound_github/resources/organization.pysrc/openhound_github/transforms.py
💤 Files with no reviewable changes (2)
- src/openhound_github/models/enterprise_external_identity.py
- src/openhound_github/models/enterprise_saml_provider.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/openhound_github/kinds/nodes.py
- src/openhound_github/models/saml_issuer.py
- src/openhound_github/models/init.py
- src/openhound_github/models/saml_assertion_consumer_service.py
- src/openhound_github/models/saml_service_provider.py
- src/openhound_github/kinds/edges.py
- src/openhound_github/models/external_identity.py
- use urllib.parse.urlparse to safely extract Okta SSO hostnames - remove the stale saml argument entry from external_identities docstring
- add GH_EnvironmentBranchPolicy nodes and GH_MatchesEnvironmentPolicy edges - model GH_CanDeployToEnvironment for unrestricted, protected, and custom-policy environments - add GH_CanEditEnvironment and GH_ApprovesDeploymentTo relationships - surface environment protection metadata including reviewers, wait timers, and admin bypass - switch branch and environment containment to GH_Contains for composition queries - add protected-branch fallback handling when no branch protection rules exist - add tests for branch policy matching and protected-branch fallback behavior
- add GH_HasSecret and GH_HasVariable edges from environments to scoped values - resolve GH_EnvironmentSecret references from workflow steps with literal job environments - resolve GH_EnvironmentVariable references from workflow steps with literal job environments - add repository access edges for organization variables across all, private, and selected visibility scopes - preserve repository and organization secret/variable resolution alongside environment-scoped matches
- add GH_CanCreateEnvironment edges from repo roles that can create branches - model environment creation through workflow edits that reference new environment names - require write-equivalent repository access before emitting the edge
- add schema entries for environment branch policy and environment deployment edges - update saved searches to use GH_Contains instead of legacy GH_Has* relationships - document GH_EnvironmentBranchPolicy and environment deployment relationships - document environment-scoped secret and variable resolution - refresh containment and workflow reference docs for environment support
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/openhound_github/models/environment_branch_policy.py (3)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring is stale — this asset now emits a node.
📝 Proposed docstring update
- """One record from `environment_branch_policies` → GH_Contains edge from synthetic policy ID. No node.""" + """One record from `environment_branch_policies` → one GH_EnvironmentBranchPolicy node + + GH_Contains edge from the environment + branch match/deploy edges."""🤖 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/environment_branch_policy.py` around lines 43 - 44, Update the EnvironmentBranchPolicy class docstring to remove the claim that it emits no node and accurately describe its current node-emission behavior, while preserving the existing record and GH_Contains relationship context.
129-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist
environment_protected_branchesout of the branch loop.It is a property that issues a DuckDB query on every access, and it is read up to twice per matching branch. Evaluate it once per asset.
⚡ Proposed fix
branches = self._lookup.branches_for_repository(self.repository_node_id) + protected_only = self.environment_protected_branches for branch_id, branch_name, protected in branches: if self.matches_branch(branch_name): yield Edge( kind=ek.MATCHES_ENVIRONMENT_POLICY, start=EdgePath(value=branch_id, match_by="id"), end=EdgePath(value=self.node_id, match_by="id"), properties=EdgeProperties(traversable=False), ) - if not self.environment_protected_branches or protected: + if not protected_only or protected: yield Edge( kind=ek.CAN_DEPLOY_TO_ENVIRONMENT, start=EdgePath(value=branch_id, match_by="id"), end=EdgePath(value=self.environment_node_id, match_by="id"), properties=GHEdgeProperties( traversable=True, composed=True, query_composition=( self._protected_policy_branch_can_deploy_query(branch_id) - if self.environment_protected_branches + if protected_only else self._policy_branch_can_deploy_query(branch_id) ), ), )🤖 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/environment_branch_policy.py` around lines 129 - 152, Evaluate self.environment_protected_branches once before iterating over branches in the branch-processing method, store the result in a local variable, and use that variable for both the deployment condition and query selection inside the loop. Keep the existing behavior unchanged while avoiding repeated property access per matching branch.
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
policy_idhelper and commented_b64import.as_nodeandedgesalready useself.node_id, so the property is unused and can be dropped along with the commented import.🤖 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/environment_branch_policy.py` at line 12, Remove the unused commented `_b64` import and delete the dead `policy_id` property from the environment branch policy model. Preserve the existing `as_node` and `edges` implementations using `self.node_id`.src/openhound_github/models/org_variable.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the new environment/org secret & variable reachability edges.
These four sites introduce new, security-relevant graph edges (traversable env/org→secret/variable access paths and environment-scoped
USES_SECRET/USES_VARIABLEedges), but none has direct unit test coverage, unlike the siblingEnvironment/EnvironmentBranchPolicyedge tests added intests/test_environment_model.py.
src/openhound_github/models/org_variable.py#L101-140: add tests forOrgVariable.edgescoveringvisibility="all","private", and default/unhandled cases, verifying theCONTAINSedge and the repo→variableHAS_VARIABLEedges.src/openhound_github/models/env_secret.py#L50-57,112-117: add a test assertingEnvironmentSecret.edgesyields both the non-traversableCONTAINSedge and the traversableHAS_SECRETedge.src/openhound_github/models/env_variable.py#L54-61,116-121: add a test assertingEnvironmentVariable.edgesyields both the non-traversableCONTAINSedge and the traversableHAS_VARIABLEedge.src/openhound_github/models/workflow_step.py#L230-252,287-313: add a test for_uses_secret_edges/_uses_variable_edgescovering thejob_environmentbranch (including the"${{" in job_environmentguard skipping the lookup).🤖 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/org_variable.py` at line 1, Add unit tests for the new graph edges in OrgVariable.edges, EnvironmentSecret.edges, EnvironmentVariable.edges, and WorkflowStep._uses_secret_edges/_uses_variable_edges. Cover OrgVariable visibility values "all", "private", and default/unhandled behavior, including CONTAINS and repository HAS_VARIABLE edges; assert environment secret/variable edges include both non-traversable CONTAINS and traversable HAS_SECRET/HAS_VARIABLE edges; and cover the workflow step job_environment path, including skipping lookup when the value contains "${{".
🤖 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/environment.py`:
- Around line 141-154: Remove the two CAN_DEPLOY_TO_ENVIRONMENT EdgeDefs in the
environment model that use nk.USER and nk.TEAM as start nodes. Keep the
repository, branch, and {repo}_admin deploy edges and the existing
APPROVES_DEPLOYMENT_TO definitions unchanged.
---
Nitpick comments:
In `@src/openhound_github/models/environment_branch_policy.py`:
- Around line 43-44: Update the EnvironmentBranchPolicy class docstring to
remove the claim that it emits no node and accurately describe its current
node-emission behavior, while preserving the existing record and GH_Contains
relationship context.
- Around line 129-152: Evaluate self.environment_protected_branches once before
iterating over branches in the branch-processing method, store the result in a
local variable, and use that variable for both the deployment condition and
query selection inside the loop. Keep the existing behavior unchanged while
avoiding repeated property access per matching branch.
- Line 12: Remove the unused commented `_b64` import and delete the dead
`policy_id` property from the environment branch policy model. Preserve the
existing `as_node` and `edges` implementations using `self.node_id`.
In `@src/openhound_github/models/org_variable.py`:
- Line 1: Add unit tests for the new graph edges in OrgVariable.edges,
EnvironmentSecret.edges, EnvironmentVariable.edges, and
WorkflowStep._uses_secret_edges/_uses_variable_edges. Cover OrgVariable
visibility values "all", "private", and default/unhandled behavior, including
CONTAINS and repository HAS_VARIABLE edges; assert environment secret/variable
edges include both non-traversable CONTAINS and traversable
HAS_SECRET/HAS_VARIABLE edges; and cover the workflow step job_environment path,
including skipping lookup when the value contains "${{".
🪄 Autofix (Beta)
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: 4ecd4ee3-189c-4553-94ef-883e245aafbd
📒 Files selected for processing (40)
descriptions/edges/GH_ApprovesDeploymentTo.mddescriptions/edges/GH_CanCreateEnvironment.mddescriptions/edges/GH_CanDeployToEnvironment.mddescriptions/edges/GH_CanEditEnvironment.mddescriptions/edges/GH_Contains.mddescriptions/edges/GH_DeploysTo.mddescriptions/edges/GH_HasBranch.mddescriptions/edges/GH_HasEnvironment.mddescriptions/edges/GH_HasSecret.mddescriptions/edges/GH_HasVariable.mddescriptions/edges/GH_MatchesEnvironmentPolicy.mddescriptions/edges/GH_UsesSecret.mddescriptions/edges/GH_UsesVariable.mddescriptions/nodes/GH_Environment.mddescriptions/nodes/GH_EnvironmentBranchPolicy.mddescriptions/nodes/GH_EnvironmentSecret.mddescriptions/nodes/GH_EnvironmentVariable.mdextension/saved_searches/environments-admin-bypass.jsonextension/saved_searches/repository-workflows.jsonextension/saved_searches/unprotected-branches.jsonextension/saved_searches/unprotected-default-branch-with-workflow.jsonextension/saved_searches/unprotected-default-branches.jsonextension/schema.jsonsrc/openhound_github/kinds/edges.pysrc/openhound_github/kinds/nodes.pysrc/openhound_github/lookup.pysrc/openhound_github/models/branch.pysrc/openhound_github/models/env_secret.pysrc/openhound_github/models/env_variable.pysrc/openhound_github/models/environment.pysrc/openhound_github/models/environment_branch_policy.pysrc/openhound_github/models/org_variable.pysrc/openhound_github/models/repository.pysrc/openhound_github/models/repository_role.pysrc/openhound_github/models/saml_helpers.pysrc/openhound_github/models/workflow.pysrc/openhound_github/models/workflow_step.pysrc/openhound_github/resources/organization.pytests/test_environment_model.pytests/test_workflow_model.py
💤 Files with no reviewable changes (1)
- src/openhound_github/resources/organization.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhound_github/models/saml_helpers.py
- src/openhound_github/models/workflow.py
Comment on lines
+141
to
+154
| EdgeDef( | ||
| start=nk.USER, | ||
| end=nk.ENVIRONMENT, | ||
| kind=ek.CAN_DEPLOY_TO_ENVIRONMENT, | ||
| description="Reviewer user can approve deployment to environment", | ||
| traversable=True, | ||
| ), | ||
| EdgeDef( | ||
| start=nk.TEAM, | ||
| end=nk.ENVIRONMENT, | ||
| kind=ek.CAN_DEPLOY_TO_ENVIRONMENT, | ||
| description="Reviewer team can approve deployment to environment", | ||
| traversable=True, | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
These two CAN_DEPLOY_TO_ENVIRONMENT EdgeDefs are never emitted and contradict the documented model.
edges() only yields CAN_DEPLOY_TO_ENVIRONMENT from the repository, branches, and the {repo}_admin role — never from GH_User/GH_Team. descriptions/edges/GH_CanDeployToEnvironment.md also states reviewers are represented via GH_ApprovesDeploymentTo metadata rather than extra deploy edges. Reviewer coverage is already declared by the two APPROVES_DEPLOYMENT_TO defs below.
🧹 Proposed removal of the unemitted reviewer deploy EdgeDefs
- EdgeDef(
- start=nk.USER,
- end=nk.ENVIRONMENT,
- kind=ek.CAN_DEPLOY_TO_ENVIRONMENT,
- description="Reviewer user can approve deployment to environment",
- traversable=True,
- ),
- EdgeDef(
- start=nk.TEAM,
- end=nk.ENVIRONMENT,
- kind=ek.CAN_DEPLOY_TO_ENVIRONMENT,
- description="Reviewer team can approve deployment to environment",
- traversable=True,
- ),
EdgeDef(
start=nk.USER,
end=nk.ENVIRONMENT,
kind=ek.APPROVES_DEPLOYMENT_TO,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| EdgeDef( | |
| start=nk.USER, | |
| end=nk.ENVIRONMENT, | |
| kind=ek.CAN_DEPLOY_TO_ENVIRONMENT, | |
| description="Reviewer user can approve deployment to environment", | |
| traversable=True, | |
| ), | |
| EdgeDef( | |
| start=nk.TEAM, | |
| end=nk.ENVIRONMENT, | |
| kind=ek.CAN_DEPLOY_TO_ENVIRONMENT, | |
| description="Reviewer team can approve deployment to environment", | |
| traversable=True, | |
| ), |
🤖 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/environment.py` around lines 141 - 154, Remove
the two CAN_DEPLOY_TO_ENVIRONMENT EdgeDefs in the environment model that use
nk.USER and nk.TEAM as start nodes. Keep the repository, branch, and
{repo}_admin deploy edges and the existing APPROVES_DEPLOYMENT_TO definitions
unchanged.
…ity matching - add deployment-aware normalized SAML IDs and routes for GitHub.com and GHES - emit normalized SAML service provider, issuer, and ACS nodes with contract metadata - anchor SAML_Implements edges from GH_SamlIdentityProvider nodes - add schema contract metadata to normalized SAML relationship edges - enrich SAML_HasAccount edges with direct binding evidence and match values - preserve legacy GH_SamlIdentityProvider and GH_ExternalIdentity relationships - add tenant-scoped foreign identity matching for Entra, Okta, and PingOne - require Entra tenant/object claims to agree with the SAML issuer before emitting correlations - expose GitHub deployment metadata on legacy SAML provider nodes - harden SAML provider replay for DLT snake_case rows and GraphQL aliases - add regression coverage for deployment scoping, normalized SAML output, and tenant-safe identity resolution
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/openhound_github/models/external_identity.py (1)
242-269: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winComposed
SYNCED_TO_GH_USERCypher query is still injectable and drops tenant scoping.Two problems in the same block:
matcher.value(an uppercased SAML/SCIM username, or an Entraobjectidclaim — IdP-supplied data) is interpolated unescaped into a single-quoted Cypher literal. A value containing'breaks or alters the query. This mirrors a previously flagged (and apparently unresolved) review comment on this same code path.composition_matchersfilters to only{"name", "objectid"}, dropping the tenant-scoping key (tenant_domain/environmentid/tenantid) thatmatchersotherwise carries. Since theMATCHpattern isn't anchored to a specific IdP node, two different tenants with a duplicate normalized username (exactly the scenario intest_external_identity_scopes_duplicate_okta_username_by_tenant) can produce an ambiguous/incorrect composed path — even though theConditionalEdgePathendpoints are correctly tenant-scoped. This gap isn't caught by existing tests, which only assert onproperty_matchers, never onquery_compositioncontent.🐛 Proposed fix
if matchers and self.user and self.user.id: - composition_matchers = [ - matcher for matcher in matchers if matcher.key in {"name", "objectid"} - ] - composition_predicates = " OR ".join( - f"n.{matcher.key} = '{matcher.value}'" - for matcher in composition_matchers - ) + def _escape(value: str) -> str: + return value.replace("\\", "\\\\").replace("'", "\\'") + + composition_predicates = " AND ".join( + f"n.{matcher.key} = '{_escape(matcher.value)}'" + for matcher in matchers + ) q = ( f"MATCH p=()<-[:GH_SyncedToEnvironment]-(:GH_SamlIdentityProvider)" f"-[:GH_HasExternalIdentity]->(:GH_ExternalIdentity)" f"-[:GH_MapsToUser]->(n) " f"WHERE {composition_predicates} RETURN p" )🤖 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/external_identity.py` around lines 242 - 269, Fix the composed query construction in the SYNCED_TO_GH_USER block: parameterize or safely escape matcher.value before using it in Cypher literals, and retain the tenant-scoping matcher (tenant_domain, environmentid, or tenantid) when building composition_matchers. Ensure query_composition constrains both the identity value and its tenant while preserving the existing composed Edge behavior.
🧹 Nitpick comments (1)
src/openhound_github/models/saml_service_provider.py (1)
67-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared SAML-asset environment/deployment scoping into a common base/mixin.
github_deployment_id/github_web_origindefaults and theservice_provider_node_idproperty (delegating togithub_saml_service_provider_id) are copy/pasted verbatim across three asset classes. This is the same duplication pattern that previously caused the Oktamatch_keydivergence bug between two near-identical edge blocks — worth deduplicating before a fourth normalized SAML asset is added.
src/openhound_github/models/saml_service_provider.py#L67-L83: extractgithub_deployment_id/github_web_originfields andservice_provider_node_idinto a shared base class/mixin thatSamlServiceProviderinherits from.src/openhound_github/models/saml_issuer.py#L60-L85: inherit the same shared base/mixin instead of redeclaring the fields andservice_provider_node_idproperty.src/openhound_github/models/saml_assertion_consumer_service.py#L63-L77: inherit the same shared base/mixin instead of redeclaring the fields andservice_provider_node_idproperty.🤖 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/saml_service_provider.py` around lines 67 - 83, Extract the shared github_deployment_id, github_web_origin, and service_provider_node_id implementation into a common SAML asset base class or mixin, preserving delegation to github_saml_service_provider_id. Update src/openhound_github/models/saml_service_provider.py:67-83 to inherit it and remove the duplicate members; update src/openhound_github/models/saml_issuer.py:60-85 and src/openhound_github/models/saml_assertion_consumer_service.py:63-77 likewise, leaving each class’s asset-specific fields intact.
🤖 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.
Duplicate comments:
In `@src/openhound_github/models/external_identity.py`:
- Around line 242-269: Fix the composed query construction in the
SYNCED_TO_GH_USER block: parameterize or safely escape matcher.value before
using it in Cypher literals, and retain the tenant-scoping matcher
(tenant_domain, environmentid, or tenantid) when building composition_matchers.
Ensure query_composition constrains both the identity value and its tenant while
preserving the existing composed Edge behavior.
---
Nitpick comments:
In `@src/openhound_github/models/saml_service_provider.py`:
- Around line 67-83: Extract the shared github_deployment_id, github_web_origin,
and service_provider_node_id implementation into a common SAML asset base class
or mixin, preserving delegation to github_saml_service_provider_id. Update
src/openhound_github/models/saml_service_provider.py:67-83 to inherit it and
remove the duplicate members; update
src/openhound_github/models/saml_issuer.py:60-85 and
src/openhound_github/models/saml_assertion_consumer_service.py:63-77 likewise,
leaving each class’s asset-specific fields intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60abc960-ef2c-4d86-832a-8da6202e5b8d
📒 Files selected for processing (14)
src/openhound_github/graphql.pysrc/openhound_github/kinds/nodes.pysrc/openhound_github/models/external_identity.pysrc/openhound_github/models/saml_assertion_consumer_service.pysrc/openhound_github/models/saml_helpers.pysrc/openhound_github/models/saml_issuer.pysrc/openhound_github/models/saml_provider.pysrc/openhound_github/models/saml_service_provider.pysrc/openhound_github/resources/enterprise.pysrc/openhound_github/resources/organization.pysrc/openhound_github/source.pytests/test_external_identity_tenant_matching.pytests/test_saml_helpers.pytests/test_saml_models.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/openhound_github/kinds/nodes.py
- src/openhound_github/models/saml_provider.py
- src/openhound_github/graphql.py
- src/openhound_github/resources/organization.py
- src/openhound_github/resources/enterprise.py
- emit normalized SCIM organization, user, and group nodes - add SCIM contains, membership, and provisioning relationships - collect enterprise SCIM users and groups from GitHub SCIM endpoints - add optional legacy Okta-to-SCIM correlation support - connect SCIM groups to enterprise teams when group IDs are present - add SCIM lookup helpers, source configuration, and regression coverage - document enterprise SCIM collection and hybrid correlation behavior
map enterprise role permissions into granular GH_* capability edges keep only confirmed enterprise privilege paths traversable add synthetic members enterprise role for consistent role modeling assign every enterprise user to the built-in members role avoid assignment API lookups for synthetic owners and members roles register new enterprise capability relationships in the schema add tests covering permission mapping, traversability, and synthetic role behavior
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 by CodeRabbit
GH_Containsfor contained resources.