From 25525bbc1432c1925d03e166d7ea2cafe1e4c16a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 03:08:16 +0200 Subject: [PATCH 1/8] feat(ladder): add per-account config + schema foundation (default-off) Foundation for commitment laddering (phase-3 PR-1). Flag-gated and default-off: nothing runs until an operator enables both the global kill-switch and a per-account config. No scheduled task, email, or execution path is wired here (those land in later phase-3 PRs). Surface: - Migrations 000079/080/081: ladder_configs (per account x provider), ladder_runs (immutable audit), ladder_tranches (timed purchase slices with a run_id trace to ladder_runs); global_config.laddering_enabled kill-switch; ladder_run_id FK on purchase_executions and ri_exchange_history. All reversible (up/down verified on PG16). - Config store: GetLadderConfigs / GetLadderConfig / UpsertLadderConfig plus StoreInterface widening and mock coverage. - API: GET/PUT /api/ladder/configs with RBAC and fail-loud validation (out-of-range numerics rejected 400, never silently defaulted; absent keys default, explicit buffer_fraction=0 honored as no-buffer). - Settings UI: Commitment Laddering section (global toggle + per-account editor) rendered into the Purchasing panel. Money-path fields are nullable (NULL, not 0) and typed enums are validated against pkg/ladder rather than redefined. Part of #1336 (phase-3 PR-1) Refs #1333 --- frontend/src/__tests__/ladder.test.ts | 136 +++++ frontend/src/api/index.ts | 4 + frontend/src/api/ladder.ts | 74 +++ frontend/src/api/types.ts | 4 + frontend/src/index.html | 3 + frontend/src/ladder.ts | 529 ++++++++++++++++++ frontend/src/settings.ts | 15 + frontend/src/types.ts | 5 + internal/analytics/collector_test.go | 10 + internal/api/handler_config.go | 28 +- internal/api/handler_config_test.go | 85 +++ internal/api/handler_ladder.go | 107 ++++ internal/api/handler_ladder_test.go | 96 ++++ internal/api/router.go | 17 + internal/config/interfaces.go | 10 + internal/config/store_postgres.go | 10 +- internal/config/store_postgres_ladder.go | 192 +++++++ .../config/store_postgres_pgxmock_test.go | 4 + internal/config/types.go | 84 +++ internal/config/validation.go | 111 ++++ internal/config/validation_ladder_test.go | 174 ++++++ .../migrations/000079_ladder_configs.down.sql | 8 + .../migrations/000079_ladder_configs.up.sql | 58 ++ .../migrations/000080_ladder_runs.down.sql | 14 + .../migrations/000080_ladder_runs.up.sql | 87 +++ .../000081_ladder_tranches.down.sql | 14 + .../migrations/000081_ladder_tranches.up.sql | 50 ++ internal/mocks/stores.go | 48 ++ internal/server/test_helpers_test.go | 10 + 29 files changed, 1983 insertions(+), 4 deletions(-) create mode 100644 frontend/src/__tests__/ladder.test.ts create mode 100644 frontend/src/api/ladder.ts create mode 100644 frontend/src/ladder.ts create mode 100644 internal/api/handler_ladder.go create mode 100644 internal/api/handler_ladder_test.go create mode 100644 internal/config/store_postgres_ladder.go create mode 100644 internal/config/validation_ladder_test.go create mode 100644 internal/database/postgres/migrations/000079_ladder_configs.down.sql create mode 100644 internal/database/postgres/migrations/000079_ladder_configs.up.sql create mode 100644 internal/database/postgres/migrations/000080_ladder_runs.down.sql create mode 100644 internal/database/postgres/migrations/000080_ladder_runs.up.sql create mode 100644 internal/database/postgres/migrations/000081_ladder_tranches.down.sql create mode 100644 internal/database/postgres/migrations/000081_ladder_tranches.up.sql diff --git a/frontend/src/__tests__/ladder.test.ts b/frontend/src/__tests__/ladder.test.ts new file mode 100644 index 000000000..307e71a7f --- /dev/null +++ b/frontend/src/__tests__/ladder.test.ts @@ -0,0 +1,136 @@ +/** + * Commitment Laddering settings module tests (issue #1333 phase 3). + * + * Covers the two security/correctness-sensitive surfaces: + * (a) renderConfigTable escapes attacker-controlled cloud_account_id/provider + * so an injected tag never lands raw in innerHTML (XSS). + * (b) saveLadderConfig's max_hourly_commit_per_run guard: blank -> null, + * positive -> passthrough, non-positive/invalid -> rejected (no store call). + */ +import { initLadderingSettings, renderConfigTable, saveLadderConfig } from '../ladder'; +import type { LadderConfig } from '../api'; + +jest.mock('../api', () => ({ + getLadderConfigs: jest.fn(), + upsertLadderConfig: jest.fn(), + getConfig: jest.fn(), + updateConfig: jest.fn(), +})); + +jest.mock('../permissions', () => ({ + canAccess: jest.fn(() => true), +})); + +const mockShowToast = jest.fn(); +jest.mock('../toast', () => ({ + showToast: (opts: unknown) => mockShowToast(opts), +})); + +import * as api from '../api'; + +function baseConfig(overrides: Partial = {}): LadderConfig { + return { + cloud_account_id: '11111111-1111-1111-1111-111111111111', + provider: 'aws', + enabled: true, + mode: 'email_approval', + cadence: 'daily', + target_coverage: 100, + buffer_fraction: 0.1, + baseline_percentile: 5, + lookback_days: 30, + buffer_utilization_threshold: 90, + max_hourly_commit_per_run: null, + max_actions_per_run: 10, + ramp_schedule: { steps: [{ after_days: 0, fraction: 1.0 }] }, + updated_at: '2026-01-15T10:00:00Z', + ...overrides, + } as LadderConfig; +} + +// Render the full section (incl. the modal form) into the DOM so the exported +// helpers have the elements they read. +async function renderSection(): Promise { + document.body.innerHTML = '
'; + (api.getLadderConfigs as jest.Mock).mockResolvedValue([]); + await initLadderingSettings(false); +} + +describe('ladder.ts', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockShowToast.mockReset(); + }); + + describe('renderConfigTable XSS escaping', () => { + test('renders a malicious cloud_account_id/provider as inert text, not markup', async () => { + await renderSection(); + const evil = ''; + renderConfigTable([baseConfig({ cloud_account_id: evil, provider: evil })]); + + const container = document.getElementById('ladder-configs-table-container')!; + + // The injected payload must not create a live element anywhere in the + // table (DOM-level assertion; reading serialized innerHTML is unreliable + // because jsdom re-serializes attribute values with raw which are + // inert inside a quoted attribute). + expect(container.querySelector('img')).toBeNull(); + + // The account-id cell holds the payload as TEXT (no child elements => + // it was escaped, not parsed as HTML). + const firstCell = container.querySelector('tbody tr td'); + expect(firstCell?.textContent).toBe(evil); + expect(firstCell?.children.length).toBe(0); + + // Exactly one edit button exists (no extra nodes injected) and it carries + // the value as an inert data-* attribute rather than as markup. + const btns = container.querySelectorAll('button.ladder-edit-btn'); + expect(btns.length).toBe(1); + expect(btns[0]!.dataset['provider']).toBe(evil); + }); + }); + + describe('saveLadderConfig max_hourly guard', () => { + // Fill the modal form with a valid baseline so save reaches the store, + // then override max-hourly per case. + async function primeValidForm(maxHourly: string): Promise { + await renderSection(); + (document.getElementById('ladder-cfg-account') as HTMLInputElement).value = 'acct-1'; + (document.getElementById('ladder-cfg-ramp-schedule') as HTMLTextAreaElement).value = + '{"steps":[{"after_days":0,"fraction":1.0}]}'; + (document.getElementById('ladder-cfg-max-hourly') as HTMLInputElement).value = maxHourly; + } + + test('blank max-hourly serializes to null in the payload', async () => { + await primeValidForm(''); + (api.upsertLadderConfig as jest.Mock).mockResolvedValue(baseConfig()); + + await saveLadderConfig(); + + expect(api.upsertLadderConfig).toHaveBeenCalledTimes(1); + const sent = (api.upsertLadderConfig as jest.Mock).mock.calls[0][0] as LadderConfig; + expect(sent.max_hourly_commit_per_run).toBeNull(); + }); + + test('positive max-hourly passes through unchanged', async () => { + await primeValidForm('5'); + (api.upsertLadderConfig as jest.Mock).mockResolvedValue(baseConfig()); + + await saveLadderConfig(); + + expect(api.upsertLadderConfig).toHaveBeenCalledTimes(1); + const sent = (api.upsertLadderConfig as jest.Mock).mock.calls[0][0] as LadderConfig; + expect(sent.max_hourly_commit_per_run).toBe(5); + }); + + test('non-positive max-hourly is rejected (guarded, no store call)', async () => { + await primeValidForm('-1'); + await saveLadderConfig(); + + expect(api.upsertLadderConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ kind: 'error' }), + ); + }); + }); +}); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index c7c5e93d7..e3fb66f73 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -200,6 +200,10 @@ export { approveRIExchange } from './riexchange'; +// Re-export commitment-laddering functions and types (issue #1336) +export type { LadderConfig, LadderRampStep } from './ladder'; +export { getLadderConfigs, upsertLadderConfig } from './ladder'; + // Re-export registrations functions and types export type { AccountRegistration } from './registrations'; export { diff --git a/frontend/src/api/ladder.ts b/frontend/src/api/ladder.ts new file mode 100644 index 000000000..53beee477 --- /dev/null +++ b/frontend/src/api/ladder.ts @@ -0,0 +1,74 @@ +/** + * Commitment Laddering API functions (issue #1333 phase 3). + * + * The feature is flag-gated default-off: the global kill-switch + * (global_config.laddering_enabled) must be true AND the per-account + * LadderConfig.enabled must be true before any laddering engine run fires. + */ + +import { apiRequest } from './client'; + +/** + * A single ramp step within a ladder ramp schedule. + * AfterDays is the delay from run start; Fraction is the share of the + * total target allocated by this tranche (fractions must sum to 1.0). + */ +export interface LadderRampStep { + after_days: number; + fraction: number; +} + +/** + * Per-account, per-provider ladder configuration. + * + * Mode controls whether runs require human approval before executing: + * email_approval - sends an approval email; purchases fire only after approval + * auto_approve - purchases fire immediately (no human gate) + * + * Cadence controls how often the engine runs: + * daily - once per day + * weekly - once per week + * + * All numeric money fields use number|null rather than 0 so absent/unconfigured + * values are distinguishable from a deliberately configured $0. + */ +export interface LadderConfig { + id?: string; + cloud_account_id: string; + provider: string; + enabled: boolean; + mode: 'email_approval' | 'auto_approve'; + cadence: 'daily' | 'weekly'; + target_coverage: number; + buffer_fraction: number; + baseline_percentile: number; + lookback_days: number; + buffer_utilization_threshold: number; + /** null = no cap on hourly commitment delta per run */ + max_hourly_commit_per_run: number | null; + max_actions_per_run: number; + ramp_schedule: { steps: LadderRampStep[] }; + created_at?: string; + updated_at?: string; +} + +/** + * List all per-account ladder configurations. + * Requires view:config permission. + */ +export async function getLadderConfigs(): Promise { + const resp = await apiRequest<{ configs: LadderConfig[] }>('/ladder/configs'); + return resp.configs ?? []; +} + +/** + * Upsert (insert or update) a per-account ladder configuration. + * The upsert key is (cloud_account_id, provider). + * Requires update:config permission. + */ +export async function upsertLadderConfig(cfg: LadderConfig): Promise { + return apiRequest('/ladder/configs', { + method: 'PUT', + body: JSON.stringify(cfg), + }); +} diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 61168840f..afd50df27 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -304,6 +304,10 @@ export interface Config { // Must be 7, 30, or 60 (AWS LookbackPeriodInDays enum). Default: 7. // GCP CUD Recommender has no equivalent parameter; applies to AWS only. recommendations_lookback_days?: number; + // Global kill-switch for the commitment-laddering feature (issue #1336). + // Default false. When true, per-account LadderConfig.enabled settings + // determine whether the engine runs for that account. + laddering_enabled?: boolean; } export interface ServiceConfig { diff --git a/frontend/src/index.html b/frontend/src/index.html index ae9280bb4..aab91e8e6 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -630,6 +630,9 @@
Cloud Storage
+ +
+
diff --git a/frontend/src/ladder.ts b/frontend/src/ladder.ts new file mode 100644 index 000000000..e1b74ebcb --- /dev/null +++ b/frontend/src/ladder.ts @@ -0,0 +1,529 @@ +/** + * Commitment Laddering settings section (issue #1333 phase 3). + * + * Renders the per-account ladder config editor inside the + * #commitment-laddering-settings placeholder div that lives inside the + * Purchasing Settings panel. The section is flag-gated default-off: + * + * - Global kill-switch: global_config.laddering_enabled (bool) + * - Per-account: LadderConfig.enabled (bool) per (account, provider) pair + * + * No laddering engine runs fire until both flags are true. This PR (phase 3, + * schema foundation) wires the UI to read and write configs; actual engine + * invocation is in a later phase. + */ + +import * as api from './api'; +import { escapeHtml, formatDate } from './utils'; +import { showToast } from './toast'; +import { canAccess } from './permissions'; + +// ========================================== +// MODULE STATE +// ========================================== + +// In-flight guard for the save action. +let saveInFlight = false; + +// Cached configs from the last successful load. +let cachedConfigs: api.LadderConfig[] = []; + +// ========================================== +// PUBLIC API +// ========================================== + +/** + * Initialize the Commitment Laddering settings section. + * + * Renders the HTML form into #commitment-laddering-settings and populates + * it with data from the API. Should be called from loadGlobalSettings after + * the global config response is available. + * + * The globalEnabled parameter is the current value of + * global_config.laddering_enabled so the kill-switch toggle reflects the + * persisted state without a second API call. + */ +export async function initLadderingSettings(globalEnabled: boolean): Promise { + const container = document.getElementById('commitment-laddering-settings'); + if (!container) return; + + container.innerHTML = renderLadderingSection(globalEnabled); + wireKillSwitchToggle(); + wireModalCloseButtons(); + + try { + cachedConfigs = await api.getLadderConfigs(); + renderConfigTable(cachedConfigs); + } catch (err) { + console.error('Failed to load ladder configs:', err); + const tableContainer = document.getElementById('ladder-configs-table-container'); + if (tableContainer) { + tableContainer.innerHTML = '

Failed to load per-account configurations.

'; + } + } + + if (canAccess('update', 'config')) { + document.getElementById('ladder-add-config-btn')?.addEventListener('click', () => openLadderConfigModal()); + } +} + +// ========================================== +// RENDERING +// ========================================== + +function renderLadderingSection(globalEnabled: boolean): string { + const canEdit = canAccess('update', 'config'); + const disabledAttr = canEdit ? '' : ' disabled'; + + return ` +
+ Commitment Laddering +

+ Commitment Laddering automatically ramps up cloud commitments towards a + target coverage level using a configurable ramp schedule. It is feature-gated: + enable the global kill-switch below, then enable individual accounts via the + per-account configuration table. +

+ +
+
+ + Global kill-switch. Must be on before any per-account config can fire. +
+
+ +
+
+ +
+

Per-Account Configurations

+
+

Loading…

+
+ ${canEdit ? ` + ` : ''} +
+
+ + + +`; +} + +// Exported for unit testing (mirrors the apikeys.ts convention of exposing +// render/action helpers so tests can drive them directly). +export function renderConfigTable(configs: api.LadderConfig[]): void { + const container = document.getElementById('ladder-configs-table-container'); + if (!container) return; + + if (configs.length === 0) { + container.innerHTML = '

No per-account configurations yet. Click Add Account Config to create one.

'; + return; + } + + const canEdit = canAccess('update', 'config'); + const rows = configs.map(cfg => ` + + ${escapeHtml(cfg.cloud_account_id)} + ${escapeHtml(cfg.provider.toUpperCase())} + ${cfg.enabled ? 'Yes' : 'No'} + ${escapeHtml(cfg.mode === 'email_approval' ? 'Email Approval' : 'Auto Approve')} + ${escapeHtml(cfg.cadence === 'daily' ? 'Daily' : 'Weekly')} + ${cfg.target_coverage.toFixed(1)}% + ${cfg.updated_at ? escapeHtml(formatDate(cfg.updated_at)) : 'N/A'} + ${canEdit + ? `` + : ''} + + `).join(''); + + container.innerHTML = ` + + + + + + + + + + + + + + ${rows} +
Account IDProviderEnabledModeCadenceTarget CoverageUpdated
+ `; + + // Wire edit buttons via querySelectorAll so no user data is ever injected + // into a JS string context (data-* attributes are HTML-attribute-context only). + container.querySelectorAll('.ladder-edit-btn').forEach(btn => { + btn.addEventListener('click', () => { + const accountID = btn.dataset['accountId'] ?? ''; + const provider = btn.dataset['provider'] ?? ''; + const cfg = cachedConfigs.find(c => c.cloud_account_id === accountID && c.provider === provider); + if (cfg) openLadderConfigModal(cfg); + }); + }); +} + +// ========================================== +// MODAL CLOSE WIRING +// ========================================== + +// closeLadderModal hides the per-account config modal. +function closeLadderModal(): void { + document.getElementById('ladder-config-modal')?.classList.add('hidden'); +} + +// wireModalCloseButtons attaches click listeners to the modal's × and Cancel +// buttons. Uses addEventListener rather than inline onclick for CSP +// consistency with the dynamically-wired edit buttons (no inline handlers +// anywhere in this module). +function wireModalCloseButtons(): void { + document.getElementById('ladder-modal-close-btn')?.addEventListener('click', closeLadderModal); + document.getElementById('ladder-modal-cancel-btn')?.addEventListener('click', closeLadderModal); +} + +// ========================================== +// KILL-SWITCH TOGGLE +// ========================================== + +function wireKillSwitchToggle(): void { + const toggle = document.getElementById('setting-laddering-enabled') as HTMLInputElement | null; + if (!toggle || !canAccess('update', 'config')) return; + + toggle.addEventListener('change', async () => { + if (saveInFlight) { + toggle.checked = !toggle.checked; // revert optimistic + return; + } + saveInFlight = true; + try { + const globalCfg = await fetchGlobalConfigForLaddering(); + if (globalCfg === null) { + toggle.checked = !toggle.checked; + return; + } + globalCfg.laddering_enabled = toggle.checked; + await api.updateConfig(globalCfg); + showToast({ + message: `Commitment Laddering ${toggle.checked ? 'enabled' : 'disabled'} globally.`, + kind: 'success', + }); + } catch (err) { + toggle.checked = !toggle.checked; // revert on error + showToast({ message: 'Failed to update laddering kill-switch.', kind: 'error' }); + console.error('Failed to toggle laddering_enabled:', err); + } finally { + saveInFlight = false; + } + }); +} + +/** + * Fetches the global config for the kill-switch toggle so we can merge the + * single field change without overwriting other fields. Returns null on error + * (error is shown via toast before returning). + */ +async function fetchGlobalConfigForLaddering(): Promise { + try { + const resp = await api.getConfig(); + const g = resp.global; + if (!g) { + showToast({ message: 'Could not load global config.', kind: 'error' }); + return null; + } + // Build a Config-shaped payload from GlobalConfig fields that are safe to + // round-trip (the union of GlobalConfig and Config). We pass only the + // fields the backend's updateConfig handler accepts. + // + // IMPORTANT: include every field the backend stores, not just the one we + // are changing. GlobalConfig.NotificationEmail is a *string in Go: omitting + // it from the PUT body decodes as nil and zeroes it out in the DB. + return { + enabled_providers: (g.enabled_providers ?? []) as api.Provider[], + notification_email: g.notification_email, + auto_collect: g.auto_collect ?? true, + collection_schedule: g.collection_schedule ?? 'daily', + notification_days_before: g.notification_days_before ?? 3, + default_term: g.default_term ?? 3, + default_payment: (g.default_payment ?? 'all-upfront') as api.PaymentOption, + default_coverage: g.default_coverage ?? 80, + grace_period_days: g.grace_period_days, + recommendations_cache_stale_hours: g.recommendations_cache_stale_hours, + recommendations_lookback_days: g.recommendations_lookback_days, + laddering_enabled: g.laddering_enabled ?? false, + }; + } catch (err) { + showToast({ message: 'Failed to load global config.', kind: 'error' }); + console.error('fetchGlobalConfigForLaddering:', err); + return null; + } +} + +// ========================================== +// PER-ACCOUNT CONFIG MODAL +// ========================================== + +function openLadderConfigModal(existing?: api.LadderConfig): void { + const modal = document.getElementById('ladder-config-modal'); + if (!modal) return; + + // Populate modal fields. + setValue('ladder-cfg-id', existing?.id ?? ''); + setValue('ladder-cfg-account', existing?.cloud_account_id ?? ''); + setSelectValue('ladder-cfg-provider', existing?.provider ?? 'aws'); + setChecked('ladder-cfg-enabled', existing?.enabled ?? false); + setSelectValue('ladder-cfg-mode', existing?.mode ?? 'email_approval'); + setSelectValue('ladder-cfg-cadence', existing?.cadence ?? 'daily'); + setValue('ladder-cfg-target-coverage', String(existing?.target_coverage ?? 100)); + setValue('ladder-cfg-buffer-fraction', String(existing?.buffer_fraction ?? 0.10)); + setValue('ladder-cfg-baseline-percentile', String(existing?.baseline_percentile ?? 5.0)); + setValue('ladder-cfg-lookback-days', String(existing?.lookback_days ?? 30)); + setValue('ladder-cfg-buf-util-threshold', String(existing?.buffer_utilization_threshold ?? 90)); + setValue('ladder-cfg-max-hourly', existing?.max_hourly_commit_per_run != null + ? String(existing.max_hourly_commit_per_run) + : ''); + setValue('ladder-cfg-max-actions', String(existing?.max_actions_per_run ?? 10)); + + const defaultRamp = JSON.stringify({ steps: [{ after_days: 0, fraction: 1.0 }] }, null, 2); + setValue('ladder-cfg-ramp-schedule', + existing?.ramp_schedule ? JSON.stringify(existing.ramp_schedule, null, 2) : defaultRamp); + + // Disable account/provider fields when editing an existing row (they form + // the upsert key and cannot be changed without a delete+re-create). + const accountInput = document.getElementById('ladder-cfg-account') as HTMLInputElement | null; + const providerSelect = document.getElementById('ladder-cfg-provider') as HTMLSelectElement | null; + if (accountInput) accountInput.readOnly = !!existing; + if (providerSelect) providerSelect.disabled = !!existing; + + // Wire the save button (remove previous listener by replacing the element + // clone so duplicate-listener accumulation cannot occur). + const form = document.getElementById('ladder-config-form'); + if (form) { + const newForm = form.cloneNode(true) as HTMLElement; + form.replaceWith(newForm); + newForm.addEventListener('submit', (e) => { + e.preventDefault(); + saveLadderConfig(); + }); + // The Cancel button lives inside the form, so cloneNode drops its + // listener (cloneNode does not copy event handlers). Re-wire it on the + // fresh node. The × close button sits outside the form and keeps the + // listener attached once in wireModalCloseButtons. + newForm.querySelector('#ladder-modal-cancel-btn') + ?.addEventListener('click', closeLadderModal); + } + + modal.classList.remove('hidden'); +} + +// Exported for unit testing (see renderConfigTable note). +export async function saveLadderConfig(): Promise { + if (saveInFlight) return; + + const maxHourlyRaw = (document.getElementById('ladder-cfg-max-hourly') as HTMLInputElement | null)?.value?.trim() ?? ''; + const maxHourly: number | null = maxHourlyRaw === '' ? null : Number(maxHourlyRaw); + if (maxHourlyRaw !== '' && (!Number.isFinite(maxHourly) || (maxHourly as number) <= 0)) { + showToast({ message: 'Max hourly commit must be a positive number (or blank for no cap).', kind: 'error' }); + return; + } + + const rampRaw = (document.getElementById('ladder-cfg-ramp-schedule') as HTMLTextAreaElement | null)?.value?.trim() ?? ''; + let rampSchedule: api.LadderConfig['ramp_schedule']; + try { + rampSchedule = JSON.parse(rampRaw); + } catch { + showToast({ message: 'Ramp schedule is not valid JSON.', kind: 'error' }); + return; + } + + const cfg: api.LadderConfig = { + id: (document.getElementById('ladder-cfg-id') as HTMLInputElement | null)?.value || undefined, + cloud_account_id: (document.getElementById('ladder-cfg-account') as HTMLInputElement | null)?.value?.trim() ?? '', + provider: (document.getElementById('ladder-cfg-provider') as HTMLSelectElement | null)?.value ?? 'aws', + enabled: (document.getElementById('ladder-cfg-enabled') as HTMLInputElement | null)?.checked ?? false, + mode: ((document.getElementById('ladder-cfg-mode') as HTMLSelectElement | null)?.value ?? 'email_approval') as api.LadderConfig['mode'], + cadence: ((document.getElementById('ladder-cfg-cadence') as HTMLSelectElement | null)?.value ?? 'daily') as api.LadderConfig['cadence'], + target_coverage: Number((document.getElementById('ladder-cfg-target-coverage') as HTMLInputElement | null)?.value ?? '100'), + buffer_fraction: Number((document.getElementById('ladder-cfg-buffer-fraction') as HTMLInputElement | null)?.value ?? '0.10'), + baseline_percentile: Number((document.getElementById('ladder-cfg-baseline-percentile') as HTMLInputElement | null)?.value ?? '5'), + lookback_days: Number((document.getElementById('ladder-cfg-lookback-days') as HTMLInputElement | null)?.value ?? '30'), + buffer_utilization_threshold: Number((document.getElementById('ladder-cfg-buf-util-threshold') as HTMLInputElement | null)?.value ?? '90'), + max_hourly_commit_per_run: maxHourly, + max_actions_per_run: Number((document.getElementById('ladder-cfg-max-actions') as HTMLInputElement | null)?.value ?? '10'), + ramp_schedule: rampSchedule, + }; + + if (!cfg.cloud_account_id) { + showToast({ message: 'Cloud Account ID is required.', kind: 'error' }); + return; + } + + saveInFlight = true; + const saveBtn = document.getElementById('ladder-config-save-btn') as HTMLButtonElement | null; + if (saveBtn) saveBtn.disabled = true; + + try { + const saved = await api.upsertLadderConfig(cfg); + + // Update the in-memory cache and re-render the table. + const idx = cachedConfigs.findIndex( + c => c.cloud_account_id === saved.cloud_account_id && c.provider === saved.provider + ); + if (idx >= 0) { + cachedConfigs[idx] = saved; + } else { + cachedConfigs.push(saved); + } + renderConfigTable(cachedConfigs); + + closeLadderModal(); + showToast({ message: 'Ladder config saved.', kind: 'success' }); + } catch (err) { + showToast({ message: 'Failed to save ladder config. Check inputs and try again.', kind: 'error' }); + console.error('saveLadderConfig:', err); + } finally { + saveInFlight = false; + if (saveBtn) saveBtn.disabled = false; + } +} + +// ========================================== +// DOM HELPERS +// ========================================== + +function setValue(id: string, value: string): void { + const el = document.getElementById(id) as HTMLInputElement | HTMLTextAreaElement | null; + if (el) el.value = value; +} + +function setSelectValue(id: string, value: string): void { + const el = document.getElementById(id) as HTMLSelectElement | null; + if (el) el.value = value; +} + +function setChecked(id: string, checked: boolean): void { + const el = document.getElementById(id) as HTMLInputElement | null; + if (el) el.checked = checked; +} diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 249cbe0e3..464095141 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -19,6 +19,7 @@ import { readServiceFilterControls, isServiceFilterError, } from './serviceFilters'; +import { initLadderingSettings } from './ladder'; /** * Issue #196 — after any mutation to an `account_service_overrides` row @@ -3165,6 +3166,11 @@ export async function loadGlobalSettings(): Promise { // Update visibility based on loaded settings updateProviderSettingsVisibility(); updateCollectionScheduleVisibility(); + + // Render the Commitment Laddering section (flag-gated default-off, + // issue #1333 phase 3). Runs asynchronously; failures are surfaced + // inside the section without blocking the rest of Settings. + void initLadderingSettings(data.global.laddering_enabled ?? false); } const services = data.services ?? []; @@ -3435,6 +3441,15 @@ export async function saveGlobalSettings(e: Event): Promise { recommendations_lookback_days: parseInt(byId('setting-recs-lookback-days')?.value || '7', 10), }; + // Include laddering_enabled in the payload when the Purchasing panel's + // toggle is present in the DOM (i.e. initLadderingSettings has run). + // When absent (General panel), the backend preserves the existing DB value + // via preserveOmittedRecommendationFields. + const ladderingToggle = byId('setting-laddering-enabled'); + if (ladderingToggle !== null) { + settings.laddering_enabled = ladderingToggle.checked; + } + try { await api.updateConfig(settings); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index c04d39308..add694e1f 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -367,6 +367,11 @@ export interface GlobalConfig { recommendations_cache_stale_hours?: number; // AWS Cost Explorer lookback window (days). One of 7, 30, or 60. Default: 7. recommendations_lookback_days?: number; + // Global kill-switch for the commitment-laddering feature (issue #1333 phase 3). + // When false (the default) no laddering engine runs fire, regardless of + // per-account LadderConfig settings. Set to true to allow per-account + // configs to activate individually. + laddering_enabled?: boolean; } // API Keys types diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 5274c1548..7ce142fb8 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -737,3 +737,13 @@ func (m *mockConfigStore) GetPendingExecutionsTx(ctx context.Context, _ pgx.Tx) return m.GetPendingExecutions(ctx) } func (m *mockConfigStore) WithTx(_ context.Context, fn func(tx pgx.Tx) error) error { return fn(nil) } + +func (m *mockConfigStore) GetLadderConfigs(_ context.Context) ([]config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStore) GetLadderConfig(_ context.Context, _, _ string) (*config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStore) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + return cfg, nil +} diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 615373d5d..b55869af3 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -68,6 +68,18 @@ func (h *Handler) getConfig(ctx context.Context, req *events.LambdaFunctionURLRe // matching the pre-fix behaviour. Extracted from updateConfig to keep that // function under the cyclomatic-complexity gate after the merge logic was // added (PR #308 CodeRabbit pass-2 review). +// allKeysPresent reports whether every key is present in m. Extracted so +// callers can express an all-present short-circuit without an N-way boolean +// conjunction (which trips the cyclomatic-complexity gate). +func allKeysPresent(m map[string]json.RawMessage, keys ...string) bool { + for _, k := range keys { + if _, ok := m[k]; !ok { + return false + } + } + return true +} + func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg *config.GlobalConfig, body string) error { var present map[string]json.RawMessage if err := json.Unmarshal([]byte(body), &present); err != nil { @@ -76,7 +88,18 @@ func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg * _, hasStale := present["recommendations_cache_stale_hours"] _, hasLookback := present["recommendations_lookback_days"] _, hasDelay := present["purchase_delay_hours"] - if hasStale && hasLookback && hasDelay { + // laddering_enabled is only present in the PUT body when the Purchasing + // panel is active (the General panel form omits it). Preserve the existing + // DB value when absent so a General-panel save cannot silently reset the + // kill-switch back to false. + _, hasLaddering := present["laddering_enabled"] + // Skip the DB read when every preserved key is already supplied. + if allKeysPresent(present, + "recommendations_cache_stale_hours", + "recommendations_lookback_days", + "purchase_delay_hours", + "laddering_enabled", + ) { return nil } existing, gcErr := h.config.GetGlobalConfig(ctx) @@ -92,6 +115,9 @@ func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg * if !hasDelay { cfg.PurchaseDelayHours = existing.PurchaseDelayHours } + if !hasLaddering { + cfg.LadderingEnabled = existing.LadderingEnabled + } return nil } diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 547ed1f21..82b3601b5 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "fmt" "testing" "github.com/LeanerCloud/CUDly/internal/config" @@ -87,6 +88,90 @@ func TestHandler_updateConfig(t *testing.T) { assert.Equal(t, "updated", result.Status) } +// TestHandler_updateConfig_LadderingEnabledPreservation asserts the +// kill-switch preservation contract added in the ladder-schema PR: +// - a PUT that OMITS laddering_enabled must NOT reset the persisted value +// (the General-settings panel never sends the field, so a plain save must +// leave a previously-enabled kill-switch on). +// - a PUT that sends laddering_enabled:false must flip it off. +// +// Both cases capture the *config.GlobalConfig handed to SaveGlobalConfig and +// assert on the final LadderingEnabled value the store would persist. +func TestHandler_updateConfig_LadderingEnabledPreservation(t *testing.T) { + // newHandler builds a handler whose GetGlobalConfig returns a config seeded + // with existingLaddering, and captures the config handed to SaveGlobalConfig + // into the returned pointer so callers can assert the persisted value. + newHandler := func(t *testing.T, existingLaddering bool) (*Handler, *config.GlobalConfig) { + t.Helper() + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + adminSession := &Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: "admin@example.com", + } + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + // The body always omits the recommendations fields, so updateConfig + // fetches the existing config to preserve them; seed LadderingEnabled + // there so the preservation branch has a concrete value to keep. + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RecommendationsCacheStaleHours: config.DefaultRecommendationsCacheStaleHours, + RecommendationsLookbackDays: config.DefaultRecommendationsLookbackDays, + LadderingEnabled: existingLaddering, + }, nil) + mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { + saved = *args.Get(1).(*config.GlobalConfig) + }).Return(nil) + + return &Handler{config: mockStore, auth: mockAuth}, &saved + } + + makeReq := func(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + } + + const bodyOmitted = `{"enabled_providers": ["aws"], "default_term": 3}` + bodyWith := func(v bool) string { + return fmt.Sprintf(`{"enabled_providers": ["aws"], "default_term": 3, "laddering_enabled": %t}`, v) + } + + cases := []struct { + name string + body string + existing bool + wantPersisted bool + }{ + {"omitted preserves existing true", bodyOmitted, true, true}, + {"omitted preserves existing false", bodyOmitted, false, false}, + {"explicit false flips existing true off", bodyWith(false), true, false}, + {"explicit true flips existing false on", bodyWith(true), false, true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + handler, saved := newHandler(t, tc.existing) + + result, err := handler.updateConfig(ctx, makeReq(tc.body)) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + assert.Equal(t, tc.wantPersisted, saved.LadderingEnabled, + "persisted laddering_enabled mismatch for case %q", tc.name) + }) + } +} + func TestHandler_updateConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go new file mode 100644 index 000000000..45168fae4 --- /dev/null +++ b/internal/api/handler_ladder.go @@ -0,0 +1,107 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/aws/aws-lambda-go/events" +) + +// ========================================== +// LADDER CONFIG HANDLERS +// ========================================== + +// getLadderConfigs returns all per-account ladder configurations. +// Requires view:config permission. +func (h *Handler) getLadderConfigs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + if _, err := h.requirePermission(ctx, req, "view", "config"); err != nil { + return nil, err + } + + configs, err := h.config.GetLadderConfigs(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list ladder configs: %w", err) + } + + return map[string]any{"configs": configs}, nil +} + +// upsertLadderConfig inserts or updates the per-account ladder configuration +// for the given (cloud_account_id, provider) pair extracted from the request +// body. Requires update:config permission. +// +// Auth dispatch mirrors handler_ri_exchange.go (session RBAC only; no token +// path exists for config writes). requirePermission denies the request when +// the auth component is nil (returns an error, never a session), so the +// handler fails closed; the exact status is a 500-class error in that case +// rather than a 403, but no unauthenticated write ever reaches the store. +func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil { + return nil, err + } + + var cfg config.LadderConfigDB + if err := json.Unmarshal([]byte(req.Body), &cfg); err != nil { + return nil, NewClientError(400, "invalid request body") + } + + if cfg.CloudAccountID == "" { + return nil, NewClientError(400, "cloud_account_id is required") + } + if cfg.Provider == "" { + return nil, NewClientError(400, "provider is required") + } + + // Presence map to distinguish an OMITTED field from an explicitly-supplied + // value, so defaults apply only to genuinely-absent keys (see + // applyLadderConfigNumericDefaults). + var present map[string]json.RawMessage + if err := json.Unmarshal([]byte(req.Body), &present); err != nil { + return nil, NewClientError(400, "invalid request body") + } + applyLadderConfigNumericDefaults(&cfg, present) + + if err := cfg.Validate(); err != nil { + return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) + } + + result, err := h.config.UpsertLadderConfig(ctx, &cfg) + if err != nil { + return nil, fmt.Errorf("failed to upsert ladder config: %w", err) + } + + return result, nil +} + +// applyLadderConfigNumericDefaults fills a numeric field with its default ONLY +// when the field's key is genuinely absent from the request body (tracked via +// present). When a key IS supplied, the caller's value is left untouched so it +// flows to Validate(), which rejects any out-of-range value -- including an +// explicit 0 on the five fields whose valid range excludes 0 -- with a 400. A +// bare `== 0` check would instead silently rewrite an explicit out-of-range 0 +// to the default, the forbidden silent-fallback pattern on a money-adjacent +// config path (feedback_no_silent_fallbacks). buffer_fraction's valid range is +// [0, 1), so an explicit 0 there is a legitimate "no buffer" choice and is +// likewise passed through untouched. +func applyLadderConfigNumericDefaults(cfg *config.LadderConfigDB, present map[string]json.RawMessage) { + if _, ok := present["target_coverage"]; !ok { + cfg.TargetCoverage = config.DefaultLadderTargetCoverage + } + if _, ok := present["buffer_fraction"]; !ok { + cfg.BufferFraction = config.DefaultLadderBufferFraction + } + if _, ok := present["baseline_percentile"]; !ok { + cfg.BaselinePercentile = config.DefaultLadderBaselinePercentile + } + if _, ok := present["lookback_days"]; !ok { + cfg.LookbackDays = config.DefaultLadderLookbackDays + } + if _, ok := present["buffer_utilization_threshold"]; !ok { + cfg.BufferUtilizationThreshold = config.DefaultLadderBufferUtilThreshold + } + if _, ok := present["max_actions_per_run"]; !ok { + cfg.MaxActionsPerRun = config.DefaultLadderMaxActionsPerRun + } +} diff --git a/internal/api/handler_ladder_test.go b/internal/api/handler_ladder_test.go new file mode 100644 index 000000000..93707ed68 --- /dev/null +++ b/internal/api/handler_ladder_test.go @@ -0,0 +1,96 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// ladderReq builds an authenticated PUT request carrying body. +func ladderReq(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } +} + +// newLadderHandler wires a Handler with an admin session and returns the mock +// store plus a pointer that captures the config handed to UpsertLadderConfig +// (nil until the store is actually reached). +func newLadderHandler(t *testing.T) (*Handler, *MockConfigStore, **config.LadderConfigDB) { + t.Helper() + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + var captured *config.LadderConfigDB + // .Maybe() so the 400-path test (store never reached) doesn't trip + // AssertExpectations on an unmet expectation. The success tests prove the + // store was reached by asserting *captured is non-nil (set only in Run). + mockStore.On("UpsertLadderConfig", ctx, mock.AnythingOfType("*config.LadderConfigDB")). + Run(func(args mock.Arguments) { + captured = args.Get(1).(*config.LadderConfigDB) + }). + Return(&config.LadderConfigDB{}, nil). + Maybe() + + return &Handler{config: mockStore, auth: mockAuth}, mockStore, &captured +} + +const ladderValidRamp = `"ramp_schedule":{"steps":[{"after_days":0,"fraction":1.0}]}` + +// TestUpsertLadderConfig_BufferFractionExplicitZeroSurvives asserts that an +// explicit buffer_fraction:0 ("no buffer") reaches the store unchanged rather +// than being silently rewritten to DefaultLadderBufferFraction. +func TestUpsertLadderConfig_BufferFractionExplicitZeroSurvives(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","buffer_fraction":0,` + ladderValidRamp + `}` + _, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, *captured, "store should have been reached") + assert.Equal(t, 0.0, (*captured).BufferFraction, + "explicit buffer_fraction:0 must survive to the store unchanged") +} + +// TestUpsertLadderConfig_BufferFractionOmittedDefaults asserts that an omitted +// buffer_fraction key defaults to DefaultLadderBufferFraction. +func TestUpsertLadderConfig_BufferFractionOmittedDefaults(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + _, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, *captured, "store should have been reached") + assert.Equal(t, config.DefaultLadderBufferFraction, (*captured).BufferFraction, + "omitted buffer_fraction must default to DefaultLadderBufferFraction") +} + +// TestUpsertLadderConfig_ExplicitZeroTargetCoverageRejected asserts the FIX 1 +// contract: an explicit out-of-range target_coverage:0 returns 400 and never +// reaches the store (it is not silently defaulted). +func TestUpsertLadderConfig_ExplicitZeroTargetCoverageRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","target_coverage":0,` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} diff --git a/internal/api/router.go b/internal/api/router.go index e41f9e406..f69a00dc6 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -308,6 +308,13 @@ func (r *Router) registerRoutes() { {PathPrefix: "/api/ri-exchange/approve/", Method: "POST", Handler: r.approveRIExchangeHandler, Auth: AuthPublic}, {PathPrefix: "/api/ri-exchange/reject/", Method: "POST", Handler: r.rejectRIExchangeHandler, Auth: AuthPublic}, + // Commitment Laddering endpoints (flag-gated default-off, issue #1336). + // GET returns all per-account ladder configs; PUT inserts or updates one. + // Both routes require update:config / view:config (checked inside the + // handler), consistent with the RI Exchange config precedent. + {ExactPath: "/api/ladder/configs", Method: "GET", Handler: r.getLadderConfigsHandler, Auth: AuthUser}, + {ExactPath: "/api/ladder/configs", Method: "PUT", Handler: r.upsertLadderConfigHandler, Auth: AuthUser}, + // Account self-registration (public, called by Terraform during federation IaC apply) {ExactPath: "/api/register", Method: "POST", Handler: r.submitRegistrationHandler, Auth: AuthPublic}, {PathPrefix: "/api/register/", Method: "GET", Handler: r.getRegistrationStatusHandler, Auth: AuthPublic}, @@ -906,3 +913,13 @@ func (r *Router) listPlanAccountsHandler(ctx context.Context, req *events.Lambda func (r *Router) setPlanAccountsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.setPlanAccounts(ctx, req, params["id"]) } + +// Commitment Laddering route wrappers (issue #1336). + +func (r *Router) getLadderConfigsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.getLadderConfigs(ctx, req) +} + +func (r *Router) upsertLadderConfigHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return r.h.upsertLadderConfig(ctx, req) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 51b2bf5ef..970e904d3 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -314,4 +314,14 @@ type StoreInterface interface { // participate in the transaction. Nested transactions are not // supported — fn must not call WithTx recursively. WithTx(ctx context.Context, fn func(tx pgx.Tx) error) error + + // Ladder configuration (per-account, per-provider). + // GetLadderConfigs returns all rows, newest first. + // GetLadderConfig returns the single row for (cloudAccountID, provider), + // or (nil, nil) when no row exists. + // UpsertLadderConfig inserts or updates via the UNIQUE(cloud_account_id, provider) + // constraint and returns the persisted row with all DB-stamped fields populated. + GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, error) + GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*LadderConfigDB, error) + UpsertLadderConfig(ctx context.Context, cfg *LadderConfigDB) (*LadderConfigDB, error) } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 5317234b4..a889cc87b 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -52,7 +52,8 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - COALESCE(purchase_delay_hours, 0) + COALESCE(purchase_delay_hours, 0), + COALESCE(laddering_enabled, false) FROM global_config WHERE id = 1 ` @@ -82,6 +83,7 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err &config.RecommendationsCacheStaleHours, &config.RecommendationsLookbackDays, &config.PurchaseDelayHours, + &config.LadderingEnabled, ) if err != nil { @@ -136,8 +138,8 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - purchase_delay_hours - ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) + purchase_delay_hours, laddering_enabled + ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21) ON CONFLICT (id) DO UPDATE SET enabled_providers = $1, notification_email = $2, @@ -159,6 +161,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf recommendations_cache_stale_hours = $18, recommendations_lookback_days = $19, purchase_delay_hours = $20, + laddering_enabled = $21, updated_at = NOW() ` @@ -215,6 +218,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf config.RecommendationsCacheStaleHours, recommendationsLookbackDays, config.PurchaseDelayHours, + config.LadderingEnabled, ) if err != nil { diff --git a/internal/config/store_postgres_ladder.go b/internal/config/store_postgres_ladder.go new file mode 100644 index 000000000..5d3290312 --- /dev/null +++ b/internal/config/store_postgres_ladder.go @@ -0,0 +1,192 @@ +package config + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" +) + +// ========================================== +// LADDER CONFIG STORE METHODS +// ========================================== + +// GetLadderConfigs returns all ladder_configs rows, newest first. +// Returns an empty slice (not nil) when no rows exist. +func (s *PostgresStore) GetLadderConfigs(ctx context.Context) ([]LadderConfigDB, error) { + query := ` + SELECT id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + FROM ladder_configs + ORDER BY created_at DESC + ` + rows, err := s.db.Query(ctx, query) + if err != nil { + return nil, fmt.Errorf("failed to query ladder_configs: %w", err) + } + defer rows.Close() + + var configs []LadderConfigDB + for rows.Next() { + cfg, err := scanLadderConfig(rows) + if err != nil { + return nil, fmt.Errorf("failed to scan ladder_config row: %w", err) + } + configs = append(configs, cfg) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("error iterating ladder_config rows: %w", err) + } + if configs == nil { + configs = []LadderConfigDB{} + } + return configs, nil +} + +// GetLadderConfig returns the ladder_config row for the given +// (cloud_account_id, provider) pair. Returns (nil, nil) when no row exists. +func (s *PostgresStore) GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*LadderConfigDB, error) { + query := ` + SELECT id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + FROM ladder_configs + WHERE cloud_account_id = $1 AND provider = $2 + ` + row := s.db.QueryRow(ctx, query, cloudAccountID, provider) + cfg, err := scanLadderConfig(row) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil + } + return nil, fmt.Errorf("failed to get ladder_config for account=%s provider=%s: %w", cloudAccountID, provider, err) + } + return &cfg, nil +} + +// UpsertLadderConfig inserts or updates the per-account ladder configuration. +// The upsert key is (cloud_account_id, provider). If ID is empty a new UUID +// is generated; existing rows retain their original id and created_at. +// +// Validate() must be called by the API handler before this method; the store +// does not re-validate to avoid duplicating error messages. +func (s *PostgresStore) UpsertLadderConfig(ctx context.Context, cfg *LadderConfigDB) (*LadderConfigDB, error) { + if cfg.ID == "" { + cfg.ID = uuid.New().String() + } + + rampJSON, err := marshalRampSchedule(cfg.RampSchedule) + if err != nil { + return nil, fmt.Errorf("failed to marshal ramp_schedule: %w", err) + } + + query := ` + INSERT INTO ladder_configs ( + id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, NOW(), NOW()) + ON CONFLICT (cloud_account_id, provider) DO UPDATE SET + enabled = $4, + mode = $5, + cadence = $6, + target_coverage = $7, + buffer_fraction = $8, + baseline_percentile = $9, + lookback_days = $10, + buffer_utilization_threshold = $11, + max_hourly_commit_per_run = $12, + max_actions_per_run = $13, + ramp_schedule = $14, + updated_at = NOW() + RETURNING id, cloud_account_id, provider, enabled, mode, cadence, + target_coverage, buffer_fraction, baseline_percentile, + lookback_days, buffer_utilization_threshold, + max_hourly_commit_per_run, max_actions_per_run, + ramp_schedule, created_at, updated_at + ` + + row := s.db.QueryRow(ctx, query, + cfg.ID, + cfg.CloudAccountID, + cfg.Provider, + cfg.Enabled, + cfg.Mode, + cfg.Cadence, + cfg.TargetCoverage, + cfg.BufferFraction, + cfg.BaselinePercentile, + cfg.LookbackDays, + cfg.BufferUtilizationThreshold, + cfg.MaxHourlyCommitPerRun, + cfg.MaxActionsPerRun, + rampJSON, + ) + result, err := scanLadderConfig(row) + if err != nil { + return nil, fmt.Errorf("failed to upsert ladder_config for account=%s provider=%s: %w", + cfg.CloudAccountID, cfg.Provider, err) + } + return &result, nil +} + +// scanLadderConfig scans a single ladder_configs row from either a pgx.Row +// or pgx.Rows. Both types satisfy the scannable interface (declared in +// store_postgres_registrations.go). This helper exists to avoid duplicating +// the 16-column scan logic. +func scanLadderConfig(row scannable) (LadderConfigDB, error) { + var cfg LadderConfigDB + var rampJSON []byte + var createdAt, updatedAt time.Time + + err := row.Scan( + &cfg.ID, + &cfg.CloudAccountID, + &cfg.Provider, + &cfg.Enabled, + &cfg.Mode, + &cfg.Cadence, + &cfg.TargetCoverage, + &cfg.BufferFraction, + &cfg.BaselinePercentile, + &cfg.LookbackDays, + &cfg.BufferUtilizationThreshold, + &cfg.MaxHourlyCommitPerRun, + &cfg.MaxActionsPerRun, + &rampJSON, + &createdAt, + &updatedAt, + ) + if err != nil { + return LadderConfigDB{}, err + } + cfg.CreatedAt = createdAt + cfg.UpdatedAt = updatedAt + cfg.RampSchedule = json.RawMessage(rampJSON) + return cfg, nil +} + +// marshalRampSchedule ensures the ramp_schedule value stored in the DB is +// valid JSON. Nil/empty input is rejected; already-valid JSON is passed +// through unchanged. Non-JSON input is rejected with a descriptive error. +func marshalRampSchedule(raw json.RawMessage) ([]byte, error) { + if len(raw) == 0 { + return nil, fmt.Errorf("ramp_schedule must not be empty") + } + // Validate it is parseable JSON before writing. + if !json.Valid(raw) { + return nil, fmt.Errorf("ramp_schedule is not valid JSON") + } + return raw, nil +} diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index ae157c24a..d5e047023 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -57,6 +57,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "grace_period_days", "recommendations_cache_stale_hours", "recommendations_lookback_days", "purchase_delay_hours", + "laddering_enabled", } rows := pgxmock.NewRows(cols).AddRow( []string{"aws"}, strPtr("ops@example.com"), true, @@ -67,6 +68,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "{}", 24, 7, 0, + false, ) mock.ExpectQuery("SELECT").WillReturnRows(rows) @@ -107,6 +109,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { "grace_period_days", "recommendations_cache_stale_hours", "recommendations_lookback_days", "purchase_delay_hours", + "laddering_enabled", } baseRow := func(graceJSON string) []any { return []any{ @@ -118,6 +121,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { graceJSON, 24, 7, 0, + false, } } diff --git a/internal/config/types.go b/internal/config/types.go index 418ce95a9..962bcce76 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -5,6 +5,8 @@ import ( "encoding/json" "strings" "time" + + "github.com/LeanerCloud/CUDly/pkg/ladder" ) // GlobalConfig represents the global CUDly configuration @@ -61,6 +63,12 @@ type GlobalConfig struct { // 0 means immediate-execute (backward compat). Valid range: [0, 168]. // Default: 48. PurchaseDelayHours int `json:"purchase_delay_hours" db:"purchase_delay_hours"` + + // LadderingEnabled is the global kill-switch for the commitment-laddering + // feature (issue #1333 phase 3). When false (the default), no laddering + // engine runs fire regardless of per-account LadderConfig.Enabled settings. + // Set to true to allow per-account configs to activate individually. + LadderingEnabled bool `json:"laddering_enabled" db:"laddering_enabled"` } // DefaultGracePeriodDays is the fallback window used when a provider @@ -930,3 +938,79 @@ type AccountRegistrationFilter struct { Provider *string Search string } + +// ========================================== +// COMMITMENT LADDERING +// ========================================== + +// Ladder default constants. These alias the canonical values in pkg/ladder so +// that internal/config callers can reference them by name without importing +// pkg/ladder directly. The string-typed mode/cadence values are also sourced +// from pkg/ladder to guarantee the DB and the engine always use the same tokens. + +// DefaultLadderTargetCoverage is the default commitment coverage target (%). +// Aliases ladder.DefaultTargetCoveragePct so only one source of truth exists. +const DefaultLadderTargetCoverage = ladder.DefaultTargetCoveragePct + +// DefaultLadderBufferFraction is the default fraction of the base allocation +// reserved in short-term / convertible buffer commitments. +const DefaultLadderBufferFraction = ladder.DefaultBufferFraction + +// DefaultLadderBaselinePercentile is the default usage percentile used to +// anchor the base commitment layer. +const DefaultLadderBaselinePercentile = ladder.DefaultBaselinePercentile + +// DefaultLadderLookbackDays is the default historical window (days) used to +// compute the usage baseline. +const DefaultLadderLookbackDays = ladder.DefaultLookbackDays + +// DefaultLadderBufferUtilThreshold is the default buffer-layer utilization % +// below which the engine emits a reshape recommendation. +const DefaultLadderBufferUtilThreshold = ladder.DefaultBufferUtilizationThresholdPct + +// DefaultLadderMaxActionsPerRun is the default cap on the number of +// PlannedActions the engine may execute per run. +const DefaultLadderMaxActionsPerRun = 10 + +// MaxLadderActionsPerRun is the ceiling enforced at validation time. A value +// above this is almost certainly a misconfiguration and should fail loud rather +// than fan out unbounded actions. +const MaxLadderActionsPerRun = 50 + +// LadderConfigDB is the DB-persistence mirror of pkg/ladder.LadderConfig. +// It stores one per-account, per-provider ladder configuration row and is +// used by the store layer (GetLadderConfig / UpsertLadderConfig) and the API +// handler. Mode and Cadence are plain strings whose valid values are defined +// by pkg/ladder (ModeEmailApproval, ModeAutoApprove, CadenceDaily, +// CadenceWeekly). Validation calls pkg/ladder's Parse* functions so the +// internal/config package never redefines those constants. +// +// MaxHourlyCommitPerRun is a pointer because nil means "no cap" (distinct from +// 0, which would cap all spending). All numeric money fields follow the project +// rule: absent = nil/pointer, never 0. +// Field order is optimized for govet fieldalignment (pointer-containing fields +// grouped first to shrink the GC pointer-scan range, then scalars, bool last). +// It intentionally does not follow the logical/SQL column order; see the +// scanLadderConfig / Upsert SQL for the wire order. +type LadderConfigDB struct { + UpdatedAt time.Time `json:"updated_at"` + CreatedAt time.Time `json:"created_at"` + // MaxHourlyCommitPerRun caps the total hourly commitment delta a single run + // may purchase. nil means no cap. + MaxHourlyCommitPerRun *float64 `json:"max_hourly_commit_per_run,omitempty"` + CloudAccountID string `json:"cloud_account_id"` + Provider string `json:"provider"` + Mode string `json:"mode"` // ladder.ModeEmailApproval | ladder.ModeAutoApprove + Cadence string `json:"cadence"` // ladder.CadenceDaily | ladder.CadenceWeekly + ID string `json:"id"` + RampSchedule json.RawMessage `json:"ramp_schedule"` + BufferUtilizationThreshold float64 `json:"buffer_utilization_threshold"` + LookbackDays int `json:"lookback_days"` + MaxActionsPerRun int `json:"max_actions_per_run"` + // BaselinePercentile is the statistical percentile used to anchor the base + // commitment layer. Must be in (0, 50]. + BaselinePercentile float64 `json:"baseline_percentile"` + BufferFraction float64 `json:"buffer_fraction"` + TargetCoverage float64 `json:"target_coverage"` + Enabled bool `json:"enabled"` +} diff --git a/internal/config/validation.go b/internal/config/validation.go index 58c0afd29..36f63c23a 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -2,10 +2,14 @@ package config import ( + "encoding/json" "fmt" + "math" "net/mail" "sort" "strings" + + "github.com/LeanerCloud/CUDly/pkg/ladder" ) // ValidProviders lists all supported cloud providers @@ -546,3 +550,110 @@ func ValidateRampScheduleEnv(val string) error { } return nil } + +// ========================================== +// LADDER CONFIG VALIDATION +// ========================================== + +// Validate validates a LadderConfigDB before persist. It delegates mode and +// cadence checks to pkg/ladder's Parse* functions (single source of truth for +// those enums) and validates numeric bounds using the same invariants that +// pkg/ladder.LadderConfig.Validate() enforces. +// +// Returns a specific, descriptive error on any violation. Callers must never +// silently default away a validation failure. +func (c *LadderConfigDB) Validate() error { + if err := validateLadderProvider(c.Provider); err != nil { + return err + } + if _, err := ladder.ParseLadderMode(c.Mode); err != nil { + return fmt.Errorf("mode: %w", err) + } + if _, err := ladder.ParseLadderCadence(c.Cadence); err != nil { + return fmt.Errorf("cadence: %w", err) + } + if err := c.validateLadderBaselineBounds(); err != nil { + return err + } + if err := c.validateLadderRunLimits(); err != nil { + return err + } + return c.validateLadderRampSchedule() +} + +// validateLadderProvider checks that Provider is a recognized cloud provider slug. +func validateLadderProvider(provider string) error { + for _, p := range ValidProviders { + if provider == p { + return nil + } + } + return fmt.Errorf("provider %q is not valid (allowed: %s)", provider, strings.Join(ValidProviders, ", ")) +} + +// validateLadderBaselineBounds checks the coverage target, buffer fraction, +// and baseline measurement parameters. Uses the same NaN-hostile invariants +// as pkg/ladder.LadderConfig.validateBaselineBounds (mirrored here so +// internal/config does not call unexported pkg methods). +func (c *LadderConfigDB) validateLadderBaselineBounds() error { + if err := c.validateLadderCoverageBounds(); err != nil { + return err + } + if math.IsNaN(c.BaselinePercentile) || !(c.BaselinePercentile > 0 && c.BaselinePercentile <= 50) { + return fmt.Errorf("baseline_percentile %g must be in (0, 50]", c.BaselinePercentile) + } + if c.LookbackDays <= 0 { + return fmt.Errorf("lookback_days %d must be > 0", c.LookbackDays) + } + return nil +} + +// validateLadderCoverageBounds checks the coverage target and buffer fraction. +// Split out from validateLadderBaselineBounds to keep each function under the +// cyclomatic-complexity gate. +func (c *LadderConfigDB) validateLadderCoverageBounds() error { + if math.IsNaN(c.TargetCoverage) || !(c.TargetCoverage > 0 && c.TargetCoverage <= 100) { + return fmt.Errorf("target_coverage %g must be in (0, 100]", c.TargetCoverage) + } + if math.IsNaN(c.BufferFraction) || !(c.BufferFraction >= 0 && c.BufferFraction < 1) { + return fmt.Errorf("buffer_fraction %g must be in [0, 1)", c.BufferFraction) + } + return nil +} + +// validateLadderRunLimits checks the per-run spend and action caps and the +// buffer utilization threshold. +func (c *LadderConfigDB) validateLadderRunLimits() error { + if v := c.MaxHourlyCommitPerRun; v != nil { + if math.IsNaN(*v) || math.IsInf(*v, 0) || !(*v > 0) { + return fmt.Errorf("max_hourly_commit_per_run %g must be a positive finite number when set (omit for no cap)", *v) + } + } + if c.MaxActionsPerRun <= 0 { + return fmt.Errorf("max_actions_per_run %d must be > 0", c.MaxActionsPerRun) + } + if c.MaxActionsPerRun > MaxLadderActionsPerRun { + return fmt.Errorf("max_actions_per_run %d exceeds ceiling %d", c.MaxActionsPerRun, MaxLadderActionsPerRun) + } + if math.IsNaN(c.BufferUtilizationThreshold) || !(c.BufferUtilizationThreshold > 0 && c.BufferUtilizationThreshold <= 100) { + return fmt.Errorf("buffer_utilization_threshold %g must be in (0, 100]", c.BufferUtilizationThreshold) + } + return nil +} + +// validateLadderRampSchedule parses the JSONB ramp_schedule payload and +// delegates structural validation to pkg/ladder.RampSchedule.Validate so +// the two validation paths stay in sync. +func (c *LadderConfigDB) validateLadderRampSchedule() error { + if len(c.RampSchedule) == 0 { + return fmt.Errorf("ramp_schedule must not be empty") + } + var rs ladder.RampSchedule + if err := json.Unmarshal(c.RampSchedule, &rs); err != nil { + return fmt.Errorf("ramp_schedule is not valid JSON: %w", err) + } + if err := rs.Validate(); err != nil { + return fmt.Errorf("ramp_schedule: %w", err) + } + return nil +} diff --git a/internal/config/validation_ladder_test.go b/internal/config/validation_ladder_test.go new file mode 100644 index 000000000..0a9cb6a06 --- /dev/null +++ b/internal/config/validation_ladder_test.go @@ -0,0 +1,174 @@ +package config + +import ( + "encoding/json" + "math" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// validLadderConfigDB returns a LadderConfigDB that passes Validate(). Each +// bounds test below clones this and mutates exactly one field so a failure +// pinpoints the single offending invariant. +func validLadderConfigDB() LadderConfigDB { + return LadderConfigDB{ + CloudAccountID: "11111111-1111-1111-1111-111111111111", + Provider: "aws", + Enabled: true, + Mode: "email_approval", + Cadence: "daily", + TargetCoverage: 100, + BufferFraction: 0.10, + BaselinePercentile: 5, + LookbackDays: 30, + BufferUtilizationThreshold: 90, + MaxActionsPerRun: 10, + MaxHourlyCommitPerRun: nil, // nil = no cap (valid) + RampSchedule: json.RawMessage(`{"steps":[{"after_days":0,"fraction":1.0}]}`), + } +} + +// TestLadderConfigDB_Validate_Bounds exercises every numeric bound of +// LadderConfigDB.Validate() at below-min / at-lower-bound / at-upper-bound / +// above-max / NaN (for float fields), asserting error vs nil per the documented +// ranges: +// +// target_coverage (0, 100] +// baseline_percentile (0, 50] +// buffer_fraction [0, 1) -- note: 0 is VALID ("no buffer") +// buffer_utilization_threshold (0, 100] +// lookback_days > 0 +// max_actions_per_run > 0 and <= MaxLadderActionsPerRun (50) +// +// Struct field order (func, string, bool) is chosen for govet fieldalignment; +// the keyed literals below keep the human-readable name/mutate/wantErr order. +func TestLadderConfigDB_Validate_Bounds(t *testing.T) { + cases := []struct { + mutate func(c *LadderConfigDB) + name string + wantErr bool + }{ + // baseline valid + {name: "valid baseline", mutate: func(c *LadderConfigDB) {}, wantErr: false}, + + // target_coverage (0, 100] + {name: "target_coverage 0 (below min)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 0 }, wantErr: true}, + {name: "target_coverage 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 0.01 }, wantErr: false}, + {name: "target_coverage 100 (upper bound)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 100 }, wantErr: false}, + {name: "target_coverage 100.01 (above max)", mutate: func(c *LadderConfigDB) { c.TargetCoverage = 100.01 }, wantErr: true}, + {name: "target_coverage NaN", mutate: func(c *LadderConfigDB) { c.TargetCoverage = math.NaN() }, wantErr: true}, + + // baseline_percentile (0, 50] + {name: "baseline_percentile 0 (below min)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 0 }, wantErr: true}, + {name: "baseline_percentile 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 0.01 }, wantErr: false}, + {name: "baseline_percentile 50 (upper bound)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 50 }, wantErr: false}, + {name: "baseline_percentile 50.01 (above max)", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = 50.01 }, wantErr: true}, + {name: "baseline_percentile NaN", mutate: func(c *LadderConfigDB) { c.BaselinePercentile = math.NaN() }, wantErr: true}, + + // buffer_fraction [0, 1) -- 0 is valid, 1 is excluded + {name: "buffer_fraction 0 (valid lower bound, no buffer)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 0 }, wantErr: false}, + {name: "buffer_fraction -0.01 (below min)", mutate: func(c *LadderConfigDB) { c.BufferFraction = -0.01 }, wantErr: true}, + {name: "buffer_fraction 0.999 (just below upper)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 0.999 }, wantErr: false}, + {name: "buffer_fraction 1.0 (upper bound excluded)", mutate: func(c *LadderConfigDB) { c.BufferFraction = 1.0 }, wantErr: true}, + {name: "buffer_fraction NaN", mutate: func(c *LadderConfigDB) { c.BufferFraction = math.NaN() }, wantErr: true}, + + // buffer_utilization_threshold (0, 100] + {name: "buffer_utilization_threshold 0 (below min)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 0 }, wantErr: true}, + {name: "buffer_utilization_threshold 0.01 (just above lower)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 0.01 }, wantErr: false}, + {name: "buffer_utilization_threshold 100 (upper bound)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 100 }, wantErr: false}, + {name: "buffer_utilization_threshold 100.01 (above max)", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = 100.01 }, wantErr: true}, + {name: "buffer_utilization_threshold NaN", mutate: func(c *LadderConfigDB) { c.BufferUtilizationThreshold = math.NaN() }, wantErr: true}, + + // lookback_days > 0 + {name: "lookback_days 0 (below min)", mutate: func(c *LadderConfigDB) { c.LookbackDays = 0 }, wantErr: true}, + {name: "lookback_days -1 (negative)", mutate: func(c *LadderConfigDB) { c.LookbackDays = -1 }, wantErr: true}, + {name: "lookback_days 1 (lower bound)", mutate: func(c *LadderConfigDB) { c.LookbackDays = 1 }, wantErr: false}, + + // max_actions_per_run > 0 and <= 50 + {name: "max_actions_per_run 0 (below min)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = 0 }, wantErr: true}, + {name: "max_actions_per_run 1 (lower bound)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = 1 }, wantErr: false}, + {name: "max_actions_per_run 50 (upper bound)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = MaxLadderActionsPerRun }, wantErr: false}, + {name: "max_actions_per_run 51 (above max)", mutate: func(c *LadderConfigDB) { c.MaxActionsPerRun = MaxLadderActionsPerRun + 1 }, wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + tc.mutate(&c) + err := c.Validate() + if tc.wantErr { + require.Error(t, err, "expected Validate() to reject case %q", tc.name) + } else { + require.NoError(t, err, "expected Validate() to accept case %q", tc.name) + } + }) + } +} + +// TestLadderConfigDB_Validate_MaxHourlyCommitPerRun covers the pointer-typed +// optional cap: nil = no cap (valid), a positive value is valid, and +// non-positive / NaN / Inf are rejected. +func TestLadderConfigDB_Validate_MaxHourlyCommitPerRun(t *testing.T) { + f := func(v float64) *float64 { return &v } + cases := []struct { + val *float64 + name string + wantErr bool + }{ + {name: "nil (no cap)", val: nil, wantErr: false}, + {name: "positive", val: f(12.5), wantErr: false}, + {name: "zero", val: f(0), wantErr: true}, + {name: "negative", val: f(-1), wantErr: true}, + {name: "NaN", val: f(math.NaN()), wantErr: true}, + {name: "positive Inf", val: f(math.Inf(1)), wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + c.MaxHourlyCommitPerRun = tc.val + err := c.Validate() + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +// TestLadderConfigDB_Validate_EnumsAndProvider covers the delegated enum checks +// (mode/cadence via pkg/ladder Parse*) and the provider allow-list. +func TestLadderConfigDB_Validate_EnumsAndProvider(t *testing.T) { + cases := []struct { + mutate func(c *LadderConfigDB) + name string + wantErr bool + }{ + {name: "unknown provider", mutate: func(c *LadderConfigDB) { c.Provider = "digitalocean" }, wantErr: true}, + {name: "empty provider", mutate: func(c *LadderConfigDB) { c.Provider = "" }, wantErr: true}, + {name: "gcp provider", mutate: func(c *LadderConfigDB) { c.Provider = "gcp" }, wantErr: false}, + {name: "unknown mode", mutate: func(c *LadderConfigDB) { c.Mode = "yolo_approve" }, wantErr: true}, + {name: "auto_approve mode", mutate: func(c *LadderConfigDB) { c.Mode = "auto_approve" }, wantErr: false}, + {name: "unknown cadence", mutate: func(c *LadderConfigDB) { c.Cadence = "hourly" }, wantErr: true}, + {name: "weekly cadence", mutate: func(c *LadderConfigDB) { c.Cadence = "weekly" }, wantErr: false}, + {name: "empty ramp_schedule", mutate: func(c *LadderConfigDB) { c.RampSchedule = nil }, wantErr: true}, + {name: "malformed ramp_schedule JSON", mutate: func(c *LadderConfigDB) { c.RampSchedule = json.RawMessage(`not-json`) }, wantErr: true}, + {name: "ramp fractions not summing to 1", mutate: func(c *LadderConfigDB) { + c.RampSchedule = json.RawMessage(`{"steps":[{"after_days":0,"fraction":0.5}]}`) + }, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := validLadderConfigDB() + tc.mutate(&c) + err := c.Validate() + if tc.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/database/postgres/migrations/000079_ladder_configs.down.sql b/internal/database/postgres/migrations/000079_ladder_configs.down.sql new file mode 100644 index 000000000..803730ff8 --- /dev/null +++ b/internal/database/postgres/migrations/000079_ladder_configs.down.sql @@ -0,0 +1,8 @@ +-- Rollback migration 000079: remove ladder_configs table and the +-- laddering_enabled kill-switch column from global_config. +-- DROP TABLE CASCADE removes the trigger automatically. + +DROP TABLE IF EXISTS ladder_configs; + +ALTER TABLE global_config + DROP COLUMN IF EXISTS laddering_enabled; diff --git a/internal/database/postgres/migrations/000079_ladder_configs.up.sql b/internal/database/postgres/migrations/000079_ladder_configs.up.sql new file mode 100644 index 000000000..4720b7af0 --- /dev/null +++ b/internal/database/postgres/migrations/000079_ladder_configs.up.sql @@ -0,0 +1,58 @@ +-- Migration 000079: commitment-laddering per-scope configuration table +-- and global_config kill-switch column. +-- +-- Default-off: laddering_enabled = false on the singleton global_config row; +-- per-account rows default to enabled = false. No laddering behaviour +-- activates until an operator explicitly enables both the global kill-switch +-- and at least one per-account row. +-- +-- Idempotent: ADD COLUMN IF NOT EXISTS and CREATE TABLE IF NOT EXISTS +-- throughout, so re-running after a partial failure is safe. + +-- Kill-switch on the global_config singleton row. +ALTER TABLE global_config + ADD COLUMN IF NOT EXISTS laddering_enabled BOOLEAN NOT NULL DEFAULT false; + +-- Per-account ladder configuration. +-- One row per (cloud_account_id, provider) pair: UNIQUE enforces the constraint. +-- ON DELETE CASCADE removes the config when the cloud account is deleted. +CREATE TABLE IF NOT EXISTS ladder_configs ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + cloud_account_id UUID NOT NULL + REFERENCES cloud_accounts(id) ON DELETE CASCADE, + provider TEXT NOT NULL, + enabled BOOLEAN NOT NULL DEFAULT false, + mode TEXT NOT NULL, -- email_approval | auto_approve + cadence TEXT NOT NULL, -- daily | weekly + target_coverage NUMERIC(5,2) NOT NULL, + buffer_fraction NUMERIC(5,4) NOT NULL, + baseline_percentile NUMERIC(5,2) NOT NULL, + lookback_days INTEGER NOT NULL, + buffer_utilization_threshold NUMERIC(5,2) NOT NULL, + max_hourly_commit_per_run NUMERIC(20,6), -- NULL = no cap + max_actions_per_run INTEGER NOT NULL, + ramp_schedule JSONB NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + + UNIQUE(cloud_account_id, provider) +); + +CREATE INDEX IF NOT EXISTS idx_ladder_configs_account + ON ladder_configs(cloud_account_id); + +CREATE INDEX IF NOT EXISTS idx_ladder_configs_provider + ON ladder_configs(provider); + +-- Trigger: keep updated_at current on every UPDATE. +-- Guard with DO block so re-runs are idempotent. +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_trigger WHERE tgname = 'update_ladder_configs_updated_at' + ) THEN + CREATE TRIGGER update_ladder_configs_updated_at + BEFORE UPDATE ON ladder_configs + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); + END IF; +END $$; diff --git a/internal/database/postgres/migrations/000080_ladder_runs.down.sql b/internal/database/postgres/migrations/000080_ladder_runs.down.sql new file mode 100644 index 000000000..a6bfbfc88 --- /dev/null +++ b/internal/database/postgres/migrations/000080_ladder_runs.down.sql @@ -0,0 +1,14 @@ +-- Rollback migration 000080: remove ladder_run_id columns from +-- ri_exchange_history and purchase_executions, then drop ladder_runs. +-- Order matters: remove the FK-holding columns before dropping the +-- table they reference. + +DROP INDEX IF EXISTS idx_ri_exchange_history_ladder_run; +ALTER TABLE ri_exchange_history + DROP COLUMN IF EXISTS ladder_run_id; + +DROP INDEX IF EXISTS idx_purchase_executions_ladder_run; +ALTER TABLE purchase_executions + DROP COLUMN IF EXISTS ladder_run_id; + +DROP TABLE IF EXISTS ladder_runs; diff --git a/internal/database/postgres/migrations/000080_ladder_runs.up.sql b/internal/database/postgres/migrations/000080_ladder_runs.up.sql new file mode 100644 index 000000000..232de7783 --- /dev/null +++ b/internal/database/postgres/migrations/000080_ladder_runs.up.sql @@ -0,0 +1,87 @@ +-- Migration 000080: ladder_runs immutable audit table. +-- +-- Each ladder engine invocation writes one row here before taking any +-- action. Status tracks the lifecycle: +-- planned -> awaiting_approval -> approved -> executing -> completed +-- or -> failed | cancelled | expired (terminal states) +-- +-- Monetary baseline columns are all NULLABLE. NULL means "not yet computed" +-- or "run failed before this could be measured" -- never coerced to $0 +-- (project rule: absent numbers are NULL/pointer, never 0). +-- +-- Also adds ladder_run_id FK columns to purchase_executions and +-- ri_exchange_history so every commitment touched by a ladder run can be +-- traced back to the run that triggered it. +-- +-- Idempotent: CREATE TABLE IF NOT EXISTS and ADD COLUMN IF NOT EXISTS +-- throughout. + +CREATE TABLE IF NOT EXISTS ladder_runs ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + config_id UUID REFERENCES ladder_configs(id) ON DELETE SET NULL, + started_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + completed_at TIMESTAMPTZ, + status TEXT NOT NULL, -- planned|awaiting_approval|approved|executing|completed|failed|cancelled|expired + mode TEXT, + cadence TEXT, + + -- Monetary baseline snapshot; all NULLABLE (NULL != $0, see docstring above). + baseline_usd_hr NUMERIC(20,6), + target_usd_hr NUMERIC(20,6), + existing_usd_hr NUMERIC(20,6), + gap_usd_hr NUMERIC(20,6), + + plan JSONB NOT NULL DEFAULT '{}', + + -- Running totals. NOT NULL DEFAULT 0 because these are accumulators + -- initialised to 0 at row creation, not baseline measurements. + total_hourly_commit NUMERIC(20,6) NOT NULL DEFAULT 0, + total_upfront_cost NUMERIC(20,6) NOT NULL DEFAULT 0, + estimated_savings NUMERIC(20,6) NOT NULL DEFAULT 0, + + approval_token TEXT, + approval_token_expires_at TIMESTAMPTZ, + approved_by TEXT, + cancelled_by TEXT, + fire_at TIMESTAMPTZ, + + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_ladder_runs_config_id + ON ladder_runs(config_id); + +CREATE INDEX IF NOT EXISTS idx_ladder_runs_status + ON ladder_runs(status); + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_trigger WHERE tgname = 'update_ladder_runs_updated_at' + ) THEN + CREATE TRIGGER update_ladder_runs_updated_at + BEFORE UPDATE ON ladder_runs + FOR EACH ROW EXECUTE FUNCTION update_updated_at_column(); + END IF; +END $$; + +-- Link purchase_executions to the ladder run that triggered them. +-- Partial index: only ladder-initiated executions carry the FK so the +-- index stays narrow and the NULL-check is free. +ALTER TABLE purchase_executions + ADD COLUMN IF NOT EXISTS ladder_run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL; + +CREATE INDEX IF NOT EXISTS idx_purchase_executions_ladder_run + ON purchase_executions(ladder_run_id) + WHERE ladder_run_id IS NOT NULL; + +-- Link ri_exchange_history to the ladder run that triggered them. +-- ri_exchange_history was introduced in migration 000001 and is confirmed +-- present on main (most recently altered in migration 000077). +ALTER TABLE ri_exchange_history + ADD COLUMN IF NOT EXISTS ladder_run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL; + +CREATE INDEX IF NOT EXISTS idx_ri_exchange_history_ladder_run + ON ri_exchange_history(ladder_run_id) + WHERE ladder_run_id IS NOT NULL; diff --git a/internal/database/postgres/migrations/000081_ladder_tranches.down.sql b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql new file mode 100644 index 000000000..d2464bba7 --- /dev/null +++ b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql @@ -0,0 +1,14 @@ +-- Rollback migration 000081: drop the ladder_tranches table. +-- +-- The run_id column and its partial index are dropped explicitly first, in +-- FK-safe order (index, then the FK-holding column), before the table itself. +-- DROP TABLE would remove both implicitly, but listing them keeps the down +-- migration an exact inverse of the up. Remaining indexes are dropped +-- automatically with the table. + +DROP INDEX IF EXISTS idx_ladder_tranches_run_id; + +ALTER TABLE ladder_tranches + DROP COLUMN IF EXISTS run_id; + +DROP TABLE IF EXISTS ladder_tranches; diff --git a/internal/database/postgres/migrations/000081_ladder_tranches.up.sql b/internal/database/postgres/migrations/000081_ladder_tranches.up.sql new file mode 100644 index 000000000..749ceae48 --- /dev/null +++ b/internal/database/postgres/migrations/000081_ladder_tranches.up.sql @@ -0,0 +1,50 @@ +-- Migration 000081: ladder_tranches table. +-- +-- A tranche is a single timed purchase slice within a ladder run's ramp +-- schedule. The scheduler fires each tranche at its scheduled_date, creates +-- a purchase_executions row, and links execution_id back here once the +-- commitment is bought. +-- +-- run_id traces a tranche back to the ladder_runs row that created it. It is +-- nullable because a tranche may be planned before its run row exists in later +-- flows, and ON DELETE SET NULL keeps the tranche for audit if the run is +-- later removed (mirrors config_id's semantics). Type matches ladder_runs.id +-- (UUID, see migration 000080). +-- +-- execution_id references purchase_executions(execution_id) rather than +-- purchase_executions(id) because callers resolve executions by their +-- execution_id business key. The UNIQUE constraint on execution_id +-- (migration 000008) satisfies the FK referential requirement. +-- +-- Idempotent: CREATE TABLE IF NOT EXISTS throughout. + +CREATE TABLE IF NOT EXISTS ladder_tranches ( + id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), + config_id UUID REFERENCES ladder_configs(id) ON DELETE SET NULL, + run_id UUID REFERENCES ladder_runs(id) ON DELETE SET NULL, + layer_type TEXT NOT NULL, + amount_usd_hr NUMERIC(20,6) NOT NULL, + term TEXT NOT NULL, + payment_option TEXT NOT NULL, + scheduled_date TIMESTAMPTZ NOT NULL, + status TEXT NOT NULL, -- scheduled|fired|completed|cancelled|failed + execution_id UUID REFERENCES purchase_executions(execution_id) ON DELETE SET NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_config_id + ON ladder_tranches(config_id); + +-- Partial index: only run-linked tranches carry the FK, so the index stays +-- narrow and traces tranches back to their originating run cheaply. +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_run_id + ON ladder_tranches(run_id) + WHERE run_id IS NOT NULL; + +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_status + ON ladder_tranches(status); + +-- Partial index for the scheduler sweep that fires due tranches. +CREATE INDEX IF NOT EXISTS idx_ladder_tranches_scheduled + ON ladder_tranches(scheduled_date) + WHERE status = 'scheduled'; diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index f9f6b6401..75f1a6878 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1287,6 +1287,54 @@ func (m *MockConfigStore) StampRIExchangeApprovedBy(ctx context.Context, id stri return args.Error(0) } +// GetLadderConfigs mocks the GetLadderConfigs operation. +// Returns an empty slice when no expectation is registered so tests that do +// not exercise laddering keep working without mock setup. +func (m *MockConfigStore) GetLadderConfigs(ctx context.Context) ([]config.LadderConfigDB, error) { + if !isExpected(&m.Mock, "GetLadderConfigs") { + return []config.LadderConfigDB{}, nil + } + args := m.Called(ctx) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).([]config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected []config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + +// GetLadderConfig mocks the GetLadderConfig operation. +// Returns (nil, nil) when no expectation is registered. +func (m *MockConfigStore) GetLadderConfig(ctx context.Context, cloudAccountID, provider string) (*config.LadderConfigDB, error) { + if !isExpected(&m.Mock, "GetLadderConfig") { + return nil, nil + } + args := m.Called(ctx, cloudAccountID, provider) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected *config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + +// UpsertLadderConfig mocks the UpsertLadderConfig operation. +func (m *MockConfigStore) UpsertLadderConfig(ctx context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + args := m.Called(ctx, cfg) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.LadderConfigDB) + if !ok { + panic(fmt.Sprintf("mock: expected *config.LadderConfigDB, got %T", args.Get(0))) + } + return v, args.Error(1) +} + // isExpected reports whether mock has any .On() expectation for method. func isExpected(mock *mock.Mock, method string) bool { for _, call := range mock.ExpectedCalls { diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index 5400f2d47..dd6b75e39 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -319,3 +319,13 @@ func (m *mockConfigStoreForHealth) GetPurchaseHistoryInFlight(_ context.Context) func (m *mockConfigStoreForHealth) GetScheduledExecutionsDue(_ context.Context) ([]config.PurchaseExecution, error) { return nil, nil } + +func (m *mockConfigStoreForHealth) GetLadderConfigs(_ context.Context) ([]config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStoreForHealth) GetLadderConfig(_ context.Context, _, _ string) (*config.LadderConfigDB, error) { + return nil, nil +} +func (m *mockConfigStoreForHealth) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { + return cfg, nil +} From 2f7111fb76d0998f012e863066802361513168a9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 04:17:27 +0200 Subject: [PATCH 2/8] fix(api): preserve omitted GlobalConfig fields on partial PUT The commitment-laddering kill-switch toggle sends a partial PUT to /api/config carrying only the field it changes. updateConfig unmarshalled that body into a zero-value GlobalConfig and only restored a handful of enumerated fields, so every omitted field (approval_required, default_ramp_schedule, and all six ri_exchange_* automation settings) was written back as its zero value, wiping the user's stored configuration. updateConfig now loads the stored GlobalConfig and json.Unmarshals the request body over a copy of it: keys present in the JSON update their field, keys absent retain the persisted value, for every field with no per-field enumeration. Malformed bodies are rejected up front; a read error fails loud rather than merging over a zero base. Deletes the fragile preserveOmittedRecommendationFields helper and adds a regression test that PUTs only {"laddering_enabled": true} and asserts the omitted fields survive on the config handed to SaveGlobalConfig. --- internal/api/handler_config.go | 93 +++++++++-------------------- internal/api/handler_config_test.go | 82 +++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 65 deletions(-) diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index b55869af3..4695a089f 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -58,67 +58,34 @@ func (h *Handler) getConfig(ctx context.Context, req *events.LambdaFunctionURLRe return resp, nil } -// preserveOmittedRecommendationFields merges persisted GlobalConfig values -// for the cycle-parameter fields when the request body omits them. -// Without this merge, a partial PUT would silently zero out -// RecommendationsCacheStaleHours / RecommendationsLookbackDays / PurchaseDelayHours, -// which all have meaningful 0-vs-omitted semantics that json.Unmarshal can't -// represent directly. Errors from GetGlobalConfig fall through: the request body's -// zero values then flow into Validate() which rejects out-of-range values, -// matching the pre-fix behaviour. Extracted from updateConfig to keep that -// function under the cyclomatic-complexity gate after the merge logic was -// added (PR #308 CodeRabbit pass-2 review). -// allKeysPresent reports whether every key is present in m. Extracted so -// callers can express an all-present short-circuit without an N-way boolean -// conjunction (which trips the cyclomatic-complexity gate). -func allKeysPresent(m map[string]json.RawMessage, keys ...string) bool { - for _, k := range keys { - if _, ok := m[k]; !ok { - return false - } - } - return true -} - -func (h *Handler) preserveOmittedRecommendationFields(ctx context.Context, cfg *config.GlobalConfig, body string) error { - var present map[string]json.RawMessage - if err := json.Unmarshal([]byte(body), &present); err != nil { - return NewClientError(400, "invalid request body") - } - _, hasStale := present["recommendations_cache_stale_hours"] - _, hasLookback := present["recommendations_lookback_days"] - _, hasDelay := present["purchase_delay_hours"] - // laddering_enabled is only present in the PUT body when the Purchasing - // panel is active (the General panel form omits it). Preserve the existing - // DB value when absent so a General-panel save cannot silently reset the - // kill-switch back to false. - _, hasLaddering := present["laddering_enabled"] - // Skip the DB read when every preserved key is already supplied. - if allKeysPresent(present, - "recommendations_cache_stale_hours", - "recommendations_lookback_days", - "purchase_delay_hours", - "laddering_enabled", - ) { - return nil - } - existing, gcErr := h.config.GetGlobalConfig(ctx) - if gcErr != nil || existing == nil { - return nil - } - if !hasStale { - cfg.RecommendationsCacheStaleHours = existing.RecommendationsCacheStaleHours +// mergeGlobalConfigUpdate loads the stored GlobalConfig and json.Unmarshals the +// request body over a copy of it, so any field ABSENT from the JSON retains its +// persisted value (present -> updated, absent -> preserved) for EVERY field, +// with no per-field enumeration. This is required for partial PUTs such as the +// laddering kill-switch toggle, which sends only { laddering_enabled }; +// unmarshalling into a zero value would otherwise wipe RI-exchange automation, +// approval_required, default_ramp_schedule, and every other omitted field. +// +// A malformed body is a 400 (rejected before any DB read). A read error fails +// loud rather than merging over a zero base: proceeding without the stored +// config would silently clobber it on a partial PUT (money-adjacent path; no +// silent fallbacks). +func (h *Handler) mergeGlobalConfigUpdate(ctx context.Context, body string) (*config.GlobalConfig, error) { + if !json.Valid([]byte(body)) { + return nil, NewClientError(400, "invalid request body") } - if !hasLookback { - cfg.RecommendationsLookbackDays = existing.RecommendationsLookbackDays + existing, err := h.config.GetGlobalConfig(ctx) + if err != nil { + return nil, err } - if !hasDelay { - cfg.PurchaseDelayHours = existing.PurchaseDelayHours + var cfg config.GlobalConfig + if existing != nil { + cfg = *existing } - if !hasLaddering { - cfg.LadderingEnabled = existing.LadderingEnabled + if err = json.Unmarshal([]byte(body), &cfg); err != nil { + return nil, NewClientError(400, "invalid request body") } - return nil + return &cfg, nil } func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (*StatusResponse, error) { @@ -127,13 +94,9 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return nil, err } - var cfg config.GlobalConfig - if err := json.Unmarshal([]byte(req.Body), &cfg); err != nil { - return nil, NewClientError(400, "invalid request body") - } - - if err := h.preserveOmittedRecommendationFields(ctx, &cfg, req.Body); err != nil { - return nil, err + cfg, mergeErr := h.mergeGlobalConfigUpdate(ctx, req.Body) + if mergeErr != nil { + return nil, mergeErr } // Validate the configuration @@ -141,7 +104,7 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) } - if err := h.config.SaveGlobalConfig(ctx, &cfg); err != nil { + if err := h.config.SaveGlobalConfig(ctx, cfg); err != nil { return nil, err } diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 82b3601b5..d425b43c6 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -172,6 +172,88 @@ func TestHandler_updateConfig_LadderingEnabledPreservation(t *testing.T) { } } +// TestHandler_updateConfig_PartialPUTPreservesOmittedFields is the F2 +// data-loss regression test. The laddering kill-switch toggle sends a partial +// PUT of only {"laddering_enabled": true}; before the merge fix, updateConfig +// unmarshalled that into a zero GlobalConfig and only restored the four +// recommendation/laddering fields, silently zeroing approval_required, +// default_ramp_schedule, and every ri_exchange_* field (wiping the user's +// RI-exchange automation). The merge must leave every omitted field at its +// stored value while applying the one field the body carries. Assert against +// the config handed to SaveGlobalConfig, not just a 200. +func TestHandler_updateConfig_PartialPUTPreservesOmittedFields(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + notifyEmail := "ops@example.com" + existing := &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + NotificationEmail: ¬ifyEmail, + AutoCollect: true, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + DefaultRampSchedule: "immediate", + RIExchangeEnabled: true, + RIExchangeMode: "automatic", + RIExchangeUtilizationThreshold: 95, + RIExchangeMaxPerExchangeUSD: 1000, + RIExchangeMaxDailyUSD: 5000, + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + PurchaseDelayHours: 48, + LadderingEnabled: false, + } + mockStore.On("GetGlobalConfig", ctx).Return(existing, nil) + mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { saved = *args.Get(1).(*config.GlobalConfig) }). + Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"laddering_enabled": true}`, + } + result, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + + // The one field the body carried was applied. + assert.True(t, saved.LadderingEnabled, "laddering_enabled from the body must be applied") + + // Every omitted field must retain its stored value (the F2 bug zeroed these). + assert.True(t, saved.ApprovalRequired, "approval_required must be preserved") + assert.Equal(t, "immediate", saved.DefaultRampSchedule, "default_ramp_schedule must be preserved") + assert.True(t, saved.RIExchangeEnabled, "ri_exchange_enabled must be preserved") + assert.Equal(t, "automatic", saved.RIExchangeMode, "ri_exchange_mode must be preserved") + assert.Equal(t, 95.0, saved.RIExchangeUtilizationThreshold, "ri_exchange_utilization_threshold must be preserved") + assert.Equal(t, 1000.0, saved.RIExchangeMaxPerExchangeUSD, "ri_exchange_max_per_exchange_usd must be preserved") + assert.Equal(t, 5000.0, saved.RIExchangeMaxDailyUSD, "ri_exchange_max_daily_usd must be preserved") + assert.Equal(t, 30, saved.RIExchangeLookbackDays, "ri_exchange_lookback_days must be preserved") + + // Other scalars stay intact too. + assert.Equal(t, []string{"aws"}, saved.EnabledProviders) + assert.Equal(t, "all-upfront", saved.DefaultPayment) + assert.Equal(t, 80.0, saved.DefaultCoverage) + assert.Equal(t, 48, saved.PurchaseDelayHours) + require.NotNil(t, saved.NotificationEmail) + assert.Equal(t, "ops@example.com", *saved.NotificationEmail) +} + func TestHandler_updateConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) From eeb34981dc9b35a01c18c6a09a0c36f6563056e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 04:17:57 +0200 Subject: [PATCH 3/8] fix(ladder): split save guards and simplify kill-switch PUT Split the shared module-level saveInFlight flag into killSwitchSaveInFlight and configModalSaveInFlight so the kill-switch toggle and the per-account modal save, which hit unrelated endpoints, no longer block or revert each other. Now that the backend merges partial config PUTs, the kill-switch sends only { laddering_enabled } instead of rebuilding and re-sending the whole config (which was fragile and silently dropped fields it did not enumerate). The frontend updateConfig client accepts Partial, and the fetchGlobalConfigForLaddering helper is removed. --- frontend/src/api/settings.ts | 9 ++++- frontend/src/ladder.ts | 72 ++++++++---------------------------- 2 files changed, 23 insertions(+), 58 deletions(-) diff --git a/frontend/src/api/settings.ts b/frontend/src/api/settings.ts index b9e4f42bb..1ba457400 100644 --- a/frontend/src/api/settings.ts +++ b/frontend/src/api/settings.ts @@ -24,9 +24,14 @@ export async function updateServiceConfig(provider: string, service: string, cfg } /** - * Update global configuration + * Update global configuration. + * + * Accepts a partial config: the backend merges the body over the stored config + * (any key absent from the JSON keeps its persisted value), so callers may send + * only the fields they intend to change (e.g. the laddering kill-switch toggle + * sends just { laddering_enabled }). A full Config is still valid input. */ -export async function updateConfig(config: Config): Promise { +export async function updateConfig(config: Partial): Promise { return apiRequest('/config', { method: 'PUT', body: JSON.stringify(config) diff --git a/frontend/src/ladder.ts b/frontend/src/ladder.ts index e1b74ebcb..833a3dce0 100644 --- a/frontend/src/ladder.ts +++ b/frontend/src/ladder.ts @@ -22,8 +22,11 @@ import { canAccess } from './permissions'; // MODULE STATE // ========================================== -// In-flight guard for the save action. -let saveInFlight = false; +// Separate in-flight guards for the two independent save paths. Sharing one +// flag let the kill-switch toggle and the per-account modal save block/revert +// each other even though they touch unrelated backend endpoints. +let killSwitchSaveInFlight = false; +let configModalSaveInFlight = false; // Cached configs from the last successful load. let cachedConfigs: api.LadderConfig[] = []; @@ -313,19 +316,17 @@ function wireKillSwitchToggle(): void { if (!toggle || !canAccess('update', 'config')) return; toggle.addEventListener('change', async () => { - if (saveInFlight) { + if (killSwitchSaveInFlight) { toggle.checked = !toggle.checked; // revert optimistic return; } - saveInFlight = true; + killSwitchSaveInFlight = true; try { - const globalCfg = await fetchGlobalConfigForLaddering(); - if (globalCfg === null) { - toggle.checked = !toggle.checked; - return; - } - globalCfg.laddering_enabled = toggle.checked; - await api.updateConfig(globalCfg); + // The backend merges a partial PUT over the stored config (json.Unmarshal + // only sets keys present in the body), so we send ONLY the field we are + // changing. Every other setting is preserved server-side; enumerating the + // whole config here was fragile and dropped fields it did not list. + await api.updateConfig({ laddering_enabled: toggle.checked }); showToast({ message: `Commitment Laddering ${toggle.checked ? 'enabled' : 'disabled'} globally.`, kind: 'success', @@ -335,52 +336,11 @@ function wireKillSwitchToggle(): void { showToast({ message: 'Failed to update laddering kill-switch.', kind: 'error' }); console.error('Failed to toggle laddering_enabled:', err); } finally { - saveInFlight = false; + killSwitchSaveInFlight = false; } }); } -/** - * Fetches the global config for the kill-switch toggle so we can merge the - * single field change without overwriting other fields. Returns null on error - * (error is shown via toast before returning). - */ -async function fetchGlobalConfigForLaddering(): Promise { - try { - const resp = await api.getConfig(); - const g = resp.global; - if (!g) { - showToast({ message: 'Could not load global config.', kind: 'error' }); - return null; - } - // Build a Config-shaped payload from GlobalConfig fields that are safe to - // round-trip (the union of GlobalConfig and Config). We pass only the - // fields the backend's updateConfig handler accepts. - // - // IMPORTANT: include every field the backend stores, not just the one we - // are changing. GlobalConfig.NotificationEmail is a *string in Go: omitting - // it from the PUT body decodes as nil and zeroes it out in the DB. - return { - enabled_providers: (g.enabled_providers ?? []) as api.Provider[], - notification_email: g.notification_email, - auto_collect: g.auto_collect ?? true, - collection_schedule: g.collection_schedule ?? 'daily', - notification_days_before: g.notification_days_before ?? 3, - default_term: g.default_term ?? 3, - default_payment: (g.default_payment ?? 'all-upfront') as api.PaymentOption, - default_coverage: g.default_coverage ?? 80, - grace_period_days: g.grace_period_days, - recommendations_cache_stale_hours: g.recommendations_cache_stale_hours, - recommendations_lookback_days: g.recommendations_lookback_days, - laddering_enabled: g.laddering_enabled ?? false, - }; - } catch (err) { - showToast({ message: 'Failed to load global config.', kind: 'error' }); - console.error('fetchGlobalConfigForLaddering:', err); - return null; - } -} - // ========================================== // PER-ACCOUNT CONFIG MODAL // ========================================== @@ -440,7 +400,7 @@ function openLadderConfigModal(existing?: api.LadderConfig): void { // Exported for unit testing (see renderConfigTable note). export async function saveLadderConfig(): Promise { - if (saveInFlight) return; + if (configModalSaveInFlight) return; const maxHourlyRaw = (document.getElementById('ladder-cfg-max-hourly') as HTMLInputElement | null)?.value?.trim() ?? ''; const maxHourly: number | null = maxHourlyRaw === '' ? null : Number(maxHourlyRaw); @@ -480,7 +440,7 @@ export async function saveLadderConfig(): Promise { return; } - saveInFlight = true; + configModalSaveInFlight = true; const saveBtn = document.getElementById('ladder-config-save-btn') as HTMLButtonElement | null; if (saveBtn) saveBtn.disabled = true; @@ -504,7 +464,7 @@ export async function saveLadderConfig(): Promise { showToast({ message: 'Failed to save ladder config. Check inputs and try again.', kind: 'error' }); console.error('saveLadderConfig:', err); } finally { - saveInFlight = false; + configModalSaveInFlight = false; if (saveBtn) saveBtn.disabled = false; } } From 79e75fe1025a865fd8092a720ec25a436c3463a3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 04:18:15 +0200 Subject: [PATCH 4/8] fix(ladder): store approval token as hash not plaintext Rename ladder_runs.approval_token to approval_token_hash in migration 000080 and document that it holds the SHA-256 hex of the run approval token: the raw token is sent only in the approval email link, and the email-approval PR (phase-3 PR-3) must hash-on-write and constant-time compare against this column. The pre-existing raw approval_token columns on purchase_executions and ri_exchange_history are out of scope here and tracked in #1356. --- .../postgres/migrations/000080_ladder_runs.up.sql | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/database/postgres/migrations/000080_ladder_runs.up.sql b/internal/database/postgres/migrations/000080_ladder_runs.up.sql index 232de7783..4c4ac71a5 100644 --- a/internal/database/postgres/migrations/000080_ladder_runs.up.sql +++ b/internal/database/postgres/migrations/000080_ladder_runs.up.sql @@ -39,7 +39,13 @@ CREATE TABLE IF NOT EXISTS ladder_runs ( total_upfront_cost NUMERIC(20,6) NOT NULL DEFAULT 0, estimated_savings NUMERIC(20,6) NOT NULL DEFAULT 0, - approval_token TEXT, + -- Stores the SHA-256 hex digest of the run approval token, never the raw + -- token. The raw token is sent only in the approval email link; the + -- email-approval PR (phase-3 PR-3) must hash-on-write and use a + -- constant-time compare against this column. (The pre-existing + -- purchase_executions / ri_exchange_history approval_token columns still + -- store raw tokens; hashing those is tracked as a separate follow-up.) + approval_token_hash TEXT, approval_token_expires_at TIMESTAMPTZ, approved_by TEXT, cancelled_by TEXT, From 001570133f0e0f470e0a81f4cb6466562916b636 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 4 Jul 2026 05:06:29 +0200 Subject: [PATCH 5/8] fix(api): serialize global_config updates to prevent lost writes The earlier F2 merge fix made updateConfig a read-modify-write across two separate statements (GetGlobalConfig then SaveGlobalConfig), so two overlapping partial PUTs could each read the same stored config and the later save would silently drop the earlier change (lost update / TOCTOU). The laddering kill-switch toggle and a Settings save racing is a realistic trigger. Add UpdateGlobalConfigAtomic(ctx, apply) on the config store: it opens one transaction, takes a transaction-scoped advisory lock, reads the current config, runs the caller's in-place apply (updateConfig passes a closure that json.Unmarshals the request body over the loaded config and validates it), and upserts the result in the same transaction. Concurrent partial PUTs are serialized, so the later writer reads the earlier committed state and no update is lost. Uses an advisory lock rather than SELECT FOR UPDATE because the global_config singleton may not exist yet on first config, and a row-level lock cannot lock a non-existent row; the advisory lock serializes the first insert too. The lock key is derived like the scheduled-task locks (FNV-64a of a namespaced string) so it cannot collide with those or the migration lock. Behavior preserved: first-config defaults, malformed-body 400 before any DB work, present-zero-wins / absent-preserved merge, and best-effort service propagation. GetGlobalConfig/SaveGlobalConfig were refactored into tx-capable helpers; the obsolete mergeGlobalConfigUpdate helper is removed. Tests: a store-level pgxmock test asserting the BEGIN -> advisory lock -> SELECT -> UPSERT -> COMMIT ordering and field round-trip; an apply-error-rolls-back test; a handler test asserting the atomic path is taken; and a -race regression test where two overlapping partial PUTs touching different fields both survive (WaitGroup + channel, no sleeps). --- internal/analytics/collector_test.go | 7 + internal/api/handler_config.go | 63 +++---- internal/api/handler_config_test.go | 154 ++++++++++++++++++ internal/config/interfaces.go | 5 + internal/config/store_postgres.go | 80 ++++++++- .../config/store_postgres_pgxmock_test.go | 112 +++++++++++++ internal/mocks/stores.go | 33 ++++ internal/server/test_helpers_test.go | 7 + 8 files changed, 417 insertions(+), 44 deletions(-) diff --git a/internal/analytics/collector_test.go b/internal/analytics/collector_test.go index 7ce142fb8..05ae41645 100644 --- a/internal/analytics/collector_test.go +++ b/internal/analytics/collector_test.go @@ -747,3 +747,10 @@ func (m *mockConfigStore) GetLadderConfig(_ context.Context, _, _ string) (*conf func (m *mockConfigStore) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { return cfg, nil } +func (m *mockConfigStore) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + cfg := &config.GlobalConfig{} + if err := apply(cfg); err != nil { + return nil, err + } + return cfg, nil +} diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 4695a089f..b9ca76e2c 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -58,53 +58,38 @@ func (h *Handler) getConfig(ctx context.Context, req *events.LambdaFunctionURLRe return resp, nil } -// mergeGlobalConfigUpdate loads the stored GlobalConfig and json.Unmarshals the -// request body over a copy of it, so any field ABSENT from the JSON retains its -// persisted value (present -> updated, absent -> preserved) for EVERY field, -// with no per-field enumeration. This is required for partial PUTs such as the -// laddering kill-switch toggle, which sends only { laddering_enabled }; -// unmarshalling into a zero value would otherwise wipe RI-exchange automation, -// approval_required, default_ramp_schedule, and every other omitted field. -// -// A malformed body is a 400 (rejected before any DB read). A read error fails -// loud rather than merging over a zero base: proceeding without the stored -// config would silently clobber it on a partial PUT (money-adjacent path; no -// silent fallbacks). -func (h *Handler) mergeGlobalConfigUpdate(ctx context.Context, body string) (*config.GlobalConfig, error) { - if !json.Valid([]byte(body)) { - return nil, NewClientError(400, "invalid request body") - } - existing, err := h.config.GetGlobalConfig(ctx) - if err != nil { - return nil, err - } - var cfg config.GlobalConfig - if existing != nil { - cfg = *existing - } - if err = json.Unmarshal([]byte(body), &cfg); err != nil { - return nil, NewClientError(400, "invalid request body") - } - return &cfg, nil -} - func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionURLRequest) (*StatusResponse, error) { // Require update:config permission if _, err := h.requirePermission(ctx, req, "update", "config"); err != nil { return nil, err } - cfg, mergeErr := h.mergeGlobalConfigUpdate(ctx, req.Body) - if mergeErr != nil { - return nil, mergeErr - } - - // Validate the configuration - if err := cfg.Validate(); err != nil { - return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) + // Reject a malformed body before any DB work so a bad request fails fast + // (and never opens a transaction). Keeps the nil-store fast-fail contract. + if !json.Valid([]byte(req.Body)) { + return nil, NewClientError(400, "invalid request body") } - if err := h.config.SaveGlobalConfig(ctx, cfg); err != nil { + // Serialized read-modify-write: the store loads the stored config and + // applies this closure under an advisory-locked transaction, then upserts + // the result in the same tx. json.Unmarshal only assigns keys present in the + // body, so any field ABSENT is preserved (present -> updated, absent -> + // preserved) for EVERY field, with no per-field enumeration. Doing the read + // and write in one locked tx prevents two concurrent partial PUTs (e.g. the + // laddering kill-switch toggle and a Settings save) from reading the same + // stale base and losing each other's update. The closure returns a + // ClientError(400) on a bad body or validation failure, which the store + // propagates unchanged; DB/transport errors surface as 500. + cfg, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error { + if uErr := json.Unmarshal([]byte(req.Body), existing); uErr != nil { + return NewClientError(400, "invalid request body") + } + if vErr := existing.Validate(); vErr != nil { + return NewClientError(400, fmt.Sprintf("validation error: %s", vErr)) + } + return nil + }) + if err != nil { return nil, err } diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index d425b43c6..3d7418c4f 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "sync" "testing" "github.com/LeanerCloud/CUDly/internal/config" @@ -254,6 +255,159 @@ func TestHandler_updateConfig_PartialPUTPreservesOmittedFields(t *testing.T) { assert.Equal(t, "ops@example.com", *saved.NotificationEmail) } +// TestHandler_updateConfig_UsesAtomicPath asserts updateConfig performs its +// read-modify-write through UpdateGlobalConfigAtomic (the serialized, +// advisory-locked store path) rather than a separate Get + Save, which is what +// prevents the concurrent-partial-PUT lost update. The explicit expectation +// means the mock dispatches through Called (it does NOT fall back to the +// Get+Save emulation), so a passing run proves the handler invoked the atomic +// method; the Run callback exercises the exact apply closure the handler builds. +func TestHandler_updateConfig_UsesAtomicPath(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + // The config the atomic apply runs against (as if freshly read under lock). + stored := &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RIExchangeEnabled: true, + RIExchangeMode: "manual", + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + LadderingEnabled: false, + } + + var merged config.GlobalConfig + mockStore.On("UpdateGlobalConfigAtomic", ctx, mock.AnythingOfType("func(*config.GlobalConfig) error")). + Run(func(args mock.Arguments) { + apply := args.Get(1).(func(*config.GlobalConfig) error) + cfg := *stored + require.NoError(t, apply(&cfg)) + merged = cfg + }). + Return(&merged, nil) + mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"laddering_enabled": true}`, + } + result, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + assert.Equal(t, "updated", result.Status) + + // The apply closure merged the body over the stored config: the sent field + // is applied and omitted fields are preserved. + assert.True(t, merged.LadderingEnabled, "sent field applied") + assert.True(t, merged.ApprovalRequired, "omitted approval_required preserved") + assert.True(t, merged.RIExchangeEnabled, "omitted ri_exchange_enabled preserved") +} + +// serializedConfigStore is a concurrency-test store double. Its +// UpdateGlobalConfigAtomic guards the read-modify-write with a real mutex, +// modeling the transaction-scoped advisory lock that +// PostgresStore.UpdateGlobalConfigAtomic takes. It embeds *MockConfigStore only +// to satisfy the rest of StoreInterface; updateConfig calls just the two +// methods overridden here. +type serializedConfigStore struct { + *MockConfigStore + stored *config.GlobalConfig + mu sync.Mutex +} + +func (s *serializedConfigStore) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + s.mu.Lock() + defer s.mu.Unlock() + cfg := *s.stored // read the current committed state under the lock + if err := apply(&cfg); err != nil { + return nil, err + } + s.stored = &cfg // commit before releasing the lock + return &cfg, nil +} + +func (s *serializedConfigStore) ListServiceConfigs(_ context.Context) ([]config.ServiceConfig, error) { + return []config.ServiceConfig{}, nil +} + +// TestHandler_updateConfig_ConcurrentPartialPUTsNoLostUpdate is the F2 +// concurrency regression test (run under -race). Two overlapping partial PUTs +// touch DIFFERENT fields; with the serialized read-modify-write (the store's +// mutex models the advisory lock PostgresStore.UpdateGlobalConfigAtomic takes) +// BOTH updates must survive. A non-atomic Get + merge + Save (the pre-fix path) +// could read the same stale base in both goroutines and let the later save drop +// the earlier change. Coordination uses a WaitGroup + channel (no sleeps); only +// the test goroutine calls require. +func TestHandler_updateConfig_ConcurrentPartialPUTsNoLostUpdate(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + store := &serializedConfigStore{ + MockConfigStore: new(MockConfigStore), + stored: &config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + ApprovalRequired: true, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RIExchangeEnabled: true, + RIExchangeMode: "manual", + RIExchangeLookbackDays: 30, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + }, + } + handler := &Handler{config: store, auth: mockAuth} + + makeReq := func(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + } + } + + var wg sync.WaitGroup + errCh := make(chan error, 2) + for _, body := range []string{ + `{"approval_required": false}`, + `{"ri_exchange_max_daily_usd": 12345}`, + } { + wg.Add(1) + go func(b string) { + defer wg.Done() + _, err := handler.updateConfig(ctx, makeReq(b)) + errCh <- err + }(body) + } + wg.Wait() + close(errCh) + for err := range errCh { + require.NoError(t, err) + } + + // Both concurrent partial updates survived (no lost update). + assert.False(t, store.stored.ApprovalRequired, "approval_required update survived") + assert.Equal(t, 12345.0, store.stored.RIExchangeMaxDailyUSD, "ri_exchange update survived") +} + func TestHandler_updateConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 970e904d3..0ed2cd7c2 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -12,6 +12,11 @@ type StoreInterface interface { // Global configuration GetGlobalConfig(ctx context.Context) (*GlobalConfig, error) SaveGlobalConfig(ctx context.Context, config *GlobalConfig) error + // UpdateGlobalConfigAtomic serializes a read-modify-write of the + // global_config singleton under an advisory-locked transaction so + // concurrent partial PUTs cannot lose each other's updates. apply mutates + // the loaded config in place; its error aborts the write and is propagated. + UpdateGlobalConfigAtomic(ctx context.Context, apply func(*GlobalConfig) error) (*GlobalConfig, error) // Service configuration GetServiceConfig(ctx context.Context, provider, service string) (*ServiceConfig, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index a889cc87b..04d832758 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "hash/fnv" "sort" "strings" "time" @@ -42,8 +43,33 @@ var _ StoreInterface = (*PostgresStore)(nil) // GLOBAL CONFIGURATION // ========================================== -// GetGlobalConfig retrieves the global configuration +// globalConfigExecutor is the subset of query methods needed to read and write +// the global_config singleton. It is satisfied by both the pool (dbConn) and a +// pgx.Tx, so the read/write helpers can run either on the pool directly or +// inside the locked transaction opened by UpdateGlobalConfigAtomic. +type globalConfigExecutor interface { + QueryRow(ctx context.Context, sql string, args ...any) pgx.Row + Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) +} + +// globalConfigLockKey is the advisory-lock key that serializes concurrent +// read-modify-write updates to the global_config singleton (updateConfig +// partial PUTs). Derived like the scheduled-task locks (FNV-64a of a +// namespaced string) so it cannot collide with those or the migration lock. +var globalConfigLockKey = func() int64 { + h := fnv.New64a() + _, _ = h.Write([]byte("cudly:global_config:singleton")) + return int64(h.Sum64()) +}() + +// GetGlobalConfig retrieves the global configuration. func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, error) { + return getGlobalConfigFrom(ctx, s.db) +} + +// getGlobalConfigFrom reads the global_config singleton via q (the pool or a +// transaction), returning defaults when no row exists yet. +func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalConfig, error) { query := ` SELECT enabled_providers, notification_email, approval_required, default_term, default_payment, default_coverage, default_ramp_schedule, @@ -62,7 +88,7 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err var enabledProviders []string var gracePeriodJSON string - err := s.db.QueryRow(ctx, query).Scan( + err := q.QueryRow(ctx, query).Scan( &enabledProviders, &config.NotificationEmail, &config.ApprovalRequired, @@ -87,7 +113,7 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err ) if err != nil { - if err == pgx.ErrNoRows { + if errors.Is(err, pgx.ErrNoRows) { // Return default config if none exists. // Values must align with DefaultSettings in defaults.go and DB DEFAULT clauses. return &GlobalConfig{ @@ -122,8 +148,52 @@ func (s *PostgresStore) GetGlobalConfig(ctx context.Context) (*GlobalConfig, err return &config, nil } -// SaveGlobalConfig saves the global configuration +// SaveGlobalConfig saves the global configuration. func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConfig) error { + return saveGlobalConfigWith(ctx, s.db, config) +} + +// UpdateGlobalConfigAtomic performs a serialized read-modify-write of the +// global_config singleton. It opens a transaction, takes a transaction-scoped +// advisory lock (which serializes even the first insert, since a row-level +// FOR UPDATE cannot lock a not-yet-existing singleton row), reads the current +// config, applies the caller's in-place mutation, and upserts the result in +// the SAME transaction. This eliminates the lost-update race where two +// concurrent partial PUTs each read the same stale base and the later save +// silently drops the earlier change. +// +// apply mutates the loaded config in place (e.g. json.Unmarshal the request +// body over it and validate); an error it returns aborts the transaction and +// is propagated unchanged (so callers can surface a 400 from validation while +// transport/DB errors surface as 500). +func (s *PostgresStore) UpdateGlobalConfigAtomic(ctx context.Context, apply func(*GlobalConfig) error) (*GlobalConfig, error) { + var merged *GlobalConfig + err := s.WithTx(ctx, func(tx pgx.Tx) error { + if _, lockErr := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", globalConfigLockKey); lockErr != nil { + return fmt.Errorf("failed to acquire global_config advisory lock: %w", lockErr) + } + cfg, readErr := getGlobalConfigFrom(ctx, tx) + if readErr != nil { + return readErr + } + if applyErr := apply(cfg); applyErr != nil { + return applyErr + } + if saveErr := saveGlobalConfigWith(ctx, tx, cfg); saveErr != nil { + return saveErr + } + merged = cfg + return nil + }) + if err != nil { + return nil, err + } + return merged, nil +} + +// saveGlobalConfigWith upserts the global_config singleton via q (the pool or a +// transaction). +func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *GlobalConfig) error { // Ensure EnabledProviders is never nil (empty slice is ok, nil is not) if config.EnabledProviders == nil { config.EnabledProviders = []string{} @@ -197,7 +267,7 @@ func (s *PostgresStore) SaveGlobalConfig(ctx context.Context, config *GlobalConf gracePeriodJSON = string(gpBytes) } - _, err := s.db.Exec(ctx, query, + _, err := q.Exec(ctx, query, config.EnabledProviders, config.NotificationEmail, config.ApprovalRequired, diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index d5e047023..cebe69d08 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -167,6 +167,118 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { }) } +// globalConfigCols is the column list returned by the GetGlobalConfig SELECT, +// in scan order. +var globalConfigCols = []string{ + "enabled_providers", "notification_email", "approval_required", + "default_term", "default_payment", "default_coverage", "default_ramp_schedule", + "ri_exchange_enabled", "ri_exchange_mode", "ri_exchange_utilization_threshold", + "ri_exchange_max_per_exchange_usd", "ri_exchange_max_daily_usd", "ri_exchange_lookback_days", + "auto_collect", "collection_schedule", "notification_days_before", + "grace_period_days", + "recommendations_cache_stale_hours", "recommendations_lookback_days", + "purchase_delay_hours", + "laddering_enabled", +} + +// TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite proves the F2 +// lost-update fix: UpdateGlobalConfigAtomic performs the read and the write in +// ONE transaction, guarded by a transaction-scoped advisory lock, in strict +// order: BEGIN -> pg_advisory_xact_lock -> SELECT global_config -> UPSERT -> +// COMMIT. Because pgxmock enforces expectation ORDER, a passing run guarantees +// the read-modify-write is serialized under the lock rather than split across +// two unsynchronized statements (the original TOCTOU). The apply closure +// mutates only one field; the round-tripped result must keep every field the +// closure did not touch, i.e. a concurrent partial update cannot lose another's +// change once the lock serializes them. +func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + // Existing row carries automation settings a partial PUT must not clobber. + seeded := pgxmock.NewRows(globalConfigCols).AddRow( + []string{"aws"}, strPtr("ops@example.com"), true, // approval_required = true + 3, "all-upfront", 80.0, RampImmediate, + true, "automatic", 95.0, // ri_exchange_enabled = true, mode = automatic + 1000.0, 5000.0, 30, + true, "daily", 3, + "{}", + 24, 7, + 48, + false, // laddering_enabled = false + ) + + // Strict order: the SELECT and the UPSERT must sit between the same + // BEGIN/COMMIT, after the advisory lock. + mock.ExpectBegin() + mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) + mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(21)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + // apply flips only laddering_enabled, exactly as the kill-switch partial + // PUT does; everything else must be preserved from the seeded row. + applied := false + merged, err := store.UpdateGlobalConfigAtomic(ctx, func(existing *GlobalConfig) error { + applied = true + existing.LadderingEnabled = true + return nil + }) + require.NoError(t, err) + require.True(t, applied, "apply closure must run against the locked-read config") + require.NotNil(t, merged) + + // Applied change survives. + assert.True(t, merged.LadderingEnabled) + // Fields the closure did NOT touch are preserved (no lost update). + assert.True(t, merged.ApprovalRequired, "approval_required must survive") + assert.True(t, merged.RIExchangeEnabled, "ri_exchange_enabled must survive") + assert.Equal(t, "automatic", merged.RIExchangeMode) + assert.Equal(t, 1000.0, merged.RIExchangeMaxPerExchangeUSD) + assert.Equal(t, 48, merged.PurchaseDelayHours) + + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack asserts that when the +// apply closure fails (e.g. validation), the transaction rolls back and no +// UPSERT is issued, and the closure's error propagates unchanged. +func TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + seeded := pgxmock.NewRows(globalConfigCols).AddRow( + []string{"aws"}, (*string)(nil), true, + 3, "all-upfront", 80.0, RampImmediate, + false, "manual", 95.0, + 0.0, 0.0, 30, + true, "daily", 3, + "{}", + 24, 7, + 0, + false, + ) + + mock.ExpectBegin() + mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) + mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) + // No ExpectExec("INSERT...") — the UPSERT must not run. + mock.ExpectRollback() + + sentinel := errors.New("validation failed") + _, err := store.UpdateGlobalConfigAtomic(ctx, func(_ *GlobalConfig) error { + return sentinel + }) + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) + assert.NoError(t, mock.ExpectationsWereMet()) +} + // ─── GetServiceConfig ───────────────────────────────────────────────────────── func TestPGXMock_GetServiceConfig_Success(t *testing.T) { diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 75f1a6878..9339192ae 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -77,6 +77,39 @@ func (m *MockConfigStore) SaveGlobalConfig(ctx context.Context, cfg *config.Glob return args.Error(0) } +// UpdateGlobalConfigAtomic mocks the atomic read-modify-write. When an +// expectation is registered it dispatches through m.Called (tests can drive the +// apply closure via a Run callback). Otherwise it emulates the real locked +// read-modify-write by routing through the mocked GetGlobalConfig + apply + +// SaveGlobalConfig, so existing tests that seed those two continue to work. +func (m *MockConfigStore) UpdateGlobalConfigAtomic(ctx context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + if isExpected(&m.Mock, "UpdateGlobalConfigAtomic") { + args := m.Called(ctx, apply) + if args.Get(0) == nil { + return nil, args.Error(1) + } + v, ok := args.Get(0).(*config.GlobalConfig) + if !ok { + panic(fmt.Sprintf("mock: expected *config.GlobalConfig, got %T", args.Get(0))) + } + return v, args.Error(1) + } + existing, err := m.GetGlobalConfig(ctx) + if err != nil { + return nil, err + } + if existing == nil { + existing = &config.GlobalConfig{} + } + if err := apply(existing); err != nil { + return nil, err + } + if err := m.SaveGlobalConfig(ctx, existing); err != nil { + return nil, err + } + return existing, nil +} + // GetServiceConfig mocks the GetServiceConfig operation func (m *MockConfigStore) GetServiceConfig(ctx context.Context, provider, service string) (*config.ServiceConfig, error) { args := m.Called(ctx, provider, service) diff --git a/internal/server/test_helpers_test.go b/internal/server/test_helpers_test.go index dd6b75e39..f1962e4c6 100644 --- a/internal/server/test_helpers_test.go +++ b/internal/server/test_helpers_test.go @@ -329,3 +329,10 @@ func (m *mockConfigStoreForHealth) GetLadderConfig(_ context.Context, _, _ strin func (m *mockConfigStoreForHealth) UpsertLadderConfig(_ context.Context, cfg *config.LadderConfigDB) (*config.LadderConfigDB, error) { return cfg, nil } +func (m *mockConfigStoreForHealth) UpdateGlobalConfigAtomic(_ context.Context, apply func(*config.GlobalConfig) error) (*config.GlobalConfig, error) { + cfg := &config.GlobalConfig{} + if err := apply(cfg); err != nil { + return nil, err + } + return cfg, nil +} From 90ccb9b23260938def562cc41fc148f772a448df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 6 Jul 2026 01:32:21 +0200 Subject: [PATCH 6/8] fix(ladder): add snake_case JSON tags to RampSchedule/RampStep The frontend sends and the DB stores the ladder ramp schedule as snake_case ({"steps":[{"after_days":N,"fraction":F}]}), but the pkg/ladder RampStep/RampSchedule structs had no json tags. Go's case-insensitive field matching bridges "fraction" -> Fraction but NOT "after_days" -> AfterDays (the underscore breaks the match), so every AfterDays decoded to 0. Multi-step ramps therefore failed RampSchedule.Validate (AfterDays must be strictly ascending) and were rejected with a 400; a single nonzero step validated but round-tripped through the same untagged struct in the scheduler PR, silently collapsing tranche timing to day 0 (money-affecting timing loss). Add json tags: steps on RampSchedule.Steps, after_days on RampStep.AfterDays, fraction on RampStep.Fraction. The internal/config RampSchedule is a different type (percent-per-step model, PascalCase, used by PurchasePlan) and is deliberately untouched. Tests: a pkg/ladder test asserts a multi-step snake_case ramp (after_days 0/30/60) unmarshals to the right AfterDays, validates, and round-trips to identical snake_case JSON. Both fail against the pre-tag struct (AfterDays 0,0,0 and PascalCase marshal). --- pkg/ladder/types.go | 13 ++++++++++--- pkg/ladder/types_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/pkg/ladder/types.go b/pkg/ladder/types.go index 299d094c2..76e06733f 100644 --- a/pkg/ladder/types.go +++ b/pkg/ladder/types.go @@ -281,14 +281,21 @@ const ( ) // RampStep is a single tranche within a ramp schedule. +// +// The json tags are load-bearing: the frontend sends and the DB stores this +// shape as snake_case (`{"after_days":N,"fraction":F}`). Without the tags Go's +// case-insensitive matching bridges `fraction`->Fraction but NOT +// `after_days`->AfterDays (the underscore breaks the match), so AfterDays would +// silently decode to 0 and multi-step ramps would fail RampSchedule.Validate +// (ascending-AfterDays) or collapse to fire at day 0. type RampStep struct { // AfterDays is the number of days after the run starts at which this // tranche fires. Must be strictly greater than the previous step's // AfterDays (ascending order required by RampSchedule.Validate). - AfterDays int + AfterDays int `json:"after_days"` // Fraction is the share of the total target allocation committed by this // tranche. Must be in (0, 1]; fractions across all steps must sum to 1.0. - Fraction float64 + Fraction float64 `json:"fraction"` } // RampSchedule spreads commitment purchases across time-indexed tranches. @@ -299,7 +306,7 @@ type RampStep struct { // and pkg/ cannot import internal/. See internal/config/types.go for the // internal variant. type RampSchedule struct { - Steps []RampStep + Steps []RampStep `json:"steps"` } // Validate checks that the ramp schedule is well-formed: diff --git a/pkg/ladder/types_test.go b/pkg/ladder/types_test.go index 665c50fe3..9e3c6f3b1 100644 --- a/pkg/ladder/types_test.go +++ b/pkg/ladder/types_test.go @@ -1,6 +1,7 @@ package ladder import ( + "encoding/json" "math" "strings" "testing" @@ -8,6 +9,45 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// TestRampSchedule_JSONSnakeCaseMultiStep is the F1 regression test. The +// frontend sends and the DB stores this shape as snake_case; without json tags +// on RampStep/RampSchedule, Go's case-insensitive field matching binds +// "fraction"->Fraction but NOT "after_days"->AfterDays (the underscore breaks +// the match), so every AfterDays would decode to 0 and a multi-step ramp would +// fail Validate's strictly-ascending check (or, for a single nonzero step, fire +// at day 0). This fails against the pre-tag code. +func TestRampSchedule_JSONSnakeCaseMultiStep(t *testing.T) { + const wantJSON = `{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}` + + var rs RampSchedule + if err := json.Unmarshal([]byte(wantJSON), &rs); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(rs.Steps) != 3 { + t.Fatalf("got %d steps, want 3", len(rs.Steps)) + } + for i, want := range []int{0, 30, 60} { + if rs.Steps[i].AfterDays != want { + t.Errorf("step %d AfterDays = %d, want %d (after_days json tag not applied?)", i, rs.Steps[i].AfterDays, want) + } + } + if rs.Steps[0].Fraction != 0.5 { + t.Errorf("step 0 Fraction = %g, want 0.5", rs.Steps[0].Fraction) + } + if err := rs.Validate(); err != nil { + t.Errorf("Validate() on the multi-step ramp = %v, want nil", err) + } + + // Round-trip: marshal back to the exact snake_case DB/frontend contract. + out, err := json.Marshal(rs) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != wantJSON { + t.Errorf("marshal round-trip = %s, want %s", out, wantJSON) + } +} + func TestLayerTypeValidate(t *testing.T) { t.Parallel() cases := []struct { From 4d1fce10572e597982b5b5f7624d8ec877596c82 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 6 Jul 2026 01:33:04 +0200 Subject: [PATCH 7/8] fix(api): serialize ri-exchange config writes and gate service-config propagation Three global_config write-path fixes surfaced by the phase review: - updateRIExchangeConfig did an unlocked GetGlobalConfig -> mutate -> SaveGlobalConfig writing all 21 columns, so a concurrent kill-switch toggle done via UpdateGlobalConfigAtomic could be silently reverted. Route it through UpdateGlobalConfigAtomic with a closure that mutates only the ri_exchange_* fields, so it shares the advisory lock. This was the last external SaveGlobalConfig caller. - updateConfig unconditionally ran the service-config propagation loop, overwriting every ServiceConfig term/payment/coverage/ramp with the global defaults. A deliberately-partial PUT (the kill-switch toggle sending only { laddering_enabled }) therefore silently rewrote per-service money customizations. Gate propagation on at least one of default_term / default_payment / default_coverage / default_ramp_schedule being present in the raw body (presence-map). - json.Unmarshal into the pre-populated GracePeriodDays map MERGES keys, so a PUT carrying grace_period_days could never delete a previously-set provider. When the key is present, nil the map before unmarshal so the body replaces it wholesale (present -> replace, absent -> preserve). Tests cover both directions: partial PUT skips propagation and keeps service customizations; a defaults PUT still propagates; grace_period_days sent with one key drops the others; ri-exchange writes go through the serialized path. --- internal/api/coverage_extras_test.go | 6 ++- internal/api/handler_config.go | 68 ++++++++++++++++++++++------ internal/api/handler_config_test.go | 58 +++++++++++++++++++++++- internal/api/handler_ri_exchange.go | 30 ++++++------ 4 files changed, 132 insertions(+), 30 deletions(-) diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index ad0e72a73..d385f8e3e 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -255,7 +255,9 @@ func TestHandler_sendPurchaseApprovalEmail_ConfigError(t *testing.T) { } // --------------------------------------------------------------------------- -// updateRIExchangeConfig — GetGlobalConfig error after validation passes +// updateRIExchangeConfig — store error after validation passes. The read now +// happens inside the serialized UpdateGlobalConfigAtomic call, so a store-read +// failure surfaces through that unified path (still a 500-class error). // --------------------------------------------------------------------------- func TestHandler_updateRIExchangeConfig_GetGlobalConfigError(t *testing.T) { @@ -274,7 +276,7 @@ func TestHandler_updateRIExchangeConfig_GetGlobalConfigError(t *testing.T) { } _, err := h.updateRIExchangeConfig(ctx, req) assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to load config") + assert.Contains(t, err.Error(), "failed to save config") } func TestHandler_updateRIExchangeConfig_SaveError(t *testing.T) { diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index b9ca76e2c..9daa5576f 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -70,6 +70,17 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return nil, NewClientError(400, "invalid request body") } + // Presence map of the top-level keys the caller actually sent. Used to + // (a) replace (not merge) grace_period_days when present, and (b) gate the + // service-config propagation on the global defaults actually being sent, so + // a deliberately-partial PUT (e.g. the kill-switch toggle) does not rewrite + // per-service customizations. + var present map[string]json.RawMessage + if err := json.Unmarshal([]byte(req.Body), &present); err != nil { + return nil, NewClientError(400, "invalid request body") + } + _, gracePresent := present["grace_period_days"] + // Serialized read-modify-write: the store loads the stored config and // applies this closure under an advisory-locked transaction, then upserts // the result in the same tx. json.Unmarshal only assigns keys present in the @@ -81,6 +92,13 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR // ClientError(400) on a bad body or validation failure, which the store // propagates unchanged; DB/transport errors surface as 500. cfg, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error { + // grace_period_days is a map: json.Unmarshal into a non-nil map MERGES + // keys (an omitted key can never be deleted). When the caller sends the + // key, nil the stored map first so the body's map REPLACES it wholesale + // (present -> replace); when absent, leave it to preserve. + if gracePresent { + existing.GracePeriodDays = nil + } if uErr := json.Unmarshal([]byte(req.Body), existing); uErr != nil { return NewClientError(400, "invalid request body") } @@ -93,24 +111,48 @@ func (h *Handler) updateConfig(ctx context.Context, req *events.LambdaFunctionUR return nil, err } - // Propagate global defaults to all service configurations + // Propagate global defaults to every service config ONLY when the caller + // actually sent at least one of those defaults. A partial PUT that omits + // them (the kill-switch toggle sends only { laddering_enabled }) must not + // overwrite money-affecting per-service term/payment/coverage/ramp overrides. + if anyKeyPresent(present, "default_term", "default_payment", "default_coverage", "default_ramp_schedule") { + h.propagateGlobalDefaults(ctx, cfg) + } + + return &StatusResponse{Status: "updated"}, nil +} + +// anyKeyPresent reports whether any of keys is present in m. +func anyKeyPresent(m map[string]json.RawMessage, keys ...string) bool { + for _, k := range keys { + if _, ok := m[k]; ok { + return true + } + } + return false +} + +// propagateGlobalDefaults overlays the global term/payment/coverage/ramp +// defaults onto every service config. Best-effort: failures are logged, not +// returned, because the global config was already saved. +func (h *Handler) propagateGlobalDefaults(ctx context.Context, cfg *config.GlobalConfig) { services, err := h.config.ListServiceConfigs(ctx) if err != nil { - // Log but don't fail - global config was saved logging.Warnf("Failed to list service configs for propagation: %v", err) - } else { - for _, svc := range services { - svc.Term = cfg.DefaultTerm - svc.Payment = cfg.DefaultPayment - svc.Coverage = cfg.DefaultCoverage - svc.RampSchedule = cfg.DefaultRampSchedule - if err := h.config.SaveServiceConfig(ctx, &svc); err != nil { - logging.Warnf("Failed to update service config %s/%s: %v", svc.Provider, svc.Service, err) - } + return + } + // Index-based iteration mutates each element in place and avoids copying the + // (large) ServiceConfig struct per iteration (gocritic rangeValCopy). + for i := range services { + svc := &services[i] + svc.Term = cfg.DefaultTerm + svc.Payment = cfg.DefaultPayment + svc.Coverage = cfg.DefaultCoverage + svc.RampSchedule = cfg.DefaultRampSchedule + if saveErr := h.config.SaveServiceConfig(ctx, svc); saveErr != nil { + logging.Warnf("Failed to update service config %s/%s: %v", svc.Provider, svc.Service, saveErr) } } - - return &StatusResponse{Status: "updated"}, nil } // checkCommitmentOptionCombo rejects saves that carry a (term, payment) diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index 3d7418c4f..dedb63c4f 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -216,7 +216,6 @@ func TestHandler_updateConfig_PartialPUTPreservesOmittedFields(t *testing.T) { LadderingEnabled: false, } mockStore.On("GetGlobalConfig", ctx).Return(existing, nil) - mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) var saved config.GlobalConfig mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). @@ -233,6 +232,12 @@ func TestHandler_updateConfig_PartialPUTPreservesOmittedFields(t *testing.T) { require.NoError(t, err) assert.Equal(t, "updated", result.Status) + // F3: a PUT that carries none of the global defaults must NOT trigger the + // service-config propagation loop (which would overwrite per-service + // term/payment/coverage/ramp customizations). + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) + mockStore.AssertNotCalled(t, "SaveServiceConfig", mock.Anything, mock.Anything) + // The one field the body carried was applied. assert.True(t, saved.LadderingEnabled, "laddering_enabled from the body must be applied") @@ -298,7 +303,6 @@ func TestHandler_updateConfig_UsesAtomicPath(t *testing.T) { merged = cfg }). Return(&merged, nil) - mockStore.On("ListServiceConfigs", ctx).Return([]config.ServiceConfig{}, nil) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ @@ -309,6 +313,9 @@ func TestHandler_updateConfig_UsesAtomicPath(t *testing.T) { require.NoError(t, err) assert.Equal(t, "updated", result.Status) + // laddering-only PUT carries no global defaults, so propagation is skipped. + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) + // The apply closure merged the body over the stored config: the sent field // is applied and omitted fields are preserved. assert.True(t, merged.LadderingEnabled, "sent field applied") @@ -316,6 +323,53 @@ func TestHandler_updateConfig_UsesAtomicPath(t *testing.T) { assert.True(t, merged.RIExchangeEnabled, "omitted ri_exchange_enabled preserved") } +// TestHandler_updateConfig_GracePeriodDaysReplaceNotMerge is the F4 regression. +// json.Unmarshal into a pre-populated (non-nil) map MERGES keys, so a PUT that +// carries grace_period_days could never delete a previously-set provider key. +// When the caller sends the key, updateConfig nils the stored map first so the +// body's map REPLACES it wholesale. Sending only { "aws": 10 } must drop the +// prior azure/gcp entries. +func TestHandler_updateConfig_GracePeriodDaysReplaceNotMerge(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockStore.AssertExpectations(t); mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", ctx, "admin-token"). + Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + EnabledProviders: []string{"aws"}, + DefaultTerm: 3, + DefaultPayment: "all-upfront", + DefaultCoverage: 80, + CollectionSchedule: "daily", + NotificationDaysBefore: 3, + RecommendationsCacheStaleHours: 24, + RecommendationsLookbackDays: 7, + GracePeriodDays: map[string]int{"aws": 7, "azure": 14, "gcp": 3}, + }, nil) + + var saved config.GlobalConfig + mockStore.On("SaveGlobalConfig", ctx, mock.AnythingOfType("*config.GlobalConfig")). + Run(func(args mock.Arguments) { saved = *args.Get(1).(*config.GlobalConfig) }). + Return(nil) + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"grace_period_days": {"aws": 10}}`, + } + _, err := handler.updateConfig(ctx, req) + require.NoError(t, err) + + assert.Equal(t, map[string]int{"aws": 10}, saved.GracePeriodDays, + "grace_period_days must be replaced wholesale, not merged (azure/gcp dropped)") + // grace_period_days is not a propagation default, so propagation is skipped. + mockStore.AssertNotCalled(t, "ListServiceConfigs", mock.Anything) +} + // serializedConfigStore is a concurrency-test store double. Its // UpdateGlobalConfigAtomic guards the read-modify-write with a real mutex, // modeling the transaction-scoped advisory lock that diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 2f4d72250..635dc324a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -927,19 +927,23 @@ func (h *Handler) updateRIExchangeConfig(ctx context.Context, req *events.Lambda return nil, err } - globalCfg, err := h.config.GetGlobalConfig(ctx) - if err != nil { - return nil, fmt.Errorf("failed to load config: %w", err) - } - - globalCfg.RIExchangeEnabled = body.AutoExchangeEnabled - globalCfg.RIExchangeMode = body.Mode - globalCfg.RIExchangeUtilizationThreshold = body.UtilizationThreshold - globalCfg.RIExchangeMaxPerExchangeUSD = body.MaxPaymentPerExchangeUSD - globalCfg.RIExchangeMaxDailyUSD = body.MaxPaymentDailyUSD - globalCfg.RIExchangeLookbackDays = body.LookbackDays - - if err := h.config.SaveGlobalConfig(ctx, globalCfg); err != nil { + // Route through the serialized read-modify-write so this partial update + // (only the ri_exchange_* columns) shares the advisory lock with other + // global_config writers. An unlocked GetGlobalConfig -> mutate -> + // SaveGlobalConfig here would write all 21 columns and could silently + // revert a concurrent kill-switch toggle done via UpdateGlobalConfigAtomic. + // The closure mutates ONLY the ri_exchange_* fields; body.validate above + // already validated the inputs, so (matching the prior behavior) we do not + // re-run the whole-config Validate here. + if _, err := h.config.UpdateGlobalConfigAtomic(ctx, func(existing *config.GlobalConfig) error { + existing.RIExchangeEnabled = body.AutoExchangeEnabled + existing.RIExchangeMode = body.Mode + existing.RIExchangeUtilizationThreshold = body.UtilizationThreshold + existing.RIExchangeMaxPerExchangeUSD = body.MaxPaymentPerExchangeUSD + existing.RIExchangeMaxDailyUSD = body.MaxPaymentDailyUSD + existing.RIExchangeLookbackDays = body.LookbackDays + return nil + }); err != nil { return nil, fmt.Errorf("failed to save config: %w", err) } From 8ea9eaa515c4ff8fc8c0ad0f6ee1be07e9da5b02 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 6 Jul 2026 01:33:39 +0200 Subject: [PATCH 8/8] fix(api): strict-decode ladder config PUT and validate account provider Harden the per-account ladder config PUT and clean up two doc/SQL nits: - Decode the body with DisallowUnknownFields so a typo'd key is rejected with 400 instead of being silently dropped. A mistyped max_hourly_commit_per_run would otherwise decode to nil, which the engine reads as "no spend cap" (unbounded spend). - Cross-check the request provider against the cloud account's actual provider and reject a mismatch with 400 (an inert gcp config attached to an AWS account would confuse scheduler scoping). Looking the account up first also turns a nonexistent cloud_account_id into a clean 400 rather than a raw 500 from the store's foreign-key violation. - Update a stale settings.ts comment that referenced the deleted preserveOmittedRecommendationFields helper to describe the atomic merge. - Simplify the 000081 down migration to a single DROP TABLE: run_id and its index live on the table being dropped, so the explicit DROP COLUMN and the "FK-safe order" comment were unnecessary and wrong. Tests: unknown-field, provider-mismatch, and nonexistent-account PUTs each return 400 and never reach the store; the existing valid-config paths still succeed. --- frontend/src/settings.ts | 5 +- internal/api/handler_ladder.go | 33 ++++++++- internal/api/handler_ladder_test.go | 73 +++++++++++++++++++ .../000081_ladder_tranches.down.sql | 15 +--- 4 files changed, 111 insertions(+), 15 deletions(-) diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 464095141..8c1376f2b 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3443,8 +3443,9 @@ export async function saveGlobalSettings(e: Event): Promise { // Include laddering_enabled in the payload when the Purchasing panel's // toggle is present in the DOM (i.e. initLadderingSettings has run). - // When absent (General panel), the backend preserves the existing DB value - // via preserveOmittedRecommendationFields. + // When absent (General panel), the backend's updateConfig merges this PUT + // over the stored config, so an omitted laddering_enabled keeps its + // persisted value rather than being reset. const ladderingToggle = byId('setting-laddering-enabled'); if (ladderingToggle !== null) { settings.laddering_enabled = ladderingToggle.checked; diff --git a/internal/api/handler_ladder.go b/internal/api/handler_ladder.go index 45168fae4..c641134de 100644 --- a/internal/api/handler_ladder.go +++ b/internal/api/handler_ladder.go @@ -1,6 +1,7 @@ package api import ( + "bytes" "context" "encoding/json" "fmt" @@ -42,9 +43,14 @@ func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunc return nil, err } + // DisallowUnknownFields so a typo'd key is rejected loudly instead of + // silently dropped -- e.g. a mistyped "max_hourly_commit_per_run" would + // decode to nil, which the engine reads as "no spend cap" (unbounded). var cfg config.LadderConfigDB - if err := json.Unmarshal([]byte(req.Body), &cfg); err != nil { - return nil, NewClientError(400, "invalid request body") + dec := json.NewDecoder(bytes.NewReader([]byte(req.Body))) + dec.DisallowUnknownFields() + if err := dec.Decode(&cfg); err != nil { + return nil, NewClientError(400, fmt.Sprintf("invalid request body: %s", err)) } if cfg.CloudAccountID == "" { @@ -67,6 +73,10 @@ func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunc return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) } + if err := h.validateLadderAccountProvider(ctx, &cfg); err != nil { + return nil, err + } + result, err := h.config.UpsertLadderConfig(ctx, &cfg) if err != nil { return nil, fmt.Errorf("failed to upsert ladder config: %w", err) @@ -75,6 +85,25 @@ func (h *Handler) upsertLadderConfig(ctx context.Context, req *events.LambdaFunc return result, nil } +// validateLadderAccountProvider confirms the (cloud_account_id, provider) pair +// refers to a real cloud account whose provider matches. This turns a +// nonexistent-account FK violation into a clean 400 (rather than a raw 500 from +// the store's FK constraint) and rejects a provider mismatch -- e.g. an inert +// gcp config attached to an AWS account, which would confuse PR-2 scoping. +func (h *Handler) validateLadderAccountProvider(ctx context.Context, cfg *config.LadderConfigDB) error { + account, err := h.config.GetCloudAccount(ctx, cfg.CloudAccountID) + if err != nil { + return fmt.Errorf("failed to look up cloud account: %w", err) + } + if account == nil { + return NewClientError(400, fmt.Sprintf("cloud account %q does not exist", cfg.CloudAccountID)) + } + if account.Provider != cfg.Provider { + return NewClientError(400, fmt.Sprintf("provider %q does not match cloud account provider %q", cfg.Provider, account.Provider)) + } + return nil +} + // applyLadderConfigNumericDefaults fills a numeric field with its default ONLY // when the field's key is genuinely absent from the request body (tracked via // present). When a key IS supplied, the caller's value is left untouched so it diff --git a/internal/api/handler_ladder_test.go b/internal/api/handler_ladder_test.go index 93707ed68..83afd5d9e 100644 --- a/internal/api/handler_ladder_test.go +++ b/internal/api/handler_ladder_test.go @@ -94,3 +94,76 @@ func TestUpsertLadderConfig_ExplicitZeroTargetCoverageRejected(t *testing.T) { assert.Equal(t, 400, ce.code) mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) } + +// TestUpsertLadderConfig_MultiStepRampAccepted is the F1 end-to-end regression: +// a multi-step ramp (after_days 0 -> 30 -> 60) must be ACCEPTED and round-trip +// to the store unchanged. Against the pre-tag pkg/ladder code every AfterDays +// decoded to 0, so Validate rejected the ramp (not strictly ascending) and this +// PUT returned 400. +func TestUpsertLadderConfig_MultiStepRampAccepted(t *testing.T) { + ctx := context.Background() + handler, _, captured := newLadderHandler(t) + + const ramp = `"ramp_schedule":{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}` + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily",` + ramp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.NoError(t, err) + require.NotNil(t, result) + require.NotNil(t, *captured, "store should have been reached") + // The multi-step ramp round-trips to the store unchanged (raw JSONB). + assert.JSONEq(t, + `{"steps":[{"after_days":0,"fraction":0.5},{"after_days":30,"fraction":0.3},{"after_days":60,"fraction":0.2}]}`, + string((*captured).RampSchedule)) +} + +// TestUpsertLadderConfig_UnknownFieldRejected is F5: a typo'd key must be +// rejected with 400 (DisallowUnknownFields), not silently dropped -- a mistyped +// max_hourly_commit_per_run would otherwise decode to nil = no spend cap. +func TestUpsertLadderConfig_UnknownFieldRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"aws","mode":"email_approval","cadence":"daily","max_hourly_commit_per_runn":5,` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_ProviderMismatchRejected is F5: the request provider +// must match the cloud account's actual provider. The default GetCloudAccount +// mock returns provider "aws"; a body claiming "azure" must 400. +func TestUpsertLadderConfig_ProviderMismatchRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + + body := `{"cloud_account_id":"acct-1","provider":"azure","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} + +// TestUpsertLadderConfig_NonexistentAccountRejected is F5: a cloud_account_id +// that does not resolve must 400 (clean rejection) rather than reaching the +// store and surfacing a raw 500 from the FK constraint. +func TestUpsertLadderConfig_NonexistentAccountRejected(t *testing.T) { + ctx := context.Background() + handler, mockStore, _ := newLadderHandler(t) + mockStore.On("GetCloudAccount", ctx, "ghost").Return(nil, nil) + + body := `{"cloud_account_id":"ghost","provider":"aws","mode":"email_approval","cadence":"daily",` + ladderValidRamp + `}` + result, err := handler.upsertLadderConfig(ctx, ladderReq(body)) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + mockStore.AssertNotCalled(t, "UpsertLadderConfig", mock.Anything, mock.Anything) +} diff --git a/internal/database/postgres/migrations/000081_ladder_tranches.down.sql b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql index d2464bba7..e4040fb20 100644 --- a/internal/database/postgres/migrations/000081_ladder_tranches.down.sql +++ b/internal/database/postgres/migrations/000081_ladder_tranches.down.sql @@ -1,14 +1,7 @@ -- Rollback migration 000081: drop the ladder_tranches table. --- --- The run_id column and its partial index are dropped explicitly first, in --- FK-safe order (index, then the FK-holding column), before the table itself. --- DROP TABLE would remove both implicitly, but listing them keeps the down --- migration an exact inverse of the up. Remaining indexes are dropped --- automatically with the table. - -DROP INDEX IF EXISTS idx_ladder_tranches_run_id; - -ALTER TABLE ladder_tranches - DROP COLUMN IF EXISTS run_id; +-- run_id and every index on the table (including idx_ladder_tranches_run_id) +-- live ON this table, so DROP TABLE removes them all; no separate DROP COLUMN +-- is needed. The run_id -> ladder_runs foreign key is on the table being +-- dropped, so there is no drop-ordering constraint here. DROP TABLE IF EXISTS ladder_tranches;