Repository navigation
fix(ci): repair Integration Tests, Lint, and Security Scanning on main - #1299
Merged
Merged
Conversation
cristim
added
priority/p1
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (215)
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:
📝 WalkthroughWalkthroughThis PR combines error-handling hardening, API and helper refactors, expanded test coverage, pinned toolchain updates, and repository-wide documentation and spelling normalization. ChangesFunctional hardening and APIs
Repository maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/server/scheduledauth/validator_test.go (1)
419-428: 🩺 Stability & Availability | 🔵 TrivialMake the post-rotation check retry-based
This still hinges on goroutine scheduling: the extra RSA work only shrinks the race window, soValidate(tokB)can still flake under load. A short retry/eventual assertion would remove the timing dependency.🤖 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/server/scheduledauth/validator_test.go` around lines 419 - 428, The post-rotation assertion in the scheduled auth validator test still depends on goroutine timing because signing tokB only narrows the race window. Update the Validate(tokB) check in validator_test.go to use a short retry/eventual assertion instead of a single immediate call, so the test waits for the inflight cleanup and key-set refresh to complete before verifying the rotated key behavior.
🤖 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/server/scheduledauth/validator_test.go`:
- Around line 419-428: The post-rotation assertion in the scheduled auth
validator test still depends on goroutine timing because signing tokB only
narrows the race window. Update the Validate(tokB) check in validator_test.go to
use a short retry/eventual assertion instead of a single immediate call, so the
test waits for the inflight cleanup and key-set refresh to complete before
verifying the rotated key behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5569de4e-85cb-45cf-b612-279951f8e127
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/workflows/ci.ymlinternal/config/store_postgres_recommendations_test.gointernal/config/store_postgres_test.gointernal/server/scheduledauth/validator_test.go
Member
Author
|
@coderabbitai review |
Contributor
Action performedReview triggered.
|
5 tasks done
Contributor
Action performedReview triggered.
|
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
internal/config/store_postgres_savings_filter_test.go (1)
173-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the filter path, not just the underlying numbers.
This test never calls the recommendation filter itself. It would still pass if
MinSavingsUSDandMinSavingsPctwere accidentally wired to the same field, because it only checks$100 > 30and20% < 30. Please run the record through the actual filter entry point with one USD threshold and one percentage threshold, and assert opposite outcomes.🤖 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_savings_filter_test.go` around lines 173 - 194, The current unit test only checks raw values from recEffectiveSavingsPct and rec.Savings, so it does not verify the real filter behavior. Update TestRecommendationFilter_UnitDistinction to exercise the actual recommendation filter entry point used by RecommendationRecord, with one case configured for MinSavingsUSD and another for MinSavingsPct, and assert that the same record passes one filter and fails the other. Use the existing symbols recEffectiveSavingsPct, RecommendationRecord, and the filter method/config path to ensure MinSavingsUSD and MinSavingsPct are validated as distinct fields.internal/analytics/postgres_analytics_test.go (1)
1179-1215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test never exercises the
rows.Err()path.The mock rows here are successful, and the assertions expect success, so a regression in the post-iteration error handling would still pass. Inject a terminal row error and assert
QuerySavingsfails.Proposed fix
- rows := pgxmock.NewRows([]string{ + rows := pgxmock.NewRows([]string{ "id", "account_id", "cloud_account_id", "timestamp", "provider", "service", "region", "commitment_type", "total_commitment", "total_usage", "total_savings", "coverage_percentage", "metadata", }).AddRow( "snapshot-1", "account-123", strPtr("cloud-1"), now, "aws", "rds", "us-east-1", "RI", 100.0, f64ptr(80.0), 20.0, f64ptr(80.0), metadataJSON, - ) + ).RowError(0, errors.New("rows error")) @@ - snapshots, err := store.QuerySavings(context.Background(), req) - require.NoError(t, err) - assert.Len(t, snapshots, 1) + _, err = store.QuerySavings(context.Background(), req) + require.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet())🤖 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/postgres_analytics_test.go` around lines 1179 - 1215, The TestRowsErr case currently only validates the happy path and never triggers the rows.Err() branch, so update the pgxmock rows setup in TestRowsErr to inject a terminal error after iteration and assert QuerySavings returns an error; keep the existing QuerySavings and rows.Err() handling as the target, and make the test fail on success so the post-iteration error path is actually covered.internal/secrets/env_resolver_coverage_test.go (1)
295-320: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the concurrent reads succeed.
The goroutines currently discard both result and error, so intermittent failures still make this test pass. Capture the outputs and assert every call returns
testValuewith no error.Proposed fix
- done := make(chan bool, 10) + errCh := make(chan error, 10) for i := 0; i < 10; i++ { go func() { - _, _ = resolver.GetSecret(ctx, testKey) - done <- true + got, err := resolver.GetSecret(ctx, testKey) + if err == nil && got != testValue { + err = fmt.Errorf("unexpected value %q", got) + } + errCh <- err }() } - // Wait for all goroutines for i := 0; i < 10; i++ { - <-done + require.NoError(t, <-errCh) }🤖 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/env_resolver_coverage_test.go` around lines 295 - 320, The concurrent access test currently ignores the results from EnvResolver.GetSecret, so failures can slip through unnoticed. Update TestEnvResolver_ConcurrentAccess to capture both the returned secret and error from each goroutine and assert that every call returns testValue with no error. Use the existing resolver.GetSecret, testKey, and testValue identifiers to keep the fix localized to this test.internal/deploy/profiles.go (2)
179-197: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize
c.Profilesbefore the first insert.
A zero-valueDeploymentConfigstill panics on the firstAddProfilecall whenProfilesis nil; add a nil-map init at the top of this method.🤖 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/deploy/profiles.go` around lines 179 - 197, Add a nil-map initialization at the start of DeploymentConfig.AddProfile so a zero-value DeploymentConfig can accept the first profile without panicking. Check c.Profiles before the exists lookup and assignment, initialize it if nil, then proceed with the current duplicate check and ActiveProfile logic.
147-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn a value here, not a pointer to a map copy
GetActiveProfile/GetProfilehand out pointers to detached structs, so any edits won't be written back toc.Profiles. ReturnProfileConfigby value, or document these as read-only accessors if mutation isn't intended.🤖 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/deploy/profiles.go` around lines 147 - 159, `GetActiveProfile` currently returns a pointer to a copied `ProfileConfig`, so callers cannot mutate `c.Profiles` through it; update the accessor to return the profile by value like `GetProfile`, or make both methods explicitly read-only if mutation is not intended. Use the existing `DeploymentConfig`, `GetActiveProfile`, and `GetProfile` symbols to keep the return behavior consistent and avoid handing out pointers to detached map copies.internal/config/types.go (1)
247-274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFinish the status rename in the Postgres cancel paths.
PurchaseExecutionnow documents/serializes the American spelling, butinternal/config/store_postgres.go:938-1040still updates both cancel flows withstatus = 'cancelled'. DB-backed cancel requests will keep persisting/returning the old value while the contract and tests here expect"canceled".🤖 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/types.go` around lines 247 - 274, The cancel-flow status rename is incomplete: `PurchaseExecution` now uses the American spelling, but the Postgres cancel paths still persist `cancelled`. Update the cancel status handling in `internal/config/store_postgres.go` for both cancel flows so they write and return `canceled`, and make sure any related query/filter logic in the same paths matches the new `PurchaseExecution.Status` contract.internal/purchase/approvals.go (1)
264-275: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
loadCancelableExecutionstill letsscheduledrows through.This guard now says only pending/notified are cancelable, but it still calls
execution.IsCancelable(), which also returns true for"scheduled". This path always proceeds toCancelExecutionAtomic, whose UPDATE only matches pending/notified, so scheduled email-link cancels fall into a fake CAS-race error instead of the dedicated scheduled-revoke flow.Proposed fix
- if !execution.IsCancelable() { + if execution.Status != "pending" && execution.Status != "notified" { return nil, fmt.Errorf("execution cannot be canceled, current status: %s", execution.Status) }🤖 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/approvals.go` around lines 264 - 275, The loadCancelableExecution guard is too broad because execution.IsCancelable() still admits scheduled rows, so scheduled email-link cancels bypass the dedicated revoke path and later fail in CancelExecutionAtomic as a misleading CAS-race. Update the cancellation check in loadCancelableExecution to exclude scheduled executions explicitly and route them through the existing scheduled-revoke handling used by the email-link/session cancel flows, keeping the policy aligned with PurchaseExecution.IsCancelable and cancelPurchaseViaSession.internal/config/store_postgres.go (1)
1117-1125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
scheduled_execution_atto the planned-executions projection.
GetPlannedExecutionsstill feedsscanExecutionRows, which scansscheduled_execution_atafteridempotency_key, and the pgxmock coverage for this method expects that column too. This SELECT now stops atidempotency_key, so planned-execution reads will fail at scan time.Proposed fix
query := ` SELECT plan_id, execution_id, status, step_number, scheduled_date, notification_sent, approval_token, recommendations, total_upfront_cost, estimated_savings, completed_at, error, expires_at, cloud_account_id, source, approved_by, canceled_by, capacity_percent, created_by_user_id, retry_execution_id, retry_attempt_n, approval_token_expires_at, executed_by_user_id, executed_at, pre_approval_skip_reason, - idempotency_key + idempotency_key, scheduled_execution_at FROM purchase_executions WHERE status = ANY($1) ORDER BY scheduled_date ASC NULLS LAST, id ASC🤖 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.go` around lines 1117 - 1125, `GetPlannedExecutions` is missing the `scheduled_execution_at` column from its SELECT projection, so the row shape no longer matches `scanExecutionRows`. Update the planned-executions query in `GetPlannedExecutions` to include `scheduled_execution_at` after `idempotency_key` so it stays aligned with the scanner and the pgxmock expectation.
🧹 Nitpick comments (3)
internal/secrets/gcp_resolver_test.go (1)
107-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one wrapped-EOF case for this new
errors.Isbranch.
MockSecretIterator.Nextstill terminates with the rawiterator.Done, so this change is not exercised any differently than the old equality check. A single case that returns a wrapped done error would lock in the behavior this refactor is aiming for.🤖 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` at line 107, Add a test case that returns a wrapped iterator.Done value so the new errors.Is branch in the secret iterator handling is actually exercised. Update the relevant test around MockSecretIterator.Next in gcp_resolver_test.go to cover a wrapped done/EOF-style error path, ensuring the resolver still treats it as completion instead of only relying on the raw iterator.Done case.internal/auth/service_api_test.go (1)
699-734: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the explicit empty-slice
AllowedAccountscase.This test now locks
niland non-empty serialization, but[]string{}can still diverge at the JSON boundary. The matchingGroupIDstest covers that distinction, so it would be worth asserting"allowed_accounts":[]here too.Suggested test addition
func TestGroupToAPIGroup_EmptyAllowedAccounts(t *testing.T) { now := time.Now() t.Run("nil AllowedAccounts serializes as []", func(t *testing.T) { // ... }) + + t.Run("empty-slice AllowedAccounts serializes as []", func(t *testing.T) { + g := &Group{ + ID: "group-empty", + Name: "EmptySlice", + AllowedAccounts: []string{}, + Permissions: []Permission{}, + CreatedAt: now, + UpdatedAt: now, + } + api := groupToAPIGroup(g) + require.NotNil(t, api) + assert.Equal(t, []string{}, api.AllowedAccounts) + b, err := json.Marshal(api) + require.NoError(t, err) + assert.Contains(t, string(b), `"allowed_accounts":[]`) + assert.NotContains(t, string(b), `"allowed_accounts":null`) + }) t.Run("non-empty AllowedAccounts preserved", func(t *testing.T) { // ... }) }🤖 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/auth/service_api_test.go` around lines 699 - 734, The new groupToAPIGroup test only covers nil and non-empty AllowedAccounts, so add an explicit empty-slice case for Group.AllowedAccounts to verify it stays an empty array through serialization. Update TestGroupToAPIGroup_EmptyAllowedAccounts to assert []string{} is preserved and json.Marshal on the API object produces "allowed_accounts":[] rather than null, matching the existing GroupIDs coverage.internal/mocks/email.go (1)
79-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert against the production sender contract.
EmailSenderAPIomits methods thatinternal/email/interfaces.gorequires, so Line 93 will still pass ifMockEmailSenderdrifts from the realemail.SenderInterface. Prefer asserting the mock againstemail.SenderInterfacedirectly, or rename this interface so the reduced surface is explicit.🤖 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/email.go` around lines 79 - 93, The mock contract is too narrow and can drift from the real sender API, so update the assertion on MockEmailSender to match the production email sender interface used by internal/email/interfaces.go. Either change the compile-time check to assert against email.SenderInterface directly, or rename/adjust EmailSenderAPI so it is clearly the intended reduced contract and includes every method required by the production sender interface.
🤖 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/api/handler_purchases_revoke_test.go`:
- Around line 584-591: The revokePurchase expectations now use the American
spelling, but the production path still returns the old DB value for scheduled
revokes. Update CancelScheduledExecutionAtomic in the store_postgres
implementation so the SQL status literal and returned value use "canceled"
instead of "cancelled", and make sure Handler.revokePurchase continues to pass
through that value consistently. Also check the related revoke tests around
revokePurchase and scheduled revoke cases so their assertions match the renamed
status across the mocked and DB-backed paths.
In `@internal/api/handler_purchases.go`:
- Around line 380-381: In the cancel flow in handler_purchases.go, only map
config.ErrExecutionNotInExpectedStatus to a 409 conflict from the execution
cancel path. Update the logic around the execution cancel check so
non-expected-status errors are returned as internal failures instead of
NewClientError(409, ...), and avoid exposing the raw store error in the
client-facing message; keep the handling localized to the cancel/execution
transition branch.
In `@internal/config/store_postgres_mock_test.go`:
- Around line 285-286: The pgxmock branch in the mock store is returning a new
string error for a missing purchase plan instead of the same not-found contract
used by PostgresStore.GetPurchasePlan. Update the missing-row handling in the
mock to return the shared ErrNotFound sentinel (the same error type the real
store wraps) so errors.Is works consistently across both implementations. Use
the GetPurchasePlan-related mock path and ErrNotFound as the key symbols to
locate the fix.
In `@internal/config/store_postgres.go`:
- Around line 958-963: The atomic cancel paths in PostgresStore are still
persisting the legacy status spelling, so update the UPDATE statements in
CancelExecutionAtomic and the other matching cancel method to write status =
'canceled' instead of 'cancelled'. Make sure any related status normalization or
checks in those methods still align with the new persisted value so runtime
behavior and stored rows stay consistent.
In `@internal/secrets/gcp_resolver_coverage_test.go`:
- Around line 153-160: The canceled-context check in gcp_resolver_coverage_test
should verify the actual cancellation path instead of any generic failure. In
the GetSecret test that uses cancelledCtx, update the assertion to match
context.Canceled (or the resolver’s specific canceled status) so the coverage in
resolver.GetSecret actually proves canceled-context handling rather than auth,
transport, or missing-secret errors.
---
Outside diff comments:
In `@internal/analytics/postgres_analytics_test.go`:
- Around line 1179-1215: The TestRowsErr case currently only validates the happy
path and never triggers the rows.Err() branch, so update the pgxmock rows setup
in TestRowsErr to inject a terminal error after iteration and assert
QuerySavings returns an error; keep the existing QuerySavings and rows.Err()
handling as the target, and make the test fail on success so the post-iteration
error path is actually covered.
In `@internal/config/store_postgres_savings_filter_test.go`:
- Around line 173-194: The current unit test only checks raw values from
recEffectiveSavingsPct and rec.Savings, so it does not verify the real filter
behavior. Update TestRecommendationFilter_UnitDistinction to exercise the actual
recommendation filter entry point used by RecommendationRecord, with one case
configured for MinSavingsUSD and another for MinSavingsPct, and assert that the
same record passes one filter and fails the other. Use the existing symbols
recEffectiveSavingsPct, RecommendationRecord, and the filter method/config path
to ensure MinSavingsUSD and MinSavingsPct are validated as distinct fields.
In `@internal/config/store_postgres.go`:
- Around line 1117-1125: `GetPlannedExecutions` is missing the
`scheduled_execution_at` column from its SELECT projection, so the row shape no
longer matches `scanExecutionRows`. Update the planned-executions query in
`GetPlannedExecutions` to include `scheduled_execution_at` after
`idempotency_key` so it stays aligned with the scanner and the pgxmock
expectation.
In `@internal/config/types.go`:
- Around line 247-274: The cancel-flow status rename is incomplete:
`PurchaseExecution` now uses the American spelling, but the Postgres cancel
paths still persist `cancelled`. Update the cancel status handling in
`internal/config/store_postgres.go` for both cancel flows so they write and
return `canceled`, and make sure any related query/filter logic in the same
paths matches the new `PurchaseExecution.Status` contract.
In `@internal/deploy/profiles.go`:
- Around line 179-197: Add a nil-map initialization at the start of
DeploymentConfig.AddProfile so a zero-value DeploymentConfig can accept the
first profile without panicking. Check c.Profiles before the exists lookup and
assignment, initialize it if nil, then proceed with the current duplicate check
and ActiveProfile logic.
- Around line 147-159: `GetActiveProfile` currently returns a pointer to a
copied `ProfileConfig`, so callers cannot mutate `c.Profiles` through it; update
the accessor to return the profile by value like `GetProfile`, or make both
methods explicitly read-only if mutation is not intended. Use the existing
`DeploymentConfig`, `GetActiveProfile`, and `GetProfile` symbols to keep the
return behavior consistent and avoid handing out pointers to detached map
copies.
In `@internal/purchase/approvals.go`:
- Around line 264-275: The loadCancelableExecution guard is too broad because
execution.IsCancelable() still admits scheduled rows, so scheduled email-link
cancels bypass the dedicated revoke path and later fail in CancelExecutionAtomic
as a misleading CAS-race. Update the cancellation check in
loadCancelableExecution to exclude scheduled executions explicitly and route
them through the existing scheduled-revoke handling used by the
email-link/session cancel flows, keeping the policy aligned with
PurchaseExecution.IsCancelable and cancelPurchaseViaSession.
In `@internal/secrets/env_resolver_coverage_test.go`:
- Around line 295-320: The concurrent access test currently ignores the results
from EnvResolver.GetSecret, so failures can slip through unnoticed. Update
TestEnvResolver_ConcurrentAccess to capture both the returned secret and error
from each goroutine and assert that every call returns testValue with no error.
Use the existing resolver.GetSecret, testKey, and testValue identifiers to keep
the fix localized to this test.
---
Nitpick comments:
In `@internal/auth/service_api_test.go`:
- Around line 699-734: The new groupToAPIGroup test only covers nil and
non-empty AllowedAccounts, so add an explicit empty-slice case for
Group.AllowedAccounts to verify it stays an empty array through serialization.
Update TestGroupToAPIGroup_EmptyAllowedAccounts to assert []string{} is
preserved and json.Marshal on the API object produces "allowed_accounts":[]
rather than null, matching the existing GroupIDs coverage.
In `@internal/mocks/email.go`:
- Around line 79-93: The mock contract is too narrow and can drift from the real
sender API, so update the assertion on MockEmailSender to match the production
email sender interface used by internal/email/interfaces.go. Either change the
compile-time check to assert against email.SenderInterface directly, or
rename/adjust EmailSenderAPI so it is clearly the intended reduced contract and
includes every method required by the production sender interface.
In `@internal/secrets/gcp_resolver_test.go`:
- Line 107: Add a test case that returns a wrapped iterator.Done value so the
new errors.Is branch in the secret iterator handling is actually exercised.
Update the relevant test around MockSecretIterator.Next in gcp_resolver_test.go
to cover a wrapped done/EOF-style error path, ensuring the resolver still treats
it as completion instead of only relying on the raw iterator.Done case.
🪄 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: 9d265e46-884f-4515-acaa-3d286fac34a4
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sum
📒 Files selected for processing (236)
.golangci.ymlcmd/cleanup-lambda/main.gocmd/configure_azure.gocmd/configure_gcp.gocmd/configure_test.gocmd/helpers.gocmd/lambda/main.gocmd/lambda/main_test.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_test.gocmd/multi_service_filters.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/secrets_store.gocmd/server/main.gocmd/validators.gogo.modinternal/accounts/org_discovery_extra_test.gointernal/analytics/collector.gointernal/analytics/collector_test.gointernal/analytics/postgres_analytics_db_test.gointernal/analytics/postgres_analytics_mock_test.gointernal/analytics/postgres_analytics_test.gointernal/api/coverage_extras_test.gointernal/api/db_rate_limiter.gointernal/api/db_rate_limiter_integration_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_config.gointernal/api/handler_coverage_test.gointernal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/api/handler_federation.gointernal/api/handler_groups.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_inventory.gointernal/api/handler_inventory_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_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_recipients_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/health.gointernal/api/inmemory_rate_limiter.gointernal/api/middleware.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/auth/interfaces.gointernal/auth/service.gointernal/auth/service_api.gointernal/auth/service_api_test.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api.gointernal/auth/service_group.gointernal/auth/service_helpers.gointernal/auth/service_lockout_test.gointernal/auth/service_mfa.gointernal/auth/service_password.gointernal/auth/service_password_test.gointernal/auth/service_test.gointernal/auth/service_user.gointernal/auth/service_user_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/commitmentopts/probe_test.gointernal/commitmentopts/service.gointernal/config/constants.gointernal/config/defaults.gointernal/config/interfaces.gointernal/config/recommendation_overrides.gointernal/config/recommendation_overrides_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/store_postgres.gointernal/config/store_postgres_additional_test.gointernal/config/store_postgres_comprehensive_test.gointernal/config/store_postgres_db_test.gointernal/config/store_postgres_increment_step_test.gointernal/config/store_postgres_mock_test.gointernal/config/store_postgres_pgxmock_test.gointernal/config/store_postgres_recommendations.gointernal/config/store_postgres_recommendations_test.gointernal/config/store_postgres_registrations.gointernal/config/store_postgres_savings_filter_test.gointernal/config/store_postgres_test.gointernal/config/store_postgres_unit_test.gointernal/config/types.gointernal/config/types_test.gointernal/config/validation.gointernal/config/validation_test.gointernal/credentials/resolver.gointernal/credentials/resolver_test.gointernal/database/config.gointernal/database/connection.gointernal/database/connection_test.gointernal/database/coverage_extra_test.gointernal/database/postgres/migrations/000053_executions_account_fk_restrict_test.gointernal/database/postgres/migrations/000065_enforce_min_one_admin_test.gointernal/database/postgres/migrations/ensure_admin_user_test.gointernal/database/postgres/migrations/helpers_test.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/testhelpers/postgres.gointernal/database/security_test.gointernal/deploy/coverage_extra_test.gointernal/deploy/profiles.gointernal/email/coverage_extra_test.gointernal/email/coverage_test.gointernal/email/factory.gointernal/email/factory_test.gointernal/email/interfaces.gointernal/email/sender.gointernal/email/sender_test.gointernal/email/smtp_sender.gointernal/email/smtp_sender_test.gointernal/email/smtp_server_test.gointernal/email/template_renderers.gointernal/email/template_renderers_test.gointernal/email/templates.gointernal/email/templates_test.gointernal/execution/fanout.gointernal/execution/fanout_test.gointernal/mocks/email.gointernal/mocks/secretsmanager.gointernal/mocks/ses.gointernal/mocks/sns.gointernal/mocks/stores.gointernal/oidc/aws_signer_test.gointernal/oidc/factory.gointernal/oidc/lambda_issuer.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/finalize_revocations.gointernal/purchase/manager.gointernal/purchase/manager_test.gointernal/purchase/messages.gointernal/purchase/messages_test.gointernal/purchase/mocks_test.gointernal/purchase/money_path_regression_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/aws_resolver_coverage_test.gointernal/secrets/aws_resolver_test.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/gcp_resolver.gointernal/secrets/gcp_resolver_coverage_test.gointernal/secrets/gcp_resolver_test.gointernal/secrets/resolver.gointernal/secrets/resolver_coverage_test.gointernal/server/analytics_collect.gointernal/server/app.gointernal/server/app_test.gointernal/server/handler.gointernal/server/handler_coverage_test.gointernal/server/handler_ri_exchange_test.gointernal/server/handler_test.gointernal/server/health.gointernal/server/health_test.gointernal/server/http.gointernal/server/integration_test.gointernal/server/interfaces.gointernal/server/lambda.gointernal/server/lambda_test.gointernal/server/scheduledauth/config.gointernal/server/scheduledauth/validator.gointernal/server/test_helpers_test.gointernal/testutil/mocks.gointernal/testutil/postgres.gointernal/testutil/testutil.go
✅ Files skipped from review due to trivial changes (178)
- internal/auth/service_test.go
- internal/email/template_renderers.go
- cmd/multi_service_stats_test.go
- internal/api/db_rate_limiter_integration_test.go
- internal/config/store_postgres_increment_step_test.go
- internal/database/postgres/migrations/helpers_test.go
- internal/api/handler_users_test.go
- internal/api/handler_router_test.go
- internal/config/store_postgres_comprehensive_test.go
- internal/api/handler_registrations.go
- internal/api/handler_apikeys_test.go
- internal/email/factory_test.go
- internal/credentials/resolver_test.go
- internal/testutil/mocks.go
- internal/api/handler_registrations_recipients_test.go
- internal/api/ri_utilization_cache.go
- internal/api/handler_auth.go
- internal/config/resolver.go
- internal/api/handler_auth_test.go
- internal/config/validation_test.go
- internal/api/handler_accounts_external_id_test.go
- internal/auth/interfaces.go
- internal/config/store_postgres_additional_test.go
- internal/runtime/runtime.go
- internal/server/integration_test.go
- internal/config/recommendation_overrides_test.go
- internal/purchase/execution_test.go
- cmd/multi_service_test.go
- cmd/lambda/main.go
- cmd/multi_service_coverage_test.go
- cmd/lambda/main_test.go
- internal/oidc/factory.go
- internal/oidc/lambda_issuer.go
- internal/reporter/reporter.go
- internal/server/scheduledauth/config.go
- internal/scheduler/permission_log_test.go
- internal/server/interfaces.go
- internal/server/handler_ri_exchange_test.go
- internal/api/handler_groups.go
- internal/purchase/money_path_regression_test.go
- internal/config/defaults.go
- internal/api/router_authuser_test.go
- internal/auth/service_mfa.go
- cmd/validators.go
- internal/analytics/collector.go
- internal/database/connection_test.go
- internal/accounts/org_discovery_extra_test.go
- internal/config/resolver_test.go
- internal/server/test_helpers_test.go
- internal/api/handler_users.go
- internal/api/middleware.go
- internal/api/handler_per_account_perms_test.go
- internal/execution/fanout_test.go
- cmd/secrets_store.go
- internal/api/types_apikeys.go
- internal/database/security_test.go
- internal/api/handler_analytics.go
- internal/api/exchange_lookup.go
- internal/api/mocks_test.go
- internal/server/handler_test.go
- internal/api/handler_ri_exchange_integration_test.go
- internal/config/store_postgres_db_test.go
- internal/server/handler_coverage_test.go
- internal/config/recommendation_overrides.go
- internal/secrets/aws_resolver_test.go
- internal/scheduler/scheduler_overrides_test.go
- internal/auth/service_helpers.go
- internal/api/handler_accounts_test.go
- internal/credentials/resolver.go
- internal/api/ri_utilization_cache_test.go
- internal/commitmentopts/probe_test.go
- internal/purchase/finalize_revocations.go
- internal/api/handler_plans_test.go
- internal/execution/fanout.go
- internal/server/health.go
- internal/database/postgres/migrations/000053_executions_account_fk_restrict_test.go
- internal/mocks/sns.go
- internal/api/rate_limiter.go
- internal/server/app_test.go
- internal/secrets/azure_resolver_test.go
- internal/database/postgres/migrations/ensure_admin_user_test.go
- cmd/multi_service_helpers_test.go
- internal/api/handler_recommendations.go
- internal/secrets/env_resolver.go
- internal/secrets/resolver.go
- internal/analytics/postgres_analytics_db_test.go
- internal/testutil/postgres.go
- internal/auth/service_user_test.go
- internal/purchase/messages.go
- internal/api/handler_analytics_test.go
- internal/purchase/scheduled_fire.go
- internal/database/config.go
- cmd/server/main.go
- internal/api/scoping.go
- internal/email/factory.go
- internal/auth/service_password_test.go
- cmd/cleanup-lambda/main.go
- internal/api/handler_config.go
- internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go
- internal/secrets/aws_resolver.go
- internal/server/health_test.go
- internal/config/store_postgres_recommendations.go
- internal/secrets/gcp_resolver.go
- internal/api/handler_plans.go
- internal/api/handler_apikeys.go
- internal/auth/service_group.go
- internal/api/health.go
- internal/auth/service_lockout_test.go
- internal/api/handler_security_test.go
- internal/api/handler_recommendations_refresh.go
- internal/auth/service_apikeys.go
- internal/config/store_postgres_pgxmock_test.go
- internal/auth/store_postgres_test.go
- internal/email/smtp_sender_test.go
- internal/email/sender_test.go
- internal/api/handler_accounts_router_test.go
- internal/auth/service_apikeys_api.go
- internal/email/interfaces.go
- internal/testutil/testutil.go
- internal/auth/service_api.go
- internal/mocks/secretsmanager.go
- internal/purchase/reaper.go
- internal/api/handler_federation.go
- internal/email/smtp_sender.go
- internal/analytics/postgres_analytics_mock_test.go
- internal/api/handler_dashboard.go
- internal/api/validation.go
- cmd/multi_service_test_common_test.go
- internal/api/handler_inventory_test.go
- internal/purchase/execution.go
- internal/database/coverage_extra_test.go
- internal/deploy/coverage_extra_test.go
- cmd/multi_service_stats.go
- internal/mocks/ses.go
- internal/email/templates_test.go
- cmd/multi_service_stats_helpers.go
- internal/secrets/azure_resolver.go
- cmd/configure_test.go
- internal/api/handler_test.go
- internal/api/handler_recommendations_test.go
- internal/api/handler_coverage_test.go
- internal/auth/service_password.go
- internal/secrets/resolver_coverage_test.go
- internal/api/handler_accounts.go
- internal/api/db_rate_limiter.go
- internal/database/postgres/testhelpers/postgres.go
- internal/server/http.go
- internal/config/interfaces.go
- internal/auth/test_helpers.go
- cmd/configure_azure.go
- cmd/multi_service.go
- internal/config/constants.go
- internal/api/handler_inventory.go
- internal/api/handler_dashboard_test.go
- internal/scheduler/scheduler.go
- cmd/main.go
- internal/purchase/notifications.go
- internal/database/connection.go
- cmd/configure_gcp.go
- internal/email/template_renderers_test.go
- internal/api/exchange_lookup_test.go
- internal/purchase/mocks_test.go
- internal/api/handler.go
- internal/secrets/azure_resolver_coverage_test.go
- internal/api/router.go
- internal/auth/store_postgres.go
- internal/config/store_postgres_unit_test.go
- internal/server/handler.go
- cmd/helpers.go
- internal/auth/types.go
- internal/secrets/constructor_error_test.go
- internal/email/templates.go
- internal/purchase/approvals_test.go
- internal/server/app.go
- internal/email/sender.go
- internal/secrets/aws_resolver_coverage_test.go
- internal/scheduler/scheduler_test.go
- internal/api/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/config/store_postgres_test.go
- internal/config/store_postgres_recommendations_test.go
cristim
force-pushed
the
fix/main-ci-remaining-failures
branch
from
July 1, 2026 19:55
f0295b7 to
bfba73f
Compare
Member
Author
|
@coderabbitai full review |
Member
Author
|
Reconciliation note (2026-07-03): before this PR can proceed it needs rework - (1) the pgx bump is already on main via #1343 (hunk disappears on rebase); (2) DROP the blind status string-literal respellings ("cancelled" -> "canceled" in production Go comparisons/returns, ~27 files): they would break reads against existing DB rows and collide with #1277's expand-contract dual-spelling approach - #1277 must merge first, then these lines are its concern; (3) DROP the frontend/package-lock.json chunk (Babel bumps) - it conflicts with #1345's security lockfile and carries no advisories; (4) KEEP and rebase the godot/comment-misspell wave and the .golangci.yml node_modules exclusions - they are a solid contribution to the #1342 burn-down. Note the .golangci.yml edit will conflict with #1277's misspell exclusion line (different sections, trivial resolution). |
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.
cristim
force-pushed
the
fix/main-ci-remaining-failures
branch
from
July 4, 2026 00:19
bfba73f to
514a7af
Compare
Member
Author
Rework summary (rebased onto current main)This PR was rebased onto Drops applied during the rebase
Kept
CR fixes folded into the lint commit
Gates (diff class: godot/comment/misspell + lint-config + status-revert)
Self-review confirms the final |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
fix/main-ci-remaining-failures
branch
from
July 9, 2026 20:26
514a7af to
9497b33
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
- GetExecutionByID not-found: function returns (nil, nil) per contract; replace assert.Error + nil.Error() panic with require.NoError + assert.Nil - UpsertRecommendations FK violations: seed cloud_accounts rows before calling UpsertRecommendations with non-nil CloudAccountID to satisfy the recommendations.cloud_account_id FK constraint (migration 000030) - OIDC key-rotation race: sign tokB before the swap POST so the RSA operation (~1 ms, CPU-bound) gives the go-oidc cleanup goroutine time to set inflight=nil under the mutex; without this, fast loopback HTTP on Linux CI completes without a goroutine switch and the stale inflight causes Validate to return pre-rotation keys
…sion Apply the safe golangci-lint --fix subset (godot, misspell, gocritic, whitespace, unconvert, revive, gofmt, goimports; excluding govet field-alignment) across the module. This drops the golangci-lint issue count from 2718 to 1329 on the current main baseline; the remaining issues are tracked for follow-up batch-fix PRs (no only-new-issues masking). Also exclude frontend/node_modules from linting in .golangci.yml: the flatted npm package bundles a Go file that is not part of this project. Status-spelling note: this PR deliberately does NOT respell the purchase execution status "cancelled" -> "canceled" (comparisons, returned values, persisted SQL, doc comments quoting those values, and the cancelled_by column). That expand-contract rename is owned by #1277; touching it here would diverge from the persisted DB contract on main. Any spelling changes the auto-fixer produced in those spots were reverted. CR fixes applied in the same commit: - gcp_resolver_coverage_test.go: tighten the canceled-context assertion from bare assert.Error to require.Error plus a positive gRPC Canceled code check (the GCP SDK wraps context cancellation in a grpc status error, which errors.Is(context.Canceled) does not traverse). - store_postgres_mock_test.go: return fmt.Errorf("%w: purchase plan %s", ErrNotFound, planID) from the mock so errors.Is(err, ErrNotFound) matches the real store's not-found contract.
…ntract seedRecommendationCloudAccount already calls CreateCloudAccount; the extra CreateCloudAccount calls added in the previous commit caused cloud_accounts_pkey duplicate-key violations in both TestPostgresStore_UpsertRecommendations_AccountScopedEviction and TestPostgresStore_UpsertRecommendations_AmbientAndRegisteredCoexist. Also fix TestPostgresStore_PurchaseExecutions/Get_execution_by_ID_-_not_found: GetExecutionByID returns ErrNotFound (not nil) when no row matches; the test was incorrectly changed to require.NoError which contradicts the real implementation.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/server/app_test.go (1)
611-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the stale subtest description.
The subtest still says it returns a nil config store, but
initConfigStoreno longer returns one. Rename it to describe the values now validated:dbConfigandresolver.🤖 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/server/app_test.go` around lines 611 - 612, Rename the affected subtest description to state that it validates the returned dbConfig and resolver values, removing the stale reference to a nil config store. Locate the subtest surrounding initConfigStore and testutil.AssertNoError.
🤖 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/server/app_test.go`:
- Around line 611-612: Rename the affected subtest description to state that it
validates the returned dbConfig and resolver values, removing the stale
reference to a nil config store. Locate the subtest surrounding initConfigStore
and testutil.AssertNoError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e73d8042-a558-4384-bac4-530e95506a3b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.github/workflows/ci.yml.github/workflows/pre-commit.ymlgo.modinternal/api/handler_dashboard.gointernal/api/handler_history.gointernal/api/handler_purchases.gointernal/api/handler_recommendations_refresh.gointernal/api/handler_version.gointernal/api/handler_version_test.gointernal/api/health.gointernal/api/health_test.gointernal/api/ri_utilization_cache_test.gointernal/api/router.gointernal/api/router_660_permission_flips_test.gointernal/api/scoping.gointernal/auth/service_password.gointernal/auth/service_user.gointernal/config/recommendation_overrides_test.gointernal/config/store_postgres_additional_test.gointernal/config/store_postgres_recommendations_test.gointernal/config/store_postgres_test.gointernal/database/postgres/migrations/migrate.gointernal/database/postgres/migrations/migrate_security_test.gointernal/email/sender_test.gointernal/email/smtp_server_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/manager.gointernal/scheduler/scheduler_overrides_test.gointernal/server/app.gointernal/server/app_test.gointernal/server/handler.gointernal/server/handler_coverage_test.go
💤 Files with no reviewable changes (6)
- internal/api/scoping.go
- internal/api/handler_dashboard.go
- internal/api/handler_purchases.go
- internal/api/handler_history.go
- internal/api/handler_recommendations_refresh.go
- internal/email/sender_test.go
✅ Files skipped from review due to trivial changes (7)
- internal/config/recommendation_overrides_test.go
- internal/config/store_postgres_additional_test.go
- internal/api/router_660_permission_flips_test.go
- internal/config/store_postgres_test.go
- internal/scheduler/scheduler_overrides_test.go
- internal/config/store_postgres_recommendations_test.go
- internal/server/handler.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/api/ri_utilization_cache_test.go
- internal/email/smtp_server_test.go
- internal/api/health.go
- internal/purchase/manager.go
- internal/database/postgres/migrations/migrate.go
The govulncheck security bumps (Go 1.26.5, aws-sdk s3 v1.97.3, pgx v5.9.2) that this commit originally carried now land via origin/main (commit a840ed1 + existing pgx v5.9.2), so after rebasing they are no-ops here; GO-2026-5856, GO-2026-5764 and GO-2026-5004 are all covered on the rebased branch (govulncheck clean across all six modules). This commit's remaining, unique content is the golangci-lint cleanup: - Drop always-nil error / unused return / always-same param across getVersion, checkAuthService, applyUpdateUserRequest, buildMigrateDSN, Manager.executePurchase (unused wasMultiAccount bool), and initConfigStore (always-nil lazy store), updating callers and tests. - Keep getEnvFloat and the two scheduled-task handlers (handleCleanupExpiredRecords, handleRefreshAnalytics) with a documented //nolint:unparam: their signatures are intentionally fixed for symmetry with sibling helpers / the task-dispatch family contract. - Remove dead private code with no call site on main or this branch: canAccessAccountID, planIntersectsAllowed, expireIfStale, duplicatePurchaseResponse, triggerColdStartCollect, and unused test helpers (testSender/newTestSender, customStore/storeWithComplete). Per-account and per-plan scoping remains enforced inline in handler_accounts.go and via isPlanAllowedCached; no gate is removed. - Apply //nolint:unparam (or blank unused params) to the flagged test helpers whose argument is fixed by design. - Fix the malformed nolint on dummyPasswordHash: "// nolint:gosec -- ..." parsed the reason as linter names ("unknown linter"); use the correct "//nolint:gosec // ..." form.
cristim
force-pushed
the
fix/main-ci-remaining-failures
branch
from
July 10, 2026 13:42
71dc8d4 to
d6ae40e
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jul 16, 2026
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.
Summary
Fixes the three CI job failures that remained after #1220 (golangci-lint v1→v2 migration) merged to main.
only-new-issues: trueso only new violations in each diff are gated (pre-existing ones tracked as follow-up)--omit=dev(production deps have 0 HIGH/CRITICAL); also appliednpm audit fixto reduce fixable moderate vulnsIntegration test details
GetExecutionByIDnot-foundGetExecutionByIDreturns(nil, nil)by contract; test callederr.Error()on nilrequire.NoError + assert.NilUpsertRecommendations_AccountScopedEvictionrecommendations.cloud_account_idnot satisfied; nocloud_accountsrows for the test UUIDsstore.CreateCloudAccountbefore first upsertUpsertRecommendations_AmbientAndRegisteredCoexistTestValidate_OIDC_KeyRotation_RefreshOnUnknownKidinflight=nilunder mutex after signaling the channel; fast loopback on Linux completes the swap POST before the goroutine is scheduled, leaving stale inflighttokB(RSA, ~1 ms, CPU-bound) before the swap POST so the goroutine gets a scheduling windowTest plan
Summary by CodeRabbit
New Features
Bug Fixes
0).