amadan

carlos / platform Public

pricing: fix the statements plan's test code, which did not compile

Round two caught that I had disposed of four findings and skipped five.
The prescribed test code used metrics.HourRollup and metrics.Stat,
neither of which exists: DayRollup.Hours is map[string]HourAgg and unit
samples are map[string]map[string]Agg. It also called approxF as though
it were a package helper returning bool, when it is a local closure
taking a name and calling t.Fatalf. And every unit-name literal was
missing the .service suffix that activator.UnitName actually produces,
so the census lookups would have silently never matched.

Task 3's extraction listed two substitutions and needed five: the
statement literal still carried UpdatedAt from the outer scope and
fleetMeters still took the old period bounds, either of which would
have given the previous month this month's data.

Census now decodes before counting and no longer sets the legacy
covered flag. foldStorageAndCensus deliberately excludes census from
coverage, and that number reaches the live economics view, so a month
of snapshots alone would have turned zero covered days into full
coverage.

The closure test now has a store seam that closes the month between the
read and the write, which is the interleaving two consoles actually
produce; the sequential test alone never exercised the race. Task 2
gains a test that drives censusPass rather than hand-writing its
output, and Task 3's commit stages the usage test it adds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 165c5101ceaf87c4157d3da2e21bf4474c1b0744 parent aa938bf
2 files changed, +214 −32
  • docs/superpowers/plans/2026-09-08-invoice-grade-statements.md +168 −32
  • docs/superpowers/plans/CODEX-FINDINGS-invoice-grade-statements-round2.md +46 −0
diff --git a/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md b/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md
index b32a3f3e3..495d19a84 100644
--- a/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md
+++ b/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md
@@ -356,9 +356,17 @@ The storage arm needs more than a flag (codex 4). A `StorageDay` that decodes cl
Then add a census arm immediately before the `if covered {` block at :433, so `app_months` has a completeness signal too, and accumulate strictly:
```go
- if _, _, gerr := s.cfg.Releases.Get(ctx, usage.CensusDayKey(date)); gerr == nil {
- censusOK = true
- covered = true
+ // Census deliberately does NOT set `covered`: the legacy covered-day
+ // sources are metrics/edge/storage only (foldStorageAndCensus says so
+ // at economicspass.go:516-517), and that number reaches the live
+ // economics view. A month of census snapshots alone must not turn
+ // zero covered days into full coverage. It must also DECODE, not
+ // merely exist — a malformed census is not a complete one.
+ if raw, _, gerr := s.cfg.Releases.Get(ctx, usage.CensusDayKey(date)); gerr == nil {
+ var cd usage.CensusDay
+ if jerr := json.Unmarshal(raw, &cd); jerr == nil {
+ censusOK = true
+ }
}
// Strict: every discovered box must have reported this day.
@@ -453,15 +461,16 @@ func TestSweepKeepsOwnerAfterRouteDeleted(t *testing.T) {
V: 1, Date: "2026-08-01",
Accounts: map[string]usage.CensusAccount{fixtureAcct: {Apps: 1, Routes: 1}},
Units: map[string]usage.CensusUnit{
- "carlos-app@a.example.com": {Account: fixtureAcct, App: fixtureApp},
+ "carlos-app@a.example.com.service": {Account: fixtureAcct, App: fixtureApp},
},
})
putStoreJSON(t, fx.rel, metrics.DayKey("box1", "2026-08-01"), metrics.DayRollup{
- Hours: []metrics.HourRollup{{
- Units: map[string]map[string]metrics.Stat{
- "carlos-app@a.example.com": {"cpu": {Avg: 0.5, N: 60}},
- },
- }},
+ V: 1, Box: "box1", Date: "2026-08-01",
+ Hours: map[string]metrics.HourAgg{
+ "00": {Units: map[string]map[string]metrics.Agg{
+ "carlos-app@a.example.com.service": {"cpu": {Avg: 0.5, N: 60}},
+ }},
+ },
})
// Day 2: the route is gone from the registry entirely — no Upsert here.
@@ -478,7 +487,7 @@ func TestSweepKeepsOwnerAfterRouteDeleted(t *testing.T) {
if err := json.Unmarshal(raw, &st); err != nil {
t.Fatal(err)
}
- if got := st.Meters[usage.MeterCPUCoreHours]; !approxF(got, 0.5) {
+ if got := st.Meters[usage.MeterCPUCoreHours]; math.Abs(got-0.5) > 1e-9 {
t.Errorf("cpu_core_hours = %v, want 0.5 — the deleted route's day must stay attributed", got)
}
}
@@ -486,6 +495,51 @@ func TestSweepKeepsOwnerAfterRouteDeleted(t *testing.T) {
**The fixture account will make this test pass either way, so do not use it.** `fixtureAcct` is `release.OpsAccount` (`economicspass_test.go:41-44`), which is exactly what `unitOwner` returns when it finds nothing — so a broken implementation and a correct one produce the same statement. Use a distinct account instead: seed the census `Units` entry with `{Account: "acme", App: fixtureApp}`, assert `usage.StatementKey("acme", "2026-08")` carries the 0.5, and assert the ops statement carries **zero** `cpu_core_hours`. Without both halves the test proves nothing.
+**Drive `censusPass` rather than hand-writing the census.** The test above seeds the object directly, which proves the sweep reads it but not that the pass ever writes it — a `censusPass` that populated `Units` wrongly, or not at all, would still pass. Add a second assertion path that exercises the real producer:
+
+```go
+// censusPass must record the unit names unitOwner would have matched, so
+// the sweep can still resolve them after the route is gone.
+func TestCensusPassRecordsUnitOwners(t *testing.T) {
+ fx := newConsoleFixture(t)
+ ctx := context.Background()
+
+ reg, err := registry.Open(fx.regPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := reg.Upsert(registry.Route{
+ Host: "a.example.com", Kind: "instance", Status: "ready",
+ App: fixtureApp, Account: "acme", Version: "v1",
+ }); err != nil {
+ t.Fatal(err)
+ }
+ reg.Close()
+
+ if err := fx.srv.censusPass(ctx, "2026-08-01"); err != nil {
+ t.Fatal(err)
+ }
+ raw, _, err := fx.rel.Get(ctx, usage.CensusDayKey("2026-08-01"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ var cd usage.CensusDay
+ if err := json.Unmarshal(raw, &cd); err != nil {
+ t.Fatal(err)
+ }
+ want := activator.UnitName("a.example.com") // carlos-app@a.example.com.service
+ cu, ok := cd.Units[want]
+ if !ok {
+ t.Fatalf("census has no entry for %q; units = %v", want, cd.Units)
+ }
+ if cu.Account != "acme" || cu.App != fixtureApp {
+ t.Errorf("census owner = %+v, want acme/%s", cu, fixtureApp)
+ }
+}
+```
+
+Note `activator.UnitName(host)` is `"carlos-app@" + host + ".service"` (`internal/activator/backend_unit.go:67`) — the `.service` suffix is part of the key, and a literal without it will silently never match.
+
Add a second test for the created-after-snapshot case the fallback protects:
```go
@@ -499,14 +553,15 @@ func TestSweepFallsBackForRouteMissingFromCensus(t *testing.T) {
putStoreJSON(t, fx.rel, usage.CensusDayKey("2026-08-01"), usage.CensusDay{
V: 1, Date: "2026-08-01",
Accounts: map[string]usage.CensusAccount{},
- Units: map[string]usage.CensusUnit{"carlos-app@other.example.com": {Account: "acme", App: "other"}},
+ Units: map[string]usage.CensusUnit{"carlos-app@other.example.com.service": {Account: "acme", App: "other"}},
})
putStoreJSON(t, fx.rel, metrics.DayKey("box1", "2026-08-01"), metrics.DayRollup{
- Hours: []metrics.HourRollup{{
- Units: map[string]map[string]metrics.Stat{
- "carlos-app@late.example.com": {"cpu": {Avg: 0.5, N: 60}},
- },
- }},
+ V: 1, Box: "box1", Date: "2026-08-01",
+ Hours: map[string]metrics.HourAgg{
+ "00": {Units: map[string]map[string]metrics.Agg{
+ "carlos-app@late.example.com.service": {"cpu": {Avg: 0.5, N: 60}},
+ }},
+ },
})
reg, err := registry.Open(fx.regPath)
@@ -532,7 +587,7 @@ func TestSweepFallsBackForRouteMissingFromCensus(t *testing.T) {
if err := json.Unmarshal(raw, &st); err != nil {
t.Fatal(err)
}
- if got := st.Meters[usage.MeterCPUCoreHours]; !approxF(got, 0.5) {
+ if got := st.Meters[usage.MeterCPUCoreHours]; math.Abs(got-0.5) > 1e-9 {
t.Errorf("acme cpu_core_hours = %v, want 0.5", got)
}
}
@@ -642,7 +697,7 @@ func (s *Server) dayOwnerFunc(ctx context.Context, date string, routes []registr
The asymmetry is the whole point, and it is worth being explicit about why it is safe. A route that existed at snapshot time is in the census, so deleting it later cannot move its usage. A route created after snapshot time is absent from the census but present in the registry, so it resolves correctly too. The only case still wrong is a route both created *and* deleted between two snapshots, which loses its usage to ops exactly as it does today — no better, no worse, and it needs the collection-time attribution the meters plan carries.
-Immediately after `date := usage.DayOf(d)` in the day loop (`:343`), add:
+Immediately after `date := usage.DayOf(d)` in the day loop (`:341`), add:
```go
owner := s.dayOwnerFunc(ctx, date, routes)
@@ -716,11 +771,12 @@ func TestStatementsPassUpdatesPreviousMonth(t *testing.T) {
ctx := context.Background()
putStoreJSON(t, fx.rel, metrics.DayKey("box1", "2026-08-31"), metrics.DayRollup{
- Hours: []metrics.HourRollup{{
- Units: map[string]map[string]metrics.Stat{
- "carlos-app@a.example.com": {"cpu": {Avg: 0.5, N: 60}},
- },
- }},
+ V: 1, Box: "box1", Date: "2026-08-01",
+ Hours: map[string]metrics.HourAgg{
+ "00": {Units: map[string]map[string]metrics.Agg{
+ "carlos-app@a.example.com.service": {"cpu": {Avg: 0.5, N: 60}},
+ }},
+ },
})
// 1 September: the August rollup only became readable now.
@@ -737,7 +793,7 @@ func TestStatementsPassUpdatesPreviousMonth(t *testing.T) {
if err := json.Unmarshal(raw, &st); err != nil {
t.Fatal(err)
}
- if got := st.Meters[usage.MeterCPUCoreHours]; !approxF(got, 0.5) {
+ if got := st.Meters[usage.MeterCPUCoreHours]; math.Abs(got-0.5) > 1e-9 {
t.Errorf("august cpu_core_hours = %v, want 0.5", got)
}
if st.Closed {
@@ -768,11 +824,12 @@ func TestStatementsPassClosesAfterGrace(t *testing.T) {
// Late data after closure must not move a closed month.
putStoreJSON(t, fx.rel, metrics.DayKey("box1", "2026-08-31"), metrics.DayRollup{
- Hours: []metrics.HourRollup{{
- Units: map[string]map[string]metrics.Stat{
- "carlos-app@a.example.com": {"cpu": {Avg: 0.5, N: 60}},
- },
- }},
+ V: 1, Box: "box1", Date: "2026-08-01",
+ Hours: map[string]metrics.HourAgg{
+ "00": {Units: map[string]map[string]metrics.Agg{
+ "carlos-app@a.example.com.service": {"cpu": {Avg: 0.5, N: 60}},
+ }},
+ },
})
if err := fx.srv.statementsPass(ctx, now.AddDate(0, 0, 1)); err != nil {
t.Fatal(err)
@@ -851,14 +908,30 @@ Move the rest of the old body into a new method whose signature is:
func (s *Server) statementsForMonth(ctx context.Context, month string, start, end, now time.Time, closing bool, routes []registry.Route) error {
```
-Inside it, compute `daysInMonth` from the period's own month rather than from `now`:
+Inside it, compute `daysInMonth` from the period's own month rather than from `now` — this is the substantive change, since the old code derived it from `now` and would give the previous month the wrong denominator:
```go
y, mo, _ := start.Date()
daysInMonth := time.Date(y, mo+1, 0, 0, 0, 0, 0, time.UTC).Day()
```
-and use `start`/`end` where the old body used `monthStart`/`nowUTC` for the sweep and fold calls.
+**Then substitute every remaining reference to the old locals. Miss one and it will not compile, or worse, will compile against the wrong period:**
+
+| Old body | New |
+| --- | --- |
+| `s.sweepUsage(ctx, monthStart, nowUTC, routes)` (:754) | `s.sweepUsage(ctx, start, end, routes)` |
+| `s.foldStorageAndCensus(ctx, monthStart, nowUTC, meters)` (:759) | `s.foldStorageAndCensus(ctx, start, end, meters)` |
+| `s.fleetMeters(ctx, monthStart, nowUTC)` (:762) | `s.fleetMeters(ctx, start, end)` |
+| `Month: month` in the statement literal | unchanged — `month` is now the parameter |
+| `UpdatedAt: nowUTC` (:813) | `UpdatedAt: now` |
+
+`reg` and `routes` come from the caller: `statementsPass` keeps `openPassRegistry` and `reg.List()` and passes `routes` in, so the registry is opened once for both periods rather than twice. `accounts` (`s.store.Accounts()` at :788) and the `universe` construction stay inside `statementsForMonth` unchanged — they do not depend on the period.
+
+After the move, confirm nothing was left behind:
+
+Run: `grep -n 'monthStart\|nowUTC' internal/console/economicspass.go`
+
+Expected: no hits inside `statementsForMonth`; `nowUTC` still appears in `statementsPass` (for the period arithmetic) and in `billPass`, which is untouched.
- [ ] **Step 5: Make closure immutable inside the CAS loop**
@@ -938,6 +1011,69 @@ func TestUpsertStatementRefusesClosedMonth(t *testing.T) {
}
```
+That covers the sequential case. **The race the finding is actually about needs the close to land between the Get and the Put**, which only a store seam can arrange. Add it:
+
+```go
+// closeDuringPut closes the statement the first time a Put is attempted, so
+// the writer's Put loses its CAS race and retries — and the retry's Get must
+// then see Closed and refuse. This is the interleaving two consoles produce.
+type closeDuringPut struct {
+ release.Store
+ fired bool
+ ctx context.Context
+ st Statement
+}
+
+func (c *closeDuringPut) Put(ctx context.Context, key string, body []byte, etag string) (string, error) {
+ if !c.fired {
+ c.fired = true
+ closed := c.st
+ closed.Closed = true
+ closed.Meters = map[string]float64{MeterInstanceHours: 10}
+ if err := UpsertStatement(c.ctx, c.Store, closed); err != nil {
+ return "", err
+ }
+ }
+ return c.Store.Put(ctx, key, body, etag)
+}
+
+func TestUpsertStatementLosesToAConcurrentClose(t *testing.T) {
+ ctx := context.Background()
+ base := release.NewDirStore(t.TempDir())
+ open := Statement{V: 1, Account: "acme", Month: "2026-08",
+ Meters: map[string]float64{MeterInstanceHours: 1}}
+ if err := UpsertStatement(ctx, base, open); err != nil {
+ t.Fatal(err)
+ }
+
+ s := &closeDuringPut{Store: base, ctx: ctx, st: open}
+ late := Statement{V: 1, Account: "acme", Month: "2026-08",
+ Meters: map[string]float64{MeterInstanceHours: 999}}
+ if err := UpsertStatement(ctx, s, late); err != nil {
+ t.Fatalf("a refused write is not an error: %v", err)
+ }
+
+ raw, _, err := base.Get(ctx, StatementKey("acme", "2026-08"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ var got Statement
+ if err := json.Unmarshal(raw, &got); err != nil {
+ t.Fatal(err)
+ }
+ if got.Meters[MeterInstanceHours] != 10 {
+ t.Errorf("instance_hours = %v, want 10 — a close between read and write must win",
+ got.Meters[MeterInstanceHours])
+ }
+}
+```
+
+Before writing this, confirm the `Put` signature and that `release.Store` is an interface a struct can embed:
+
+Run: `grep -n 'type Store interface' -A 12 internal/release/store.go`
+
+If `Put` differs, match it exactly — the embedding only works if the override's signature is identical.
+
Run: `GOCACHE=/tmp/claude-1001/gocache GOFLAGS=-mod=mod GOPROXY=off go test ./internal/usage/ -run TestUpsertStatementRefusesClosedMonth -count=1 -v`
Expected: FAIL before the change (999 overwrites 10), PASS after.
@@ -957,7 +1093,7 @@ Expected: all packages pass.
- [ ] **Step 8: Commit**
```bash
-git add internal/usage/statement.go internal/console/economicspass.go internal/console/economicspass_test.go
+git add internal/usage/statement.go internal/usage/passes_test.go internal/console/economicspass.go internal/console/economicspass_test.go
git commit -m "usage: close the month, and catch up the one before it
statementsPass derived one month from now and never revisited it, but
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-invoice-grade-statements-round2.md b/docs/superpowers/plans/CODEX-FINDINGS-invoice-grade-statements-round2.md
new file mode 100644
index 000000000..04eabe255
--- /dev/null
+++ b/docs/superpowers/plans/CODEX-FINDINGS-invoice-grade-statements-round2.md
@@ -0,0 +1,46 @@
+## 1. DISPOSITION TABLE
+
+**The plan at `aa938bf` is not executable as written.** This is a static review against that checkout; I did not modify files or run the proposed implementation.
+
+The checked-in round-one findings contain **six P1s, two P2s, and the anchor table**, rather than four P1s. All are accounted for below. “Plan” references mean `docs/superpowers/plans/2026-09-08-invoice-grade-statements.md`.
+
+| Round-one finding | Disposition | Revised text / remaining gap |
+|---|---|---|
+| #1 P1: closure races | **PARTIAL** | Plan:870 correctly enforces closure against each attempt’s ETag, but `TestUpsertStatementRefusesClosedMonth` at Plan:912 only tests sequential writes, not a closing write between GET and PUT. |
+| #2 P1: routes created after census lose ownership | **ADDRESSED** | `dayOwnerFunc` now falls through to the live registry when the unit is absent; the explicitly deferred collection-time attribution work remains separate. |
+| #3 P1: fleet/per-box completeness | **ADDRESSED** | Fleet meters now return no source, so platform coverage cannot certify them; platform metrics and edge coverage require every discovered box. |
+| #4 P1: unswept storage counted complete | **ADDRESSED** | Plan:340 rejects storage completeness when any app has `StoreUnswept`, and the new storage test uses the correct types and fields. |
+| #5 P1: prescribed tests do not compile | **NOT ADDRESSED** | Plan:460, :505, :719 and :771 still use nonexistent `metrics.HourRollup`/`metrics.Stat`; :481, :535 and :740 still call the unavailable `approxF`. |
+| #6 P1: extracted body retains undefined variables | **NOT ADDRESSED** | Plan:861 only prescribes sweep/fold substitutions; it still needs `UpdatedAt: now` and an explicit `s.fleetMeters(ctx, start, end)` replacement. |
+| #7 P2: ownership test passes through ops fallback | **PARTIAL** | Plan:487 now mandates a distinct account and an ops-zero assertion, but the test still does not exercise `censusPass` before deleting a real route, and its unit spelling remains artificial. |
+| #8 P2: malformed census counted complete | **NOT ADDRESSED** | Plan:359 still counts successful GETs without decoding, whereas `internal/console/economicspass.go:520` skips malformed census data. |
+| #9 P3: anchor/interface table | **PARTIAL** | Most corrections are present, but Plan:645 still locates `date := usage.DayOf(d)` at `:343`; its actual location is `internal/console/economicspass.go:341`. |
+
+The retained compile findings are directly confirmed by [`internal/metrics/rollup.go:38`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/metrics/rollup.go:38), where `Hours` is `map[string]HourAgg`, and [`internal/console/economicspass_test.go:336`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/console/economicspass_test.go:336), where `approxF` is a local three-argument closure returning nothing. The extracted statement literal still contains [`UpdatedAt: nowUTC` at economicspass.go:813](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/console/economicspass.go:813).
+
+## 2. NEW findings
+
+1. **[P1] The new fallback test fails even after its compile errors are repaired.**
+
+ [`Plan:507`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md:507) samples `carlos-app@late.example.com`, but the route inserted at Plan:516 has no explicit `Unit`. [`routeForUnit` at economicspass.go:229](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/console/economicspass.go:229) therefore requires a derived name; [`activator.UnitName` at backend_unit.go:67](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/activator/backend_unit.go:67) returns `carlos-app@late.example.com.service`. The proposed fallback consequently attributes this sample to ops, and the `acme` statement lookup fails.
+
+ **Fix:** use `activator.UnitName("late.example.com")`, adding the test-file import, or the exact `.service` string. `"acme"` itself is accepted by `release.ValidAccount`; account normalization is not the failure.
+
+2. **[P2] The census arm changes existing `CoveredDays` semantics.**
+
+ [`Plan:361`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md:361) sets `covered = true` for census alone. Existing code explicitly excludes census from coverage at [`economicspass.go:516`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/console/economicspass.go:516), and the shared sweep’s coverage reaches the live economics view at [`economicsview.go:630`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/internal/console/economicsview.go:630). A month containing only census snapshots therefore changes from zero covered days to full general coverage, despite no metrics, edge or storage measurements.
+
+ **Fix:** count successfully decoded census objects in `SourceDays[SourceCensus]` without setting the legacy `covered` flag; add a census-only regression test.
+
+3. **[P3] Task 3’s commit omits its new closure test.**
+
+ Step 5b adds `TestUpsertStatementRefusesClosedMonth` to `internal/usage/passes_test.go`, but [`Plan:960`](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/pricing-plans-spec/docs/superpowers/plans/2026-09-08-invoice-grade-statements.md:960) does not stage that file. Following the commands leaves the test outside the commit and subsequent PR.
+
+ **Fix:** include `internal/usage/passes_test.go` in Task 3’s `git add`.
+
+The remaining requested checks are sound:
+
+- **Per-box counting:** counters reset per day; `len(...) > 0` prevents zero discovered boxes from certifying coverage. All-time discovery conservatively blocks days before newly added boxes existed as well as days after retirement. The inclusive loops and previous-month endpoint have no off-by-one error.
+- **New tests:** `TestStatementsPassSourceDaysNeedsEveryBox`, `TestStatementsPassUnsweptStorageIsNotComplete`, and `TestUpsertStatementRefusesClosedMonth` match real signatures and available imports.
+- **CAS body and callers:** the proposed `UpsertStatement` body is valid Go with existing imports. Returning `nil` for a closed statement is safe for its only production caller, which checks only the error.
+- **Task 3 extraction:** removing the caller-side guard creates no additional dependency problem. The unresolved variable substitutions remain round-one finding #6.
\ No newline at end of file