Skip to content

feat(common): add OrderIDFromToken, which fails loud on a malformed idempotency token - #487

Merged
cristim merged 1 commit into
mainfrom
feat/104-reservation-order-id-fail-loud
Oct 17, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/104-reservation-order-id-fail-loud

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Summary

Part 1 of 2 for #104 item A09-020. ReservationOrderID(token, fallback) silently returns the caller's non-idempotent fallback when a non-empty idempotency token cannot be formatted as a GUID (shorter than 32 chars, or non-hex in the first 32). A re-drive of such an Azure Savings Plan purchase then PUTs a fresh order alias and can create a second plan.

This PR adds, in pkg/common/tokens.go:

  • ErrInvalidIdempotencyToken (exported sentinel).
  • OrderIDFromToken(token, fallback string) (string, error): empty token returns the fallback (CLI path, unchanged); a valid token returns the deterministic GUID; a non-empty invalid token returns an error wrapping the sentinel, with the token masked via MaskToken.

ReservationOrderID is unchanged apart from a doc note to prefer OrderIDFromToken.

Why additive: make build and lint use go.work (local pkg), while the "Verify standalone provider modules" step (ci.yml, GOWORK: 'off') builds providers/azure against its pinned older pkg. An in-place signature change breaks one of the two. A Deprecated: marker is avoided for the same reason (staticcheck SA1019 on the azure callers under go.work).

Part 2 (after this merges): bump the providers/azure pkg pin, switch savingsplans/client.go to OrderIDFromToken (validated before the purchase body and alias client are built), and delete ReservationOrderID and its test.

Token producers checked: the platform and mcp derive tokens with DeriveIdempotencyToken (64-char lowercase SHA-256 hex); the CLI sets none. No producer sends dashed UUIDs, so the strict check rejects no real purchase.

Refs #104

Tests

TestOrderIDFromToken table:

  • Rejected: abc, 31 hex, 32 non-hex, a non-hex char inside the first 32 of a 64-char token, a dashed UUID. Each asserts errors.Is(err, ErrInvalidIdempotencyToken), that the error contains MaskToken(token) and does not contain the raw token.
  • Accepted: exactly 32 lowercase hex, 32 uppercase hex (lowercased GUID), 64 hex (first 32 used), empty (fallback).

Mutation probes (scratch copies): returning the fallback on an invalid token, dropping the %w wrap, printing the raw token, and leaking a 20-char prefix each fail the table.

Verification

make build, make vet (all five modules, go.work), (cd pkg && GOWORK=off go test -race -short ./...), (cd providers/azure && GOWORK=off go build ./...), pinned golangci-lint v2.10.1 on pkg and on providers/azure under go.work (0 issues each), gofmt, and the commit/pre-push hooks pass. Unit-level evidence only.

Local review: the plan had two adversarial passes by a non-author reviewer before code, and the diff had two more by the same reviewer (pass 1: masking assertion too weak, fixed; pass 2: commit message wording, fixed).

🤖 Generated with Claude Code

@cristim cristim added impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner type/bug Defect triaged Item has been triaged labels Oct 16, 2026
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • 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 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 70 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-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: ef580542-47f6-4c53-94d5-f87901c416f9
📥 Commits

Reviewing files that changed from the base of the PR and between 86b86a0 and 00a0422.

📒 Files selected for processing: 2
  • pkg/common/tokens.go
  • pkg/common/tokens_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 16, 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

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 90f9c1e

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Coordinator: claude-cc-reviewer.

Blocking findings: none.

How the order ID is extracted:

  • Tokens come from DeriveIdempotencyToken (pkg/common/tokens.go:64-67), a 64-character SHA-256 hex string.
  • OrderIDFromToken (:137-146) truncates the token to its first 32 hex characters via IdempotencyGUID (:98-107) and lowercases the result. It truncates the token; it does not decode it.

Behaviour by input:

  • Valid token: returns the GUID, and the round trip from DeriveIdempotencyToken is tested.
  • Short token, or non-hex in the first 32 characters: returns ("", err) wrapping ErrInvalidIdempotencyToken, with the token masked (:143).
  • Empty token: returns the fallback (:138), which is ("", nil) on the CLI path.

Note (non-blocking): characters after position 32 are not checked. For example, "<32hex> " returns the GUID with a nil error. That is fine for the real 64-character tokens, but loose; requiring exactly 32 or 64 hex characters would be stricter.

Caller: none yet. The body names part 2 (azure savingsplans/client.go:360, which still calls ReservationOrderID) as the follow-up.

What was attacked and how:

  • go vet ./common/... is clean. go test -race -count=1 ./common/... passes 342, including TestOrderIDFromToken with 11 subtests.
  • Mutation probes, both killed:
    • returning the fallback on an invalid token;
    • relaxing the length check to 16.

Gate: p3, no closing issue. Merge once CI is green, in its turn in the go merge order. (claude-cc-reviewer)

…dempotency token

ReservationOrderID silently returns the caller's non-idempotent fallback
whenever a non-empty token cannot be formatted as a GUID (shorter than 32
chars or non-hex in the first 32). A re-drive of such a purchase then uses a
fresh order ID and creates a second reservation. OrderIDFromToken returns an
error wrapping the new ErrInvalidIdempotencyToken for that case, with the
token masked via MaskToken; an empty token still returns the fallback.

ReservationOrderID is kept unchanged, with a doc note to prefer
OrderIDFromToken, so providers pinned to an older pkg keep compiling (no
Deprecated: marker, which would fail SA1019 on the azure callers under
go.work). PR-B switches providers/azure and
removes ReservationOrderID.

Refs #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the feat/104-reservation-order-id-fail-loud branch from 90f9c1e to 00a0422 Compare October 17, 2026 00:01
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Pure rebase onto main after go#484 merged. git range-diff of 90f9c1e against new head 00a0422 shows the commit identical (=), so the CLEAN verdict carries. (claude-cc-reviewer)

@cristim
cristim merged commit 7383c3a into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the feat/104-reservation-order-id-fail-loud branch October 17, 2026 00:15
cristim added a commit that referenced this pull request Oct 17, 2026
Move providers/azure from pkg 4566498 to 7383c3a so the
savingsplans purchase path can call common.OrderIDFromToken (#487).

Notable pkg changes pulled in (all additive):
- feat(common): OrderIDFromToken and ErrInvalidIdempotencyToken (#487)
- feat(common): EC2 platform, tenancy and scope dimensions on Commitment
- fix(recfilter): match EC2 commitments on those dimensions when deduplicating
- feat(insurance): decode plan line-item identity and read complete pages
- docs(ladder): document provider commitment precision at PurchaseLayer

Refs #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cristim added a commit that referenced this pull request Oct 17, 2026
… the purchase PUT (#495)

* build(azure): bump the pkg pin past OrderIDFromToken

Move providers/azure from pkg 4566498 to 7383c3a so the
savingsplans purchase path can call common.OrderIDFromToken (#487).

Notable pkg changes pulled in (all additive):
- feat(common): OrderIDFromToken and ErrInvalidIdempotencyToken (#487)
- feat(common): EC2 platform, tenancy and scope dimensions on Commitment
- fix(recfilter): match EC2 commitments on those dimensions when deduplicating
- feat(insurance): decode plan line-item identity and read complete pages
- docs(ladder): document provider commitment precision at PurchaseLayer

Refs #104

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

* fix(azure/savingsplans)!: refuse a malformed idempotency token before the purchase PUT

PurchaseCommitment silently turned a non-empty but malformed idempotency
token into a timestamp alias name, so a re-drive would PUT a second alias
and buy a second Savings Plan. Derive the alias name with
common.OrderIDFromToken right after the currency is resolved, before the
body and the alias client are built, and return the error without any PUT.
An empty token still falls back to the timestamp name (CLI path).

Remove the now caller-less common.ReservationOrderID and its test.

BREAKING CHANGE: common.ReservationOrderID was removed in favor of
common.OrderIDFromToken, which returns an error wrapping
common.ErrInvalidIdempotencyToken for a malformed non-empty token.

Refs #104

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

* test(azure/savingsplans): tighten the malformed-token test and fix the PurchaseCommitment doc

Name OrderIDFromToken in the PurchaseCommitment doc and document the
malformed-token error. Reword the comments to claim only what the test
proves, and give the mock a poller so the fallback mutant fails on an
assertion instead of a nil-pointer panic.

Refs #104

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant