Add ctxutil.OrBackground helper to consolidate nil-context fallback duplication - #50093
Conversation
|
Hey Process Note: CONTRIBUTING.md explicitly states that this project uses agentic development by a core team only, and 🚫 Traditional Pull Requests Are Not Enabled for non-Core team members. Since you're operating as a core team agent, this appears to follow the intended process—but ensure the authorization is properly established. On-Topic ✅: This PR addresses a legitimate code quality improvement: consolidating 8+ duplicate nil-context fallback patterns into a single Current Status: This is a WIP PR with only a planning commit (0 file changes). Before it's ready for review, ensure:
Keep the PR description updated as you make progress. Once all migrations are complete and tests pass (
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete. Hard violation found: pkg/ctxutil/ctxutil_test.go missing required (go/redacted):build !integration tag on line 1. Test quality is high (100% design tests with 4 assertions covering nil/non-nil/value preservation), but build tag violation requires fix before merge. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (82 additions detected). |
There was a problem hiding this comment.
🟢 Ready to approve
The helper preserves existing semantics, covers every listed call site, and includes focused tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Introduces a shared context fallback helper and migrates eight duplicated call sites without changing behavior.
Changes:
- Added and tested
ctxutil.OrBackground. - Replaced inline nil-context guards across CLI and workflow packages.
File summaries
| File | Description |
|---|---|
pkg/ctxutil/ctxutil.go |
Adds the shared context helper. |
pkg/ctxutil/ctxutil_test.go |
Tests nil and non-nil contexts. |
pkg/cli/retry.go |
Uses the helper for repeat operations. |
pkg/cli/signal_aware_poll.go |
Uses the helper for polling. |
pkg/cli/setup_repository.go |
Consolidates two setup fallbacks. |
pkg/cli/init.go |
Uses the helper during initialization. |
pkg/workflow/auto_update_workflow.go |
Uses the helper during generation. |
pkg/workflow/compiler_model_pricing.go |
Uses the helper for pricing resolution. |
pkg/workflow/github_cli_wasm.go |
Uses the helper in the WASM stub. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
The refactor is clean and correct.
ctxutil.OrBackgroundis implemented correctly with proper nil-guard semantics.- Table-driven tests cover both nil and non-nil cases, including value preservation.
- All 8 call sites migrated consistently; unused
contextimports removed where applicable. - No behavior change introduced.
LGTM ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.7 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a textbook deep-module extraction and the change looks great.
📋 Key Themes & Highlights
Positive Highlights
- ✅
OrBackgroundis the right abstraction: zero-surface-area interface, rich convention enforcement across 8 call sites - ✅ Package name
ctxutilfollows the existing-utilsuffix convention (fileutil,gitutil) — consistent vocabulary - ✅ Table-driven tests cover both branches (nil →
context.Background()singleton check, non-nil → identity + value preservation) - ✅
compiler_model_pricing.gocorrectly drops the now-unusedcontextimport - ✅
github_cli_wasm.gocorrectly uses reassignment (ctx =) rather than short declaration sincectxis already a parameter
No actionable issues found. The change reduces future drift exactly as intended.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.3 AIC · ⌖ 11.6 AIC · ⊞ 7.1K
Comment /matt to run again
🧪 Test Quality Sentinel Report❌ Test Quality Score: FAILED — Hard Violation
📊 Metrics (1 test)
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: Hard violation detected. New Go test file pkg/ctxutil/ctxutil_test.go is missing required (go/redacted):build !integration tag on line 1. All *_test.go files must declare a build tag per gh-aw conventions. Fix: Add (go/redacted):build !integration as the first line. Test quality is high (1 design test, 4 assertions, comprehensive nil/non-nil/value coverage) but build tag is a blocker.
There was a problem hiding this comment.
Verdict: Approve
Clean, low-risk refactor. Extracts the duplicated nil-context fallback into ctxutil.OrBackground, migrates all 9 call sites, and adds solid table-driven tests covering both nil and non-nil paths.
Review notes
- Verified build (
go build ./...) andgo vetpass on all touched packages. - Confirmed the
contextimport remains used in every modified file (still referenced forcontext.Contexttype,context.Canceled,context.Background()at other call sites), so no dead imports were introduced. - Logic is a pure 1:1 behavioral substitution — no functional changes, no edge cases altered.
- Test coverage in
ctxutil_test.gois appropriate for the small surface area (nil input, and value-preservation for a non-nil context).
No blocking issues found.
🔎 Code quality review by PR Code Quality Reviewer · auto · 17.3 AIC · ⌖ 4.48 AIC · ⊞ 7.9K
Comment /review to run again
|
@copilot This PR still needs another pass before maintainers can investigate quickly.
|
Current head is
Actions taken: refreshed |
|
🎉 This pull request is included in a new release. Release: |
At least 8 call sites in
pkg/cliandpkg/workflowduplicated the same inline nil-context fallback:New helper
pkg/ctxutilpackage withOrBackground(ctx context.Context) context.Context, returningcontext.Background()whenctxis nil, otherwisectxunchanged.context.WithValuecontext.Migrated call sites
pkg/cli/retry.go,pkg/cli/signal_aware_poll.go,pkg/cli/setup_repository.go(2 occurrences),pkg/cli/init.gopkg/workflow/auto_update_workflow.go,pkg/workflow/compiler_model_pricing.go,pkg/workflow/github_cli_wasm.goAll replaced with:
Removed now-unused
contextimports where applicable (e.g.compiler_model_pricing.go). No behavior change — this centralizes the convention and reduces future drift.Branch refresh requested by PR Sous Chef · run: https://github.com/github/gh-aw/actions/runs/30857060078