diff --git a/app/Console/Commands/SyncStripeCatalogue.php b/app/Console/Commands/SyncStripeCatalogue.php index 839041a..b34071c 100644 --- a/app/Console/Commands/SyncStripeCatalogue.php +++ b/app/Console/Commands/SyncStripeCatalogue.php @@ -104,9 +104,27 @@ class SyncStripeCatalogue extends Command $productId = $stripe->createProduct( $family->name, ['plan_family' => $family->key, 'plan_family_id' => (string) $family->id], - // Keyed on our row, so a crash between Stripe creating - // the product and us storing its id gives back the same - // product on the next run rather than a second one. + // Covers a RETRY, for twenty-four hours, and nothing + // after that: Stripe forgets a key at the end of them. + // A crash between Stripe creating the Product and us + // storing its id therefore leaves an orphan, and the + // next run past the expiry makes a second Product for + // this family. There is no recognition step for Products + // — App\Services\Billing\AdoptStripePrice covers Prices + // only — so nothing here ever asks Stripe what Products + // it already has. A KNOWN GAP, not something this key + // closes, and a worse one than a duplicate Price: + // activePricesFor() would then be asked about the new + // Product, and the Price recognition goes blind for the + // whole family. + // + // IdempotencyKey::forProduct() folds the name and the + // metadata into what goes on the wire, so a renamed + // family under an unstored id now mints a second Product + // where it used to answer HTTP 400 for a day. That is + // the trade this branch chose deliberately: a blockade + // reaches a paying customer, a duplicate Product does + // not. idempotencyKey: "clupilot-product-{$family->id}", ); $family->update(['stripe_product_id' => $productId]); @@ -130,9 +148,16 @@ class SyncStripeCatalogue extends Command return self::SUCCESS; } + // "or adopted", because the count is taken BEFORE ensure() runs and + // ensure() may find the Price already at Stripe and adopt it instead of + // creating anything — see App\Services\Billing\AdoptStripePrice. Telling + // which of the two happened is the whole purpose of that step, so the + // first thing an operator reads after an interrupted run must not assert + // a creation that did not take place. The log entry adoption writes names + // the Price it took over. $this->info($dryRun - ? "{$created} object(s) would be created. Run without --dry-run to create them." - : "{$created} object(s) created in Stripe."); + ? "{$created} object(s) would be created or adopted. Run without --dry-run to do it." + : "{$created} object(s) created or adopted in Stripe."); return self::SUCCESS; } diff --git a/app/Services/Billing/AddonPrices.php b/app/Services/Billing/AddonPrices.php index 6973cdd..ecb49bd 100644 --- a/app/Services/Billing/AddonPrices.php +++ b/app/Services/Billing/AddonPrices.php @@ -44,7 +44,10 @@ use Illuminate\Database\UniqueConstraintViolationException; */ final class AddonPrices { - public function __construct(private readonly StripeClient $stripe) {} + public function __construct( + private readonly StripeClient $stripe, + private readonly AdoptStripePrice $adopt, + ) {} /** * The net a whole term of this module costs. @@ -125,30 +128,45 @@ final class AddonPrices $productId = $this->product($addonKey); - $priceId = $this->stripe->createPrice( + $metadata = [ + // Read back when a Stripe invoice line has to be turned into + // wording a customer can read — see StripeInvoiceLines. + 'addon' => $addonKey, + // Which of the module's two Prices this is, for anyone reading + // Stripe's own dashboard, where they would otherwise differ only + // by an amount. + 'tax_treatment' => $reverseCharge ? 'reverse_charge' : 'domestic', + ]; + + // Asked BEFORE minting, because a run that died between Stripe's create + // and our insert left a Price no table of ours knows — and the key below + // stops protecting it after twenty-four hours. See AdoptStripePrice for + // what may be taken over and why so narrowly. + $priceId = ($this->adopt)( productId: $productId, amountCents: $amount, currency: $currency, interval: $interval, - metadata: [ - // Read back when a Stripe invoice line has to be turned into - // wording a customer can read — see StripeInvoiceLines. - 'addon' => $addonKey, - // Which of the module's two Prices this is, for anyone reading - // Stripe's own dashboard, where they would otherwise differ only - // by an amount. - 'tax_treatment' => $reverseCharge ? 'reverse_charge' : 'domestic', - ], - // Keyed on what the Price IS, so a crash between Stripe creating it - // and us storing its id gives back the same Price on the next - // attempt rather than a second one at the same money. The amount is - // the CHARGED one, so the move from net to gross does not replay the - // net Price the old key was minted under — and the treatment is in - // there because at a rate of nought the two Prices are the same - // amount: one key would have Stripe hand the same object back for - // both, and the two rows would then share a Price, so archiving the - // gross one at the next rate change would withdraw the very Price the - // net side is still selling. + metadata: $metadata, + identifying: ['addon'], + claimed: fn (string $id) => StripeAddonPrice::query() + ->where('stripe_price_id', $id) + ->exists(), + ); + + $priceId ??= $this->stripe->createPrice( + productId: $productId, + amountCents: $amount, + currency: $currency, + interval: $interval, + metadata: $metadata, + // Says "I have already sent this call", nothing more. The amount is + // the CHARGED one and the treatment is in there because at a rate of + // nought the two Prices are the same amount — but what stops a + // second Price for one figure is AdoptStripePrice above, not this: + // Stripe forgets a key after twenty-four hours. The metadata is + // folded in by IdempotencyKey inside the client, so changing the + // format above can never again refuse the call for a day. idempotencyKey: "clupilot-addon-price-{$addonKey}-{$interval}-{$amount}-{$currency}" .($reverseCharge ? '-rc' : ''), ); @@ -269,6 +287,11 @@ final class AddonPrices return $this->stripe->createProduct( app(AddonCatalogue::class)->name($addonKey), ['addon' => $addonKey], + // A retry's guard for twenty-four hours and nothing beyond them, the + // same as the plan side's in SyncStripeCatalogue — and the same known + // gap: there is no recognition step for Products, so a run that + // created this one and died before any row carried its id leaves an + // orphan nothing here will ever ask Stripe about. idempotencyKey: "clupilot-addon-product-{$addonKey}", ); } @@ -298,6 +321,28 @@ final class AddonPrices // Two bookings of the same module landed together. Stripe replayed // one Price for both — the idempotency key saw to that — so there is // nothing to correct here beyond letting the first row stand. + // + // Since the unique index on `stripe_price_id` below, this catch has a + // SECOND meaning: another TUPLE claims this Price id, which is two + // callers adopting the same orphan at once. The outcome then is no row + // for this tuple at all, not "the first row stands" — the row that + // exists belongs to the other tuple. That self-heals on the next + // ensure(): the orphan is claimed, so adoption refuses it and + // createPrice() mints this tuple its own Price. No money moves in the + // meantime, because both callers matched the same `unit_amount` + // before adopting; what a caller gets back here is the Price id it + // asked for, and it charges what was asked. + // + // What keeps two ROWS off one Price id is the `claimed` callback in + // ensure(), asked before minting. The unique index on + // `stripe_price_id` (`stripe_addon_prices_price_unique`, since + // 2026_07_31_210000_one_row_per_stripe_price, matching + // `stripe_plan_prices.stripe_price_id` on the plan side) is the net + // UNDER that callback, not a substitute for it — two rows sharing one + // Price would be worse than the ordinary duplicate this catch + // handles: archiving one — the ordinary response to a superseded + // figure — would then withdraw the very Price the other row is still + // selling. } } } diff --git a/app/Services/Billing/AdoptStripePrice.php b/app/Services/Billing/AdoptStripePrice.php new file mode 100644 index 0000000..41461d5 --- /dev/null +++ b/app/Services/Billing/AdoptStripePrice.php @@ -0,0 +1,188 @@ + $metadata what the create call would send + * @param array $identifying metadata keys that mark a Price as ours + * @param callable(string): bool $claimed is this Price id already in our register? + */ + public function __invoke( + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata, + array $identifying, + callable $claimed, + ): ?string { + // Stripe hands metadata back as strings. An un-cast int here would then + // never satisfy confirms()'s strict ===, silently disabling adoption for + // that Price forever and warning on every sweep and every booking. One + // cast here makes the class independent of what a caller happens to pass. + $metadata = array_map(fn ($value) => (string) $value, $metadata); + + $candidates = []; + + foreach ($this->stripe->activePricesFor($productId) as $price) { + if ($price['unit_amount'] !== $amountCents + || $price['currency'] !== strtoupper($currency) + || $price['interval'] !== $interval) { + continue; + } + + if ($claimed($price['id'])) { + continue; + } + + // Contradicts on something it carries: another Price of ours, not a + // mystery. Silent — at a VAT rate of nought the two treatments share + // an amount, and this would otherwise warn on every sweep. + if ($this->contradicts($price['metadata'], $metadata)) { + continue; + } + + if (! $this->confirms($price['metadata'], $metadata, $identifying)) { + Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ + 'price' => $price['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + + continue; + } + + $candidates[] = $price; + } + + if ($candidates === []) { + return null; + } + + usort($candidates, fn (array $a, array $b) => $a['created'] <=> $b['created']); + + $adopted = array_shift($candidates); + + foreach ($candidates as $duplicate) { + $this->stripe->archivePrice($duplicate['id']); + + Log::warning('stripe: stopped selling a duplicate price for one figure', [ + 'price' => $duplicate['id'], + 'adopted' => $adopted['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + ]); + } + + // Only when it differs, so a sweep over a healthy catalogue makes no + // writes at Stripe at all. Compared through array_intersect_key() rather + // than a bare !==, for two reasons: Stripe MERGES a metadata write rather + // than replacing it, so a Price can carry a key of its own that a write + // would never remove and a bare !== would then never call equal — and + // PHP's array !== is key-order sensitive, so Stripe handing the same + // values back in a different order would trigger a write every single + // sweep. ksort() on both sides settles the order; the intersect settles + // the extra key, by looking only at what WE would send. + $overlap = array_intersect_key($adopted['metadata'], $metadata); + $wanted = $metadata; + ksort($overlap); + ksort($wanted); + + if ($overlap !== $wanted) { + $this->stripe->updatePriceMetadata($adopted['id'], $metadata); + } + + Log::info('stripe: adopted an existing price instead of creating a second one', [ + 'price' => $adopted['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + + return $adopted['id']; + } + + /** + * Does this Price say something about itself that we do not? + * + * @param array $found + * @param array $expected + */ + private function contradicts(array $found, array $expected): bool + { + foreach ($expected as $key => $value) { + if (array_key_exists($key, $found) && $found[$key] !== $value) { + return true; + } + } + + return false; + } + + /** + * Does it prove it is ours? + * + * Failing to contradict is not enough — an empty metadata bag contradicts + * nothing. At least one identifying key has to be there and agree. + * + * @param array $found + * @param array $expected + * @param array $identifying + */ + private function confirms(array $found, array $expected, array $identifying): bool + { + foreach ($identifying as $key) { + if (isset($found[$key]) && $found[$key] === ($expected[$key] ?? null)) { + return true; + } + } + + return false; + } +} diff --git a/app/Services/Billing/PlanPrices.php b/app/Services/Billing/PlanPrices.php index ed8d631..5d58ce7 100644 --- a/app/Services/Billing/PlanPrices.php +++ b/app/Services/Billing/PlanPrices.php @@ -40,7 +40,10 @@ use App\Services\Stripe\StripeClient; */ final class PlanPrices { - public function __construct(private readonly StripeClient $stripe) {} + public function __construct( + private readonly StripeClient $stripe, + private readonly AdoptStripePrice $adopt, + ) {} /** What Stripe is asked to take for this row from a customer treated so. */ public static function chargedCents(PlanPrice $price, TaxTreatment $treatment): int @@ -131,30 +134,56 @@ final class PlanPrices $existing = $this->registered($price, $treatment, includeArchived: true); $priceId = $existing?->stripe_price_id; + $interval = $price->term === Subscription::TERM_YEARLY ? 'year' : 'month'; if ($priceId === null) { - $priceId = $this->stripe->createPrice( + $metadata = [ + 'plan_family' => $family->key, + 'plan_version' => (string) $version->version, + 'plan_version_id' => (string) $version->id, + 'plan_price_id' => (string) $price->id, + // Read back by anything that has a Price id and needs to know + // what kind of sale it was — and by a person looking at + // Stripe's own dashboard, where two Prices on one Product + // would otherwise differ only by an amount. + 'tax_treatment' => $treatment->reverseCharge ? 'reverse_charge' : 'domestic', + ]; + + // Asked BEFORE minting: a run that died between Stripe's create and + // our insert left a Price the register does not know, and the key + // below stops protecting it after twenty-four hours. `plan_price_id` + // is what proves such a Price is this row's — one Product carries + // every version and term of a family, so the amount alone would not. + $priceId = ($this->adopt)( productId: (string) $productId, amountCents: $charged, currency: (string) $price->currency, - interval: $price->term === Subscription::TERM_YEARLY ? 'year' : 'month', - metadata: [ - 'plan_family' => $family->key, - 'plan_version' => (string) $version->version, - 'plan_version_id' => (string) $version->id, - 'plan_price_id' => (string) $price->id, - // Read back by anything that has a Price id and needs to know - // what kind of sale it was — and by a person looking at - // Stripe's own dashboard, where two Prices on one Product - // would otherwise differ only by an amount. - 'tax_treatment' => $treatment->reverseCharge ? 'reverse_charge' : 'domestic', - ], - // The CHARGED amount is part of the key, so a run after a rate - // change cannot replay the Price minted at the old figure. So is - // the treatment, and it has to be: at a rate of nought the two - // Prices are the same amount, and one key would have Stripe hand - // back the same object for both — which the register, where a - // Price id is unique, would refuse to record twice. + interval: $interval, + metadata: $metadata, + identifying: ['plan_price_id'], + claimed: fn (string $id) => StripePlanPrice::query() + ->where('stripe_price_id', $id) + ->exists(), + ); + + $priceId ??= $this->stripe->createPrice( + productId: (string) $productId, + amountCents: $charged, + currency: (string) $price->currency, + interval: $interval, + metadata: $metadata, + // Says "I have already sent this call", nothing more. The + // CHARGED amount is part of the key, so a run after a rate + // change cannot replay the Price minted at the old figure. So + // is the treatment, and it has to be: at a rate of nought the + // two Prices are the same amount, and one key would have + // Stripe hand back the same object for both — which the + // register, where a Price id is unique, would refuse to + // record twice. The metadata is folded in by IdempotencyKey + // inside the client, so changing the format above can never + // again refuse the call for a day. What stops a second Price + // for one figure is the adoption step above, not this key — + // Stripe forgets a key after twenty-four hours. idempotencyKey: "clupilot-price-{$price->id}-{$charged}" .($treatment->reverseCharge ? '-rc' : ''), ); diff --git a/app/Services/Stripe/FakeStripeClient.php b/app/Services/Stripe/FakeStripeClient.php index 5a5398d..25d4ebc 100644 --- a/app/Services/Stripe/FakeStripeClient.php +++ b/app/Services/Stripe/FakeStripeClient.php @@ -19,7 +19,7 @@ class FakeStripeClient implements StripeClient /** @var array */ public array $products = []; - /** @var array */ + /** @var array */ public array $prices = []; /** @var array */ @@ -34,7 +34,25 @@ class FakeStripeClient implements StripeClient */ public array $activated = []; - /** Idempotency key → the id first returned for it. @var array */ + /** + * Every metadata write, in order, so a test can assert that an adopted + * orphan was brought up to today's metadata instead of being replaced. + * + * @var array}> + */ + public array $metadataUpdates = []; + + /** + * Idempotency key → what was first answered for it, and a fingerprint of + * what it was first sent with. + * + * The fingerprint is the half that was missing. This ledger replayed the + * first id for a repeated key without ever looking at what the second call + * was asking for — so it was unrealistic in precisely the way that mattered, + * and no test could see the 2026-07-29 blockade. + * + * @var array + */ public array $keys = []; /** @@ -150,17 +168,25 @@ class FakeStripeClient implements StripeClient public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { + $key = IdempotencyKey::forProduct($idempotencyKey, $name, $metadata); + + // The same parameters forProduct() just fingerprinted, asked for + // through the one method that knows what they are — a local literal + // here could drift from what the key actually covers without a test + // ever seeing it. + $parameters = IdempotencyKey::productParameters($name, $metadata); + // Replays the first answer for a repeated key, as Stripe does. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; } $id = 'prod_'.substr(sha1($name.count($this->products)), 0, 12); $this->products[$id] = ['name' => $name, 'metadata' => $metadata]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($key, $id, $parameters); return $id; } @@ -173,8 +199,17 @@ class FakeStripeClient implements StripeClient array $metadata = [], ?string $idempotencyKey = null, ): string { - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + $key = IdempotencyKey::forPrice($idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata); + + // Same reasoning as createProduct(): the parameters forPrice() just + // fingerprinted, not a second literal that has to be kept in step by + // hand. + $parameters = IdempotencyKey::priceParameters($productId, $amountCents, $currency, $interval, $metadata); + + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; } $id = 'price_'.substr(sha1($productId.$amountCents.$interval.count($this->prices)), 0, 12); @@ -184,11 +219,18 @@ class FakeStripeClient implements StripeClient 'currency' => $currency, 'interval' => $interval, 'metadata' => $metadata, + // Stripe stamps every object with its creation time and + // AdoptStripePrice takes the OLDEST of several orphans. Counted + // rather than clocked, because two Prices minted in the same second + // would share a timestamp — but the two counters are independent: + // plantPrice() stamps `created: 1` of its own accord, which ties with + // a Price minted here while this list was still empty. Any test whose + // outcome depends on which of two prices is older passes `created:` + // itself, as the adoption tests do. + 'created' => count($this->prices) + 1, ]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($key, $id, $parameters); return $id; } @@ -211,6 +253,76 @@ class FakeStripeClient implements StripeClient $this->activated[] = $priceId; } + public function activePricesFor(string $productId): array + { + // No failIfAsked(): createPrice(), the call this one stands in front of, + // does not fail either, and a test that scripts an outage for a refund + // must not have its catalogue sync change behaviour underneath it. + // + // None of HttpStripeClient's charging-property filter either — the four + // fields it names (interval_count, usage_type, transform_quantity, + // billing_scheme). This fake only ever holds prices its own + // createPrice()/plantPrice() put here, and neither takes an argument for + // any of them: there is nothing non-standard a test could plant, so the + // filter would have nothing to do. Not an oversight. What that filter + // does is proven where it lives, over Http::fake — see + // tests/Feature/Billing/StripeIdempotencyKeyTest.php. + $found = []; + + foreach ($this->prices as $id => $price) { + if ($price['product'] !== $productId || in_array($id, $this->archived, true)) { + continue; + } + + $found[] = [ + 'id' => $id, + 'unit_amount' => $price['amount'], + 'currency' => strtoupper($price['currency']), + 'interval' => $price['interval'], + 'created' => $price['created'], + 'metadata' => $price['metadata'], + ]; + } + + return $found; + } + + public function updatePriceMetadata(string $priceId, array $metadata): void + { + if (isset($this->prices[$priceId])) { + $this->prices[$priceId]['metadata'] = $metadata; + } + + $this->metadataUpdates[] = ['price' => $priceId, 'metadata' => $metadata]; + } + + /** + * A Price that exists at Stripe and in no table of ours — the orphan a run + * leaves behind when it dies between Stripe's create and our insert. + * + * Here rather than in a test file because it is the state the whole adoption + * step exists for, and every test that needs it would otherwise reach into + * $prices and write the shape by hand. + */ + public function plantPrice( + string $id, + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata = [], + int $created = 1, + ): void { + $this->prices[$id] = [ + 'product' => $productId, + 'amount' => $amountCents, + 'currency' => $currency, + 'interval' => $interval, + 'metadata' => $metadata, + 'created' => $created, + ]; + } + public function updateSubscriptionPrice( string $subscriptionId, string $itemId, @@ -308,9 +420,12 @@ class FakeStripeClient implements StripeClient throw new RuntimeException("Unknown cancellation timing: {$when}"); } - // Replays the first answer for a repeated key, as Stripe does, so a retry - // after a timeout records one order to stop rather than two. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { + // No fingerprint: for a subscription cancellation, a changed parameter + // under a used key is exactly what must fail loudly. See IdempotencyKey. + $parameters = ['subscription' => $subscriptionId, 'when' => $when]; + $replayed = $this->replay($idempotencyKey, $parameters); + + if ($replayed !== null) { return; } @@ -320,9 +435,7 @@ class FakeStripeClient implements StripeClient 'key' => $idempotencyKey, ]; - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $subscriptionId; - } + $this->rememberKey($idempotencyKey, $subscriptionId, $parameters); } public function refund( @@ -332,10 +445,13 @@ class FakeStripeClient implements StripeClient ): string { $this->failIfAsked(); - // Replays the first answer for a repeated key, as Stripe does — which - // is the whole point of sending one on a refund. - if ($idempotencyKey !== null && isset($this->keys[$idempotencyKey])) { - return $this->keys[$idempotencyKey]; + // No fingerprint: for a refund, a changed parameter under a used key is + // exactly what must fail loudly. See IdempotencyKey. + $parameters = ['payment' => $paymentReference, 'amount' => (string) $amountCents]; + $replayed = $this->replay($idempotencyKey, $parameters); + + if ($replayed !== null) { + return $replayed; } $this->refunds[] = [ @@ -346,9 +462,7 @@ class FakeStripeClient implements StripeClient $id = 're_'.substr(sha1($paymentReference.count($this->refunds)), 0, 12); - if ($idempotencyKey !== null) { - $this->keys[$idempotencyKey] = $id; - } + $this->rememberKey($idempotencyKey, $id, $parameters); return $id; } @@ -382,4 +496,33 @@ class FakeStripeClient implements StripeClient throw new RuntimeException($this->failWith); } } + + /** + * The id Stripe already answered for this key, or null when it is new. + * + * Throws on a key that comes back with different parameters, which is what + * Stripe does — with this wording — and what makes a test able to see the + * failure at all. + */ + private function replay(?string $key, array $parameters): ?string + { + if ($key === null || ! isset($this->keys[$key])) { + return null; + } + + if ($this->keys[$key]['fingerprint'] !== IdempotencyKey::fingerprint($parameters)) { + throw new RuntimeException( + 'Keys for idempotent requests can only be used with the same parameters they were first used with.', + ); + } + + return $this->keys[$key]['id']; + } + + private function rememberKey(?string $key, string $id, array $parameters): void + { + if ($key !== null) { + $this->keys[$key] = ['id' => $id, 'fingerprint' => IdempotencyKey::fingerprint($parameters)]; + } + } } diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index a88a1a0..fc90f32 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -86,7 +86,7 @@ class HttpStripeClient implements StripeClient public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string { - return (string) $this->request($idempotencyKey) + return (string) $this->request(IdempotencyKey::forProduct($idempotencyKey, $name, $metadata)) ->asForm() ->post($this->url('products'), array_filter([ 'name' => $name, @@ -104,7 +104,9 @@ class HttpStripeClient implements StripeClient array $metadata = [], ?string $idempotencyKey = null, ): string { - return (string) $this->request($idempotencyKey) + return (string) $this->request(IdempotencyKey::forPrice( + $idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata, + )) ->asForm() ->post($this->url('prices'), [ 'product' => $productId, @@ -142,6 +144,91 @@ class HttpStripeClient implements StripeClient ->throw(); } + public function activePricesFor(string $productId): array + { + $prices = []; + $after = null; + + // Paged through to the end, for the same reason invoiceLines() is: Stripe + // caps a page at a hundred, and a family Product collects a Price per + // version, term, treatment and rate change. Stopping at the first page + // would leave an orphan unfound and mint the duplicate anyway. + do { + $page = $this->request() + ->get($this->url('prices'), array_filter([ + 'product' => $productId, + 'active' => 'true', + 'type' => 'recurring', + 'limit' => 100, + 'starting_after' => $after, + ], fn ($value) => $value !== null)) + ->throw() + ->json(); + + $data = (array) ($page['data'] ?? []); + + foreach ($data as $price) { + $recurring = (array) ($price['recurring'] ?? []); + + // Only prices we could have minted ourselves. createPrice() sends + // product, unit_amount, currency, recurring[interval] and metadata + // and nothing else, so Stripe defaults every other field that + // decides what a Price CHARGES — and anything but those defaults + // is a Price this platform did not write. Skipped here rather than + // returned, because AdoptStripePrice compares amount, currency and + // interval only and trusts that triple as the whole of what a + // Price charges: + // + // - interval_count multiplies the period — 3 bills every three + // months — while `interval` still reads 'month'; + // - usage_type other than 'licensed' bills metered usage; + // - a transform_quantity divides the quantity before charging, + // and modules are billed BY quantity — SyncStripeAddonItems + // sums a pack into one item at quantity n — so divide_by 10 + // would charge a customer holding three for one; + // - billing_scheme 'tiered' keeps the money in tiers, leaving + // unit_amount null. Read as 0 below, so such a Price fails the + // amount match only while the caller's own figure is not 0 — + // and PlanPrices::ensure() has no zero guard, so that is not a + // condition to rest on. + // + // An ABSENT key is Stripe's default and no reason to reject. + // Read the other way round this filter would refuse every + // legitimate price and turn recognition into a permanent no-op. + if ((int) ($recurring['interval_count'] ?? 1) !== 1 + || ($recurring['usage_type'] ?? 'licensed') !== 'licensed' + || (array) ($price['transform_quantity'] ?? []) !== [] + || ($price['billing_scheme'] ?? 'per_unit') !== 'per_unit') { + continue; + } + + $prices[] = [ + 'id' => (string) ($price['id'] ?? ''), + 'unit_amount' => (int) ($price['unit_amount'] ?? 0), + 'currency' => strtoupper((string) ($price['currency'] ?? '')), + 'interval' => (string) ($recurring['interval'] ?? ''), + 'created' => (int) ($price['created'] ?? 0), + 'metadata' => array_map( + fn ($value) => (string) $value, + (array) ($price['metadata'] ?? []), + ), + ]; + } + + $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); + } while (($page['has_more'] ?? false) === true && $after !== null); + + return $prices; + } + + public function updatePriceMetadata(string $priceId, array $metadata): void + { + $this->request() + ->asForm() + ->post($this->url('prices/'.$priceId), $this->flatten('metadata', $metadata)) + ->throw(); + } + public function updateSubscriptionPrice( string $subscriptionId, string $itemId, @@ -345,10 +432,13 @@ class HttpStripeClient implements StripeClient $request = Http::withToken($secret)->acceptJson()->timeout(20); // Stripe replays the original response for a repeated key instead of - // creating a second object. That covers the gap this cannot close on - // its own: a crash between Stripe creating a Price and us storing its - // id. Their keys expire after 24 hours, so it protects a retry, not a - // sync re-run next week — for which the stored ids are the guard. + // creating a second object — but ONLY for a call repeated exactly; a + // changed parameter under a used key is HTTP 400 for twenty-four hours. + // The catalogue calls therefore fold a fingerprint of their parameters + // into the key (see IdempotencyKey); the money calls deliberately do + // not. Their keys expire after 24 hours, so this protects a retry, not + // a sync re-run next week — for which the stored ids and + // AdoptStripePrice are the guard. return $idempotencyKey !== null ? $request->withHeaders(['Idempotency-Key' => $idempotencyKey]) : $request; diff --git a/app/Services/Stripe/IdempotencyKey.php b/app/Services/Stripe/IdempotencyKey.php new file mode 100644 index 0000000..d71f8ea --- /dev/null +++ b/app/Services/Stripe/IdempotencyKey.php @@ -0,0 +1,111 @@ + $productId, + 'unit_amount' => $amountCents, + 'currency' => strtolower($currency), + 'interval' => $interval, + 'metadata' => $metadata, + ]; + } + + /** What counts as "the same call" for a Product — same reason as priceParameters(). */ + public static function productParameters(string $name, array $metadata): array + { + return ['name' => $name, 'metadata' => $metadata]; + } + + /** + * Eight hex characters over the parameters, canonically ordered. + * + * Ordered, because PHP keeps insertion order and two callers writing the + * same metadata in a different order would otherwise send two keys for one + * call — which would mint two Prices and be a worse bug than the one this + * closes. + */ + public static function fingerprint(array $parameters): string + { + return substr(sha1(self::canonical($parameters)), 0, 8); + } + + private static function with(?string $key, array $parameters): ?string + { + // Null stays null: a caller who sends no key wants no replay, and + // inventing one here would change what the call means. + return $key === null ? null : $key.'-'.self::fingerprint($parameters); + } + + private static function canonical(array $parameters): string + { + ksort($parameters); + + foreach ($parameters as $name => $value) { + $parameters[$name] = is_array($value) ? self::canonical($value) : (string) $value; + } + + return json_encode($parameters, JSON_THROW_ON_ERROR); + } +} diff --git a/app/Services/Stripe/StripeClient.php b/app/Services/Stripe/StripeClient.php index 841db2d..0ffed51 100644 --- a/app/Services/Stripe/StripeClient.php +++ b/app/Services/Stripe/StripeClient.php @@ -124,6 +124,54 @@ interface StripeClient ?string $idempotencyKey = null, ): string; + /** + * Every active recurring Price of a Product, as Stripe holds them. + * + * The half of the catalogue mirror that was missing: nothing here ever asked + * Stripe what it already had. A run that created a Price and died before the + * row was written left an orphan our table never learned about, and once the + * idempotency key expired the next run made a SECOND live Price for the same + * money — the exact duplicate the key exists to prevent. See + * App\Services\Billing\AdoptStripePrice, which is the only caller. + * + * Active ones only, because the question being asked is "is there something + * here I can sell on?". `currency` comes back UPPER CASE, the way our own + * tables hold it. + * + * FOUR properties are checked, and they are named because the list is what + * the contract is: `recurring.interval_count` is 1, `recurring.usage_type` + * is `licensed`, there is no `transform_quantity`, and `billing_scheme` is + * `per_unit`. Those are Stripe's own defaults, and createPrice() sets none of + * the four — so anything else is a Price this platform did not write, and is + * filtered out here rather than returned. + * + * The reason it is filtered at all: AdoptStripePrice compares the amount, + * currency and interval of the shape below and trusts that triple as the + * WHOLE of what a Price charges. A Price billed every three months, on + * metered usage, dividing the quantity before charging, or pricing in tiers + * would otherwise pass that triple on `interval => 'month'` alone and be + * adopted as if it charged our figure per unit per month — which is adoption + * moving money, the one thing nothing here may do. + * + * Not a list of everything Stripe can put on a Price: it is the list of + * fields known to change what one CHARGES. A fifth would have to be added + * here, at the boundary, before the shape below is handed on. + * + * @return array}> + */ + public function activePricesFor(string $productId): array; + + /** + * Write metadata onto a Price that already exists. + * + * Metadata is one of the few fields a Stripe Price lets you change — the + * same property activatePrice() rests on, and the reason the metadata format + * is NOT part of a Price's identity. An adopted orphan is brought up to + * today's metadata rather than replaced, which is exactly what was done by + * hand after 2026-07-29 and keeps Stripe's own dashboard readable. + */ + public function updatePriceMetadata(string $priceId, array $metadata): void; + /** Stop a Price being offered. The Price itself stays, as Stripe requires. */ public function archivePrice(string $priceId): void; diff --git a/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php b/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php new file mode 100644 index 0000000..359efba --- /dev/null +++ b/database/migrations/2026_07_31_210000_one_row_per_stripe_price.php @@ -0,0 +1,107 @@ +select('stripe_price_id') + ->groupBy('stripe_price_id') + ->havingRaw('count(*) > 1') + ->pluck('stripe_price_id'); + + foreach ($shared as $priceId) { + $rows = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->get(); + + // The lowest id stays — the row that was written first, which is the + // one anything already billing is likeliest to have been reading. + $keep = (int) $rows->min('id'); + $kept = $rows->first(fn ($row) => (int) $row->id === $keep); + $discarded = $rows->reject(fn ($row) => (int) $row->id === $keep); + + // The only forensic trace this delete ever leaves — see the class + // docblock for why that has to be enough. + Log::warning('stripe: kept one row of several claiming one price, deleted the rest', [ + 'stripe_price_id' => $priceId, + 'kept' => [ + 'id' => $kept->id, + 'addon_key' => $kept->addon_key, + 'reverse_charge' => (bool) $kept->reverse_charge, + 'amount_cents' => $kept->amount_cents, + 'currency' => $kept->currency, + 'interval' => $kept->interval, + ], + 'deleted' => $discarded->map(fn ($row) => [ + 'id' => $row->id, + 'addon_key' => $row->addon_key, + 'reverse_charge' => (bool) $row->reverse_charge, + 'amount_cents' => $row->amount_cents, + 'currency' => $row->currency, + 'interval' => $row->interval, + ])->values()->all(), + ]); + + DB::table('stripe_addon_prices') + ->where('stripe_price_id', $priceId) + ->where('id', '!=', $keep) + ->delete(); + } + + Schema::table('stripe_addon_prices', function (Blueprint $table) { + $table->unique('stripe_price_id', 'stripe_addon_prices_price_unique'); + }); + } + + public function down(): void + { + Schema::table('stripe_addon_prices', function (Blueprint $table) { + $table->dropUnique('stripe_addon_prices_price_unique'); + }); + } +}; diff --git a/docs/handoffs/2026-07-30-real-run-handoff.md b/docs/handoffs/2026-07-30-real-run-handoff.md index 8f51e21..3364113 100644 --- a/docs/handoffs/2026-07-30-real-run-handoff.md +++ b/docs/handoffs/2026-07-30-real-run-handoff.md @@ -172,8 +172,11 @@ Neuer Schritt zwischen `SecureHostFirewall` und `CompleteHostOnboarding`: - `VerifyVmTemplate` prüft nur **Existenz**, nicht `template: 1`. - `applyFirewall()` setzt **nicht** `firewall=1` an `net0` der VM — ohne das greifen die Gastregeln trotz Datacenter-Firewall nicht. Prüfen, ob die Vorlage es mitbringt. -- `stripe_addon_prices.stripe_price_id` ist **nicht** eindeutig (Plan-Seite schon): - zwei Zeilen könnten einen Preis teilen, Archivieren würde den anderen mitentziehen. +- ~~`stripe_addon_prices.stripe_price_id` ist **nicht** eindeutig (Plan-Seite schon): + zwei Zeilen könnten einen Preis teilen, Archivieren würde den anderen mitentziehen.~~ + **Erledigt** — eindeutig seit `2026_07_31_210000_one_row_per_stripe_price`; zugleich + das Netz unter dem Wiedererkennungsschritt, siehe + `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md`. - `clupilot:end-due-services` schließt den **Vertrag** nicht — ein geschenkter Vertrag bleibt nach Ende der Instanz für immer `active` und zählt in Umsatz/Dashboard mit. - Eine Abbuchung, die **vor** dem Widerruf entstand und **danach** bezahlt wird, diff --git a/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md b/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md new file mode 100644 index 0000000..6334685 --- /dev/null +++ b/docs/superpowers/plans/2026-07-30-stripe-price-adoption.md @@ -0,0 +1,1883 @@ +# Stripe Price Adoption — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Ein Abgleich, der zwischen Stripes Anlage und unserem Schreiben abbricht, erkennt den vorhandenen Preis beim nächsten Lauf wieder statt einen zweiten anzulegen — und eine Änderung an den Metadaten von `createPrice` blockiert Abgleich und Kundenbuchung nie wieder mit HTTP 400. + +**Architecture:** Zwei neue Bauteile. `App\Services\Stripe\IdempotencyKey` faltet einen Fingerabdruck der tatsächlich gesendeten Parameter in den Schlüssel — angewandt **nur** auf `createPrice`/`createProduct`, nie auf Geldbewegungen. `App\Services\Billing\AdoptStripePrice` fragt Stripe vor dem Anlegen nach den aktiven Preisen des Produkts und übernimmt einen beweisbar eigenen. Beide Preis-Dienste (`AddonPrices`, `PlanPrices`) bekommen den Wiedererkennungsschritt zwischen Tabellen-Blick und `createPrice`. Dazu ein eindeutiger Index auf `stripe_addon_prices.stripe_price_id` als Netz darunter. + +**Tech Stack:** Laravel 11, Pest, Livewire (nicht berührt), Stripe REST über `Illuminate\Http\Client`, MariaDB in Produktion / SQLite `:memory:` in Tests. + +**Spec:** `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md` + +## Global Constraints + +- **Commit-Disziplin (Nutzervorgabe, nicht verhandelbar):** Eine zweite Session arbeitet im selben Git-Index. **Immer** `git add -- ` und `git commit -F - -- `. **Nie** `git add -A`, `git add .`, `git commit -a`. +- **Sprache:** Code, Kommentare, Log-Meldungen und Commit-Botschaften **englisch**. Spec/Plan/Handoff-Dokumente **deutsch**. Kommentare erklären das *Warum*, nicht das *Was* — das ist die Hausschrift dieses Repos, siehe die vorhandenen Klassenkommentare in `app/Services/Billing/`. +- **Bestehende Verträge behalten ihre alten Preise.** Neue Preise gelten nur für neu abschließende Kunden. Keine Änderung dieses Plans darf einen laufenden Vertrag auf einen anderen Betrag bewegen. +- **Jede Buchung friert ihren Preis ein.** Ein später gebuchtes Modul kommt zum heutigen Preis; eine Stornierung zieht den damaligen Betrag ab. +- **Der Fingerabdruck ist ausschließlich für `createPrice` und `createProduct`.** Bei `refund`, `cancelSubscription`, `addSubscriptionItem` und `createCheckoutSession` muss Stripes HTTP 400 bei geänderten Parametern erhalten bleiben — dort ist ein Duplikat doppelt abgebuchtes Kundengeld. +- **`TaxTreatment` bleibt die einzige Steuerinstanz.** `automatic_tax` bleibt aus. +- **Kein Testlauf mit `--filter` als Abschluss:** jede Task endet mit dem vollen Billing-Ordner (`php artisan test --testsuite=Feature tests/Feature/Billing`), bevor committet wird. +- **Zahlen für Tests, aus dem echten Katalog:** `priority_support` = `2900` Netto (`config/provisioning.php:308`), Steuersatz 20 % → `3480` verrechnet monatlich, `34800` Netto/`41760` verrechnet jährlich. `3480 EUR monatlich` ist genau der Preis aus dem Vorfall (`price_1TygdEC7u8NpJ8pOt3nsoyYw`). + +--- + +## Korrekturen + +**Dieses Dokument ist nicht geprüft, sondern korrigiert.** Die Reviews der +einzelnen Tasks haben **fünf Defekte im Plan selbst** gefunden — nicht in der +Umsetzung. Sie stehen hier, damit niemand die Anweisungen weiter unten für +verifiziert hält. Der vollständige Verlauf, Task für Task, liegt in +`.superpowers/sdd/2026-07-30-stripe-price-adoption/progress.md`. + +1. **Ein zerstörerischer Befehl.** Task 6, Step 5 verlangte + `php artisan migrate:fresh --env=testing`. Es gibt in diesem Repo keine + `.env.testing`, also hätte der Befehl die echte, von laufenden Containern + benutzte Entwicklungsdatenbank neu gebaut. Der Umsetzer hat den Schritt + verweigert — richtig — und den Index über den SQLite-Migrationslauf der Suite + belegt. Der Schritt steht unten durchgestrichen mit Warnung, statt gelöscht. +2. **Eine Begründung, deren Mechanismus nicht feuern kann.** Der + Migrations-Kommentar im vorgeschriebenen Codeblock von Task 6 sagt, die + gelöschte Zeile baue sich wieder auf, indem `ensure()` den Preis „through the + adoption step" bei Stripe findet. Genau das kann nicht passieren: nach dem + Entdoppeln beansprucht die überlebende Zeile diese Preis-ID, also lehnt + `AdoptStripePrice` sie über die `claimed`-Prüfung immer ab. Der Wiederaufbau + ist echt, läuft aber über `createPrice()` unter dem tupel-eigenen Schlüssel. + Die ausgelieferte Migration ist berichtigt; der Plantext schreibt die falsche + Fassung weiter vor. Die Commit-Botschaft in Step 8 sagt nur „the row rebuilds + itself on the next ensure()" und nennt den Mechanismus nicht — das ist wahr, + aber es ist nicht die Begründung, die ausgeliefert wurde. +3. **Ein tautologischer Test.** Der einzige Test, der in Task 4 die + Auftraggeber-Regel „jede Buchung friert ihren Preis ein" bewachte, prüfte + `subscription_addons.stripe_price_id` nach einem `stripe:sync-catalogue`-Lauf — + eine Spalte, die dieses Kommando nie schreibt. Er war grün, bevor + `AddonPrices.php` überhaupt angefasst war. +4. **Zwei Plan-Tests, die mit entfernter Übernahme bestanden.** Beide neuen Tests + aus Task 5 liefen grün, nachdem der `adopt()`-Aufruf entfernt war. Ursache: + sie erzeugten ihre Waise über einen echten Sync-Lauf, sodass der + Schlüssel-Ledger des Fakes dieselbe Preis-ID zurückspielte — also genau der + Zustand (Schlüssel noch in Kraft), in dem es nichts zu beweisen gibt. Der + Vorfall vom 29.07. konnte erst **nach** dem Ablauf des Schlüssels entstehen. +5. **Drei falsche Testzahlen.** Die „Expected"-Zeilen von Task 1, 2 und 3 nennen + sieben, zehn und acht Tests; die dort vorgeschriebenen Dateien enthalten sechs, + neun und sieben. + +Die Abschnitte darunter sind **absichtlich nicht umgeschrieben** — die +vorgeschriebenen Codeblöcke bleiben stehen, wie sie umgesetzt wurden, damit Plan +und Verlauf vergleichbar bleiben. Diese Liste ist die Korrektur. + +--- + +## Dateistruktur + +**Neu** + +| Datei | Verantwortung | +|---|---| +| `app/Services/Stripe/IdempotencyKey.php` | Bildet Header-Werte: sprechender Schlüssel + Fingerabdruck der Aufrufparameter. Die **einzige** Stelle, an der ein Fingerabdruck entsteht — beide Clients müssen denselben Wert bilden. | +| `app/Services/Billing/AdoptStripePrice.php` | Der Wiedererkennungsschritt und seine fünf Bedingungen. Weiß nichts über Module oder Pakete; bekommt Produkt, Betrag, Metadaten und eine Frage „ist diese ID schon beansprucht?" gestellt. | +| `database/migrations/2026_07_31_210000_one_row_per_stripe_price.php` | Entdoppelt `stripe_addon_prices` und macht `stripe_price_id` eindeutig. | +| `tests/Feature/Billing/StripeIdempotencyKeyTest.php` | Was auf der Leitung landet (`Http::fake` gegen `HttpStripeClient`) und dass der Fake Stripe nachbildet. | +| `tests/Feature/Billing/StripePriceAdoptionTest.php` | Die Übernahmeregeln, beide Seiten, plus der eindeutige Index und die eingefrorene Buchung. | + +**Geändert** + +| Datei | Änderung | +|---|---| +| `app/Services/Stripe/StripeClient.php` | Zwei Methoden am Vertrag: `activePricesFor()`, `updatePriceMetadata()`. | +| `app/Services/Stripe/HttpStripeClient.php` | Beide Methoden umgesetzt; Fingerabdruck an `createPrice`/`createProduct`. | +| `app/Services/Stripe/FakeStripeClient.php` | Beide Methoden; `created` an den Preisen; `plantPrice()` für Tests; Schlüssel-Ledger vergleicht Parameter. | +| `app/Services/Billing/AddonPrices.php` | Wiedererkennung vor `createPrice`; Schlüssel-Kommentar berichtigt. | +| `app/Services/Billing/PlanPrices.php` | Dasselbe. | +| `docs/handoffs/2026-07-30-real-run-handoff.md` | Block-D-Punkt zur Eindeutigkeit als erledigt markieren. | + +**Reihenfolge und Abhängigkeit:** Task 1 (Schlüssel) und Task 2 (Client-Methoden) sind unabhängig voneinander. Task 3 braucht Task 2. Task 4 und 5 brauchen Task 3. Task 6 ist unabhängig, kommt aber zuletzt, weil ihr Test die Übernahme aus Task 4 mitprüft. + +--- + +### Task 1: Der Idempotenz-Schlüssel trägt, was gesendet wird + +**Files:** +- Create: `app/Services/Stripe/IdempotencyKey.php` +- Modify: `app/Services/Stripe/HttpStripeClient.php:87-121` (`createProduct`, `createPrice`) +- Modify: `app/Services/Stripe/FakeStripeClient.php:37-38` (Ledger-Eigenschaft), `:151-193` (`createProduct`, `createPrice`), `:313`, `:337-350` (`cancelSubscription`, `refund`) +- Test: `tests/Feature/Billing/StripeIdempotencyKeyTest.php` + +**Interfaces:** +- Consumes: nichts. +- Produces: + - `IdempotencyKey::forPrice(?string $key, string $productId, int $amountCents, string $currency, string $interval, array $metadata): ?string` + - `IdempotencyKey::forProduct(?string $key, string $name, array $metadata): ?string` + - `IdempotencyKey::fingerprint(array $parameters): string` — acht Hex-Zeichen + - `FakeStripeClient::$keys` wechselt von `array` zu `array`. **Kein Test liest die Eigenschaft** (geprüft), nur der Fake selbst an vier Stellen. + +- [ ] **Step 1: Write the failing test** + +`tests/Feature/Billing/StripeIdempotencyKeyTest.php`: + +```php +set('services.stripe.secret', 'sk_test_plan_task_one'); +}); + +it('sends a different key once the metadata changes', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + $spoken = 'clupilot-addon-price-priority_support-month-3480-EUR'; + + // The call as it stood before 9da1358, and the call after it: same money, + // same interval, one metadata field more. + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], $spoken); + + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], $spoken); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[0])->toStartWith($spoken) + ->and($sent[1])->toStartWith($spoken) + ->and($sent[1])->not->toBe($sent[0]); +}); + +it('sends the same key for the very same call', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + + foreach ([1, 2] as $ignored) { + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + } + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->unique() + ->all(); + + expect($sent)->toHaveCount(1); +}); + +it('fingerprints the product call too, where the same trap was waiting', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'prod_x'])]); + + $client = new HttpStripeClient; + + $client->createProduct('Priority Support', ['addon' => 'priority_support'], 'clupilot-addon-product-x'); + $client->createProduct('Priority Support', ['addon' => 'priority_support', 'sold_as' => 'entitlement'], 'clupilot-addon-product-x'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[1])->not->toBe($sent[0]); +}); + +it('leaves the money calls their bare key, so Stripe still refuses a changed one', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'x'])]); + + $client = new HttpStripeClient; + + $client->refund('pi_1', 500, 'clupilot-refund-7'); + $client->cancelSubscription('sub_1', 'at_period_end', 'clupilot-cancel-7'); + $client->addSubscriptionItem('sub_1', 'price_1', 1, 'none', 'clupilot-item-7'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent)->toBe(['clupilot-refund-7', 'clupilot-cancel-7', 'clupilot-item-7']); +}); + +it('reproduces the refusal Stripe makes, which the fake used to swallow', function () { + $fake = new FakeStripeClient; + + $fake->refund('pi_1', 500, 'clupilot-refund-7'); + + // Same key, different amount. Stripe answers 400; the fake said nothing and + // replayed the first refund's id, which is how a test could pass over the + // very failure that stopped production. + expect(fn () => $fake->refund('pi_1', 900, 'clupilot-refund-7')) + ->toThrow(RuntimeException::class, 'same parameters'); +}); + +it('mints a second price rather than blocking when the metadata moved', function () { + $fake = new FakeStripeClient; + + $first = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + + $second = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], 'clupilot-addon-price'); + + // Two objects, no exception. That the second one is not WANTED is the job of + // AdoptStripePrice, not of the key — see StripePriceAdoptionTest. + expect($second)->not->toBe($first); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: FAIL — `Class "App\Services\Stripe\IdempotencyKey" not found` erscheint noch nicht, weil der Test die Klasse nicht direkt anfasst; stattdessen schlagen die Erwartungen fehl (`$sent[1]` **ist** gleich `$sent[0]`), und der Refund-Test schlägt fehl, weil der Fake nicht wirft. + +- [ ] **Step 3: Write `IdempotencyKey`** + +`app/Services/Stripe/IdempotencyKey.php`: + +```php + $productId, + 'unit_amount' => $amountCents, + 'currency' => strtolower($currency), + 'interval' => $interval, + 'metadata' => $metadata, + ]); + } + + /** The key for a Product, for the same reason and against the same trap. */ + public static function forProduct(?string $key, string $name, array $metadata): ?string + { + return self::with($key, ['name' => $name, 'metadata' => $metadata]); + } + + /** + * Eight hex characters over the parameters, canonically ordered. + * + * Ordered, because PHP keeps insertion order and two callers writing the + * same metadata in a different order would otherwise send two keys for one + * call — which would mint two Prices and be a worse bug than the one this + * closes. + */ + public static function fingerprint(array $parameters): string + { + return substr(sha1(self::canonical($parameters)), 0, 8); + } + + private static function with(?string $key, array $parameters): ?string + { + // Null stays null: a caller who sends no key wants no replay, and + // inventing one here would change what the call means. + return $key === null ? null : $key.'-'.self::fingerprint($parameters); + } + + private static function canonical(array $parameters): string + { + ksort($parameters); + + foreach ($parameters as $name => $value) { + $parameters[$name] = is_array($value) ? self::canonical($value) : (string) $value; + } + + return json_encode($parameters, JSON_THROW_ON_ERROR); + } +} +``` + +- [ ] **Step 4: Wire it into `HttpStripeClient`** + +`app/Services/Stripe/HttpStripeClient.php` — `createProduct` (ab `:87`): + +```php + public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string + { + return (string) $this->request(IdempotencyKey::forProduct($idempotencyKey, $name, $metadata)) + ->asForm() + ->post($this->url('products'), array_filter([ + 'name' => $name, + ...$this->flatten('metadata', $metadata), + ])) + ->throw() + ->json('id'); + } +``` + +`createPrice` (ab `:99`) — nur die erste Zeile des `return` ändert sich: + +```php + return (string) $this->request(IdempotencyKey::forPrice( + $idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata, + )) + ->asForm() + ->post($this->url('prices'), [ +``` + +Und den Kommentar in `request()` (`:344-351`) ergänzen — er behauptet heute, der Schlüssel schütze einen Retry, und verschweigt, dass er einen geänderten Aufruf abwürgt: + +```php + // Stripe replays the original response for a repeated key instead of + // creating a second object — but ONLY for a call repeated exactly; a + // changed parameter under a used key is HTTP 400 for twenty-four hours. + // The catalogue calls therefore fold a fingerprint of their parameters + // into the key (see IdempotencyKey); the money calls deliberately do + // not. Their keys expire after 24 hours, so this protects a retry, not + // a sync re-run next week — for which the stored ids and + // AdoptStripePrice are the guard. +``` + +`use App\Services\Stripe\IdempotencyKey;` ist **nicht** nötig — dieselbe Namespace. + +- [ ] **Step 5: Make the fake faithful** + +`app/Services/Stripe/FakeStripeClient.php` — Eigenschaft ersetzen (`:37-38`): + +```php + /** + * Idempotency key → what was first answered for it, and a fingerprint of + * what it was first sent with. + * + * The fingerprint is the half that was missing. This ledger replayed the + * first id for a repeated key without ever looking at what the second call + * was asking for — so it was unrealistic in precisely the way that mattered, + * and no test could see the 2026-07-29 blockade. + * + * @var array + */ + public array $keys = []; +``` + +Zwei private Helfer am Ende der Klasse, neben `failIfAsked()`: + +```php + /** + * The id Stripe already answered for this key, or null when it is new. + * + * Throws on a key that comes back with different parameters, which is what + * Stripe does — with this wording — and what makes a test able to see the + * failure at all. + */ + private function replay(?string $key, array $parameters): ?string + { + if ($key === null || ! isset($this->keys[$key])) { + return null; + } + + if ($this->keys[$key]['fingerprint'] !== IdempotencyKey::fingerprint($parameters)) { + throw new RuntimeException( + 'Keys for idempotent requests can only be used with the same parameters they were first used with.', + ); + } + + return $this->keys[$key]['id']; + } + + private function rememberKey(?string $key, string $id, array $parameters): void + { + if ($key !== null) { + $this->keys[$key] = ['id' => $id, 'fingerprint' => IdempotencyKey::fingerprint($parameters)]; + } + } +``` + +`createProduct` (`:151-166`) — Schlüssel **mit Fingerabdruck**, genau wie der HTTP-Client, sonst prüft der Test etwas anderes als die Produktion sendet: + +```php + public function createProduct(string $name, array $metadata = [], ?string $idempotencyKey = null): string + { + $key = IdempotencyKey::forProduct($idempotencyKey, $name, $metadata); + $parameters = ['name' => $name, 'metadata' => $metadata]; + + // Replays the first answer for a repeated key, as Stripe does. + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; + } + + $id = 'prod_'.substr(sha1($name.count($this->products)), 0, 12); + $this->products[$id] = ['name' => $name, 'metadata' => $metadata]; + + $this->rememberKey($key, $id, $parameters); + + return $id; + } +``` + +`createPrice` (`:168-194`) — dasselbe Muster, plus `created` am gespeicherten Preis (Task 3 sortiert danach): + +```php + public function createPrice( + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata = [], + ?string $idempotencyKey = null, + ): string { + $key = IdempotencyKey::forPrice($idempotencyKey, $productId, $amountCents, $currency, $interval, $metadata); + $parameters = [ + 'product' => $productId, + 'unit_amount' => $amountCents, + 'currency' => strtolower($currency), + 'interval' => $interval, + 'metadata' => $metadata, + ]; + + $replayed = $this->replay($key, $parameters); + + if ($replayed !== null) { + return $replayed; + } + + $id = 'price_'.substr(sha1($productId.$amountCents.$interval.count($this->prices)), 0, 12); + $this->prices[$id] = [ + 'product' => $productId, + 'amount' => $amountCents, + 'currency' => $currency, + 'interval' => $interval, + 'metadata' => $metadata, + // Stripe stamps every object with its creation time and + // AdoptStripePrice takes the OLDEST of several orphans. A counter + // is enough and beats a clock: it cannot tie. + 'created' => count($this->prices) + 1, + ]; + + $this->rememberKey($key, $id, $parameters); + + return $id; + } +``` + +Das `@var` der Eigenschaft `$prices` (`:22`) um `created: int` ergänzen. + +`cancelSubscription` (`:313`, `:324`) und `refund` (`:337`, `:350`) auf `replay()`/`rememberKey()` umstellen — **ohne** `IdempotencyKey`, mit dem rohen Schlüssel: + +```php + // No fingerprint: for a refund, a changed parameter under a used key is + // exactly what must fail loudly. See IdempotencyKey. + $parameters = ['payment' => $paymentReference, 'amount' => (string) $amountCents]; + $replayed = $this->replay($idempotencyKey, $parameters); + + if ($replayed !== null) { + return $replayed; + } +``` + +und nach dem Anlegen `$this->rememberKey($idempotencyKey, $id, $parameters);`. Für `cancelSubscription` sind die Parameter `['subscription' => $subscriptionId, 'when' => $when]`, die gemerkte „id" bleibt `$subscriptionId` wie heute. + +- [ ] **Step 6: Run the test to verify it passes** + +```bash +php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: PASS, sieben Tests. + +- [ ] **Step 7: Run the whole billing folder — the fake is used by a dozen files** + +```bash +php artisan test tests/Feature/Billing +``` + +Expected: PASS. Wenn hier etwas fällt, ist es ein Test, der einen Schlüssel zweimal mit verschiedenen Parametern schickte und das bisher nicht merkte — **das ist ein echter Fund**, nicht ein zu unterdrückender Testfehler: den Aufrufer korrigieren, nicht den Fake. + +- [ ] **Step 8: Commit** + +```bash +git add -- app/Services/Stripe/IdempotencyKey.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +```bash +git commit -F - -- app/Services/Stripe/IdempotencyKey.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php <<'MSG' +Put in the key everything the call actually sends + +A key says "I already sent this call", not "this is what the object is". The +catalogue calls sent metadata that the key knew nothing about, so adding the +tax_treatment field in 9da1358 poisoned yesterday's key for a day — and +AddonPrices::ensure() runs inside a customer's module booking, not only in the +sweep. + +createPrice and createProduct now fold a fingerprint of their parameters into +the key. refund, cancelSubscription, addSubscriptionItem and the checkout +deliberately do not: there a second object is the customer's money taken twice, +and Stripe's refusal is the thing worth keeping. + +The fake could not see any of this. Its ledger replayed the first id for a +repeated key without ever comparing what the second call asked for. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 2: Stripe fragen, was es schon hat + +**Files:** +- Modify: `app/Services/Stripe/StripeClient.php:118-140` (zwischen `createPrice` und `archivePrice`) +- Modify: `app/Services/Stripe/HttpStripeClient.php` (nach `activatePrice`, `:143`) +- Modify: `app/Services/Stripe/FakeStripeClient.php` (nach `activatePrice`, `:201-212`) +- Test: `tests/Feature/Billing/StripeIdempotencyKeyTest.php` (zwei Tests angehängt — dieselbe Datei, weil es wieder um das geht, was auf der Leitung landet) + +**Interfaces:** +- Consumes: nichts aus Task 1. +- Produces: + - `StripeClient::activePricesFor(string $productId): array` — Liste von + `array{id: string, unit_amount: int, currency: string, interval: string, created: int, metadata: array}`. + **`currency` kommt GROSSGESCHRIEBEN zurück** (Stripe liefert klein; unsere Tabellen halten groß), `interval` ist `month`/`year`. + - `StripeClient::updatePriceMetadata(string $priceId, array $metadata): void` + - `FakeStripeClient::plantPrice(string $id, string $productId, int $amountCents, string $currency, string $interval, array $metadata, int $created = 1): void` + - `FakeStripeClient::$metadataUpdates` — `array}>` + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripeIdempotencyKeyTest.php` anhängen: + +```php +it('pages through every active price of a product', function () { + Http::fake([ + 'api.stripe.com/*' => Http::sequence() + ->push([ + 'data' => [ + ['id' => 'price_a', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_b', 'unit_amount' => 41760, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'year'], 'metadata' => []], + ], + 'has_more' => true, + ]) + ->push([ + 'data' => [ + ['id' => 'price_c', 'unit_amount' => 2900, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month'], + 'metadata' => ['addon' => 'priority_support', 'tax_treatment' => 'reverse_charge']], + ], + 'has_more' => false, + ]), + ]); + + $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + + expect($prices)->toHaveCount(3) + ->and($prices[0])->toBe([ + 'id' => 'price_a', + 'unit_amount' => 3480, + // Upper case, because that is how our own tables hold it and the + // comparison in AdoptStripePrice must not have to remember which + // side is which. + 'currency' => 'EUR', + 'interval' => 'month', + 'created' => 100, + 'metadata' => ['addon' => 'priority_support'], + ]) + ->and($prices[2]['id'])->toBe('price_c'); + + // The second page has to be asked for, or this reintroduces the very gap it + // exists to close — a family product accumulates prices across versions, + // terms, treatments and every rate change. + Http::assertSent(fn ($request) => str_contains($request->url(), 'starting_after=price_b')); + + // Archived prices are none of our business here: we are looking for + // something to SELL on. + Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true')); +}); + +it('writes metadata onto a price that already exists', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_a'])]); + + (new HttpStripeClient)->updatePriceMetadata('price_a', ['addon' => 'priority_support']); + + Http::assertSent(fn ($request) => $request->url() === 'https://api.stripe.com/v1/prices/price_a' + && $request['metadata[addon]'] === 'priority_support'); +}); + +it('lets the fake answer with the prices it holds, minus the archived ones', function () { + $fake = new FakeStripeClient; + + $kept = $fake->createPrice('prod_1', 3480, 'EUR', 'month', ['addon' => 'priority_support']); + $gone = $fake->createPrice('prod_1', 2900, 'EUR', 'month', ['addon' => 'priority_support']); + $other = $fake->createPrice('prod_2', 3480, 'EUR', 'month', []); + $fake->archivePrice($gone); + + $fake->plantPrice('price_orphan', 'prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 0); + + $found = collect($fake->activePricesFor('prod_1'))->pluck('id')->all(); + + expect($found)->toContain($kept, 'price_orphan') + ->and($found)->not->toContain($gone, $other); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: FAIL — `Call to undefined method App\Services\Stripe\HttpStripeClient::activePricesFor()`. + +- [ ] **Step 3: Add both methods to the contract** + +`app/Services/Stripe/StripeClient.php`, zwischen `createPrice` und `archivePrice`: + +```php + /** + * Every active recurring Price of a Product, as Stripe holds them. + * + * The half of the catalogue mirror that was missing: nothing here ever asked + * Stripe what it already had. A run that created a Price and died before the + * row was written left an orphan our table never learned about, and once the + * idempotency key expired the next run made a SECOND live Price for the same + * money — the exact duplicate the key exists to prevent. See + * App\Services\Billing\AdoptStripePrice, which is the only caller. + * + * Active ones only, because the question being asked is "is there something + * here I can sell on?". `currency` comes back UPPER CASE, the way our own + * tables hold it. + * + * @return array}> + */ + public function activePricesFor(string $productId): array; + + /** + * Write metadata onto a Price that already exists. + * + * Metadata is one of the few fields a Stripe Price lets you change — the + * same property activatePrice() rests on, and the reason the metadata format + * is NOT part of a Price's identity. An adopted orphan is brought up to + * today's metadata rather than replaced, which is exactly what was done by + * hand after 2026-07-29 and keeps Stripe's own dashboard readable. + */ + public function updatePriceMetadata(string $priceId, array $metadata): void; +``` + +- [ ] **Step 4: Implement in `HttpStripeClient`** (nach `activatePrice`) + +```php + public function activePricesFor(string $productId): array + { + $prices = []; + $after = null; + + // Paged through to the end, for the same reason invoiceLines() is: Stripe + // caps a page at a hundred, and a family Product collects a Price per + // version, term, treatment and rate change. Stopping at the first page + // would leave an orphan unfound and mint the duplicate anyway. + do { + $page = $this->request() + ->get($this->url('prices'), array_filter([ + 'product' => $productId, + 'active' => 'true', + 'type' => 'recurring', + 'limit' => 100, + 'starting_after' => $after, + ], fn ($value) => $value !== null)) + ->throw() + ->json(); + + $data = (array) ($page['data'] ?? []); + + foreach ($data as $price) { + $prices[] = [ + 'id' => (string) ($price['id'] ?? ''), + 'unit_amount' => (int) ($price['unit_amount'] ?? 0), + 'currency' => strtoupper((string) ($price['currency'] ?? '')), + 'interval' => (string) ($price['recurring']['interval'] ?? ''), + 'created' => (int) ($price['created'] ?? 0), + 'metadata' => array_map( + fn ($value) => (string) $value, + (array) ($price['metadata'] ?? []), + ), + ]; + } + + $after = $data === [] ? null : ($data[array_key_last($data)]['id'] ?? null); + } while (($page['has_more'] ?? false) === true && $after !== null); + + return $prices; + } + + public function updatePriceMetadata(string $priceId, array $metadata): void + { + $this->request() + ->asForm() + ->post($this->url('prices/'.$priceId), $this->flatten('metadata', $metadata)) + ->throw(); + } +``` + +- [ ] **Step 5: Implement in `FakeStripeClient`** (nach `activatePrice`) + +```php + public function activePricesFor(string $productId): array + { + // No failIfAsked(): createPrice(), the call this one stands in front of, + // does not fail either, and a test that scripts an outage for a refund + // must not have its catalogue sync change behaviour underneath it. + $found = []; + + foreach ($this->prices as $id => $price) { + if ($price['product'] !== $productId || in_array($id, $this->archived, true)) { + continue; + } + + $found[] = [ + 'id' => $id, + 'unit_amount' => $price['amount'], + 'currency' => strtoupper($price['currency']), + 'interval' => $price['interval'], + 'created' => $price['created'], + 'metadata' => $price['metadata'], + ]; + } + + return $found; + } + + public function updatePriceMetadata(string $priceId, array $metadata): void + { + if (isset($this->prices[$priceId])) { + $this->prices[$priceId]['metadata'] = $metadata; + } + + $this->metadataUpdates[] = ['price' => $priceId, 'metadata' => $metadata]; + } + + /** + * A Price that exists at Stripe and in no table of ours — the orphan a run + * leaves behind when it dies between Stripe's create and our insert. + * + * Here rather than in a test file because it is the state the whole adoption + * step exists for, and every test that needs it would otherwise reach into + * $prices and write the shape by hand. + */ + public function plantPrice( + string $id, + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata = [], + int $created = 1, + ): void { + $this->prices[$id] = [ + 'product' => $productId, + 'amount' => $amountCents, + 'currency' => $currency, + 'interval' => $interval, + 'metadata' => $metadata, + 'created' => $created, + ]; + } +``` + +Eigenschaft dazu, neben `$activated`: + +```php + /** + * Every metadata write, in order, so a test can assert that an adopted + * orphan was brought up to today's metadata instead of being replaced. + * + * @var array}> + */ + public array $metadataUpdates = []; +``` + +- [ ] **Step 6: Run the tests** + +```bash +php artisan test tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +Expected: PASS, zehn Tests. + +- [ ] **Step 7: Run the billing folder** + +```bash +php artisan test tests/Feature/Billing +``` + +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add -- app/Services/Stripe/StripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php +``` + +```bash +git commit -F - -- app/Services/Stripe/StripeClient.php app/Services/Stripe/HttpStripeClient.php app/Services/Stripe/FakeStripeClient.php tests/Feature/Billing/StripeIdempotencyKeyTest.php <<'MSG' +Let the catalogue ask Stripe what it already has + +Nothing here could. The client could create, archive and unarchive a Price and +had no way to list one, so a run that died between Stripe's create and our +insert left an orphan our table never learned about — and once the key expired, +the next run made a second live Price for the same money. + +Paged to the end like invoiceLines(), because a family Product collects a Price +per version, term, treatment and rate change, and stopping at the first page +would leave the orphan unfound and mint the duplicate anyway. + +updatePriceMetadata() alongside it: metadata is one of the few fields a Price +lets you change, which is why the metadata format is not part of a Price's +identity and why an adopted orphan is brought up to date rather than replaced. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 3: `AdoptStripePrice` — die fünf Bedingungen + +**Files:** +- Create: `app/Services/Billing/AdoptStripePrice.php` +- Test: `tests/Feature/Billing/StripePriceAdoptionTest.php` + +**Interfaces:** +- Consumes: `StripeClient::activePricesFor()`, `updatePriceMetadata()`, `archivePrice()` aus Task 2. `FakeStripeClient::plantPrice()`, `$metadataUpdates`. +- Produces: + +```php +public function __invoke( + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata, + array $identifying, + callable $claimed, // fn (string $priceId): bool +): ?string +``` + + Gibt die übernommene Preis-ID zurück oder `null`, wenn nichts zu übernehmen ist. `$identifying` sind die Metadatenschlüssel, die einen Preis als unseren ausweisen (`['addon']` bzw. `['plan_price_id']`). + +- [ ] **Step 1: Write the failing test** + +`tests/Feature/Billing/StripePriceAdoptionTest.php`: + +```php +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); +}); + +/** The module metadata as AddonPrices sends it today. */ +function moduleMetadata(string $treatment = 'domestic'): array +{ + return ['addon' => 'priority_support', 'tax_treatment' => $treatment]; +} + +/** Ask the adoption step the question AddonPrices asks it. */ +function adoptModulePrice(?array $metadata = null, ?callable $claimed = null): ?string +{ + return app(AdoptStripePrice::class)( + productId: 'prod_support', + amountCents: 3480, + currency: 'EUR', + interval: 'month', + metadata: $metadata ?? moduleMetadata(), + identifying: ['addon'], + claimed: $claimed ?? fn (string $id) => false, + ); +} + +it('adopts the orphan of 2026-07-29 instead of minting a second price', function () { + // The state that morning: the Price exists at Stripe, carries the metadata + // of the code that made it — WITHOUT tax_treatment, which 9da1358 added + // afterwards — and no row of ours knows it. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + + expect(adoptModulePrice())->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw'); + + // Brought up to today's metadata rather than replaced: metadata is mutable + // at Stripe, the amount is not, which is the whole reason the format is no + // part of a Price's identity. + expect($this->stripe->metadataUpdates)->toBe([[ + 'price' => 'price_1TygdEC7u8NpJ8pOt3nsoyYw', + 'metadata' => moduleMetadata(), + ]]); +}); + +it('leaves the metadata alone when it already says the right thing', function () { + $this->stripe->plantPrice('price_ok', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBe('price_ok') + ->and($this->stripe->metadataUpdates)->toBe([]); +}); + +it('adopts nothing when the amount, currency or interval differ', function () { + $this->stripe->plantPrice('price_cheaper', 'prod_support', 2900, 'EUR', 'month', moduleMetadata()); + $this->stripe->plantPrice('price_yearly', 'prod_support', 3480, 'EUR', 'year', moduleMetadata()); + $this->stripe->plantPrice('price_dollars', 'prod_support', 3480, 'USD', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price nothing proves is ours, and says so', function () { + Log::spy(); + + // What a person clicking through Stripe's own dashboard leaves behind: the + // right money on our product, and not one word about what it is for. + $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); + + expect(adoptModulePrice())->toBeNull(); + + Log::shouldHaveReceived('warning')->once(); +}); + +it('passes silently over another of our own prices', function () { + Log::spy(); + + // At a VAT rate of nought both treatments are the same amount, so the + // reverse-charge Price sits at the domestic one's money — and contradicts on + // tax_treatment. That is not a mystery worth a warning; it is a Price of + // ours that is not the one being asked for. + $this->stripe->plantPrice('price_rc', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata('reverse_charge')); + + expect(adoptModulePrice())->toBeNull(); + + Log::shouldNotHaveReceived('warning'); +}); + +it('never hands out a price a row already claims', function () { + $this->stripe->plantPrice('price_taken', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice(claimed: fn (string $id) => $id === 'price_taken'))->toBeNull(); +}); + +it('adopts the oldest of several orphans and stops selling the rest', function () { + Log::spy(); + + $this->stripe->plantPrice('price_second', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 200); + $this->stripe->plantPrice('price_first', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 100); + $this->stripe->plantPrice('price_third', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 300); + + // The oldest, because it is the one a lost row is likeliest to have been + // billing on. + expect(adoptModulePrice())->toBe('price_first') + ->and($this->stripe->archived)->toBe(['price_second', 'price_third']); + + Log::shouldHaveReceived('warning'); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: FAIL — `Target class [App\Services\Billing\AdoptStripePrice] does not exist.` + +- [ ] **Step 3: Write the implementation** + +`app/Services/Billing/AdoptStripePrice.php`: + +```php + $metadata what the create call would send + * @param array $identifying metadata keys that mark a Price as ours + * @param callable(string): bool $claimed is this Price id already in our register? + */ + public function __invoke( + string $productId, + int $amountCents, + string $currency, + string $interval, + array $metadata, + array $identifying, + callable $claimed, + ): ?string { + $candidates = []; + + foreach ($this->stripe->activePricesFor($productId) as $price) { + if ($price['unit_amount'] !== $amountCents + || $price['currency'] !== strtoupper($currency) + || $price['interval'] !== $interval) { + continue; + } + + if ($claimed($price['id'])) { + continue; + } + + // Contradicts on something it carries: another Price of ours, not a + // mystery. Silent — at a VAT rate of nought the two treatments share + // an amount, and this would otherwise warn on every sweep. + if ($this->contradicts($price['metadata'], $metadata)) { + continue; + } + + if (! $this->confirms($price['metadata'], $metadata, $identifying)) { + Log::warning('stripe: left an unexplained active price alone rather than adopting it', [ + 'price' => $price['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + + continue; + } + + $candidates[] = $price; + } + + if ($candidates === []) { + return null; + } + + usort($candidates, fn (array $a, array $b) => $a['created'] <=> $b['created']); + + $adopted = array_shift($candidates); + + foreach ($candidates as $duplicate) { + $this->stripe->archivePrice($duplicate['id']); + + Log::warning('stripe: stopped selling a duplicate price for one figure', [ + 'price' => $duplicate['id'], + 'adopted' => $adopted['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + ]); + } + + // Only when it differs, so a sweep over a healthy catalogue makes no + // writes at Stripe at all. + if ($adopted['metadata'] !== $metadata) { + $this->stripe->updatePriceMetadata($adopted['id'], $metadata); + } + + Log::info('stripe: adopted an existing price instead of creating a second one', [ + 'price' => $adopted['id'], + 'product' => $productId, + 'amount_cents' => $amountCents, + 'currency' => strtoupper($currency), + 'interval' => $interval, + ]); + + return $adopted['id']; + } + + /** + * Does this Price say something about itself that we do not? + * + * @param array $found + * @param array $expected + */ + private function contradicts(array $found, array $expected): bool + { + foreach ($expected as $key => $value) { + if (array_key_exists($key, $found) && $found[$key] !== $value) { + return true; + } + } + + return false; + } + + /** + * Does it prove it is ours? + * + * Failing to contradict is not enough — an empty metadata bag contradicts + * nothing. At least one identifying key has to be there and agree. + * + * @param array $found + * @param array $expected + * @param array $identifying + */ + private function confirms(array $found, array $expected, array $identifying): bool + { + foreach ($identifying as $key) { + if (isset($found[$key]) && $found[$key] === ($expected[$key] ?? null)) { + return true; + } + } + + return false; + } +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: PASS, acht Tests. + +- [ ] **Step 5: Commit** + +```bash +git add -- app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Billing/AdoptStripePrice.php tests/Feature/Billing/StripePriceAdoptionTest.php <<'MSG' +Recognise the price Stripe already has + +An abandoned run leaves an orphan: Stripe made the Price, our insert never +happened, and the table that decides everything afterwards does not know it +exists. The key covers a day; after that the next run makes a second live Price +for the same money. + +What may be taken over is narrow. Same amount, currency and interval — a Price +at another figure would move money, and nothing here may: a running contract +keeps the Price it was sold on, and a booking stays frozen at what it cost that +day. Plus proof in the metadata, because an unexplained active Price at the +right money is what somebody clicking through Stripe's dashboard leaves behind, +and adopting that is worse than minting a second one. + +Several orphans: the oldest is adopted — likeliest to be the one a lost row was +billing on — and the rest are archived, which stops them being sold and moves +nobody. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 4: Die Modulseite benutzt den Wiedererkennungsschritt + +**Files:** +- Modify: `app/Services/Billing/AddonPrices.php:47` (Konstruktor), `:126-159` (der Anlege-Pfad in `ensure`) +- Test: `tests/Feature/Billing/StripePriceAdoptionTest.php` (angehängt) + +**Interfaces:** +- Consumes: `AdoptStripePrice::__invoke()` aus Task 3. +- Produces: `AddonPrices::__construct(StripeClient $stripe, AdoptStripePrice $adopt)`. **Geprüft: niemand konstruiert `AddonPrices` mit `new`** (weder App noch Tests), der Container löst auf. + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripePriceAdoptionTest.php` anhängen: + +```php +use App\Models\StripeAddonPrice; +use App\Models\Subscription; +use App\Models\SubscriptionAddon; +use App\Services\Billing\AddonPrices; +use App\Services\Billing\TaxTreatment; +use Illuminate\Contracts\Console\Kernel; + +it('takes the orphan over instead of minting a second module price', function () { + // The product exists because a previous run got that far; the Price exists + // because the run that made it died before the insert. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + + $before = count($this->stripe->prices); + + $id = app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect($id)->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw') + // Nothing new at Stripe: the orphan was taken over, not replaced. + ->and(count($this->stripe->prices))->toBe($before) + ->and(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('interval', 'month') + ->where('reverse_charge', false) + ->value('stripe_price_id'))->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw'); +}); + +it('does not block a booking because the metadata format moved', function () { + // 2026-07-29, exactly: orphan with the old metadata, code with the new. This + // is the call that answered HTTP 400 for twenty-four hours. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + + $id = app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect($id)->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw') + ->and($this->stripe->metadataUpdates)->toHaveCount(1); +}); + +it('leaves a frozen booking on the price it was sold at', function () { + app(Kernel::class)->call('stripe:sync-catalogue'); + + $sold = app(AddonPrices::class)->liveFor( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + $subscription = Subscription::factory()->plan('team')->create(); + $booking = SubscriptionAddon::query()->create([ + 'subscription_id' => $subscription->id, + 'addon_key' => 'priority_support', + // Net, per month, per unit — frozen at booking. `currency` and + // `booked_at` are both NOT NULL (2026_07_26_060000), and `uuid` fills + // itself through the model's uniqueIds(). + 'price_cents' => 2900, + 'currency' => 'EUR', + 'quantity' => 1, + 'booked_at' => now(), + 'stripe_price_id' => $sold, + ]); + + // The catalogue moves, and somebody has already left an orphan at the new + // figure. Neither may reach a booking that is already frozen. + config()->set('provisioning.addons.priority_support.price_cents', 3900); + $this->stripe->plantPrice('price_orphan_new_figure', 'prod_support', + 4680, 'EUR', 'month', ['addon' => 'priority_support']); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + expect($booking->refresh()->stripe_price_id)->toBe($sold) + ->and($this->stripe->archived)->not->toContain($sold); +}); +``` + +**Zum letzten Test:** `SubscriptionAddon::booted()` wirft bei jeder Änderung an `FROZEN` (`subscription_id`, `addon_key`, `price_cents`, `currency`, `quantity`, `booked_at`) — die Regel „eine Buchung friert ihren Preis ein" steht also schon im Modell. `stripe_price_id` gehört nicht dazu und ist damit änderbar; genau deshalb muss ein Test sagen, dass die Übernahme sie nicht anfasst. `Subscription::factory()->plan('team')` ist die Form aus `tests/Feature/Billing/ReverseChargePriceTest.php:220`. + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: FAIL — `ensure()` legt einen neuen Preis an, `count($this->stripe->prices)` ist um eins gewachsen, und die zurückgegebene ID ist nicht die der Waise. + +- [ ] **Step 3: Wire the adoption into `AddonPrices`** + +Konstruktor (`:47`): + +```php + public function __construct( + private readonly StripeClient $stripe, + private readonly AdoptStripePrice $adopt, + ) {} +``` + +Der Anlege-Pfad in `ensure()` (`:126-159`) wird zu: + +```php + $productId = $this->product($addonKey); + + $metadata = [ + // Read back when a Stripe invoice line has to be turned into + // wording a customer can read — see StripeInvoiceLines. + 'addon' => $addonKey, + // Which of the module's two Prices this is, for anyone reading + // Stripe's own dashboard, where they would otherwise differ only + // by an amount. + 'tax_treatment' => $reverseCharge ? 'reverse_charge' : 'domestic', + ]; + + // Asked BEFORE minting, because a run that died between Stripe's create + // and our insert left a Price no table of ours knows — and the key below + // stops protecting it after twenty-four hours. See AdoptStripePrice for + // what may be taken over and why so narrowly. + $priceId = ($this->adopt)( + productId: $productId, + amountCents: $amount, + currency: $currency, + interval: $interval, + metadata: $metadata, + identifying: ['addon'], + claimed: fn (string $id) => StripeAddonPrice::query() + ->where('stripe_price_id', $id) + ->exists(), + ); + + $priceId ??= $this->stripe->createPrice( + productId: $productId, + amountCents: $amount, + currency: $currency, + interval: $interval, + metadata: $metadata, + // Says "I have already sent this call", nothing more. The amount is + // the CHARGED one and the treatment is in there because at a rate of + // nought the two Prices are the same amount — but what stops a + // second Price for one figure is AdoptStripePrice above, not this: + // Stripe forgets a key after twenty-four hours. The metadata is + // folded in by IdempotencyKey inside the client, so changing the + // format below can never again refuse the call for a day. + idempotencyKey: "clupilot-addon-price-{$addonKey}-{$interval}-{$amount}-{$currency}" + .($reverseCharge ? '-rc' : ''), + ); + + $this->remember($addonKey, $reverseCharge, $amount, $netCents, $currency, $interval, $productId, $priceId); + $this->archiveSuperseded($addonKey, $reverseCharge, $netCents, $currency, $interval, $amount); + + return $priceId; +``` + +`use App\Services\Billing\AdoptStripePrice;` ist nicht nötig (gleiche Namespace). + +Den Kommentar in `remember()` (`:297-301`) um den zweiten Grund ergänzen, aus dem die Ausnahme kommen kann: + +```php + } catch (UniqueConstraintViolationException) { + // Two bookings of the same module landed together. Stripe replayed + // one Price for both — the idempotency key saw to that — so there is + // nothing to correct here beyond letting the first row stand. The + // same catch now also covers a Price id another row already claims, + // which the unique index refuses: the caller is handed the id + // regardless, because refusing would fail a customer's booking over + // a register that is one row short. + } +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: PASS, elf Tests. + +- [ ] **Step 5: Run the billing folder** + +```bash +php artisan test tests/Feature/Billing +``` + +Expected: PASS. Ein Fehler in `StripeAddonBillingTest` oder `AddonCancellationTest` bedeutet, dass der Wiedererkennungsschritt einen Preis übernimmt, wo ein Test einen neuen erwartet — dann prüfen, ob die Erwartung des Tests oder die Bedingung in `AdoptStripePrice` falsch ist, **bevor** irgendetwas geändert wird. + +- [ ] **Step 6: Commit** + +```bash +git add -- app/Services/Billing/AddonPrices.php tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Billing/AddonPrices.php tests/Feature/Billing/StripePriceAdoptionTest.php <<'MSG' +Ask before minting a module price + +The sweep that died on 2026-07-29 left price_1TygdEC7u8NpJ8pOt3nsoyYw at Stripe +and no row here. This is the step that finds it: same money, same interval, our +metadata on it, unclaimed — taken over and brought up to date instead of +duplicated. + +The comment on the key was the root of it. It read "keyed on what the Price IS", +and a key is not that: identity is product, amount, currency and interval, and +recognising those is AdoptStripePrice's job. Stripe forgets a key after a day; +it never was the guard against a duplicate. + +A booking stays where it was sold. Adoption only ever matches an identical +amount, so a frozen booking cannot be moved by it — there is a test that says so. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 5: Die Paketseite benutzt ihn ebenso + +**Files:** +- Modify: `app/Services/Billing/PlanPrices.php:43` (Konstruktor), `:130-160` (der Anlege-Zweig in `ensure`) +- Test: `tests/Feature/Billing/StripePriceAdoptionTest.php` (angehängt) + +**Interfaces:** +- Consumes: `AdoptStripePrice::__invoke()` aus Task 3. +- Produces: `PlanPrices::__construct(StripeClient $stripe, AdoptStripePrice $adopt)`. **Geprüft: niemand konstruiert `PlanPrices` mit `new`.** + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripePriceAdoptionTest.php` anhängen: + +```php +use App\Models\PlanPrice; +use App\Models\StripePlanPrice; +use App\Services\Billing\PlanPrices; + +it('takes over an orphaned package price', function () { + // A catalogue mirrored once, so families have Products and rows have Prices. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + + // The register loses its row and Stripe keeps the Price: a run that died + // between the two, seen from the next run's point of view. + $orphan = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->toBe($orphan) + ->and(count($this->stripe->prices))->toBe($before) + ->and(StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->where('charged_cents', $charged) + ->value('stripe_price_id'))->toBe($orphan) + // The pointer for the ordinary domestic sale is written as before. + ->and((string) $row->refresh()->stripe_price_id)->toBe($orphan); +}); + +it('does not take a package price belonging to another catalogue row', function () { + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $product = (string) $row->version->family->stripe_product_id; + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $interval = $row->term === Subscription::TERM_YEARLY ? 'year' : 'month'; + + StripePlanPrice::query()->where('plan_price_id', $row->id)->delete(); + + // Same product, same money, same interval — and plan_price_id says it is a + // DIFFERENT row's Price. One Product carries every version and term of a + // family, so this is the ordinary case, not an exotic one. + $this->stripe->plantPrice('price_other_row', $product, $charged, (string) $row->currency, + $interval, ['plan_price_id' => (string) ($row->id + 1000), 'tax_treatment' => 'domestic']); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->not->toBe('price_other_row'); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: FAIL im ersten der beiden — `ensure()` legt einen zweiten Preis an, `count($this->stripe->prices)` wächst. + +- [ ] **Step 3: Wire the adoption into `PlanPrices`** + +Konstruktor (`:43`): + +```php + public function __construct( + private readonly StripeClient $stripe, + private readonly AdoptStripePrice $adopt, + ) {} +``` + +Der Zweig `if ($priceId === null)` in `ensure()` (`:135-160`): + +```php + $interval = $price->term === Subscription::TERM_YEARLY ? 'year' : 'month'; + + if ($priceId === null) { + $metadata = [ + 'plan_family' => $family->key, + 'plan_version' => (string) $version->version, + 'plan_version_id' => (string) $version->id, + 'plan_price_id' => (string) $price->id, + // Read back by anything that has a Price id and needs to know + // what kind of sale it was — and by a person looking at + // Stripe's own dashboard, where two Prices on one Product + // would otherwise differ only by an amount. + 'tax_treatment' => $treatment->reverseCharge ? 'reverse_charge' : 'domestic', + ]; + + // Asked BEFORE minting: a run that died between Stripe's create and + // our insert left a Price the register does not know, and the key + // below stops protecting it after twenty-four hours. `plan_price_id` + // is what proves such a Price is this row's — one Product carries + // every version and term of a family, so the amount alone would not. + $priceId = ($this->adopt)( + productId: (string) $productId, + amountCents: $charged, + currency: (string) $price->currency, + interval: $interval, + metadata: $metadata, + identifying: ['plan_price_id'], + claimed: fn (string $id) => StripePlanPrice::query() + ->where('stripe_price_id', $id) + ->exists(), + ); + + $priceId ??= $this->stripe->createPrice( + productId: (string) $productId, + amountCents: $charged, + currency: (string) $price->currency, + interval: $interval, + metadata: $metadata, + // Says "I have already sent this call", nothing more — the + // metadata is folded in by IdempotencyKey inside the client, so + // changing the format above can never again refuse the call for + // a day. What stops a second Price for one figure is the + // adoption step above; Stripe forgets a key after 24 hours. + idempotencyKey: "clupilot-price-{$price->id}-{$charged}" + .($treatment->reverseCharge ? '-rc' : ''), + ); + } elseif ($existing?->archived_at !== null) { +``` + +Der bestehende `elseif`-Zweig und alles darunter bleiben unverändert. + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: PASS, dreizehn Tests. + +- [ ] **Step 5: Run the billing folder** + +```bash +php artisan test tests/Feature/Billing +``` + +Expected: PASS. `ReverseChargePriceTest` ist hier der wichtigste Zeuge: `:342-347` zählt genau 16 Register-Zeilen und 16 verschiedene Preis-IDs nach einem doppelten Sync-Lauf. Bleibt das grün, hat der Wiedererkennungsschritt nichts verdoppelt und nichts zusammengelegt. + +- [ ] **Step 6: Commit** + +```bash +git add -- app/Services/Billing/PlanPrices.php tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +```bash +git commit -F - -- app/Services/Billing/PlanPrices.php tests/Feature/Billing/StripePriceAdoptionTest.php <<'MSG' +Ask before minting a package price + +Same defect, same fix, one difference: a family Product carries every version +and every term, so the amount alone does not say which row a Price belongs to. +plan_price_id in the metadata does, and it is what has to agree before a Price +is taken over. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +### Task 6: Eine Zeile pro Stripe-Preis + +**Files:** +- Create: `database/migrations/2026_07_31_210000_one_row_per_stripe_price.php` +- Modify: `docs/handoffs/2026-07-30-real-run-handoff.md:175-176` +- Test: `tests/Feature/Billing/StripePriceAdoptionTest.php` (angehängt) + +**Interfaces:** +- Consumes: nichts. +- Produces: eindeutiger Index `stripe_addon_prices_price_unique` auf `stripe_addon_prices.stripe_price_id`. + +- [ ] **Step 1: Write the failing test** + +An `tests/Feature/Billing/StripePriceAdoptionTest.php` anhängen: + +```php +use Illuminate\Database\UniqueConstraintViolationException; + +it('lets no two module rows claim one stripe price', function () { + $shared = [ + 'addon_key' => 'priority_support', 'net_cents' => 2900, 'currency' => 'EUR', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared', + ]; + + StripeAddonPrice::query()->create([...$shared, + 'reverse_charge' => false, 'amount_cents' => 3480, 'interval' => 'month']); + + // Two rows on one Price is what Block D warned about: archiving the one + // would withdraw the Price the other is still selling. + expect(fn () => StripeAddonPrice::query()->create([...$shared, + 'reverse_charge' => true, 'amount_cents' => 2900, 'interval' => 'month'])) + ->toThrow(UniqueConstraintViolationException::class); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: FAIL — keine Ausnahme, die zweite Zeile entsteht. + +- [ ] **Step 3: Write the migration** + +`database/migrations/2026_07_31_210000_one_row_per_stripe_price.php`: + +```php +select('stripe_price_id') + ->groupBy('stripe_price_id') + ->havingRaw('count(*) > 1') + ->pluck('stripe_price_id'); + + foreach ($shared as $priceId) { + // The lowest id stays — the row that was written first, which is the + // one anything already billing is likeliest to have been reading. + $keep = DB::table('stripe_addon_prices')->where('stripe_price_id', $priceId)->min('id'); + + DB::table('stripe_addon_prices') + ->where('stripe_price_id', $priceId) + ->where('id', '!=', $keep) + ->delete(); + } + + Schema::table('stripe_addon_prices', function (Blueprint $table) { + $table->unique('stripe_price_id', 'stripe_addon_prices_price_unique'); + }); + } + + public function down(): void + { + Schema::table('stripe_addon_prices', function (Blueprint $table) { + $table->dropUnique('stripe_addon_prices_price_unique'); + }); + } +}; +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +php artisan test tests/Feature/Billing/StripePriceAdoptionTest.php +``` + +Expected: PASS, vierzehn Tests. + +- [ ] ~~**Step 5: Prove the deduplication itself, on a real migration run**~~ — **NICHT AUSFÜHREN** + +~~`php artisan migrate:fresh --env=testing && php artisan migrate:status | tail -3`~~ + +> **Warnung, nicht stillschweigend gelöscht, weil die Falle es wert ist:** Dieser Befehl darf nicht laufen — es gibt in diesem Repo **keine** `.env.testing`, also greift die Verbindung aus `.env` und `migrate:fresh` löscht und baut die **echte Entwicklungsdatenbank** neu, die laufende Container benutzen. Der Index ist durch den SQLite-Migrationslauf der Suite belegt (Step 1 und Step 4); ein zweiter Beleg ist keinen Datenverlust wert. + +- [ ] **Step 6: Mark the Block D item done** + +`docs/handoffs/2026-07-30-real-run-handoff.md:175-176` ersetzen: + +```markdown +- ~~`stripe_addon_prices.stripe_price_id` ist **nicht** eindeutig (Plan-Seite schon): + zwei Zeilen könnten einen Preis teilen, Archivieren würde den anderen mitentziehen.~~ + **Erledigt** — eindeutig seit `2026_07_31_210000_one_row_per_stripe_price`; zugleich + das Netz unter dem Wiedererkennungsschritt, siehe + `docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md`. +``` + +- [ ] **Step 7: Run the full suite — this is a schema change** + +```bash +php artisan test +``` + +Expected: PASS. Eine `UniqueConstraintViolationException` in einem fremden Test bedeutet, dass dort zwei Modul-Zeilen absichtlich eine Preis-ID teilten — das ist ein Fund, der gemeldet und nicht durch Zurücknehmen des Index behoben wird. + +- [ ] **Step 8: Commit** + +```bash +git add -- database/migrations/2026_07_31_210000_one_row_per_stripe_price.php tests/Feature/Billing/StripePriceAdoptionTest.php docs/handoffs/2026-07-30-real-run-handoff.md +``` + +```bash +git commit -F - -- database/migrations/2026_07_31_210000_one_row_per_stripe_price.php tests/Feature/Billing/StripePriceAdoptionTest.php docs/handoffs/2026-07-30-real-run-handoff.md <<'MSG' +One row per Stripe price on the module side too + +The plan register has had this since 2026_07_30_110000. The module register +never did, so two rows could claim one Price and archiving the one would have +withdrawn the Price the other was still selling — Block D of the real-run +handoff. + +It is also the net under the adoption step: at a VAT rate of nought both +treatments charge the same amount, and the check that refuses an already-claimed +Price is then the only thing keeping the two apart. + +Duplicates are deleted, not archived. An archived row goes on claiming the id +and a unique index knows nothing about archived_at; the row rebuilds itself on +the next ensure(), and subscription_addons holds the Stripe id as text rather +than a foreign key, so no booking loses its price. + +Co-Authored-By: Claude Opus 5 +MSG +``` + +--- + +## Nach dem letzten Task + +- [ ] **Voller Testlauf** (nicht nur `tests/Feature/Billing`): + +```bash +php artisan test +``` + +- [ ] **Ein Review über den Gesamtdiff**, dann **eine** Fix-Runde, dann **ein** Re-Review über den Fix-Diff — und danach werden offene Befunde mit schriftlicher Begründung geparkt (R22.1). Ein zweiter Re-Review desselben Befundes ist verboten. +- [ ] **Nicht geplant, bewusst offen** (Spec §12): ein Aufräum-Kommando für Waisen, die niemand übernehmen kann. Sie stehen mit ihrer ID im Log (`stripe: left an unexplained active price alone…`). Als Folgepunkt in den Merge Request. diff --git a/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md b/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md new file mode 100644 index 0000000..be50139 --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md @@ -0,0 +1,359 @@ +# Spec — Bestehende Stripe-Preise wiedererkennen, statt sie zu verdoppeln + +**Datum:** 2026-07-30 +**Status:** entworfen, noch nicht umgesetzt +**Vorgänger-Kontext:** `docs/handoffs/2026-07-30-real-run-handoff.md` Block D +(`stripe_addon_prices.stripe_price_id` ist nicht eindeutig) + +--- + +## 1. Der Vorfall + +Ein Lauf am **2026-07-29 23:11** legte bei Stripe +`price_1TygdEC7u8NpJ8pOt3nsoyYw` an (`priority_support`, 3480 EUR, monatlich) und +brach ab, **bevor** die Zeile in `stripe_addon_prices` geschrieben war. Commit +`9da1358` fügte danach das Metadatenfeld `tax_treatment` hinzu. Der nächste +Abgleich schickte **denselben** Idempotenz-Schlüssel mit den **neuen** Metadaten +und bekam von Stripe: + +> HTTP 400 — Keys for idempotent requests can only be used with the same +> parameters they were first used with. + +Der Abgleich war damit hart blockiert und wäre es bis zum Ablauf des Schlüssels +(24 h) geblieben. Von Hand bereinigt: Zeile für den vorhandenen Preis +nachgetragen, Metadaten bei Stripe angeglichen. Die Ursache stand offen. + +## 2. Ausgangsbefund — gemessen, nicht gelesen + +| Befund | Beleg | +|---|---| +| Der Modul-Schlüssel deckt Betrag, Währung, Intervall, Modul und Behandlung ab — **keine** Metadaten | `app/Services/Billing/AddonPrices.php:152` | +| Der Paket-Schlüssel ebenso | `app/Services/Billing/PlanPrices.php:158` | +| Beide senden Metadaten mit, die **nicht** im Schlüssel stehen — u. a. das neue `tax_treatment` | `AddonPrices.php:133-141`, `PlanPrices.php:141-151` | +| `createProduct` hat dieselbe Falle latent: Metadaten mit, Schlüssel ohne | `AddonPrices.php:269-273`, `SyncStripeCatalogue.php:104-111`, `HttpStripeClient.php:87-97` | +| Kein Aufruf fragt Stripe nach **vorhandenen** Preisen. Der Client hat gar keine Methode dafür | `app/Services/Stripe/StripeClient.php` — `createPrice`, `archivePrice`, `activatePrice`, kein Auflisten | +| `ensure()` läuft **nicht nur** im Abgleich, sondern in der **Kundenbuchung** | `BookAddon.php:259` und `:360` → `SyncStripeAddonItems.php:183` und `:291` → `AddonPrices::ensure()` | +| Der Fake spielt einen bekannten Schlüssel zurück, **ohne die Parameter zu vergleichen** | `FakeStripeClient.php:168-193`, Ledger `$keys` (`:38`) | +| `stripe_plan_prices.stripe_price_id` ist eindeutig | Migration `2026_07_30_110000:56` | +| `stripe_addon_prices.stripe_price_id` ist **nicht** eindeutig; eindeutig ist nur das Tupel | Migration `2026_07_31_200000:59-77` | +| Der Client blättert schon woanders durch Stripe-Listen | `HttpStripeClient::invoiceLines()`, `:312-336` | +| Produktion `mariadb`, Tests `sqlite :memory:` | `.env:24`, `phpunit.xml:42-43` | + +**Der 400 ist damit kein Betriebsfehler, sondern ein Kundenfehler.** Ein +Kunde, der ein Modul bucht, während ein Waisen-Preis mit altem Metadatenformat +bei Stripe liegt, bekommt 24 Stunden lang keine Buchung zustande. + +## 3. Zwei getrennte Mängel + +1. **Waise ohne Wiedererkennung.** Ein Abgleich, der zwischen Stripes Anlage und + unserem Schreiben abbricht, hinterlässt einen Preis bei Stripe, den unsere + Tabelle nie erfährt. Nach Ablauf der 24 Stunden legt der nächste Lauf einen + **zweiten aktiven** Preis über dieselbe Summe an — genau das, was der + Schlüssel verhindern soll. +2. **Metadaten im Aufruf, nicht im Schlüssel.** Jede künftige Änderung an den + Metadaten von `createPrice` blockiert den Abgleich für 24 Stunden auf + dieselbe Weise. + +## 4. Gehört das Metadatenformat zur Identität? — Nein + +**Identität** eines Preises ist, was Stripe unveränderlich macht: Produkt, +Betrag, Währung, Intervall — und, weil bei einem Steuersatz von 0 % beide +Behandlungen denselben Betrag haben, welche der beiden es ist. Metadaten sind +bei Stripe **nachträglich änderbar**. Deshalb wird ein wiedererkannter Preis mit +korrigierten Metadaten **übernommen**, nicht durch einen neuen ersetzt. + +Der Idempotenz-Schlüssel behauptet aber gar keine Identität. Er behauptet +„**diesen Aufruf habe ich schon abgeschickt**". Weil Stripe das wörtlich prüft, +müssen **alle gesendeten Parameter** hinein — sonst ist der Schlüssel eine +Lüge, die mit 400 endet. + +Der Kommentar im Code verwechselt beides („Keyed on what the Price IS", +`AddonPrices.php:142-151`). Das ist die eigentliche Ursache und wird +mitkorrigiert: die Identität wandert in den Wiedererkennungsschritt, der +Schlüssel wird auf seine tatsächliche Aussage zurückgeführt. + +## 5. Regeln des Auftraggebers, die dieser Entwurf nicht anfassen darf + +Beide wurden im Gespräch am 2026-07-30 gesetzt und gelten als Prüfstein: + +1. **Bestehende Verträge behalten ihre alten Preise.** Neue Preise gelten nur + für Kunden, die neu abschließen. Daraus folgt die Freigabe zum Archivieren: + ein archivierter Stripe-Preis bleibt für laufende Abos in Kraft, er wird nur + nicht mehr **verkauft** — worauf `archiveSuperseded()` schon heute beruht. +2. **Jede Buchung friert ihren Preis ein.** Ein Paket und zwei Module werden + immer gleich verrechnet; ein später gebuchtes Modul kommt zum heutigen Preis; + eine Stornierung zieht genau den damaligen Betrag ab und wird danach nicht + mehr verrechnet. + +Beides bleibt strukturell gewahrt, weil die Übernahme **ausschließlich** Preise +mit **identischem** Betrag, Währung und Intervall betrifft. Sie kann niemandes +Verrechnung verschieben. §11 schreibt es als Test fest. + +## 6. Bauteil 1 — `App\Services\Billing\AdoptStripePrice` + +Der Wiedererkennungsschritt, **einmal** für beide Seiten (Modul und Paket), weil +zwei Fassungen derselben Regel der Anfang ihres Auseinanderlaufens sind. + +**Stellung im Ablauf:** nach dem Blick in unsere eigene Tabelle, vor +`createPrice`. Also genau dann ein zusätzliches GET, wenn ohnehin ein POST +fällig gewesen wäre — nie im Normalfall. + +``` +ensure() + ├─ Zeile in unserer Tabelle? → ja: wie heute (ggf. reaktivieren) + ├─ AdoptStripePrice: Kandidat bei Stripe? → ja: übernehmen + └─ createPrice(…) → nein: wie heute anlegen +``` + +**Ein Kandidat wird übernommen, wenn er alle fünf Bedingungen erfüllt:** + +1. bei Stripe **aktiv** und an **unserem** Produkt hängend + (`active=true`, `product=…`, `type=recurring`); +2. **exakt** gleich in Betrag, Währung (klein/groß normalisiert) und Intervall; +3. **widerspricht keinem** unserer Metadatenschlüssel, den er trägt; +4. **bestätigt mindestens einen** tragenden Schlüssel — `addon` auf der + Modulseite, `plan_price_id` auf der Paketseite. Ein von Hand im Dashboard + angelegter Preis wird damit nie übernommen; +5. ist **von keiner Zeile beansprucht** (Prüfung gegen die eigene Tabelle, + archivierte Zeilen eingeschlossen). + +**Danach:** Metadaten bei Stripe angleichen (genau das, was von Hand getan +wurde), Zeile schreiben, `archiveSuperseded()` wie im Anlege-Pfad, Preis-ID +zurückgeben. + +**Mehrere Kandidaten:** der **älteste** (`created` aufsteigend) wird übernommen — +er ist am ehesten der, auf dem eine verlorene Zeile schon bilanziert. Die +übrigen werden bei Stripe **archiviert** (Freigabe aus §5.1) und protokolliert. + +**Kein beweisbar eigener Kandidat:** Verhalten wie heute (anlegen), mit Warnung +im Log. Bewusst so: einen fremden Preis zu übernehmen ist schlimmer als einen +zweiten anzulegen, und die Warnung nennt die ID, damit ein Betreiber entscheiden +kann. + +**Meldeweg ist das Log** (`Log::warning`), nicht die Konsolenausgabe: `ensure()` +läuft auch im Web-Request, wo es keine Kommandozeile gibt. + +## 7. Bauteil 2 — `App\Services\Stripe\IdempotencyKey` + +Bildet aus dem sprechenden Schlüssel plus einem kurzen Fingerabdruck der +**tatsächlich gesendeten Parameter** den Header-Wert: + +```php +IdempotencyKey::for('clupilot-addon-price-priority_support-month-3480-EUR', $payload) +// → 'clupilot-addon-price-priority_support-month-3480-EUR-9f3a1c07' +``` + +Kanonisch gebildet (Schlüssel sortiert, dann `sha1`, auf 8 Zeichen gekürzt), +damit dieselbe Nutzlast immer denselben Fingerabdruck ergibt. In **einer** +Support-Klasse, weil `HttpStripeClient` und `FakeStripeClient` denselben Wert +bilden müssen — sonst prüft der Test etwas anderes, als die Produktion sendet. + +**Angewandt nur auf `createPrice` und `createProduct`.** Bewusst **nicht** auf: + +| Aufruf | Warum der 400 dort bleiben muss | +|---|---| +| `refund` | Ein zweiter Refund schickt dem Kunden das Geld zweimal. | +| `cancelSubscription` | Andere Parameter unter demselben Schlüssel heißt: eine andere Kündigungsart als die schon abgeschickte. | +| `addSubscriptionItem` | Ein zweites Item verrechnet dasselbe Modul doppelt. | +| `createCheckoutSession` | Eine zweite Session zu geänderten Parametern ist ein zweiter Kaufvorgang. | + +Die Regel lautet also: **der Fingerabdruck ist für die Katalogseite, wo ein +Duplikat reparierbar und eine Blockade kundenwirksam ist.** Bei +Geldbewegungen ist es umgekehrt. §11 schreibt diese Grenze als Test fest, damit +sie niemand „vereinheitlicht". + +## 8. Zwei neue Client-Methoden + +```php +/** @return array}> */ +public function activePricesFor(string $productId): array; + +public function updatePriceMetadata(string $priceId, array $metadata): void; +``` + +`activePricesFor()` blättert nach dem Muster von `invoiceLines()` +(`HttpStripeClient.php:312-336`) — ein Familien-Produkt sammelt über Versionen, +Terme, Behandlungen und Steuersatzänderungen hinweg leicht mehr als eine Seite, +und beim ersten Seitenende stehenzubleiben wäre dieselbe Lücke in neuer Form. + +`updatePriceMetadata()` benutzt, dass Metadaten eines der wenigen Felder sind, +die ein Stripe-Preis ändern lässt — dieselbe Eigenschaft, auf der +`activatePrice()` schon beruht. + +Beide im `FakeStripeClient` nachgezogen. + +## 9. Der Fake muss Stripe erst einmal nachbilden + +`FakeStripeClient::createPrice` spielt einen bekannten Schlüssel zurück, **ohne +die Parameter zu vergleichen** (`:168-193`). Er ist genau in der Sache +unrealistisch, um die es hier geht — deshalb konnte kein Test den Vorfall sehen. + +Der Ledger `$keys` speichert künftig neben der ID auch die Nutzlast und wirft +bei gleichem Schlüssel mit anderen Parametern — mit Stripes Wortlaut. Das ist +der Regressionstest, der den 29.07. gesehen hätte. + +## 10. Migration — eine Zeile pro Stripe-Preis + +`2026_07_31_210000_one_row_per_stripe_price.php` (nach der letzten vorhandenen +Migration einsortiert): + +1. **Entdoppeln:** Zeilen in `stripe_addon_prices`, die eine + `stripe_price_id` teilen — niedrigste Zeilen-ID bleibt, die übrigen werden + **gelöscht**. Nicht archiviert: eine archivierte Zeile beansprucht die ID + weiter und der eindeutige Index kennt `archived_at` nicht. +2. **Index:** `stripe_price_id` wird eindeutig — wie auf der Paketseite seit + `2026_07_30_110000:56`. + +Löschen ist gefahrlos: die Zeile baut sich beim nächsten `ensure()` über die +Wiedererkennung selbst wieder auf, und `subscription_addons.stripe_price_id` +hält die Stripe-ID als **Text**, nicht als Fremdschlüssel. + +Treiberneutral formuliert (Auswahl über `groupBy`/`having`, Löschen über +IDs) — Produktion ist `mariadb`, Tests laufen auf `sqlite`. + +**Was der Index absichert:** Bei einem Steuersatz von 0 % sind die Beträge +beider Behandlungen gleich. Die Übernahme lehnt einen bereits beanspruchten +Preis dann über §6 Bedingung 5 ab und legt für die zweite Behandlung einen +eigenen an (der Schlüssel trägt `-rc`). Der Index ist das Netz darunter, falls +diese Prüfung je gerissen wird. + +## 11. Tests + +Zwei Regeldatei-Tests nach Repo-Sitte, getrennt danach, **gegen wen** sie prüfen: + +- `tests/Feature/Billing/StripeIdempotencyKeyTest.php` — was auf der Leitung + landet. Läuft über `Http::fake()` gegen `HttpStripeClient`, weil geprüft werden + muss, was **gesendet** wird, nicht was der Fake sich merkt. Hier steht auch, + dass der Fake Stripes 400 überhaupt nachbildet. +- `tests/Feature/Billing/StripePriceAdoptionTest.php` — die Übernahmeregeln, + beide Seiten, über den `FakeStripeClient`. + +| Prüfung | Deckt ab | +|---|---| +| Waise bei Stripe → wird übernommen, `createPrice` **nicht** aufgerufen, Zeile trägt die vorhandene ID (Modul **und** Paket) | Mangel 1 | +| Waise mit **altem** Metadatenformat → wird übernommen, Metadaten bei Stripe angeglichen | der Vorfall selbst | +| Zwei Waisen → älteste übernommen, andere bei Stripe archiviert, Warnung im Log | §6 | +| Aktiver Preis an unserem Produkt **ohne** unsere Metadaten → **nicht** übernommen, neuer angelegt, Warnung | §6 Bedingung 4 | +| Preis-ID schon von einer Zeile beansprucht → **nicht** zweimal vergeben | §6 Bedingung 5 | +| **Eine eingefrorene Buchung behält ihren Betrag**: ein auf altem Preis laufendes Modul wird von einem Abgleich nicht verschoben | §5.2 | +| Gleicher sprechender Schlüssel + geänderte Metadaten → **unterschiedlicher** Header, kein 400 | Mangel 2 | +| Fingerabdruck **nur** bei `createPrice`/`createProduct`, nicht bei `refund`, `cancelSubscription`, `addSubscriptionItem`, `createCheckoutSession` | §7 | +| Der Fake wirft bei gleichem Schlüssel mit anderen Parametern | §9 | +| Zwei Zeilen können keine `stripe_price_id` teilen | §10 | + +## 12. Nicht Teil dieses Vorhabens + +- **Ein Aufräum-Kommando** für Waisen, die niemand mehr übernehmen kann (etwa + ohne unsere Metadaten). Der Log-Eintrag nennt sie; ein Kommando dafür ist ein + Folgepunkt. +- **Die übrigen Block-D-Punkte** aus `docs/handoffs/2026-07-30-real-run-handoff.md`. +- **`stripe:reprice-subscriptions`** und alles, was laufende Verträge bewegt. + Dieser Entwurf bewegt keinen einzigen — siehe §5. +- **Stripes `automatic_tax`** bleibt aus, `TaxTreatment` bleibt die einzige + Steuerinstanz. + +## 13. Berührte Dateien + +**Neu** +- `app/Services/Billing/AdoptStripePrice.php` +- `app/Services/Stripe/IdempotencyKey.php` +- `database/migrations/2026_07_31_210000_one_row_per_stripe_price.php` +- `tests/Feature/Billing/StripeIdempotencyKeyTest.php` +- `tests/Feature/Billing/StripePriceAdoptionTest.php` + +**Geändert** +- `app/Services/Stripe/StripeClient.php` — zwei Methoden +- `app/Services/Stripe/HttpStripeClient.php` — Auflisten, Metadaten schreiben, + Fingerabdruck bei den zwei Anlege-Aufrufen +- `app/Services/Stripe/FakeStripeClient.php` — beide Methoden, Ledger mit + Parametervergleich +- `app/Services/Billing/AddonPrices.php` — Wiedererkennung vor `createPrice`, + Kommentar zur Schlüssel-Begründung berichtigt +- `app/Services/Billing/PlanPrices.php` — dasselbe +- `docs/handoffs/2026-07-30-real-run-handoff.md` — Block-D-Punkt als erledigt + markieren + +--- + +## 14. Folgepunkte, die aus der Umsetzung entstanden sind + +Nachgetragen nach dem Abschluss-Review. Alle Befunde sind offengelegt, keiner +blockiert den Merge — aber keiner ist erledigt. + +### Die eine echte Lücke: Waisen-**Produkte** + +`activePricesFor()` erkennt einen verwaisten *Preis* wieder. Für ein verwaistes +**Produkt** gibt es kein Gegenstück, und dort ist der Schaden größer: ein +zweites Produkt lässt jedes `activePricesFor($neueId)` leer zurückkommen, damit +ist die Wiedererkennung für dieses Modul oder diese Familie **dauerhaft blind** +und jede Waise am ersten Produkt unerreichbar. Der Zweig deaktiviert seine +eigene Sicherung durch die Lücke, die er nicht geschlossen hat. + +Betroffen: `SyncStripeCatalogue::handle()` (Produkt anlegen, ID danach in einem +zweiten Schreibvorgang speichern) und `AddonPrices::product()`. Beide Kommentare +benennen die Lücke jetzt; der Fix wäre ein `productsFor()` nach dem Muster von +`activePricesFor()`, das Stripe nach den Produkten fragt und über die Metadaten +wiedererkennt. + +### Das Geld-Tor sitzt an der falschen Stelle + +`AdoptStripePrice` verspricht in seinem Kopfkommentar, keine Übernahme könne +Geld verschieben — kann aber nur Betrag, Währung und Intervall vergleichen. Was +sonst noch entscheidet, was ein Preis verrechnet (`interval_count`, +`usage_type`, `transform_quantity`, `billing_scheme`), filtert +`HttpStripeClient::activePricesFor()`. Zwei Folgen: der `FakeStripeClient` kann +diese Felder gar nicht ausdrücken, also kann **kein Test von +`AdoptStripePrice` das Geld-Tor abdecken**; und eine dritte Implementierung von +`StripeClient` ließe es still fallen. Die Prüfung gehört in die Klasse, die sie +zusagt. + +Verwandt: `PlanPrices::ensure()` hat keine `<= 0`-Schranke, wo +`AddonPrices::ensure()` eine hat. Das ist das Einzige, was einen +`billing_scheme: tiered`-Preis (bei dem Stripe `unit_amount: null` liefert, hier +als 0 gelesen) auf der Paketseite überhaupt erreichbar macht. + +### Kleineres, benannt statt vergessen + +- **Der Abgleich zählt „angelegt", auch wo er übernommen hat.** Die Wortwahl der + Konsolenausgabe ist entschärft („created or adopted"), gezählt wird weiterhin + vor dem Aufruf. Eine echte Trennung der beiden Zahlen fehlt — und das ist die + eine Zahl, die ein Mensch nach einem Vorfall zuerst liest. +- **Die Warnung über einen unerklärlichen Preis hat keine Drosselung.** Sie + feuert bei jedem Abgleich *und* jeder Kundenbuchung, solange die Waise + existiert — und weil das Aufräum-Kommando (§12) ein Folgepunkt ist, auf + unbestimmte Zeit. Der Widerspruchs-Pfad wurde aus genau diesem Grund still + gestellt; der Bestätigungs-Pfad bekam dieselbe Überlegung nicht. +- **`FakeStripeClient::updatePriceMetadata()` ersetzt, wo Stripe zusammenführt**, + und `activePricesFor()` gibt Metadaten ungecastet zurück, wo der HTTP-Client + auf String castet. Heute nicht beobachtbar, aber es ist genau die Sorte + Untreue des Fakes, gegen die §9 geschrieben wurde. +- **`created` im Fake kann gleichstehen**: `createPrice()` stempelt + `count($prices) + 1`, `plantPrice()` hat die Vorgabe `1`. Der Kommentar sagt + das jetzt zu, statt es zu bestreiten; ein Gleichstand-Tiebreak auf die ID + fehlt weiter. +- **`HttpStripeClient::activePricesFor()` bricht das Blättern still ab**, wenn + dem letzten Eintrag einer nicht-letzten Seite die `id` fehlt — getreue Kopie + desselben Verhaltens in `invoiceLines()`. Die Folge hat sich aber geändert: in + `invoiceLines()` entsteht ein zu kurzer Belegtext, hier eine ungesehene Waise + und ein zweiter aktiver Preis, also der Vorfall. Wer es anfasst, soll werfen + statt anhalten. +- **Der Idempotenz-Fingerabdruck und der POST-Rumpf müssen zusammenbleiben.** + Ein Feld, das künftig in `createPrice()` gesendet, aber nicht in + `IdempotencyKey::priceParameters()` aufgenommen wird, öffnet den Vorfall + wieder. Der Test „does not block a booking because the metadata format moved" + wacht über diese Nahtstelle — mit einem gesetzten Ledger-Eintrag, weil der + Zustand heute nicht mehr fahrbar ist. + +### Zwei fremde, vorbestehende Testfehler, die dieser Zweig aufgedeckt hat + +Keiner davon wird von diesem Zweig verursacht. + +1. **`HostStepsTest.php:1064`** schreibt `fsn-01.node.clupilot.com` hart hinein, + während `.env:107` `CLUPILOT_DNS_ZONE=clupilot.cloud` setzt und `phpunit.xml` + neun andere Variablen mit `force="true"` festnagelt, diese aber nicht. Eine + Zeile in `phpunit.xml`. Derselbe Punkt steht als offene Aufgabe in + `docs/handoffs/2026-07-30-real-run-handoff.md`. +2. **`PlanCatalogueTest.php`** ist einmal ausgefallen und lief beim + Wiederholen durch. „Beim zweiten Mal grün" ist keine Diagnose, und ein + sprunghafter Fehler in den Katalogpreis-Tests liegt im Wirkungskreis dieses + Zweigs, auch wenn er nicht von ihm kommt. diff --git a/tests/Feature/Billing/StripeIdempotencyKeyTest.php b/tests/Feature/Billing/StripeIdempotencyKeyTest.php new file mode 100644 index 0000000..ead4498 --- /dev/null +++ b/tests/Feature/Billing/StripeIdempotencyKeyTest.php @@ -0,0 +1,266 @@ +set('services.stripe.secret', 'sk_test_plan_task_one'); +}); + +it('sends a different key once the metadata changes', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + $spoken = 'clupilot-addon-price-priority_support-month-3480-EUR'; + + // The call as it stood before 9da1358, and the call after it: same money, + // same interval, one metadata field more. + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], $spoken); + + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], $spoken); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[0])->toStartWith($spoken) + ->and($sent[1])->toStartWith($spoken) + ->and($sent[1])->not->toBe($sent[0]); +}); + +it('sends the same key for the very same call', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_x'])]); + + $client = new HttpStripeClient; + + foreach ([1, 2] as $ignored) { + $client->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + } + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->unique() + ->all(); + + expect($sent)->toHaveCount(1); +}); + +it('fingerprints the product call too, where the same trap was waiting', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'prod_x'])]); + + $client = new HttpStripeClient; + + $client->createProduct('Priority Support', ['addon' => 'priority_support'], 'clupilot-addon-product-x'); + $client->createProduct('Priority Support', ['addon' => 'priority_support', 'sold_as' => 'entitlement'], 'clupilot-addon-product-x'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent[1])->not->toBe($sent[0]); +}); + +it('leaves the money calls their bare key, so Stripe still refuses a changed one', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'x'])]); + + $client = new HttpStripeClient; + + $client->refund('pi_1', 500, 'clupilot-refund-7'); + $client->cancelSubscription('sub_1', 'at_period_end', 'clupilot-cancel-7'); + $client->addSubscriptionItem('sub_1', 'price_1', 1, 'none', 'clupilot-item-7'); + + $sent = collect(Http::recorded()) + ->map(fn (array $pair) => $pair[0]->header('Idempotency-Key')[0] ?? null) + ->all(); + + expect($sent)->toBe(['clupilot-refund-7', 'clupilot-cancel-7', 'clupilot-item-7']); +}); + +it('reproduces the refusal Stripe makes, which the fake used to swallow', function () { + $fake = new FakeStripeClient; + + $fake->refund('pi_1', 500, 'clupilot-refund-7'); + + // Same key, different amount. Stripe answers 400; the fake said nothing and + // replayed the first refund's id, which is how a test could pass over the + // very failure that stopped production. + expect(fn () => $fake->refund('pi_1', 900, 'clupilot-refund-7')) + ->toThrow(RuntimeException::class, 'same parameters'); +}); + +it('mints a second price rather than blocking when the metadata moved', function () { + $fake = new FakeStripeClient; + + $first = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], 'clupilot-addon-price'); + + $second = $fake->createPrice('prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support', 'tax_treatment' => 'domestic'], 'clupilot-addon-price'); + + // Two objects, no exception. That the second one is not WANTED is the job of + // AdoptStripePrice, not of the key — see StripePriceAdoptionTest. + expect($second)->not->toBe($first); +}); + +it('pages through every active price of a product', function () { + Http::fake([ + 'api.stripe.com/*' => Http::sequence() + ->push([ + 'data' => [ + ['id' => 'price_a', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_b', 'unit_amount' => 41760, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'year'], 'metadata' => []], + ], + 'has_more' => true, + ]) + ->push([ + 'data' => [ + ['id' => 'price_c', 'unit_amount' => 2900, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month'], + 'metadata' => ['addon' => 'priority_support', 'tax_treatment' => 'reverse_charge']], + ], + 'has_more' => false, + ]), + ]); + + $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + + expect($prices)->toHaveCount(3) + ->and($prices[0])->toBe([ + 'id' => 'price_a', + 'unit_amount' => 3480, + // Upper case, because that is how our own tables hold it and the + // comparison in AdoptStripePrice must not have to remember which + // side is which. + 'currency' => 'EUR', + 'interval' => 'month', + 'created' => 100, + 'metadata' => ['addon' => 'priority_support'], + ]) + ->and($prices[2]['id'])->toBe('price_c'); + + // The second page has to be asked for, or this reintroduces the very gap it + // exists to close — a family product accumulates prices across versions, + // terms, treatments and every rate change. + Http::assertSent(fn ($request) => str_contains($request->url(), 'starting_after=price_b')); + + // Archived prices are none of our business here: we are looking for + // something to SELL on. + Http::assertSent(fn ($request) => str_contains($request->url(), 'active=true')); +}); + +it('skips a price it could not have created itself, so the money gate never trusts a partial recurrence', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + 'data' => [ + ['id' => 'price_monthly', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_quarterly', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'month', 'interval_count' => 3], + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_metered', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month', 'usage_type' => 'metered'], + 'metadata' => ['addon' => 'priority_support']], + ], + 'has_more' => false, + ]), + ]); + + $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + + // A hand-duplicated dashboard price keeps our metadata, so nothing + // downstream could tell it apart, and the module would bill quarterly. + // + // None of the three planted prices carries `transform_quantity` or + // `billing_scheme` at all, and the ordinary one still comes back — so this + // also holds the other direction of the next test's filter: an absent key is + // Stripe's default, not a reason to reject. + expect(collect($prices)->pluck('id')->all())->toBe(['price_monthly']); +}); + +it('skips a price that would charge our figure for the wrong quantity', function () { + Http::fake([ + 'api.stripe.com/*' => Http::response([ + 'data' => [ + // Spelled out as Stripe actually sends them for an ordinary + // price: transform_quantity null, billing_scheme per_unit. + ['id' => 'price_ordinary', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 100, 'recurring' => ['interval' => 'month'], + 'transform_quantity' => null, 'billing_scheme' => 'per_unit', + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_divided', 'unit_amount' => 3480, 'currency' => 'eur', + 'created' => 101, 'recurring' => ['interval' => 'month'], + 'transform_quantity' => ['divide_by' => 10, 'round' => 'up'], + 'billing_scheme' => 'per_unit', + 'metadata' => ['addon' => 'priority_support']], + ['id' => 'price_tiered', 'unit_amount' => null, 'currency' => 'eur', + 'created' => 102, 'recurring' => ['interval' => 'month'], + 'billing_scheme' => 'tiered', + 'metadata' => ['addon' => 'priority_support']], + ], + 'has_more' => false, + ]), + ]); + + $prices = (new HttpStripeClient)->activePricesFor('prod_1'); + + // The concrete harm: modules are billed BY quantity — SyncStripeAddonItems + // sums a pack into ONE item at quantity n — so price_divided is our exact + // figure, on our Product, carrying our `addon` key, and adoption would take + // it. A customer holding three would then be charged ceil(3/10) = 1. + // + // price_tiered is refused by name rather than by luck: Stripe reports + // unit_amount null for a tiered price, activePricesFor() casts that to 0, and + // the amount match alone only rejects it while the caller's own figure is not + // 0 — which PlanPrices::ensure(), unlike AddonPrices::ensure(), does not + // guarantee. + expect(collect($prices)->pluck('id')->all())->toBe(['price_ordinary']); +}); + +it('writes metadata onto a price that already exists', function () { + Http::fake(['api.stripe.com/*' => Http::response(['id' => 'price_a'])]); + + (new HttpStripeClient)->updatePriceMetadata('price_a', ['addon' => 'priority_support']); + + Http::assertSent(fn ($request) => $request->url() === 'https://api.stripe.com/v1/prices/price_a' + && $request['metadata[addon]'] === 'priority_support'); +}); + +it('lets the fake answer with the prices it holds, minus the archived ones', function () { + $fake = new FakeStripeClient; + + $kept = $fake->createPrice('prod_1', 3480, 'EUR', 'month', ['addon' => 'priority_support']); + $gone = $fake->createPrice('prod_1', 2900, 'EUR', 'month', ['addon' => 'priority_support']); + $other = $fake->createPrice('prod_2', 3480, 'EUR', 'month', []); + $fake->archivePrice($gone); + + $fake->plantPrice('price_orphan', 'prod_1', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 0); + + $found = collect($fake->activePricesFor('prod_1'))->pluck('id')->all(); + + expect($found)->toContain($kept, 'price_orphan') + ->and($found)->not->toContain($gone, $other); +}); diff --git a/tests/Feature/Billing/StripePriceAdoptionTest.php b/tests/Feature/Billing/StripePriceAdoptionTest.php new file mode 100644 index 0000000..df68ab1 --- /dev/null +++ b/tests/Feature/Billing/StripePriceAdoptionTest.php @@ -0,0 +1,612 @@ +stripe = new FakeStripeClient; + app()->instance(StripeClient::class, $this->stripe); +}); + +/** The module metadata as AddonPrices sends it today. */ +function moduleMetadata(string $treatment = 'domestic'): array +{ + return ['addon' => 'priority_support', 'tax_treatment' => $treatment]; +} + +/** Ask the adoption step the question AddonPrices asks it. */ +function adoptModulePrice(?array $metadata = null, ?callable $claimed = null): ?string +{ + return app(AdoptStripePrice::class)( + productId: 'prod_support', + amountCents: 3480, + currency: 'EUR', + interval: 'month', + metadata: $metadata ?? moduleMetadata(), + identifying: ['addon'], + claimed: $claimed ?? fn (string $id) => false, + ); +} + +it('adopts the orphan of 2026-07-29 instead of minting a second price', function () { + // The state that morning: the Price exists at Stripe, carries the metadata + // of the code that made it — WITHOUT tax_treatment, which 9da1358 added + // afterwards — and no row of ours knows it. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + + expect(adoptModulePrice())->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw'); + + // Brought up to today's metadata rather than replaced: metadata is mutable + // at Stripe, the amount is not, which is the whole reason the format is no + // part of a Price's identity. + expect($this->stripe->metadataUpdates)->toBe([[ + 'price' => 'price_1TygdEC7u8NpJ8pOt3nsoyYw', + 'metadata' => moduleMetadata(), + ]]); +}); + +it('leaves the metadata alone when it already says the right thing', function () { + $this->stripe->plantPrice('price_ok', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBe('price_ok') + ->and($this->stripe->metadataUpdates)->toBe([]); +}); + +it('leaves the metadata alone when Stripe merged it in a different order plus a key of its own', function () { + // plantPrice() stores our own array in our own order, so the test above + // cannot catch an order- or merge-sensitive comparison: Stripe returns + // metadata as the result of a MERGE, never a replace, in whatever key + // order it likes — and a write never removes a key it did not send. + $this->stripe->plantPrice('price_ok', 'prod_support', 3480, 'EUR', 'month', [ + 'tax_treatment' => 'domestic', + 'addon' => 'priority_support', + 'internal_note' => 'added by hand in the Stripe dashboard', + ]); + + expect(adoptModulePrice())->toBe('price_ok') + ->and($this->stripe->metadataUpdates)->toBe([]); +}); + +it('adopts nothing when the amount, currency or interval differ', function () { + $this->stripe->plantPrice('price_cheaper', 'prod_support', 2900, 'EUR', 'month', moduleMetadata()); + $this->stripe->plantPrice('price_yearly', 'prod_support', 3480, 'EUR', 'year', moduleMetadata()); + $this->stripe->plantPrice('price_dollars', 'prod_support', 3480, 'USD', 'month', moduleMetadata()); + + expect(adoptModulePrice())->toBeNull(); +}); + +it('refuses a price nothing proves is ours, and says so', function () { + Log::spy(); + + // What a person clicking through Stripe's own dashboard leaves behind: the + // right money on our product, and not one word about what it is for. + $this->stripe->plantPrice('price_by_hand', 'prod_support', 3480, 'EUR', 'month', []); + + expect(adoptModulePrice())->toBeNull(); + + Log::shouldHaveReceived('warning')->once(); +}); + +it('passes silently over another of our own prices', function () { + Log::spy(); + + // At a VAT rate of nought both treatments are the same amount, so the + // reverse-charge Price sits at the domestic one's money — and contradicts on + // tax_treatment. That is not a mystery worth a warning; it is a Price of + // ours that is not the one being asked for. + $this->stripe->plantPrice('price_rc', 'prod_support', 3480, 'EUR', 'month', + moduleMetadata('reverse_charge')); + + expect(adoptModulePrice())->toBeNull(); + + Log::shouldNotHaveReceived('warning'); +}); + +it('never hands out a price a row already claims', function () { + $this->stripe->plantPrice('price_taken', 'prod_support', 3480, 'EUR', 'month', moduleMetadata()); + + expect(adoptModulePrice(claimed: fn (string $id) => $id === 'price_taken'))->toBeNull(); +}); + +it('adopts the oldest of several orphans and stops selling the rest', function () { + Log::spy(); + + $this->stripe->plantPrice('price_second', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 200); + $this->stripe->plantPrice('price_first', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 100); + $this->stripe->plantPrice('price_third', 'prod_support', 3480, 'EUR', 'month', + ['addon' => 'priority_support'], created: 300); + + // The oldest, because it is the one a lost row is likeliest to have been + // billing on. + expect(adoptModulePrice())->toBe('price_first') + ->and($this->stripe->archived)->toBe(['price_second', 'price_third']); + + Log::shouldHaveReceived('warning'); +}); + +it('takes the orphan over instead of minting a second module price', function () { + // The product exists because a previous run got that far; the Price exists + // because the run that made it died before the insert. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + + $before = count($this->stripe->prices); + + $id = app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + expect($id)->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw') + // Nothing new at Stripe: the orphan was taken over, not replaced. + ->and(count($this->stripe->prices))->toBe($before) + ->and(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('interval', 'month') + ->where('reverse_charge', false) + ->value('stripe_price_id'))->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw'); +}); + +it('does not block a booking because the metadata format moved', function () { + // 2026-07-29, exactly: orphan with the old metadata, code with the new. This + // is the call that answered HTTP 400 for twenty-four hours. + $this->stripe->plantPrice('price_1TygdEC7u8NpJ8pOt3nsoyYw', 'prod_support', + 3480, 'EUR', 'month', ['addon' => 'priority_support']); + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + + // Stripe's own side of that morning, and load-bearing for the same reason as + // the cleared ledger in 'takes over an orphaned package price' below — only + // the other way round. Without an entry here createPrice() would simply mint, + // and the test could not tell "adoption prevented the 400" from "no 400 was + // possible". So the ledger is seeded with what the pre-9da1358 call left + // behind: the id it was answered with, and a fingerprint over the metadata + // that call sent — `addon` alone, before tax_treatment existed. The entry sits + // under the header today's code sends, because that is the only way a ledger + // keyed by header can put this createPrice() in front of a poisoned key: the + // fingerprint is folded into the header now, so a changed call also changes + // the header, which is the branch's OTHER half of the same fix. Seeded + // directly, therefore, and honestly so — what it stands in for is Stripe + // holding this call's key against different parameters. + $this->stripe->keys[IdempotencyKey::forPrice( + 'clupilot-addon-price-priority_support-month-3480-EUR', + 'prod_support', 3480, 'EUR', 'month', moduleMetadata(), + )] = [ + 'id' => 'price_1TygdEC7u8NpJ8pOt3nsoyYw', + 'fingerprint' => IdempotencyKey::fingerprint(IdempotencyKey::priceParameters( + 'prod_support', 3480, 'EUR', 'month', ['addon' => 'priority_support'], + )), + ]; + + $id = app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + // Adoption is what keeps createPrice() from ever being reached. Take the + // adopt() call out of AddonPrices::ensure() and this test does not merely + // return the wrong id — it dies inside the fake with Stripe's own sentence, + // 'Keys for idempotent requests can only be used with the same parameters + // they were first used with.' That is the 2026-07-29 blockade, reproduced. + expect($id)->toBe('price_1TygdEC7u8NpJ8pOt3nsoyYw') + ->and($this->stripe->metadataUpdates)->toHaveCount(1); +}); + +it('leaves a frozen booking on the price it was sold at', function () { + // Seeds the product id AddonPrices::product() will find and reuse — same + // reason as the first two tests in this file. Without it, the sync below + // mints its own Stripe Product with a generated id, and the orphan planted + // further down (deliberately at the literal 'prod_support' Stripe uses + // throughout this file) would sit on a product nothing ever asks about, + // making it inert rather than the competing figure the test needs. + StripeAddonPrice::query()->create([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 41760, 'net_cents' => 34800, 'currency' => 'EUR', + 'interval' => 'year', 'stripe_product_id' => 'prod_support', + 'stripe_price_id' => 'price_yearly_already_known', + ]); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + $sold = app(AddonPrices::class)->liveFor( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ); + + $subscription = Subscription::factory()->plan('team')->create(); + $booking = SubscriptionAddon::query()->create([ + 'subscription_id' => $subscription->id, + 'addon_key' => 'priority_support', + // Net, per month, per unit — frozen at booking. `currency` and + // `booked_at` are both NOT NULL (2026_07_26_060000), and `uuid` fills + // itself through the model's uniqueIds(). + 'price_cents' => 2900, + 'currency' => 'EUR', + 'quantity' => 1, + 'booked_at' => now(), + 'stripe_price_id' => $sold, + ]); + + // The catalogue moves, and somebody has already left an orphan at the new + // figure. Neither may reach a booking that is already frozen. + config()->set('provisioning.addons.priority_support.price_cents', 3900); + $this->stripe->plantPrice('price_orphan_new_figure', 'prod_support', + 4680, 'EUR', 'month', ['addon' => 'priority_support']); + + app(Kernel::class)->call('stripe:sync-catalogue'); + + // The `archived` assertion right below — the second of this chain — is + // this test's only adoption tooth. AdoptStripePrice archives a Price at + // Stripe when it treats one as a duplicate of another it adopted, and + // that is the one way it could reach past the figure it was asked for + // (4680, the new one) and take the frozen booking's own Price (3480) down + // with it as a false "duplicate". + // + // The three assertions after it read the SWEEP's own row-writing code, + // not adoption — AdoptStripePrice performs no database writes at all, so + // nothing it does, singly or in combination, can fail them. They earn + // their place anyway, because they are exactly what a defect in the + // sweep's OTHER two writers would fail: archiveSuperseded() losing its + // `->where('net_cents', $netCents)` scoping (its own docblock is about + // precisely why that scoping has to hold) would sweep the 2900-net row up + // alongside the 3900-net one it is meant to supersede, and remember() + // rewritten as an updateOrCreate() keyed without `amount_cents` — the + // plausible-looking fix for the UniqueConstraintViolationException catch + // a few lines below it — would silently overwrite the very row these + // assertions read. + // + // Neither of adoption's two guards is isolated by this test — the amount + // match and the `claimed` callback cover for each other in this scenario, + // which is why sabotaging either alone during this fix round left the + // booking untouched. Each is isolated on its own elsewhere in this file: + // it('adopts nothing when the amount, currency or interval differ') and + // it('never hands out a price a row already claims'). + expect($booking->refresh()->stripe_price_id)->toBe($sold) + ->and($this->stripe->archived)->not->toContain($sold) + // The OLD figure's Price is still what a checkout for it would use — + // adoption of the orphan at the NEW figure must not have reached + // backwards and pulled the frozen figure's Price out of circulation. + ->and(app(AddonPrices::class)->liveFor( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ))->toBe($sold) + // The row itself, not only whether it is archived — `archived_at` + // reads null for a row that is still live AND for one that is gone + // or rewritten onto a different net_cents, so `exists()` has to stand + // beside it or the row's disappearance would pass silently. + ->and(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('reverse_charge', false) + ->where('net_cents', 2900) + ->where('interval', 'month') + ->exists())->toBeTrue() + ->and(StripeAddonPrice::query() + ->where('addon_key', 'priority_support') + ->where('reverse_charge', false) + ->where('net_cents', 2900) + ->where('interval', 'month') + ->value('archived_at'))->toBeNull() + // And asking for the OLD figure again — the same call a renewal on + // this very booking would make — must still be handed $sold, not the + // orphan sitting at the new figure's amount. + ->and(app(AddonPrices::class)->ensure( + 'priority_support', 2900, 'EUR', Subscription::TERM_MONTHLY, TaxTreatment::domestic(), + ))->toBe($sold); +}); + +it('takes over an orphaned package price', function () { + // A catalogue mirrored once, so families have Products and rows have Prices. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + + // The register loses its row and Stripe keeps the Price: a run that died + // between the two, seen from the next run's point of view. + $orphan = (string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id'); + StripePlanPrice::query()->where('stripe_price_id', $orphan)->delete(); + + // Load-bearing, not incidental cleanup: Stripe forgets an idempotency key + // after twenty-four hours, and that expiry is the only condition under + // which the 2026-07-29 duplicate was ever minted. With the sync's own key + // still in the fake's ledger, createPrice() would just replay $orphan's + // id on its own — proving nothing about adoption. Clearing it is what + // makes createPrice() able to mint a genuine second Price, which is the + // only way this test can fail. + $this->stripe->keys = []; + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->toBe($orphan) + ->and(count($this->stripe->prices))->toBe($before) + ->and(StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->where('charged_cents', $charged) + ->value('stripe_price_id'))->toBe($orphan) + // The pointer for the ordinary domestic sale is written as before. + ->and((string) $row->refresh()->stripe_price_id)->toBe($orphan); +}); + +it('does not take a package price belonging to another catalogue row', function () { + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $product = (string) $row->version->family->stripe_product_id; + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $interval = $row->term === Subscription::TERM_YEARLY ? 'year' : 'month'; + + // This row's OWN domestic Price from the sync above becomes a legitimate + // orphan the moment its register row is gone — exactly like the test + // above — and adopt() would be right to reclaim it. Archived here so the + // only Price left at this exact figure is the wrong-owner one below: + // otherwise adoption would correctly adopt the row's own orphan before + // ever weighing 'price_other_row', and the rejection this test exists to + // prove would never be reached. + $this->stripe->archivePrice((string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id')); + + StripePlanPrice::query()->where('plan_price_id', $row->id)->delete(); + + // Same product, same money, same interval — and plan_price_id says it is a + // DIFFERENT row's Price. One Product carries every version and term of a + // family, so this is the ordinary case, not an exotic one. + $this->stripe->plantPrice('price_other_row', $product, $charged, (string) $row->currency, + $interval, ['plan_price_id' => (string) ($row->id + 1000), 'tax_treatment' => 'domestic']); + + // Same reason as the test above: with the sync's own idempotency key + // still in force, createPrice() would replay THIS row's own previous + // Price — which would also not be 'price_other_row', whether or not + // adoption ever looked at the wrong-owner Price at all. Clearing the + // ledger forces a real mint when nothing adoptable is found, which is + // what gives the assertions below something to catch. + $this->stripe->keys = []; + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->not->toBe('price_other_row') + // A fresh Price was minted rather than 'price_other_row' being handed + // out. Proves AdoptStripePrice's contradicts()/confirms() pair + // correctly refused the wrong-owner Price WHEN adoption runs — the + // two guard each other here exactly as they do on the module side, so + // either alone still catches this; only disabling both together lets + // 'price_other_row' through. It does NOT prove PlanPrices wires + // adopt() into ensure() at all: skipping that wiring entirely mints + // fresh here too, indistinguishable from a correct refusal. The test + // above ("takes over an orphaned package price") is what proves the + // wiring is present. + ->and(count($this->stripe->prices))->toBe($before + 1) + // The Price belonging to the other row must survive untouched. + // AdoptStripePrice archives every orphan but the one it adopts, so a + // defect that let it treat 'price_other_row' as a duplicate of + // whatever it did adopt would take this down as collateral damage — + // exactly the failure a wrong plan_price_id must never cause. + ->and($this->stripe->archived)->not->toContain('price_other_row'); +}); + +it('does not adopt a package price that only proves the family, not this row', function () { + // The whole reason the plan side needs `identifying: ['plan_price_id']` + // rather than the module side's simpler key: one Product carries a Price + // for every version, term AND treatment of a family, so a Price that + // proves only the FAMILY could be any of them. If `identifying` were ever + // loosened to ['plan_family'], a Price like the one planted below would + // wrongly confirm — nothing else here would catch it. + app(Kernel::class)->call('stripe:sync-catalogue'); + + $row = PlanPrice::query()->firstOrFail(); + $product = (string) $row->version->family->stripe_product_id; + $charged = PlanPrices::chargedCents($row, TaxTreatment::domestic()); + $interval = $row->term === Subscription::TERM_YEARLY ? 'year' : 'month'; + + // This row's OWN domestic Price is a legitimate orphan the moment its + // register row is gone — same reason as the two tests above — and would + // otherwise be correctly re-adopted before the planted Price below is + // ever weighed, masking the very rejection this test means to prove. + $this->stripe->archivePrice((string) StripePlanPrice::query() + ->where('plan_price_id', $row->id) + ->where('reverse_charge', false) + ->value('stripe_price_id')); + + StripePlanPrice::query()->where('plan_price_id', $row->id)->delete(); + + // Right product, right money, right currency, right interval, and the + // right FAMILY — the one thing missing is plan_price_id, which is the + // only key that says which of the family's many rows a Price belongs to. + $this->stripe->plantPrice('price_family_only', $product, $charged, (string) $row->currency, + $interval, ['plan_family' => $row->version->family->key]); + + // Same reason as the two tests above: with the sync's own idempotency + // key still in force, createPrice() would replay this row's own previous + // Price rather than genuinely minting, leaving nothing for the + // assertions below to catch. + $this->stripe->keys = []; + + $before = count($this->stripe->prices); + + $id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic()); + + expect($id)->not->toBe('price_family_only') + // A fresh Price was minted rather than the family-only Price being + // handed out — this is the assertion `identifying: ['plan_price_id']` + // exists to keep true; swap it for ['plan_family'] and this fails. + ->and(count($this->stripe->prices))->toBe($before + 1) + ->and($this->stripe->archived)->not->toContain('price_family_only'); +}); + +it('lets no two module rows claim one stripe price', function () { + $shared = [ + 'addon_key' => 'priority_support', 'net_cents' => 2900, 'currency' => 'EUR', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared', + ]; + + StripeAddonPrice::query()->create([...$shared, + 'reverse_charge' => false, 'amount_cents' => 3480, 'interval' => 'month']); + + // Two rows on one Price is what Block D warned about: archiving the one + // would withdraw the Price the other is still selling. + expect(fn () => StripeAddonPrice::query()->create([...$shared, + 'reverse_charge' => true, 'amount_cents' => 2900, 'interval' => 'month'])) + ->toThrow(UniqueConstraintViolationException::class); +}); + +it('keeps only the lowest-id row in every group of duplicates, and leaves the rest of the table alone', function () { + // This is the one branch of 2026_07_31_210000_one_row_per_stripe_price that + // deletes rows from a live table, and it cannot be reached through the + // ordinary migrated schema: RefreshDatabase already ran this migration, so + // its OWN unique index would refuse the very duplicate this test needs to + // insert. Dropping the index first puts the table back into the shape the + // migration was written to find — the shape a real, never-yet-migrated + // production table was in — so up() can be run again and actually take its + // delete branch instead of finding nothing to do. + Schema::table('stripe_addon_prices', fn (Blueprint $table) => $table->dropUnique('stripe_addon_prices_price_unique')); + + // Group one: two rows sharing a Price, differing in `reverse_charge` so the + // pre-existing COMPOSITE unique index (addon_key, reverse_charge, + // amount_cents, currency, interval) does not itself refuse the insert — + // the VAT-rate-zero case Block D warned about, where only the shared + // stripe_price_id gives the duplicate away. + $kept = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 2900, 'net_cents' => 2900, 'currency' => 'EUR', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared', + 'created_at' => now(), 'updated_at' => now(), + ]); + + $duplicate = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => true, + 'amount_cents' => 2900, 'net_cents' => 2900, 'currency' => 'EUR', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared', + 'created_at' => now(), 'updated_at' => now(), + ]); + + // Group two: THREE rows sharing a second, different Price — its own tuple + // dimension varied per row (interval, then currency) so the composite index + // never fires here either. One group of two proves the loop runs; this one + // proves it runs more than once AND that a single iteration can delete more + // than one row. + $kept2 = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + $duplicate2a = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'EUR', 'interval' => 'year', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + $duplicate2b = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 3900, 'net_cents' => 3900, 'currency' => 'USD', 'interval' => 'month', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared_two', + 'created_at' => now(), 'updated_at' => now(), + ]); + + // On its own Price entirely — not part of any duplicate group — and must + // survive completely untouched. + $unrelated = DB::table('stripe_addon_prices')->insertGetId([ + 'addon_key' => 'extra_storage', 'reverse_charge' => false, + 'amount_cents' => 1000, 'net_cents' => 1000, 'currency' => 'EUR', 'interval' => 'month', + 'stripe_product_id' => 'prod_storage', 'stripe_price_id' => 'price_untouched', + 'created_at' => now(), 'updated_at' => now(), + ]); + + // require, not require_once: the file `return`s a fresh anonymous-class + // instance every time it is evaluated, which is what lets this run the + // migration's up() a second time in this same process. + (require database_path('migrations/2026_07_31_210000_one_row_per_stripe_price.php'))->up(); + + $keptRow = DB::table('stripe_addon_prices')->where('id', $kept)->first(); + $kept2Row = DB::table('stripe_addon_prices')->where('id', $kept2)->first(); + $unrelatedRow = DB::table('stripe_addon_prices')->where('id', $unrelated)->first(); + + // The LOWER id survives in each group — the row that was written first, + // which is the one anything already billing is likeliest to have been + // reading. Were that ordering silently inverted, the surviving row could be + // one nothing ever pointed at. Read back, not merely checked for existence: + // a survivor that exists but was mangled in the process — the wrong + // stripe_price_id, or an amount_cents that no longer matches what the + // shared Price actually charges — would be exactly the "kept the right row + // but damaged it" failure existence alone cannot catch. + expect($keptRow)->not->toBeNull() + ->and($keptRow->stripe_price_id)->toBe('price_shared') + ->and((int) $keptRow->amount_cents)->toBe(2900) + ->and($kept2Row)->not->toBeNull() + ->and($kept2Row->stripe_price_id)->toBe('price_shared_two') + ->and((int) $kept2Row->amount_cents)->toBe(3900) + ->and($unrelatedRow)->not->toBeNull() + ->and($unrelatedRow->stripe_price_id)->toBe('price_untouched') + ->and((int) $unrelatedRow->amount_cents)->toBe(1000) + // Every duplicate in both groups is gone — the higher id from group + // one, and BOTH higher ids from group two's three-way tie. + ->and(DB::table('stripe_addon_prices')->where('id', $duplicate)->exists())->toBeFalse() + ->and(DB::table('stripe_addon_prices')->where('id', $duplicate2a)->exists())->toBeFalse() + ->and(DB::table('stripe_addon_prices')->where('id', $duplicate2b)->exists())->toBeFalse() + // Six rows went in, three survive: nothing beyond the two kept + // duplicates and the untouched row is left standing. + ->and(DB::table('stripe_addon_prices')->count())->toBe(3); + + // And the index is back in force: a further row on a third, otherwise + // distinct tuple, but the SAME Price id, is refused by stripe_price_id + // alone rather than being silently accepted. + expect(fn () => DB::table('stripe_addon_prices')->insert([ + 'addon_key' => 'priority_support', 'reverse_charge' => false, + 'amount_cents' => 4900, 'net_cents' => 4900, 'currency' => 'EUR', 'interval' => 'year', + 'stripe_product_id' => 'prod_support', 'stripe_price_id' => 'price_shared', + 'created_at' => now(), 'updated_at' => now(), + ]))->toThrow(UniqueConstraintViolationException::class); +});