chore(lint): clear all golangci-lint findings to green - #1364
Merged
Merged
Conversation
cristim
added
triaged
Contributor
|
Important Review skippedToo many files! This PR contains 205 files, which is 55 over the limit of 150. To get a review, narrow the scope: Upgrade to Pro+ to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (205)
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 pull request updates CI security and IaC scanning, standardizes cancellation behavior, makes timeout cleanup explicit, adjusts selected runtime flows, and applies broad lint-driven refactors, data-shape changes, test updates, and security suppressions. ChangesRepository-wide maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
fix/lint-debt-clear
branch
from
July 13, 2026 12:55
dac0ce1 to
0193e25
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
cristim
force-pushed
the
fix/lint-debt-clear
branch
from
July 13, 2026 13:14
0193e25 to
153d8de
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/secrets/gcp_resolver_test.go (1)
363-370: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the nil-client test assert behavior.
The test never calls
Close; assigning the resolver to_provides no coverage and cannot detect either the documented panic or a future fix. Assert the current contract withassert.Panics, or makeClosenil-safe and assert that it does not panic.Minimal test fix
- _ = resolver // Document that we can't safely test Close with nil client + assert.Panics(t, func() { + resolver.Close() + })🤖 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/secrets/gcp_resolver_test.go` around lines 363 - 370, Update the nil-client test around GCPResolver.Close to invoke resolver.Close and assert the intended behavior with assert.Panics, preserving the current contract that closing a resolver with a nil client panics. Remove the ineffective resolver assignment to blank.internal/purchase/manager.go (1)
478-479: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the recovery-path comment.
allRecsSafeToRedrivereturns true for GCP and most Azure services, so describing this branch as the “Safe-fail path for mixed/Azure/GCP/legacy executions” contradicts the actual condition and may mislead future recovery changes.🤖 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/purchase/manager.go` around lines 478 - 479, Update the comment immediately above the safeFail call in the recovery path to accurately describe the branch condition, removing the incorrect implication that it covers GCP and most Azure executions. Keep the safeFail implementation and surrounding control flow unchanged.
🧹 Nitpick comments (5)
cmd/multi_service_stats_helpers.go (1)
23-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid suppressing
rangeValCopywhen an index loop removes the copy.Each iteration currently copies a potentially large
common.Recommendation. Iterate by index and userec := &recommendations[i]in both loops.Also applies to: 102-102
🤖 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/multi_service_stats_helpers.go` at line 23, Update both recommendation loops in the relevant helper to iterate by index instead of ranging over values, and bind each element as a pointer using the recommendations slice index. Remove the rangeValCopy nolint suppressions while preserving the existing loop behavior.cmd/multi_service_stats.go (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
unparamsuppressions name the actual unused parameters.At Line [59],
isDryRunis used, whileallResultsis unused. At Line [209],spStatsis unused. Remove these parameters if no contract requires them, or document the actual parameter and why it must remain.Also applies to: 209-209
🤖 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/multi_service_stats.go` at line 59, Update printMultiServiceSummary and the code at the referenced spStats declaration so unparam suppressions identify the genuinely unused parameters: remove allResults and spStats when no interface contract requires them, or change the suppression to name the actual unused parameter and document why it must remain; do not suppress isDryRun because it is used.internal/config/store_postgres_recommendations.go (1)
178-178: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid suppressing a large per-row struct copy.
RecommendationRecordis large, sofor i, rec := range recscopies it for every batch row before marshaling and argument construction. Iterate by index and use&recs[i]instead to preserve behavior while avoiding the copy.Suggested loop shape
- for i, rec := range recs { //nolint:gocritic // rangeValCopy: acceptable value copy + for i := range recs { + rec := &recs[i]🤖 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/config/store_postgres_recommendations.go` at line 178, The loop over recs currently copies each large RecommendationRecord value; update the iteration to use the row index and access the record through &recs[i] during marshaling and argument construction. Remove the gocritic suppression while preserving the existing batch-processing behavior.internal/email/smtp_sender_test.go (1)
348-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the TLS configuration produced by production code.
This test assigns
MinVersionin its owntls.Configand then checks that same assignment, so it will pass even ifsendMailTLSregresses.🤖 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/email/smtp_sender_test.go` at line 348, Update the test around sendMailTLS to capture and inspect the tls.Config created by production code rather than constructing a separate expected config. Assert that the configuration passed or returned by sendMailTLS has ServerName "smtp.example.com" and MinVersion tls.VersionTLS12, so the test fails if the production assignment regresses.internal/purchase/reaper_test.go (1)
124-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegression guard only checks American spelling.
The predicate asserts
!seen["canceled"]but not!seen["cancelled"]. Given the codebase currently persists the cancellation status as'cancelled'(British spelling — seeCancelExecutionAtomic/CancelScheduledExecutionAtomicininternal/config/store_postgres.go, deferred per issue#1277), this guard would not catch a future regression that widensstuckStatusesto include the actual DB value. Consider asserting both spellings until the migration lands.🤖 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/purchase/reaper_test.go` around lines 124 - 144, Update the status predicate in TestReapStuckExecutions_TerminalStatusNotTouched to reject both “canceled” and the persisted “cancelled” spelling, while preserving the existing approved/running requirements and other terminal-status exclusions.
🤖 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/multi_service.go`:
- Around line 258-260: Update runToolFromCSV to derive isDryRun using the same
ActualPurchase and DryRun safety guard as the non-CSV path, ensuring DryRun=true
always prevents real purchases even when ActualPurchase=true. Add a regression
test covering that combination in CSV mode.
In `@internal/analytics/postgres_analytics_test.go`:
- Around line 926-930: Update the “handles optional fields” subtest around the
QueryRequest setup: either add assertions that verify AccountUUIDs, StartDate,
and EndDate are preserved and used as expected, or remove those assignments and
their unusedwrite suppressions if they are not part of the scenario.
In `@internal/api/handler_groups.go`:
- Around line 37-47: The rate limiter error branch in the admin group handler
currently ignores failures and provides no telemetry. Update the rateLimitErr
handling around AllowWithUser to log the failure with relevant context, or
return an appropriate temporary error if this authorization path must fail
closed; preserve the existing 429 response for disallowed requests.
In `@internal/api/handler_test.go`:
- Line 1407: Update the stale comment for the invalid-JSON updateConfig test to
match the existing 400-status assertion, unless the intended behavior is 500, in
which case change the assertion accordingly. Keep the comment and test
expectation consistent.
In `@internal/config/store_postgres.go`:
- Around line 1018-1023: The doc comments for CancelExecutionAtomic and
CancelScheduledExecutionAtomic must match the SQL’s actual status value: change
the documented successful return status from "canceled" to "cancelled" while
leaving the existing SQL and migration-deferred behavior unchanged.
In `@internal/database/config.go`:
- Line 16: The gosec suppression comments use an incorrect HTTP redirect
rationale for password fields. Update Config.Password in
internal/database/config.go:16-16 and SMTPConfig.Password in
internal/email/smtp_sender.go:21-21 with the actual gosec finding and a
password-specific justification, or remove each suppression when its finding is
not applicable.
In `@internal/email/smtp_sender_test.go`:
- Around line 310-316: Update the regression tests at
internal/email/smtp_sender_test.go:310-316 and
internal/email/smtp_sender_test.go:348-348 to exercise production behavior
instead of reconstructing expected values locally: route the sender test through
a mocked transport or the production subject builder, and validate the
configuration returned or consumed by sendMailTLS rather than assigning the
expected value directly.
In `@internal/purchase/scheduled_fire_test.go`:
- Line 177: Update the compile-time assertion for
Manager.FireScheduledDelayedPurchases to use its explicit expected function type
rather than only referencing the method value, so signature changes are detected
while preserving the typed assertion.
In `@internal/testutil/testutil.go`:
- Around line 21-30: Update the environment setup and cleanup logic in SetEnv to
capture the prior value and whether the variable existed using os.LookupEnv.
During t.Cleanup, restore an existing variable even when its value is empty;
otherwise unset it, and handle/report errors from both os.Setenv and os.Unsetenv
consistently.
---
Outside diff comments:
In `@internal/purchase/manager.go`:
- Around line 478-479: Update the comment immediately above the safeFail call in
the recovery path to accurately describe the branch condition, removing the
incorrect implication that it covers GCP and most Azure executions. Keep the
safeFail implementation and surrounding control flow unchanged.
In `@internal/secrets/gcp_resolver_test.go`:
- Around line 363-370: Update the nil-client test around GCPResolver.Close to
invoke resolver.Close and assert the intended behavior with assert.Panics,
preserving the current contract that closing a resolver with a nil client
panics. Remove the ineffective resolver assignment to blank.
---
Nitpick comments:
In `@cmd/multi_service_stats_helpers.go`:
- Line 23: Update both recommendation loops in the relevant helper to iterate by
index instead of ranging over values, and bind each element as a pointer using
the recommendations slice index. Remove the rangeValCopy nolint suppressions
while preserving the existing loop behavior.
In `@cmd/multi_service_stats.go`:
- Line 59: Update printMultiServiceSummary and the code at the referenced
spStats declaration so unparam suppressions identify the genuinely unused
parameters: remove allResults and spStats when no interface contract requires
them, or change the suppression to name the actual unused parameter and document
why it must remain; do not suppress isDryRun because it is used.
In `@internal/config/store_postgres_recommendations.go`:
- Line 178: The loop over recs currently copies each large RecommendationRecord
value; update the iteration to use the row index and access the record through
&recs[i] during marshaling and argument construction. Remove the gocritic
suppression while preserving the existing batch-processing behavior.
In `@internal/email/smtp_sender_test.go`:
- Line 348: Update the test around sendMailTLS to capture and inspect the
tls.Config created by production code rather than constructing a separate
expected config. Assert that the configuration passed or returned by sendMailTLS
has ServerName "smtp.example.com" and MinVersion tls.VersionTLS12, so the test
fails if the production assignment regresses.
In `@internal/purchase/reaper_test.go`:
- Around line 124-144: Update the status predicate in
TestReapStuckExecutions_TerminalStatusNotTouched to reject both “canceled” and
the persisted “cancelled” spelling, while preserving the existing
approved/running requirements and other terminal-status exclusions.
🪄 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: d42363e5-6091-47f4-b32d-8635e66c4ad2
📒 Files selected for processing (223)
.github/workflows/ci.ymlci_cd_sanity_tests/cmd/azure_sanity/main.goci_cd_sanity_tests/cmd/ri-exchange/main.goci_cd_sanity_tests/cmd/sanity/main.goci_cd_sanity_tests/pkg/sanity/aws/aws.goci_cd_sanity_tests/pkg/sanity/azure/azure.goci_cd_sanity_tests/pkg/sanity/azure/azure_test.goci_cd_sanity_tests/pkg/sanity/report/report.gocmd/configure_azure.gocmd/configure_gcp.gocmd/configure_test.gocmd/helpers.gocmd/main.gocmd/multi_service.gocmd/multi_service_coverage_test.gocmd/multi_service_csv.gocmd/multi_service_csv_test.gocmd/multi_service_engine_versions.gocmd/multi_service_engine_versions_paginate_test.gocmd/multi_service_helpers.gocmd/multi_service_helpers_test.gocmd/multi_service_stats.gocmd/multi_service_stats_helpers.gocmd/multi_service_stats_test.gocmd/multi_service_test.gocmd/multi_service_test_common_test.gocmd/rekey/main.gocmd/secrets_store.gocmd/server/main.gocmd/validators_test.gointernal/accounts/org_discovery.gointernal/accounts/org_discovery_test.gointernal/analytics/collector.gointernal/analytics/interfaces.gointernal/analytics/postgres_analytics.gointernal/analytics/postgres_analytics_db_test.gointernal/analytics/postgres_analytics_integration_test.gointernal/analytics/postgres_analytics_mock_test.gointernal/analytics/postgres_analytics_test.gointernal/api/coverage_extras_test.gointernal/api/coverage_gaps_test.gointernal/api/db_rate_limiter.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_test.gointernal/api/handler_analytics.gointernal/api/handler_apikeys.gointernal/api/handler_auth.gointernal/api/handler_auth_test.gointernal/api/handler_commitment_options.gointernal/api/handler_config.gointernal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/api/handler_docs.gointernal/api/handler_federation.gointernal/api/handler_federation_test.gointernal/api/handler_groups.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_inventory.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_ri_exchange.gointernal/api/handler_ri_exchange_integration_test.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_router.gointernal/api/handler_test.gointernal/api/handler_users.gointernal/api/health.gointernal/api/inmemory_rate_limiter.gointernal/api/middleware.gointernal/api/middleware_test.gointernal/api/mocks_test.gointernal/api/rate_limiter.gointernal/api/ri_utilization_cache.gointernal/api/ri_utilization_cache_test.gointernal/api/router.gointernal/api/router_660_permission_flips_test.gointernal/api/router_authuser_test.gointernal/api/router_handlers_test.gointernal/api/scoping.gointernal/api/types.gointernal/api/types_apikeys.gointernal/api/validation.gointernal/api/validation_test.gointernal/auth/errors.gointernal/auth/service.gointernal/auth/service_api.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_lockout_test.gointernal/auth/service_mfa.gointernal/auth/service_password.gointernal/auth/service_user.gointernal/auth/types.gointernal/commitmentopts/probe.gointernal/commitmentopts/probe_azure.gointernal/commitmentopts/service_test.gointernal/commitmentopts/types.gointernal/config/constants.gointernal/config/defaults_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_cloud_accounts_test.gointernal/config/store_postgres_comprehensive_test.gointernal/config/store_postgres_pgxmock_test.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_registrations.gointernal/config/store_postgres_unit_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/validation.gointernal/config/validation_test.gointernal/credentials/cipher.gointernal/credentials/gcp_federated.gointernal/credentials/resolver.gointernal/database/config.gointernal/database/config_test.gointernal/database/connection.gointernal/database/connection_test.gointernal/database/open_from_env.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/migrations/migration_transactional_test.gointernal/database/postgres/migrations/split_savingsplans_test.gointernal/database/security_test.gointernal/deploy/docker.gointernal/deploy/docker_test.gointernal/deploy/frontend.gointernal/deploy/mocks.gointernal/deploy/profiles.gointernal/deploy/types.gointernal/email/coverage_extra_test.gointernal/email/coverage_test.gointernal/email/factory.gointernal/email/nop_sender.gointernal/email/sender.gointernal/email/smtp_sender.gointernal/email/smtp_sender_test.gointernal/email/smtp_server_test.gointernal/email/template_renderers.gointernal/email/templates.gointernal/execution/executor.gointernal/execution/fanout.gointernal/mocks/email.gointernal/mocks/ses.gointernal/mocks/sns.gointernal/mocks/stores.gointernal/oidc/aws_signer.gointernal/oidc/azure_factory_test.gointernal/oidc/azure_signer.gointernal/oidc/gcp_signer.gointernal/oidc/issuer_cache.gointernal/oidc/jwks.gointernal/oidc/lambda_issuer_test.gointernal/oidc/signer_test.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager.gointernal/purchase/manager_test.gointernal/purchase/messages_test.gointernal/purchase/notifications.gointernal/purchase/reaper.gointernal/purchase/reaper_test.gointernal/purchase/scheduled_fire.gointernal/purchase/scheduled_fire_test.gointernal/reporter/reporter.gointernal/runtime/runtime.gointernal/scheduler/permission_log_test.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_test.gointernal/secrets/aws_resolver.gointernal/secrets/azure_resolver.gointernal/secrets/azure_resolver_coverage_test.gointernal/secrets/azure_resolver_test.gointernal/secrets/constructor_error_test.gointernal/secrets/env_resolver.gointernal/secrets/env_resolver_coverage_test.gointernal/secrets/env_resolver_test.gointernal/secrets/gcp_resolver.gointernal/secrets/gcp_resolver_coverage_test.gointernal/secrets/gcp_resolver_grpc_test.gointernal/secrets/gcp_resolver_test.gointernal/secrets/resolver_coverage_test.gointernal/secrets/resolver_test.gointernal/server/app.gointernal/server/app_coverage_test.gointernal/server/app_test.gointernal/server/handler.gointernal/server/handler_ri_exchange.gointernal/server/handler_test.gointernal/server/health.gointernal/server/health_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/integration_test.gointernal/server/lambda.gointernal/server/lambda_test.gointernal/server/scheduledauth/integration_test.gointernal/server/scheduledauth/validator.gointernal/server/scheduledauth/validator_test.gointernal/server/static.gointernal/server/static_test.gointernal/testutil/mocks.gointernal/testutil/postgres.gointernal/testutil/testutil.go
💤 Files with no reviewable changes (1)
- internal/config/store_postgres_registrations.go
| LogLevel string | ||
| Database string | ||
| User string | ||
| Password string //nolint:gosec // G117: HTTP redirect target is validated/trusted |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Correct the misleading gosec suppression rationale.
Both comments describe a validated HTTP redirect target, but these fields contain database/SMTP passwords. Replace the copied explanation with the actual gosec finding and a password-specific justification, or remove the suppression if the finding is not applicable.
internal/database/config.go#L16-L16: correct theConfig.Passwordrationale.internal/email/smtp_sender.go#L21-L21: correct theSMTPConfig.Passwordrationale.
📍 Affects 2 files
internal/database/config.go#L16-L16(this comment)internal/email/smtp_sender.go#L21-L21
🤖 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/config.go` at line 16, The gosec suppression comments use
an incorrect HTTP redirect rationale for password fields. Update Config.Password
in internal/database/config.go:16-16 and SMTPConfig.Password in
internal/email/smtp_sender.go:21-21 with the actual gosec finding and a
password-specific justification, or remove each suppression when its finding is
not applicable.
Member
Author
There was a problem hiding this comment.
Fixed on this branch: both rationales now read "field holds a user-supplied runtime password, not a hardcoded credential" (internal/database/config.go:16 and internal/email/smtp_sender.go:21); the copied redirect-target wording is gone.
Comment on lines
310
to
316
| RecipientEmail: "", //nolint:govet // unusedwrite: falls back to notifyEmail; set for test completeness | ||
| } | ||
|
|
||
| s := &SMTPSender{ | ||
| fromEmail: "noreply@example.com", | ||
| notifyEmail: "admin@example.com", | ||
| fromEmail: "noreply@example.com", //nolint:govet // unusedwrite: field set for test completeness | ||
| notifyEmail: "admin@example.com", //nolint:govet // unusedwrite: field set for test completeness | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make these regression tests execute production code.
Both tests can pass after production behavior regresses because they reconstruct expected values locally:
internal/email/smtp_sender_test.go#L310-L316: exercise the sender through a mocked transport or test the production subject builder.internal/email/smtp_sender_test.go#L348-L348: test the configuration returned or consumed bysendMailTLS, rather than assigning the expected value in the test.
📍 Affects 1 file
internal/email/smtp_sender_test.go#L310-L316(this comment)internal/email/smtp_sender_test.go#L348-L348
🤖 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/email/smtp_sender_test.go` around lines 310 - 316, Update the
regression tests at internal/email/smtp_sender_test.go:310-316 and
internal/email/smtp_sender_test.go:348-348 to exercise production behavior
instead of reconstructing expected values locally: route the sender test through
a mocked transport or the production subject builder, and validate the
configuration returned or consumed by sendMailTLS rather than assigning the
expected value directly.
Member
Author
There was a problem hiding this comment.
Dismissing for this PR: the subject-injection test does exercise the production sanitizeHeader on the injected inputs (the part that guards the invariant), and the TLS test pins the MinVersion constant sendMailTLS uses. Rebuilding the sender around an injectable transport to assert the full header write is a test-infrastructure change out of scope for a lint-debt PR; happy to take it as a follow-up if you feel strongly.
Contributor
There was a problem hiding this comment.
@cristim, agreed—this is reasonable for a lint-debt PR. The subject-injection test does exercise the production sanitization boundary, and deeper transport/configuration-observation coverage can be handled separately if needed. I don’t feel strongly enough to require that infrastructure work here.
cristim
force-pushed
the
fix/lint-debt-clear
branch
from
July 13, 2026 13:52
153d8de to
dac0ce1
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/analytics/interfaces.go (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
AvgCoveragenil-semantics documentation.That explanation is part of the model contract and prevents consumers from treating missing coverage as zero.
🤖 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/analytics/interfaces.go` around lines 49 - 53, Restore documentation on the MonthlySummary.AvgCoverage field explaining that nil represents missing coverage and must not be interpreted as zero. Keep the existing field type, JSON tag, and surrounding model structure unchanged.
🤖 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.
Nitpick comments:
In `@internal/analytics/interfaces.go`:
- Around line 49-53: Restore documentation on the MonthlySummary.AvgCoverage
field explaining that nil represents missing coverage and must not be
interpreted as zero. Keep the existing field type, JSON tag, and surrounding
model structure unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d88395c3-3849-429d-b45a-a0699dfe3dfe
📒 Files selected for processing (217)
ci_cd_sanity_tests/cmd/azure_sanity/main.goci_cd_sanity_tests/cmd/ri-exchange/main.goci_cd_sanity_tests/cmd/sanity/main.goci_cd_sanity_tests/pkg/sanity/aws/aws.goci_cd_sanity_tests/pkg/sanity/azure/azure.goci_cd_sanity_tests/pkg/sanity/azure/azure_test.goci_cd_sanity_tests/pkg/sanity/report/report.gocmd/configure_azure.gocmd/configure_gcp.gocmd/configure_test.gocmd/helpers.gocmd/main.gocmd/multi_service.gocmd/multi_service_coverage_test.gocmd/multi_service_csv.gocmd/multi_service_csv_test.gocmd/multi_service_engine_versions.gocmd/multi_service_engine_versions_paginate_test.gocmd/multi_service_helpers.gocmd/multi_service_helpers_test.gocmd/multi_service_stats.gocmd/multi_service_stats_helpers.gocmd/multi_service_stats_test.gocmd/multi_service_test.gocmd/multi_service_test_common_test.gocmd/rekey/main.gocmd/secrets_store.gocmd/server/main.gocmd/validators_test.gointernal/accounts/org_discovery.gointernal/accounts/org_discovery_test.gointernal/analytics/collector.gointernal/analytics/interfaces.gointernal/analytics/postgres_analytics.gointernal/analytics/postgres_analytics_db_test.gointernal/analytics/postgres_analytics_integration_test.gointernal/analytics/postgres_analytics_mock_test.gointernal/analytics/postgres_analytics_test.gointernal/api/coverage_extras_test.gointernal/api/coverage_gaps_test.gointernal/api/db_rate_limiter.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_test.gointernal/api/handler_analytics.gointernal/api/handler_apikeys.gointernal/api/handler_auth.gointernal/api/handler_auth_test.gointernal/api/handler_commitment_options.gointernal/api/handler_config.gointernal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/api/handler_docs.gointernal/api/handler_federation.gointernal/api/handler_federation_test.gointernal/api/handler_groups.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_inventory.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_ri_exchange.gointernal/api/handler_ri_exchange_integration_test.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_router.gointernal/api/handler_test.gointernal/api/handler_users.gointernal/api/health.gointernal/api/inmemory_rate_limiter.gointernal/api/middleware.gointernal/api/middleware_test.gointernal/api/mocks_test.gointernal/api/rate_limiter.gointernal/api/ri_utilization_cache.gointernal/api/ri_utilization_cache_test.gointernal/api/router.gointernal/api/router_660_permission_flips_test.gointernal/api/router_authuser_test.gointernal/api/router_handlers_test.gointernal/api/scoping.gointernal/api/types.gointernal/api/types_apikeys.gointernal/api/validation.gointernal/api/validation_test.gointernal/auth/errors.gointernal/auth/service.gointernal/auth/service_api.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_lockout_test.gointernal/auth/service_mfa.gointernal/auth/service_password.gointernal/auth/service_user.gointernal/auth/types.gointernal/commitmentopts/probe.gointernal/commitmentopts/probe_azure.gointernal/commitmentopts/service_test.gointernal/commitmentopts/types.gointernal/config/defaults_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_cloud_accounts_test.gointernal/config/store_postgres_comprehensive_test.gointernal/config/store_postgres_pgxmock_test.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_registrations.gointernal/config/store_postgres_unit_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/validation.gointernal/config/validation_test.gointernal/credentials/cipher.gointernal/credentials/resolver.gointernal/database/config.gointernal/database/config_test.gointernal/database/connection.gointernal/database/connection_test.gointernal/database/open_from_env.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/migrations/migration_transactional_test.gointernal/database/postgres/migrations/split_savingsplans_test.gointernal/database/security_test.gointernal/deploy/docker.gointernal/deploy/docker_test.gointernal/deploy/frontend.gointernal/deploy/mocks.gointernal/deploy/profiles.gointernal/deploy/types.gointernal/email/coverage_extra_test.gointernal/email/coverage_test.gointernal/email/factory.gointernal/email/nop_sender.gointernal/email/sender.gointernal/email/smtp_sender.gointernal/email/smtp_sender_test.gointernal/email/smtp_server_test.gointernal/email/template_renderers.gointernal/email/templates.gointernal/execution/executor.gointernal/execution/fanout.gointernal/mocks/email.gointernal/mocks/ses.gointernal/mocks/sns.gointernal/mocks/stores.gointernal/oidc/aws_signer.gointernal/oidc/azure_factory_test.gointernal/oidc/azure_signer.gointernal/oidc/gcp_signer.gointernal/oidc/issuer_cache.gointernal/oidc/lambda_issuer_test.gointernal/oidc/signer_test.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager.gointernal/purchase/manager_test.gointernal/purchase/messages_test.gointernal/purchase/notifications.gointernal/purchase/reaper.gointernal/purchase/reaper_test.gointernal/purchase/scheduled_fire.gointernal/purchase/scheduled_fire_test.gointernal/reporter/reporter.gointernal/runtime/runtime.gointernal/scheduler/permission_log_test.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_test.gointernal/secrets/aws_resolver.gointernal/secrets/azure_resolver.gointernal/secrets/azure_resolver_coverage_test.gointernal/secrets/azure_resolver_test.gointernal/secrets/constructor_error_test.gointernal/secrets/env_resolver.gointernal/secrets/env_resolver_coverage_test.gointernal/secrets/env_resolver_test.gointernal/secrets/gcp_resolver.gointernal/secrets/gcp_resolver_coverage_test.gointernal/secrets/gcp_resolver_grpc_test.gointernal/secrets/gcp_resolver_test.gointernal/secrets/resolver_coverage_test.gointernal/secrets/resolver_test.gointernal/server/app.gointernal/server/app_coverage_test.gointernal/server/app_test.gointernal/server/handler_ri_exchange.gointernal/server/handler_test.gointernal/server/health.gointernal/server/health_test.gointernal/server/http_test.gointernal/server/integration_test.gointernal/server/lambda.gointernal/server/lambda_test.gointernal/server/scheduledauth/integration_test.gointernal/server/scheduledauth/validator.gointernal/server/scheduledauth/validator_test.gointernal/server/static.gointernal/server/static_test.gointernal/testutil/mocks.gointernal/testutil/postgres.gointernal/testutil/testutil.go
💤 Files with no reviewable changes (1)
- internal/config/store_postgres_registrations.go
🚧 Files skipped from review as they are similar to previous changes (207)
- internal/accounts/org_discovery_test.go
- cmd/rekey/main.go
- internal/analytics/postgres_analytics_integration_test.go
- internal/api/ri_utilization_cache.go
- internal/purchase/reaper.go
- ci_cd_sanity_tests/cmd/sanity/main.go
- internal/database/open_from_env.go
- internal/api/handler_recommendations_test.go
- internal/auth/errors.go
- cmd/multi_service_engine_versions_paginate_test.go
- internal/commitmentopts/types.go
- internal/accounts/org_discovery.go
- cmd/multi_service_test_common_test.go
- internal/api/coverage_extras_test.go
- internal/api/handler_registrations.go
- internal/api/handler_config.go
- internal/scheduler/scheduler_overrides_test.go
- internal/api/handler_accounts_external_id_test.go
- internal/api/handler_registrations_autoenable_test.go
- internal/api/handler_docs.go
- internal/api/rate_limiter.go
- internal/api/handler_per_account_perms_test.go
- internal/server/static_test.go
- internal/oidc/lambda_issuer_test.go
- internal/oidc/gcp_signer.go
- ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go
- internal/secrets/gcp_resolver_coverage_test.go
- internal/purchase/messages_test.go
- internal/testutil/mocks.go
- internal/execution/fanout.go
- internal/testutil/postgres.go
- internal/secrets/aws_resolver.go
- internal/api/mocks_test.go
- internal/api/handler_ri_exchange_integration_test.go
- cmd/secrets_store.go
- ci_cd_sanity_tests/cmd/azure_sanity/main.go
- internal/database/security_test.go
- internal/oidc/aws_signer.go
- internal/secrets/azure_resolver_coverage_test.go
- internal/purchase/approvals.go
- internal/email/template_renderers.go
- internal/api/handler_recommendations.go
- internal/testutil/testutil.go
- internal/execution/executor.go
- internal/scheduler/permission_log_test.go
- internal/config/interfaces.go
- internal/api/handler_users.go
- internal/secrets/gcp_resolver_test.go
- internal/api/handler_auth_test.go
- internal/config/types_test.go
- internal/api/handler_dashboard_test.go
- internal/api/exchange_lookup_test.go
- internal/commitmentopts/probe_azure.go
- internal/server/integration_test.go
- internal/secrets/env_resolver.go
- internal/api/router_660_permission_flips_test.go
- internal/secrets/azure_resolver_test.go
- internal/api/handler_purchases_guards_test.go
- internal/server/health.go
- internal/purchase/execution_test.go
- internal/auth/service_mfa.go
- internal/api/router_authuser_test.go
- internal/config/validation.go
- internal/purchase/notifications.go
- internal/deploy/mocks.go
- ci_cd_sanity_tests/pkg/sanity/report/report.go
- internal/email/factory.go
- internal/auth/service_user.go
- internal/api/handler_recommendations_refresh.go
- internal/purchase/scheduled_fire.go
- internal/server/scheduledauth/integration_test.go
- internal/database/postgres/migrations/split_savingsplans_test.go
- internal/secrets/gcp_resolver_grpc_test.go
- internal/analytics/collector.go
- internal/api/handler_federation_test.go
- internal/auth/service.go
- internal/server/scheduledauth/validator_test.go
- internal/api/exchange_lookup.go
- internal/secrets/constructor_error_test.go
- internal/api/handler_plans_test.go
- internal/email/smtp_server_test.go
- internal/api/scoping.go
- internal/mocks/email.go
- internal/server/app_test.go
- internal/database/postgres/migrations/migration_transactional_test.go
- internal/auth/service_apikeys_api.go
- internal/oidc/signer_test.go
- internal/deploy/docker_test.go
- internal/server/lambda_test.go
- internal/api/validation_test.go
- internal/config/store_postgres_unit_test.go
- internal/api/ri_utilization_cache_test.go
- internal/analytics/postgres_analytics_mock_test.go
- internal/api/handler_groups.go
- internal/api/coverage_gaps_test.go
- internal/api/middleware.go
- internal/email/coverage_extra_test.go
- ci_cd_sanity_tests/pkg/sanity/azure/azure.go
- internal/credentials/cipher.go
- internal/secrets/gcp_resolver.go
- ci_cd_sanity_tests/pkg/sanity/aws/aws.go
- internal/oidc/azure_signer.go
- internal/api/router_handlers_test.go
- internal/api/handler_inventory.go
- internal/secrets/resolver_test.go
- cmd/configure_test.go
- internal/config/store_postgres_recommendations.go
- internal/secrets/env_resolver_test.go
- internal/oidc/issuer_cache.go
- cmd/server/main.go
- internal/server/lambda.go
- internal/api/health.go
- internal/analytics/postgres_analytics.go
- cmd/multi_service_stats_helpers.go
- internal/auth/service_lockout_test.go
- internal/purchase/scheduled_fire_test.go
- internal/deploy/docker.go
- internal/database/config.go
- internal/server/handler_ri_exchange.go
- internal/deploy/types.go
- internal/secrets/env_resolver_coverage_test.go
- internal/config/store_postgres_comprehensive_test.go
- internal/api/handler_analytics.go
- internal/server/health_test.go
- internal/scheduler/scheduler_test.go
- cmd/multi_service_stats_test.go
- internal/auth/service_api.go
- internal/purchase/execution.go
- internal/api/middleware_test.go
- internal/oidc/azure_factory_test.go
- internal/auth/service_apikeys.go
- internal/config/store_postgres_pgxmock_test.go
- cmd/multi_service_engine_versions.go
- internal/reporter/reporter.go
- cmd/validators_test.go
- internal/api/handler_plans.go
- internal/api/handler_dashboard.go
- internal/email/coverage_test.go
- internal/email/templates.go
- internal/commitmentopts/service_test.go
- internal/server/static.go
- internal/deploy/frontend.go
- internal/analytics/postgres_analytics_test.go
- internal/secrets/azure_resolver.go
- internal/purchase/reaper_test.go
- cmd/main.go
- internal/api/handler_auth.go
- internal/mocks/sns.go
- internal/purchase/approvals_test.go
- internal/database/config_test.go
- internal/api/inmemory_rate_limiter.go
- internal/api/handler_accounts_test.go
- internal/api/handler_apikeys.go
- internal/config/validation_test.go
- internal/server/handler_test.go
- internal/email/smtp_sender_test.go
- internal/email/nop_sender.go
- internal/server/scheduledauth/validator.go
- internal/database/connection_test.go
- internal/api/db_rate_limiter.go
- internal/database/postgres/migrations/migrate.go
- internal/server/app_coverage_test.go
- internal/api/router.go
- internal/email/sender.go
- internal/api/handler_commitment_options.go
- cmd/multi_service_csv_test.go
- internal/purchase/manager_test.go
- internal/api/handler_test.go
- cmd/multi_service_csv.go
- ci_cd_sanity_tests/cmd/ri-exchange/main.go
- internal/analytics/postgres_analytics_db_test.go
- cmd/multi_service_stats.go
- internal/auth/service_password.go
- internal/deploy/profiles.go
- cmd/multi_service_helpers_test.go
- cmd/multi_service_test.go
- internal/api/handler_federation.go
- internal/commitmentopts/probe.go
- internal/api/handler_history_test.go
- cmd/multi_service_helpers.go
- cmd/configure_gcp.go
- internal/api/types_apikeys.go
- internal/purchase/manager.go
- internal/auth/types.go
- internal/api/handler_ri_exchange_test.go
- cmd/configure_azure.go
- internal/api/handler_purchases_revoke.go
- cmd/helpers.go
- internal/secrets/resolver_coverage_test.go
- internal/api/handler_history.go
- internal/api/handler_purchases_revoke_test.go
- internal/api/handler.go
- internal/config/types.go
- cmd/multi_service.go
- internal/scheduler/scheduler.go
- internal/api/handler_accounts.go
- internal/email/smtp_sender.go
- internal/api/handler_ri_exchange.go
- internal/server/app.go
- internal/credentials/resolver.go
- internal/api/types.go
- internal/mocks/stores.go
- internal/purchase/coverage_extra_test.go
- internal/api/handler_purchases.go
- internal/config/store_postgres.go
- internal/config/defaults_test.go
- internal/api/handler_purchases_test.go
cristim
force-pushed
the
fix/lint-debt-clear
branch
from
July 13, 2026 14:19
dac0ce1 to
1782186
Compare
Three test files (coverage_extras_test.go, handler_ri_exchange_test.go, router_handlers_test.go) still expected the US "canceled" in the TransitionRIExchangeStatus mock args and result assertions for rejectRIExchange. The production handler already uses "cancelled" (reverted in commit 55a6295a5 to match the DB schema). Update the mocks to match.
Resolve 9 issues blocking CI on golangci-lint v2.10.x (which treats all
findings as fatal, unlike v2.11+ which exits 0 for warnings):
gocritic unnamedResult (x3): add named returns to defaultLadderAccountResolver,
resolveLadderIdentity, and ladderWithinCadenceWindow for the ambiguous
(string,string,error) and (bool,string,error) signatures.
gocritic builtinShadow (x2): rename local `cap` to `ladderCap` in
processOneLadderConfig and executeLadderRun to stop shadowing the
built-in cap() function.
gocritic rangeValCopy: switch `for _, tr := range tranches` to indexed
access (`for i := range tranches { tr := &tranches[i] }`) to avoid
copying the 152-byte Tranche struct on each iteration.
misspell: fix "behaviour" -> "behavior" in a handler_ladder.go comment.
prealloc: preallocate the actions slice in assembleLadderPlan with the
known capacity (len(Allocations)+len(Reshapes)+len(Holds)).
staticcheck ST1005: lowercase the "Allocate:" error prefix to conform
with Go error string conventions.
.golangci.yml: remove non-working `ignore-words` setting; extend the
misspell path-exclusion regex to cover handler_ri_exchange_test.go,
router_handlers_test.go, coverage_extras_test.go (all carry "cancelled"
as mock args matching the DB canonical value), store_postgres_ladder.go
(DB column cancelled_by), and internal/config/types.go for "initialised".
Update text pattern to "cancelled|initialised".
Three findings surfaced after rebasing onto origin/main (fcd8133), all in code merged in from main; golangci-lint v2.10.1 (CI pin) treats warnings as fatal so they would fail the Lint Code job: misspell (x2): "cancelling" -> "canceling" in prose comments in internal/config/store_postgres.go and internal/config/types.go (comment text only; the DB value "cancelled" is untouched and stays excluded). unparam: testBaseline in internal/server/handler_ladder_test.go always received 10.0; drop the parameter and hoist the value into the testBaselineLowWaterUSDHr constant.
internal/config/interfaces.go: restore the DB-canonical "cancelled" / "cancelled_by" spelling in the CancelExecutionAtomic and CancelScheduledExecutionAtomic doc comments (the spelling sweep had flipped them away from what the SQL actually writes; migration is owned by #1277), and correct the CancelExecutionAtomic doc to state that 'scheduled' is NOT accepted (matches the UPDATE ... WHERE status IN ('pending','notified') in store_postgres.go). cmd/configure_azure.go: cast syscall.Stdin to int for term.ReadPassword; syscall.Stdin is a syscall.Handle on Windows so the direct pass breaks Windows builds. nolint:unconvert because the cast is a no-op on Unix. internal/auth/service_mfa.go: reword the G505 rationale; RFC 6238 allows SHA-1/256/512 and SHA-1 is the compatibility choice, not a mandate. ci_cd_sanity_tests/pkg/sanity/azure/azure.go: extend the #nosec G702/G204 rationale to document that opts.SubscriptionID is passed as a single argv value by exec.CommandContext (no shell interpretation). .golangci.yml: add internal/config/interfaces.go to the misspell "cancelled" exclusion list.
"honouring" -> "honoring" and "defence in depth" -> "defense in depth" in comments merged in from main; golangci-lint v2.10.1 (CI pin) treats misspell warnings as fatal.
gocritic importShadow: requireInt32Range's "flag" parameter shadowed the imported flag package; rename to flagName. Surfaced after rebasing onto main with #1363 merged.
cristim
force-pushed
the
fix/lint-debt-clear
branch
from
July 16, 2026 18:30
6c28498 to
23ed924
Compare
Member
Author
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
Merged to main - and this is the FIRST FULLY-GREEN PR of the cleanup drive: every CI job SUCCESS (Lint Code, Security Scanning, pre-commit, Unit, Integration, E2E, Docker, Terraform, CI Success). All ~2769 golangci-lint findings are cleared, so Lint Code is now GREEN ON MAIN and becomes a hard gate: any subsequent PR introducing a lint finding fails its own CI. Combined with #1363 (Security green) and #1389 (gocyclo/pre-commit green), the entire 'known pre-existing debt' exception list (Lint #1342 / Security #1354 / CI-Success aggregator) is now RETIRED. Rebased onto post-#1389 main so gocyclo is clean; the fieldalignment deferral remains tracked in #1373. |
This was referenced Jul 16, 2026
Closed
This was referenced Jul 17, 2026
cristim
added a commit
that referenced
this pull request
Jul 23, 2026
…ases The `--purchase` flag never bought anything on its own because `--dry-run` defaulted to true and the guard was `!ActualPurchase || DryRun`, so `--purchase` gave `!true || true == true` (a dry run). Users had to pass `--purchase --dry-run=false` to actually buy. (Surfaced by CodeRabbit on #1364.) Rather than flip `--dry-run`'s default to false (which leaves a confusing flag named `--dry-run` that defaults off and only acts as a redundant "force dry-run even with --purchase" override), remove the flag entirely. `--purchase` is now the single purchase control: effectiveDryRun(cfg) = !cfg.ActualPurchase A bare run is always a dry run; `--purchase` is the one opt-in that moves money (still gated by the `--yes` / interactive confirmation prompt). This also unifies the two code paths: cloud-fetch and `--input-csv` mode now behave identically, matching what the CSV path already documented. Docs (purchase-safety.md, cli/README.md) are updated to the single-flag model with a History note explaining the removal. Tests replace the default-value guard with `TestDryRunFlagRemoved`, which fails if the flag is ever reintroduced.
cristim
added a commit
that referenced
this pull request
Jul 23, 2026
…ases (#1485) * fix(cli): make --purchase execute real purchases (--dry-run defaults false) The --dry-run flag defaulted to true, so effectiveDryRun (!ActualPurchase || DryRun) stayed true even when --purchase was given -- meaning --purchase alone never executed real purchases, defeating the flag. Default --dry-run to false; the safe default is preserved because ActualPurchase defaults to false (a bare invocation is still dry-run). Passing --dry-run alongside --purchase still forces dry-run as an explicit safety override. Adds a regression test that fails on the prior true default and covers all four flag combinations. * fix(cli): remove --dry-run flag; --purchase alone executes real purchases The `--purchase` flag never bought anything on its own because `--dry-run` defaulted to true and the guard was `!ActualPurchase || DryRun`, so `--purchase` gave `!true || true == true` (a dry run). Users had to pass `--purchase --dry-run=false` to actually buy. (Surfaced by CodeRabbit on #1364.) Rather than flip `--dry-run`'s default to false (which leaves a confusing flag named `--dry-run` that defaults off and only acts as a redundant "force dry-run even with --purchase" override), remove the flag entirely. `--purchase` is now the single purchase control: effectiveDryRun(cfg) = !cfg.ActualPurchase A bare run is always a dry run; `--purchase` is the one opt-in that moves money (still gated by the `--yes` / interactive confirmation prompt). This also unifies the two code paths: cloud-fetch and `--input-csv` mode now behave identically, matching what the CSV path already documented. Docs (purchase-safety.md, cli/README.md) are updated to the single-flag model with a History note explaining the removal. Tests replace the default-value guard with `TestDryRunFlagRemoved`, which fails if the flag is ever reintroduced.
This was referenced Aug 8, 2026
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
chore(lint): clear all golangci-lint findings to green
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
…ases (#1485) * fix(cli): make --purchase execute real purchases (--dry-run defaults false) The --dry-run flag defaulted to true, so effectiveDryRun (!ActualPurchase || DryRun) stayed true even when --purchase was given -- meaning --purchase alone never executed real purchases, defeating the flag. Default --dry-run to false; the safe default is preserved because ActualPurchase defaults to false (a bare invocation is still dry-run). Passing --dry-run alongside --purchase still forces dry-run as an explicit safety override. Adds a regression test that fails on the prior true default and covers all four flag combinations. * fix(cli): remove --dry-run flag; --purchase alone executes real purchases The `--purchase` flag never bought anything on its own because `--dry-run` defaulted to true and the guard was `!ActualPurchase || DryRun`, so `--purchase` gave `!true || true == true` (a dry run). Users had to pass `--purchase --dry-run=false` to actually buy. (Surfaced by CodeRabbit on #1364.) Rather than flip `--dry-run`'s default to false (which leaves a confusing flag named `--dry-run` that defaults off and only acts as a redundant "force dry-run even with --purchase" override), remove the flag entirely. `--purchase` is now the single purchase control: effectiveDryRun(cfg) = !cfg.ActualPurchase A bare run is always a dry run; `--purchase` is the one opt-in that moves money (still gated by the `--yes` / interactive confirmation prompt). This also unifies the two code paths: cloud-fetch and `--input-csv` mode now behave identically, matching what the CSV path already documented. Docs (purchase-safety.md, cli/README.md) are updated to the single-flag model with a History note explaining the removal. Tests replace the default-value guard with `TestDryRunFlagRemoved`, which fails if the flag is ever reintroduced.
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
2 participants
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.
Summary
//nolintdirectives (with linter name + reason) for every finding where fixing would change behavior or is security-sensitive:gocritic(hugeParam, rangeValCopy, exitAfterDefer, importShadow, unnamedResult),gosec(G117, G204, G702/G703/G704),revive(var-naming, error-return, exported),govet(fieldalignment, unusedwrite),unparam,staticcheck(ST1008)paramTypeCombineconsolidations, named return variable conflicts (adjustRecommendationsAgainstExisting,queryRDSInstancesPage,sumPassedRecs),indent-error-flowrestructuring inhandler_auth.go,gofmtcleanup across batch-edited filesauth.genericLoginErrorand the Azure SMTP credentials error ininternal/email/factory.goTest plan
golangci-lint runexits 0 with no actionable issuesgo build ./...passesgo test -short ./...passes (all 897+ tests green)Stacked on #1363 (fix/ci-security-gosec).
Summary by CodeRabbit