Skip to content

fix(github): align codebase to use a consistent github uri#159

Merged
behinddwalls merged 2 commits into
mainfrom
wua/fix-github-change-parsing
May 28, 2026
Merged

fix(github): align codebase to use a consistent github uri#159
behinddwalls merged 2 commits into
mainfrom
wua/fix-github-change-parsing

Conversation

@albertywu
Copy link
Copy Markdown
Contributor

@albertywu albertywu commented May 27, 2026

Summary

Aligns proto documentation and internal implementation to use a consistent canonical github uri shape:

github://uber/repo/pull/123/<sha>

Before

ParseChangeID silently misparsed input URLs because it wasn't handling /pull/ in the URI:

For example:
github://uber/repo/pull/123/<sha> would parse to Org="uber/repo" Repo="pull" instead of erroring.


After

  • github://uber/repo/pull/123/<sha> parses correctly to Org="uber" Repo="repo"
  • parser ensures that <sha> is canonical 40-char lowercase hex and returns error if not. Parsing happens at edge of application such that downstream internals can be assured that the <sha> is always in standard format.

Test Plan

make test

ParseChangeID silently misparsed the canonical /pull/-form URIs used by
the proto example and the bulk of orchestrator/gateway/integration
fixtures (e.g. github://uber/repo/pull/123/<sha>), producing
Org="uber/repo" Repo="pull" instead of erroring. Align the parser with
the documented format and reject the no-/pull/ form explicitly.

Also require the head commit SHA to be 40 lowercase hex characters
(GitHub's canonical headRefOid form). The downstream staleness check
in mergechecker/changeprovider already compares with strict string
equality against the full SHA, so abbreviated or non-hex SHAs would
fail later with a confusing "head SHA changed" message. Fail fast at
the parser instead.

Fixtures throughout entity/, gateway/, orchestrator/, extension/,
test/e2e, and test/integration updated to use the canonical form with
realistic 40-char hex SHAs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 27, 2026 22:16
Comment thread entity/github/change_id.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Tightens entity/github.ParseChangeID to require the canonical /pull/ literal segment and a full 40-char lowercase hex head commit SHA, matching GitHub's headRefOid. Previously, the missing-/pull/ form silently misparsed (e.g. Repo="pull") and abbreviated/non-hex SHAs would only fail later in the staleness check with a confusing message. All fixtures across entity, gateway, orchestrator, extension, and integration/e2e suites are updated to the canonical form with realistic 40-char SHAs; the proto comment and generated .pb.go are also updated.

Changes:

  • Parser now requires a literal pull segment third-to-last and a strict 40-char lowercase hex SHA; String() re-emits the canonical /pull/ form.
  • Parser tests cover all new rejection cases (missing/wrong literal, abbreviated/uppercase/non-hex/too-long SHA) and round-tripping for nested-org paths.
  • Proto, entity, and downstream doc comments updated to document <scheme>, /pull/ template, full hex SHA requirement, and supported schemes (github, ghe, ghes).

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
entity/github/change_id.go Adds pullSegment/shaLength constants, enforces literal pull segment and isFullHexSHA, updates String() and format docs.
entity/github/change_id_test.go Adds shared SHA constants and negative cases for missing/wrong literal segment and SHA validation; updates round-trip cases.
entity/request.go, entity/change_record.go, extension/changeprovider/change_provider.go Doc-only updates to URI template and example.
entity/request_test.go, entity/land_request_test.go Fixture URIs updated to canonical /pull/ form with 40-char hex SHAs.
gateway/proto/gateway.proto, gateway/protopb/gateway.pb.go Proto comment expanded to document template, schemes, and SHA constraints; generated Go mirrors comment.
gateway/controller/land_test.go Test URIs updated to canonical form.
orchestrator/controller/{start,validate,batch,score,merge}/*_test.go Fixture URIs updated to canonical form with 40-char SHAs.
extension/mergechecker/github/checker_test.go Adds shared SHA constants; updates URIs and HeadRefOid values; updates expected error message.
extension/mergechecker/multi_test.go Updates routing test URIs; unknown-scheme case keeps non-canonical URI since routing fails before parsing.
extension/changeprovider/github/provider_test.go Adds shared SHA constants; updates URIs/HeadRefOid consistently across cases.
extension/pusher/git/git_pusher_test.go uri() helper now emits /pull/1/<sha>.
test/integration/gateway/suite_test.go, test/e2e/suite_test.go Integration/E2E land-request fixtures updated to canonical form.
test/integration/extension/storage/suite.go, test/integration/extension/changestore/mysql/changestore_test.go Storage/change-store fixture URIs updated to canonical form with 40-char SHAs.
Files not reviewed (1)
  • gateway/protopb/gateway.pb.go: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@albertywu albertywu changed the title fix(github): enforce /pull/ segment and full SHA in change-ID parser fix(github): align codebase to use a consistent github uri May 27, 2026
@albertywu albertywu marked this pull request as ready for review May 27, 2026 22:36
@albertywu albertywu requested review from a team, behinddwalls and sbalabanov as code owners May 27, 2026 22:36
@behinddwalls behinddwalls enabled auto-merge May 27, 2026 22:56
@behinddwalls behinddwalls added this pull request to the merge queue May 28, 2026
Merged via the queue into main with commit 0cf0ff8 May 28, 2026
13 checks passed
@github-actions github-actions Bot deleted the wua/fix-github-change-parsing branch May 28, 2026 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants