Repository navigation
Conversation
golangci-lint v2 activated errcheck.check-blank:true which exposed 107 pre-existing blank-identifier error discards. This commit handles each violation properly: - Exclude generated mock/test-helper paths from errcheck in .golangci.yml (internal/mocks/, internal/auth/test_helpers.go, internal/database/postgres/testhelpers/) - Handle errors in production code with log/return as appropriate - Use //nolint:errcheck with specific justification only for documented fire-and-forget cases (singleflight, best-effort STS, w.Write) - Extract readLine helper (configure_azure.go) and getGCPProjectID, logMigrateVersion, createAzureServicePrincipal, payloadTypeMatches helpers to keep cyclomatic complexity within the gocyclo <=10 gate - Save purchase execution errors on money path are now logged as Errorf - errgroup.Wait + ctx.Err() checked in scheduler fan-out loops
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates CLI prompting, API handler and server contracts, transaction cleanup, credential resolution, purchase and scheduler pointer flow, Azure recommendation merging, and mock return handling. ChangesPrimary functional and wiring changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/configure_gcp.go (1)
388-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-creating stdin readers inside command execution.
Line [415] creates a fresh
bufio.Reader(os.Stdin)rather than reusing the caller’sreader. In piped/scripted flows this can lose buffered bytes and stall follow-up prompts.Suggested fix
func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { @@ switch choice { case "r", "run", "": - return executeGCPCommand(displayCmd, program, args...) + return executeGCPCommand(reader, displayCmd, program, args...) @@ -func executeGCPCommand(displayCmd string, program string, args ...string) error { +func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { @@ if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, rfErr := readLine(reader)Also applies to: 415-418
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_gcp.go` around lines 388 - 389, The code is creating a fresh bufio.Reader from os.Stdin instead of reusing the reader parameter passed to the function, which causes loss of buffered bytes in piped/scripted flows. Replace the bufio.Reader(os.Stdin) instantiation (around line 415) with the existing reader parameter that was passed to the current function, ensuring the same reader instance is reused throughout the command execution flow to preserve any buffered input.cmd/configure_azure.go (1)
386-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the same buffered reader for follow-up prompts.
Line [413] creates a new
bufio.Reader(os.Stdin)insideexecuteExplicitCommand. If input is piped and the caller’s reader already buffered bytes, this can drop buffered input and block/hang on retries.Suggested fix
func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { @@ switch choice { case "r", "run", "": - return executeExplicitCommand(displayCmd, program, args...) + return executeExplicitCommand(reader, displayCmd, program, args...) @@ -func executeExplicitCommand(displayCmd string, program string, args ...string) error { +func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { @@ if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, readErr := readLine(reader)Also applies to: 413-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_azure.go` around lines 386 - 387, The executeExplicitCommand function creates a new bufio.Reader(os.Stdin) at lines 413-417, which drops any buffered bytes from the caller's existing reader and causes input loss when piped. Modify executeExplicitCommand to accept a *bufio.Reader parameter instead of creating a new one internally. Then update the function call at lines 386-387 (and any other calls to executeExplicitCommand) to pass the caller's existing buffered reader as an argument, ensuring the same reader instance is reused for all prompts and input operations.providers/azure/recommendations.go (1)
167-167:⚠️ Potential issue | 🟠 MajorHandle
g.Wait()error instead of blank-discarding it.With
check-blank: trueenabled in.golangci.yml, blank-discarding the error fromg.Wait()at line 167 violates lint rules and masks potential failures. Add proper error handling:Proposed fix
- _ = g.Wait() + if err := g.Wait(); err != nil { + return nil, err + } if err := ctx.Err(); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations.go` at line 167, The g.Wait() error is being blank-discarded with the underscore assignment which violates the golangci-yml check-blank rule and hides potential failures. Replace the `_ = g.Wait()` statement with proper error handling by capturing the returned error, checking if it is not nil, and then either returning the error to the caller, logging it appropriately, or handling it based on the context of the recommendations.go function logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/configure_azure.go`:
- Around line 261-266: In the readLine function, the issue is that when io.EOF
is encountered (indicating closed or non-interactive stdin), the error is being
silently converted to nil even when no data was read. This allows empty input
from closed stdin to be treated as a valid empty choice. Fix this by checking
whether the trimmed line is actually empty when io.EOF is encountered. If the
line is empty and we received an EOF error, return the EOF error instead of nil
to properly indicate that no valid input was provided. Only convert EOF to nil
success when actual data was successfully read before the EOF.
In `@internal/credentials/resolver.go`:
- Around line 411-416: The current code treats all LoadRaw errors uniformly by
setting raw to nil, but according to the CredentialStore interface, only
not-found cases should fall back to the federated path while other errors
represent real store failures that need to be surfaced. Modify the error
handling for the store.LoadRaw call to use errors.Is(loadErr, pgx.ErrNoRows) to
specifically check if the error is a not-found error, and only set raw = nil in
that case. For other non-nil errors, propagate or return the error instead of
silently collapsing it to nil.
In `@internal/scheduler/scheduler.go`:
- Around line 352-355: In the collectAllProviders function, the error returned
from g.Wait() is being logged but not propagated, which allows the function to
continue and return success even when goroutines have failed. Instead of only
logging the waitErr, check if waitErr is not nil and return it immediately to
ensure that any goroutine errors are properly surfaced to the caller and prevent
partial/incorrect results from being returned.
---
Outside diff comments:
In `@cmd/configure_azure.go`:
- Around line 386-387: The executeExplicitCommand function creates a new
bufio.Reader(os.Stdin) at lines 413-417, which drops any buffered bytes from the
caller's existing reader and causes input loss when piped. Modify
executeExplicitCommand to accept a *bufio.Reader parameter instead of creating a
new one internally. Then update the function call at lines 386-387 (and any
other calls to executeExplicitCommand) to pass the caller's existing buffered
reader as an argument, ensuring the same reader instance is reused for all
prompts and input operations.
In `@cmd/configure_gcp.go`:
- Around line 388-389: The code is creating a fresh bufio.Reader from os.Stdin
instead of reusing the reader parameter passed to the function, which causes
loss of buffered bytes in piped/scripted flows. Replace the
bufio.Reader(os.Stdin) instantiation (around line 415) with the existing reader
parameter that was passed to the current function, ensuring the same reader
instance is reused throughout the command execution flow to preserve any
buffered input.
In `@providers/azure/recommendations.go`:
- Line 167: The g.Wait() error is being blank-discarded with the underscore
assignment which violates the golangci-yml check-blank rule and hides potential
failures. Replace the `_ = g.Wait()` statement with proper error handling by
capturing the returned error, checking if it is not nil, and then either
returning the error to the caller, logging it appropriately, or handling it
based on the context of the recommendations.go function logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a5dd19b-1687-434c-a0a1-ceb2589cf9eb
📒 Files selected for processing (24)
.golangci.ymlcmd/cleanup-lambda/main.gocmd/configure_azure.gocmd/configure_gcp.gocmd/rekey/main.gointernal/api/handler.gointernal/api/handler_accounts.gointernal/api/handler_dashboard.gointernal/api/handler_recommendations_refresh.gointernal/api/handler_registrations.gointernal/api/ri_utilization_cache.gointernal/api/validation.gointernal/auth/service_apikeys.gointernal/auth/service_mfa.gointernal/commitmentopts/store_postgres.gointernal/credentials/cipher.gointernal/credentials/resolver.gointernal/database/connection.gointernal/database/postgres/migrations/migrate.gointernal/purchase/execution.gointernal/scheduler/scheduler.gointernal/server/http.goproviders/azure/recommendations.goproviders/azure/recommendations_test.go
Three Major CR findings on the errcheck changes, each fixed at root cause rather than silencing the linter: - cmd/configure_azure.go: readLine treated a zero-byte io.EOF as a successful empty line, so a closed/non-interactive stdin looked like an empty choice and fell through to the default Run in prompt handlers. Now EOF is success only when bytes were actually read (final line without a trailing newline); an EOF with no data surfaces the error. Uses the raw read buffer (not the trimmed text) so a bare newline still counts as a valid empty line rather than a closed stream. - internal/credentials/resolver.go: resolveGCPWIFCredential swallowed every LoadRaw error by setting raw=nil and falling back to the federated path. LoadRaw already maps not-found to (nil, nil), so any non-nil error is a real store failure (DB connectivity, decrypt) that must be surfaced. Matches the established pattern in resolveAccessKeyProvider. - internal/scheduler/scheduler.go: collectAllProviders logged g.Wait()'s error but still merged outcomes, risking a partial aggregate reported as success. It now returns the error. fanOutPerAccount has no error return, so an unexpected g.Wait() error is reflected into the accountOutcome (FailedCount/LastErr) instead of being silently swallowed. Regression tests added: TestReadLine_EOFHandling (table-driven, covers the closed-stream-surfaces-EOF and bare-newline cases) and TestResolveGCPWIF_LoadRawError_Surfaces (wires the full federated path so the pre-fix code would mask the store error). Both fail on the pre-fix code and pass after.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Per owner directive: no //nolint:errcheck and no errcheck path-exclusions
in .golangci.yml. Every errcheck violation is now handled in code.
Inline //nolint:errcheck removed (8 sites):
- internal/config/store_postgres.go (DeleteCloudAccount, SetPlanAccounts):
deferred tx.Rollback now runs in a closure that logs a real rollback
failure and ignores only the already-committed case (errors.Is
pgx.ErrTxClosed).
- internal/config/store_postgres_recommendations.go (ReplaceRecommendations,
UpsertRecommendations): same deferred-rollback closure pattern.
- internal/api/ri_utilization_cache.go: singleflight callback now returns
nil after logging its own fetch error (no double-log); the outer Do result
is captured and logged defensively instead of blank-discarded.
- internal/auth/service_apikeys.go: same singleflight treatment for the
fire-and-forget UpdateLastUsed write.
- internal/api/handler.go (resolveSourceIdentity): the
resolveAWSCallerIdentity error is captured and logged; AccountID is left
empty so the consumer's empty-string check stays the security gate.
- internal/api/handler_dashboard.go (getDeploymentInfo): the
resolveAWSAccountID error is captured and logged; empty-string-safe
downstream behavior is unchanged.
.golangci.yml errcheck path-exclusions removed (internal/mocks/,
internal/auth/test_helpers.go, internal/database/postgres/testhelpers/),
and the violations in those files are hand-handled instead:
- 67 testify-mock type assertions converted from the single-value
args.Get(0).(T) form to a checked two-value form: val, ok :=
args.Get(0).(T); if !ok { return <zero>, args.Error(1) }. This satisfies
errcheck check-type-assertions without suppression and handles a
wrong-typed value a test might register.
- internal/database/postgres/testhelpers/postgres.go: the
postgresContainer.Terminate cleanup error on the connection-failure path
is now checked and logged via t.Logf.
Verified with golangci-lint v2.10.1 (the CI-pinned version): zero errcheck
issues, no //nolint:errcheck anywhere, no errcheck path-exclusions in
.golangci.yml (now identical to origin/main). go build ./... and
go test ./... pass (the one pre-existing Azure scheduler mock failure is
unrelated). The three earlier CR-finding fixes are preserved.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added
severity/medium
Raw ==/!= comparisons against sentinel errors fail on wrapped errors. Convert every such comparison in the files this PR touches to errors.Is. internal/config/store_postgres.go (7 sites): - lines 89, 263, 2271, 2559, 2609, 2827, 2881: err == pgx.ErrNoRows -> errors.Is(err, pgx.ErrNoRows) internal/database/postgres/migrations/migrate.go (7 sites): - lines 45, 450, 485: err != migrate.ErrNoChange -> !errors.Is(err, migrate.ErrNoChange) - lines 51, 455, 518: err != migrate.ErrNilVersion -> !errors.Is(err, migrate.ErrNilVersion) - line 376: err == migrate.ErrNilVersion -> errors.Is(err, migrate.ErrNilVersion) errors is already imported in both files. nil comparisons (err != nil) and non-error-sentinel comparisons are left unchanged. Verified with golangci-lint v2.10.1: zero errorlint issues in the changed files, no new issues introduced, .golangci.yml unchanged, no errcheck suppression. go build ./... and go test ./... pass (the one pre-existing Azure scheduler mock failure is unrelated).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
The previous errorlint conversion landed on three lines whose inline `if err := m.Op(); ...` declarations shadow an outer `err` (govet enable-all flags this as shadow). Rename each inner variable so the diff stays clean: - RunMigrations (line 45): m.Up() -> upErr - RollbackMigrations (line 450): m.Steps(-steps) -> stepErr - MigrateToVersion (line 485): m.Migrate(version) -> migErr Renaming (rather than reusing the outer err) avoids an ineffassign, since the outer err is overwritten immediately afterward by m.Version(). Verified with golangci-lint v2.10.1: zero new govet shadow issues and zero errorlint issues vs origin/main; .golangci.yml unchanged; no errcheck suppression. go build ./... and go test ./internal/database/... pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…touched files Make every .go file in this PR's diff fully golangci-lint v2.10.1 clean (all linters), so the change introduces zero new lint findings. No .golangci.yml changes; handled in code, with justified inline nolint only for genuinely-unfixable cases. Code fixes (no nolint): - godot: added terminating periods to flagged comments (auto-fixed). - misspell: British -> US spelling in prose comments and user-facing messages (behaviour->behavior, defence->defense, cancelled->canceled in ctx/error-message prose, etc.). - gocritic: equalFold (strings.ToLower == -> strings.EqualFold), builtinShadow (max -> maxDepth), sloppyReassign, exitAfterDefer (rekey main runs cancel before log.Fatalf), importShadow (renamed local accounts/mock/provider vars), paramTypeCombine (merged same-type params), unnamedResult (named multi-returns). - govet: shadow (renamed inner err in if-init statements), fieldalignment (reordered struct fields via the fieldalignment fixer). - gosec G115: added explicit [0, MaxInt32] bounds guards before the int32 / int narrowing conversions in connection.go and migrate.go. - revive: reordered gcpTokenExchangeAttempt results so error is last. - staticcheck: removed a dead freshness read (SA4006); switched the GCP loader off the deprecated CredentialsFromJSON. - unused: removed dead planIntersectsAllowed and triggerColdStartCollect. - unconvert: dropped a redundant int() on syscall.Stdin. - noctx: exec.Command -> exec.CommandContext in the configure-* CLI flows. Justified nolint (genuinely unfixable / cross-cutting): - misspell: DB status value 'cancelled' (status CHECK constraint) and column cancelled_by (migration 000035) inside SQL strings / status literals; documented and tracked for rename in PR #1277. - gosec: G505 HMAC-SHA1 is the RFC 6238 TOTP default; G117 fields must carry credential secrets (not logged); G204/G702/G703 are local CLI subprocess/file reads with hardcoded or validated arguments; G705 the Lambda->HTTP adapter relays an already-escaped handler response. - gocritic: hugeParam and tooManyResults on interface-bound / range-fed / constructor signatures where a pointer/struct rewrite is broad aliasing-prone churn for a marginal copy saving; rangeValCopy on read-only loops over large elements. - revive: established exported names (SchedulerConfig, MockSESClient, MockSNSClient) and the package name "api". - unparam: router-handler contract error returns, the buildMigrateDSN sslmode override seam, and executePurchase's documented wasMultiAccount signal. Verified with golangci-lint v2.10.1: 0 issues across all touched files, --new-from-rev=origin/main reports 0 new issues, .golangci.yml is identical to origin/main, no errcheck suppression. go build ./... passes; go test ./... passes (the one pre-existing Azure scheduler mock failure is unrelated).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Eliminate the gocritic/unparam/revive/staticcheck //nolint suppressions
that were genuinely fixable, leaving only the gosec security exceptions,
the DB-schema 'cancelled' misspell exceptions, and a small set of
interface/SDK-contract judgment cases.
Code-fixed (nolint removed):
- gocritic rangeValCopy (15): converted every read-only range-over-large-
element loop to an index loop (for i := range xs { x := &xs[i] / xs[i] }),
preserving value-copy semantics where the body mutates or appends.
- gocritic hugeParam (28): pointer-ized the value params and threaded the
change through all callers and tests. The account/rec fan-out paths in
purchase/execution.go and scheduler.go (incl. fanOutPerAccount's callback
type) are race-clean (verified with go test -race). NewHandler and
NewScheduler now take *Config; ListStoredRecommendations' standalone
helper buildRecommendationFilter takes *RecommendationFilter.
- gocritic tooManyResults (1): collectAllProviders now returns a single
collectAllResult struct instead of six values.
- unparam (5): removed the always-"gcloud" program param from the GCP CLI
helpers; removed the always-"" sslModeOverride param from buildMigrateDSN;
dropped executePurchase's never-used wasMultiAccount result; dropped the
always-nil error returns from getPublicInfo/getDeploymentInfo (router
wrappers + tests updated).
Kept as documented exceptions:
- 18 gosec (each with a specific security justification).
- 16 misspell on the DB status value 'cancelled' / column cancelled_by
(status CHECK + migration 000035; rename tracked in #1277).
Verified with golangci-lint v2.10.1: 0 issues on the 33 PR-touched files,
--new-from-rev=origin/main reports 0 new issues, .golangci.yml is identical
to origin/main, no nolint:errcheck. go build ./... and go test ./... pass
(the one pre-existing Azure scheduler mock failure is unrelated).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/postgres/migrations/migrate.go (1)
530-561: 🩺 Stability & Availability | 🟠 MajorPreserve the configured
sslmodehere.ConnConfig.TLSConfigonly says TLS is enabled, soverify-caandverify-fullget flattened torequireand certificate/hostname verification is lost for migrations. Useconfig.ConnConfig.SSLModeinstead of inferring fromTLSConfig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/migrate.go` around lines 530 - 561, The DSN builder is flattening all TLS-enabled connections to require, which drops the configured Postgres sslmode behavior. Update buildMigrateDSN to use config.ConnConfig.SSLMode instead of inferring from ConnConfig.TLSConfig, so migrate preserves verify-ca and verify-full semantics. Keep the rest of the DSN assembly in migrate.go unchanged, including the existing host/user/password/database handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/configure_gcp.go`:
- Line 391: The failure-continuation prompt is creating a new bufio.Reader from
os.Stdin instead of reusing the caller-provided reader, which can drop buffered
scripted input and cause blocking/EOF. Update executeGCPCommand and its prompt
flow to accept and use the same injected reader that the earlier prompt already
consumed from, and thread that reader through the call site in
configureGCPCommand so both prompts share one input source.
In `@internal/api/handler_registrations.go`:
- Around line 240-244: The best-effort email notification in notifyRegistrant is
using context.Background(), which can block the approval/rejection flow
indefinitely if the notifier stalls. Update notifyRegistrant to create a
short-lived timeout context before calling
h.emailNotifier.SendRegistrationDecisionNotification, and pass that context into
the send so the notification cannot hang the request. Keep the existing
nil/empty checks and preserve the best-effort behavior by ignoring
timeout-related failures after the send attempt.
In `@internal/api/handler.go`:
- Around line 77-78: NewHandler currently dereferences cfg immediately, so it
will panic when called with nil and should preserve zero-value-friendly
behavior. Update NewHandler to normalize a nil HandlerConfig to an empty/default
config before reading CORSAllowedOrigin, and keep the rest of the handler setup
using that safe config path so NewHandler(nil) works without crashing.
---
Outside diff comments:
In `@internal/database/postgres/migrations/migrate.go`:
- Around line 530-561: The DSN builder is flattening all TLS-enabled connections
to require, which drops the configured Postgres sslmode behavior. Update
buildMigrateDSN to use config.ConnConfig.SSLMode instead of inferring from
ConnConfig.TLSConfig, so migrate preserves verify-ca and verify-full semantics.
Keep the rest of the DSN assembly in migrate.go unchanged, including the
existing host/user/password/database handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f93dd029-2435-4ae5-95ce-8a401c70b656
📒 Files selected for processing (32)
cmd/configure_gcp.gocmd/lambda/main_test.gointernal/api/coverage_extras_test.gointernal/api/handler.gointernal/api/handler_accounts.gointernal/api/handler_commitment_options_test.gointernal/api/handler_coverage_test.gointernal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/api/handler_federation_test.gointernal/api/handler_inventory.gointernal/api/handler_inventory_test.gointernal/api/handler_registrations.gointernal/api/handler_test.gointernal/api/router.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_savings_filter_test.gointernal/database/connection.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/migrations/migrate_security_test.gointernal/mocks/stores.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.gointernal/server/app.gointernal/server/app_test.gointernal/server/http_test.gointernal/server/lambda_coverage_test.gointernal/server/lambda_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/api/handler_commitment_options_test.go
- internal/api/handler_federation_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/database/connection.go
- internal/mocks/stores.go
…osec+misspell only Drive the residual non-security nolints to zero so the only suppressions left on #1265 are the 18 gosec security exceptions and the 16 DB-schema 'cancelled' misspell exceptions. CUDly is an application with no external consumers, so internal renames are safe (all in-repo callers updated). RecommendationFilter cascade (matches #1276 so the branches reconcile): - ListStoredRecommendations / ListRecommendations now take *config.RecommendationFilter across the StoreInterface, the server SchedulerInterface, the api recsLister/SchedulerInterface, the PostgresStore and Scheduler impls, the MockConfigStore/MockScheduler mocks, all test fakes, and ~40 call sites. parseRecommendationFilter / buildCoverageRecFilter return *RecommendationFilter; testify matchers use mock.Anything or &config.RecommendationFilter{...}, and the captured-filter assertions and MatchedBy closures take the pointer type. revive renames: - mocks.MockSESClient -> mocks.SESClient, mocks.MockSNSClient -> mocks.SNSClient. - scheduler.SchedulerConfig -> scheduler.Config (NewScheduler takes *Config). - package api -> package apihttp (the 8 external importers alias it back to api so their api.X references are unchanged). SDK-mirror pointer-ize: - Connection.BeginTx takes *pgx.TxOptions (nil = default opts), dereferencing at the pgx boundary; callers pass nil. - Handler.runOrgDiscovery takes *aws.Config, dereferencing at the accounts.DiscoverOrgAccounts / discoverOrgFn boundary. staticcheck SA1019: - loadStoredGCPTokenSource now detects the credential JSON's "type" field (service_account vs external_account) and calls the non-deprecated CredentialsFromJSONWithTypeAndParams with the matching google.CredentialsType, so both service-account keys and WIF configs load without the deprecated CredentialsFromJSON. Also dropped a stray gocritic appendAssign suppression in handler_history_test by building the slice on a fresh backing array. Verified with golangci-lint v2.10.1: --new-from-rev=origin/main reports 0 new issues; the only nolints in any changed file are gosec and misspell; .golangci.yml is identical to origin/main; no nolint:errcheck. go build ./... and go test ./... pass (the one pre-existing Azure scheduler mock failure is unrelated), and go test -race on the scheduler/purchase paths is clean.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/mocks/sns.go (1)
21-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail loudly on non-nil wrong return types.
The nil guard is useful for
(nil, error), but a non-nil value of the wrong type currently returnsnil, nilwhen no error is configured, masking broken mock setup.Proposed fix
val, ok := args.Get(0).(*sns.PublishOutput) if !ok { - return nil, args.Error(1) + panic("SNSClient.Publish mock return[0] must be *sns.PublishOutput") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mocks/sns.go` around lines 21 - 24, The Publish mock in sns.go currently hides misconfigured returns by falling through to nil, nil when the first return value is not a *sns.PublishOutput and no error is set. Update the Publish method to fail loudly on non-nil wrong return types by checking the first return from args.Get(0) before the nil guard and returning a clear error (or panicking if that matches the mock pattern) instead of silently succeeding; keep the existing nil,error behavior for valid nil returns.
♻️ Duplicate comments (1)
internal/api/handler_registrations.go (1)
240-244: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the best-effort notification call with a timeout.
notifyRegistrantstill usescontext.Background()for the synchronous email send, so a stalled notifier can hold the approval/rejection request indefinitely after the registration state has already changed. Wrap the send in a shortcontext.WithTimeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_registrations.go` around lines 240 - 244, `notifyRegistrant` currently sends the email with `context.Background()`, so a slow notifier can block the request indefinitely; update this method to create a short-lived `context.WithTimeout` for the `h.emailNotifier.SendRegistrationDecisionNotification` call, and make sure the timeout context is canceled with `defer`. Keep the behavior best-effort by preserving the existing nil/email guards and only changing the context used in `notifyRegistrant`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/interfaces.go`:
- Line 248: The recommendation store still assumes a non-nil filter and can
panic when ListStoredRecommendations calls buildRecommendationFilter with a nil
pointer. Update the ListStoredRecommendations path in
internal/config/store_postgres_recommendations.go to normalize a nil
RecommendationFilter to an empty one before building the query, and keep the
handling consistent with the ListStoredRecommendations interface contract and
buildRecommendationFilter helper.
---
Outside diff comments:
In `@internal/mocks/sns.go`:
- Around line 21-24: The Publish mock in sns.go currently hides misconfigured
returns by falling through to nil, nil when the first return value is not a
*sns.PublishOutput and no error is set. Update the Publish method to fail loudly
on non-nil wrong return types by checking the first return from args.Get(0)
before the nil guard and returning a clear error (or panicking if that matches
the mock pattern) instead of silently succeeding; keep the existing nil,error
behavior for valid nil returns.
---
Duplicate comments:
In `@internal/api/handler_registrations.go`:
- Around line 240-244: `notifyRegistrant` currently sends the email with
`context.Background()`, so a slow notifier can block the request indefinitely;
update this method to create a short-lived `context.WithTimeout` for the
`h.emailNotifier.SendRegistrationDecisionNotification` call, and make sure the
timeout context is canceled with `defer`. Keep the behavior best-effort by
preserving the existing nil/email guards and only changing the context used in
`notifyRegistrant`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35ff029f-f133-4c88-b304-d64ff33f0793
📒 Files selected for processing (121)
cmd/lambda/main_test.gocmd/rekey/main.gointernal/analytics/collector_test.gointernal/api/analytics_postgres.gointernal/api/analytics_postgres_test.gointernal/api/base64_password_guard_test.gointernal/api/build_suppressions_test.gointernal/api/coverage_extras_test.gointernal/api/coverage_gaps_test.gointernal/api/db_rate_limiter.gointernal/api/db_rate_limiter_integration_test.gointernal/api/db_rate_limiter_test.gointernal/api/exchange_helpers_test.gointernal/api/exchange_lookup.gointernal/api/exchange_lookup_test.gointernal/api/handler.gointernal/api/handler_accounts.gointernal/api/handler_accounts_external_id_test.gointernal/api/handler_accounts_router_test.gointernal/api/handler_accounts_test.gointernal/api/handler_analytics.gointernal/api/handler_analytics_test.gointernal/api/handler_apikeys.gointernal/api/handler_apikeys_test.gointernal/api/handler_auth.gointernal/api/handler_auth_test.gointernal/api/handler_commitment_options.gointernal/api/handler_commitment_options_test.gointernal/api/handler_config.gointernal/api/handler_config_test.gointernal/api/handler_coverage_test.gointernal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/api/handler_docs.gointernal/api/handler_docs_test.gointernal/api/handler_federation.gointernal/api/handler_federation_test.gointernal/api/handler_groups.gointernal/api/handler_groups_test.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_inventory.gointernal/api/handler_inventory_test.gointernal/api/handler_oidc.gointernal/api/handler_oidc_test.gointernal/api/handler_per_account_perms_test.gointernal/api/handler_plans.gointernal/api/handler_plans_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_guards_test.gointernal/api/handler_purchases_revoke.gointernal/api/handler_purchases_revoke_test.gointernal/api/handler_purchases_test.gointernal/api/handler_recommendations.gointernal/api/handler_recommendations_refresh.gointernal/api/handler_recommendations_test.gointernal/api/handler_registrations.gointernal/api/handler_registrations_autoenable_test.gointernal/api/handler_registrations_recipients_test.gointernal/api/handler_registrations_validate_test.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_integration_test.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_router.gointernal/api/handler_router_test.gointernal/api/handler_security_test.gointernal/api/handler_test.gointernal/api/handler_users.gointernal/api/handler_users_test.gointernal/api/handler_version.gointernal/api/handler_version_test.gointernal/api/health.gointernal/api/health_test.gointernal/api/inmemory_rate_limiter.gointernal/api/inmemory_rate_limiter_test.gointernal/api/middleware.gointernal/api/middleware_test.gointernal/api/mocks_test.gointernal/api/rate_limiter.gointernal/api/rate_limiter_test.gointernal/api/ri_utilization_cache.gointernal/api/ri_utilization_cache_integration_test.gointernal/api/ri_utilization_cache_test.gointernal/api/router.gointernal/api/router_660_permission_flips_test.gointernal/api/router_auth_test.gointernal/api/router_authuser_test.gointernal/api/router_handlers_test.gointernal/api/scoping.gointernal/api/types.gointernal/api/types_apikeys.gointernal/api/types_test.gointernal/api/validation.gointernal/api/validation_test.gointernal/config/interfaces.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_recommendations_test.gointernal/credentials/resolver.gointernal/credentials/resolver_extra_test.gointernal/database/connection.gointernal/database/coverage_extra_test.gointernal/email/coverage_test.gointernal/email/sender_test.gointernal/email/templates_test.gointernal/mocks/ses.gointernal/mocks/sns.gointernal/mocks/stores.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_suppressions_test.gointernal/scheduler/scheduler_test.gointernal/server/adapter_test.gointernal/server/app.gointernal/server/app_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/interfaces.gointernal/server/lambda_coverage_test.gointernal/server/lambda_test.gointernal/server/test_helpers_test.gointernal/testutil/mocks.go
✅ Files skipped from review due to trivial changes (18)
- internal/api/types_test.go
- internal/api/db_rate_limiter_integration_test.go
- internal/server/adapter_test.go
- internal/api/handler_purchases_guards_test.go
- internal/api/scoping.go
- internal/api/handler_accounts_external_id_test.go
- internal/api/handler_oidc.go
- internal/api/handler_ri_exchange.go
- internal/api/handler_version.go
- internal/analytics/collector_test.go
- internal/api/handler_ri_exchange_integration_test.go
- internal/api/ri_utilization_cache_test.go
- internal/api/router_auth_test.go
- internal/api/handler_auth_test.go
- internal/api/router_authuser_test.go
- internal/api/handler_oidc_test.go
- internal/api/handler_purchases_test.go
- internal/api/handler_router_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- internal/api/handler_coverage_test.go
- internal/server/http_test.go
- internal/server/lambda_test.go
- internal/api/handler_commitment_options_test.go
- cmd/lambda/main_test.go
- internal/api/handler_test.go
- internal/server/app.go
- internal/server/lambda_coverage_test.go
- internal/server/http.go
- cmd/rekey/main.go
- internal/server/app_test.go
- internal/api/handler_inventory.go
- internal/credentials/resolver.go
- internal/api/handler_recommendations_refresh.go
- internal/api/handler_dashboard_test.go
- internal/config/store_postgres_recommendations.go
- internal/api/handler_dashboard.go
- internal/mocks/stores.go
- Thread configure_gcp.go: pass the caller-supplied bufio.Reader into executeGCPCommand instead of creating a fresh bufio.NewReader(os.Stdin); avoids double-buffering that drops buffered scripted/piped input on the failure-continuation prompt. - Thread handler_registrations.go: wrap the best-effort email send in a 5-second timeout context so a stalled notifier cannot hold the approval/rejection request open after state has already changed. - Thread handler.go: guard NewHandler against a nil *HandlerConfig to prevent panic on NewHandler(nil). - Thread store_postgres_recommendations.go: guard buildRecommendationFilter against a nil *RecommendationFilter; nil is a valid call under the ListStoredRecommendations interface contract (means no filter).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Replace silent cfg=&HandlerConfig{} / cfg=&Config{} fallbacks in
NewHandler and NewScheduler with explicit panics that surface the
programming error at construction time (no-silent-fallback rule).
Normalize nil *RecommendationFilter at the top of
ListStoredRecommendations (single normalization site) instead of
inside buildRecommendationFilter, and document the nil-means-match-all
contract on StoreInterface, PurchaseManagerInterface, and the
exchange-lookup and server interface.
Extend handler_test.go and scheduler_test.go with PanicsWithValue
assertions for the nil-config path; add
TestPostgresStore_ListStoredRecommendations_NilFilter to guard the
nil-filter contract with a real Postgres round-trip.
Reuse the caller-supplied bufio.Reader in executeExplicitCommand
(configure_azure.go) to avoid double-buffering piped stdin; bump
notifyRegistrant timeout from 5s to 20s with its own background
context and a documenting comment.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…Failed The pre-existing TestScheduler_CollectAzureRecommendations_Success expected `require.NoError(t, err)` and was passing back when collectAzureRecommendations logged-and-swallowed per-account credential failures. After PR #1265 fan-out wiring landed errAllAccountsFailed (fail-loud when every enabled account fails so a partial / empty result isn't merged into the aggregate as success), the test breaks: 1 enabled account + 1 inevitable DefaultAzureCredential failure = FailedCount == len(accounts) -> error returned. The test name was misleading (it never actually exercised a successful Azure recommendation fetch — managed_identity always fails in test). Rename to _AllAccountsFail and assert the new contract: error returned, recs nil, message contains "all 1 accounts failed". Pins the regression the new fan-out contract is meant to prevent.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scheduler/scheduler_test.go (1)
1459-1498: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake this failure-path test deterministic.
This test currently passes only when ambient Azure auth is unavailable, so it can flip on CI or a developer machine that has valid Azure credentials. Please force the per-account failure through an injected stub/provider mock instead of relying on
DefaultAzureCredentialfailing in the host environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduler/scheduler_test.go` around lines 1459 - 1498, The failure-path test for collectAzureRecommendations is nondeterministic because it depends on DefaultAzureCredential being unavailable in the host environment. Update TestScheduler_CollectAzureRecommendations_AllAccountsFail to inject a stubbed Azure provider or client that always returns an error for the enabled account, and wire it through Scheduler/collectAzureRecommendations so the per-account failure is forced regardless of local credentials. Keep the assertions that all accounts fail, but remove the ambient-auth dependency by mocking the provider path used for azure recommendations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/scheduler/scheduler_test.go`:
- Around line 1459-1498: The failure-path test for collectAzureRecommendations
is nondeterministic because it depends on DefaultAzureCredential being
unavailable in the host environment. Update
TestScheduler_CollectAzureRecommendations_AllAccountsFail to inject a stubbed
Azure provider or client that always returns an error for the enabled account,
and wire it through Scheduler/collectAzureRecommendations so the per-account
failure is forced regardless of local credentials. Keep the assertions that all
accounts fail, but remove the ambient-auth dependency by mocking the provider
path used for azure recommendations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df2c0359-f901-4917-bf91-89a74de2e0da
📒 Files selected for processing (12)
cmd/configure_azure.gointernal/api/exchange_lookup.gointernal/api/handler.gointernal/api/handler_registrations.gointernal/api/handler_test.gointernal/api/types.gointernal/config/interfaces.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_recommendations_test.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.gointernal/server/interfaces.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/server/interfaces.go
- internal/config/interfaces.go
- internal/api/types.go
- internal/api/handler_test.go
- internal/config/store_postgres_recommendations.go
- internal/api/exchange_lookup.go
- internal/api/handler_registrations.go
- cmd/configure_azure.go
- internal/scheduler/scheduler.go
This was referenced Jun 26, 2026
Member
Author
|
Closing as superseded. Disposition from the open-PR reconciliation (2026-07-03): (1) the errcheck work here (107 findings) is fully on main via #1343 with an equivalent implementation; (2) the undocumented package api -> apihttp rename across 91 files was never in this PR's stated scope and has no recorded motivation - if a linter in the #1342 burn-down set genuinely requires it, it will resurface there with a concrete driver and should land as its own documented PR; (3) the ListStoredRecommendations pointer-receiver change is a gocritic hugeParam-class fix covered by #1342's remaining 446 gocritic findings. Nothing unique is lost. |
cristim
added a commit
that referenced
this pull request
Jul 3, 2026
Fix the ~45 golangci-lint v2 violations that #1276 deliberately skips because these files are also touched by other open PRs (#1265, #1299). internal/deploy/* is excluded here as #1246 deletes that package. Files fixed and linters addressed: - cmd/helpers_test.go: fieldalignment (govet), unparam - cmd/main_test.go: fieldalignment (govet), also fix positional struct literals broken by field reordering - cmd/multi_service_filters.go: hugeParam + rangeValCopy (gocritic), equalFold (gocritic), godot; all filter functions updated to *Config / *Recommendation params with callers updated across the cmd package - cmd/multi_service_engine_versions_test.go: fieldalignment (govet), godot - internal/auth/service_password_test.go: fieldalignment (govet), godot - internal/auth/store_postgres_test.go: fieldalignment (govet), godot - internal/purchase/approvals.go: err-shadow (govet), misspell (analogue->analog, cancelled->canceled, cancelling->canceling) - internal/purchase/messages.go: hugeParam + rangeValCopy (gocritic), godot, misspell (authorised->authorized) Incidental changes: caller sites in cmd/multi_service{,_helpers,_test, _filters_test}.go; handle*Message signature callers in internal/purchase/{coverage_extra,money_path_regression}_test.go; test assertions updated to match renamed error strings.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
Fix the ~45 golangci-lint v2 violations that #1276 deliberately skips because these files are also touched by other open PRs (#1265, #1299). internal/deploy/* is excluded here as #1246 deletes that package. Files fixed and linters addressed: - cmd/helpers_test.go: fieldalignment (govet), unparam - cmd/main_test.go: fieldalignment (govet), also fix positional struct literals broken by field reordering - cmd/multi_service_filters.go: hugeParam + rangeValCopy (gocritic), equalFold (gocritic), godot; all filter functions updated to *Config / *Recommendation params with callers updated across the cmd package - cmd/multi_service_engine_versions_test.go: fieldalignment (govet), godot - internal/auth/service_password_test.go: fieldalignment (govet), godot - internal/auth/store_postgres_test.go: fieldalignment (govet), godot - internal/purchase/approvals.go: err-shadow (govet), misspell (analogue->analog, cancelled->canceled, cancelling->canceling) - internal/purchase/messages.go: hugeParam + rangeValCopy (gocritic), godot, misspell (authorised->authorized) Incidental changes: caller sites in cmd/multi_service{,_helpers,_test, _filters_test}.go; handle*Message signature callers in internal/purchase/{coverage_extra,money_path_regression}_test.go; test assertions updated to match renamed error strings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
golangci-lint v2 activated
errcheck.check-blank: truewhich exposed 107 pre-existing blank-identifier error discards (_ = someCall()). This causedmainCI to fail on every lint run.Approach
The owner decided to properly handle each error, not flip
check-blankto false.Config changes (
.golangci.yml)Three path exclusions for generated/test-helper code where the pattern is unavoidable:
internal/mocks/- testify mockargs.Get(0).(*Type)type assertionsinternal/auth/test_helpers.gointernal/database/postgres/testhelpers/Production code changes
Each of the 16 remaining sites is handled appropriately:
internal/purchase/execution.go):SavePurchaseExecutionerrors are now logged asErrorf(AUDIT LOSS level); early-exit save extracted tosaveExecWithLoghelperinternal/scheduler/scheduler.go):g.Wait()andGetRecommendationserrors are now checked and loggedinternal/server/http.go):w.Writeerror loggedcmd/cleanup-lambda/main.go,cmd/rekey/main.go,internal/commitmentopts/store_postgres.go): proper rollback error handling witherrors.Is(pgx.ErrTxClosed)guardinternal/api/handler.go, various):buildResponseerrors propagated or logged;resolveAWSCallerIdentityannotated with//nolint:errcheck(best-effort, STS failure = empty AccountID = security gate)internal/auth/service_mfa.go,internal/auth/service_apikeys.go): update errors logged; singleflight.Doannotated with justificationinternal/credentials/cipher.go):DevKeypanics on invalid compile-time constant (programming error)cmd/configure_azure.go,cmd/configure_gcp.go):bufio.Reader.ReadStringerrors handled viareadLinehelper; subcommands extracted to keep gocyclo within gateComplexity helpers extracted (to keep gocyclo gate green)
readLine(reader)- bufio.ReadString + io.EOF handling (configure_azure.go)getGCPProjectID(reader)- read + validate GCP project ID (configure_gcp.go)createAzureServicePrincipal(reader, subscriptionID)- SP creation block (configure_azure.go)payloadTypeMatches(payload, want)- type-safe payload type check (internal/api/validation.go)logMigrateVersion(m, steps)- pre-rollback version logging (migrate.go)saveExecWithLog(ctx, exec, accountID)- best-effort save in early-exit path (execution.go)Test plan
go build ./...passesgolangci-lint run --enable errcheckreports 0 errcheck violationsgocyclo -over 10 $(git ls-files "*.go" | grep -v _test.go | grep -v vendor/)reports 0 violationsgo test ./...passesSummary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Tests