plan: record the phase C review findings

plans/badges-codes/2026-08-21-badges-web-checkout.md

@@ -1134,7 +1134,7 @@ The "Redeem code" entry point already exists on both platforms (`BadgesSupportSi
 - A paste button.
 - Submit calls `APIPurchaseBadge`.
 - Inline errors mapped from `CEBadgeServiceError`: `code_invalid` to "This code isn't valid", `code_used` to "This code has already been used", `code_expired` to "This code has expired", `rate_limited` to a wait message using `retryAfter`. Any other code, `internal` included, renders the response's `message` with the support hint, so a locally failed credential verification (C4) is never a blank screen.
-- Disable submit while in flight. The RPC has a per-call timeout, so show progress and allow a retry with the identical code; the service is idempotent.
+- Disable submit while in flight and show progress: the RPC has a per-call timeout. **On a timeout, do not offer a retry of the same code.** `redeemBadgeCode` mints a fresh purchase key on every call and persists nothing before the send (C1, C4), so a retry reaches the service as a *different signer*; the service's replay path keys on the purchase key, so it answers `code_used` on a code the user paid for. The timeout is the one outcome that may have been delivered, and `sendBadgeRequest` says so in its own sentence (C4, §9) — render that message and direct the user to the support contact. The recovery is the operator's `codes unredeem` (H2), not another attempt in the app.
 - Success routes to the badge screen.
 
 **Verify:** Manual on desktop against a locally run service: a good code issues a badge, and each error renders inline.
@@ -1147,6 +1147,8 @@ The iOS command, response and event unions live in `AppAPITypes.swift` (`:15`, `
 
 **Do:** Mirror G2, including its default case for an unmapped error code, and add the Swift wrapper for `APIGetBadgeState` in `AppAPITypes.swift` and `SimpleXAPI.swift`, the counterpart of G1's Kotlin wrapper, which G5 calls from the badge screens. The entry point already exists (`BadgesSupportSimplexView.swift:108-126`) and the "Redeem code" terminal action on the duration screen landed in G0, so this step adds only that view and that wrapper.
 
+Stated here rather than left to "mirror G2", because it is the one rule whose cost is a code the user paid for: **on a timeout, do not offer a retry of the same code.** The client mints a fresh purchase key per call, so a retry is a different signer and the service answers `code_used`; render `sendBadgeRequest`'s timeout message, point at the support contact, and leave the recovery to `codes unredeem` (H2).
+
 **Verify:** Manual on iOS against a locally run service: a good code issues a badge, and each error renders inline.
 
 #### G4 — Kotlin: catalog pricing and badge-state refresh
@@ -1159,6 +1161,7 @@ The iOS command, response and event unions live in `AppAPITypes.swift` (`:15`, `
 - Amounts arrive in minor units. Format them with one private formatter in `BadgeStore.kt`, following D3's rule: divide by 100, pad the remainder to two digits, prefix the symbol for `currency` (`usd` → `$`), and render an unknown currency as its ISO code before the digits. Never divide a total by a month count.
 - Rename the fetch-state wrapper to `BadgePriceState` so it no longer collides with the protocol type `BadgePrice` (§3). `BadgePriceState.Loading` covers the fetch; `BadgePriceState.Unavailable` covers a failed fetch, a missing `active` price, or an offer whose `total` is `Nothing`, which no service implementing this plan sends (A2), and that tier or duration renders disabled rather than hidden (UX §2.1), matching D3.
 - Call `APIGetBadgeState` (G1's wrapper) when a badge screen opens and when it regains focus, caching the `CRBadgeState` for G1's hand-off URL and G6's paid-through date. That call is also the client-side trigger C3's worker relies on for the month boundary, so without these call sites a crossed month is invisible until the daily timer fires.
+- **`UserBadge.monthsLeft` excludes the month currently issued**, so a freshly redeemed 3-month code reads as `monthsLeft = 2` with `paidThrough` three months out (C2, §9; the haddock on `UserBadge` states the arithmetic). Both figures are right, but "2 months left" shown on its own straight after a purchase reads as if the user was shortchanged. Make `paidThrough` the primary figure on any surface that shows one number.
 - Handle `CEvtBadgeChanged` in the event dispatcher: refresh the cached badge state and recompose the badge screens. G2's redeem success path reads state directly and does not need this handler; only the worker's month-boundary re-issue (C3) does, and without it that re-issue is invisible until the app restarts.
 - `BadgeStore`'s `load()`, `price()` and `annualSavings()` lose their last Kotlin callers, and the three `load()` call sites go with them (`BadgesPayView.kt:76`, `BadgesYourLevelView.kt:60`, `BadgesSupportSimplexView.kt:37`). The `BadgePriceState` type stays in `BadgeStore.kt`, so the module remains compiled with only its store-facing surface unused, kept for store-evidence verification (§6, G0).
 - Resolves the `TODO [badges]` markers at `BadgesPayView.kt:28,176`, `BadgesYourLevelView.kt:26` and `BadgeStore.kt:130`.
@@ -1169,7 +1172,7 @@ The iOS command, response and event unions live in `AppAPITypes.swift` (`:15`, `
 
 **Files:** `apps/ios/Shared/Views/Badges/{BadgesPayView.swift,BadgesYourLevelView.swift,BadgesSupportSimplexView.swift,BadgeStore.swift}`, `apps/ios/Shared/Model/{SimpleXAPI.swift,AppAPITypes.swift}`
 
-**Do:** Mirror G4, including the open-and-focus `APIGetBadgeState` calls on G3's Swift wrapper, the rename of the Swift fetch-state wrapper to `BadgePriceState` in `BadgeStore.swift` (§3), its own private minor-unit formatter following D3's rule, and the `CEvtBadgeChanged` handling. The Swift `load()` call sites are `BadgesPayView.swift:126`, `BadgesYourLevelView.swift:101` and `BadgesSupportSimplexView.swift:66`; the price and savings calls are `BadgesPayView.swift:144,167` and `BadgesYourLevelView.swift:121`. Resolves the `TODO [badges]` markers at `BadgesPayView.swift:12` and `BadgesYourLevelView.swift:12`; `BadgesPayView.kt:176` and `BadgeStore.kt:130` are desktop and `foss` only and have no Swift counterpart.
+**Do:** Mirror G4, including the open-and-focus `APIGetBadgeState` calls on G3's Swift wrapper, the rename of the Swift fetch-state wrapper to `BadgePriceState` in `BadgeStore.swift` (§3), its own private minor-unit formatter following D3's rule, the `CEvtBadgeChanged` handling, and G4's `monthsLeft` rule — the month currently issued is not counted, so a 3-month code reads as "2 month(s) left" the moment it is redeemed and `paidThrough` is the figure to lead with. The Swift `load()` call sites are `BadgesPayView.swift:126`, `BadgesYourLevelView.swift:101` and `BadgesSupportSimplexView.swift:66`; the price and savings calls are `BadgesPayView.swift:144,167` and `BadgesYourLevelView.swift:121`. Resolves the `TODO [badges]` markers at `BadgesPayView.swift:12` and `BadgesYourLevelView.swift:12`; `BadgesPayView.kt:176` and `BadgeStore.kt:130` are desktop and `foss` only and have no Swift counterpart.
 
 **Verify:** Manual on iOS against a locally run service, as for G4; run the app in the simulator so the badge-state refresh sees the same advanced host clock as the service.
 
@@ -1214,12 +1217,12 @@ The redemption path runs over service RPC and has no IP; B5's per-signer throttl
 
 **Files:** `apps/simplex-badge-service/src/BadgeService/Admin.hs`, `apps/simplex-badge-service/src/BadgeService/Options.hs`, `tests/Bots/BadgeServiceTests.hs`
 
-**Do:**
+**Do:** Take `codes unredeem` **first**. Phase C ships the client without it, and until it lands there is **no operator remedy at all** for a code burned by a lost response: `redeemBadgeCode` persists nothing before the send and mints a fresh purchase key per call, so a redemption whose reply is lost consumes the code with nothing stored on the client, and every later attempt — by the app or by hand — is a different signer and answers `code_used` (§9). Every other item here is convenience beside that.
 
+- `codes unredeem --order <orderId> | --ref <shortRef> | --code <SXB-…>` calls B1's `unredeemCode`, so the user can retry the same code and E4 discloses it again for a further `codeDisclosureDays`. A batch code has no order and no `shortRef`, so `--code` is its only selector; the operator holds the plaintext from `codes issue`. This covers C4's lost-response case. **Re-point C5's `codes unredeem` example at this command when it lands**: `testC5SameCodeAgainThenUnredeemed` (`tests/Bots/BadgeServiceTests.hs`) calls `unredeemCode` directly today, because this command did not exist when C5 was written (§9). Reissuing a *different* code **for the same order** is impossible by construction: the code is a pure function of `orderId` (decision 9), so unredeeming is the only coherent recovery for that order. Compensation outside an order uses a batch code from `codes issue` (B8, H3).
 - No expiry sweeper. Expiry is evaluated at redemption time by B3's classifier against `expires_at`, so there is no expired state at rest to maintain.
 - `codes status --order <orderId>` and `codes status --ref <shortRef>`. Both print the order status, badge type, months and redemption state. With `--reveal` they additionally derive and print the plaintext code and the `?order=` resume URL, which is the support path E6 promises. `--reveal` requires an operator reason, is refused for a redeemed or revoked code, and its use is logged with the reason and the `shortRef`, never the code or the `orderId` (H4).
 - Authorisation is possession of the database file and `[codes] secret_file`; there is no in-band authentication and no HTTP surface (decision 3). The operator runs it on the service host as the service user.
-- `codes unredeem --order <orderId> | --ref <shortRef> | --code <SXB-…>` calls B1's `unredeemCode`, so the user can retry the same code and E4 discloses it again for a further `codeDisclosureDays`. A batch code has no order and no `shortRef`, so `--code` is its only selector; the operator holds the plaintext from `codes issue`. This covers C4's lost-response case. **Re-point C5's `codes unredeem` example at this command when it lands**: `testC5SameCodeAgainThenUnredeemed` (`tests/Bots/BadgeServiceTests.hs`) calls `unredeemCode` directly today, because this command did not exist when C5 was written (§9). Reissuing a *different* code **for the same order** is impossible by construction: the code is a pure function of `orderId` (decision 9), so unredeeming is the only coherent recovery for that order. Compensation outside an order uses a batch code from `codes issue` (B8, H3).
 - Order of operations for a lost response, which is the case support meets: `codes status --ref` confirms the redemption, `codes unredeem --ref` clears it, then `codes status --ref --reveal` prints the code, which `--reveal` now permits because the code is unredeemed again. Support holds only the `shortRef` (E6), so both support subcommands, `status` and `unredeem`, accept `--ref`.
 - Every admin action logged with a timestamp and the operator-supplied reason.
 
@@ -1410,10 +1413,10 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
 - **C3 — the Postgres cross-check was not run for this step.** SQLite-only: `cabal test --test-options='-m "Supporter badges" -m "Badge service"'`, `CI` unset. The `--flags=client_postgres` cross-check B10 established is optional per-step and was skipped here rather than run; nothing in this step is guarded `#if !defined(dbPostgres)`, so there is no reason to expect a backend-specific gap, but it has not been exercised against Postgres and that remains open until it is.
 - **C3 review round — a pass for a non-active profile silently switched the active user. Fixed.** `startBadgeWorkers` starts a worker for every user from `getUsers`, not just the active one, and the daily timer re-fires each — so once `sendBadgeRequest` is live (C4), a background profile crossing a month boundary would overwrite `currentUser` with itself, changing the app's active profile with nothing having asked. Latent under C3 alone because the stub never issues. Fixed with the same read-then-compare-then-write idiom already used at `Commands.hs:576,1647,4373`: `storeBadgeIssueResponse` now reads `currentUser` and only writes the pass's `User` back when its `userId` is the one already active. The Do bullet above is corrected to state this rather than "writing the `User` it returns to the `currentUser` TVar" unconditionally, which is what produced the bug — that phrasing was written from a single-profile viewpoint.
 - **C3 review round — the worker loop did not gate on `waitChatStartedAndActivated`. Fixed.** Every other periodic loop in this module gates each iteration on it (`cleanupManager`, `runRelayGroupLinkChecks`, `expireChatItems`); `runBadgeWorker` did not, so after `APISuspendChat` the timer still fired passes that read the store and, once C4 lands, would attempt a signed send through a suspended agent. `lift waitChatStartedAndActivated` now sits at the top of the `forever` body, before the `race_`.
-- **C3 review round — a shown purchase with no ledger rows stalled silently. Fixed with a log line.** Unreachable via C1's `createPurchase`, which always inserts an opening credit, but nothing signalled the stall if it ever happened. `issueDueBadgePeriod` now `logWarn`s the purchase and user id in that case rather than falling through to `BadgeUnchanged` with no trace.
+- **C3 review round — a shown purchase with no ledger rows stalled silently. Fixed with a log line.** Unreachable, but nothing signalled the stall if it ever happened. `issueDueBadgePeriod` now `logWarn`s the purchase and user id in that case rather than falling through to `BadgeUnchanged` with no trace. **The reason it gave for being unreachable was false, and is corrected below** (phase C review round).
 - **C3 review round — a healed ledger's complete history could write a duplicate issuance row. Fixed.** `badge_issuances` has no uniqueness on `(badge_purchase_id, period_start)`, and `createIssuance` mints a fresh id per call; the "no duplicate issuance from a re-delivered history" argument above covers the append case but not REPLACE-after-heal, where the service returns a complete history whose last entry is a `debit(badge)` the client already holds an issuance for. `Store/Badges.hs` adds `hasIssuanceForPeriod`, checked before the worker's `createIssuance`. **C4's redeem path calls `createIssuance` without it** (`Commands.hs`), and correctly so: it writes the issuance of a purchase row created two statements earlier in the same transaction, which can hold no issuance yet — the check applies where a statement can re-present a period the purchase already has an issuance for, which is the healed-ledger case only.
 - **C3 review round — the lock ordering that is this step's central risk has no regression test.** `badgeManagerPass` reads the user row, then takes the badge lock and runs `issueDueBadgePeriod`, then releases it before `presentBadgeChange`. This is guaranteed by source shape alone — nothing tests that a concurrent operation on the same profile cannot interleave between the lock's release and `presentUserBadgeToContacts`. `BadgeGate`'s doc comment in `BadgeManagerTests.hs` was also corrected: the gated clock parks a pass after `badgeManagerPass`'s own `getUser` but before `issueDueBadgePeriod` reads the purchase or ledger, not "before it reads any state".
-- **C3 review round — `APIGetBadgeState` racing `stopChatController`'s worker-map swap can re-insert an uncancelled worker. Recorded, not fixed.** `getAgentWorker` can create and insert a fresh entry into `badgeWorkers` between the map being swapped to empty and the old workers being cancelled; that entry is then never cancelled. Same shape as a pattern already present in the agent's own worker bookkeeping elsewhere in the codebase, not introduced by this step. Left as a known gap: `stopChatController` runs once per process shutdown, and the window is one `getAgentWorker` call wide.
+- **C3 review round — `APIGetBadgeState` racing `stopChatController`'s worker-map swap can re-insert an uncancelled worker. Recorded, not fixed.** `getAgentWorker` can create and insert a fresh entry into `badgeWorkers` between the map being swapped to empty and the old workers being cancelled; that entry is then never cancelled. Same shape as a pattern already present in the agent's own worker bookkeeping elsewhere in the codebase, not introduced by this step. Left as a known gap: `stopChatController` runs once per process shutdown, and the window is one `getAgentWorker` call wide. **The same race has a second branch, found in the phase C review and FIXED — see the entry below.** This entry recorded only the re-insertion branch; the other one hangs the command thread forever.
 
 - **C4 — `sendBadgeRequest` also takes the `NetworkRequestMode` and the `User`.** The step's signature is `Maybe C.PrivateKeyEd25519 -> BadgeServiceRequest -> CM BadgeServiceResponse`, but the agent's service request is created under an agent user (`aUserId`) and resolves a short link or a SimpleX name under a request mode. Reading the ACTIVE user inside the send would put C3's per-profile pass on whichever profile happens to be active — the same defect class as the `currentUser` write the C3 review round fixed — so the caller states the profile instead: the two commands pass `withUserId`'s user and `processChatCommand`'s `nm`, the worker passes its own pass's user and `NRMBackground`. It is still one send path with three call sites, not a second one.
 - **C4 — the service signs the tier the CODE funds, and the client states nothing.** The step, core §5 and UX 2.8 all have the RESPONSE state the badge type, and the client needs it stated: `badge_purchases.initial_badge_type`/`current_badge_type` are `NOT NULL` and a code carries no tier (B3: 20 opaque characters, with none of the tier in them). B7 had added, beyond its own step, a check refusing a `purchaseBadge` whose `badgeRequest` names a tier other than the code's — which makes code redemption unimplementable for any client that was not told the tier out of band, and no client is: G2's and G3's redeem views are a code field and a paste button, and a code may come from `codes issue` (B8) or from an order the app never opened. The two could not both hold. **Resolved on the service side, in C4's own range**, after an independent review confirmed it: `handlePurchaseCode.redeem` applies `withFundedBadgeType` to the request immediately before `resolveIssue`, so the credential carries the code's tier whatever the request named. The security property B7's check defended is kept in full — a credential can never exceed its funding, because the tier is read from the code's row and never from the request — and it is now asserted directly instead of through a proxy: B10's `testBadgeServiceSignsFundedTier` (renamed from `testBadgeServiceTierMismatchIsBadRequest`) presents a **legend** request over a **supporter** code and pins the credential, the wire ledger and the purchase row as supporter. Proved able to fail: with the override dropped, it reports `expected ("credential tier", BTSupporter) but got ("credential tier", BTLegend)` — the service handing out a signed legend credential for a supporter code, which is exactly the defect the assertion exists to catch. Three things deliberately did NOT change: the override is at this ONE call site and not in `issueSignedBadge`, which `handleIssueBadge` shares; `requestMasterKey` still reads the original request, so the credential stays bound to the client's own master key; and `planTxn`'s `currentBadgeType /= badgeType` guard is untouched, because it compares the existing purchase row's tier to the CODE's and is what stops a legend code crediting a supporter purchase (B10's per-signer bucket test still trips exactly that guard, verified, not assumed). The replay path's tier check was deleted with the refusal it belonged to: it could only ever have refused an honest client repeating a request after a timeout without knowing the tier, which is the case the idempotency rule exists for. `codeBadgeTypes` and the tier probe C4 first shipped are gone; `APIPurchaseBadge` sends exactly one request, whose `badgeInfo.badgeType` is `BTSupporter` because the field is not optional on the wire and nothing reads it.
@@ -1447,6 +1450,24 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
 - **C5 fix round — `drainEcho` eats blank lines, and now says so.** `""` is a prefix of every command, so up to 8 blank rows arriving before an echo are consumed silently, and a blank line a later `<## ""` expected would go missing there instead of failing here. Left as it is deliberately, and documented: tightening it to "the fragment must have added something" would reject a bare `"> "` row, which is one of the two shapes the artifact this tolerance exists for might have — and C5 could not reproduce it to find out which. A blank line before a command's own echo has never been observed.
 - **C5 fix round — three comments corrected, one plan cross-reference added.** The happy-path example claimed that verifying the stored credential proved the client had checked it before storing; it does not (the real service only signs valid credentials), and the comment now points at `testC5UnverifiableCredentialWritesNothing` and its mutation, which is the actual proof. The catalog example now names the six-line comparison as what catches `catalogRequest` starting to sign. And H2's **Do** now carries the instruction to re-point `testC5SameCodeAgainThenUnredeemed` at `codes unredeem` when that command lands, instead of that depending on H2's author reading §9.
 
+- **Phase C review — `purchaseBadge{code}`'s replay path is unreachable from the shipped client, and a timeout during redemption therefore burns a paid code.** RPC §Idempotency and §10's second invariant both read as if a client could safely repeat a redemption after a lost reply. It cannot. `redeemBadgeCode` mints a fresh purchase keypair and master key on **every** call and persists nothing before the send (C1's "nothing is persisted before the send" rule, which `initial_badge_type NOT NULL` forces), and the service's replay path keys on the purchase key: `classifyRow` answers `RedeemAlreadyRedeemedBySameKey` only when `redeemerKey == Just purchaseKey`, and `planTxn`'s own comment says "C4 mints a fresh key per redemption, so there is no purchase row". So a retry arrives as a **different signer** and can only ever be answered `code_used` — on a code the user paid for. **Consequence:** a redemption whose reply is lost consumes the code with nothing written on the client, and neither the app nor the user can recover it. The only recovery is the operator's `codes unredeem` (H2), which is why H2's **Do** now takes that command first and G2's and G3's steps now forbid offering a retry of the same code after a timeout. Documentation only: persisting the purchase key before the send would overturn C1's rule and is a design change, not a fix. Note what is **not** affected: `issueBadge`'s `IssueCached` path is reachable and exercised — the worker signs with the purchase key stored on the row, so its retry after a lost reply is the same signer and is answered with the cached credential (B10's `testBadgeServiceIssueBadgeCachedInLastFundedMonth`). It is `purchaseBadge{code}`'s replay alone that no shipped client can reach.
+- **Phase C review — `sendBadgeRequest` told the user a timeout had not been delivered. Fixed.** `tryAllErrors (sendRequest target)` collapsed four outcomes into one sentence — "could not be reached, and the request was not delivered" — for an unconfigured address, a pre-delivery transport failure, an **agent timeout** and an undecodable response. For the timeout that claim is affirmatively false: the request may have reached the service and consumed the code. G2 renders this exact `message` for an `internal` code, so combined with the entry above it steered the user into the retry that reports `code_used`. The timeout now has its own branch, matched on the constructor path (`ChatErrorAgent {agentError = AGENT (A_SERVICE ASETimeout)}`, which is what `withAgent`'s `chatErrorAgent` wraps `serviceRequest`'s timeout in) rather than on a rendered string, and its own sentence: the request may still have been delivered, a code presented with it may already have been used, entering it again will not help, contact support. No catch-all was added — every other error still reports non-delivery, which for them is true. `testC5ServiceStoppedIsInternal` was asserting the false sentence (nothing is listening, so it times out); it now asserts the timeout line in full, spelled out rather than derived from the function under test.
+- **Phase C review — the `internal` code is overloaded, and G2/G3 cannot tell the two kinds apart from the code alone.** `docs/protocol/badges-rpc.md:64` makes every code other than the marked transient ones "terminal for the attempted command", and `internal` is one of them. But the client emits `BSEInternal` for its **own** local failures too (`localBadgeError`, `sendBadgeRequest`): an unconfigured service address, a transport failure, an undecodable response, a credential that fails verification, a store payment method. Their retryability differs per command and none of them is the service's `internal`. No new error code was added — that would be a protocol change (§6, and it is explicitly out of this round's scope). What distinguishes them is the `message`: the service sends `internal` with no message, and every client-local failure carries a non-empty one. **G2 and G3 must treat a non-empty `message` on an `internal` as the local kind** and render it, which they already do for the blank-screen reason; this entry is the record of *why* that rule exists rather than being a fallback.
+- **Phase C review — `APIGetBadgeState`'s worker signal could hang the command thread forever. Fixed.** The second branch of the race the C3 entry above records, and the worse one. `getAgentWorker'` reads the map and calls `runWorker` in two separate `atomically` blocks (`Agent/Client.hs:408`); `cancelWorker` (`:1005-1007`) empties the worker's `action` TMVar without replacing it; `runWorkerAsync` (`:463-465`) opens with a blocking `takeTMVar action`. So `APIGetBadgeState` could read worker *W* from the map, `stopChatController` could swap the map and cancel *W*, and the command thread would then block on `runWorkerAsync W` with nothing left to put the TMVar back. Swapping the map before cancelling — which `stopChatController`'s comment says fixes the next caller — does not close the in-flight window. The shape is pre-existing in the agent (`disconnectAgentClient` does the same over `smpDeliveryWorkers`) and **simplexmq is deliberately not changed**; what is new is the exposure, since every other `getAgentWorker` caller runs from a start or a background loop while this one runs synchronously on a user-facing command thread that G1 and G3 hit on every badge-screen open — on mobile, a hung `chatSendCmd`. The signal is now detached (`forkIO`), which turns the hang into a dropped signal: harmless, because the pass is idempotent and the worker's own timer re-fires.
+- **Phase C review — "unreachable via C1's `createPurchase`, which always inserts an opening credit" named a reason that does not exist. Corrected in place.** `Store/Badges.hs`'s `createPurchase` inserts the `badge_purchases` row and nothing else, and nothing in this milestone writes an opening credit at all (§9, above). The `issueDueBadgePeriod` case *is* unreachable, for a different reason: `redeemBadgeCode` is the only writer of a purchase row and of `users.shown_badge_id`, it rejects a response whose statement carries no issued period (`issuedBadgePeriod`) **before** it writes anything, and it inserts that statement's entries in the same transaction as the purchase row — so a shown purchase always has ledger rows. The comment now says that. A reason that does not hold is worse than no reason: the next reader would have looked for the opening credit.
+- **Phase C review — `UserBadge.monthsLeft` excludes the month currently issued, and nothing said so.** `badgePaidThrough` adds `balanceMonths` to `balanceStartTs`, and a successful issue moves `balanceStartTs` to the END of the period it just issued — so a freshly redeemed **3-month** code renders as "2 month(s) left" with `paidThrough` three months out. That is exactly what `threeMonthBadgeState` pins in C5, and the arithmetic is right: the pair is consistent, and it is `monthsLeft` alone that reads as if a month had been lost. Neither the computation nor the test expectation changed; `UserBadge`'s haddock now states it, and G4's and G5's steps carry the clause telling a UI to lead with `paidThrough`.
+- **Phase C review — the ledger parity check was asserted from one example, and now covers the worker's append path too.** `shouldMatchServiceLedger` compares §10's six columns with the entry type expanded into three, plus an explicit client `payment_id IS NULL` assertion, entry by entry with a naming label — but its only call site was `testC5RedeemCodeShowsBadge`, against a two-row ledger written by one transaction. The cursor-append path had no cross-database comparison at all, which is the one place the two codecs can drift on a row the two databases did not write together. `testC5WorkerIssuesSecondPeriod` now calls it after the second period is issued, against the three-row ledger, with the service row counts beside it. The helper is unchanged. Proved able to fail: mangling the appended `entry_uuid` in `storeBadgeIssueResponse` fails the new assertion at `("ledger entry 3", ...)` naming both uuids, while `testC5RedeemCodeShowsBadge` still passes — so the failure is the new call site's and not the old one's. Reverted.
+- **Phase C review — C2 minor 5, four draft types with no references, kept deliberately.** `BadgePayment` and `BadgeCharge` (`Badges/Types.hs`) have plausible D0/F1 consumers and are kept on that basis. `BadgeAlert` and `BadgeAlertKind` do not: they serve a feature §6 defers with **no scheduled step**, so nothing under this plan will pick them up, and the outcome recorded for them is *kept deliberately*, as core §3 drafts, not *to be deleted* — deleting them is out of this round's scope and would be a decision about the deferred feature rather than about this milestone. Recorded here so the set does not grow: a fifth unused draft should be argued for, not added.
+- **Phase C review — C2 minor 6 is CLOSED, not carried.** The carried risk was that `verifyUserBadge`'s free-form `Text` would make a C4 caller string-match to tell "unknown badge key index" from "does not verify". Both C4 call sites were checked: `storeRedeemedBadge` does `verifyUserBadge cred >>= either (throwChatError . localBadgeError) pure` and `storeBadgeIssueResponse` does `Left e -> badgeFailed e`, where `badgeFailed = ... localBadgeError`. Neither inspects the `Text`; both pass it straight through as the message of a `BSEInternal`. The third caller, `addUserBadge`, does the same through `throwCmdError . T.unpack`. Nothing string-matches, and nothing needs the distinction, so the signature stays as the C2 brief pinned it.
+- **B8 — argv is parsed twice on a plain service start, and `getBadgeServiceCommand` kept its name after its return type changed. Deferred to H.** The combined `CliCommand` parser and `welcomeGetOpts` in `Service.hs` each parse the process arguments; harmless today, and a latent trap, since the two must stay in lockstep or diverge silently. `getBadgeServiceCommand`/`badgeServiceCommand` still read as if they returned a badge-service command rather than a `CliCommand`; no stale references remain, only the names. Both belong with whichever H step next opens `Options.hs`.
+- **B9 — an unwritable path and a missing parent directory share one `IOException` catch. Deferred to H5.** `writeAddressFile`'s three deliberate failure modes are recorded above; what is not recorded there is that two of them are indistinguishable to the operator, who gets the same message either way while debugging a missing address file. H5 documents the operator procedure for this file and is where the distinction has to be made, in the message or in the README.
+- **Test harness — two Postgres harness flakes come from the pre-existing 5s `getTermLine` timeout, now hit about 15 times per run. Deferred, and the cause is named here so it is not re-diagnosed.** `getTermLine'` (`tests/ChatClient.hs`) waits a hardcoded `5000000` microseconds for a line from the virtual terminal. Under the `client_postgres` flag the badge suites are slow enough to exceed it on examples that pass on rerun. It is not a badge defect and not the echo artefact C5's `drainEcho` addresses — that one is about *which* line arrives, this one about *when*. Whoever raises it should raise the constant or make it configurable, not chase the examples.
+- **Phase C review — the C5 integration suite is skipped in CI, so no CI run exercises the client↔service chain.** `tests/Test.hs` registers it as `xdescribe'' "SimpleX Badge service bot"`, and `xdescribe''` skips when `CI=true`. That follows the precedent of the broadcast bot and the directory service beside it, both registered the same way, and the suite genuinely needs a local SMP server and a real service process. The cost is specific: **every** end-to-end assertion in it is skipped in CI, the ledger parity check above included, so a codec divergence introduced by a later step is not caught until someone runs the suite locally with `CI` unset. Run it that way before offering any badge step for merge; `cabal test --test-options='-m "Supporter badges" -m "Badge service"'` with `CI` unset is the command, and a run reporting far fewer than ~148 examples means `CI` was set.
+- **Phase C review — the generated clients carry no badge types, and §9's "belongs with C4" was not honoured.** `APIGetBadgeCatalog`, `APIGetBadgeState` and `APIPurchaseBadge` are in `undocumentedCommands`, `CRBadgeState` and `CRBadgeCatalog` in `undocumentedResponses`, and `CEvtBadgeChanged` in `undocumentedEvents` (`bots/src/API/Docs/{Commands,Responses,Events}.hs`) — all exemptions from `tests/APIDocs.hs`'s completeness check. So `bots/api/TYPES.md`, `types.ts` and `_types.py` carry only `BadgeServiceErrorCode` and `CEBadgeServiceError`, and `UserBadge`, `UserBadgeState`, `CRBadgeState`, `CRBadgeCatalog` and `CEvtBadgeChanged` reach no generated client. The C2 entry above said documenting the three commands "belongs with C4, when they do something"; C4 made them do something and did not document them. **Assigned to a G step** — G4 is the natural one, since it is the first step that reads `CRBadgeCatalog` and `CRBadgeState` field by field and would notice a wrong shape. Moving a constructor out of an `undocumented*` list regenerates `COMMANDS.md`, `EVENTS.md`, `TYPES.md`, `types.ts` and `_types.py`, and `describe "Bot API docs"` must pass afterwards.
+- **Phase C review — mobile cannot configure the badge service at all, which makes G2's and G3's Verify lines impossible as written. Assigned to G0.** `defaultChatConfig` sets `badgeServiceAddress = Nothing` and `badgeWebBaseUrl = ""`; `defaultMobileConfig` (`Mobile.hs`) overrides three unrelated fields and none of these; and `mobileChatOpts` hardcodes `optBadgeServiceAddress = Nothing`, `optBadgeWebUrl = Nothing` and `optBadgeIssuerKeys = []`. The three overrides exist only on the terminal CLI (`--badge-service-address`, `--badge-web-url`, `--badge-issuer-key`), so "verify manually against a locally run service" cannot be done on iOS or Android by any means the client offers. G0 is the assignment: it is the first mobile step, it lands before G2 and G3 need it, and until it does every mobile Verify line in Phase G is unrunnable. Whatever shape it takes — build-time constants, a debug-only setting, `mobileChatOpts` parameters — the three values must be reachable from a mobile build.
+- **Phase C review — `redeemBadgeCode` reads the reported badge state outside the badge lock. Accepted, not fixed.** The lock is released after `storeRedeemedBadge` and before `presentUserBadgeToContacts` (it must be: `presentUserBadgeToContacts` takes `chatLock`), and `getUserBadgeState` for the response runs after that. A concurrent redemption on the same profile could supersede this one in between, so the response would name the *other* purchase as shown. Cosmetic: both purchases are stored correctly, the rows are right, and the next `APIGetBadgeState` reports the truth. Fixing it would mean either computing the response inside the lock — which is a different value from the one the app will read next — or widening the lock over `chatLock`, which is the inversion C3 exists to avoid.
+- **Phase C review — two one-line inconsistencies fixed.** `hasIssuanceForPeriod` (`Store/Badges.hs`) used a bare string literal for its query where every other query in that module uses `[sql| |]`; it was the only one, and it is now the same as the rest. And `runBadgeWorker`'s haddock did not mention the `waitChatStartedAndActivated` gate the C3 review round added to the top of its loop, which is the one thing about that loop a reader most needs to know after a suspend; it now names it and the three sibling loops that gate the same way.
+
 ## 10. End-to-end verification
 
 After F5:
@@ -1519,7 +1540,7 @@ curl -X POST localhost:9000/_settle/<invoiceId>   # delivers a signed webhook
 Invariants that must hold:
 
 - for the same purchase, the client's and the service's `badge_ledger` rows agree on `entry_uuid`, `change_months`, `balance_months`, `balance_start_ts`, `balance_badge_type` and the entry type — **not** on every column: `entry_id` is a per-database IDENTITY that means nothing across databases, and `payment_id` is a service-minted UUID on the service side and NULL on the client's, which has no `@payments` row for a code redemption (B10, §9). Comparing whole rows will fail; compare those six columns.
-- re-sending the same `purchaseBadge` writes nothing and returns the same credential
+- re-sending the same `purchaseBadge` writes nothing and returns the same credential — a **service-side** invariant that the shipped client cannot exercise, and so is **not** part of the script above: the replay path keys on the purchase key, and `redeemBadgeCode` mints a fresh one on every call (§9), so a client retry arrives as a different signer and is answered `code_used`. It is verified by driving the service directly: B10's `testBadgeServiceRedeemCodeIdempotent` repeats the identical signed request under one key and pins both halves, and `testBadgeServiceReplayAfterLapseHealsOnce` pins the one row the replay may append
 - the same code from a second purchase key returns `code_used`
 - a replayed webhook creates no second code, and an unprocessed one is reprocessed
 - an invoice settling after expiry still yields its code