[IAmJSD]

Summary

The stateless admin API key's session UserID is the sentinel admin-api-key, not a user UUID. resolveCreatorUserID therefore stamps created_by_user_id = NULL onto every pending execution the key creates, and created_by_system stays false. Under four-eyes (require_different_approver), both gates deny such a row for every approver:

• requireDifferentApprover in the handler;
• checkDifferentApprover in the manager, which runs on session approve, token approve, SQS approve, run-now and direct execute.

So the row could only sit pending until it expired. On the direct-execute path the row was written first and then refused, which left a stuck pending row behind.

Merged PRs #989 and #1071 both reference this issue, but neither fixes it:

• fix(ri-exchange): require an identified approver for creator-less exchanges under 4-eyes #989 is RI-exchange only.
• fix(purchase): let an identified approver approve scheduler-created executions under four-eyes #1071 added created_by_system for sweep rows. The owner decision on fix(purchase): scheduler-created executions have no creator and cannot pass four-eyes #233 explicitly kept API-key rows denied.

Fix: option (b) from the issue

The issue offered (a) a stable non-UUID creator identity or (b) refusing creation. This PR takes (b), because it is fail-closed and changes no approval rule.

• New guard: requireAttributablePurchaseCreator (internal/api/handler_purchases.go). It refuses the request with a 403 when:

  • the session would resolve to no creator, and
  • four-eyes is on.

  The error tells the caller to sign in or use a per-user API key. Per-user API keys already carry the owner's UUID, so they are compared normally under four-eyes.

• Fail-closed behavior:

  • A global-config load failure denies.
  • A nil config means four-eyes is off, which matches requireDifferentApprover.
  • Sessions with a user UUID return before any config read, so signed-in users and per-user keys see no new failure mode.

• Where the guard runs. Each call site refuses before anything is written:

  • executePurchase, through validateExecutePurchaseRequest → requirePurchaseCreatingPermission. This covers both the approval-email path and execute_mode: "direct".
  • createPlannedPurchases (POST /api/plans/{id}/purchases), through the same wrapper.
  • persistRetryExecution, where the retry successor's creator is stamped. This one is defense in depth: the retry route requires a bearer session, so the admin key gets a 401 there today. The issue's "retry re-drive" path is therefore not reachable with the admin key. The guard still covers any session type that would stamp NULL on the successor. A creator-less successor would also be bad for a second reason: the linkage written in the same transaction blocks retrying the original again.

• No relaxed checks. No approval check is relaxed, and the key is never stamped with a human creator, as the issue required.

• openapi.yaml documents the new 403 on both endpoints.

Behavior change to note

With four-eyes on, the admin API key can no longer submit purchases or schedule planned purchases.

• Purchases: these were never approvable, so nothing that worked before is lost.
• Planned purchases: for an AutoPurchase plan, the scheduler used to buy key-scheduled rows unattended after the veto window, and that no longer happens. I refused these uniformly because AutoPurchase and the global switch can be turned off after scheduling, and a row created then would deadlock. Automation that needs this under four-eyes should use a per-user API key.

If you'd rather keep scheduling via the admin key for AutoPurchase plans, I can narrow that call site.

Concurrency

There is no schema or DB change, and the guard only reads. One race is possible: an admin turns four-eyes on between the guard and the insert. The row then carries a NULL creator and is denied like a legacy row. That fails closed, exactly as on main today, so no new exposure.

Verification

All Go commands ran on macOS with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's local AWS config profile is broken. The evidence is unit tests with the repo's MockConfigStore/MockAuthService, driving the real handlers end to end from the HTTP request. No real cloud calls, purchases, email or deploys were made, and there is no deployed-environment or Postgres coverage. The change has no SQL.

New tests in internal/api/four_eyes_unattributed_creator_test.go:

• Refusal tests:

  • TestExecutePurchase_AdminAPIKey_FourEyesOn_RefusedBeforeAnyWrite covers the approval-email and direct bodies.
  • TestCreatePlannedPurchases_AdminAPIKey_FourEyesOn_RefusedBeforeAnyWrite.
  • TestRetryPurchase_UnattributedSession_FourEyesOn_RefusedBeforeAnyWrite.

  Each asserts the 403 and that WithTx, SavePurchaseExecution(Tx) and LinkRetryExecutionAtomic were not called.

• Mode-off controls (*_FourEyesOff_ReachesPersistence): they assert that the request gets past the guard to the write, using a sentinel WithTx error.

• TestRequireAttributablePurchaseCreator:

  • user sessions and per-user API keys skip the config read;
  • a config error denies;
  • a nil config means off;
  • a nil session and a non-UUID id are refused.

Fail-before. I reverted only the three call sites and kept the helper so the tests compile. Each *_FourEyesOn_* test then fails:

• executePurchase and retry: the handler goes on to call SavePurchaseExecutionTx with CreatedByUserID:nil. That is the bug, caught as a mock panic on the unexpected write.
• createPlannedPurchases: the handler gets inside the transaction ("plan must be enabled..." does not contain "no attributable user").

The mode-off controls pass on both versions.

Pass-after: go test ./internal/api/ -run 'AdminAPIKey_FourEyes|UnattributedSession_FourEyes|TestRequireAttributablePurchaseCreator' -count=1 gives ok.

Mutation probes: 6 of 6 killed.

1. Refusing regardless of mode is killed by the 3 mode-off controls.
2. Swallowing the config error is killed.
3. Treating a nil config as on is killed.
4. Letting any non-nil session pass is killed.
5. Reading the config for user sessions too is killed, by the guard test and the existing DirectExec_FourEyesOn_DeniesSelfExecute.
6. Having the wrapper skip the guard is killed.

Other checks:

• go build ./... passes.
• go vet ./internal/api/ and go vet -tags integration ./internal/api/ pass.
• go test ./internal/api/ ./internal/mocks/ ./internal/server/ -count=1 gives all ok. That includes TestNoUnfailableMockAssertions.
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/... reports 0 issues, with and without --build-tags integration.
• gocyclo -over 10 -ignore '_test\.go' internal/api/ prints nothing. executePurchase and validateExecutePurchaseRequest were already at 10, which is why the guard sits in a requirePermission wrapper.
• I ran git-secrets (repo setup-git-secrets.sh patterns plus .gitallowed, in a fresh git init repo with an isolated HOME) on the changed files: clean. scripts/test-git-secrets-allowlist.sh reports 42 PASS, 0 FAIL.
• openapi.yaml parses, and git diff --check is clean.
• Not run: gosec, which isn't installed on this machine.

Out of scope

• Option (a): a stable non-UUID creator identity for the admin key. That needs a policy decision on whether the key holder may approve their own key's requests. I can file it if you want it.
• No backfill of existing pending rows: existing NULL-creator rows created by the admin key stay denied under four-eyes, consistent with the fix(purchase): scheduler-created executions have no creator and cannot pass four-eyes #233 "no backfill" decision.
• No frontend change: the History badge from fix(frontend): say when a pending row has no recorded creator under four-eyes #672 already explains the NULL-creator state.

Closes #677

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

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

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]

VERDICT: APPROVE 14f3dd7

Base: merge-base 8caed39; git merge-tree origin/main(b6a0649a) 14f3dd71 is clean (tree 2a595802). One commit, 294+/4- = 298 changed lines (within the 400 cap).

Findings

• nit (owner decision, disclosed in the PR body) internal/api/handler_plans.go:477 - with 4-eyes on, the admin API key can no longer schedule planned purchases even for AutoPurchase plans, which previously ran unattended. Refusing uniformly is the safer reading of fix(purchase): API-key-created pending purchases have a null creator and can never be approved under four-eyes #677 option (b); flagging it so the owner confirms. Any external automation (including MCP tooling configured with the admin key) that calls POST /api/purchases/execute or POST /api/plans/{id}/purchases will now get a 403 under 4-eyes instead of a row that could never be approved.
• nit internal/api/handler_purchases.go:2890 - the guard runs inside the permission wrapper, before body validation, so an admin-key request with a malformed body under 4-eyes gets 403 rather than 400. Harmless.

Entry points checked (merged tree)

• Every non-system writer of a new pending row: executePurchase (handler_purchases.go:3471, both approval-email and direct paths, via validateExecutePurchaseRequest:2890), createPlannedPurchases (handler_plans.go:477, the only caller of createPlannedPurchasesTx, which stamps CreatedByUserID at :746), persistRetryExecution (handler_purchases.go:2465, before the token mint and the tx; retryPurchase has no write before it). All three are guarded.
• System-created flows are untouched on purpose: plan_step_attach.go:329-333 sets CreatedBySystem (000130) and approvals.go:276 approves it via checkSystemCreatedApprover; failStep (:301) and ramp_target_met (:55) write failed/completed rows that never need approval; execution.go per-account rows are post-approval execution records.
• RI exchange has no API path that mints a creator-less pending row; handler_ri_exchange.go:2611 already handles NULL-creator scheduler rows (fix(ri-exchange): exchange records never carry a creator, so four-eyes is a no-op and audit attribution is lost #518/fix(ri-exchange): require an identified approver for creator-less exchanges under 4-eyes #989). Run-now, approve, pause/resume, cancel only transition existing rows.
• Per-user API keys and signed-in users carry a UUID, so resolveCreatorUserID returns non-nil and no config read happens (tested).
• Fail-closed: config error denies (500), nil config = off, matching requireDifferentApprover.

Local results (merged tree 2a595802, macOS)

• go test ./internal/api/ ./internal/purchase/ -count=1: ok / ok
• go vet -tags integration ./internal/api/: ok

Attacked:

• P1 drop the retry guard: killed (TestRetryPurchase_UnattributedSession_FourEyesOn_RefusedBeforeAnyWrite).
• P2 handler_plans back to requirePermission: killed (TestCreatePlannedPurchases_AdminAPIKey_FourEyesOn_...).
• P3 validateExecutePurchaseRequest back to requirePermission: killed (TestExecutePurchase_AdminAPIKey_FourEyesOn_...).
• P4 new guard swallows config error: killed (config_load_failure_fails_closed).
• P5 nil config treated as on: killed (TestRequireAttributablePurchaseCreator).
• Grepped every SavePurchaseExecution(Tx) caller and every PurchaseExecution{} literal outside tests to enumerate creation paths; read issue fix(purchase): API-key-created pending purchases have a null creator and can never be approved under four-eyes #677 to confirm option (b) and the "never stamp a human creator" rule are honored.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 14f3dd7

Findings (none blocking):

• [non-blocking, product decision for the owner] internal/api/handler_plans.go:463 - With four-eyes on, the admin key can no longer schedule planned purchases on AutoPurchase plans.
  • Those rows were not deadlocked before. The scheduler's auto-execute path (internal/purchase/manager.go:758-836) never calls enforceFourEyesPolicy; its only caller is the approve path (internal/purchase/approvals.go:409).
  • So key-scheduled AutoPurchase rows used to be bought unattended, and they are now refused.
  • The PR body states this and offers to narrow the guard. Its reasoning holds: AutoPurchase or the kill switch can be turned off after scheduling, which would deadlock the row.
  • The owner should confirm this before merge.
• [non-blocking] internal/api/handler_purchases.go:2804 - The guard runs before body and scope validation, so under four-eyes an admin key gets a 403 rather than a 400 for a malformed body. This is harmless.
• Coverage:
  • All three API writers of new purchase_executions rows are covered (handler_plans.go:735, handler_purchases.go:2497 and :3216).
  • The other API-layer SavePurchaseExecution calls (:1266, :3022) update existing rows.
  • The writers in internal/purchase write scheduler or system rows.

Against #677 option (b):

• resolveCreatorUserID (handler_purchases.go:3075) returns nil for the admin key.
• Both checkDifferentApprover (approvals.go:279) and requireDifferentApprover (handler_purchases.go:1390) deny a NULL non-system creator.
• The guard refuses before any write and never stamps a human creator. Confirmed.

Attacked:

• Per-user API keys and signed-in sessions carry a user UUID (handler.go:366/472), so they pass without a config read.
• Retry: the guard in persistRetryExecution runs before the transaction and the linkage, and nothing is mutated before it.
• Direct execute: it previously wrote the row and was then refused at :3549. It is now refused before the write.
• Race: if four-eyes is turned on between the guard and the insert, the row is denied like a legacy row. This fails closed.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 6 were caught:
  • M1, the retry guard dropped.
  • M2, the plans call site back to requirePermission.
  • M3, the executePurchase call site back to requirePermission.
  • M4, the mode check inverted.
  • M5, the config error swallowed.
  • M6, a nil config treated as on.
• Merged tree:
  • git merge-tree --write-tree origin/main <head> is clean.
  • Main has 40 commits since the base, including changes to handler_purchases.go and handler_plans.go. None of them touch the changed functions.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK.
  • go vet ./internal/api/, with and without -tags integration: OK.
  • gofmt -l on the 3 changed files: clean.
  • go test -count=1 ./internal/api/ ./internal/purchase/: ok.
• Merged tree: build and the same tests: ok.
• Fail-before, with the new tests on base (minus the test of the new helper), all three FourEyesOn tests FAIL:
  • the executePurchase test, on an unexpected save;
  • the createPlannedPurchases test, on a missing "no attributable user" error;
  • the retry test, which saves with CreatedByUserID nil (the bug).
• openapi.yaml parses.
• Not run: golangci-lint, gosec, integration tests. No SQL or migration.

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

[cristim]

Local verification at 14f3dd7144df3e497371accead99ff0c8c3cf8f6 merged onto current main (f5ad8c60; merge-tree 1c632722; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at 14f3dd7144df3e497371accead99ff0c8c3cf8f6).