Skip to content

fix(purchase): let an identified approver approve scheduler-created executions under four-eyes - #1071

Merged
cristim merged 2 commits into
mainfrom
fix/four-eyes-system-created-executions
Oct 18, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/four-eyes-system-created-executions

Conversation

@cristim

@cristim cristim commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Summary

Part of #233 (P1). This PR is the backend change. A small frontend PR follows so the Approve button appears for these rows; it will complete the issue.

The notification sweep creates purchase executions without a creator. It does this for AutoPurchase plans that have no pre-created root (getOrCreateExecution → claimPlanStep → attachStep(root=nil) → writeAttached). checkDifferentApprover denied every row with no creator ("predates the dual-control feature"). With RequireDifferentApprover on, nobody could approve or run-now these rows early. The scheduler still bought them unattended once the veto window passed.

Simply allowing rows with no creator would fail open. API-key-created rows (#677), rows whose creator was deleted, and legacy rows also have no creator. The owner decided (2026-10-17, on #233):

  • Option B: an explicit marker.
  • Approve-any only on the session path.
  • No backfill.

Changes

  • Migration 000130: purchase_executions.created_by_system BOOLEAN NOT NULL DEFAULT false, plus CHECK (NOT created_by_system OR created_by_user_id IS NULL). Existing rows all get false, which satisfies the CHECK.
  • PurchaseExecution.CreatedBySystem: server-only (json:"-"). It is written by the INSERT and read through the single positional scanExecutionRows by all 13 full-row SELECT lists. The upsert's DO UPDATE excludes the column, so the value is insert-only.
  • Writer: only writeAttached's root == nil sweep path sets the flag. failStep and target-met terminal rows, API-key paths, retry successors and managed plans (which always have a root) never set it.
  • checkDifferentApprover (purchase) and requireDifferentApprover (handler):
    • A row with a creator is compared to the approver as before. The creator always takes precedence over the flag.
    • A row with no creator and no flag keeps the existing deny.
    • A row with no creator and the flag set is allowed for an identified approver. An empty actor is denied.
    • RI exchange passes false and behaves exactly as before.
  • History: created_by_system is projected read-only, for the frontend PR.

Policy notes (owner decision B)

  • Session path: approve-own does not count for system rows. authorizeSessionApprove still denies it, because there is no creator. Only approve-any holders can approve.
  • Email-link (token) path: as today for human-created rows, the approver is the account's contact_email. A signed-in contact approver can therefore approve a system row without holding approve-any. "Approve-any only" applies to the session path.
  • Run-now and direct-execute: an identified approver who passes those paths' own execute RBAC can run a sweep row alone and skip the veto window. The system is treated as the first eye.

Tests (each fails on the unfixed rule)

Fail-before is shown with the decision branches disabled, because the tests depend on the new field and do not compile on main.

  • TestFourEyes_SweepMintedRowIsApprovableByIdentifiedApprover runs real sendPlanNotification into Postgres, then the approval. It failed with "predates the dual-control feature" before the fix.
  • TestRequireDifferentApprover_ModeOn_SystemCreated (handler) and TestManager_ApproveAndExecute_FourEyesOn_SystemCreated fail the same way.
  • Guard tests stay passing:
    • an anonymous or empty actor is denied;
    • the creator wins over the flag;
    • legacy, API-key and deleted-creator rows are still denied;
    • RI exchange is unchanged.
  • The store round-trip and insert-only tests cover 6 readers directly. A SELECT list missing the column fails loudly through the shared scan, which mutant M7 confirms. The migration test covers up, down and the CHECK violation.
  • Mutants:
    • Author's probes: 16 of 16 killed.
    • Reviewer's probes: 6 of 7 killed. The survivor (RI exchange passing true) is equivalent, because RI calls it only with a creator.

Historical verification at 4f928f1 (local, macOS)

  • go build ./... and go vet ./... pass.
  • go test -race passes on purchase, api, config, database and mocks. The mock-assertion guard passes.
  • Integration tests pass (testcontainers): migrations, config execution and plan-step, and the purchase four-eyes, attach and sweep tests.
  • golangci-lint v2.10.1 with --build-tags integration reports 0 issues. The pre-commit gocyclo hook passes.
  • Migration ordering: 000128 is on main and 000129 is fix(scheduler): record partial multi-subscription sweeps in collection state #1022, so 000130 is free and claimed on the bus. The isolated branch migration test covers its available migrations through 130. Combined migration-129 runtime verification has not been performed. PR fix(scheduler): record partial multi-subscription sweeps in collection state #1022 (migration 129) must merge before this PR (migration 130).

Current recovery evidence for head 430f4d9 is recorded in the recovery comment below. CI and CodeRabbit remain pending.

Review

  • Plan (plans/233-four-eyes-system-executions.md): independent review with R1-R7, then the owner's decision.
  • Diff: two independent non-author passes. Approved at 4f928f1, with no blocking findings.

🤖 Generated with Claude Code

@cristim cristim added severity/medium Moderate harm urgency/this-sprint Within the current sprint priority/p1 Next up; this sprint impact/many Affects most users effort/s Hours type/bug Defect triaged Item has been triaged labels Oct 17, 2026
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 20 billable files and costs up to $5.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 1 minute for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 79 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: aec6be4c-8df3-48a0-82ac-2d519d416203
📥 Commits

Reviewing files that changed from the base of the PR and between c632bc1 and 430f4d9.

📒 Files selected for processing: 20
  • internal/api/handler_history.go
  • internal/api/handler_history_test.go
  • internal/api/handler_plans_test.go
  • internal/api/handler_purchases.go
  • internal/api/handler_purchases_test.go
  • internal/api/handler_ri_exchange.go
  • internal/config/store_postgres.go
  • internal/config/store_postgres_created_by_system_integration_test.go
  • internal/config/store_postgres_pgxmock_test.go
  • internal/config/store_postgres_ramp_baseline.go
  • internal/config/store_postgres_ramp_step.go
  • internal/config/types.go
  • internal/database/postgres/migrations/000130_purchase_executions_created_by_system.down.sql
  • internal/database/postgres/migrations/000130_purchase_executions_created_by_system.up.sql
  • internal/database/postgres/migrations/000130_purchase_executions_created_by_system_test.go
  • internal/purchase/approvals.go
  • internal/purchase/approvals_test.go
  • internal/purchase/four_eyes_system_execution_integration_test.go
  • internal/purchase/plan_step_attach.go
  • internal/purchase/plan_step_ledger_migration_integration_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim and others added 2 commits October 17, 2026 22:42
…utions under four-eyes

Under four-eyes mode the plan-step sweep minted executions with a NULL
creator, and the gate denied every NULL creator ("predates the dual-control
feature"), so the emailed approve link was dead and the error text was false.
A NULL creator also covers API-key, deleted-user and legacy rows, so allowing
all NULLs would fail open for those.

Add purchase_executions.created_by_system (migration 000130, NOT NULL DEFAULT
false, CHECK that a flagged row has no creator). Only writeAttached's
root==nil sweep path sets it; the upsert never rewrites it. The flag is
server-only (json:"-" on the execution) and is projected read-only as
created_by_system on the history record.

checkDifferentApprover and requireDifferentApprover: a creator is compared as
before (the flag cannot override it); NULL creator without the flag keeps the
existing deny; NULL creator with the flag accepts any identified approver and
still denies an empty actor. RI exchange passes false and is unchanged.

Policy notes: no backfill, so existing NULL-creator rows stay denied. On the
token path the approver is the per-account contact_email, as for human rows
today. Under this model a single human with approve-any can run-now or
direct-execute a sweep row and skip the veto window; the owner accepted this.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Insert the legacy execution directly while the test database is at
version 126. The current store requires the system-creator column
introduced in version 130.
@cristim
cristim force-pushed the fix/four-eyes-system-created-executions branch from 4f928f1 to 430f4d9 Compare October 17, 2026 23:51
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. Your current included review allowance is based on your included PR review attempts over the past 7 days. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 1 minute.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Recovered and published PR head 430f4d9.

The recovered rebase preserved the backend patch (range-diff equal). The added fixture fix seeds the pre-127 execution using a parameterized historical-schema INSERT. Reproduction before the fix failed with SQLSTATE 42703 because the current store required the version-130 created_by_system column while this fixture deliberately staged version 126.

Current local macOS verification: all three focused integration cases passed against Docker PostgreSQL with no skips, including TestMigration_NeverStartedLegacyRampsBecomeNew and both TestFourEyes_Sweep cases. The migration fixture exercised version 126 followed by 130. go build ./... and normal pre-commit hooks passed. This is local fixture/integration evidence, with no real purchases or deployments.

Independent non-author gpt-6.1-sol medium reviewer completed two passes over the full backend change and fixture fix. Reviewed staged tree bc8027e7b636731f4b32978dc3112b120a4ce1b1, which is the committed head tree. No actionable findings within backend scope. Attacks covered system-marker provenance, insert-only storage, all 13 execution read shapes, fail-closed identity checks, and historical fixture semantics. The reviewer did not independently rerun tests or mutation probes; those limits remain explicit. The audit peer separately reviewed the fixture twice with no actionable findings.

Earlier verification and mutation counts in the PR description describe historical head 4f928f1, not new runs on this head. No combined migration-129 runtime proof is claimed: migration 129 belongs to prerequisite #1022, which must merge before this PR's migration 130. The dashboard approval predicate remains the planned frontend slice of #233; this PR remains backend-only.

CI watchers are armed for runs 39080639737 and 39080639918. A separate CodeRabbit watcher is armed, and a full review was requested because the previous attempt was rate limited. That earlier green bot check was not a completed review. This head is not yet merge-ready.

@cristim
cristim merged commit 2cd38cc into main Oct 18, 2026
25 checks passed
@cristim
cristim deleted the fix/four-eyes-system-created-executions branch October 18, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant