Improve fix command test robustness in fix_command_test.go

[Copilot]

This issue targets test-quality improvements in pkg/cli/fix_command_test.go.
The update tightens one flaky setup path in TestFixCommand_UpdatesPromptAndAgentFiles so failures are explicit and environment-dependent skips are handled cleanly.

• Setup reliability

  • Move git availability check to the start of the test and skip immediately when unavailable.
  • Keep setup preconditions strict by asserting all critical setup calls succeed (git init, git config, cwd changes, file writes).

• Assertion quality

  • Replace manual error branching in key setup/execute paths with require.NoError(...) to fail fast at the point of failure.
  • Keep the test focused on command behavior rather than tolerant setup failures.

• Cleanup behavior

  • Use t.Cleanup with explicit error reporting when restoring the original working directory.

if _, err := exec.LookPath("git"); err != nil {
	t.Skip("Git not available")
}

originalDir, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() {
	if chdirErr := os.Chdir(originalDir); chdirErr != nil {
		t.Errorf("Failed to restore current directory: %v", chdirErr)
	}
})

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

[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 #35564 does not have the 'implementation' label and has only 19 new lines of code in business logic directories (well below the 100-line threshold).

[Copilot]

Pull request overview

This PR improves robustness of a fix command smoke test by making setup failures explicit and skipping early when git is unavailable. It also includes a regenerated workflow lock file.

Changes:

• Adds testify/require assertions to fail fast during test setup and execution.
• Checks for git availability before creating the test repository.
• Regenerates .github/workflows/outcome-collector.lock.yml with updated generated workflow content and image/action references.

Show a summary per file

File	Description
pkg/cli/fix_command_test.go	Tightens setup and execution assertions in TestFixCommand_UpdatesPromptAndAgentFiles.
.github/workflows/outcome-collector.lock.yml	Updates generated workflow lock output and dependency versions.

Copilot's findings

Tip

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

• Files reviewed: 2/2 changed files
• Comments generated: 0

[github-actions]

🧪 Test Quality Sentinel Report

⚠️ Test Quality Score: 70/100 — Acceptable

Analyzed 1 test (modified): TestFixCommand_UpdatesPromptAndAgentFiles — 1 design test, 0 implementation tests, 0 guideline violations. Changes improve test robustness without altering behavioral coverage.

📊 Metrics & Test Classification (1 test analyzed)

Metric	Value
New/modified tests analyzed	1
✅ Design tests (behavioral contracts)	1 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	0 (0%)
Duplicate test clusters	0
Test inflation detected	No
🚨 Coding-guideline violations	0

Test Classification Details

Test	File	Classification	Issues Detected
TestFixCommand_UpdatesPromptAndAgentFiles	pkg/cli/fix_command_test.go:625	✅ Design	Happy-path only (pre-existing)

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 1 test — unit (//go:build !integration)

Verdict

✅ Check passed. 0% of new/modified tests are implementation tests (threshold: 30%). The PR improves setup robustness by (1) moving the git availability skip to the top of the test before any resource allocation, (2) replacing t.Fatalf with require.NoError for proper testify-style assertions with descriptive messages, and (3) using t.Cleanup instead of defer for more reliable directory restoration. All assertion messages are present. The behavioral contract tested (RunFix succeeds on a valid workflow) is unchanged.

i️ Note on edge-case score: The tested function has no negative-path coverage, but this is pre-existing and not introduced by this PR. The 70/100 score reflects the absence of error-case coverage in the modified test.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 1M · ◷

[github-actions]

✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new/modified tests are implementation tests (threshold: 30%). PR improves TestFixCommand_UpdatesPromptAndAgentFiles robustness: early git skip, require.NoError with descriptive messages, t.Cleanup for safe teardown. No guideline violations detected.

[github-actions]

Skills-Based Review 🧠

Applied /tdd — approving with one observation.

📋 Key Themes & Highlights

Positive Highlights

• ✅ Early exec.LookPath guard correctly separates "git not installed" (→ skip) from "git present but setup fails" (→ fail), fixing a previously misleading t.Skip inside git init
• ✅ t.Cleanup with t.Errorf replaces defer _ = os.Chdir(...) — directory-restore failures are now visible instead of silently swallowed
• ✅ require.NoError throughout makes the test fail fast at the precise point of failure rather than propagating incorrect state

Observations

• Other test functions in the file (TestFixCommand_TimeoutMinutesMigration, TestFixCommand_GrepToolRemoval, etc.) still use the old manual if err != nil { t.Fatalf } pattern. The improvements here are correct and merge-safe as-is, but a follow-up pass to apply the same require.NoError style uniformly would improve consistency.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.4M

[github-actions]

[/tdd] Behavior change worth noting: previously git config failures were silently swallowed; they now fail the test via require.NoError. This is correct — exec.LookPath at line 626 already guards against git being unavailable, so a git config failure here indicates a genuine environment problem worth surfacing.

💡 Why this is safe

The old code:

exec.Command("git", "config", "user.name", "Test User").Run() // ignored error

could silently produce misleading test state (a git repo without user config). The new require.NoError makes it explicit that config setup is a hard precondition.

[github-actions]

Approved — clean test hygiene improvements

Both changed files look correct.

pkg/cli/fix_command_test.go — all changes are improvements:

• Moving exec.LookPath("git") to the top correctly skips before any side-effects.
• require.NoError on git init: LookPath already confirmed the binary; a failed init indicates a real environment problem, not "git unavailable". The old t.Skip on init failure was masking genuine failures.
• git config (no --global) writes to tmpDir/.git/config — always writable after git init, so require.NoError is safe.
• t.Cleanup with explicit t.Errorf vs the old silent _ = os.Chdir(...) gives a clear failure message on restore errors.

outcome-collector.lock.yml — auto-generated recompile: container bump 0.25.55→0.25.56, mcpg v0.3.19→v0.3.20, schema v3→v4, switch from pinned external github/gh-aw-actions/setup to local ./actions/setup.

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.4M