fix(source_status): use REST API for SHA lookup to support SAML-protected private repos#173
Open
hsperker wants to merge 1 commit into
Open
fix(source_status): use REST API for SHA lookup to support SAML-protected private repos#173hsperker wants to merge 1 commit into
hsperker wants to merge 1 commit into
Conversation
…cted private repos
The web atom feed at github.com/.../commits/{ref}.atom does not accept
Bearer auth. For SAML-protected private repos, GitHub silently returns
HTTP 200 with an empty body — the SHA regex finds no match and raises
ValueError, which bypasses the HTTPStatusError handler in
_check_all_cached_modules and surfaces as 'Unexpected error checking
{module}: ValueError: Could not extract commit SHA from Atom feed'.
PR microsoft#100 added Bearer auth on the assumption GitHub would return
401/403/404 for unauthorized atom requests. It does not — the empty-200
response path was missed.
Switch _get_github_commit_sha to the REST API
(api.github.com/repos/{owner}/{repo}/commits/{ref}), which honors PAT
auth and returns proper status codes. _get_commit_details in the same
file already uses this endpoint, so no new auth or dependency surface.
Rate limits are equivalent (5000/hr authenticated).
Author
|
@microsoft-github-policy-service agree |
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
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.
Fixes
amplifier updatefailing withUnexpected error checking {module}: ValueError: Could not extract commit SHA from Atom feedfor any module hosted in a SAML-enforced private GitHub org.Repro
git+https://github.com/hsperker/amplifier-module-hooks-langfuse@main)gh auth loginwith a token that hasreposcope but is not SSO-authorized for that org (the common case — SSO auth has to be granted explicitly per token)amplifier updateRoot cause
_get_github_commit_shacalls the web atom feed atgithub.com/{owner}/{repo}/commits/{ref}.atom. This endpoint authenticates via session cookie, not Bearer token — the PAT inAuthorization: Bearer ...is silently ignored. For a private repo accessed without a valid session, GitHub returns:(no redirect, no 401, no 404).
response.raise_for_status()passes, the SHA regex finds no match, andValueError("Could not extract commit SHA from Atom feed: ...")is raised. That ValueError falls through the carefully-craftedHTTPStatusErrorhandler in_check_all_cached_modules(added in #100) and ends up in the genericexcept Exceptionbranch, surfacing asUnexpected error checking ...instead of the friendly "private repo or rate limited" message #100 was meant to provide.PR #100 added auth headers on the assumption GitHub would return 401/403/404 for unauthorized atom requests. It does not — the empty-200 path was missed because the test suite mocked atom XML responses and never exercised the SAML/private-repo case.
Fix
Switch
_get_github_commit_shato the REST API:Properties:
Authorization: Bearer ...(it is the canonical PAT auth surface)HTTPStatusErrorhandler from fix: use auth for Atom feed requests and improve private repo error message #100 fires correctly_get_commit_details), so no new auth or dependency surfaceTests
test_source_status_auth.pytests updated from atom XML mocks to JSON mocks (the auth-header behavior under test is unchanged)TestUsesRestApiNotAtomFeed:test_calls_rest_api_endpoint: asserts the URL starts withhttps://api.github.com/and contains no.atom— catches accidental reversiontest_403_surfaces_as_http_status_error: asserts that a 403 raisesHTTPStatusErrorrather than the previous opaqueValueError, preserving the contract_check_all_cached_modulesrelies onAll 8 tests in
test_source_status_auth.pypass locally.Evidence
vs. the API path with the same token:
(after the token has been SSO-authorized for the org, which the API correctly enforces with 403; without SSO authorization it returns 403 with a clear message rather than a silent empty 200.)
Note on local test run
tests/test_pre_turn_repair.pyfails to collect locally withImportError: cannot import name 'diagnose_transcript' from 'amplifier_foundation.session'. This is pre-existing onmain(unrelated to this PR — same failure with my changes reverted) and appears to be a version skew between this repo and the installedamplifier-foundation. All other 508 tests collect; the 8 intest_source_status_auth.pypass.