Two follow-up nits surfaced during PR #519 review (drift class enum + apply --refresh). Neither blocks the PR — both deferred to a separate cleanup PR.
1. runInfraApply post-refresh closer ordering — cmd/wfctl/infra.go:1033-1042
The refresh-phase block correctly closes the provider whether or not the refresh succeeded:
refreshErr := runInfraApplyRefreshPhase(...)
if closer != nil {
if cerr := closer.Close(); cerr != nil { /* warn */ }
}
if refreshErr != nil {
return fmt.Errorf("refresh phase: %w", refreshErr)
}
This pattern is functionally correct but invites a refactor trap: a future maintainer adding an early return on refreshErr before the close block (or moving the close past additional logic) would silently leak the provider connection.
Proposed fix: replace the inline close with defer closer.Close() (or a deferred warn-on-error wrapper) immediately after the nil-check on provErr, so close is guaranteed regardless of subsequent control flow.
2. TestApplyRefresh_AutoApprovePrunesAndApplies — narrow stderr assertion
cmd/wfctl/infra_apply_refresh_test.go:143-146 only asserts the resource name appears in stderr:
if !strings.Contains(stderr.String(), "test-vpc") { ... }
If the audit log format silently changes (e.g., the state mutation prune operation keyword is dropped, or the log is misrouted to stdout), the test still passes because the resource name is also present in the dry-run stdout output that this test does not capture as separate from stderr.
Proposed fix: add a strings.Contains(stderr.String(), "state mutation prune") check alongside the existing name check, so format regressions are caught.
Scope
Both are non-blocking. Bundle into a small follow-up PR after the drift-recovery chain (PR-D1, PR-D3a, PR-D3b) lands, or roll into iteration work in PR-D4. Tag: maintenance / followup.
Two follow-up nits surfaced during PR #519 review (drift class enum + apply --refresh). Neither blocks the PR — both deferred to a separate cleanup PR.
1.
runInfraApplypost-refresh closer ordering —cmd/wfctl/infra.go:1033-1042The refresh-phase block correctly closes the provider whether or not the refresh succeeded:
This pattern is functionally correct but invites a refactor trap: a future maintainer adding an early return on
refreshErrbefore the close block (or moving the close past additional logic) would silently leak the provider connection.Proposed fix: replace the inline close with
defer closer.Close()(or a deferred warn-on-error wrapper) immediately after the nil-check onprovErr, so close is guaranteed regardless of subsequent control flow.2.
TestApplyRefresh_AutoApprovePrunesAndApplies— narrow stderr assertioncmd/wfctl/infra_apply_refresh_test.go:143-146only asserts the resource name appears in stderr:If the audit log format silently changes (e.g., the
state mutation pruneoperation keyword is dropped, or the log is misrouted to stdout), the test still passes because the resource name is also present in the dry-run stdout output that this test does not capture as separate from stderr.Proposed fix: add a
strings.Contains(stderr.String(), "state mutation prune")check alongside the existing name check, so format regressions are caught.Scope
Both are non-blocking. Bundle into a small follow-up PR after the drift-recovery chain (PR-D1, PR-D3a, PR-D3b) lands, or roll into iteration work in PR-D4. Tag: maintenance / followup.