test: isolate update checks from agent CI environment - #50918
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 67bef6e0-d62a-4050-aa8e-42e75327fb7d
There was a problem hiding this comment.
Pull request overview
Isolates update-check tests from Copilot agent CI environment variables.
Changes:
- Clears
COPILOT_AGENT_SESSION_IDin non-CI test paths. - Restores the original environment after testing.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_check_test.go |
Isolates general update-check tests. |
pkg/cli/compile_update_check_test.go |
Isolates compile update-check tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
|
|
|
|
|
Test Quality Sentinel: Infrastructure-only PR with proper environment isolation. No new behavioral tests, no violations. 8 lines added for COPILOT_AGENT_SESSION_ID environment variable isolation across compile_update_check_test.go and update_check_test.go. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — the changes are correct and well-targeted.
📋 Summary
Key Themes
- Test isolation improved:
COPILOT_AGENT_SESSION_IDis now cleared in 6 test functions across 2 files, preventing the agent CI environment from silently disabling update-check code paths under test. - Pattern consistency: Tests that already use
t.Setenvget the new env var viat.Setenv(automatic restore). The one test that uses manualos.Getenv/os.Setenvsave-restore (TestShouldCheckForUpdate) is handled correctly.
Positive Highlights
- ✅
t.Setenvused where possible — automatic cleanup, no risk of leaking state between subtests. - ✅ Minimal, surgical change — exactly what was needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.1 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
Verdict: Approve — minor consistency change
This PR mechanically extends existing env-var isolation (already used for CI, GITHUB_ACTIONS, CONTINUOUS_INTEGRATION) to also cover COPILOT_AGENT_SESSION_ID. No logic changes, no new risk introduced.
Review notes
- The restore pattern in
update_check_test.go(os.Setenv("COPILOT_AGENT_SESSION_ID", origCopilotAgentSessionID)) will set the var to empty string rather than unset it if it was originally unset — but this exactly mirrors the pre-existingorigCIhandling on the same lines, so it is not a new defect introduced by this diff. - Coverage of the new env var is consistent across all affected test functions in both files.
- No correctness, security, or performance concerns within the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 10.5 AIC · ⊞ 7.8K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
Summary
COPILOT_AGENT_SESSION_IDin update-check tests that require a non-CI environmentValidation
make test-unit-allmake agent-report-progress