[cristim]

Summary

Adds common.ErrRefusedBeforePurchase to pkg on its own, ahead of #672, which wraps it around the AWS approved-upfront guard refusals. The provider modules are verified standalone (GOWORK=off) against a pinned pkg pseudo-version. #672 therefore can't use a symbol that isn't on main yet.

This follows the repo's multi-module order: the pkg change lands first, as with provider.Config (205cf99, before #609). #672 then re-pins providers/aws to this merge commit. Pinning to a PR branch commit is avoided, because the squash merge strands it (the clean-up in #585).

Contract

• What it means: it wraps any error from the approved-upfront check, which runs before the purchase request is built or sent.
• Presence vs absence: its presence is sufficient to know that the call sent no purchase request. Its absence proves nothing.
• Scope: it describes only the call that returned it, never earlier calls with the same idempotency token.
• Wrappers: a wrapper that combines several purchase calls must not propagate it unless none of them sent a request.

This is the same text claude-nibblets and claude-darling reviewed in #672 (da179ab), reworded for readability.

Verification

go vet ./common/ and go test -count=1 ./common/ pass in pkg. Nothing uses the sentinel yet; #672 is its first user.

Refs LeanerCloud/cloud-commitments-mcp#124

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: feat/err-refused-before-purchase-pkg
• File: .coderabbit.yaml
• Commit: a37da6c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: fafcc318-e627-4c63-aee7-bdf1832bfefe

Commits

Reviewing files that changed from the base of the PR and between fe95607 and a37da6c.

Files selected for processing: 3

• CHANGELOG.md
• pkg/common/types.go
• pkg/common/types_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

Independent review (claude-darling, cold): CHANGES REQUESTED 8ccfd8c

I reviewed this cold on an export of 8ccfd8c (pkg standalone, GOWORK=off), treating the PR body as a claim. I used no other reviewer's verdict. CI is green at this SHA and there are 0 unresolved threads. go vet ./... and go test -count=1 ./... pass in pkg. Splitting pkg out first is the right order: a standalone provider build can only use a pkg symbol once it is on main, as with 205cf99 before #609.

Finding: the new sentinel has no test, unlike its siblings

pkg/common/types_test.go pins each existing purchase sentinel:

• TestErrOutcomeUnknown (:584) checks that the error survives wrapping with %w, that an errors.New with the same text does not match it, and its exact text;
• TestErrCurrencyNotAccepted (:624) checks wrapping and that same-text mismatch.

#673 adds ErrRefusedBeforePurchase without one. Nothing else in the repo uses it yet, so nothing checks its identity. Each probe below changed one line on a fresh export, then ran go vet and go test ./... for the whole pkg module:

Probe in pkg/common/types.go:45	Result
var ErrRefusedBeforePurchase = ErrOutcomeUnknown	survives (all pass)
var ErrRefusedBeforePurchase = ErrCurrencyNotAccepted	survives
Text changed to "purchase refused"	survives

The first probe is the one that matters. The sentinel exists so that a consumer (mcp#124) can treat errors.Is(err, ErrRefusedBeforePurchase) as "no money was spent". If it ever became the same error as ErrOutcomeUnknown, every unknown-outcome purchase, which may have bought something, would be counted as unspent.

#672's provider tests will only cover this once #672 lands. Even then, they check the sentinel only through its five call sites and never compare it with the other sentinels. The pkg test is the place to pin it, as is done for its siblings. Suggested, next to TestErrCurrencyNotAccepted:

func TestErrRefusedBeforePurchase(t *testing.T) {
	wrapped := fmt.Errorf("%w: %w", ErrRefusedBeforePurchase, errors.New("upfront 1200 exceeds approved 1000"))
	assert.ErrorIs(t, wrapped, ErrRefusedBeforePurchase)
	assert.NotErrorIs(t, errors.New("refused before purchase"), ErrRefusedBeforePurchase)
	for _, other := range []error{ErrOutcomeUnknown, ErrCurrencyNotAccepted, ErrCommitmentPurchaseNotSupported} {
		assert.NotErrorIs(t, ErrRefusedBeforePurchase, other)
		assert.NotErrorIs(t, other, ErrRefusedBeforePurchase)
	}
	assert.Equal(t, "refused before purchase", ErrRefusedBeforePurchase.Error())
}

The text assertion is needed only if consumers match on the text (#672 adds the prefix refused before purchase: to the guard errors). Otherwise it can be dropped.

Checked (no other findings)

• The doc comment matches the contract in the body: presence is sufficient, absence proves nothing, it covers only the call that returned it, and the rule for wrappers. One wording nit, optional: the sentinel does not "wrap" anything. "Errors wrapping ErrRefusedBeforePurchase come from the approved-upfront check..." would be more precise.
• The text is unique in pkg. It doesn't collide with any other sentinel's text.
• CHANGELOG: the entry sits under [Unreleased] / ### Added, which is correct.
• Scope: only pkg/common/types.go (+10) and CHANGELOG.md (+2) change. No provider module is touched, so no standalone pin needs moving.

[cristim]

Independent review (claude-fleet, cold): CHANGES REQUESTED 8ccfd8c

I reviewed this cold at 8ccfd8c (base fe95607), treating the PR body as a claim. CI is green. The sentinel and its contract text are sound. The one gap is that nothing pins the sentinel, while its sibling has exactly that test.

Blocking

1. The new sentinel is untested, unlike ErrOutcomeUnknown.

   • TestErrOutcomeUnknown (pkg/common/types_test.go:584-589) pins three properties of that sentinel: detection through a %w wrap, that a different error with the same text is not it, and its exact text. The doc at pkg/common/types.go:32-33 says why: consumers such as the platform persist errors as strings, so the text is a contract.
   • ErrRefusedBeforePurchase gets none of these. I ran mutation probes on an export of pkg, each restored and checked with cmp. All three pass vet and the whole pkg test suite:
     • text changed to "purchase refused": survived;
     • var ErrRefusedBeforePurchase = ErrOutcomeUnknown: survived;
     • var ErrRefusedBeforePurchase = ErrCurrencyNotAccepted: survived.
   • The second is the dangerous one for the stated purpose. The contract says this sentinel's presence means "this call sent no purchase request", so a consumer may treat it as safe to retry. If it ever aliased ErrOutcomeUnknown, an outcome-unknown purchase would read as refused-before-purchase and could be re-sent. For EC2 and Redshift, which have no idempotency token, that is the double purchase the ErrOutcomeUnknown doc warns about (types.go:27-31).
   • Please add TestErrRefusedBeforePurchase, mirroring TestErrOutcomeUnknown. It should assert:
     • errors.Is through fmt.Errorf("…: %w", ErrRefusedBeforePurchase);
     • NotErrorIs for errors.New("refused before purchase");
     • the exact Error() text;
     • NotErrorIs both ways against ErrOutcomeUnknown and ErrCurrencyNotAccepted, so the three can never alias.

   With that test, all three probes above fail.

Checked

• The contract text (types.go:37-45) is careful: presence is sufficient, absence proves nothing, it covers only this call and not earlier calls with the same idempotency token, and a wrapper combining several calls must not propagate it unless none of them sent a request. Those are the right four rules for a "nothing was sent" signal.
• It doesn't duplicate an existing sentinel. ErrCurrencyNotAccepted is also a refusal before purchase, but its meaning is the reason (currency), not "nothing sent". The contract's "absence proves nothing" rule keeps the two independent. No other sentinel in pkg or the providers means "not sent".
• The CHANGELOG entry sits under ## [Unreleased] and ### Added.
• Landing pkg first, before feat(common): mark approved-upfront guard refusals with ErrRefusedBeforePurchase #672 re-pins providers/aws, matches the repo's multi-module order, and the PR body's reasoning holds.
• One go job: in pkg, go vet ./common/ and go test -count=1 ./... pass (13 packages). That ran with GOWORK=off and the local toolchain (go1.27.2, which satisfies pkg/go.mod). CI's own runs are green too.

Non-blocking

• The doc ties the sentinel to "the approved-upfront check", an AWS guard, inside the provider-agnostic common package. Something like "a pre-purchase guard (today, the AWS approved-upfront check)" would keep the contract true when Azure or GCP add their own guards.

Not merging.

[cristim]

Independent review (claude-darling, cold, delta): APPROVE a37da6c

This re-reviews the delta after my CHANGES REQUESTED at 8ccfd8c. 8ccfd8c...a37da6c is one commit that adds only TestErrRefusedBeforePurchase to pkg/common/types_test.go (+13). types.go is unchanged since my first pass.

The new test

It checks everything the finding asked for:

• Wrap: errors.Is finds the sentinel through fmt.Errorf("%w: %w", ...).
• Same text is not the same error: a fresh errors.New("refused before purchase") does not match.
• Distinct from its siblings, in both directions: ErrOutcomeUnknown, ErrCurrencyNotAccepted and ErrCommitmentPurchaseNotSupported.
• Exact text: the message is pinned exactly.

Probes

Each probe changed one line, pkg/common/types.go:45, on a fresh export of a37da6c, then ran go vet and go test ./common/ (GOWORK=off). All 5 fail TestErrRefusedBeforePurchase:

Probe	Result
= ErrOutcomeUnknown (alias)	killed
= ErrCurrencyNotAccepted (alias)	killed
= ErrCommitmentPurchaseNotSupported (alias)	killed
= fmt.Errorf("refused before purchase: %w", ErrOutcomeUnknown) (same text, but wraps the sibling)	killed (it compiles; the failure is an assertion)
Text changed to "purchase refused"	killed

The first three are the probes that survived at 8ccfd8c. The fourth is new. It is the dangerous variant: the text is correct, but an outcome-unknown purchase would match the sentinel. The test's reverse-direction NotErrorIs kills it.

Other checks

• go vet ./... and go test -count=1 ./... pass across the whole pkg module at a37da6c.
• CI is all green at this SHA, and there are 0 unresolved threads.
• My earlier wording nit on the doc comment is optional and not blocking.

No findings.

[cristim]

Independent review (claude-fleet, cold): APPROVE a37da6c

Delta re-review of 8ccfd8c..a37da6c (one commit, test-only, pkg/common/types_test.go +13). It fixes my CHANGES REQUESTED at 8ccfd8c. CI is green at this SHA.

Checked

• The new TestErrRefusedBeforePurchase covers everything I asked for:

  • wrapping: errors.Is through fmt.Errorf("%w: %w", ErrRefusedBeforePurchase, cause);
  • same-text non-identity: NotErrorIs for errors.New("refused before purchase");
  • non-alias, in both directions, against ErrOutcomeUnknown, ErrCurrencyNotAccepted and ErrCommitmentPurchaseNotSupported;
  • the exact Error() text.

• My earlier probes now fail the test. I ran them on an export of pkg, each restored and checked with cmp:

  • text changed to "purchase refused": killed;
  • var ErrRefusedBeforePurchase = ErrOutcomeUnknown: killed;
  • var ErrRefusedBeforePurchase = ErrCurrencyNotAccepted: killed;
  • var ErrRefusedBeforePurchase = ErrCommitmentPurchaseNotSupported (new): killed.

  All four fail in TestErrRefusedBeforePurchase.

• One go job: in pkg, go vet ./common/ and go test -count=1 ./... pass (13 packages).

• The production code (pkg/common/types.go) and the CHANGELOG are unchanged since 8ccfd8c, so my earlier checks of the contract text and the multi-module order still hold.

The non-blocking doc note from my earlier review (the comment names an AWS-specific check inside common) still stands as a suggestion.

Not merging.