amadan
Per-line tax codes: the third review's fixes (a race test for the single read of a resumed finalize; the roll rule keyed to freeze; wording)
Opus P3-1 and Astra 1: a fake knob finalizes the invoice right after a resumed run reads it; the run completes with no billing_invoice_finalized_externally review, inside and past the window, and the double-read revert (mutant M33) dies on it. Opus P3-2: the roll rule is keyed to freeze (before any build whose freeze can write a two-code calculation runs anywhere) in the design, the plan, the dispositions, the comment and the merge's enablement text. The plan's exact shares, copied wellFormed and taxCodesForLines switch, the design's test list and the dispositions' one-code sentence corrected. Both reports and the review-three dispositions in the tree.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8 files changed,
+403
−13
docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md+3 −3docs/superpowers/specs/2026-10-03-per-line-tax-codes-design.md+3 −3docs/superpowers/specs/CODEX-FINDINGS-per-line-tax-codes-review3.md+193 −0docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md+12 −2docs/superpowers/specs/OPUS-REVIEW-per-line-tax-codes-review3.md+148 −0internal/console/billinginvoice.go+2 −2internal/console/billinginvoice_test.go+11 −3internal/console/billingtaxlifecycle_test.go+31 −0
CI passed — run details
diff --git a/docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md b/docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md| index 433a5e4cb..404cf7294 100644 |
| --- a/docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md |
| +++ b/docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md |
| @@ -28,17 +28,17 @@ In `internal/console` and `internal/plans`, a test file `billingtaxshape_fixture |
| - `usageByCode(statement, spans, codeOf func(plan) string, rule)`: per (code, statement meter) float sums over eligible days in date order (16a's proposal as the one rule body: the span containing 00:00 UTC of the day); one `billingBaseUnits` per sum; transfer meters added per code with the overflow check; returns the four axes' weights per code. `Days` absent: waived when every billed meter is zero, else `errBillingDays`. |
| - `apportion(amountMinor, weights map[code]int64) []share`: floor shares, largest remainder, byte-order ties, zero shares omitted, zero weight sum with a positive amount an error. |
| - `taxCodesForLines(...)`: returns `errBillingTaxCodeMixed` for two codes (today's behaviour); two unexported bodies, `splitByGrossUsage` and `lastSpansCode`, exist and are tested but are not called from `freeze`. |
| -- Tests: the attribution and apportionment lists in the design, including the fractional transfer boundary with exact shares, the shuffled-map determinism, and the end-to-end two-code calculation produced through a test seam (not through `freeze`), with its JSON shape, digest and lines. |
| +- Tests: the attribution and apportionment lists in the design, including the fractional transfer boundary asserted on the weights, the shuffled-map determinism, and the end-to-end two-code calculation produced through a test seam (not through `freeze`), with its JSON shape, digest and lines. |
| ## Task 4. The lifecycle and the older reader |
| - A test seam lets a test freeze a two-code calculation (the body chosen explicitly); the invoice run then creates two lines for one axis, with equal and unequal amounts; in-window replay and expired-window recovery for each split line, a paginated draft listing; a one-code month's journal unchanged. |
| -- `billingoldreader_test.go`: the old `wellFormed` and entry guard copied verbatim with a note of the commit they come from; a two-code record makes the old flow file `billing_invoice_record_mismatch`, write nothing to the provider and leave the bytes unchanged; the new reader blocked until the review is resolved; an unrelated second review keeps the month held after that. |
| +- The older reader as a harness (`olderReaderStep` in `billingtaxlifecycle_test.go`): the entry of `29ea6676` copied (the terminal skip, the decode, the account, month, money and top-level checks, the review filed by the run's own method), calling the shared `wellFormed`, unchanged since; a two-code record makes the old flow file `billing_invoice_record_mismatch`, write nothing to the provider and leave the bytes unchanged; the new reader blocked until the review is resolved; an unrelated second review keeps the month held after that. |
| - A mixed history that is waived, and one emitting one code only, through both readers. |
| ## Task 5. Records, at the merge |
| -On the launch branch: `ENABLEMENT.md` gains the activation rule (older invoice workers stopped before `taxCodesForLines` is switched; no rollback past activation; the generic rollback note does not apply after it); `OUTSTANDING.md` item 17 points at the build and the hold; `READINESS.md` names the branch. The design's "Not in this design" stays true. |
| +On the launch branch: `ENABLEMENT.md` gains the activation rule, keyed to `freeze` (before any build whose `freeze` can write a two-code calculation runs anywhere, every invoice worker is on a build containing the merge and older ones are stopped, and stopped before their accidental reviews are resolved; no rollback below that build afterwards; the generic rollback note does not apply past activation; 16d's conditions on a held month's release), and states the every-month tightening from the merge (a resumed finalize validates the draft for every month; a one-code draft that changed under a journaled finalize holds with `billing_invoice_lines_mismatch`); `OUTSTANDING.md` item 17 points at the build and the hold; `READINESS.md` names the branch. The design's "Not in this design" stays true. |
| ## Task 6. Mutants and dispositions |
diff --git a/docs/superpowers/specs/2026-10-03-per-line-tax-codes-design.md b/docs/superpowers/specs/2026-10-03-per-line-tax-codes-design.md| index c62646a3a..846cb7653 100644 |
| --- a/docs/superpowers/specs/2026-10-03-per-line-tax-codes-design.md |
| +++ b/docs/superpowers/specs/2026-10-03-per-line-tax-codes-design.md |
| @@ -1,6 +1,6 @@ |
| # Per-line tax codes for a month in which the plan changed |
| -Date: 2026-10-03. Status: design, revision 6, as built (the second code review, both accept with fixes, no P1 or P2, corrected: the body's shape covers the two proposals and not every conceivable answer; the shape rule is `taxShapeOK` beside `wellFormed`; the apportionment review is tested through the preparer seam; a resumed finalize validates the invoice it read and that tightening applies to every month. Revision 5 was as built after the first code review (the first code review, Opus and Astra both accept with fixes, corrected three claims here: activation is more than one line, the glue now exists uncalled; maps are not all iterated in sorted order, and need not be; a waived calculation has no lines. Revision 4 was built from (revision 3 accepted with fixes by Astra's confirming review: no P1; one P2, the month-end answer's daily-data exemption contradicted partial-month pricing, and two qualifications, the waiver sentence and the older worker's nonterminal records; corrected here. Revision 2 accepted with fixes by Astra: no P1; three P2, a daily-data requirement that would regress one-code months, the line identity not checked on every acceptance path, two readings of the transfer arithmetic; two P3, references not yet written, "the next pass" promised without its conditions; each answered below. Revision 1 rejected by Astra on two P1s, four P2s and a P3: the invoice engine's per-meter line identity, held months that cannot resume, an older worker's durable review, the day-eligibility rule, policy stated as mechanics, imprecise apportionment, the zero-usage statement; each answered below). Answers `OUTSTANDING.md` item 17 and prepares decision 16 (`DECISIONS-FOR-PAUL.md`), which is Paul's; nothing here chooses it. Branch `design-per-line-tax-codes`, off the launch branch at `29ea6676`. |
| +Date: 2026-10-03. Status: design, revision 6, as built and confirmed (the third code review, both accept with fixes, no P1 or P2: a race test for the single read, the roll rule keyed to `freeze`, wording; made before the merge). Revision 6 was written after (the second code review, both accept with fixes, no P1 or P2, corrected: the body's shape covers the two proposals and not every conceivable answer; the shape rule is `taxShapeOK` beside `wellFormed`; the apportionment review is tested through the preparer seam; a resumed finalize validates the invoice it read and that tightening applies to every month. Revision 5 was as built after the first code review (the first code review, Opus and Astra both accept with fixes, corrected three claims here: activation is more than one line, the glue now exists uncalled; maps are not all iterated in sorted order, and need not be; a waived calculation has no lines. Revision 4 was built from (revision 3 accepted with fixes by Astra's confirming review: no P1; one P2, the month-end answer's daily-data exemption contradicted partial-month pricing, and two qualifications, the waiver sentence and the older worker's nonterminal records; corrected here. Revision 2 accepted with fixes by Astra: no P1; three P2, a daily-data requirement that would regress one-code months, the line identity not checked on every acceptance path, two readings of the transfer arithmetic; two P3, references not yet written, "the next pass" promised without its conditions; each answered below. Revision 1 rejected by Astra on two P1s, four P2s and a P3: the invoice engine's per-meter line identity, held months that cannot resume, an older worker's durable review, the day-eligibility rule, policy stated as mechanics, imprecise apportionment, the zero-usage statement; each answered below). Answers `OUTSTANDING.md` item 17 and prepares decision 16 (`DECISIONS-FOR-PAUL.md`), which is Paul's; nothing here chooses it. Branch `design-per-line-tax-codes`, off the launch branch at `29ea6676`. |
| ## What happens today |
| @@ -42,7 +42,7 @@ It builds the mechanics every answer needs, keeps the hold exactly where it is u |
| ## Compatibility and the roll rule |
| -- **Roll order is not free.** An older invoice worker that reads a nonterminal two-code calculation (terminal records are skipped before the guard) passes today's money-only `wellFormed`, fails the top-level code check, and files `billing_invoice_record_mismatch` for the month: a durable, account-wide hold that then blocks the new worker too (`checkBillingHoldTx`), and that it can file again after an operator resolves it while it still runs. It does not invoice and does not rewrite the calculation, but it is not harmless. So: every console that can run the invoice pass is on a build with this change, and stopped if older, before `taxCodesForLines` stops returning the error; no older build is rolled back to afterwards; and the generic rollback note in `ENABLEMENT.md` ("an older binary runs against the migrated schema") does not apply past activation. Those sentences are a required edit to `ENABLEMENT.md` in the plan's last task, beside the retirement protocol's roll rule, before activation; until activation no two-code calculation exists, so the change itself rolls in any order. |
| +- **Roll order is not free.** An older invoice worker that reads a nonterminal two-code calculation (terminal records are skipped before the guard) passes today's money-only `wellFormed`, fails the top-level code check, and files `billing_invoice_record_mismatch` for the month: a durable, account-wide hold that then blocks the new worker too (`checkBillingHoldTx`), and that it can file again after an operator resolves it while it still runs. It does not invoice and does not rewrite the calculation, but it is not harmless. So: before any build whose `freeze` can write a two-code calculation runs anywhere, every console that can run the invoice pass is on a build containing this change, and older ones are stopped (and stopped before their accidental reviews are resolved, since a running one files them again); no older build is rolled back to afterwards; and the generic rollback note in `ENABLEMENT.md` ("an older binary runs against the migrated schema") does not apply past activation. Those sentences are a required edit to `ENABLEMENT.md` in the plan's last task, beside the retirement protocol's roll rule, before activation; until activation no two-code calculation exists, so the change itself rolls in any order. |
| - A finalize that was journaled and then lost is not replayed or renewed over a draft whose lines would not pass today: the resumed step reads the invoice once and, while it is a draft, validates that reading's lines (the metadata code among them) first; a finalize landing between reads cannot look like one made outside. This applies to every month from the merge, one-code months included: a one-code draft that changed under a journaled finalize now holds (`billing_invoice_lines_mismatch`) instead of being finalized. `ENABLEMENT.md` says so. |
| - A calculation frozen before this change is read exactly as before. The barrier trigger on `billing_months` reads `$.v` and is untouched; the version of a price-book calculation stays 2, a legacy one stays 1. |
| - Months already held: 16d, with its conditions (no other open review, the backoff, the twelve-month lookback). Nothing resolves a review but the operator door, and this design adds no automatic resolution and no handling for a held month outside the lookback. |
| @@ -56,7 +56,7 @@ It builds the mechanics every answer needs, keeps the hold exactly where it is u |
| - The apportionment: each axis's shares sum to its amount and the subtotal is unchanged; a code with all of an axis's usage gives one line; equal remainders broken by byte order; a tiny share rounding to zero omitted; a zero weight sum with a positive amount holding; overflow refused; shuffled map insertion giving identical bytes; a fractional boundary in one transfer meter (two fractions of a byte under one code, a whole gigabyte under the other) asserted on the weights (floor each meter, then add), which conservation alone would not catch. |
| - The invoice lifecycle for a two-code month, through a record frozen by hand: two lines for one axis with equal and with unequal amounts; for each split line an in-window replay (one more arrival under the same key) and an expired-window recovery (no arrival), with a paginated draft listing; recovery by the three metadata keys; the draft validated against both kinds; the POST answer and the draft listing each refused when `metadata[tax_code]` is missing or wrong on one line (the fake provider stores and returns the tax code and the metadata, which today it does not); the transmitted `tax_code` and `metadata[tax_code]` asserted to be the frozen line's; a one-code month's journal, request bytes and hashes unchanged; a mixed history that is waived, and one that emits one code only, through both reader generations. |
| - The older reader, as a harness copying the entry of `29ea6676` (the terminal skip, the decode, the account, month, money and top-level code checks, the review filed by the run's own method; `wellFormed` is the shared function, unchanged since): a two-code record makes it file `billing_invoice_record_mismatch`, write nothing to the provider, leave the calculation bytes unchanged, and block a subsequent new-reader run until the review is resolved; and with a second, unrelated review open on the account, the resolution of the tax-code review alone does not free the month. |
| -- `wellFormed` and `lineParams`: a code at both levels; at neither; a two-code calculation with one line lacking a code; a malformed code; a non-waived calculation with no lines. |
| +- `taxShapeOK` and `lineParams`: a code at both levels; at neither; a two-code calculation with one line lacking a code; a malformed code; a non-waived calculation with no lines; a waived one with lines. |
| - Mutants in a tree copy once the assertions above exist, each with its killing test named. |
| ## Not in this design |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-per-line-tax-codes-review3.md b/docs/superpowers/specs/CODEX-FINDINGS-per-line-tax-codes-review3.md| new file mode 100644 |
| index 000000000..47fbbb097 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-per-line-tax-codes-review3.md |
| @@ -0,0 +1,193 @@ |
| +The design dispositions hold in the implementation, subject to Task 5’s pending rollout records. I found no P1 or P2. Two P3 verification/documentation fixes remain. |
| + |
| +This was source-only review: no changes, tests, or network commands. References are at **6488c198**, except the explicitly identified launch commits. Unqualified Go filenames below are under `internal/console`. Test counterfactuals are assessed by inspection. |
| + |
| +**Design dispositions, finding by finding** |
| + |
| +| Design finding | Assessment at 6488c198 | |
| +|---|---| |
| +| Round 1 P1: meter-only operation identity | Holds. `lineKind`, POST validation, recovery and draft validation distinguish codes (`billinginvoice.go:994`, `:1036`, `:1048`, `:1144`). | |
| +| Round 1 P1: held months cannot resume automatically | Holds. No automatic resolution was introduced. Operator resolution, other account reviews, backoff and lookback remain separate conditions. | |
| +| Round 1 P2: older workers create durable account-wide reviews | Holds. Nonterminal per-line records fail the older top-level predicate. Account-wide enforcement remains at `billinginvoicestore.go:165`. Task 5 remains due. | |
| +| Round 1 P2: eligibility before attribution | Holds. Whole-day eligibility precedes span selection (`billingtaxsplit.go:87`). Full coverage gets a whole-month interval (`:51`). | |
| +| Round 1 P2: midnight, gross weighting and cancelled-month attribution are policy | Holds. These are dormant proposals; production `freeze` calls none of their bodies. | |
| +| Round 1 P2: precise apportionment | Holds. Chronological sums, separate meter floors, checked transfer addition and deterministic largest remainders are implemented (`billingtaxsplit.go:119`, `:168`). | |
| +| Round 1 P3: zero usage without `Days` | Holds. Missing days with zero billed meters are usable. The empty split now avoids requesting weights altogether (`billingtaxsplit.go:346`). | |
| +| Round 2 P2: universal daily requirement would regress one-code months | Holds. Existing pricing requirements are unchanged (`billingcalculation.go:183`); the full-month, nonzero-usage, no-days fixture remains literal. | |
| +| Round 2 P2: POST and draft identity checks | Holds. Both require the frozen line’s metadata code. Missing and wrong codes have meaningful negative tests. | |
| +| Round 2 P2: transfer arithmetic ambiguity | Holds. Each transfer meter is floored before its integer is added (`billingtaxsplit.go:126`). The fractional test checks weights, not shares. | |
| +| Round 2 P3: decision and rollout references | Decision refinements exist at the supplied launch commits. The operational rollout edit is still Task 5, not completed at this tip. | |
| +| Round 2 P3: unconditional “next pass” | Corrected. Decision 16d at `68b16c52:implementation/carloku-paid-launch/DECISIONS-FOR-PAUL.md:60` states other reviews, backoff and the twelve-month lookback. | |
| +| Round 3 P2: month-end exemption versus partial-month pricing | Holds. Partial-month pricing requires daily data before either body runs (`billingtaxsplit_test.go:301`). The month-end body adds no requirement. | |
| +| Round 3 qualifications: waivers and terminal records | Hold. Waivers require no lines (`billingcalculation.go:104`); terminal records are skipped before decoding and guards (`billinginvoice.go:332`). | |
| + |
| +**Second review, finding by finding** |
| + |
| +| Finding | Closed? | Would the relevant test pass without the fix? | |
| +|---|---|---| |
| +| Opus P3-1: duplicate invoice read; every-month tightening unstated | Code fix closed: `stepFinalizing` passes its invoice to `validateDraftLinesOf` (`billinginvoice.go:1223`). Design states the scope; ENABLEMENT remains pending. | **Yes, if the duplicate read is restored.** The existing corruption tests do not exercise the intervening finalization. They fail if validation itself is removed. See P3-1 below. | |
| +| Opus P3-2: weights requested for an empty split | Closed (`billingtaxsplit.go:346`). | **No.** The house month with usage and no days at `billingtaxsplit_test.go:348` would return `errBillingDays`. | |
| +| Opus P3-3: body signature described as supporting any answer | Closed substantively. Design and comments describe the two proposals and the additional work alternative answers require (`billingtaxsplit.go:280`). | Not a behavioral fix. Tests do not establish wording accuracy. | |
| +| Opus P3-4: one-line record rewritten with a stale digest | Closed. The final one-line calculation is marshalled and passed to `freezeBillingMonth` (`billingtaxlifecycle_test.go:395`). | Restoring the old SQL construction could still pass the lifecycle assertions: they do not independently recompute the digest. Closure rests on using the real freeze function, which computes it. | |
| +| Opus P3-5: top-level-code and transfer-overflow guards untested | Closed (`billingtaxsplit_test.go:337`, `:343`). | **No.** Removing either guard fails its new assertion. | |
| +| Astra 1: apportionment review mapping untested | Closed (`billingtaxlifecycle_test.go:426`). | **No.** Removing the mapping returns the apportionment error without `errBillingInvoiceHeld` or the required review. | |
| +| Astra 2: weak wrong-code recovery assertion; missing first-line replay | Closed (`billingtaxlifecycle_test.go:112`, `:175`). | Removing recovery’s code predicate now fails the unacknowledged-operation, no-sibling and exact-review assertions. The added first-line replay case passes the previously correct implementation, intentionally; it now detects failure to replay or a second mutation. | |
| + |
| +**Remaining findings, most severe first** |
| + |
| +1. **P3 — The duplicate-read regression has no distinguishing test.** |
| + |
| + **Location:** `billingtaxlifecycle_test.go:197`; fix at `billinginvoice.go:1223`; disposition at `docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md:59`. |
| + |
| + **Scenario:** Restore `r.validateDraftLines(ctx)` at the resumed-finalize call. All four cited cases still read a persistently corrupted draft and produce the expected lines-mismatch review. None makes a previously sent finalize land between invoice reads, so none detects the false `billing_invoice_finalized_externally` review identified by Opus. |
| + |
| + **Smallest acceptable fix:** Add a controlled provider case that returns a valid draft on the resumed read and makes the already-sent finalize complete immediately afterward. Assert successful recovery without an externally-finalized review. Restoring the second validation read must fail that test. The production fix itself is correct. |
| + |
| +2. **P3 — The plan still contains claims the second review explicitly required correcting.** |
| + |
| + **Location:** `docs/superpowers/plans/2026-10-03-per-line-tax-codes-plan.md:31`, `:36`, `:41`; related surviving wording at `docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md:72`. |
| + |
| + **Scenario:** A maintainer following Task 5 still sees activation described as switching `taxCodesForLines`, despite that function returning only a string or error. The same plan promises exact fractional-transfer shares and a copied old `wellFormed`, whereas the test checks weights and the harness calls the shared unchanged function. |
| + |
| + **Smallest acceptable fix:** Align the plan with revision 6: activation changes `freeze` to invoke the decided body through the glue; the fractional test asserts weights; the harness copies the entry checks and shares unchanged `wellFormed`. Include the every-month resumed-finalize tightening explicitly in Task 5. These corrections can accompany the launch records. |
| + |
| +**A. Byte identity** |
| + |
| +I found no valid production one-code input whose calculation JSON, digest, request JSON/hash or operation kind changes. |
| + |
| +- `OverageLine.TaxCode` and `MonthAllowance.Spans` are omitted when empty (`internal/plans/lines.go:30`, `internal/plans/month.go:87`). Existing field names and order remain. |
| +- `priceBillingCalculation` clears spans before either version’s return (`billingcalculation.go:174`). The only additional production caller of `AllowanceForMonth` is the dormant glue. |
| +- `InvoiceItemParams.TaxCodeMetadata` is omitted when empty, including from legacy requests whose currency is stripped before hashing (`internal/billingstripe/invoices.go:274`; `billinginvoice.go:895`). One-code forms gain no metadata field. |
| +- Daily validation and meter scaling were extracted without changing their checks or arithmetic. JSON map keys remain deterministically encoded. |
| + |
| +The fixture expectations are literal. There are **seven pricing fixtures and six frozen-shape fixtures**, not seven frozen one-code records (`billingtaxshape_fixtures_test.go:31`, `:145`). `legacy_two_codes` correctly has no frozen fixture; production holds it. The one-plan price-book fixture also supplies the nonzero-usage/no-days case. |
| + |
| +The second table exercises `lineParams`, `lineKind`, `draftParams` and `requestHash`. It reconstructs freeze’s code-setting step with empty statement evidence rather than executing complete evidence preparation and database freezing. That limits its coverage but does not invalidate its compatibility assertions. Source inspection cannot independently prove the claimed historical generator execution. |
| + |
| +**B. Tax shape and entry guard** |
| + |
| +The acceptance sets deliberately differ (`billingcalculation.go:94`; `billinginvoice.go:341`): |
| + |
| +| Record | Old guard | New guard | |
| +|---|---|---| |
| +| Valid top-level code, ordinary untagged lines | Accepts | Accepts | |
| +| Empty top-level code, every line tagged validly | Refuses | Accepts | |
| +| Waived, no lines, empty top-level code | Refuses | Accepts | |
| +| Nonwaived, no lines, valid top-level code | Accepts entry | Refuses entry | |
| +| Waived with lines, valid top-level code | Accepts entry | Refuses entry | |
| +| Valid top-level code plus line codes | Older decoder ignores line codes | Refuses | |
| + |
| +For the malformed empty-line/waived cases, `stepFrozen` already refused them with record-mismatch. Earlier rejection does not change valid produced records. |
| + |
| +`validTaxCode` preserves the old syntactic rule: `txcd_` plus at least one byte. It does not verify provider existence. Both-level, neither-level and malformed line requests are refused by `lineParams` (`billinginvoice.go:970`). Its checks are local to the supplied line; it does not call `taxShapeOK` for the entire calculation. `lineKind` retains old names for untagged lines and adds the code for tagged ones. |
| + |
| +**C. Identity through the engine** |
| + |
| +The relevant paths are consistent: |
| + |
| +- Pending replay compares request hashes before sending (`billinginvoice.go:753`). |
| +- POST checking requires the frozen line’s code (`:1036`). |
| +- Expired recovery matches meter, digest and code (`:1048`). |
| +- Draft validation binds provider item IDs to acknowledged operations of each line’s kind, then checks metadata, amounts, currency and subtotal (`:1142`). |
| +- `has_more` causes complete line listing; client pagination remains implemented in `internal/billingstripe/invoices.go:152`. |
| + |
| +Equal amounts do not collapse identity. I found no normal engine path acknowledging both an old-kind and a new-kind operation for one axis of the same frozen calculation. The stored shape determines all its kinds; older workers reject the per-line shape. |
| + |
| +One qualification matters: a mismatching POST response containing an object ID is **acknowledged as an existing object, then held** (`billinginvoice.go:834`). It is not permission to finalize. |
| + |
| +A tagged line lacking metadata is rejected on POST, recovery or draft validation, including resumed draft finalization. Already-finalized recovery does not revalidate historical line metadata. Nor does validation provide an atomic guarantee against provider-side changes after the read. Those are limits of the claim “checked on every acceptance path,” not a newly identified bypass in this delta. |
| + |
| +**D. Attribution** |
| + |
| +Eligibility remains the existing whole-paid-day rule (`billingcalculation.go:322`). Attribution applies that rule before selecting the half-open span containing midnight (`billingtaxsplit.go:87`). |
| + |
| +Concrete cases behave as intended: |
| + |
| +- A continuous 15:00 plan change retains the day and attributes it to the earlier plan under proposal 16a. |
| +- A 15:00 lapse excludes that partially covered day. |
| +- Full coverage, whose `Covered` is nil, receives the entire month as its eligible interval. |
| +- Repeated spans sharing a code accumulate together. |
| +- An eligible day with no containing span returns `errBillingDaysMismatch`; an unmapped selected plan returns `errBillingUnavailable`. |
| +- Missing days with zero billed meters yield no weights. Missing days with usage fail when weights are needed. |
| +- Partial months still require usable daily data before either proposed body. Whole-month month-end attribution adds no such requirement. |
| +- Waived empty splits now request no weights. |
| + |
| +Malformed daily data retains the previous date, finite/nonnegative-value and sum checks. These helpers allocate existing charges; they do not replace billed usage, `CountedDays`, allowances or prices. |
| + |
| +**E. Arithmetic** |
| + |
| +The implementation follows the specified order (`billingtaxsplit.go:119`, `:168`, `:239`): |
| + |
| +1. Accumulate each `(code, meter)` in date order. |
| +2. Floor each accumulated meter once. |
| +3. Add the two transfer integers per code, with overflow checking. |
| +4. Divide each existing axis amount using exact integer products, floors and largest remainders. |
| +5. Break ties by code bytes and omit zero shares. |
| + |
| +Negative inputs and overflowing weight totals are refused. A positive amount with zero total weight returns `errBillingTaxApportion`. Successful shares conserve each axis amount, hence the subtotal. Invoice-wide rounding is not rerun over expanded lines. |
| + |
| +Unsorted map iteration does not affect successful output: floating accumulation is chronological, final code ordering is explicit, and intervening sums are checked nonnegative integers. |
| + |
| +**F. Policy and the hold** |
| + |
| +Nothing chooses decision 16 or 16a–16d. `twoCodeCalculation` and both bodies have no production caller. `freeze` still returns the mixed-code error before persisting a calculation (`billinginvoice.go:487`). |
| + |
| +Under valid production configuration, one-code and mixed-code behavior is unchanged, including mixed histories that would otherwise waive. The previously noted lookup-order difference for an unconfigured later plan remains unreachable with the required configuration (`billingservice.go:136`). |
| + |
| +The last-span body implements one proposed answer to 16c; its comments now say so. Alternative weighting or cancellation-hold policy still requires implementation and review at activation. |
| + |
| +**G. Older reader and rollout** |
| + |
| +The harness copies the base entry predicate’s behavior (`billingtaxlifecycle_test.go:270`); `wellFormed` is shared and unchanged from the base. |
| + |
| +An older worker: |
| + |
| +- Refuses a **nonterminal** per-line record, including an empty-code waiver, and files `billing_invoice_record_mismatch`. |
| +- Leaves the frozen bytes unchanged and performs no invoice mutation. |
| +- Skips terminal `complete`, `waived` and `payment_failed` records before decoding. |
| +- Can reopen its review after resolution while it continues running. |
| + |
| +The review blocks authorizing work account-wide. Resolving it does not release the account while another review remains open. |
| + |
| +Task 5 must land in the same merge and state: |
| + |
| +- Before any build can **freeze per-line calculations**, upgrade every invoice worker or stop older ones. |
| +- After activation, prohibit rollback to an older reader and override `ENABLEMENT.md:206`’s generic older-binary statement. |
| +- Stop offending old workers before resolving their accidental reviews. |
| +- Preserve 16d’s other-review, backoff and lookback conditions. |
| +- From this merge, resumed-finalize draft validation applies to **every month**, including one-code months. |
| +- Mark OUTSTANDING item 17 as mechanics built but inactive, with the mixed-code hold and decision still outstanding; record the reviewed commit and actual gate/CI results in READINESS. |
| + |
| +**H. Tests and mutants** |
| + |
| +The new guard, empty-split, mapping and recovery assertions distinguish their intended regressions. Compatibility tests intentionally pass against the previous correct behavior; test-only improvements do not need a production failure to be useful. |
| + |
| +M14’s stated equivalence holds **under the production invariant**: ordered, disjoint spans and coverage come from the same `AllowanceForMonth`. After whole-day eligibility, the first overlapping span necessarily contains midnight. It is not equivalent for inconsistent helper inputs. |
| + |
| +Removing the M27 review mapping now fails the preparer-seam test by inspection. However, `logs/taxcodes-mutants-round3.txt` is absent from 6488c198. I cannot independently confirm the reported 32 executions or classify unseen mutation definitions. |
| + |
| +One concrete additional mutant would survive the cited regression tests: restoring the duplicate validation read, as described in P3-1. The one-code form of the resumed-finalize tightening also lacks a dedicated corruption case; its implementation is shared and unconditional. Real attribution-to-review integration through production `freeze` remains an activation obligation. |
| + |
| +**I. Delta and merge readiness** |
| + |
| +The delta closes the identified implementation problems and strengthens the two Astra verification gaps. It does not overturn either report’s conclusions about byte identity, prices, the hold, owner policy or older-reader refusal. It does overturn Opus’s claim that M27 cannot be tested before activation. |
| + |
| +I would require the distinguishing race test and the remaining wording corrections before landing. Decision 16 is **not** required. After those fixes, merge into `feat/carloku-paid-launch-20260916` as a merge commit with Task 5’s records included. If the test changes the branch tip, run the isolated gate and CI on that resulting tip; a green run on 6488c198 alone would not cover it. |
| + |
| +**Claims still overstated or contradicted** |
| + |
| +| Prior claim | Status | |
| +|---|---| |
| +| Opus claim 1: any answer fits one body | Corrected in design revision 6 and `billingtaxsplit.go:280`. | |
| +| Opus claim 2: activation switches `taxCodesForLines` | Corrected in launch decision 16 at `68b16c52:56`; still present in plan `:41`, disposition `:72` and `billinginvoice.go:317`. | |
| +| Opus claim 3: `wellFormed` gains the tax shape | Corrected in the design’s main description. Its test list still names `wellFormed` at design `:59`; the actual tested predicate is `taxShapeOK`. | |
| +| Opus claim 4: weights requested only when dividing | Now true: the empty split returns first. | |
| +| Astra: apportionment mapping cannot be tested today | Corrected, with a distinguishing test. | |
| +| Astra: each split line has in-window replay | Now true. | |
| +| Astra: old `wellFormed` copied | Corrected in design `:58`; still claimed in plan `:36`. | |
| +| Astra: fractional-transfer test asserts exact shares | Corrected to weights in design `:56`; still claimed in plan `:31`. | |
| +| Astra: decision says nothing is built; stale revision | “Nothing is built” is corrected at launch commit `68b16c52`. Its revision reference is 5, while this design is now revision 6. | |
| +| Disposition `:72`: nothing changes for a one-code month | True for bytes and pricing, too broad for behavior: resumed-finalize validation now also holds corrupted one-code drafts. | |
| +| Design `:46`: ENABLEMENT states the tightening | Still a Task 5 obligation; it is absent from this commit’s ENABLEMENT. | |
| + |
| +**Verdict: accept with fixes.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md b/docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md| index 4ddc5d8d3..9667dffae 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-per-line-tax-codes.md |
| @@ -65,8 +65,18 @@ Reports: `OPUS-REVIEW-per-line-tax-codes-review2.md`, `CODEX-FINDINGS-per-line-t |
| | Astra 2: the wrong-code recovery test accepted any error; the first line had no in-window replay case. | The recovery case asserts the `billing_invoice_unrecovered` review alone, the first operation unacknowledged and no sibling created; the first line's loss inside the window asserts one more arrival under its key. | | `TestAWrongMetadataCodeHoldsTheMonth`, `TestATwoCodeLinesLostAnswerIsRecoveredByItsCode` | |
| | Both: claims (`wellFormed` gains the shape; copied old `wellFormed`; exact shares; "the switch in `taxCodesForLines`"; decision 16's "nothing is built", revision 4). | Reworded in the design, this file, and decision 16 on the launch branch (`68b16c52`). | | | |
| +## Code review three (`6488c198`, confirming; Opus: accept with fixes, three P3; Astra: accept with fixes, two P3) |
| + |
| +Reports: `OPUS-REVIEW-per-line-tax-codes-review3.md`, `CODEX-FINDINGS-per-line-tax-codes-review3.md`. No P1 or P2; every second-review finding closed; both name the branch ready to merge once these are made, and neither requires another round. The gate on `6488c198` passed alone and CI passed. |
| + |
| +| Finding | Disposition | Where | Test | |
| +| --- | --- | --- | --- | |
| +| Opus P3-1, Astra 1: the single-read fix had no test that fails without it. | A fake knob finalizes the invoice right after a resumed run reads it; the run completes with no `billing_invoice_finalized_externally` review, inside and past the window; the double-read revert (mutant M33) dies on it. | `billingtaxlifecycle_test.go`, the fake | `TestAResumedFinalizeReadsTheInvoiceOnce` | |
| +| Opus P3-2 (required at the merge): the roll rule was keyed to `taxCodesForLines`, which can never express a two-code result. | Keyed to `freeze`: before any build whose `freeze` can write a two-code calculation runs anywhere; in the design, the plan, this file, the comment and the merge's `ENABLEMENT.md` text, with Opus's four points and Astra's six (older workers stopped before their accidental reviews are resolved; no rollback below the merge after activation; section 7's note overridden; 16d's conditions; the every-month tightening; OUTSTANDING 17 as built but inactive). | design, plan, this file, `billinginvoice.go`, Task 5 | | |
| +| Opus P3-3, Astra 2: wording the code contradicted (the plan's exact shares, copied old `wellFormed`, the switch in `taxCodesForLines`; the design's test list naming `wellFormed`; this file's "nothing changes for a one-code month"; decision 16's revision). | Each corrected; decision 16 cites revision 6 (`1bda76a7`). | | | |
| + |
| ## The build |
| -Tasks 0 to 4 and the mutants of Task 6 are committed; Task 5 (the records on the launch branch) comes at the merge. `wellFormed` kept its money shape and the tax shape is `taxShapeOK`, asked beside it at the entry guard, as the second review's note on "`wellFormed` alone" asked; the design's "gains the tax shape" is met by the guard, not by one function. Mutants: twenty-three in the first batch and twenty-nine in the second, in git-less tree copies, each with its killing tests logged (`taxcodes-mutants-round{1,2}.txt`); all die but M14, equivalent under the production invariant (spans and coverage from one `AllowanceForMonth`; not for arbitrary inconsistent helper inputs); M27 survived the second batch and dies in the third (`taxcodes-mutants-round3.txt`) on the preparer-seam test. Every one-code month's bytes, digest, request hashes and operation kinds are the ones pinned in Task 0 from the unchanged code. |
| +Tasks 0 to 4 and the mutants of Task 6 are committed; Task 5 (the records on the launch branch) comes at the merge. `wellFormed` kept its money shape and the tax shape is `taxShapeOK`, asked beside it at the entry guard, as the second review's note on "`wellFormed` alone" asked; the design's "gains the tax shape" is met by the guard, not by one function. Mutants: twenty-three in the first batch, twenty-nine in the second and thirty-three in the third, in git-less tree copies, each with its killing tests logged (`taxcodes-mutants-round{1,2,3}.txt`); all die but M14, equivalent under the production invariant (spans and coverage from one `AllowanceForMonth`; not for arbitrary inconsistent helper inputs); M27 survived the second batch and dies in the third (`taxcodes-mutants-round3.txt`) on the preparer-seam test. Every one-code month's bytes, digest, request hashes and operation kinds are the ones pinned in Task 0 from the unchanged code. |
| -What a reader of the invoice should know: a two-code month does not exist until decision 16 is taken and `taxCodesForLines` is switched, after the roll rule; nothing on this branch changes a one-code month, an amount, the floor, or the price book. |
| +What a reader of the invoice should know: a two-code month does not exist until decision 16 is taken and `freeze` is switched to the glue, after the roll rule; nothing on this branch changes a one-code month's bytes, digest, requests, amounts, the floor or the price book. One behaviour does change for every month from the merge: a resumed finalize validates the draft first, so a one-code draft that changed under a journaled finalize holds (`billing_invoice_lines_mismatch`) rather than being finalized. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-per-line-tax-codes-review3.md b/docs/superpowers/specs/OPUS-REVIEW-per-line-tax-codes-review3.md| new file mode 100644 |
| index 000000000..f818c1377 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-per-line-tax-codes-review3.md |
| @@ -0,0 +1,148 @@ |
| +# Third code review: `design-per-line-tax-codes` at 6488c198 (base 29ea6676) |
| + |
| +I found no P1 or P2. The branch is ready to merge into `feat/carloku-paid-launch-20260916` as a merge commit once CI is green on 6488c198. The gate log shows `gate exit 0 … on 6488c198`, with the darwin, linux/arm64 and windows cross-builds ok. I did not check CI. |
| + |
| +One condition: the merge's Task 5 records must key the roll rule to `freeze`, not to `taxCodesForLines` (P3-2 below). The other two findings are a test gap and wording, and can land with Task 5 or after it. |
| + |
| +The standard holds: |
| +- **One-code months:** byte-identical. Bytes, digest, request hashes and operation kinds are unchanged. |
| +- **Money:** no amount, floor or price changes. |
| +- **Decision 16:** nothing decides 16 or 16a–16d, and the hold is today's. |
| +- **Older binary:** it refuses a nonterminal two-code record with a review and never invoices it. |
| + |
| +## The design's dispositions against the code |
| + |
| +All three design rounds' dispositions hold. |
| +- **Round 1:** |
| + - The line identity is built. |
| + - There is no automatic resolution. |
| + - The older reader's behaviour is tested. |
| + - Eligibility comes before attribution. |
| + - 16a–16d are stated as the owner's. |
| + - The apportionment is exact. |
| + - Zero usage without `Days` behaves as stated. |
| +- **Round 2:** |
| + - One-code months gain no daily-data requirement. |
| + - The POST answer and the draft both check the metadata code. |
| + - Transfer meters are floored first, then added. |
| + - The references are written. |
| +- **Round 3:** the month-end answer adds no `Days` requirement, and a partly covered month without `Days` still can't be priced. |
| + |
| +## Second review, finding by finding |
| + |
| +| Finding | Closed at 6488c198? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1:** the resumed finalize read the invoice twice | Yes. `stepFinalizing` reads once (billinginvoice.go:1216) and validates that reading through `validateDraftLinesOf` (:1223, :1127). The every-month tightening is stated in design §Compatibility. | **Yes, it would pass.** No test lands a finalize between two reads, and no mutant reverts to `validateDraftLines(ctx)`. The disposition names `TestAResumedFinalizeValidatesTheDraftFirst`, but that test passes with the double read. See P3-1 below. | |
| +| **Opus P3-2:** the split body asked for weights with no lines | Yes (billingtaxsplit.go:346–350). | No. M30 dies on `TestTheGluesGuards` (:360). | |
| +| **Opus P3-3:** the body's shape was called "any answer" | Yes, in the design and the comments (billingtaxsplit.go:260–290). | n/a | |
| +| **Opus P3-4:** the one-emitted-code record was rewritten by SQL | Yes. It is frozen through `freezeBillingMonth` (billingtaxlifecycle_test.go:395–404). | Test-only fix. | |
| +| **Opus P3-5:** two guards untested | Yes. | No. M31 and M32 die. | |
| +| Opus claim 1: "any answer is one body" | Corrected. | | |
| +| Opus claim 2: "the switch in `taxCodesForLines`" | **Partly.** Corrected in decision 16 (68b16c52:56) and design §6. It survives in design §Compatibility (:45), plan Task 5 (:41), DISPOSITIONS (:72) and the comment at billinginvoice.go:317 (P3-2 below). | | |
| +| Opus claim 3: "`wellFormed` gains the tax shape" | Corrected. A new slip in design §1, "asked … and in `lineParams`", is listed in the claims section below. | | |
| +| Opus claim 4: weights asked for "only if the body divides" | Now true. | | |
| +| **Astra 1:** the apportionment review mapping was untested | Yes. `TestAnUnattributedChargeHoldsTheMonthForAPerson` checks the exact review, that nothing is frozen and that nothing reaches the provider. | No. M27 now dies. | |
| +| **Astra 2:** the wrong-code recovery test accepted any error; no in-window first-line replay | Yes. The recovery case asserts that the operation is not acknowledged, that the sibling was not created, and the `billing_invoice_unrecovered` review alone. If the recovery predicate (:1048) is removed, recovery acknowledges the wrong-coded item and the test fails at :179. The in-window first-line case asserts one more arrival, and `assertMutations` asserts one mutation; the fake only counts a mutation for a new key, so the key is the same. | No. | |
| +| Astra's overstated claims | Mostly corrected: design :41 and :57, design :58 (the copied `wellFormed`), design :56 (shares versus weights), and decision 16's "nothing is built". Plan :31 ("exact shares") and plan :36 (`billingoldreader_test.go`, "old `wellFormed` … copied verbatim") are unchanged. | | |
| + |
| +**Does the delta since 8715e4cd change a conclusion?** No. |
| +- **Production changes:** the delta touches only two production files. |
| + - `validateDraftLinesOf` is a pure extraction. `validateDraftLines` still does one read and calls it, so its other callers behave identically. |
| + - `stepFinalizing` now has strictly fewer false reviews than before. |
| + - `bodySplitByGrossUsage` is uncalled. |
| +- **Unaffected:** one-code bytes, the hold and the older reader are untouched. The Task 0 fixtures file is unchanged since 8715e4cd. |
| + |
| +## Findings |
| + |
| +**P3-1. The single-read fix has no test that fails without it** (billinginvoice.go:1216–1226) |
| +- **Scenario:** change :1223 back to `r.validateDraftLines(ctx)`, which reads the invoice again. Every test still passes, and the round-3 mutant log has no such mutant. |
| +- **Effect:** the fix is correct by inspection. The disposition's test column overclaims. |
| +- **Smallest fix:** a fake hook that turns the invoice `open` after the first `GetInvoice` in finalizing. Assert there is no `billing_invoice_finalized_externally` review and the month reaches `open` or `complete`. Alternatively, mark the disposition "by inspection". |
| + |
| +**P3-2. The roll-rule trigger is still keyed to `taxCodesForLines`** (design :45; plan :41; DISPOSITIONS :72; billinginvoice.go:317) |
| +- **Why it's wrong:** `taxCodesForLines` returns one string, so it can never express a two-code result. Activation can leave it unchanged and catch `errBillingTaxCodeMixed` in `freeze`. In that case "before `taxCodesForLines` stops returning the error" never happens, and an ENABLEMENT sentence copied from plan Task 5 never triggers. |
| +- **Smallest fix:** in the Task 5 text, key it to "before any build whose `freeze` can write a two-code calculation". Reword the four places above to match. |
| +- **This one is required at the merge,** because the merge carries the ENABLEMENT text. |
| + |
| +**P3-3. Wording the code contradicts.** See "Claims the code contradicts" below. Doc-only; can land with Task 5. |
| + |
| +## A–I |
| + |
| +**A. Byte identity.** I found no input that changes a one-code calculation. |
| +- **New fields:** `OverageLine.TaxCode`, `MonthAllowance.Spans` and `InvoiceItemParams.TaxCodeMetadata` are all `omitempty`. The last sits before `Currency` in field order, so an empty value moves nothing. |
| +- **Spans:** `priceBillingCalculation` sets `allowance.Spans = nil` before every return (billingcalculation.go:174). `AllowanceForMonth` has no other caller. |
| +- **Metadata:** `metadata[tax_code]` is sent only when the field is set (billingstripe/invoices.go:295). |
| +- **Hashing:** `requestHash`'s legacy strip is unchanged, and `lineKind` is `line-<meter>` for an untagged line. |
| +- **Extractions:** `checkedStatementDays` and `billingMeterScale` keep every check, its order and its errors. |
| +- **Map ordering:** JSON map keys are sorted. |
| +- **Task 0 fixtures:** right and literal. Both tables were recorded at 29ea6676 (5429b690 and 8715e4cd) and have not changed since. M11, M12 and M13 die on them. |
| + |
| +**B. Shape and guard.** |
| +- **New guard refuses what the old accepted:** a non-waived record with no lines, and a waived record with lines. Neither can be produced: a non-waived total is at least the floor, so it has lines. `stepFrozen` already refused both. |
| +- **New guard accepts what the old refused:** the two-code shapes, including waived with no code. A waived one goes to `waived` with no provider call. This is intended and tested. |
| +- **Bad codes:** both `taxShapeOK` and `lineParams` refuse a code at both levels, at neither, or a malformed code (M1, M2, M3, M5). |
| +- **`lineKind`:** it can't collide with an old kind. |
| +- **Note:** `taxShapeOK` doesn't require unique (meter, code) pairs. A duplicate can't come from `apportion`, and if one were stored by hand the shared kind would leave the draft sum short of the total and hold with `billing_invoice_lines_mismatch`. Not a finding. |
| + |
| +**C. Line identity.** |
| +- **One shape per record:** the record is frozen before any operation, and the digest is checked on every `reload`. Its shape decides every kind, and an older worker refuses the two-code shape at entry. So no path acknowledges an old-kind and a new-kind operation for the same axis. |
| +- **Code checked everywhere:** the POST answer (:1036), recovery (:1048), paged listing and draft validation (:1154) all require the code. Equal amounts are told apart by it. |
| +- **No metadata, no finalize:** a line without metadata is caught at the POST answer, the draft, and the resumed finalize before it reaches finalization. |
| + |
| +**D. Attribution.** |
| +- **Eligibility:** uses the same whole-day predicate as `billingUsageOnPaidDays`. A fully covered month (`Covered` nil, `!Partial`) takes the whole month. |
| +- **Spans:** half-open; the first-instant rule is labelled as 16a's proposal; two spans of one code aggregate. |
| +- **Edge cases:** |
| + - A day no span holds is unreachable, and returns `errBillingDaysMismatch` anyway. |
| + - A plan with no code returns `errBillingUnavailable`. |
| + - `Days` absent with zero usage gives no weights; absent with usage gives `errBillingDays`. Malformed `Days` fail as today. |
| +- **Unchanged:** billed days, `CountedDays`, usage and amounts. |
| + |
| +**E. Arithmetic.** |
| +- **Weights:** per (code, meter), summed in date order and floored once. Transfer is added per code with the overflow check (now tested by M32). |
| +- **`apportion`:** |
| + - exact `big.Int` arithmetic, floor, then largest remainder with a stable sort over byte-sorted codes; |
| + - zero shares are omitted; |
| + - a negative amount or weight, zero weight with a positive amount, and a wrapped sum are all errors. |
| +- **Determinism and subtotal:** the result doesn't depend on map order, and shares sum to each axis's amount, so the subtotal can't change. Nothing rounds beyond the axis. |
| + |
| +**F. Policy.** |
| +- **The hold:** `taxCodesForLines` is today's loop, so one code is carried as before and two hold with `billing_invoice_tax_code_mixed`. The only difference is an unconfigured plan being looked up before the mix check, which the configuration makes unreachable. |
| +- **The bodies:** both are uncalled and labelled as proposals. `bodyLastSpansCode` is now described as conditional on 16c. |
| + |
| +**G. Older reader and roll rule.** |
| +- **The harness:** its predicate (billingtaxlifecycle_test.go:282) is 29ea6676:336 verbatim, and `wellFormed` is unchanged. |
| +- **What an older binary does:** |
| + - A nonterminal two-code record, waived included, gets `billing_invoice_record_mismatch` before any provider call. |
| + - A terminal record is skipped. |
| + - The review holds the whole account, and the older binary can file it again after it is resolved. Both are tested. |
| +- **What ENABLEMENT.md must say at the merge:** |
| + 1. Before any build whose `freeze` can write a two-code calculation, every console that can run the invoice pass is on a build containing this merge, and older ones are stopped. |
| + 2. No rollback below that build afterwards. |
| + 3. Line 206 ("an older binary runs against the migrated schema") does not apply past activation. |
| + 4. From this merge, a resumed finalize validates the draft for every month. A one-code draft that changed under a journaled finalize now holds with `billing_invoice_lines_mismatch`. |
| +- **Also in the merge:** OUTSTANDING item 17 points at the build and the hold; READINESS names the branch. Until activation the merge rolls in any order. |
| + |
| +**H. Tests and mutants.** |
| +- **Kills:** M1–M13 and M15–M32 each die on a named test. |
| +- **M14 is equivalent for the reason given:** spans are ordered and disjoint, eligibility comes first, and the span holding a day's first instant is the first span that overlaps that day. |
| +- **Would survive:** the double-read revert in P3-1. |
| +- **Unreachable guards a mutant would survive:** the glue's final `taxShapeOK`, `len(Spans)==0`, and `apportion`'s `extra` and `IsInt64` bounds. That's acceptable. |
| +- **Untested:** the one-code side of the every-month tightening. Its code path is the same one the two-code tests exercise. |
| + |
| +**I. Before the merge.** Required: |
| +- CI green on 6488c198 (the gate already passed alone on it); |
| +- Task 5's records worded as in G, keyed to `freeze` (P3-2). |
| + |
| +Nothing else, and decision 16 is not needed. P3-1 and P3-3 can land with Task 5 or after. |
| + |
| +## Claims the code contradicts |
| + |
| +1. **The roll-rule trigger** (design :45, plan :41, DISPOSITIONS :72, billinginvoice.go:317): "before `taxCodesForLines` stops returning the error" / "`taxCodesForLines` is switched". Activation is in `freeze` (P3-2). |
| +2. **Design §1:** "`taxShapeOK`, asked beside `wellFormed` at the entry guard and in `lineParams`". `lineParams` doesn't call it; it has its own equivalent check (billinginvoice.go:973–983). |
| +3. **Design §Tests:** "its review resolved by the operator door and the month priced on the next pass once a body is enabled in the test". No test enables a body through `freeze`; that is the activation change's test. |
| +4. **DISPOSITIONS row for Opus P3-1:** names `TestAResumedFinalizeValidatesTheDraftFirst`. That test doesn't exercise the double read (P3-1). |
| +5. **Plan Tasks 3 and 4** (:31, :36): "exact shares", `billingoldreader_test.go`, "the old `wellFormed` and entry guard copied verbatim". The test asserts weights, the file is `billingtaxlifecycle_test.go`, and `wellFormed` is shared. The DISPOSITIONS row says these were reworded, but the plan was not. |
| +6. **Revision numbers:** the DISPOSITIONS header cites design revision 4 and decision 16 (68b16c52:56) cites revision 5. The design is revision 6. |
| + |
| +**Verdict: accept with fixes.** The one required fix is the `freeze`-keyed wording in the merge's own Task 5 records. |
diff --git a/internal/console/billinginvoice.go b/internal/console/billinginvoice.go| index 4ef5db72e..ebdfdb06d 100644 |
| --- a/internal/console/billinginvoice.go |
| +++ b/internal/console/billinginvoice.go |
| @@ -314,8 +314,8 @@ func (s *Server) invoiceBillingMonth(ctx context.Context, acct *Account, month s |
| return r.review("billing_invoice_tax_code_mixed") |
| } |
| // A two-code month whose charge has no attributed usage to divide it |
| - // by (reachable only once decision 16 switches taxCodesForLines) is a |
| - // hold for a person, not a quiet retry. |
| + // by (reachable only once decision 16 switches freeze to the glue, |
| + // twoCodeCalculation) is a hold for a person, not a quiet retry. |
| if errors.Is(err, errBillingTaxApportion) { |
| return r.review("billing_invoice_tax_code_mismatch") |
| } |
diff --git a/internal/console/billinginvoice_test.go b/internal/console/billinginvoice_test.go| index d2a925059..894592816 100644 |
| --- a/internal/console/billinginvoice_test.go |
| +++ b/internal/console/billinginvoice_test.go |
| @@ -51,9 +51,13 @@ type fakeInvoiceStripe struct { |
| // swapItemMetaTax and swapLineMetaTax report the other code in |
| // metadata[tax_code]: a provider that kept the wrong one. |
| swapItemMetaTax, swapLineMetaTax bool |
| - listDuplicate bool |
| - hasMoreLines bool |
| - seq int |
| + // openAfterRead makes the next read of a draft invoice answer "draft" |
| + // and then finalize it: a finalize sent by a dead process landing |
| + // right after a resumed run has read the invoice. |
| + openAfterRead bool |
| + listDuplicate bool |
| + hasMoreLines bool |
| + seq int |
| // The fake is in whatever currency each request named, as the |
| // provider is. currencies records what each kind of request named. |
| // The report* fields make it answer in another currency than it was |
| @@ -335,6 +339,10 @@ func (f *fakeInvoiceStripe) serve(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| out = f.invoiceJSON(inv, false) |
| + if f.openAfterRead && inv.status == "draft" { |
| + inv.status = "open" |
| + f.openAfterRead = false |
| + } |
| default: |
| w.WriteHeader(http.StatusNotFound) |
| return |
diff --git a/internal/console/billingtaxlifecycle_test.go b/internal/console/billingtaxlifecycle_test.go| index 23476406e..166c512b6 100644 |
| --- a/internal/console/billingtaxlifecycle_test.go |
| +++ b/internal/console/billingtaxlifecycle_test.go |
| @@ -441,3 +441,34 @@ func TestAnUnattributedChargeHoldsTheMonthForAPerson(t *testing.T) { |
| t.Fatalf("the provider was written: %v", mutations) |
| } |
| } |
| + |
| +// The resumed finalize validates the invoice it read, not a second |
| +// reading: a finalize from the dead process that lands between the read |
| +// and the validation must not look like one made outside this engine. The |
| +// draft is sound, the read says draft, the invoice is open by the time |
| +// anything else looks; the month completes without a |
| +// billing_invoice_finalized_externally review. |
| +func TestAResumedFinalizeReadsTheInvoiceOnce(t *testing.T) { |
| + for name, later := range map[string]time.Duration{"inside the window": 2 * time.Hour, "past the window": 25 * time.Hour} { |
| + t.Run(name, func(t *testing.T) { |
| + ix := taxLifecycleFixture(t) |
| + storeTwoCodeMonth(t, ix, func(total int64) int64 { return total / 2 }) |
| + ix.fake.dropRequest["finalize"] = true |
| + if err := ix.run(t); !errors.Is(err, errBillingPending) { |
| + t.Fatalf("a finalize that never left: %v", err) |
| + } |
| + delete(ix.fake.dropRequest, "finalize") |
| + ix.fake.openAfterRead = true |
| + ix.now = ix.now.Add(later) |
| + if err := ix.run(t); err != nil { |
| + t.Fatalf("resumed as the dead process's finalize landed: %v", err) |
| + } |
| + if reviews := ix.reviews(t); len(reviews) != 0 { |
| + t.Fatalf("reviews: %v", reviews) |
| + } |
| + if rec := ix.month(t); rec.State != "complete" && rec.State != "open" { |
| + t.Fatalf("month: %+v", rec) |
| + } |
| + }) |
| + } |
| +} |