Conversation
…Command logging - Remove the duplicated runGHWithSpinner function; RunGH and RunGHCombined now delegate to RunGHContext/RunGHCombinedContext with context.Background(), which is functionally equivalent since context.Background() never cancels - Consolidate the token logging in setupGHCommand: hoist the log.Printf calls outside the ctx branch since the token-check logic is identical in both branches; the 'with context' wording was not useful for debugging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hey The PR is well-structured, includes updated tests, and comes with a thorough description. It looks ready for maintainer review. 🟢
|
There was a problem hiding this comment.
Pull request overview
Refactors pkg/workflow GitHub CLI helpers to reduce duplication by delegating non-context RunGH* functions to their context-aware variants and simplifying related logging.
Changes:
- Removed duplicated
runGHWithSpinnerimplementation and routedRunGH/RunGHCombinedthroughRunGHContext/RunGHCombinedContext. - Simplified
setupGHCommandlogging by hoisting token-check logging outside the context/no-context branch. - Updated tests to call the public
RunGH/RunGHCombinedAPIs instead of the removed helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/github_cli.go | Deduplicates spinner execution by delegating wrappers to context variants; consolidates token-related logging. |
| pkg/workflow/github_cli_test.go | Adjusts tests to target public RunGH APIs after helper removal. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/github_cli_test.go:352
- In
TestRunGHFunctions, the deferred restore usesos.Getenv+os.Setenv, which will setGH_TOKEN/GITHUB_TOKENto an empty string even if they were originally unset. That can leak global env state into other tests. Useos.LookupEnvand restore by eitherSetenvorUnsetenvdepending on whether the var was originally present (similar to other tests in this file).
// Save original environment
originalGHToken := os.Getenv("GH_TOKEN")
originalGitHubToken := os.Getenv("GITHUB_TOKEN")
defer func() {
os.Setenv("GH_TOKEN", originalGHToken)
os.Setenv("GITHUB_TOKEN", originalGitHubToken)
}()
- Files reviewed: 2/2 changed files
- Comments generated: 2
| // Verify both public functions can be called; they delegate to RunGHContext/RunGHCombinedContext. | ||
| // We expect an error since gh requires auth, but no panic. | ||
| _, err := RunGH("Test spinner...", "auth", "status") | ||
| _ = err | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Just verify the function can be called | ||
| // We expect it to fail since gh command requires auth | ||
| _, err := runGHWithSpinner("Test spinner...", tt.combined, "auth", "status") | ||
| // We don't care about the error - we just want to verify the function exists | ||
| // and doesn't panic when called | ||
| _ = err | ||
| }) | ||
| } | ||
| _, err = RunGHCombined("Test spinner...", "auth", "status") | ||
| _ = err |
There was a problem hiding this comment.
TestRunGHFunctions currently has no assertions and will pass regardless of whether gh is installed or whether the user is authenticated (both err values are ignored). To actually validate the wrappers, consider asserting behavior (e.g., compare RunGH(...) with RunGHContext(context.Background(), ...), and same for combined) and/or assert an expected error/output for a deterministic command; if that requires a real gh, it likely belongs behind the integration build tag with appropriate skips.
See below for a potential fix:
// Set up test environment with no tokens to keep behavior deterministic for unit tests.
os.Unsetenv("GH_TOKEN")
os.Unsetenv("GITHUB_TOKEN")
t.Run("RunGH matches RunGHContext", func(t *testing.T) {
gotOut, gotErr := RunGH("Test spinner...", "auth", "status")
wantOut, wantErr := RunGHContext(context.Background(), "Test spinner...", "auth", "status")
assert.Equal(t, wantOut, gotOut, "RunGH should return the same output as RunGHContext")
if wantErr == nil {
assert.NoError(t, gotErr, "RunGH should match RunGHContext error behavior")
} else {
require.Error(t, gotErr, "RunGH should match RunGHContext error behavior")
assert.Equal(t, wantErr.Error(), gotErr.Error(), "RunGH should return the same error text as RunGHContext")
}
})
t.Run("RunGHCombined matches RunGHCombinedContext", func(t *testing.T) {
gotOut, gotErr := RunGHCombined("Test spinner...", "auth", "status")
wantOut, wantErr := RunGHCombinedContext(context.Background(), "Test spinner...", "auth", "status")
assert.Equal(t, wantOut, gotOut, "RunGHCombined should return the same output as RunGHCombinedContext")
if wantErr == nil {
assert.NoError(t, gotErr, "RunGHCombined should match RunGHCombinedContext error behavior")
} else {
require.Error(t, gotErr, "RunGHCombined should match RunGHCombinedContext error behavior")
assert.Equal(t, wantErr.Error(), gotErr.Error(), "RunGHCombined should return the same error text as RunGHCombinedContext")
}
})
| } | ||
|
|
||
| if ghToken != "" || githubToken != "" { | ||
| githubCLILog.Printf("Using gh CLI for command: gh %v", args) |
There was a problem hiding this comment.
The log branch here is keyed off token availability, but the "Using gh CLI for command" message doesn’t actually convey what’s different versus the no-token branch (both cases execute gh). Consider adjusting the message to explicitly reflect token presence (e.g., "Token detected…" vs "No token…") so the conditional logging remains meaningful for debugging.
| githubCLILog.Printf("Using gh CLI for command: gh %v", args) | |
| githubCLILog.Printf("Token detected, using gh CLI for command: gh %v", args) |
🧪 Test Quality Sentinel ReportTest Quality Score: 30/100❌ Poor test quality — the modified test has zero assertions
Test Classification Details
Flagged Tests — Requires Review
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 30/100. 100% of modified tests are classified as low-value smoke tests (threshold: 30%). The single modified test (TestRunGHFunctions) discards both return values with _ = err and makes zero assertions — it cannot catch any behavioral regression. Please add at minimum assert.Error calls to verify the expected authentication failure, and consider verifying that RunGH/RunGHCombined delegate correctly to their *Context counterparts (the core behavioral contract of this refactor).
|
@copilot review all comments |
…feedback Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4452668a-e5cf-4e0f-b590-7645574d92d5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both review comments in commit
|
This PR simplifies recently modified code from #26372 (context propagation) to reduce duplication while preserving all functionality.
Files Simplified
pkg/workflow/github_cli.go— removed duplicatedrunGHWithSpinnerhelper and consolidated loggingpkg/workflow/github_cli_test.go— updated test to use the publicRunGH/RunGHCombinedfunctionsImprovements Made
1. Removed
runGHWithSpinnerduplication (~30 lines)runGHWithSpinnerandrunGHWithSpinnerContextwere identical except for usingExecGHvsExecGHContext. Sincecontext.Background()never cancels, delegating the no-context versions to the context variants is functionally equivalent:2. Simplified
setupGHCommandloggingThe token-check logging was duplicated inside both branches of the
if ctx != nilblock. The only difference was the phrase "with context" in the message, which adds no debugging value (the caller's function name already conveys this). Hoisting the log calls outside the branch eliminates 8 lines:Changes Based On
Follows #26372 — fix: propagate context through all CLI subprocess calls for Ctrl-C cancellation
Testing
pkg/workflowtests pass (go test ./pkg/workflow/...)make build)gofmt -l)context.Background()never cancels)