[IAmJSD]

Root cause

PUT /api/config unmarshals the request body onto the stored GlobalConfig and then runs GlobalConfig.Validate, which never checked RIExchangeMode. So "", "Auto", "automatic" or a typo was stored as-is. PUT /api/ri-exchange/config already accepted only manual or auto, so the two write routes disagreed.

Fix

• internal/config/validation.go: new RIExchangeModeManual / RIExchangeModeAuto constants and IsValidRIExchangeMode (an exact, case-sensitive match with no trimming, because the consumer compares the literal values).
• internal/api/handler_config.go: updateConfig now returns 400 ri_exchange_mode must be 'manual' or 'auto' when the body sets ri_exchange_mode to anything else. This mirrors the dedicated route's mode must be 'manual' or 'auto', using the field name this route takes. The check runs inside the existing advisory-locked closure, before the #1765 arming gate, so a malformed write gets a 400 and never a 403 that reveals the caller's grants. Plain Validate errors are unchanged.
• internal/api/handler_ri_exchange.go: the dedicated route's validate() uses the same predicate. Its message and behavior are unchanged.
• internal/api/handler.go: rewrote the armed() comment. armed() itself stays fail-safe (!= "manual"), as the issue asks.

Deliberate deviation: the check is in the handler, not in GlobalConfig.Validate

The issue prefers GlobalConfig.Validate. I put the check in the handler instead, and it applies only when the body sets ri_exchange_mode (a JSON null does not set it, because json.Unmarshal leaves the stored value in place). The reason: Validate runs on the merged row. If it rejected bad modes, a row stored before this PR with e.g. "Auto" would make every partial PUT /api/config fail with a 400, including the laddering kill-switch toggle ({"laddering_enabled": ...}). The frontend never sends ri_exchange_mode on that route. TestHandler_updateConfig_PartialPUTPreservesOmittedFields already pins a stored "automatic" surviving such a save.

Coverage is the same as in Validate: the only two writers of ri_exchange_mode are this handler and PUT /api/ri-exchange/config, and both now enforce the rule. If you would rather fail loudly on legacy rows, moving the check into Validate is a small change, but the kill-switch lockout would come with it.

Existing rows (one-off check before bumping the go pin past cloud-commitments-go#578)

Rows written before this change may still hold an invalid mode. This PR does not rewrite them. A silent rewrite would change what the scheduler does with them, and the issue asks to keep armed() fail-safe for them. To find one:

SELECT ri_exchange_enabled, ri_exchange_mode FROM global_config
WHERE ri_exchange_mode NOT IN ('manual', 'auto');

To repair one, send a valid mode through either route, e.g. PUT /api/config with {"ri_exchange_mode":"manual"} (covered by a test). Sending the same bad value back is rejected. No migration is included. If you want one (normalize to 'manual' plus a CHECK constraint), it can follow separately.

Verification

All of this is unit / mock-store evidence through Router.Route. There is no real Postgres or cloud coverage. No DB code changed, and the new check is a pure function inside the existing advisory-locked UpdateGlobalConfigAtomic closure, so it adds no new concurrency surface and I wrote no race test. Tests were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail before / pass after. The new test file internal/api/handler_config_ri_exchange_mode_test.go uses literal mode strings, so it compiles against main. I ran it on main before writing the fix:

go test ./internal/api/ -count=1 -v -run 'TestUpdateConfig_.*RIExchangeMode|TestUpdateConfig_LegacyStoredMode'

• Before the fix: TestUpdateConfig_RejectsInvalidRIExchangeMode failed in all 9 subtests (Auto, AUTO, Manual, "", atuo, automatic, enabled, " auto", "manual "). TestUpdateConfig_InvalidRIExchangeModeRejectedBeforeGate and TestUpdateConfig_LegacyStoredModeResentIsRejected also failed (403 or stored instead of 400).
• After the fix: all pass. The accept, legacy-preserve, null and repair tests pass both before and after; they are scope controls.

Retargeted test. TestRIExchangeAutoModeGate_NonManualModeIsArmed used to send invalid modes on PUT /api/config and expect 403. Those writes now get a 400, so the test now seeds the legacy mode as the stored row and sends only {"ri_exchange_enabled":true}. That is the case where armed() staying fail-safe still matters.

Mutation probes. Each was applied, the tests run, then the change reverted. All five were killed:

Mutation	Killed by
treat JSON null as "sent"	LegacyStoredModeDoesNotBlockUnrelatedWrites/explicit_JSON_null, /null_with_whitespace
validate the merged value regardless of presence (what moving it into Validate does)	LegacyStoredModeDoesNotBlockUnrelatedWrites/*, NonManualModeIsArmed/*
drop the mode check	RejectsInvalidRIExchangeMode/*
case-insensitive / trimmed match	RejectsInvalidRIExchangeMode/{Auto,AUTO,Manual,_auto,manual_}
armed() as mode == "auto"	NonManualModeIsArmed/{Auto,…,"",enabled}

Package runs and checks.

go build ./...
go test ./internal/config/ ./internal/api/ ./internal/mocks/ ./internal/server/ -count=1   # one package at a time, all ok
go vet ./internal/config/ ./internal/api/ && go vet -tags integration ./internal/api/ ./internal/config/
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/... ./internal/config/...   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api internal/config                        # silent
bash scripts/gosec-hook.sh <changed .go files>                                            # exit 0
git secrets --scan   # in an isolated temp repo holding the changed files: clean; a planted fake key was flagged
scripts/test-git-secrets-allowlist.sh                                                     # 137 PASS, 0 FAIL

Out of scope

• No migration and no backfill of existing rows (see above).
• GlobalConfig.Validate is unchanged. "" stays valid there as "unset"; saveGlobalConfigWith already stores "" as manual.

Frontend follow-up for the UI owner

None needed. The frontend does not send ri_exchange_mode on PUT /api/config. If it ever shows this route's 400 text, the message is ri_exchange_mode must be 'manual' or 'auto'.

Closes #1196

Maintainers: please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/internal, effort/xs, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES 9616ae7

Findings:

• [blocking] internal/api/handler_config.go:82,139-142 - Whether the mode was "sent" is decided from the presence map (present["ri_exchange_mode"], an exact key match).
  • The body is then applied with json.Unmarshal onto GlobalConfig, which matches keys case-insensitively and lets the last duplicate win.
  • So a body can set RIExchangeMode while modeSent stays false, and the check is skipped.
  • Reproduced on head through Router.Route with a probe test. All three bodies returned 200 and stored an invalid value:

Body	Stored
{"RI_EXCHANGE_MODE":"Auto"}	"Auto"
{"Ri_Exchange_Mode":"automatic"}	"automatic"
{"ri_exchange_mode":"Auto","ri_exchange_mode":null}	"Auto" (the map sees null; the struct keeps "Auto")

• So the invariant this PR exists for, that the platform never stores anything but manual or auto, does not hold.
• After the go#578 pin bump, such a row with exchange enabled makes the scheduled run fail with "auto exchange failed" (internal/server/handler_ri_exchange.go:111-128).
• Suggested fix: decide from values, not keys. Validate whenever the merged existing.RIExchangeMode != before.mode, using the snapshot already taken at :106.
  • That covers case-folded keys, duplicates and null with no special case, and still leaves untouched legacy rows alone.
  • With that fix, LegacyStoredModeResentIsRejected becomes a harmless no-op write, so adjust or drop that test.
• Alternatively: add a migration that normalises the column to 'manual' and adds CHECK (mode IN ('manual','auto')). That closes it at the DB layer for every writer.
• [non-blocking, already on main, worth a known-issues entry] handler_config.go:81 - gracePresent has the same case-sensitivity hole: {"Grace_Period_Days":{...}} merges into the stored map instead of replacing it.
• [non-blocking] Legacy rows get no migration and no read-time guard. Migration 000010 has DEFAULT 'manual' but no CHECK.
  • A stored "automatic" with ri_exchange_enabled=true will fail loudly after fix(api): GET /api/purchases/cancel/{id} cancels an execution on a side-effecting GET #578, not be silently rewritten.
  • The PR body documents a SELECT that finds such rows. That is acceptable, but someone must run it before the pin bump.
• [non-blocking, predates this PR] internal/api/openapi.yaml:3241-3267, 1462-1474 - OpenAPI enumerates the mode nowhere.
  • The GlobalConfig schema omits every ri_exchange_* field.
  • PUT /api/ri-exchange/config has an untyped type: object body.

Attacked:

• Writers:
  • PUT /api/config: checked only on an exact key match (the bypass above).
  • PUT /api/ri-exchange/config (handler_ri_exchange.go:2857): exact match, and a missing mode decodes to "" and gets a 400.
  • store_postgres.go:313-315 saves "" as "manual", and the no-row default at :153 is "manual".
  • Migration 000010: DEFAULT 'manual', no CHECK.
  • No ladder, DynamoDB or seed code writes the field.
  • IsValidRIExchangeMode rejects "Auto", " auto" and "manual ", and tests pin that.
• Frontend:
  • riexchange.ts:81 builds its select from {manual, auto}, typed "manual" | "auto" (types.ts:847).
  • The settings, ladder and recommendations callers of updateConfig never send ri_exchange_mode.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, the check dropped: caught.
  • M2, null treated as sent: caught by explicit_JSON_null and null_with_whitespace.
  • M3, "Auto" and " auto" accepted: caught.
  • M4, no mode check on the dedicated route: TestHandler_updateRIExchangeConfig_ValidationError FAILS.
  • M5, armed() as mode == "auto": NonManualModeIsArmed FAILS.
  • M6, the merged value always validated: 9 tests FAIL, including PartialPUTPreservesOmittedFields.
  • No test covers case-folded or duplicate keys.

Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• git merge-tree --write-tree origin/main 9616ae72 against origin/main 6af022f: clean.
• go build ./... and go vet ./internal/config/ ./internal/api/: ok. gofmt -l on the 7 changed files: clean.
• go test -count=1 ./internal/config/ ./internal/api/: ok.
• Fail-before: 3 tests FAIL on base: RejectsInvalidRIExchangeMode, InvalidRIExchangeModeRejectedBeforeGate and LegacyStoredModeResentIsRejected.
• Bypass probe: all 3 bypass bodies are accepted on head.
• No migration. Frontend jest was skipped, because node_modules are absent from the export.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: REQUEST_CHANGES 9616ae7

Findings:

• BLOCKING internal/api/handler_config.go:82 and :137-141 - the presence check reads present["ri_exchange_mode"], an exact-key lookup on map[string]json.RawMessage. The struct unmarshal at :117 matches keys case-insensitively, and the last duplicate key wins. Reproduced through Router.Route on the merged tree (PUT /api/config, a principal with execute:ri-exchange, a disarmed row). Every one of these returned 200 and called SaveGlobalConfig with an invalid mode:
  {"RI_EXCHANGE_MODE":"Auto"} stored "Auto"
  {"Ri_Exchange_Mode":"automatic"} stored "automatic"
  {"ri_exchange_mode":"Auto","ri_exchange_mode":null} stored "Auto"
  {"RI_EXCHANGE_MODE":"bogus","ri_exchange_mode":null} stored "bogus"
  Failure scenario: fix(config): validate RIExchangeMode on PUT /api/config #1196 stays open. After the go#578 pin bump, RunAutoExchange fails closed on that row. The arming gate still holds: with admin-only rights, arming an enabled row through RI_EXCHANGE_MODE gets 403. So this is an integrity bug, not an authorization bypass.
  Fix: validate on the merged value whenever it changed, if (modeSent || existing.RIExchangeMode != before.mode) && !config.IsValidRIExchangeMode(existing.RIExchangeMode). before is already the advisory-locked snapshot. Probe: with only existing.RIExchangeMode != before.mode, all 6 bypass bodies return 400. A legacy row still saves partially: the LegacyStoredModeDoesNotBlockUnrelatedWrites and null cases pass, and only LegacyStoredModeResentIsRejected fails. Keeping modeSent || makes that test pass as well. A DB CHECK alone is not enough, because legacy rows would need a migration and the issue asks to keep them.
• nit, existing bug, not this PR: internal/api/handler_config.go:81. gracePresent has the same exact-key lookup, so {"GRACE_PERIOD_DAYS":{...}} merges into the stored map instead of replacing it.
• Commit size: one commit with 244 changed lines, under the cap.

The rest is correct: the IsValidRIExchangeMode predicate matches exactly, the dedicated route keeps its behavior, the 400 comes before the gate's 403, and armed() stays fail-safe. An empty "" mode is stored as "manual" by the store (store_postgres.go:313), so it does no harm.

Merge check: git merge-tree origin/main(6776e27e) 9616ae72 is clean (tree b4e60219). On the merged tree, go build ./... and go vet are clean. Unit tests for api, config, mocks and server pass.

Attacked (scratch copy, each change reverted and checked with cmp):

1. Mode check dropped: 12 failures.
2. JSON null treated as sent: the explicit_JSON_null and null_with_whitespace cases failed.
3. Predicate made case-insensitive and trimmed: the Auto, AUTO, Manual, " auto" and "manual " subtests failed.
4. The case-fold and duplicate-key bypass probe: confirmed, see above.
5. The proposed before-snapshot fix: it closes every bypass, as described above.

CI on 9616ae7: Security Scanning, Build Docker Image and CI Success are red. All three fail on govulncheck GO-2026-6809, crypto/x509 in go1.26.9, which comes from the old base c8bdd46. Main is now on 1.26.10, so a rebase should clear them. Everything else passes, including Integration Tests.