[IAmJSD]

Summary

Closes #518. This fixes the parts of the issue that #602 left open, legs (a) and (b) from the status comment. #602 already shipped leg (c), token-path attribution (transitioned_by and approved_by when a session resolves). This PR does not touch it.

Root cause

Every ri_exchange_history row is written by the scheduler (ri_exchange_reshape) or the ladder engine through configExchangeStoreAdapter / exchangeToConfigRecord. pkg/exchange.ExchangeRecord (cloud-commitments-go ad67030387e4) has no creator field, so created_by_user_id is always NULL. No API route creates a pending exchange: /api/ri-exchange/execute runs directly and is already refused under 4-eyes. requireDifferentRIExchangeApprover returned nil straight away for a NULL creator. That return also skipped the shared helper's fail-closed "no session" check. The result: with RequireDifferentApprover on, an anonymous POST carrying only the email token could approve and execute any production exchange, with no recorded actor. openapi.yaml already promised that "a token request with no session fails with 500" under 4-eyes. That was only true for creator-bearing rows, and no such rows exist in production.

Fix

In requireDifferentRIExchangeApprover (internal/api/handler_ri_exchange.go):

• Creator recorded: unchanged. The existing requireDifferentApprover runs: no session gives 500, the creator gives 403.
• No creator (every real row), 4-eyes on, no session: now denied with the same 500 and message as creator-bearing rows. The row stays pending and is not attributed.
• No creator, 4-eyes on, global-config load fails: denied (error, fail-closed).
• No creator, 4-eyes on, session resolved: still allowed. The approver is recorded in transitioned_by and approved_by through fix(ri-exchange): attribute token-path approvals to the resolved session user #602.
• 4-eyes off: unchanged.

Comment-only updates: config.RIExchangeRecord.CreatedByUserID now points at the actual policy rather than #233 (which covers purchases), and exchangeToConfigRecord documents that leaving the creator nil is intentional. openapi.yaml now describes creator-less behaviour.

Decision for the maintainer: which creator should scheduler exchanges carry? (leg a)

The status comment asked which creator scheduler exchanges should carry: a system user, the config owner, or NULL by design. This PR picks NULL by design, for these reasons:

• No human requests these exchanges. A "config owner" would have to come from the RI-exchange global config or ladder_configs, and neither records who saved them today. Inventing one needs new columns and a policy, and it is easy to bypass: someone else just re-saves the config.
• Treating NULL as "nobody may approve" (the purchase-side legacy rule) would make every RI exchange unapprovable under 4-eyes. That is exactly the fix(purchase): scheduler-created executions have no creator and cannot pass four-eyes #233 failure on purchases.
• So 4-eyes on a system-created exchange means at least one identified human approves it, and that person is recorded. It can never be an anonymous token holder.

If you prefer config-owner attribution, that is a follow-up: owner columns on the config tables, a creator field in pkg/exchange or an adapter-side stamp, and a migration. I can file it.

Who can approve their own exchange: before and after

Users cannot create pending exchanges, so "own exchange" only applies to a creator-bearing row, which production does not write. That case is unchanged: the creator is refused under 4-eyes. What changes is the creator-less row, which is every real row:

4-eyes	Request	Before	After
on	token, no session	approved, no actor recorded	500, stays pending
on	token + resolved session	approved, actor recorded	approved, actor recorded
on	session (CSRF)	approved	approved
off	any	approved	approved

Migration / backfill

No migration. created_by_user_id already exists (000054), and the store already writes and reads it. No writer has ever set it on an exchange row, so every existing row is system-created and NULL. NULL now means "system-created" for those rows, which is accurate. No backfill is needed. The only effect on existing pending rows: under 4-eyes they now need a signed-in approver.

Verification

All Go commands ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, on macOS against base 2ef2f5d2. No real AWS: the integration test injects executeExchangeFn with a fake that fails.

• Fail-before (fix reverted, new tests kept): go test ./internal/api/ -run 'TestApproveRIExchange_FourEyes|TestRequireDifferentRIExchangeApprover|TestHandleRequest_ApproveRIExchange_StatusCodes' -count=1 fails on:
  • TestApproveRIExchange_FourEyes/automated_exchange:_token_without_session_fails_closed
  • TestRequireDifferentRIExchangeApprover_ConfigLoadFailsClosed
  • TestHandleRequest_ApproveRIExchange_StatusCodes/500_4-eyes_on_and_token_without_session_on_a_creator-less_exchange
• Pass-after: the same command plus TestHandleRequest_RejectRIExchange gives ok.
• Real Postgres (testcontainers): go test -tags integration ./internal/api/ -run TestApproveRIExchange_Integration_SystemCreatedFourEyes -count=1 -v
  • The test seeds a scheduler-shaped row and asserts that it reads back with a nil creator.
  • After the fix, both subtests PASS: under 4-eyes, a token request with no session gets 500, the row stays pending, approved_by is NULL and the executor is never called. The control run with 4-eyes off approves the same seed.
  • With the fix reverted, the 4-eyes subtest FAILS ("An error is expected but got nil": the anonymous approval went through) and the control still passes.
• Mutation probes on the new branch, each run against the unit tests above. All were killed:
  1. Removing the session != nil allow: 3 failures (session and token+session approvals of creator-less rows).
  2. Swallowing the config-load error: ConfigLoadFailsClosed fails.
  3. Denying even when 4-eyes is off: 7 failures (the legacy token tests and the mode-off case).
• go build ./... OK.
• go vet ./internal/api/ ./internal/server/ ./internal/config/ and go vet -tags integration ./internal/api/ OK.
• go test ./internal/api/ ./internal/server/ ./internal/config/ -count=1 all ok.
• golangci-lint run --allow-parallel-runners (v2.10.1) on ./internal/api/... ./internal/server/... ./internal/config/... reports 0 issues.
• gocyclo -over 10 on the changed non-test files prints nothing.
• git-secrets --scan (AWS patterns plus the repo's .gitallowed) on the changed files is clean, and bash scripts/test-git-secrets-allowlist.sh reports 42 PASS, 0 FAIL.
• git diff --check is clean and openapi.yaml parses as YAML.

Not verified: a deployed environment or a real AWS exchange. That is out of scope by design.

Out of scope

• Config-owner or system-user attribution for scheduler and ladder exchanges (see the decision above).
• Adding a creator field to pkg/exchange.ExchangeRecord in cloud-commitments-go.
• The purchase-side NULL-creator policy (fix(purchase): scheduler-created executions have no creator and cannot pass four-eyes #233, fix(purchase): API-key-created pending purchases have a null creator and can never be approved under four-eyes #677) and the email-link approval UX (fix(approval): email-link approvers without view:purchases cannot approve, and the retry toast is wrong for permanent errors #529).
• The reject endpoint (token-only, not an approval).

Maintainer: please mirror the issue's labels onto this PR: priority/p2, severity/medium, urgency/this-sprint, impact/few, effort/m, type/bug, triaged.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Four-eyes approval rules now apply to system-created exchanges without a recorded creator. A signed-in approver can approve them; token-only approval fails when four-eyes mode is enabled.
  • When token-only approval fails, the exchange remains pending and is not attributed to an approver. Token approval remains available when four-eyes mode is disabled.
• Documentation
  • Clarified the approval behavior for exchanges without a recorded creator.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 77bbba72-2acd-4896-a03c-e0e8f02677e6

📥 Commits

Reviewing files that changed from the base of the PR and between adcd80c and 42b852e.

📒 Files selected for processing: 7

• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/openapi.yaml
• internal/api/ri_exchange_approve_reject_status_test.go
• internal/api/ri_exchange_four_eyes_system_integration_test.go
• internal/config/types.go
• internal/server/handler_ri_exchange.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The approval handler now applies four-eyes checks to exchanges without a recorded creator. Tests cover token and session approval, configuration-load errors, and PostgreSQL-backed behavior.

Changes: Creatorless RI Exchange Approval

Layer / File(s)	Summary
Four-eyes policy and API coverage internal/api/handler_ri_exchange.go, internal/api/*ri_exchange*test.go, internal/api/openapi.yaml, internal/config/types.go, internal/server/handler_ri_exchange.go	When an exchange has no creator, a resolved session is allowed. Without a session, approval is allowed if four-eyes mode is disabled or configuration is nil; otherwise, the handler returns an error. Tests cover these paths and configuration-load errors. The API description and comments document creatorless records and approval behavior.
PostgreSQL integration coverage internal/api/ri_exchange_four_eyes_system_integration_test.go	The integration test verifies that token-only approval fails with four-eyes enabled and leaves the exchange pending and unattributed without calling the executor. With four-eyes disabled, the token approval moves the exchange out of pending status.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 42b85

No merge-blocking risk is identified; the change is ready for normal checks.

Security architecture risk: 🔵 Low · up to 42b85

The change restricts approval authority rather than expanding it. Anonymous token holders are denied before exchange state changes when four-eyes approval is enabled. Existing session and token authorization modes remain intact. Some uncertainty remains around downstream execution privileges and recovery after process interruption.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The token authorizes its referenced exchange record. Execution uses that record's region, source RI IDs, target offering and quantity, with a reserved payment ceiling. The inspected path does not establish the executor's broader credential or IAM scope.

Security Findings and Attack Paths

• observed — The prior path from possession of a valid approval token to anonymous creatorless exchange execution under four-eyes is blocked at head before state consumption. This is a reduction of existing exposure, not a newly introduced attack path.

Trust Boundaries and Controls

• observed — The session-presence control resolves a bearer token through the production authentication adapter. The validator checks the stored token, expiry, and active user before returning a session; the adapter preserves that user's ID and email. Session approval separately enforces CSRF, while token fallback independently validates the approval token.

Resilience and Maintainability Implications

• observed — The unchanged execution path reserves spending capacity before the provider call, detaches execution and terminal writes from request cancellation, and retries completion persistence three times. Persistent completion-write failure is surfaced; failure-state persistence errors are logged. Process-crash reconciliation remains unverified, but the new denial occurs before processing state and does not create that recovery exposure.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #518 requires session-created exchange rows to store created_by_user_id and a real-adapter test that rejects self-approval. The PR explicitly leaves scheduler and ladder rows creator-less and …	Implement creator propagation for exchanges created by a session user. Add the required real-adapter self-approval test. Keep or verify the existing token-path actor attribution.
Docstring Coverage		Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: requiring an identified approver for creator-less RI exchanges when four-eyes mode is enabled.
Out of Scope Changes check		The changed handler logic, approval tests, integration test, OpenAPI text, and comments all address RI-exchange four-eyes enforcement or its audit behavior for issue #518. No unrelated change is shown…

Full details: Out of Scope Changes check

Explanation

The changed handler logic, approval tests, integration test, OpenAPI text, and comments all address RI-exchange four-eyes enforcement or its audit behavior for issue #518. No unrelated change is shown in the supplied change summary.

Full details: Linked Issues check

Explanation

Issue #518 requires session-created exchange rows to store created_by_user_id and a real-adapter test that rejects self-approval. The PR explicitly leaves scheduler and ladder rows creator-less and adds no creator propagation or self-approval test. The token-approval actor requirement is already covered by the implementation attributed to #602, but the other two coding requirements remain unmet.

Resolution

Implement creator propagation for exchanges created by a session user. Add the required real-adapter self-approval test. Keep or verify the existing token-path actor attribution.

Full details: Docstring Coverage

Explanation

Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• 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 verdict: CLEAN at 42b852e

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. The route that let a token-only request approve an exchange under four-eyes is closed by this PR.

Route table:

Route	Where	Status
POST /api/ri-exchange/approve/{id}, token only	router.go:318, handler_ri_exchange.go:2243 → approveRIExchangeViaToken :2285	Covered (this was the hole). requireDifferentRIExchangeApprover (:2546) now returns 500 with no session under four-eyes
Same route with a session	:2332	Covered: CSRF, then requireSession, then the same helper
Session without the approve permission, plus a token	falls through to the token branch	Covered: the approver is identified and stamped (note 2)
POST /api/ri-exchange/reject/{id}	:2733	Unaffected by design: a token-only reject is allowed, which is the safer direction
/api/ri-exchange/execute, azure-instances exchange	:1207, :2518	Already refused (403) under four-eyes via requireNoDirectExchangeUnderFourEyes
Scheduler and ladder auto modes	internal/server/handler_ri_exchange.go:95 (RunAutoExchange)	Unaffected by design: unattended and operator-selected (note 1)

Behaviour checks:

• A recorded creator is unchanged: 500 with no session, 403 for the creator.
• Four-eyes off is unchanged: token-only approval still works, and the integration control subtest confirms it.
• The token is validated first (:2565), so the 500 is visible only to someone holding a valid token. Nothing extra leaks.
• A global-config load error fails closed.
• config/types.go changes a comment only.
• openapi.yaml matches the behaviour.

Non-blocking notes:

1. Pre-existing policy question for the maintainer: the scheduler and ladder auto modes execute exchanges with no human approver even when four-eyes is on. Please confirm "auto = unattended" is intended under four-eyes.
2. A signed-in user without the approve permission who holds a valid token can approve under four-eyes (:2268, 2275). The approver is identified and stamped, which matches the existing behaviour for rows that have a creator.
3. The refusal is HTTP 500 for what is really an authentication failure. That is consistent with the purchase-side wording.

What was attacked and how (local macOS; no real exchange):

• go vet ./internal/... is clean. go test -race -count=1 ./internal/api/... ./internal/server/... passes 3934. The only RI-exchange-related skip is unrelated (TestRevokePurchase_FourEyesApproval).
• go test -tags integration on TestApproveRIExchange_Integration_SystemCreatedFourEyes ran against Docker Postgres: both subtests pass, with no skip.
• Mutation probes:
  • Restoring the early nil return (fail-before) fails FourEyes, ConfigLoadFailsClosed, StatusCodes and the integration test.
  • Allowing no session on creator-less rows fails FourEyes, StatusCodes and the integration test.
  • Dropping the approver stamp fails three unit tests. The integration test doesn't assert the stamp.
  • Removing the session allow fails two unit tests.

Gate:

• Labels are mirrored from #518.
• CI must be green at this SHA.
• The draft must be marked ready, and CodeRabbit must review once it is.