Fix T-1325: Compile integration tests in default build & fix loadAWSConfig stubs

[ArjenSchwarz]

Summary

go test -tags integration ./cmd/ failed to build: two loadAWSConfig stubs in cmd/deploy_integration_test.go used the stale signature func(c config.Config) instead of the current func(ctx context.Context, config config.Config) (loadAWSConfig = config.DefaultAwsConfig, which gained a context.Context).

This went unnoticed because the integration files were gated by //go:build integration, while the Makefile (test-integration, test-all) and CI run INTEGRATION=1 go test ./... without -tags integration — so the files were never compiled or run by any documented path. The build tag was also redundant with the testutil.SkipIfIntegration(t) runtime gate the files already use.

Changes

• Fix both loadAWSConfig stub signatures (add context.Context).
• Remove //go:build integration from cmd/deploy_integration_test.go and cmd/drift_integration_test.go. They still skip at runtime via SkipIfIntegration, but now compile in the default go test ./... build (so CI catches this class of drift) and run under INTEGRATION=1. Aligns with the project Go rules (env-var gating over build tags).
• Skip the rotted TestDeploymentWorkflow_EndToEnd (it makes a real CreateChangeSet call → "Missing Region", and treats inline tags as a file) pending T-1340.
• Update CLAUDE.md integration-test docs.

Verification

• go test ./... compiles + passes (integration bodies skip at runtime).
• INTEGRATION=1 go test ./... passes — 10/11 integration tests run (7 drift + 3 deploy workflow); TestDeploymentWorkflow_EndToEnd skipped pending T-1340.
• golangci-lint run ./... reports 0 issues.

Bugfix report: specs/bugfixes/deploy-integration-test-build/report.md

[Copilot]

Pull request overview

This pull request fixes a build break in cmd integration tests caused by loadAWSConfig’s signature change, and ensures these integration test files are compiled by default so similar drift is caught by CI/standard test runs.

Changes:

• Update loadAWSConfig stubs in cmd/deploy_integration_test.go to accept context.Context.
• Remove //go:build integration gating from cmd integration test files so they compile in default go test ./... runs while still runtime-skip unless INTEGRATION=1.
• Temporarily skip the rotted TestDeploymentWorkflow_EndToEnd test (tracked as T-1340) and document integration test gating in CLAUDE.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
specs/bugfixes/deploy-integration-test-build/report.md	Adds a bugfix report documenting root cause, resolution, and verification for T-1325.
cmd/drift_integration_test.go	Removes the integration build tag so the file compiles in default test builds.
cmd/deploy_integration_test.go	Fixes loadAWSConfig stub signatures, removes build tag, and skips the known-rotted end-to-end test.
CLAUDE.md	Updates integration test documentation to reflect env-var/runtime gating and default compilation.

---

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

[claude]

Code Review — T-1325: Fix integration test build & loadAWSConfig stubs

Overview

This PR correctly identifies and fixes two layered defects:

1. Immediate: two loadAWSConfig stubs in deploy_integration_test.go had a stale signature after DefaultAwsConfig gained a context.Context parameter
2. Structural: a //go:build integration tag gated the files from ever compiling under the project's actual test commands (INTEGRATION=1 go test ./... / make test-integration), making the drift invisible

The fix — remove the build tags, rely on the existing testutil.SkipIfIntegration(t) runtime gate — is the right call. The files now compile under the default go test ./..., so future signature drift is caught immediately by CI.

---

What's Good

• Correct architectural fix. Removing the build tags rather than adding -tags integration to the Makefile is the right call. It aligns with the project's env-var gating convention and makes CI useful for catching this class of drift.
• Underscore parameter naming (_ context.Context, _ config.Config) correctly avoids shadowing the imported config package — a subtle but important detail.
• CLAUDE.md updated to document the new convention so future contributors don't reintroduce build tags.
• specs/bugfixes/ report follows the established project convention.

---

Issues

1. Redundant double-skip in TestDeploymentWorkflow_EndToEnd (minor, but confusing)

func TestDeploymentWorkflow_EndToEnd(t *testing.T) {
    testutil.SkipIfIntegration(t)  // skips unless INTEGRATION=1
    t.Skip("T-1340: ...")          // skips always
    ...
}

The test is now always skipped regardless of mode, but the reason surfaced depends on which mode you're in:

• Default go test ./... → "skipping integration test" (the real reason — T-1340 rot — is invisible)
• INTEGRATION=1 go test ./... → "T-1340: mock wiring drifted…" (correct)

Consider swapping the order or dropping SkipIfIntegration entirely for this test since it's unconditionally broken:

func TestDeploymentWorkflow_EndToEnd(t *testing.T) {
    t.Skip("T-1340: mock wiring drifted — re-enable after repair")
    // testutil.SkipIfIntegration(t) not needed; test is disabled pending repair
    ...
}

This makes the T-1340 reason visible even in go test ./... output, and removes confusion about what the effective skip condition is.

2. CI still never runs integration test bodies (not a blocker, but worth noting)

The PR description correctly flags this in the Prevention section. With the build tag removed, CI now compiles the integration files — which catches signature drift. But INTEGRATION=1 is still not set in CI, so the test bodies never execute. The 10/11 tests that run locally under INTEGRATION=1 are never exercised in CI. A follow-up CI job (INTEGRATION=1 go test ./...) would complete the fix. The report already recommends this, so no change needed here, just flagging it as a remaining gap.

---

Summary

The core fix is correct and well-reasoned. The only actionable suggestion is swapping the skip order in TestDeploymentWorkflow_EndToEnd so the T-1340 reason surfaces in all modes, not just when INTEGRATION=1 is set. Everything else looks good — tests pass, linter is clean, and the structural change prevents this class of drift from going undetected again.

[claude]

Code Review: Fix T-1325 — Compile integration tests in default build & fix loadAWSConfig stubs

Overall assessment: This is a well-reasoned structural fix that correctly diagnoses and resolves a root-cause bug. The changes are minimal, targeted, and the rationale is sound. A few minor issues are noted below.

---

What the PR does well

Root cause identification is correct. The PR correctly identifies two layered defects: (1) the immediate stale stub signatures, and (2) the structural problem that made the breakage invisible — the //go:build integration tag excluded the files from every CI path while INTEGRATION=1 go test ./... (without -tags integration) never compiled them.

The fix is the right approach. Removing the build tag so the files always compile, while keeping the SkipIfIntegration runtime gate, is consistent with the project's own Go rules documented in CLAUDE.md. The default go test ./... (and CI's go test ./... -v) now compiles the integration test files, catching future signature drift immediately.

Stub signature fixes are correct. Both loadAWSConfig stubs in deploy_integration_test.go now match the actual config.DefaultAwsConfig signature func(ctx context.Context, config config.Config) (config.AWSConfig, error). Using _ for both unused parameters avoids shadowing the config package.

CLAUDE.md update is accurate and helpful. The revised integration-test section now correctly documents the env-var gating approach and explicitly discourages adding //go:build integration tags.

---

Issues found

Medium: t.Skip before testutil.SkipIfIntegration makes the integration gate unreachable

File: cmd/deploy_integration_test.go, lines ~21–22

t.Skip("T-1340: mock wiring drifted — ...")
testutil.SkipIfIntegration(t)   // unreachable — t.Skip already called

t.Skip calls runtime.Goexit() immediately, so testutil.SkipIfIntegration on the next line is dead code. This is functionally benign now (the test is intentionally skipped), but when T-1340 is resolved and the t.Skip is removed, the intent becomes clearer if the order is swapped:

testutil.SkipIfIntegration(t)
t.Skip("T-1340: ...")

This way: the integration gate fires first (skipping in normal go test), and the T-1340 note fires only when INTEGRATION=1 is set but the test isn't ready yet.

---

Low: deploy_output_integration_test.go has no SkipIfIntegration gate

File: cmd/deploy_output_integration_test.go (not modified by this PR)

This file (TestDeploy_SuccessfulWithJSONOutput, TestDeploy_FailureWithFormattedOutput, TestDeploy_QuietMode, TestDeploy_DryRunMultipleFormats, TestDeploy_NoChanges) has no //go:build integration tag and no testutil.SkipIfIntegration calls. It runs unconditionally under go test ./....

This is inconsistent with the _integration_test.go naming convention and the pattern this PR establishes. Worth reviewing in a follow-up to either add a SkipIfIntegration gate or rename the file.

---

Low: tc := tc loop variable capture is unnecessary in Go 1.22+

Files: cmd/deploy_integration_test.go (lines ~125, 333, 450, 592), cmd/drift_integration_test.go (lines ~109, 283, 598, 728)

The module is go 1.25.3. Since Go 1.22, loop variables are re-bound per iteration so the tc := tc idiom is a no-op. Since the parallel runs are commented out anyway (due to global Viper state), this is harmless but creates unnecessary noise. Can be cleaned up in a follow-up.

---

Observation: CI does not run integration tests

The push.yml workflow runs go test ./... -v only — it does not set INTEGRATION=1. This PR ensures the integration files compile in CI (a clear improvement), but the integration test bodies still only run locally via make test-integration. The specs report notes this and suggests adding an INTEGRATION=1 CI job as a follow-up.

---

Summary

Finding	Severity	Action
t.Skip before SkipIfIntegration makes guard unreachable	Medium	Swap order before T-1340 is resolved
deploy_output_integration_test.go lacks SkipIfIntegration	Low	Follow-up to align naming or add gate
tc := tc loop capture unnecessary in Go 1.25	Low	Follow-up cleanup
CI doesn't run integration test bodies	Observation	Tracked in specs report; follow up with T-1340

The core fix (stub signatures + build tag removal) is correct and should be merged.

Reviewed by Claude Sonnet 4.6