fix(db): rename status value cancelled->canceled (expand-contract, US spelling)#1277
Open
cristim wants to merge 5 commits into
Open
fix(db): rename status value cancelled->canceled (expand-contract, US spelling)#1277cristim wants to merge 5 commits into
cristim wants to merge 5 commits into
Conversation
cristim
added
triaged
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR standardizes cancellation spelling to Changescancelled → canceled rollout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
4 tasks
… spelling) Expand-contract migration 000078: widens the CHECK constraints on purchase_executions and ri_exchange_history to accept BOTH spellings, adds the canceled_by column alongside cancelled_by, backfills existing rows to the new value, then updates all Go code to write/read "canceled" (US English) exclusively. A follow-up CONTRACT migration will drop the old spelling from the constraints and remove the cancelled_by column. The nolint:misspell directives on these identifiers are removed; no new ones are needed.
… 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.
…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).
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.
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
d90e39c to
61cffea
Compare
Member
Author
|
Rebased onto current `origin/main` (PR #1343 / `a2d8e111a`). New head: `61cffea31`. Conflicts resolved (2 hunks, 1 file): Both conflicts were in `internal/api/handler_purchases.go` inside `cancelOrRecoverExecution`, at the same logical site across two successive commits:
#1343 contract preserved: All `errors.Is(err, config.ErrNotFound)` branches in `handler_purchases.go`, `handler_purchases_revoke.go`, `config/store_postgres.go`, `purchase/approvals.go`, and related test files landed cleanly (commits 3-5 applied with no conflicts). Migration number: branch uses `000078`, main's highest is `000077` -- no collision, no renumbering. Gates (all passed):
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
5 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler_purchases.go (1)
907-937: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmail-token cancel skips the revoke-specific guard
guardImmediatelyCancelableis only used by the session path;h.purchase.CancelExecutionrejects scheduled executions with a generic 409, so email-link cancels won’t get the same revoke-endpoint message. Either apply the same check in the token flow or remove the “Shared by the session and email-token cancel gates” claim.🤖 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 907 - 937, The email-token cancel path in h.cancelPurchaseViaSession is missing the scheduled-execution guard, so it can return a generic 409 instead of the revoke-specific message. Update the token branch that calls h.purchase.CancelExecution to invoke guardImmediatelyCancelable on execution before canceling, or otherwise ensure the same scheduled-status check is applied there. Keep the behavior aligned with the session flow and the guardImmediatelyCancelable helper so both cancel gates share the same policy.
🧹 Nitpick comments (2)
frontend/src/history.ts (1)
369-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated dual-spelling check into a helper.
The
s === 'canceled' || s === 'cancelled'(ornormalizedequivalent) condition is duplicated acrossstatusBadgeHTML(Lines 381-382),buildStatusChipRowHTML(Line 421), and twice inrenderHistoryList(Lines 748-752, 768-770). A smallisCanceledStatus(s: string): booleanhelper would consolidate this and make the eventual CONTRACT-migration cleanup (referenced in the comments here) a single-point change instead of four.♻️ Suggested refactor
+// Migration 000078: matches both spellings until the CONTRACT migration +// (`#1278`) normalizes stored data; remove the 'cancelled' branch then. +function isCanceledStatus(s: string): boolean { + return s === 'canceled' || s === 'cancelled'; +} + function statusBadgeHTML(status: string): string { const normalized = (status || 'completed').toLowerCase(); switch (normalized) { ... - case 'canceled': - case 'cancelled': - return '<span class="badge badge-muted">Cancelled</span>';🤖 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 `@frontend/src/history.ts` around lines 369 - 403, Extract the repeated canceled-status check into a shared helper so the dual-spelling logic lives in one place. Add a small isCanceledStatus helper near statusBadgeHTML and update statusBadgeHTML, buildStatusChipRowHTML, and renderHistoryList to use it instead of repeating the normalized canceled/cancelled comparison. Keep the existing badge behavior unchanged, and make sure the CONTRACT-migration cleanup can later remove the legacy spelling from a single helper only.internal/api/handler_purchases.go (1)
351-417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDual-spelling recovery logic correctly resolves prior 409-misclassification and idempotency issues.
The CAS-then-fetch flow now returns a real error (not a 409) for non-CAS failures, and treats both
StatusCanceled/LegacyStatusCanceledas already-canceled on the recovery path, normalizing the in-memory value. This matches the two previously-flagged issues on this function.One nit: line 376 passes the literal
"canceled"toTransitionExecutionStatusinstead ofconfig.StatusCanceled. Given this constant exists specifically to prevent spelling drift during this migration, prefer it here (and at lines 366, 915, 1004 which also use literal"canceled").♻️ Suggested consistency fix
- result, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "canceled", actor) + result, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, config.StatusCanceled, actor)🤖 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 351 - 417, The cancel/recover flow still hardcodes the string "canceled" in `cancelOrRecoverExecution` and related call sites, which can drift from the canonical status constant during the migration. Replace those literal status values with `config.StatusCanceled` everywhere this execution status is set or compared so the `TransitionExecutionStatus` path and any other status updates stay consistent with the existing enum/constant usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/api/handler_purchases.go`:
- Around line 907-937: The email-token cancel path in h.cancelPurchaseViaSession
is missing the scheduled-execution guard, so it can return a generic 409 instead
of the revoke-specific message. Update the token branch that calls
h.purchase.CancelExecution to invoke guardImmediatelyCancelable on execution
before canceling, or otherwise ensure the same scheduled-status check is applied
there. Keep the behavior aligned with the session flow and the
guardImmediatelyCancelable helper so both cancel gates share the same policy.
---
Nitpick comments:
In `@frontend/src/history.ts`:
- Around line 369-403: Extract the repeated canceled-status check into a shared
helper so the dual-spelling logic lives in one place. Add a small
isCanceledStatus helper near statusBadgeHTML and update statusBadgeHTML,
buildStatusChipRowHTML, and renderHistoryList to use it instead of repeating the
normalized canceled/cancelled comparison. Keep the existing badge behavior
unchanged, and make sure the CONTRACT-migration cleanup can later remove the
legacy spelling from a single helper only.
In `@internal/api/handler_purchases.go`:
- Around line 351-417: The cancel/recover flow still hardcodes the string
"canceled" in `cancelOrRecoverExecution` and related call sites, which can drift
from the canonical status constant during the migration. Replace those literal
status values with `config.StatusCanceled` everywhere this execution status is
set or compared so the `TransitionExecutionStatus` path and any other status
updates stay consistent with the existing enum/constant usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 430bb784-fdf3-4415-a61c-31bceedbe152
📒 Files selected for processing (36)
.golangci.ymlfrontend/src/__tests__/history.test.tsfrontend/src/history.tsfrontend/src/riexchange.tsinternal/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/openapi.yamlinternal/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/database/postgres/migrations/000078_rename_cancelled_to_canceled_test.gointernal/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
✅ Files skipped from review due to trivial changes (10)
- internal/purchase/messages_test.go
- internal/purchase/scheduled_fire.go
- internal/purchase/manager_test.go
- internal/api/coverage_extras_test.go
- internal/purchase/scheduled_fire_test.go
- internal/api/router_660_permission_flips_test.go
- internal/api/handler_dashboard.go
- internal/config/store_postgres_pgxmock_test.go
- internal/purchase/approvals_test.go
- internal/config/interfaces.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/purchase/reaper_test.go
- internal/server/test_helpers_test.go
- internal/api/handler_ri_exchange_test.go
- internal/api/handler_purchases_revoke_test.go
- internal/api/handler_history_test.go
- internal/config/types_test.go
- internal/api/handler_history.go
- internal/api/handler_purchases_revoke.go
- internal/api/router_handlers_test.go
- internal/api/handler_ri_exchange.go
- internal/api/handler_test.go
- internal/config/types.go
- internal/database/postgres/migrations/000078_rename_cancelled_to_canceled_test.go
- internal/mocks/stores.go
- internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.up.sql
- internal/purchase/approvals.go
- internal/purchase/coverage_extra_test.go
- internal/database/postgres/migrations/000078_rename_cancelled_to_canceled.down.sql
- internal/config/store_postgres.go
- internal/api/handler_purchases_test.go
4 tasks
cristim
added a commit
that referenced
this pull request
Jul 4, 2026
…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.
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
Style
Bug Fixes