plan: fix ambiguous build command

This commit is contained in:
shum
2026-08-27 10:28:26 +00:00
parent 74dc4bed59
commit baa18653ff
+2 -1
View File
@@ -114,7 +114,7 @@ Each step below is one reviewable commit.
**Build and test commands:**
```bash
cabal build simplex-chat simplex-badge-service
cabal build lib:simplex-chat exe:simplex-chat simplex-badge-service
cabal test --test-options='-m "Supporter badges" -m "Badge service"'
cd apps/simplex-badge-service/web && npm run build
```
@@ -1343,6 +1343,7 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
- **A2 — three more `Int64` id fields need the same correction the step already mandates.** The step lists `BadgePurchase.paymentId`, `BadgePayment.paymentId` and `BadgeIssuance.issuanceId`. An audit of `badges-rpc.schema.json` against all four modules found the same defect in three further places, all `TEXT` columns typed as `Int64`: `StatementCreditType.SCCharge {chargeId}` (`Badges/Service.hs:163`, against `subscription_charges.charge_id TEXT NOT NULL PRIMARY KEY`), and `BadgeCharge.chargeId` and `BadgeCharge.paymentId` (`Badges/Types.hs:163-164`). `SCCharge` is the load-bearing one: it is a wire type whose `taggedObjectJSON` instance A2 writes, the schema declares `chargeId` as `string` (`badges-rpc.schema.json:230`), and Aeson would encode an `Int64` as a JSON number — so leaving it ships a payload that fails its own schema. Its sibling `SCPayment` already carries `Maybe InvoiceId`, a newtype over `Text`. A2 corrects all six.
- **`LedgerCreditType.CTPayment {invoiceId :: Int64}` and `CTCharge {chargeId :: Int64}` are wrong against their columns but are marked `-- confirmed`. OPEN — needs a decision, not a mechanical fix.** `invoices.invoice_id` and `subscription_charges.charge_id` are both `TEXT`. These are the DB-side twins of the wire types above, and `CTTransferIn {fromPurchaseId :: Maybe Int64}` beside them is correct because `from_purchase_id` really is `INTEGER`. A2 does not touch them: they sit outside its tagged-sum list, and altering a type someone marked confirmed is above a mechanical step. C1's `insertLedgerEntries` is the first code that would persist them, so this must be settled before C1.
- **A4 — `offerTotal` calls `error` on an impossible offer, which B6 and D4 must not let reach a request thread.** `chargeableMonths` (`BadgeService/Catalog.hs`) rejects `freeMonths >= months` with `error` rather than wrapping a `Word8` subtraction, and `seedCatalog` forces it at startup so a bad catalog kills the process before the service accepts traffic. That fences it for Phase A, where `seedCatalog` is the only writer. It stops being fenced the moment `offerTotal`/`catalogTotals` run inside request handling over rows read from the database, which is B6 (`getBadgeCatalog`) and D4 (`/api/catalog`). The bot's `processQueuedRequests` is a single-threaded `forever` loop (`BadgeService/Service.hs:96-99`), so an uncaught `error` there would take the whole service down for every user rather than failing one request — strictly worse than the mispricing the guard prevents. Before B6, either give `BadgeOffer` a smart constructor so `freeMonths >= months` is unrepresentable, or catch at the request boundary so the blast radius is one response.
- **§4 — the stated build command did not work; corrected in place.** `cabal build simplex-chat simplex-badge-service` fails with `Ambiguous target 'simplex-chat'`, because `simplex-chat` names both a library and an executable component. It is now `cabal build lib:simplex-chat exe:simplex-chat simplex-badge-service`, which was run and succeeds. The test command beside it was correct as written and passes: 41 examples, 0 failures across both `Supporter badges` and `Badge service`.
- **A6 — a duplicated ini section or key is silently accepted, keeping only one of the two. OPEN.** `Data.Ini`'s `parseIni` is `parseOnly iniParser` over a `many`-based parser, so it essentially cannot fail on malformed-but-textual input: `many` never fails and `parseOnly` does not require full input consumption, so trailing garbage is dropped without error. `readIniFile` therefore returns `Left` only on I/O errors. Two consequences the config parser does not check for. A repeated `[section]` header discards the earlier block entirely, because `iniSections` is a `HashMap.fromList` and the last value wins; a repeated key within one section keeps the first, because the lookup takes the first match. If the surviving block is itself valid, the service starts with a silently wrong configuration — including, for instance, a secret-file path from the wrong block. Add a duplicate-section and duplicate-key check to `Config.hs`, and until then say so in the operator documentation at H5.
- **A6 — `seedCatalog` calls `getCurrentTime` directly, against the rule that only `BadgeServiceEnv.now` reads the clock.** This is structurally forced rather than an oversight: `seedCatalog` runs before `newBadgeServiceEnv` exists, since the env is built after migrations and seeding. Either seed later, or give `seedCatalog` a clock parameter, whenever a step first needs to control service time across startup.
- **A1 — `chat_lint.sql` gains 5 fkey-index advisories, left unfixed by design.** The badge migration introduces unindexed foreign keys: `badge_invoices.offer_id`, `badge_invoices.price_id`, `badge_offers.price_id`, `badge_issuances.entry_id`, `users.shown_badge_id`. The lint output is committed literally rather than adding indexes, since index design is outside A1's scope and the repo has precedent for this (`9e000d6bc`). The first three point at rarely-mutated reference tables. The last two are the ones likely to matter under load — `badge_issuances.entry_id` for issuance lookup by ledger entry, and `users.shown_badge_id` for per-user badge display (C1's `getShownPurchase`). Decide on indexes for those two before release.