Skip to content

feat(api): gate auto-purchase arming behind execute:purchases - #841

Merged
cristim merged 2 commits into
mainfrom
feat/631-r1b-gates
Oct 13, 2026
Merged

cristim merged 2 commits into
mainfrom
feat/631-r1b-gates

Conversation

@cristim

@cristim cristim commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Refs #631 (R1b: kill-switch grant; follows R1a #840; R2 stacks on this).

What

  • Arming now needs execute:purchases (already carved out of admin:*): a plan going not-armed -> armed (planArmed = enabled && auto_purchase && max_step_commitment != nil) or an armed plan's cap rising, on POST, PUT and PATCH /api/plans, regardless of switch state (plan handlers do not read global_config); and auto_purchase_enabled off -> on in PUT /api/config, inside the UpdateGlobalConfigAtomic closure with before from existing, after the RI-exchange grant.

  • The stateless admin API key is refused (403) for those escalations; it skips every permission check, so reusing the verb alone would not stop it. Disarm, lower cap and switching off stay allowed with the ordinary verbs, including the admin key.

  • Switch changes write the R1a stamps (who, when, credential kind); plan arming/cap changes emit a structured log line (actor, credential kind, plan id, armed and cap old -> new). No plan stamp columns.

  • Cap validation: max_step_commitment must be within [0.01, 1e15] and have at most 2 decimal places (parsed from the decimal text via StepCapCents; a half-cent cap was stored rounded by Postgres while the gate compared the unrounded value). Stored == compared proved on real PG17. Before, 0.004 rounded to 0 and violated the column CHECK, and 1e308 overflowed NUMERIC(18,2): both were 500s. Raise detection compares cent-rounded values.

  • Not changed: the RI-exchange auto-mode path has the same admin-key hole; follow-up issue, out of scope here.

  • Owner decision (N4, left as is): PUT /api/plans/{id}/accounts and term/payment/coverage edits on an armed plan need only update:plans (they change what a plan buys, not the per-step ceiling).

Tests

  • auto_purchase_grant_test.go drives patchPlan/updatePlan/createPlan/updateConfig with mock auth that answers from the real auth.AuthContext (admin:* alone is denied execute:purchases; admin + execute:purchases allowed; admin API key). Cases: arm, enable, disarm, rename, NULL cap not armed, cap raise/lower/same/sub-cent/one-cent, create, switch on/off, unchanged switch not stamped. Fixture/mock-based evidence, labelled as such.
  • Cap validation: 0, negative, 0.004, 1e308, quoted number (400), plus a config unit test.
  • Stamp forging: PUT /api/config bodies using snake_case, upper/mixed case and Go field names leave stored stamps untouched (json:"-"; gate follow-up from feat(config): add auto-purchase switch stamp columns (migration 000115) #840).
  • Mutation probes run (each makes tests fail): remove the gate call in create, PUT, PATCH and config; > -> >= in the raise check; drop the admin-key refusal (two variants).
  • Default-off remains covered by R1a's fresh-migrate test (feat(config): add auto-purchase switch stamp columns (migration 000115) #840).

Verification

go build ./..., go test ./internal/api ./internal/config ./internal/server, golangci-lint (0 issues), gofmt, gocyclo hook, git-secrets. No database cluster was needed (no SQL change). Size is about 450 lines of which ~300 are tests.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Access Controls

    • Auto-purchase plan changes that enable purchasing or raise a spending cap now require purchase execution permission. Admin API keys cannot authorize these escalations.
    • Enabling the global auto-purchase setting also requires purchase execution permission; disabling it remains available to admins.
  • Validation

    • Spending caps must be positive amounts with no more than two decimal places and within the supported range. Invalid values are rejected without saving the plan.
  • Auditability

    • Changes to the global auto-purchase setting record who made the change and when.

Arming a plan (enabled + auto_purchase + non-NULL max_step_commitment),
raising an armed plan's cap, or flipping the global kill switch off->on now
requires execute:purchases (carved out of admin:*), on POST/PUT/PATCH plans
and PUT /api/config (inside the atomic closure, after the RI-exchange grant).
The stateless admin API key is refused for these escalations; disarming,
lowering a cap and switching off stay on the ordinary verbs. Switch changes
are stamped (who, when, credential kind); plan changes are logged.

Reject max_step_commitment below 0.01 or above 1e15 with 400 (the column is
NUMERIC(18,2); a sub-cent cap or 1e308 was a 500). Raise detection compares
cent-rounded values.

Refs #631

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/many Affects most users effort/xl Multi-week / refactor type/feat New capability labels Oct 13, 2026
@cristim

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate review (Opus 5.5, claude-opus-5-5): CHANGES REQUESTED at b6e4450d9d5a5d2fcfd09afd5068087f49bd9a28

Reviewed from git archive of the exact head, GOTOOLCHAIN=go1.26.9, macOS.

Blocking finding (reproduced)

B1. A cap raise can bypass the grant through half-cent inputs. internal/api/auto_purchase_grant.go:36-41,52 compares int64(math.Round(v*100)) on the binary float, but Postgres rounds the decimal pgx sends (shortest repr, e.g. 1.005) half away from zero. For about 6.6% of cap values the gate sees "same cents" while the column stores the next cent up.

  • Real PG17 probe (throwaway cluster, NUMERIC(18,2) CHECK > 0, pgx v5.9.2, *float64 param exactly as UpdatePurchasePlanTx at internal/config/store_postgres.go:822 sends it): sent=1.005 capCents=100 storedInDB=1.01; sent=1000.004 -> 1000.00 (fine); 1000.005 -> capCents 100001, stored 1000.01 (fine).
  • Handler repro (mock-based, labelled as such): an admin holding only admin:* PUTs {"enabled":true,"auto_purchase":true,"max_step_commitment":1.005} onto an armed plan with cap 1.00: no 403, the write reaches UpdatePurchasePlan, and PG stores 1.01. No log line is emitted either.
  • Sweep over caps 0.01..10000.00: 65,629 of 1,000,000 values allow a silent 1-cent raise; longest consecutive chain is 4 cents. The money impact is small, but the PR's invariant ("no cap raise without execute:purchases") does not hold, and the tests at auto_purchase_grant_test.go:113-114 only check .004 and .01.
  • Suggested fix: make the stored value and the compared value the same. For example, in validateNotificationAndCap (internal/config/validation.go:539) reject caps with more than 2 decimal places (strconv.FormatFloat(v,'f',-1,64) has at most 2 fractional digits). Then capCents is exact. Add 1.005 to the cap table as a 400 or a refused raise.

Non-blocking

  • N1 auto_purchase_grant.go:56-64: in production the router always puts a Principal in ctx (router.go:404-419), so the stamp via is session / user-api-key / admin-api-key, while the test asserts "user" (auto_purchase_grant_test.go:238), a value only the no-principal path produces. Consider one vocabulary and a test that goes through the principal path.
  • N2 auto_purchase_grant.go:88,123: both "changed" log lines are written inside the gate, before the store commits, so a failed write still logs a change. Log after UpdatePurchasePlan / UpdateGlobalConfigAtomic succeed.
  • N3 internal/config/validation.go:524-533: the new const block now sits between validateNotificationAndCap's doc comment and the func, so godoc attaches that comment to the consts.
  • N4 (OWNER): PUT /api/plans/{id}/accounts and edits to term/payment/coverage on an armed plan stay on update:plans. They change what the plan buys but not the per-step ceiling, which is consistent with A1. Recording it here so the decision is explicit.

Verified OK

  • planArmed = enabled && auto_purchase && cap != nil (auto_purchase_grant.go:30-32). It gates POST (handler_plans.go:181), PUT after mergeUpdatedPlan keeps an omitted cap (:314, so an omitted cap cannot dodge it), and PATCH (:720), regardless of switch state. Plan handlers do not read global_config. Optimistic updated_at (store_postgres.go:823) closes the read-gate-write race.
  • The switch gate runs inside the UpdateGlobalConfigAtomic closure, with switchBefore taken from existing before unmarshal, chained after requireRIExchangeAutoModeGrant (handler_config.go:105,120-123).
  • execute:purchases is in adminCarvedOuts (internal/auth/types.go:136-145), so admin:* alone gets a 403 (the mock answers from the real AuthContext.HasPermission). The admin API key is refused for arming, switch on and cap raise, while disarm, cap lowering and switch off still work with it.
  • Other writers: disablePlan (handler_purchases.go:578) only de-escalates; the RI-exchange closure (handler_ri_exchange.go:2135) touches only ri_*; updatePlanNextExecutionDateTx rewrites the plan as loaded; there are no other UPDATE purchase_plans / global_config writers, no bulk/import/v2 routes, and nothing in non-test code sets the switch to true. Stamps are in the read and upsert (store_postgres.go:97,269,297-299) and are json:"-", and the forging test covers case variants.
  • Cap bounds: 0, negative, 0.004, NaN, Inf, 1e308 and quoted numbers give 400s.

Evidence

  • go build ./..., go vet, go test -count=1 ./internal/api ./internal/config ./internal/server ./internal/auth: all ok at head.
  • Fails before the fix: the new TestAutoPurchaseGate_* tests copied onto the merge-base 91217251 fail in 16 subtests (create, PUT, PATCH, switch, cap validation).
  • Mutants: 14 run, all killed: removed create/PUT/PATCH/config gate calls, > to >=, dropped admin-key refusal, dropped stamp write, dropped cap from planArmed, never-escalate-from-unarmed, switchBefore constant, min/max cap bounds, credential-kind label. M3/M4 were re-run without compile errors.
  • CI at review time: Integration Tests still in progress; mergeStateStatus BLOCKED. Not merged.

Caps with more than two decimals are now rejected (400) instead of rounded:
Postgres rounds the decimal pgx sends, so 1.005 was stored as 1.01 while the
gate compared 1.00 and let an admin raise a cap without execute:purchases.
StepCapCents parses the decimal text, so the compared and stored values are
identical (proved on a real PG17 round trip). Also: stamp credential kind is
"session" in production vocabulary, the kill-switch change is logged after the
commit, and the cap bounds const no longer splits the validator's doc comment.

Refs #631

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: Essentials
  • Run ID: 635810a1-5616-4a73-a51d-7bf94daf67f9
📥 Commits

Reviewing files that changed from the base of the PR and between 9121725 and 692d801.

📒 Files selected for processing (7)
  • internal/api/auto_purchase_grant.go
  • internal/api/auto_purchase_grant_test.go
  • internal/api/handler_config.go
  • internal/api/handler_plans.go
  • internal/config/store_postgres_auto_purchase_stamps_integration_test.go
  • internal/config/validation.go
  • internal/config/validation_step_cap_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

📝 Walkthrough

Plan creation and updates require execute:purchases when they arm auto-purchase or raise an armed plan’s cap. Enabling the global auto-purchase switch also requires that permission. Step caps must be representable as whole cents within the configured bounds.

Changes: Auto-purchase controls

Layer / File(s) Summary
Whole-cent step-cap validation
internal/config/validation.go, internal/config/validation_step_cap_test.go, internal/config/store_postgres_auto_purchase_stamps_integration_test.go
StepCapCents accepts caps from 0.01 through 1e15 with no more than two decimal places. Tests check accepted and rejected values and compare converted cents with values stored in PostgreSQL.
Plan escalation authorization
internal/api/auto_purchase_grant.go, internal/api/handler_plans.go, internal/api/auto_purchase_grant_test.go
Plan creation and updates validate plans before saving. Arming a plan or raising an armed plan’s cap requires execute:purchases; tests cover authorization, rejected writes, and invalid caps.
Global switch authorization and change stamps
internal/api/auto_purchase_grant.go, internal/api/handler_config.go, internal/api/auto_purchase_grant_test.go
Enabling the global switch requires execute:purchases; disabling it does not. Switch changes record the actor, credential kind, and UTC timestamp, and tests check that config input cannot forge these stamps.

Priority: ➖ Normal

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

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant PlanAPI
  participant validateAndGatePlan
  participant CallerSession
  participant PurchasePlanStore
  PlanAPI->>validateAndGatePlan: validate plan and detect escalation
  validateAndGatePlan->>CallerSession: check execute:purchases for escalation
  validateAndGatePlan-->>PlanAPI: return validation or authorization result
  PlanAPI->>PurchasePlanStore: save permitted plan
Loading
Merge Risk: ⚪ Minimal · up to 692d8

The investigated cap and plan-update paths preserve the intended purchase controls. No issue identified here needs to be resolved before merge.

Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: requiring the execute:purchases grant before arming auto-purchase plans.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Gate re-review (Opus 5.5, claude-opus-5-5): APPROVED at 692d80124f5d301b3a19d8cf041c9e302011f373

Diff b6e4450d..692d8012 reviewed in full (6 files). Local runs used git archive of the exact head, GOTOOLCHAIN=go1.26.9, macOS.

B1 resolved. config.StepCapCents (internal/config/validation.go:540-553) parses strconv.FormatFloat(v,'f',-1,64) and rejects more than 2 fractional digits. pgx v5.9.2 encodes a float64 into NUMERIC using exactly that call (pgtype/numeric.go:372 binary, :526 text), so the value that is validated and compared is the value Postgres stores. Validate calls it on POST, on PUT after the cap merge, and on PATCH. capCents (auto_purchase_grant.go:39-47) now uses it, so the cap path has no float multiplication. formatCap is used for logging only.

  • Real PG17 (throwaway cluster, deleted afterwards): 1.005, 0.015 and 1000.005 are rejected by StepCapCents (PG would have stored 1.01, 0.02 and 1000.01). 1.0049 is rejected. 1e-3 is out of bounds. 100.10 gives 10010 cents and stores 100.10. 1e15 gives exact cents. Handler: 1e308 and the quoted number "100" return 400.
  • The old repro (admin:* PUT of 1.00 -> 1.005 on an armed plan) now returns an error, and nothing is written.
  • go test -tags integration against the throwaway PG17: TestPostgresStoreDB_StepCapStoredEqualsCompared (3005 caps, stored cents equal compared cents) and TestPostgresStoreDB_AutoPurchaseStamps_FreshMigrateAndRIWritePreserve PASS.

N1-N3 resolved.

  • via now uses one vocabulary (session, user-api-key, admin-api-key).
  • The switch log is written after UpdateGlobalConfigAtomic returns successfully (handler_config.go:129). The plan log is reworded "authorized (write follows)", which is acceptable.
  • The doc comment is back on its func.

Mutants on the changed lines (run once): dropping the >2-decimals check, >2 -> >3, dropping the cents, dropping the bounds, and skipping the cap in Validate were all killed. N6-N9 could not run because the host disk was full. Analysis instead: the MaxInt64 error branch and a math.Round fallback sit behind Validate, so they are redundant; the switch log has no test; the via label is asserted at auto_purchase_grant_test.go:243. None of these block.

Non-blocking follow-up: a plan whose cap was stored before this PR above 1e15 or with more than 2 decimals (the column allows up to 9999999999999999.99) can no longer be PATCHed, even to disarm, because Validate runs on the whole plan. The feature is dormant and no UI sets caps, so this is unlikely. Switching the global kill switch off still works.

Evidence:

  • go vet ./internal/api ./internal/config ./cmd/... OK.
  • go test -count=1 ./internal/api ./internal/config ./internal/server ./internal/auth: all ok.
  • The cmd/* link step could not run locally (host disk full); CI Build covers it.
  • git-secrets with --register-aws, BSD and GNU grep: clean on the changed files.
  • CI: all jobs green, mergeStateStatus CLEAN at this SHA.
  • From the first pass (unchanged here): the fails-before run against base and 14 earlier mutants were all killed.

Merging with --match-head-commit 692d80124f5d301b3a19d8cf041c9e302011f373.

@cristim
cristim merged commit 33c3667 into main Oct 13, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xl Multi-week / refactor impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant