feat(ladder): add per-account config + schema foundation (default-off)#1355
Open
cristim wants to merge 5 commits into
Open
feat(ladder): add per-account config + schema foundation (default-off)#1355cristim wants to merge 5 commits into
cristim wants to merge 5 commits into
Conversation
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
cristim
added
priority/p1
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds commitment-laddering support across database schema, backend storage/validation/API, and frontend settings UI, including a global ChangesCommitment Laddering feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LadderUI as ladder.ts
participant Api as api/ladder.ts
participant Router as internal/api/router.go
participant Handler as internal/api/handler_ladder.go
participant Store as config store
User->>LadderUI: Submit ladder config form
LadderUI->>LadderUI: validate input and build LadderConfig
LadderUI->>Api: upsertLadderConfig(cfg)
Api->>Router: PUT /api/ladder/configs
Router->>Handler: upsertLadderConfig(req)
Handler->>Handler: parse JSON, apply defaults, validate
Handler->>Store: UpsertLadderConfig(cfg)
Store-->>Handler: persisted config
Handler-->>Router: response
Router-->>Api: response body
Api-->>LadderUI: updated config
LadderUI->>LadderUI: update cache, rerender table, close modal
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
Note on CI: the Build frontend job failure is pre-existing debt, not introduced by this PR. The single blocking eslint error is |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler_config.go (1)
105-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
GetGlobalConfigfailure silently zeroes omitted fields instead of failing loud — undermines the kill-switch preservation guarantee.When
h.config.GetGlobalConfig(ctx)errors, the function returnsnil(success) without preserving anything:existing, gcErr := h.config.GetGlobalConfig(ctx) if gcErr != nil || existing == nil { return nil }
updateConfigthen proceeds tocfg.Validate()andSaveGlobalConfig, persisting whatever zero values the initialjson.Unmarshalleft incfgfor any omitted key — includingLadderingEnabled = falseif the request body omittedladdering_enabled(e.g. a General-panel save). This is precisely the "silently reset the kill-switch back to false" scenario the comment on Line 92-94 explicitly says must be avoided, and it also silently resets the olderrecommendations_cache_stale_hours/recommendations_lookback_days/purchase_delay_hoursfields on a transient store read failure. The PR's stated design goal is fail-loud validation on money-path fields, but this path fails silently on a DB error instead of surfacing it.🐛 Proposed fix — propagate the read error instead of swallowing it
existing, gcErr := h.config.GetGlobalConfig(ctx) - if gcErr != nil || existing == nil { + if gcErr != nil { + return fmt.Errorf("failed to load existing config for field preservation: %w", gcErr) + } + if existing == nil { return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_config.go` around lines 105 - 121, `updateConfig` in `handlerConfig` is swallowing `GetGlobalConfig` read failures and returning success, which can persist zero-valued omitted fields and break the kill-switch preservation behavior. Change the `GetGlobalConfig` handling so `gcErr` is returned to the caller instead of treated like success, and keep the merge logic in this function using `existing` only when the read succeeds. This preserves the omitted-field fallback for `RecommendationsCacheStaleHours`, `RecommendationsLookbackDays`, `PurchaseDelayHours`, and `LadderingEnabled` while failing loud on config store errors.
🧹 Nitpick comments (5)
frontend/src/__tests__/ladder.test.ts (1)
13-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding regression coverage for the kill-switch save payload.
Good coverage for XSS escaping and the max-hourly guard. Once the
fetchGlobalConfigForLadderingfield-completeness fix (flagged inladder.ts) lands, consider a test assertingapi.updateConfigis called with all persistedConfigfields (not just the ones currently listed) when the kill-switch toggle fires, so a future field addition toConfigdoesn't silently regress into the same zeroing bug.Want me to draft this test?
Also applies to: 104-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/ladder.test.ts` around lines 13 - 18, Add regression coverage around the kill-switch save path in the ladder tests: once the field-completeness fix in fetchGlobalConfigForLaddering lands, update the ladder toggle test so it verifies api.updateConfig is invoked with the full persisted Config payload, not only the currently enumerated fields. Use the existing ladder.ts save flow and api.updateConfig mock in ladder.test.ts to assert future Config additions are preserved and not zeroed out by omission.internal/database/postgres/migrations/000081_ladder_tranches.up.sql (1)
25-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSame CHECK-constraint consideration for
layer_type,term,payment_option,status.Consistent with the earlier comments on this same recurring pattern in
000079_ladder_configs.up.sqland000080_ladder_runs.up.sql, these enum-like TEXT columns have no CHECK constraints. Not blocking given app-layer validation viapkg/ladder, but consider consolidating into constraints across all three tables in a follow-up migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000081_ladder_tranches.up.sql` around lines 25 - 31, Add CHECK constraints for the enum-like TEXT columns in this migration’s ladder tranche table definition, matching the same pattern discussed for the related ladder config/run migrations. Update the CREATE TABLE statement that defines layer_type, term, payment_option, and status so each is constrained to its allowed values, and keep the constraint names consistent with the existing ladder migration naming style.internal/database/postgres/migrations/000079_ladder_configs.up.sql (1)
19-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider CHECK constraints for enum/text and numeric-range columns.
provider,mode,cadenceare free-form TEXT with valid values documented only in comments (line 25-26), andtarget_coverage,buffer_fraction,baseline_percentile,buffer_utilization_threshold,lookback_days,max_actions_per_runhave no bound checks. Sincepkg/laddervalidates these at the application layer, this is not blocking, but adding CHECK constraints would guard against bad data from future direct writes, ad-hoc scripts, or bugs bypassing app validation on a table that ultimately drives real financial commitments.💡 Example CHECK constraints
provider TEXT NOT NULL, enabled BOOLEAN NOT NULL DEFAULT false, - mode TEXT NOT NULL, -- email_approval | auto_approve - cadence TEXT NOT NULL, -- daily | weekly + mode TEXT NOT NULL + CHECK (mode IN ('email_approval', 'auto_approve')), + cadence TEXT NOT NULL + CHECK (cadence IN ('daily', 'weekly')), target_coverage NUMERIC(5,2) NOT NULL, - buffer_fraction NUMERIC(5,4) NOT NULL, + buffer_fraction NUMERIC(5,4) NOT NULL CHECK (buffer_fraction BETWEEN 0 AND 1),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000079_ladder_configs.up.sql` around lines 19 - 39, Add CHECK constraints to the ladder_configs table so the database enforces the same value rules already implied by pkg/ladder validation. Update the migration for ladder_configs to restrict provider, mode, and cadence to the allowed text values, and add range checks for target_coverage, buffer_fraction, baseline_percentile, buffer_utilization_threshold, lookback_days, and max_actions_per_run. Keep the constraints close to the existing CREATE TABLE definition so future direct writes or bypasses cannot insert invalid ladder_configs data.internal/database/postgres/migrations/000080_ladder_runs.up.sql (1)
24-24: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSame CHECK-constraint consideration applies to
status.Consistent with the earlier comment on
internal/database/postgres/migrations/000079_ladder_configs.up.sql(lines 19-39),statusis a free-form TEXT enum documented only in a comment. Consider a CHECK constraint against the documented lifecycle values for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000080_ladder_runs.up.sql` at line 24, The status column in the ladder runs migration is still a free-form TEXT value, so add a CHECK constraint in the same migration to restrict it to the documented lifecycle states. Update the ladder run table definition in 000080_ladder_runs.up.sql so the constraint matches the statuses used by the ladder run model and stays consistent with the earlier ladder config migration.internal/config/validation.go (1)
558-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the ladder bounds checks to avoid drift.
internal/config.LadderConfigDB.Validate()mirrorspkg/ladder.LadderConfig’s range checks for coverage, buffer fraction, baseline percentile, and run limits, so the two paths can diverge over time. Exporting a small shared helper (or moving the bounds logic into one shared validator) would keep both configs aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/validation.go` around lines 558 - 582, `LadderConfigDB.Validate` is duplicating the ladder bounds logic that already exists in `pkg/ladder.LadderConfig`, which can drift over time. Refactor the range checks for coverage, buffer fraction, baseline percentile, and run limits into a shared helper/validator and have both `LadderConfigDB.Validate` and the `pkg/ladder` validation path call it so they use the same invariants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/ladder.ts`:
- Line 26: The module-level save guard is shared between two unrelated flows,
causing the kill-switch toggle and the per-account modal save to block or revert
each other. Split the current `saveInFlight` state in `ladder.ts` into separate
flags for each flow, then update `wireKillSwitchToggle` to use
`killSwitchSaveInFlight` and `saveLadderConfig` to use `configModalSaveInFlight`
so each operation is independently gated.
- Around line 343-382: The payload built in fetchGlobalConfigForLaddering is
missing persisted GlobalConfig fields, so the kill-switch save can overwrite
unrelated settings. Update this helper to round-trip every backend-stored field
handled by updateConfig/SaveGlobalConfig, including approval_required,
default_ramp_schedule, and the ri_exchange_* values, alongside the existing
laddering_enabled and other fields. Keep the shape aligned with api.Config so
toggling laddering preserves all other global settings.
In `@internal/database/postgres/migrations/000080_ladder_runs.up.sql`:
- Around line 42-46: The ladder runs approval schema currently stores
approval_token as plaintext TEXT, which should be changed to a hashed-token
storage pattern before the approval flow depends on it. Update the migration
around the approval_token and approval_token_expires_at columns so the database
only persists a hash (using a dedicated hashed column or equivalent naming in
this migration), and keep the raw token for email delivery only in application
logic. Preserve the existing ladder_runs schema fields like approved_by,
cancelled_by, and fire_at while making the approval token storage safe by
default.
---
Outside diff comments:
In `@internal/api/handler_config.go`:
- Around line 105-121: `updateConfig` in `handlerConfig` is swallowing
`GetGlobalConfig` read failures and returning success, which can persist
zero-valued omitted fields and break the kill-switch preservation behavior.
Change the `GetGlobalConfig` handling so `gcErr` is returned to the caller
instead of treated like success, and keep the merge logic in this function using
`existing` only when the read succeeds. This preserves the omitted-field
fallback for `RecommendationsCacheStaleHours`, `RecommendationsLookbackDays`,
`PurchaseDelayHours`, and `LadderingEnabled` while failing loud on config store
errors.
---
Nitpick comments:
In `@frontend/src/__tests__/ladder.test.ts`:
- Around line 13-18: Add regression coverage around the kill-switch save path in
the ladder tests: once the field-completeness fix in
fetchGlobalConfigForLaddering lands, update the ladder toggle test so it
verifies api.updateConfig is invoked with the full persisted Config payload, not
only the currently enumerated fields. Use the existing ladder.ts save flow and
api.updateConfig mock in ladder.test.ts to assert future Config additions are
preserved and not zeroed out by omission.
In `@internal/config/validation.go`:
- Around line 558-582: `LadderConfigDB.Validate` is duplicating the ladder
bounds logic that already exists in `pkg/ladder.LadderConfig`, which can drift
over time. Refactor the range checks for coverage, buffer fraction, baseline
percentile, and run limits into a shared helper/validator and have both
`LadderConfigDB.Validate` and the `pkg/ladder` validation path call it so they
use the same invariants.
In `@internal/database/postgres/migrations/000079_ladder_configs.up.sql`:
- Around line 19-39: Add CHECK constraints to the ladder_configs table so the
database enforces the same value rules already implied by pkg/ladder validation.
Update the migration for ladder_configs to restrict provider, mode, and cadence
to the allowed text values, and add range checks for target_coverage,
buffer_fraction, baseline_percentile, buffer_utilization_threshold,
lookback_days, and max_actions_per_run. Keep the constraints close to the
existing CREATE TABLE definition so future direct writes or bypasses cannot
insert invalid ladder_configs data.
In `@internal/database/postgres/migrations/000080_ladder_runs.up.sql`:
- Line 24: The status column in the ladder runs migration is still a free-form
TEXT value, so add a CHECK constraint in the same migration to restrict it to
the documented lifecycle states. Update the ladder run table definition in
000080_ladder_runs.up.sql so the constraint matches the statuses used by the
ladder run model and stays consistent with the earlier ladder config migration.
In `@internal/database/postgres/migrations/000081_ladder_tranches.up.sql`:
- Around line 25-31: Add CHECK constraints for the enum-like TEXT columns in
this migration’s ladder tranche table definition, matching the same pattern
discussed for the related ladder config/run migrations. Update the CREATE TABLE
statement that defines layer_type, term, payment_option, and status so each is
constrained to its allowed values, and keep the constraint names consistent with
the existing ladder migration naming style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b68e2785-9664-4d88-8ef2-6b7d1ca4b0d3
📒 Files selected for processing (29)
frontend/src/__tests__/ladder.test.tsfrontend/src/api/index.tsfrontend/src/api/ladder.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/ladder.tsfrontend/src/settings.tsfrontend/src/types.tsinternal/analytics/collector_test.gointernal/api/handler_config.gointernal/api/handler_config_test.gointernal/api/handler_ladder.gointernal/api/handler_ladder_test.gointernal/api/router.gointernal/config/interfaces.gointernal/config/store_postgres.gointernal/config/store_postgres_ladder.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_ladder_test.gointernal/database/postgres/migrations/000079_ladder_configs.down.sqlinternal/database/postgres/migrations/000079_ladder_configs.up.sqlinternal/database/postgres/migrations/000080_ladder_runs.down.sqlinternal/database/postgres/migrations/000080_ladder_runs.up.sqlinternal/database/postgres/migrations/000081_ladder_tranches.down.sqlinternal/database/postgres/migrations/000081_ladder_tranches.up.sqlinternal/mocks/stores.gointernal/server/test_helpers_test.go
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.
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<Config>, and the
fetchGlobalConfigForLaddering helper is removed.
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.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/ladder.ts (1)
421-436: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCleared numeric inputs silently become
0, not the documented default.
??only falls back when the left side isnull/undefined— an<input>'s.valueis always a string, so clearing the field yields'', andNumber('')is0, not the intended default. This affectstarget_coverage,buffer_fraction,baseline_percentile,lookback_days,buffer_utilization_threshold, andmax_actions_per_run.maxHourlyRawjust above (Lines 405-410) correctly special-cases the empty string — the other fields don't, so a cleared field silently submits0for a per-account, money-adjacent ladder setting instead of falling back to the shown default.🔧 Proposed fix: treat empty string as "use default" for numeric fields
+ const numOrDefault = (id: string, fallback: number): number => { + const raw = (document.getElementById(id) as HTMLInputElement | null)?.value?.trim() ?? ''; + return raw === '' ? fallback : Number(raw); + }; + const cfg: api.LadderConfig = { ... - 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'), + target_coverage: numOrDefault('ladder-cfg-target-coverage', 100), + buffer_fraction: numOrDefault('ladder-cfg-buffer-fraction', 0.10), + baseline_percentile: numOrDefault('ladder-cfg-baseline-percentile', 5), + lookback_days: numOrDefault('ladder-cfg-lookback-days', 30), + buffer_utilization_threshold: numOrDefault('ladder-cfg-buf-util-threshold', 90), max_hourly_commit_per_run: maxHourly, - max_actions_per_run: Number((document.getElementById('ladder-cfg-max-actions') as HTMLInputElement | null)?.value ?? '10'), + max_actions_per_run: numOrDefault('ladder-cfg-max-actions', 10), ramp_schedule: rampSchedule, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/ladder.ts` around lines 421 - 436, The numeric fields in the ladder config builder are treating cleared inputs as 0 instead of the intended defaults. Update the cfg assembly in ladder.ts so the numeric reads for target_coverage, buffer_fraction, baseline_percentile, lookback_days, buffer_utilization_threshold, and max_actions_per_run explicitly treat an empty string as “use the default” before calling Number(), similar to how maxHourlyRaw is handled above. Keep the fix localized to the LadderConfig construction logic and preserve the existing defaults shown in the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_config.go`:
- Around line 61-89: The current mergeGlobalConfigUpdate/updateConfig flow can
lose concurrent partial PUT changes because it reads the stored GlobalConfig and
later writes the merged snapshot without any concurrency guard. Fix this by
adding a row lock or optimistic version check around the read/merge/write
sequence in mergeGlobalConfigUpdate and the save path in updateConfig so
overlapping updates cannot overwrite each other silently. Ensure the
locking/versioning is applied to the global_config record used by Handler.
---
Outside diff comments:
In `@frontend/src/ladder.ts`:
- Around line 421-436: The numeric fields in the ladder config builder are
treating cleared inputs as 0 instead of the intended defaults. Update the cfg
assembly in ladder.ts so the numeric reads for target_coverage, buffer_fraction,
baseline_percentile, lookback_days, buffer_utilization_threshold, and
max_actions_per_run explicitly treat an empty string as “use the default” before
calling Number(), similar to how maxHourlyRaw is handled above. Keep the fix
localized to the LadderConfig construction logic and preserve the existing
defaults shown in the UI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 412386d5-c3af-4d04-870b-957be617449a
📒 Files selected for processing (5)
frontend/src/api/settings.tsfrontend/src/ladder.tsinternal/api/handler_config.gointernal/api/handler_config_test.gointernal/database/postgres/migrations/000080_ladder_runs.up.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/database/postgres/migrations/000080_ladder_runs.up.sql
- internal/api/handler_config_test.go
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).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Foundation for commitment laddering (phase-3 PR-1 of tracker #1333). 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:
ladder_configs(per account x provider),ladder_runs(immutable audit),ladder_tranches(timed purchase slices with arun_idtrace toladder_runs);global_config.laddering_enabledkill-switch;ladder_run_idFK onpurchase_executionsandri_exchange_history. All reversible.GetLadderConfigs/GetLadderConfig/UpsertLadderConfig, plusStoreInterfacewidening and mock coverage.GET/PUT/api/ladder/configswith RBAC and fail-loud validation. Out-of-range numerics are rejected with 400 and never silently defaulted; absent keys take their default, and an explicitbuffer_fraction=0is honored as "no buffer".Money-path fields are nullable (
NULL, not0) and typed enums are validated againstpkg/ladderrather than redefined.Verification
go build ./...,go vet ./...(whole module): passgolangci-lint --new-from-rev=origin/mainon internal/api, internal/config, internal/server, internal/analytics: 0 new issuesgo test ./...(whole root module): pass, 0 failurestsctypecheck,eslint, andjest(78 suites, 2557 passed, 1 skipped): passladder_tranches.run_id -> ladder_runsFK verified; final version 81, not dirtyLadderConfigDB.Validate()bounds coverage, handler tests for the fail-loud defaulting contract, and frontend jsdom tests forrenderConfigTableXSS escaping and thesaveLadderConfigmax-hourly guardOut of scope (follow-up phase-3 PRs)
ladder_runscheduled task, email-approval mode, auto-approve mode, run-history UI, and the Terraform EventBridge tick.Part of #1336 (phase-3 PR-1)
Refs #1333
Summary by CodeRabbit