From 4eb90c834af9723b4d30fb70cc8aaeba73833cbd Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 14:02:51 +0200 Subject: [PATCH] Write down what the reviews found and nobody fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten follow-ups, none of them blocking and none of them done. The one that matters: activePricesFor() recognises an orphaned Price and nothing recognises an orphaned PRODUCT — and a second Product is worse than a duplicate Price, because every activePricesFor() then asks about the wrong Product and recognition goes blind for that whole family. The branch disables its own guard through the gap it did not close. Second: AdoptStripePrice promises in its own docblock that no adoption can move money, but can only compare amount, currency and interval. The four properties that also decide what a Price charges are filtered in HttpStripeClient, which the fake cannot express — so no test of the class can reach the promise it makes. Also recorded: two pre-existing test failures this branch surfaced without causing, with the diagnosis, so the next person does not derive them again. Co-Authored-By: Claude Opus 5 --- ...2026-07-30-stripe-price-adoption-design.md | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) 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 index 1f3987a..be50139 100644 --- a/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md +++ b/docs/superpowers/specs/2026-07-30-stripe-price-adoption-design.md @@ -272,3 +272,88 @@ Zwei Regeldatei-Tests nach Repo-Sitte, getrennt danach, **gegen wen** sie prüfe - `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.