fix(xtest): Fixes unbuildable dist paths on merge_group events - #532
Conversation
When a go ref resolves from a commit SHA (as the platform merge_group caller passes), the SHA is often pointed at by exactly one ref — the gh-readonly-queue branch. The single-match path only stripped refs/tags/, so it returned tag=refs/heads/gh-readonly-queue/... (slashes become a bad nested dist path) and never set head:true, so setup-cli-tool skipped the source build entirely (only main got built). Extract _classify_sha_match()/_ref_specificity() and route both single- and multi-match SHA lookups through them so a merge-queue commit resolves to a flat tag (mq-main-N) flagged as a head regardless of how many refs point at it.
|
Warning Review limit reached
More reviews will be available in 47 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo new internal helpers, ChangesSHA Ref Classification and Disambiguation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dmihalcik-virtru
changed the title
dmihalcik-virtru
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SHA matching logic in resolve.py by extracting helper functions _ref_specificity and _classify_sha_match to handle both single- and multi-match SHA lookups consistently. It also adds unit tests to verify merge-queue and branch resolution. The review feedback highlights an unreachable fallback block in the merge-queue regex handling and suggests using removeprefix instead of split for safer and more idiomatic string manipulation when stripping prefixes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Per Gemini review on #532: MERGE_QUEUE_REGEX requires both named groups, so the to_branch/pr_number truthiness check and its fallback were dead code. Also switch ref-prefix stripping from split(...)[-1] to removeprefix() for clarity and safety.
X-Test Failure Report |
There was a problem hiding this comment.
Pull request overview
Fixes SDK version resolution for merge_group (merge-queue) events so SHA-based refs resolve to filesystem-safe dist tags and are correctly flagged as head, ensuring the Go CLI is built from source instead of looking for an unbuilt nested dist/refs/heads/... path.
Changes:
- Refactors SHA match handling in
resolve.pyinto_classify_sha_match()and_ref_specificity()and routes SHA lookups through the shared logic for both single- and multi-match cases. - Prioritizes refs when multiple point at the same SHA (PR > merge-queue > other) and consistently marks branch-like refs as
head. - Adds regression tests for single-match merge-queue and single-match branch SHAs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py | Normalizes SHA→ref resolution and prioritizes PR/merge-queue refs; ensures merge-queue SHA refs produce safe dist tags and head=true. |
| otdf-sdk-mgr/tests/test_resolve.py | Adds tests covering single-ref merge-queue and single-branch SHA resolution behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The refs/tags branch of _classify_sha_match could return a namespaced tag with a remaining slash, contradicting the docstring's filesystem-safe promise and risking a nested dist/<tag>/ path. Flatten it (no-op for plain semver tags).
X-Test Failure Report |
|
## Problem Follow-up to #532. xtest still fails during go CLI setup, now on **push-to-main** events from `opentdf/platform`, as in [this example failure](https://github.com/opentdf/platform/actions/runs/28032142207/job/82975485936): ``` FileNotFoundError: SDK executable not found at path: sdk/go/dist/HEAD/cli.sh ``` The matrix value was `go@HEAD` and only `main` got built. ## Root cause The platform caller passes the go ref as the main-tip commit **SHA** (`otdfctl-ref: <sha> main`). On a push to main, `git ls-remote` lists that SHA under **two** refs: ``` <sha> HEAD <sha> refs/heads/main ``` `_ref_specificity` scored every non-PR/non-merge-queue ref equally (`2`), so `min()` kept the **first** entry — the symbolic `HEAD`. That fell through `_classify_sha_match` to the generic tag path, producing `tag = "HEAD"` with no `head` flag. So `setup-cli-tool` skipped the source build (only `main`, separately resolved as a head, was built) and the test looked for the non-existent `dist/HEAD/cli.sh`. ## Fix Rank real refs above the bare `HEAD`: **PR > merge-queue > branch > tag > other**. Now `refs/heads/main` wins over `HEAD`, giving `tag = "main"`, `head = true` (built from source). Branch is deliberately ranked **above** tag: the SHA path always resolves a commit-under-test, and only the branch case sets `head=true`. So a commit that is simultaneously a branch tip and a release tag (e.g. right after release-please) still gets a source build rather than resolving to a tag that wouldn't be built. ## Testing - Added `test_head_and_branch_prefers_branch` and `test_branch_preferred_over_tag`. - `uv run pytest` — 137 pass. - `uv run ruff check .`, `uv run ruff format .`, `uv run pyright` — clean.
Problem
xtest fails for
merge_groupevents fromopentdf/platformduring go CLI setup:The
Prepare go clistep only builtmain, yet the matrix value wasgo@refs/heads/gh-readonly-queue/....Root cause
This is a version-resolution bug in
otdf_sdk_mgr/resolve.py, not a workflow-config bug.The platform caller (
opentdf/platform→checks.yaml,platform-xtest) passes the go ref as a commit SHA (otdfctl-ref: <merge-sha> main). On a merge-queue run that SHA is pointed at by exactly one ref — the temporaryrefs/heads/gh-readonly-queue/main/pr-<n>-<sha>branch.resolve.pyhad all the merge-queue / PR / branch normalization (→mq-main-N,pull-N, flattened slashes,head: true) only in thelen(matching_tags) > 1branch. The single-match path stripped justrefs/tags//infix, so it:tag = "refs/heads/gh-readonly-queue/main/pr-…"— slashes become the bad nesteddist/refs/heads/…path the test looked for, andhead: true, sosetup-cli-toolskipped the source checkout/build entirely (onlymain, which is flagged head, got built).Fix
_classify_sha_match()— normalizes any matched(sha, ref)into a filesystem-safetag(slashes →--) and flags PR / merge-queue / branch refs ashead._ref_specificity()— when a SHA is pointed at by several refs, prefer PR > merge-queue > branch/tag.Now the example ref resolves to
tag = mq-main-3630,head = true,pr = 3630— a clean dist dir that gets source-built, with matrix valuego@mq-main-3630.Testing
test_single_match_merge_queueandtest_single_match_branch_flagged_headregression tests.uv run pytest— all 135 pass.uv run ruff check .,uv run ruff format .,uv run pyright— clean.Summary by CodeRabbit