Skip to content

Deduplicate commit-ref SHA resolution in action_resolver - #51472

Merged
pelikhan merged 5 commits into
mainfrom
copilot/duplicate-code-resolve-sha
Aug 9, 2026
Merged

Deduplicate commit-ref SHA resolution in action_resolver#51472
pelikhan merged 5 commits into
mainfrom
copilot/duplicate-code-resolve-sha

Conversation

Copilot AI commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

pkg/workflow/action_resolver.go had two independent implementations of "resolve a ref to a commit SHA via gh api /repos/.../commits/{ref} --jq .sha" — one in resolveRefViaCommitsEndpoint, one in ResolveGhAwRef. They had already drifted (Output vs CombinedOutput, GH_HOST forced in only one path), making future fixes easy to apply inconsistently.

Changes

  • Shared helper: new resolveCommitRefSHA(ctx, repoSlug, ref) owns path construction, the 30s timeout, ExecGHContext invocation, ForceGHHostEnv, output trimming, and full-SHA validation.
  • Callers are thin wrappers: both call sites keep their existing logging and error wording; ResolveGhAwRef retains its full-SHA short-circuit.
  • Typed error for diagnostics: notFullCommitSHAError carries the raw response so each caller can still emit its own "unexpected response resolving …" message without the helper needing a second return value.
  • Behavior deltas worth reviewing:
    • ResolveGhAwRef now uses Output() + enrichGHError instead of CombinedOutput(). Stderr still reaches the error message, but can no longer contaminate the parsed SHA.
    • ResolveGhAwRef now forces GH_HOST=github.com, so a GHE host in the environment can't hijack a lookup that always targets github/gh-aw.
  • Wasm: added a pass-through enrichGHError stub in github_cli_wasm.go (it previously lived only in the non-wasm file).
  • Tests: action_resolver_commit_ref_test.go covers the full-SHA short-circuit and asserts command failures are not misreported as notFullCommitSHAError.
sha, err := resolveCommitRefSHA(ctx, baseRepo, version)
if err != nil {
    var badSHA *notFullCommitSHAError
    if errors.As(err, &badSHA) {
        return "", fmt.Errorf("unexpected response resolving %s@%s: got %q (expected 40-char hex SHA)", repo, version, badSHA.response)
    }
    return "", fmt.Errorf("failed to resolve %s@%s: %w", repo, version, err)
}

run: https://github.com/github/gh-aw/actions/runs/31293411688> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 3.76 AIC · ⌖ 6.55 AIC · ⊞ 8.5K ·

Comment /souschef to run again

Copilot AI and others added 2 commits August 8, 2026 22:11
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix duplicate code in action resolver for SHA resolution Deduplicate commit-ref SHA resolution in action_resolver Aug 8, 2026
Copilot AI requested a review from pelikhan August 8, 2026 22:15
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR Triage

  • Category: refactor
  • Risk: medium (dedupes commit-ref SHA resolution; behavior deltas noted for ResolveGhAwRefOutput() vs CombinedOutput(), forced GH_HOST)
  • Score: 47/100 (impact 22, urgency 10, quality 15)
  • CI: unknown — no check-run data returned yet; PR still draft
  • Recommendation: defer — draft status; behavior deltas called out by the author deserve careful review once ready; revisit after CI completes.

Generated by 🔧 PR Triage Agent · auto · 51 AIC · ⌖ 2.57 AIC · ⊞ 8K ·

@pelikhan
pelikhan marked this pull request as ready for review August 9, 2026 03:08
Copilot AI balanced review requested due to automatic review settings August 9, 2026 03:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Deduplicates commit-ref SHA resolution while preserving caller-specific diagnostics.

Changes:

  • Adds a shared resolver with timeout, host forcing, validation, and enriched errors.
  • Updates both callers to use the shared helper.
  • Adds Wasm compatibility and initial tests.
Show a summary per file
File Description
pkg/workflow/action_resolver.go Centralizes commit-ref resolution.
pkg/workflow/github_cli_wasm.go Adds the Wasm error-enrichment stub.
pkg/workflow/action_resolver_commit_ref_test.go Tests short-circuiting and cancellation.

Review details

Tip

Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +25 to +29
func TestResolveCommitRefSHACancelledContext(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel()

sha, err := resolveCommitRefSHA(ctx, "github/gh-aw", "main")
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Please refresh this branch if needed, address any remaining review feedback, and run the pr-finisher skill.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 3.76 AIC · ⌖ 6.55 AIC · ⊞ 8.5K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Please refresh this branch if needed, address any remaining review feedback, and run the pr-finisher skill.

Generated by [👨‍🍳 PR Sous Chef](https://github.com/github/gh-aw/actions/...

Added fake-gh PATH tests in commit covering: stdout-only SHA parsing (stderr noise doesn't contaminate the returned SHA), nonzero exit surfaced as plain error and not notFullCommitSHAError, malformed stdout classified as notFullCommitSHAError with the raw response captured, and GH_HOST=github.com forced regardless of ambient GH_HOST.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

PR Triage

  • Category: refactor
  • Risk: medium
  • Priority: medium
  • Score: 43/100 (impact 25 + urgency 8 + quality 10)
  • Recommended action: batch_review
    Batch: lockfile-regen-cluster\n
    Similar large refactor pattern to Unify skill reference parsing and classification #51473, blocked, no CI - batch with related large-diff refactors.

Generated by 🔧 PR Triage Agent · auto · 58.3 AIC · ⌖ 2.48 AIC · ⊞ 8K ·

@pelikhan
pelikhan merged commit 98fe9df into main Aug 9, 2026
26 checks passed
@pelikhan
pelikhan deleted the copilot/duplicate-code-resolve-sha branch August 9, 2026 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[duplicate-code] Duplicate Code: Commit SHA resolution duplicated in action_resolver

4 participants