Repository navigation
fix(ri-exchange): run unattended exchanges as manual while 4-eyes is on - #1108
Conversation
With RIExchangeEnabled and a non-manual RIExchangeMode, the scheduled ri_exchange_reshape task called exchange.RunAutoExchange in auto mode and executed exchanges with no approver, even with RequireDifferentApprover on. The direct execute endpoints already refuse under 4-eyes (LeanerCloud#673), so auto mode was the remaining path that moved money with no second person. With 4-eyes on, every non-manual mode ("auto", "" and typos, which pkg/exchange all treats as auto) now runs as "manual": the run only raises pending records with approval tokens, sends the pending-approval email, and each exchange then goes through the approve endpoint and its 4-eyes check. The stored mode is unchanged, so turning 4-eyes off brings auto back. The ladder exchange runner gets the same downgrade; it reads the global config itself and fails closed (no AWS call) when it cannot. Also tightens the LeanerCloud#673 tests: the AWS direct-execute 4-eyes tests now spy on the exchange executor and assert it is never called, and the Azure global-config lookup failure has a fail-closed test. Closes LeanerCloud#679 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 8 billable files and costs up to $2.00. Or wait 59 minutes for your next included review. View limit details
Comment |
|
VERDICT: APPROVE 360a0dc Reviewed head 360a0dc (1 commit, 379 changed lines, under the 400 cap) against origin/main b6a0649. The base is 40 commits stale; Findings (none blocking):
Attacked:
Local results (merged tree 301be9e4, macOS, AWS creds disabled): |
|
Independent review 2 (claude-fleet, cold): APPROVE 360a0dc Findings (none blocking):
Root cause:
Attacked:
Verified locally, on
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: REQUEST_CHANGES Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rA1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Bring the branch up to date with main (7cde88e), which now validates RI exchange account IDs as 12-digit AWS account IDs (LeanerCloud#1170). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main now refuses to record an RI exchange whose account ID is not a 12-digit AWS account ID (LeanerCloud#1170), so the "acct-four-eyes" fixture made the 4-eyes downgrade tests fail before any assertion they make. Use the 123456789012 documentation placeholder instead; every assertion is kept. The one-line adapter literals are split so the account field sits on its own line, the only shape the .gitallowed placeholder rule exempts from the AWS account-ID detector. Refs LeanerCloud#679 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pick up LeanerCloud#1111 and LeanerCloud#1418 so CI runs against the fixed main (LeanerCloud#1418 fixes main's red purchase integration test). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Updated the branch. New head:
Checks on the merged tree:
|
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale: none Reviewer: rB9. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB11. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
Conflicts in both monthlyCostFromConvertibleRI copies: main (LeanerCloud#1260) replaced them with one shared ricost.MonthlyCostFromConvertibleRI, and this branch added the no-positive-price exclusion (LeanerCloud#1188) to each copy. Kept main's single implementation and moved this branch's check into it, with the sentinels exported as ricost.ErrNonPositiveDuration and ricost.ErrNoPricing so the API's excludedRIReason still picks the per-RI reason. Both wrappers now just delegate, so the "keep in sync" notes are gone. ricost's table pinned "zero prices -> 0, no error" as today's behavior "rather than endorsing it"; that row now expects the no-pricing error, and a negative-sum row is added. The 4-eyes fixture added on main (LeanerCloud#1108) used an unpriced RI, which is now excluded, so it gets the same FixedPrice the other fixtures on this branch use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Fixes the gaps from the #673 review that are still open on main: unattended auto-mode exchanges under 4-eyes (A) and the test gaps (C). Part B (creator-less rows) was already handled by #989; details below.
Root cause (A)
executeRIExchangeReshape(internal/server/handler_ri_exchange.go) passedcfg.RIExchangeModestraight toexchange.RunAutoExchange. Any non-manualmode executes exchanges on the spot, and nothing on that path readsRequireDifferentApprover. With 4-eyes on, the scheduledri_exchange_reshapetask could therefore move money with no approver at all. Since #673 the direct execute endpoints refuse under 4-eyes, which left this path as the only way around the policy. The ladder exchange runner (exchangeRunnerAdapterininternal/server/ladder_write.go) has the same shape, and it always asks for auto mode.Fix
fourEyesExchangeMode(internal/server/ri_exchange_four_eyes.go): with 4-eyes on, every mode exceptmanualruns asmanual. That coversauto,""and typos, becausepkg/exchangetreats every non-manualvalue as auto. A nil global config fails closed and is treated as 4-eyes on.executeRIExchangeReshapeuses the effective mode. A downgraded run raises pending records with approval tokens and sends the pending-approval email. Each exchange then goes through/api/ri-exchange/approve, which applies the existing 4-eyes check (fix(ri-exchange): require an identified approver for creator-less exchanges under 4-eyes #989: an identified approver is required, and token-only requests are refused).exchangeRunnerAdapter.RunAutoExchangebuilds its AWS clients and hands them to a newrun, which reads the global config and applies the same downgrade. If the config can't be read,runreturns an error before any AWS call (fails closed). Client construction is unchanged apart from moving intoriExchangeClients, which makesruntestable with mocks.ri_exchange_modeis not modified. Turning 4-eyes off restores auto behavior at the next run.RequireDifferentApprovercomment ininternal/config/types.goand thePUT /api/ri-exchange/configdescription inopenapi.yaml.Owner decision needed (issue question 1)
I implemented the conservative option: under 4-eyes, auto mode does not bypass approval and runs as manual. I chose a runtime downgrade over refusing the config write for two reasons. It also covers turning 4-eyes on while auto is already armed. And it fails closed whatever order the two settings are written in.
Not made configurable. An "auto is exempt from 4-eyes" opt-out would need a new
global_configcolumn, a migration, store read/write and API plumbing. It would also add another 4-eyes bypass knob. That isn't cheap, and it isn't something I should add without your call. If you want auto to stay exempt, say so and it can be a follow-up (or this PR can be closed).Part B (issue question 2): already handled, no change here
#989 (4e23d2c) changed
requireDifferentRIExchangeApprover. Under 4-eyes, a creator-less (system) exchange now needs a resolved session, so an anonymous email-token approval is refused, and a config-load failure fails closed. An identified approver can still approve it. This matches your 2026-10-17 decision for system-created purchases on #233 (Option B, shipped in #1071): the system is the first eye and an identified human the second. This PR leaves that untouched. If you want creator-less exchanges denied outright under 4-eyes, note that every real exchange row is creator-less, so all exchanges would become unapprovable while the mode is on.Part C: tests
TestExecuteExchange_FourEyesRefusesDirectExecuteandTestExecuteExchange_FourEyesPolicyLookupErrorFailsClosednow install a spyexecuteExchangeFn(spyOnAWSExchange) that would report success, and assert zero calls. Before, they relied on the real executor failing on missing AWS credentials.TestExecuteAzureExchange_FourEyesPolicyLookupErrorFailsClosed: aGetGlobalConfigerror must give a non-client (500) error containing4-eyes policy check, with noCalculateExchangeorExecuteExchangecall on the strict ops-client mock.Verification
All Go commands ran on macOS with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid. The evidence is unit/mock-based: no real AWS or Azure calls and no real Postgres. This PR has no DB or schema change. The 4-eyes flag is read once per run from the global config, so there's no new race to test against Postgres.New server tests (
internal/server/ri_exchange_four_eyes_test.go):TestExecuteRIExchangeReshape_FourEyesDowngradesAutoToManual(modesauto,"",Auto): zero executes, result modemanual, one pending record with an approval token, approval email sent, no completed email.TestExecuteRIExchangeReshape_FourEyesOffAutoStillExecutes(control).TestExchangeRunnerAdapter_FourEyesDowngradesLadderReshape(pending record keeps itsLadderRunID) andTestExchangeRunnerAdapter_FourEyesOffLadderStillExecutes(control).TestExchangeRunnerAdapter_PolicyLookupErrorFailsClosed: error returned, no RI listing, no execute, no record.TestFourEyesExchangeMode(table, including nil config).Fail-before / mutation probes. Each probe was applied to the fixed tree and then reverted. Command:
go test ./internal/server/ -run 'FourEyes|ExchangeRunnerAdapter|ExecuteRIExchangeReshape' -count=1.FourEyesDowngradesAutoToManual,FourEyesDowngradesLadderReshapeandPolicyLookupErrorFailsClosedfail.TestFourEyesExchangeModefail.PolicyLookupErrorFailsClosedfails.TestFourEyesExchangeModefails.API tests with
requireNoDirectExchangeUnderFourEyesmade a no-op, each run alone withgo test ./internal/api/ -run '^<name>$':TestExecuteExchange_FourEyesRefusesDirectExecute: FAIL ("no exchange may be submitted while 4-eyes is on").TestExecuteExchange_FourEyesPolicyLookupErrorFailsClosed: FAIL ("an unreadable 4-eyes policy must not let the exchange run").TestExecuteAzureExchange_FourEyesPolicyLookupErrorFailsClosed: FAIL (the strict mock rejects the unexpectedCalculateExchange).All pass with the fix.
Checks run
go build ./...: OK.go vet ./internal/server/ ./internal/api/ ./internal/config/, plus-tags integrationfor server and api: OK.go test -count=1for./internal/server/,./internal/api/,./internal/mocks/and./internal/config/: allok. The targeted tests also pass under-race.golangci-lintv2.10.1run ./internal/server/... ./internal/api/... ./internal/config/...: 0 issues.gocyclo -over 10 -ignore "_test\.go"on the changed files: silent.scripts/gosec-hook.shon the changed packages: clean.scripts/setup-git-secrets.sh: clean.scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.openapi.yamlparses.git diff --check: clean.Not run locally: the full repo-wide race suite (disk constraints; CI covers it), and anything against a real AWS account.
Out of scope / notes
automode while 4-eyes makes it run as manual. An "effective mode" field onGET /api/ri-exchange/config, plus the frontend change, could be a follow-up. Frontend work is off-limits for me right now.ReshapeBuffer, so this is defense in depth for when it gets wired.Maintainer: please mirror the issue's triage labels on this PR:
priority/p2,severity/medium,urgency/this-quarter,impact/few,effort/m,type/security,triaged.Closes #679
🤖 Generated with Claude Code