pkg/cli tests: migrate assert.NoError to require.NoError for fail-fast error handling

[Copilot]

pkg/cli tests had assert.NoError checks on critical error paths, allowing execution to continue after setup/command failures and creating cascading noise. This change applies a mechanical fail-fast migration to require.NoError across the suite.

• Mechanical refactor

  • Replaced all assert.NoError(t, err, ...) occurrences in pkg/cli/*_test.go with require.NoError(t, err, ...).
  • Scope is test-only and limited to assertion semantics.

• Import alignment

  • Added github.com/stretchr/testify/require where newly needed (notably mcp_server_workflow_validation_test.go).
  • Left non-NoError assertions unchanged.

• Behavioral intent

  • Error preconditions now stop the test immediately, so downstream assertions only run on valid state.

// before
err := runSomething()
assert.NoError(t, err)
assert.Equal(t, "expected", got)

// after
err := runSomething()
require.NoError(t, err)
assert.Equal(t, "expected", got)

• Fixes [deep-report] Migrate 251 assert.NoError calls to require.NoError in pkg/cli/ tests #47344

[Copilot]

Pull request overview

Migrates pkg/cli tests to fail fast when critical operations return errors.

Changes:

• Replaces remaining assert.NoError calls with require.NoError.
• Adds the newly required Testify import.

Show a summary per file

File	Description
pkg/cli/update_integration_test.go	Makes update integration checks fail fast.
pkg/cli/update_extension_check_test.go	Makes backup existence validation fatal.
pkg/cli/update_command_test.go	Makes update command validation fatal.
pkg/cli/trial_dry_run_test.go	Makes successful dry-run preconditions fatal.
pkg/cli/status_remote_test.go	Makes remote status error validation fatal.
pkg/cli/spec_test.go	Makes successful API validation checks fatal.
pkg/cli/secrets_command_test.go	Makes command execution errors fatal.
pkg/cli/remote_workflow_test.go	Makes remote workflow operations fail fast.
pkg/cli/project_command_test.go	Makes valid argument checks fatal.
pkg/cli/outcome_eval_test.go	Makes endpoint validation fail fast.
pkg/cli/mcp_server_workflow_validation_test.go	Adds require and uses fatal validation.
pkg/cli/logs_download_agent_outputs_test.go	Makes artifact flattening errors fatal.
pkg/cli/logs_artifact_set_test.go	Makes successful set validation fatal.
pkg/cli/list_workflows_command_test.go	Makes workflow-listing errors fatal.
pkg/cli/domains_command_test.go	Makes domain command failures fatal.
pkg/cli/copilot_billing_check_test.go	Makes successful billing checks fatal.
pkg/cli/completion_integration_test.go	Makes completion-file validation fatal.
pkg/cli/compile_safe_update_integration_test.go	Makes compilation and file checks fatal.
pkg/cli/compile_guard_policy_test.go	Makes successful compilation checks fatal.
pkg/cli/checks_command_test.go	Makes valid argument checks fatal.
pkg/cli/add_command_test.go	Makes successful add operations fatal.
pkg/cli/actions_build_command_test.go	Makes action command errors fatal.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 22/22 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

PR #47355 is purely a refactoring migration from assert.NoError to require.NoError for fail-fast error handling. No new tests were added or modified in logic — only assertion method calls were changed. Test Quality Sentinel analysis skipped as there are no new or substantively modified test functions to analyze.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #47355 does not have the 'implementation' label and has only 50 new lines of code in business logic directories (threshold: 100).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

Clean, mechanical migration from assert.NoError to require.NoError across CLI test files. This is the correct pattern — fail-fast error handling prevents cascading failures and makes test output easier to diagnose. No issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 15.6 AIC · ⌖ 4.45 AIC · ⊞ 5K

[github-actions]

Skills-Based Review

Applied /tdd — this is a clean, correct mechanical migration.

Positive Highlights

• All assert.NoError on critical error paths correctly replaced with require.NoError for fail-fast semantics
• Scope is precisely limited to test files — no production code touched
• Import additions are minimal and correct
• Non-NoError assertions intentionally left unchanged — appropriate

No issues found. The migration is complete and consistent across all 22 changed files.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 18 AIC · ⌖ 4.48 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

🧪 Test Quality Sentinel Report

i️ Refactoring: No New Test Coverage

Analyzed 0 new/modified test functions. This PR is a code style migration from assert.NoError to require.NoError for fail-fast error handling across 22 existing test files.

📊 Analysis Summary

Change Classification:

• Files modified: 22 test files in pkg/cli/
• Assertion calls changed: 49 instances of assert.NoError → require.NoError
• New test functions: 0
• New test files: 0
• Test logic changes: None

Imports Added:

• github.com/stretchr/testify/require added where needed

Assessment:
This PR is purely a refactoring that improves test failure semantics without changing test coverage or behavioral verification. The migration from assert to require means tests now fail fast on error instead of continuing, which is a best-practice improvement but does not affect the existing test suite's scope or quality.

Verdict

✅ Pass — No action needed. Test Quality Sentinel does not score refactoring-only changes. Existing test coverage remains unchanged; the PR improves failure handling semantics only.

🧪 Test quality analysis by Test Quality Sentinel · haiku45 19.8 AIC · ⌖ 7.67 AIC · ⊞ 7.1K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: Refactoring migration from assert.NoError to require.NoError. No new test coverage added; existing tests remain unchanged. The fail-fast improvement is a best-practice enhancement.

[github-actions]

REQUEST_CHANGES — one migration artifact left behind

The mechanical assert.NoError → require.NoError migration is correct and well-scoped, but it introduced one piece of dead code that must be fixed before merge.

Blocking finding

pkg/cli/completion_integration_test.go line 158: The if err == nil guard that wraps the ReadFile block was necessary when line 155 used assert.NoError (execution continued on failure). Now that line 155 uses require.NoError, the test aborts immediately on a stat failure — the guard is always true when reached and should be removed.

No other issues found. The migration is otherwise correct across all 22 files.

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 58.8 AIC · ⌖ 4.66 AIC · ⊞ 5.7K
_{Comment /review to run again}

[github-actions]

Dead code: if err == nil guard is now always true after migrating to require.NoError.

💡 Suggested fix

Before the migration, assert.NoError on line 155 let the test continue even when os.Stat failed, so the if err == nil guard was meaningful — it protected the ReadFile call from panicking.

After replacing it with require.NoError, the test halts immediately on a stat failure. The guard is therefore dead code: it is always true when execution reaches line 158. It should be removed to avoid misleading future readers.

// After the migration, guard is redundant — remove it:
_, err = os.Stat(completionPath)
require.NoError(t, err, "Completion file should exist at: %s", completionPath)

content, readErr := os.ReadFile(completionPath)
require.NoError(t, readErr, "Should be able to read completion file")
assert.Contains(t, string(content), "# bash completion")
assert.Contains(t, string(content), "__start_gh")

This is a direct consequence of the assert → require change on line 155; the guard must be cleaned up alongside it.

Done

[pelikhan]

@copilot run pr-reviewer skill

[Copilot]

@copilot run pr-reviewer skill

Ran a PR review on the current diff and a targeted go test ./pkg/cli -count=1. No actionable issues were found in these changes.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.83.1