fix(db): rename status value cancelled->canceled (expand-contract, US spelling) - #1277
Merged
Merged
Conversation
cristim
added
triaged
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 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 (12)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces canonical ChangesCancellation schema and migration
Persistence and cancellation flows
History and RI exchange handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PurchaseHandler
participant PurchaseExecutionStore
participant HistoryUI
Client->>PurchaseHandler: request cancellation
PurchaseHandler->>PurchaseExecutionStore: conditional status transition
PurchaseExecutionStore-->>PurchaseHandler: canceled status or conflict
PurchaseHandler-->>Client: canceled response
Client->>HistoryUI: load purchase history
HistoryUI->>HistoryUI: group canceled and cancelled statuses
Possibly related PRs
🚥 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.
|
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/store_postgres.go (1)
1117-1125:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winProject
scheduled_execution_atbefore scanning planned executions.Line 1125 stops the
GetPlannedExecutionsSELECT atidempotency_key, but Line 1438 scansscheduled_execution_atas the final destination. Real DB rows for this query will fail with a scan column-count mismatch.Proposed projection fix
- executed_by_user_id, executed_at, pre_approval_skip_reason, - idempotency_key + executed_by_user_id, executed_at, pre_approval_skip_reason, + idempotency_key, scheduled_execution_atAlso applies to: 1411-1438
🤖 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, The SELECT statement in the GetPlannedExecutions query is missing the scheduled_execution_at column. The query ends at idempotency_key on line 1125, but the rows.Scan() call at line 1438 attempts to scan scheduled_execution_at as the final field, causing a column-count mismatch. Add scheduled_execution_at to the SELECT clause in the GetPlannedExecutions query before the closing backtick to match the number of fields being scanned.
🧹 Nitpick comments (1)
internal/config/store_postgres_pgxmock_test.go (1)
546-575: ⚡ Quick winAssert
scheduled_execution_atin the projection guard.This mock row includes
scheduled_execution_at, but the expectation only matchesidempotency_key, so the production query can omit the final scan column while this test still passes. Make the matcher enforce both columns.Proposed matcher tightening
- mock.ExpectQuery("idempotency_key"). + mock.ExpectQuery(`idempotency_key,\s*scheduled_execution_at`).🤖 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_pgxmock_test.go` around lines 546 - 575, The mock expectation in TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns uses a simple regex matcher that only validates idempotency_key is projected, but the test row includes scheduled_execution_at as the final column. Strengthen the ExpectQuery regex pattern to enforce that both idempotency_key and scheduled_execution_at columns are included in the SELECT projection, preventing the production query from omitting the final scan column while the test incorrectly passes.
🤖 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_history.go`:
- Line 117: The historyExecutionStatuses list on line 117 needs to include both
"cancelled" and "canceled" spellings to maintain backward compatibility during
the migration period, as legacy instances may still write the old spelling to
the database. Similarly, the status comparison logic at lines 348-349 and
1025-1029 currently only branch on "canceled" and must be updated to also check
for "cancelled". Ensure the read paths accept both spellings while normalizing
all output to consistently use "canceled" in responses.
In `@internal/config/interfaces.go`:
- Around line 85-92: The comment documentation for the CancelExecutionAtomic
method incorrectly claims it handles the 'scheduled' status, but the actual
implementation only supports canceling executions in pending or notified states.
Remove the reference to 'scheduled' status from the CancelExecutionAtomic
comment and clarify that it only handles pending and notified statuses, since
scheduled execution cancellation is handled by the separate
CancelScheduledExecutionAtomic method. This will prevent callers from being
misled about which method to use for each status type.
In `@internal/config/store_postgres.go`:
- Line 1080: The store_postgres.go file has multiple SELECT queries that project
only the new column spelling canceled_by (at line 1080 and also at lines 1428
and 1524) but during the EXPAND phase, legacy code can still write to the old
column spelling cancelled_by. To maintain compatibility with late legacy writes,
modify the column projections to dual-read both the old cancelled_by and new
canceled_by columns using COALESCE to prefer the new spelling but fall back to
the legacy one. Additionally, ensure any cleanup or filtering predicates that
currently match only "canceled" are updated to also match "cancelled" to prevent
late legacy cancellations from being skipped.
In
`@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql`:
- Around line 22-27: The rollback migration adds CHECK constraints that restrict
status to 'cancelled' (lines 22-27 and 42-43) before converting existing
'canceled' rows back to 'cancelled' (lines 55-61). This causes the constraint to
fail if any 'canceled' rows exist. Reorder the migration by moving the UPDATE
statements that convert 'canceled' to 'cancelled' to execute before the ALTER
TABLE ADD CONSTRAINT statements for both the purchase_executions and
refund_executions tables.
- Around line 47-50: The down migration in
000078_rename_cancelled_to_canceled.down.sql is dropping the canceled_by column
without first restoring its values back to the cancelled_by column, causing
actor attribution data to be lost during rollback. Before the ALTER TABLE
purchase_executions DROP COLUMN IF EXISTS canceled_by statement, add an UPDATE
statement to backfill the cancelled_by column with values from canceled_by where
canceled_by is not null, ensuring data preservation during migration reversal.
In `@internal/purchase/approvals.go`:
- Around line 264-275: The IsCancelable() predicate currently allows scheduled
status to pass through, but the CancelExecutionAtomic function only accepts
pending and notified statuses in its WHERE clause, causing scheduled executions
to fail with a misleading 409 error. Align the two checks by either updating the
IsCancelable() predicate to explicitly reject scheduled status (to match the
documented pending/notified-only policy), or adding scheduled to the allowed
statuses in the CancelExecutionAtomic WHERE clause if that is the intended
product behavior. Ensure both the session cancel path and email-link cancel path
use the same consistent predicate.
---
Outside diff comments:
In `@internal/config/store_postgres.go`:
- Around line 1117-1125: The SELECT statement in the GetPlannedExecutions query
is missing the scheduled_execution_at column. The query ends at idempotency_key
on line 1125, but the rows.Scan() call at line 1438 attempts to scan
scheduled_execution_at as the final field, causing a column-count mismatch. Add
scheduled_execution_at to the SELECT clause in the GetPlannedExecutions query
before the closing backtick to match the number of fields being scanned.
---
Nitpick comments:
In `@internal/config/store_postgres_pgxmock_test.go`:
- Around line 546-575: The mock expectation in
TestPGXMock_GetPlannedExecutions_ProjectsAllScanColumns uses a simple regex
matcher that only validates idempotency_key is projected, but the test row
includes scheduled_execution_at as the final column. Strengthen the ExpectQuery
regex pattern to enforce that both idempotency_key and scheduled_execution_at
columns are included in the SELECT projection, preventing the production query
from omitting the final scan column while the test incorrectly passes.
🪄 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: c99dae24-1276-4570-a642-e3fc55590f93
📒 Files selected for processing (30)
internal/api/coverage_extras_test.gointernal/api/handler_dashboard.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_revoke.gointernal/api/handler_purchases_revoke_test.gointernal/api/handler_purchases_test.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_test.gointernal/api/router_660_permission_flips_test.gointernal/api/router_handlers_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/types_test.gointernal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sqlinternal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sqlinternal/mocks/stores.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/manager_test.gointernal/purchase/messages_test.gointernal/purchase/reaper_test.gointernal/purchase/scheduled_fire.gointernal/purchase/scheduled_fire_test.gointernal/server/test_helpers_test.go
cristim
added
severity/medium
cristim
added a commit
that referenced
this pull request
Jun 24, 2026
…touched files Make every .go file in this PR's diff fully golangci-lint v2.10.1 clean (all linters), so the change introduces zero new lint findings. No .golangci.yml changes; handled in code, with justified inline nolint only for genuinely-unfixable cases. Code fixes (no nolint): - godot: added terminating periods to flagged comments (auto-fixed). - misspell: British -> US spelling in prose comments and user-facing messages (behaviour->behavior, defence->defense, cancelled->canceled in ctx/error-message prose, etc.). - gocritic: equalFold (strings.ToLower == -> strings.EqualFold), builtinShadow (max -> maxDepth), sloppyReassign, exitAfterDefer (rekey main runs cancel before log.Fatalf), importShadow (renamed local accounts/mock/provider vars), paramTypeCombine (merged same-type params), unnamedResult (named multi-returns). - govet: shadow (renamed inner err in if-init statements), fieldalignment (reordered struct fields via the fieldalignment fixer). - gosec G115: added explicit [0, MaxInt32] bounds guards before the int32 / int narrowing conversions in connection.go and migrate.go. - revive: reordered gcpTokenExchangeAttempt results so error is last. - staticcheck: removed a dead freshness read (SA4006); switched the GCP loader off the deprecated CredentialsFromJSON. - unused: removed dead planIntersectsAllowed and triggerColdStartCollect. - unconvert: dropped a redundant int() on syscall.Stdin. - noctx: exec.Command -> exec.CommandContext in the configure-* CLI flows. Justified nolint (genuinely unfixable / cross-cutting): - misspell: DB status value 'cancelled' (status CHECK constraint) and column cancelled_by (migration 000035) inside SQL strings / status literals; documented and tracked for rename in PR #1277. - gosec: G505 HMAC-SHA1 is the RFC 6238 TOTP default; G117 fields must carry credential secrets (not logged); G204/G702/G703 are local CLI subprocess/file reads with hardcoded or validated arguments; G705 the Lambda->HTTP adapter relays an already-escaped handler response. - gocritic: hugeParam and tooManyResults on interface-bound / range-fed / constructor signatures where a pointer/struct rewrite is broad aliasing-prone churn for a marginal copy saving; rangeValCopy on read-only loops over large elements. - revive: established exported names (SchedulerConfig, MockSESClient, MockSNSClient) and the package name "api". - unparam: router-handler contract error returns, the buildMigrateDSN sslmode override seam, and executePurchase's documented wasMultiAccount signal. Verified with golangci-lint v2.10.1: 0 issues across all touched files, --new-from-rev=origin/main reports 0 new issues, .golangci.yml is identical to origin/main, no errcheck suppression. go build ./... passes; go test ./... passes (the one pre-existing Azure scheduler mock failure is unrelated).
cristim
added a commit
that referenced
this pull request
Jun 24, 2026
Eliminate the gocritic/unparam/revive/staticcheck //nolint suppressions
that were genuinely fixable, leaving only the gosec security exceptions,
the DB-schema 'cancelled' misspell exceptions, and a small set of
interface/SDK-contract judgment cases.
Code-fixed (nolint removed):
- gocritic rangeValCopy (15): converted every read-only range-over-large-
element loop to an index loop (for i := range xs { x := &xs[i] / xs[i] }),
preserving value-copy semantics where the body mutates or appends.
- gocritic hugeParam (28): pointer-ized the value params and threaded the
change through all callers and tests. The account/rec fan-out paths in
purchase/execution.go and scheduler.go (incl. fanOutPerAccount's callback
type) are race-clean (verified with go test -race). NewHandler and
NewScheduler now take *Config; ListStoredRecommendations' standalone
helper buildRecommendationFilter takes *RecommendationFilter.
- gocritic tooManyResults (1): collectAllProviders now returns a single
collectAllResult struct instead of six values.
- unparam (5): removed the always-"gcloud" program param from the GCP CLI
helpers; removed the always-"" sslModeOverride param from buildMigrateDSN;
dropped executePurchase's never-used wasMultiAccount result; dropped the
always-nil error returns from getPublicInfo/getDeploymentInfo (router
wrappers + tests updated).
Kept as documented exceptions:
- 18 gosec (each with a specific security justification).
- 16 misspell on the DB status value 'cancelled' / column cancelled_by
(status CHECK + migration 000035; rename tracked in #1277).
Verified with golangci-lint v2.10.1: 0 issues on the 33 PR-touched files,
--new-from-rev=origin/main reports 0 new issues, .golangci.yml is identical
to origin/main, no nolint:errcheck. go build ./... and go test ./... pass
(the one pre-existing Azure scheduler mock failure is unrelated).
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
internal/api/handler_history.go (1)
117-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep dual-read status compatibility during EXPAND.
History filtering/annotation/summary now accept only
"canceled". In EXPAND rollout, old instances can still write"cancelled", so these rows can be skipped or misbucketed until CONTRACT. Accept both spellings on read paths, while continuing to normalize outward text to"canceled".Suggested patch
-var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "canceled"} +var historyExecutionStatuses = []string{"pending", "notified", "scheduled", "approved", "running", "paused", "completed", "partially_completed", "failed", "expired", "cancelled", "canceled"} @@ - case "canceled": + case "cancelled", "canceled": annotateCancelled(row, exec, approver) @@ - case "canceled": + case "cancelled", "canceled": // A canceled purchase represents zero committed spend and zeroAlso applies to: 348-348, 1025-1028
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_history.go` at line 117, The history status read path currently only recognizes "canceled", which breaks EXPAND compatibility with older rows still written as "cancelled". Update the status handling in the history-related code paths (including historyExecutionStatuses and the filtering/annotation/summary logic around the referenced symbols) to accept both spellings on read, while keeping outward normalization and emitted text as "canceled". Ensure any matching, bucketing, or comparisons treat "cancelled" and "canceled" as equivalent until CONTRACT.internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql (2)
47-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore
cancelled_bybefore droppingcanceled_by.Any cancellations written by the new code keep attribution only in
canceled_by. Dropping the column here loses that data on rollback.Suggested fix
--- 3. Remove canceled_by column added by the up migration. +-- 3. Backfill canceled_by -> cancelled_by before dropping canceled_by. +UPDATE purchase_executions +SET cancelled_by = canceled_by +WHERE cancelled_by IS NULL + AND canceled_by IS NOT NULL; + +-- 4. Remove canceled_by column added by the up migration. ALTER TABLE purchase_executions DROP COLUMN IF EXISTS canceled_by;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql` around lines 47 - 50, The down migration currently drops canceled_by without restoring the older cancelled_by field, which would lose rollback attribution data. Update the migration around the purchase_executions ALTER TABLE step to add or restore cancelled_by from the existing canceled_by values before removing canceled_by, so the rollback preserves cancellation ownership. Use the migration’s purchase_executions block and the canceled_by/cancelled_by column names to locate the change.
22-27: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winBackfill
canceledrows before restoringcancelled-only constraints.If any
canceledrows exist—which is the normal post-up state—theADD CONSTRAINTstatements run before Lines 55-61 and the rollback aborts.Suggested fix
--- 1. Restore purchase_executions status CHECK to 'cancelled' only. +-- 1. Backfill rows first: 'canceled' -> 'cancelled'. +UPDATE purchase_executions +SET status = 'cancelled' +WHERE status = 'canceled'; + +UPDATE ri_exchange_history +SET status = 'cancelled' +WHERE status = 'canceled'; + +-- 2. Restore purchase_executions status CHECK to 'cancelled' only. @@ --- 2. Restore ri_exchange_history status CHECK to 'cancelled' only. +-- 3. Restore ri_exchange_history status CHECK to 'cancelled' only. @@ --- 4. Backfill rows back: 'canceled' -> 'cancelled'. -UPDATE purchase_executions -SET status = 'cancelled' -WHERE status = 'canceled'; - -UPDATE ri_exchange_history -SET status = 'cancelled' -WHERE status = 'canceled'; +-- already done aboveAlso applies to: 42-43, 55-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql` around lines 22 - 27, The down migration restores a `cancelled`-only check constraint before handling existing `canceled` data, so rollback fails when post-up rows are present. Update the `purchase_executions` rollback path to backfill any `status = 'canceled'` rows to `cancelled` before re-adding `purchase_executions_status_check`, and ensure the same ordering is applied for the other affected constraint blocks in this migration so the constraint changes succeed cleanly.internal/config/interfaces.go (1)
85-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
CancelExecutionAtomicstill documents the wrong status set.The caller path and implementation only CAS
pending/notified; scheduled cancels go throughCancelScheduledExecutionAtomicbelow. Leavingscheduledhere advertises the wrong contract.Suggested fix
- // CancelExecutionAtomic atomically flips status from pending / notified / - // scheduled to canceled, setting canceled_by. The 'scheduled' status - // supports the Gmail-style pre-fire delay revoke path (issue `#290`). + // CancelExecutionAtomic atomically flips status from pending / notified + // to canceled, setting canceled_by.🤖 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/interfaces.go` around lines 85 - 92, `CancelExecutionAtomic` is documenting the wrong status transition set. Update the comment on `CancelExecutionAtomic` in `interfaces.go` so it only describes the actual CAS behavior for `pending` and `notified`, and remove the `scheduled`/Gmail-style revoke wording since that path belongs to `CancelScheduledExecutionAtomic`; keep the return contract and `WithTx` requirement aligned with the real implementation.internal/config/store_postgres.go (1)
1076-1089: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep execution reads and canceled-only cleanup compatible with late legacy writes.
Migration 000078 backfills once, but the rollout still leaves old writers alive for a window. Projecting only
canceled_byand matching onlystatus = 'canceled'means latecancelled/cancelled_bywrites can lose actor attribution or miss canceled-only cleanup until the CONTRACT step. The same issue applies to the other executionSELECTs updated in this block.Example compatibility shape
- cloud_account_id, source, approved_by, canceled_by, capacity_percent, + cloud_account_id, source, approved_by, + COALESCE(canceled_by, cancelled_by) AS canceled_by, capacity_percent,- status IN ('completed', 'canceled') + status IN ('completed', 'canceled', 'cancelled')Also applies to: 1470-1485, 1520-1525
🤖 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 1076 - 1089, The execution read queries and canceled-only cleanup are still too strict for the migration rollout window. Update the purchase_executions SELECTs and the canceled cleanup path to remain compatible with late legacy writes by accepting both canceled/cancelled spellings and both canceled_by/cancelled_by attribution fields, instead of relying only on a single status/value or column name. Apply the same compatibility handling in the affected execution query helpers in this block so older writers do not lose actor data or get skipped before the CONTRACT step.
🧹 Nitpick comments (1)
internal/config/store_postgres_pgxmock_test.go (1)
440-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the SQL projection, not just the mock column names.
Changing these pgxmock column labels to
canceled_byis good, but the currentExpectQuery(...)matchers still don't require the production SQL to projectcanceled_by. That means a query regression can stay green even though these fixtures were renamed. Please make at least one matcher per query contract assert theapproved_by, canceled_byportion explicitly.Example tightening
- mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg()).WillReturnRows(rows) + mock.ExpectQuery(`(?s)SELECT.*approved_by,\s*canceled_by,\s*capacity_percent.*FROM purchase_executions`). + WithArgs(pgxmock.AnyArg()). + WillReturnRows(rows)Also applies to: 490-490, 557-557, 1896-1896
🤖 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_pgxmock_test.go` at line 440, The pgxmock expectations for the store queries only rename fixture columns and still don’t verify the actual SQL projection, so tighten the relevant ExpectQuery matchers to explicitly require the approved_by, canceled_by portion in the projected SELECT. Update the query contracts in store_postgres_pgxmock_test.go for the affected cases so at least one matcher per query asserts those columns in the SQL text, rather than relying only on the mock column labels.
🤖 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_history_test.go`:
- Around line 1711-1714: The KPI regression fixtures in handler_history_test
were fully switched to the new spelling, which removes coverage for mixed-fleet
rollout behavior. In the regression table used for dollar KPI and TotalCompleted
checks, keep one legacy-spelled status value ("cancelled") alongside the new
"canceled" rows so the test still exercises both spellings. Update the affected
fixture entries in the KPI regression block and the corresponding later block to
preserve that legacy case.
In `@internal/api/handler_purchases.go`:
- Around line 390-393: The idempotent cancel path in the purchase handler is too
strict during the migration window: in the logic around the existing execution
status check, treat both "canceled" and legacy "cancelled" as already-canceled
so retries remain idempotent. Update the status guard in the handler that uses
GetExecutionByID and the cancellation flow to dual-read both spellings until the
CONTRACT migration is complete, while still preserving the plan-disable side
effect for already-canceled executions.
- Around line 380-381: The cancel path in TransitionExecutionStatus is
incorrectly converting real backend failures into a 409 conflict. Update the
error handling around errors.Is(err, config.ErrExecutionNotInExpectedStatus) in
the execution cancel flow so only ErrExecutionNotInExpectedStatus maps to
NewClientError(409), and all other errors are returned as server-side failures
instead of client conflicts. Use the existing executionID and
TransitionExecutionStatus call site to keep the behavior limited to the intended
status-conflict case.
In
`@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sql`:
- Around line 18-29: The EXPAND step in the migration is not backward-compatible
with the stated rollout order because it rewrites live rows to canceled and only
copies cancelled_by once, while pre-rename code may still read/write cancelled
and cancelled_by. Update the migration logic around the affected
rename/normalize blocks to keep old and new spellings synchronized during the
overlap window, or defer the row rewrite until after the code rollout; use the
existing DO blocks and status/column migration section to ensure both binaries
remain functional until the follow-up CONTRACT step.
---
Duplicate comments:
In `@internal/api/handler_history.go`:
- Line 117: The history status read path currently only recognizes "canceled",
which breaks EXPAND compatibility with older rows still written as "cancelled".
Update the status handling in the history-related code paths (including
historyExecutionStatuses and the filtering/annotation/summary logic around the
referenced symbols) to accept both spellings on read, while keeping outward
normalization and emitted text as "canceled". Ensure any matching, bucketing, or
comparisons treat "cancelled" and "canceled" as equivalent until CONTRACT.
In `@internal/config/interfaces.go`:
- Around line 85-92: `CancelExecutionAtomic` is documenting the wrong status
transition set. Update the comment on `CancelExecutionAtomic` in `interfaces.go`
so it only describes the actual CAS behavior for `pending` and `notified`, and
remove the `scheduled`/Gmail-style revoke wording since that path belongs to
`CancelScheduledExecutionAtomic`; keep the return contract and `WithTx`
requirement aligned with the real implementation.
In `@internal/config/store_postgres.go`:
- Around line 1076-1089: The execution read queries and canceled-only cleanup
are still too strict for the migration rollout window. Update the
purchase_executions SELECTs and the canceled cleanup path to remain compatible
with late legacy writes by accepting both canceled/cancelled spellings and both
canceled_by/cancelled_by attribution fields, instead of relying only on a single
status/value or column name. Apply the same compatibility handling in the
affected execution query helpers in this block so older writers do not lose
actor data or get skipped before the CONTRACT step.
In
`@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql`:
- Around line 47-50: The down migration currently drops canceled_by without
restoring the older cancelled_by field, which would lose rollback attribution
data. Update the migration around the purchase_executions ALTER TABLE step to
add or restore cancelled_by from the existing canceled_by values before removing
canceled_by, so the rollback preserves cancellation ownership. Use the
migration’s purchase_executions block and the canceled_by/cancelled_by column
names to locate the change.
- Around line 22-27: The down migration restores a `cancelled`-only check
constraint before handling existing `canceled` data, so rollback fails when
post-up rows are present. Update the `purchase_executions` rollback path to
backfill any `status = 'canceled'` rows to `cancelled` before re-adding
`purchase_executions_status_check`, and ensure the same ordering is applied for
the other affected constraint blocks in this migration so the constraint changes
succeed cleanly.
---
Nitpick comments:
In `@internal/config/store_postgres_pgxmock_test.go`:
- Line 440: The pgxmock expectations for the store queries only rename fixture
columns and still don’t verify the actual SQL projection, so tighten the
relevant ExpectQuery matchers to explicitly require the approved_by, canceled_by
portion in the projected SELECT. Update the query contracts in
store_postgres_pgxmock_test.go for the affected cases so at least one matcher
per query asserts those columns in the SQL text, rather than relying only on the
mock column labels.
🪄 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: 4df8fe4c-5f9b-4206-ab5b-26773daae389
📒 Files selected for processing (30)
internal/api/coverage_extras_test.gointernal/api/handler_dashboard.gointernal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_revoke.gointernal/api/handler_purchases_revoke_test.gointernal/api/handler_purchases_test.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_test.gointernal/api/router_660_permission_flips_test.gointernal/api/router_handlers_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/types_test.gointernal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sqlinternal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sqlinternal/mocks/stores.gointernal/purchase/approvals.gointernal/purchase/approvals_test.gointernal/purchase/coverage_extra_test.gointernal/purchase/manager_test.gointernal/purchase/messages_test.gointernal/purchase/reaper_test.gointernal/purchase/scheduled_fire.gointernal/purchase/scheduled_fire_test.gointernal/server/test_helpers_test.go
cristim
added a commit
that referenced
this pull request
Jun 24, 2026
… EXPAND Address CodeRabbit review on PR #1277. The expand-contract rename must accept both spellings/columns throughout the EXPAND phase; only the deferred contract migration (#1278) removes the legacy spelling. Down migration (CRITICAL rollback bug + actor loss): - Reorder the DOWN path so data is converted 'canceled'->'cancelled' BEFORE the 'cancelled'-only CHECK constraints are re-added. The old order re-added the narrowed constraint first, so any row still in the new state violated it and the whole rollback failed. - Backfill canceled_by into cancelled_by BEFORE dropping canceled_by so actor attribution recorded by new code during the deploy window survives. Reads stay dual-compatible: - handler_history.go: accept both 'canceled' and 'cancelled' in the history fetch filter and in the KPI-summary / annotation switches so legacy rows written mid-deploy are neither dropped nor allowed to inflate KPIs. - store_postgres.go: project COALESCE(canceled_by, cancelled_by) on every read so a late legacy write landing in cancelled_by is still surfaced. - handler_purchases.go: the cancel-recovery idempotency check accepts both spellings. Scheduled-cancel routing (doc + predicate reconciliation): - interfaces.go: align the CancelExecutionAtomic doc to the implementation (pending/notified only; scheduled is handled by CancelScheduledExecutionAtomic). - types.go: add IsImmediatelyCancelable (pending/notified) distinct from the broader IsCancelable (adds scheduled). The /cancel paths (cancelPurchaseViaSession, loadCancelableExecution) now gate on the narrow predicate so a scheduled row is routed to a clear "use the revoke endpoint" 409 instead of misrouting into the pending/notified-only CAS and surfacing a misleading "concurrent operation" error. Tests: - New integration test 000078_..._test.go exercises up (both spellings accepted, legacy backfilled) and down (rollback succeeds with 'canceled' rows present, data converted back, actor attribution preserved, narrowed constraint restored). - Unit tests for the cancelable predicates and the scheduled-routing behaviour on both cancel paths.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
cristim
added a commit
that referenced
this pull request
Jun 25, 2026
…l errors Address CodeRabbit round-2 review on PR #1277. 1. cancelOrRecoverExecution: classify errors correctly. A TransitionExecutionStatus failure that is NOT ErrExecutionNotInExpectedStatus is a real server-side fault, not a CAS conflict. Return a plain wrapped error (router -> generic 500, raw detail logged) instead of a 409 that misclassified a retriable backend failure as the caller's fault and leaked backend text. The genuine status-mismatch 409 path is unchanged. 2. Keep a legacy-spelling fixture in the KPI regression test so the dual-spelling read path stays covered during EXPAND. 3+4. Make the EXPAND migration genuinely deploy-order-safe with live old code. The UP migration is now purely ADDITIVE: it widens both CHECK constraints to accept both spellings and copies existing cancelled_by into canceled_by, but it NO LONGER normalizes status values. A one-time UP normalization was a false guarantee -- old code still running writes fresh 'cancelled' rows the instant after it runs. Instead the schema accepts both spellings and the application reads both at all times (status filter + KPI/cancel switches + COALESCE(canceled_by, cancelled_by) on every one of the 10 read projections), so any row written by old OR new code at any point reads correctly. The authoritative, complete normalization is deferred to the CONTRACT migration (#1278), which runs after all old code is gone. The down.sql still converts new-code 'canceled' rows back and drains canceled_by before narrowing the constraint (the round-1 critical-ordering fix is preserved). Migration header/down comments and issue #1278 updated to match exactly what the SQL does. Misspell-clean without nolint in production code: add config.StatusCanceled and config.LegacyStatusCanceled (the latter built by concatenation so the US-locale misspell linter does not flag the legacy value) and reference them from the dual-spelling code paths. The 000078 integration test carries one file-level //nolint:misspell because it must reference the real legacy column name cancelled_by and assert literal legacy values until #1278 renames them. Tests: go build + go test ./... pass (5490). New unit test asserts the backend-error path returns a non-ClientError (generic 5xx). Migration up/down test updated for the additive design (status preserved, dual-read via COALESCE); runs in CI (local Docker unavailable).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler_purchases.go (1)
399-404: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize the legacy status before returning the idempotent result.
This dual-read path accepts
LegacyStatusCanceled, but then returnsexistingunchanged. During EXPAND, a retry against a legacy-canceled row can still surface"cancelled"in the handler result even though the API contract is moving to"canceled".Proposed fix
if existing.Status != config.StatusCanceled && existing.Status != config.LegacyStatusCanceled { return nil, NewClientError(409, fmt.Sprintf( "execution %s cannot be canceled (status=%s)", executionID, existing.Status)) } + if existing.Status == config.LegacyStatusCanceled { + normalized := *existing + normalized.Status = config.StatusCanceled + existing = &normalized + } return existing, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases.go` around lines 399 - 404, The idempotent cancel path in the purchase handler currently accepts both current and legacy canceled statuses but returns the legacy value unchanged. Update the return path in the purchase cancellation logic so that when the status is LegacyStatusCanceled it is normalized to StatusCanceled before returning the existing record, keeping the handler response consistent with the API contract.
🤖 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.go`:
- Around line 954-958: The cancel flow in guardImmediatelyCancelable and
authorizeSessionCancel is leaking execution state because the status-specific
409 check runs before authorization. Move the authorization check earlier in the
cancel path so an unauthorized caller cannot infer whether an execution exists
or is in a cancelable state, while still preserving the revoke guidance for
callers who are authorized for that execution.
In
`@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled_test.go`:
- Around line 4-9: Remove the file-level misspell suppression from the migration
test and adjust the test/lint strategy so the legacy identifiers in
000078_rename_cancelled_to_canceled_test are still covered without adding a new
`//nolint:misspell`; update the relevant test setup around the migration
regression case so the file stays compliant with the PR’s no-suppression
requirement.
- Around line 60-62: The rollback test currently uses RunMigrations, which
advances to head and can include future migrations, so it is no longer pinned to
the 000078 behavior. Update the test in the 000078 migration rollback case to
migrate only up to renameCanceledVersion before rolling back to 77, using the
migration helper in that test instead of RunMigrations so the regression guard
stays fixed to migration 000078.
In
`@internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sql`:
- Around line 42-46: The deployment-order comment in the 000078 migration is too
permissive and should be tightened to match the actual rollout sequence. Update
the note in the migration header to state that 000078 must run first and the
code deploy should follow, since the schema only accepts canceled after the
widened CHECKs are in place; reference the migration comment near the DEPLOY
ORDER note in 000078_rename_cancelled_to_canceled.up.sql.
---
Outside diff comments:
In `@internal/api/handler_purchases.go`:
- Around line 399-404: The idempotent cancel path in the purchase handler
currently accepts both current and legacy canceled statuses but returns the
legacy value unchanged. Update the return path in the purchase cancellation
logic so that when the status is LegacyStatusCanceled it is normalized to
StatusCanceled before returning the existing record, keeping the handler
response consistent with the API contract.
🪄 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: f1f28d9d-e0f0-4566-befb-fbde533a47d8
📒 Files selected for processing (13)
internal/api/handler_history.gointernal/api/handler_history_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/types.gointernal/config/types_test.gointernal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sqlinternal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sqlinternal/database/postgres/migrations/000078_rename_cancelled_to_canceled_test.gointernal/purchase/approvals.gointernal/purchase/approvals_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/purchase/approvals_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/handler_history_test.go
- internal/config/interfaces.go
- internal/api/handler_history.go
- internal/config/store_postgres.go
cristim
added a commit
that referenced
this pull request
Jun 26, 2026
Security: move authorizeSessionCancel before guardImmediatelyCancelable in cancelPurchaseViaSession so unauthorized callers cannot infer execution state from 409 status-specific errors. Update the two status-guard tests to grant cancel-own so auth passes before the guard fires (and the guard still rejects). Migration test: pin the rollback test to MigrateToVersion(renameCanceledVersion) instead of RunMigrations (head), so future migrations can't corrupt the 000078-specific regression guard. Remove the file-level //nolint:misspell directive (non-functional with a space; contradicts PR's no-suppression goal) and add a .golangci.yml exclusion for the migration test file instead, which is the deliberate strategy for a file that must reference legacy DB spellings. Migration comment: tighten the DEPLOY ORDER note in up.sql to state that 000078 must complete before new code writes 'canceled'; remove the misleading "races ahead" phrasing that implied out-of-order deploys are safe.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 26, 2026
Adversarial review of #1277 (cancelled -> canceled expand-contract) found the rename was incomplete on the read side: - frontend/src/history.ts: statusBadgeHTML, the chip-row counts, and the active-filter predicates only matched the British 'cancelled'. After the backend cut over to writing 'canceled', a row written by the new code fell through to the green Completed badge, was counted in the Completed total, and was invisible to the Cancelled chip filter -- the exact regression the rename is supposed to prevent. Match both spellings everywhere a status enum is compared. - frontend/src/riexchange.ts: getStatusBadgeClass had the same single-spelling bug for ri_exchange_history rows. - internal/api/openapi.yaml: PurchaseExecution.status enum still advertised only 'cancelled', so strict OpenAPI clients would treat the new 'canceled' value as an enum violation. Add both during the deploy window; the contract migration (#1278) drops 'cancelled'. Also addresses the two outside-diff CodeRabbit comments on this PR: - internal/api/handler_purchases.go: when cancelOrRecoverExecution recovers an idempotent cancel and finds the row in the legacy British spelling, normalize the in-memory copy to StatusCanceled before returning. Stored row is untouched -- the contract migration's authoritative backfill still observes every legacy row. - internal/database/postgres/migrations/000078_*.up.sql: tighten the DEPLOY ORDER comment to the strict 3-step sequence (apply 000078 against the OLD code; only then release the new code; then 1278) so a future operator can't infer a more permissive ordering and crash the cancel paths against the original 'cancelled'-only CHECK. Tests: - frontend/src/__tests__/history.test.ts: new test seeds rows in both spellings and a real completed row, asserts the badge renders Cancelled (not Completed) for both, the chip counts 2 (not 1), and clicking the Cancelled chip surfaces both rows. Fails on the pre-fix FE -- the canonical regression-replicates-the-real-scenario guard. - internal/api/handler_purchases_test.go: new test seeds the recovery branch with status=LegacyStatusCanceled and asserts the response Status is normalized to StatusCanceled. CI: Lint Code and Security Scanning are pre-existing main breakages (2707 repo-wide lint issues; pgx GO-2026-5004 advisory) unrelated to this PR's diff -- verified against the most recent main run.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Adversarial review of #1277 (cancelled -> canceled expand-contract) found the rename was incomplete on the read side: - frontend/src/history.ts: statusBadgeHTML, the chip-row counts, and the active-filter predicates only matched the British 'cancelled'. After the backend cut over to writing 'canceled', a row written by the new code fell through to the green Completed badge, was counted in the Completed total, and was invisible to the Cancelled chip filter -- the exact regression the rename is supposed to prevent. Match both spellings everywhere a status enum is compared. - frontend/src/riexchange.ts: getStatusBadgeClass had the same single-spelling bug for ri_exchange_history rows. - internal/api/openapi.yaml: PurchaseExecution.status enum still advertised only 'cancelled', so strict OpenAPI clients would treat the new 'canceled' value as an enum violation. Add both during the deploy window; the contract migration (#1278) drops 'cancelled'. Also addresses the two outside-diff CodeRabbit comments on this PR: - internal/api/handler_purchases.go: when cancelOrRecoverExecution recovers an idempotent cancel and finds the row in the legacy British spelling, normalize the in-memory copy to StatusCanceled before returning. Stored row is untouched -- the contract migration's authoritative backfill still observes every legacy row. - internal/database/postgres/migrations/000078_*.up.sql: tighten the DEPLOY ORDER comment to the strict 3-step sequence (apply 000078 against the OLD code; only then release the new code; then 1278) so a future operator can't infer a more permissive ordering and crash the cancel paths against the original 'cancelled'-only CHECK. Tests: - frontend/src/__tests__/history.test.ts: new test seeds rows in both spellings and a real completed row, asserts the badge renders Cancelled (not Completed) for both, the chip counts 2 (not 1), and clicking the Cancelled chip surfaces both rows. Fails on the pre-fix FE -- the canonical regression-replicates-the-real-scenario guard. - internal/api/handler_purchases_test.go: new test seeds the recovery branch with status=LegacyStatusCanceled and asserts the response Status is normalized to StatusCanceled. CI: Lint Code and Security Scanning are pre-existing main breakages (2707 repo-wide lint issues; pgx GO-2026-5004 advisory) unrelated to this PR's diff -- verified against the most recent main run.
cristim
force-pushed
the
fix/db-rename-cancelled-to-canceled
branch
from
July 16, 2026 20:34
1b16ab9 to
6e6a992
Compare
Member
Author
|
Renumbered this PR's migration 000082 -> 000084 (also re-rebased onto current origin/main). Why: main now tops at migration 000083 ( Changes: renamed up/down SQL + integration test file; bumped pinned version constants ( Verified:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Defect 1 -- migration number collision: Renumber marketplace migration from 000068 (already deployed, skipped by golang-migrate on prod) to 000084 (next free after main's 000083). Verified: main tops at 000083; #1277 uses 000082; no open PR holds 000084. All "000068" references in SQL comments and types.go updated to 000084. Defect 2 -- offering_class never written: CUDly's EC2 client hardwires OfferingClassTypeConvertible; savePurchaseHistory never stamped the field, so every row landed with offering_class=NULL and the Sell button never rendered. Fix: (a) stamp offering_class='convertible' in savePurchaseHistory for all AWS EC2 purchases so future CUDly-bought rows are correctly classified at write time; (b) add FetchOfferingClass to the marketplaceEC2Client interface and implement it via DescribeReservedInstances so the marketplace-list handler can lazily populate offering_class for pre-migration rows and for externally-created Standard RIs (purchased before CUDly existed); (c) add StampOfferingClass to ConfigStore + PostgresStore + mock to persist the fetched class so subsequent requests skip the extra AWS call; (d) a new populateOfferingClass helper in the handler ties it together -- it runs when offering_class is empty after validateMarketplaceListRequest and before the definitive "standard only" gate. How a 'standard' row comes to exist: an externally-created Standard RI will get its offering_class populated from AWS DescribeReservedInstances on the first POST .../marketplace-list call and the value persisted for future calls. Rows purchased by CUDly are stamped 'convertible' at write time (CUDly only ever buys Convertible EC2 RIs). Defect 3 -- $0 price guardrail gap: resolveMarketplacePriceSchedule accepted Price >= 0, allowing zero-dollar listings. Fix: reject Price <= 0 with a clear error; add a named constant awsMarketplaceMinPriceFloorFraction (5%) and a total-schedule floor check so a schedule that sums to less than 5% of the prorated residual value is also rejected with an explicit message. Extract floor check into checkSuppliedScheduleFloor and the class-check predicate into isKnownNonStandardOfferingClass to keep all touched functions below gocyclo 10. Regression tests: - TestMigration084_MarketplaceColumns: integration test proves the three new columns exist and round-trip after migrating a fresh DB through 000084. - TestMarketplaceList_EmptyOfferingClassFetchedStandard: proves an externally-created Standard RI (offering_class="") is listable after the lazy-populate path fetches and stamps 'standard' from AWS. - TestMarketplaceList_EmptyOfferingClassFetchedConvertible: proves the gate still rejects when AWS reports 'convertible' even if DB had no class. - TestResolveMarketplacePriceSchedule_ZeroPriceRejected: Price=0 rejected. - TestResolveMarketplacePriceSchedule_BelowFloorRejected: sub-floor rejected.
7 tasks
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
…collision PR #1277 (cancelled->canceled rename) was concurrently renumbered to 000084 and merges before #808. Once it lands, main will hold 000084, so #808's 000084 would collide on the merge ref (pre-commit --all-files migration check) and give golang-migrate two version-84 migrations. Since #1277 takes the lower number and merges first, #808 moves to 000085. - git mv the up/down/test migration files 000084_* -> 000085_* - update the "-- Migration" header in the up.sql and "-- Revert migration" in the down.sql to 000085 - update every 000084 reference in comments: config/types.go (x3), config/interfaces.go, config/store_postgres.go, api/handler_marketplace.go (x2), providers/aws/services/ec2/client.go - rename TestMigration084_MarketplaceColumns -> TestMigration085_* and its fixture identifiers (ri-085-test, ril-085-test) Also fix the integration-test fixture surfaced by running it against a real testcontainer PG: plan_id is a UUID FK, so the literal 'plan-085' failed with SQLSTATE 22P02. The INSERT now lists only NOT-NULL-without-default columns plus the three new marketplace columns and omits plan_id/plan_name/ramp_step/ source (nullable or defaulted), so the test needs no purchase_plans fixture. Verified: migrations apply cleanly through version 85 and the three columns round-trip.
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
) The Approve button was visible for a user's own queued purchase even when their role lacked approve-any or approve-own permission. Ownership alone was incorrectly treated as sufficient authorization. Root causes fixed: - canApprovePendingRow() in history.ts now requires an explicit approve-own permission before consulting ownership; approve-any continues to bypass the ownership check as before. - DefaultUserPermissions() no longer includes approve-own:purchases. The Standard Users group must not self-approve by default; an explicit custom group grant is required (four-eyes principle). - Migration 000086 removes approve-own from the seeded Standard Users Postgres group for existing deployments. Numbered 000086 to sit after the in-flight 000084 (#1277) and 000085 (#808), which merge first. - permissions.generated.ts regenerated to match the updated Go default. Regression tests added (fail-before / pass-after): - Frontend: "user without any approve permission sees NO Approve button even on their own rows (issue #1407 four-eyes)" in src/__tests__/history-approve-button.test.ts - Backend: TestHandler_approvePurchase_RejectsCreatorWithoutApprovePermission in internal/api/handler_purchases_test.go -- asserts 403 when the creator calls approve without holding approve-any or approve-own. - Migration: TestMigration_RemoveApproveOwnFromStandardUsers (integration) confirms 000086 removes approve-own while retaining cancel-own/retry-own, and that its down migration restores approve-own. - Unit: DefaultUserPermissions four-eyes guard in types_test.go.
7 tasks done
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
… EXPAND Address CodeRabbit review on PR #1277. The expand-contract rename must accept both spellings/columns throughout the EXPAND phase; only the deferred contract migration (#1278) removes the legacy spelling. Down migration (CRITICAL rollback bug + actor loss): - Reorder the DOWN path so data is converted 'canceled'->'cancelled' BEFORE the 'cancelled'-only CHECK constraints are re-added. The old order re-added the narrowed constraint first, so any row still in the new state violated it and the whole rollback failed. - Backfill canceled_by into cancelled_by BEFORE dropping canceled_by so actor attribution recorded by new code during the deploy window survives. Reads stay dual-compatible: - handler_history.go: accept both 'canceled' and 'cancelled' in the history fetch filter and in the KPI-summary / annotation switches so legacy rows written mid-deploy are neither dropped nor allowed to inflate KPIs. - store_postgres.go: project COALESCE(canceled_by, cancelled_by) on every read so a late legacy write landing in cancelled_by is still surfaced. - handler_purchases.go: the cancel-recovery idempotency check accepts both spellings. Scheduled-cancel routing (doc + predicate reconciliation): - interfaces.go: align the CancelExecutionAtomic doc to the implementation (pending/notified only; scheduled is handled by CancelScheduledExecutionAtomic). - types.go: add IsImmediatelyCancelable (pending/notified) distinct from the broader IsCancelable (adds scheduled). The /cancel paths (cancelPurchaseViaSession, loadCancelableExecution) now gate on the narrow predicate so a scheduled row is routed to a clear "use the revoke endpoint" 409 instead of misrouting into the pending/notified-only CAS and surfacing a misleading "concurrent operation" error. Tests: - New integration test 000078_..._test.go exercises up (both spellings accepted, legacy backfilled) and down (rollback succeeds with 'canceled' rows present, data converted back, actor attribution preserved, narrowed constraint restored). - Unit tests for the cancelable predicates and the scheduled-routing behaviour on both cancel paths.
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
…l errors Address CodeRabbit round-2 review on PR #1277. 1. cancelOrRecoverExecution: classify errors correctly. A TransitionExecutionStatus failure that is NOT ErrExecutionNotInExpectedStatus is a real server-side fault, not a CAS conflict. Return a plain wrapped error (router -> generic 500, raw detail logged) instead of a 409 that misclassified a retriable backend failure as the caller's fault and leaked backend text. The genuine status-mismatch 409 path is unchanged. 2. Keep a legacy-spelling fixture in the KPI regression test so the dual-spelling read path stays covered during EXPAND. 3+4. Make the EXPAND migration genuinely deploy-order-safe with live old code. The UP migration is now purely ADDITIVE: it widens both CHECK constraints to accept both spellings and copies existing cancelled_by into canceled_by, but it NO LONGER normalizes status values. A one-time UP normalization was a false guarantee -- old code still running writes fresh 'cancelled' rows the instant after it runs. Instead the schema accepts both spellings and the application reads both at all times (status filter + KPI/cancel switches + COALESCE(canceled_by, cancelled_by) on every one of the 10 read projections), so any row written by old OR new code at any point reads correctly. The authoritative, complete normalization is deferred to the CONTRACT migration (#1278), which runs after all old code is gone. The down.sql still converts new-code 'canceled' rows back and drains canceled_by before narrowing the constraint (the round-1 critical-ordering fix is preserved). Migration header/down comments and issue #1278 updated to match exactly what the SQL does. Misspell-clean without nolint in production code: add config.StatusCanceled and config.LegacyStatusCanceled (the latter built by concatenation so the US-locale misspell linter does not flag the legacy value) and reference them from the dual-spelling code paths. The 000078 integration test carries one file-level //nolint:misspell because it must reference the real legacy column name cancelled_by and assert literal legacy values until #1278 renames them. Tests: go build + go test ./... pass (5490). New unit test asserts the backend-error path returns a non-ClientError (generic 5xx). Migration up/down test updated for the additive design (status preserved, dual-read via COALESCE); runs in CI (local Docker unavailable).
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Security: move authorizeSessionCancel before guardImmediatelyCancelable in cancelPurchaseViaSession so unauthorized callers cannot infer execution state from 409 status-specific errors. Update the two status-guard tests to grant cancel-own so auth passes before the guard fires (and the guard still rejects). Migration test: pin the rollback test to MigrateToVersion(renameCanceledVersion) instead of RunMigrations (head), so future migrations can't corrupt the 000078-specific regression guard. Remove the file-level //nolint:misspell directive (non-functional with a space; contradicts PR's no-suppression goal) and add a .golangci.yml exclusion for the migration test file instead, which is the deliberate strategy for a file that must reference legacy DB spellings. Migration comment: tighten the DEPLOY ORDER note in up.sql to state that 000078 must complete before new code writes 'canceled'; remove the misleading "races ahead" phrasing that implied out-of-order deploys are safe.
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Adversarial review of #1277 (cancelled -> canceled expand-contract) found the rename was incomplete on the read side: - frontend/src/history.ts: statusBadgeHTML, the chip-row counts, and the active-filter predicates only matched the British 'cancelled'. After the backend cut over to writing 'canceled', a row written by the new code fell through to the green Completed badge, was counted in the Completed total, and was invisible to the Cancelled chip filter -- the exact regression the rename is supposed to prevent. Match both spellings everywhere a status enum is compared. - frontend/src/riexchange.ts: getStatusBadgeClass had the same single-spelling bug for ri_exchange_history rows. - internal/api/openapi.yaml: PurchaseExecution.status enum still advertised only 'cancelled', so strict OpenAPI clients would treat the new 'canceled' value as an enum violation. Add both during the deploy window; the contract migration (#1278) drops 'cancelled'. Also addresses the two outside-diff CodeRabbit comments on this PR: - internal/api/handler_purchases.go: when cancelOrRecoverExecution recovers an idempotent cancel and finds the row in the legacy British spelling, normalize the in-memory copy to StatusCanceled before returning. Stored row is untouched -- the contract migration's authoritative backfill still observes every legacy row. - internal/database/postgres/migrations/000078_*.up.sql: tighten the DEPLOY ORDER comment to the strict 3-step sequence (apply 000078 against the OLD code; only then release the new code; then 1278) so a future operator can't infer a more permissive ordering and crash the cancel paths against the original 'cancelled'-only CHECK. Tests: - frontend/src/__tests__/history.test.ts: new test seeds rows in both spellings and a real completed row, asserts the badge renders Cancelled (not Completed) for both, the chip counts 2 (not 1), and clicking the Cancelled chip surfaces both rows. Fails on the pre-fix FE -- the canonical regression-replicates-the-real-scenario guard. - internal/api/handler_purchases_test.go: new test seeds the recovery branch with status=LegacyStatusCanceled and asserts the response Status is normalized to StatusCanceled. CI: Lint Code and Security Scanning are pre-existing main breakages (2707 repo-wide lint issues; pgx GO-2026-5004 advisory) unrelated to this PR's diff -- verified against the most recent main run.
cristim
force-pushed
the
fix/db-rename-cancelled-to-canceled
branch
from
July 16, 2026 21:08
6e6a992 to
25454aa
Compare
Member
Author
|
Rebased onto current origin/main (81cd1f4) to pick up the flaky-test fix from #1399 (TestVerifyRetry_RebuildRateLimited is now deterministic). All 7 PR-owned commits carried over cleanly; validator_test.go now matches main's version. Migration 000084 remains valid (main stops at 000083 - no collision). All gates passed: go build, go vet, scheduledauth -count=5 (exit 0), affected packages (internal/config, internal/api, internal/purchase, internal/server), gocyclo -over 10 on PR-owned non-test files (clean), golangci-lint full run (0 issues). |
cristim
added a commit
that referenced
this pull request
Jul 16, 2026
Defect 1 -- migration number collision: Renumber marketplace migration from 000068 (already deployed, skipped by golang-migrate on prod) to 000084 (next free after main's 000083). Verified: main tops at 000083; #1277 uses 000082; no open PR holds 000084. All "000068" references in SQL comments and types.go updated to 000084. Defect 2 -- offering_class never written: CUDly's EC2 client hardwires OfferingClassTypeConvertible; savePurchaseHistory never stamped the field, so every row landed with offering_class=NULL and the Sell button never rendered. Fix: (a) stamp offering_class='convertible' in savePurchaseHistory for all AWS EC2 purchases so future CUDly-bought rows are correctly classified at write time; (b) add FetchOfferingClass to the marketplaceEC2Client interface and implement it via DescribeReservedInstances so the marketplace-list handler can lazily populate offering_class for pre-migration rows and for externally-created Standard RIs (purchased before CUDly existed); (c) add StampOfferingClass to ConfigStore + PostgresStore + mock to persist the fetched class so subsequent requests skip the extra AWS call; (d) a new populateOfferingClass helper in the handler ties it together -- it runs when offering_class is empty after validateMarketplaceListRequest and before the definitive "standard only" gate. How a 'standard' row comes to exist: an externally-created Standard RI will get its offering_class populated from AWS DescribeReservedInstances on the first POST .../marketplace-list call and the value persisted for future calls. Rows purchased by CUDly are stamped 'convertible' at write time (CUDly only ever buys Convertible EC2 RIs). Defect 3 -- $0 price guardrail gap: resolveMarketplacePriceSchedule accepted Price >= 0, allowing zero-dollar listings. Fix: reject Price <= 0 with a clear error; add a named constant awsMarketplaceMinPriceFloorFraction (5%) and a total-schedule floor check so a schedule that sums to less than 5% of the prorated residual value is also rejected with an explicit message. Extract floor check into checkSuppliedScheduleFloor and the class-check predicate into isKnownNonStandardOfferingClass to keep all touched functions below gocyclo 10. Regression tests: - TestMigration084_MarketplaceColumns: integration test proves the three new columns exist and round-trip after migrating a fresh DB through 000084. - TestMarketplaceList_EmptyOfferingClassFetchedStandard: proves an externally-created Standard RI (offering_class="") is listable after the lazy-populate path fetches and stamps 'standard' from AWS. - TestMarketplaceList_EmptyOfferingClassFetchedConvertible: proves the gate still rejects when AWS reports 'convertible' even if DB had no class. - TestResolveMarketplacePriceSchedule_ZeroPriceRejected: Price=0 rejected. - TestResolveMarketplacePriceSchedule_BelowFloorRejected: sub-floor rejected.
Member
Author
|
Rebased onto Rebase: Resolved conflicts with merged PR #889 (email revoke / Migration renumbered: 000082 -> 000089. Previous migration on main is 000086; in-flight #808=000087 and #1428=000088 merge before this one, so 000089 is the correct slot. Updated all code/comment/test/OpenAPI/frontend references from 000082 to 000089. Bug fix found during rebase: The conflict resolution had incorrectly changed Two lint fixes: Gate results (commit 421dbc5):
This PR should merge AFTER #808 (000087) and #1428 (000088). |
Member
Author
|
Rebase onto current main (7f0573d) complete + fix commit applied Branch fix/db-rename-cancelled-to-canceled rebased onto origin/main (old base was 9a8a9c7). Two commits now above main:
Force pushed: 421dbc5 -> a11f036 Migration 000089 confirmed present. renameCanceledPrevVersion=88 confirmed. Local gate results:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
The rebase of 000089 onto current main incorrectly resolved two conflicts in handler_purchases.go: 1. cancelOrRecoverExecution: reverted fmt.Errorf (router->500) back to NewClientError(409,...), misclassifying retriable backend faults as caller faults (feedback_http_status_classification). 2. cancelPurchaseViaSession: took main's broad !IsCancelable() guard instead of the PR's narrower guardImmediatelyCancelable, allowing "scheduled" executions through the pending/notified-only CAS path and producing a misleading "concurrent operation" 409 instead of the clear "use the revoke endpoint" message. Fix: restore fmt.Errorf for the backend-failure branch; add guardCancelableViaSession that explicitly routes "scheduled" to the revoke endpoint before falling through to IsCancelable; rename residual cancelledBy -> canceledBy; remove extra blank line in types.go. Regression tests: TestHandler_deletePlannedPurchase_BackendErrorReturns5xx and TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke now pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
purchase_executionsandri_exchange_historyto accept both'cancelled'and'canceled'; addscanceled_byalongsidecancelled_by; backfills all existing rows from'cancelled'to'canceled'."canceled"andcanceled_by; all//nolint:misspelldirectives on these identifiers removed.cancelled_bycolumn is tracked in the follow-up issue (to be filed).Expand-contract deploy ordering
"canceled"only).'cancelled'from constraints and dropcancelled_by.Test plan
go build ./...passes (no compilation errors)go test ./...passes (5476 tests, 0 failures)//nolint:misspelldirectives forcancelled/cancelled_byIF EXISTSguards andADD COLUMN IF NOT EXISTS)canceled_by, backfills"canceled"->"cancelled")Summary by CodeRabbit
New Features
Bug Fixes