fix(github): align codebase to use a consistent github uri#159
Conversation
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>
There was a problem hiding this comment.
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
pullsegment 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.
Summary
Aligns proto documentation and internal implementation to use a consistent canonical github uri shape:
Before
ParseChangeIDsilently misparsed input URLs because it wasn't handling/pull/in the URI:For example:
github://uber/repo/pull/123/<sha>would parse toOrg="uber/repo" Repo="pull"instead of erroring.After
github://uber/repo/pull/123/<sha>parses correctly toOrg="uber" Repo="repo"<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