Trades ledger misreads aggregated CurrencyTrade rows (same rollup defect as #131) #132

New issue

Closed

opened 2026-07-21 04:15:18 +00:00 by coilyco-ops · 2 comments

coilyco-ops commented

2026-07-21 04:15:18 +00:00

Member

Sibling of #131, discovered while fixing it. CurrencyTrade (via TradeAction) is also an AggregatableAction: the server's StatsAggregator merges rows older than its detail window into per-citizen hourly rollups where Count becomes the merged-event total, Time the max, and non-key columns (Buyer, Seller, ShopOwner, ItemUsed, NumberOfItems, CurrencyAmount unless summed) keep one arbitrary merged record's values.

In the cycle-14 snapshot, 79% of CurrencyTrade rows have Count > 1 (max 38) - so most of the ledger's history rows are rollups, and trades.py currently treats each as one trade with trusted buyer/seller/item/price. Consequences to audit:

Row-level ledger (/trade page, who-sold-what) - old rows attribute merged trades to one representative buyer/seller/item.
Unit prices computed as CurrencyAmount / NumberOfItems on a rollup row may divide mismatched aggregates.
fair_price / market history consumers of the same CSV inherit the distortion.
Needs source verification of which TradeAction fields carry SumInAggregateGrouping (summed on merge - valid) vs representative-only.

Fix shape mirrors #131: treat Count>1 rows as rollups, exclude them from per-item/per-party attribution (or bucket them as "aggregated older trades"), keep whatever fields are genuinely summed, and label the UI accordingly. Note the gather actions (HarvestOrHunt / ChopTree / DigOrMine) have NO per-row discriminator (their Count is biomass even on detail rows), so by_gathered species labels on old rows carry the same residual risk - documented here rather than fixed.

Sibling of #131, discovered while fixing it. `CurrencyTrade` (via `TradeAction`) is also an `AggregatableAction`: the server's StatsAggregator merges rows older than its detail window into per-citizen hourly rollups where Count becomes the merged-event total, Time the max, and non-key columns (Buyer, Seller, ShopOwner, ItemUsed, NumberOfItems, CurrencyAmount unless summed) keep one arbitrary merged record's values. In the cycle-14 snapshot, 79% of CurrencyTrade rows have Count > 1 (max 38) - so most of the ledger's history rows are rollups, and `trades.py` currently treats each as one trade with trusted buyer/seller/item/price. Consequences to audit: * Row-level ledger (`/trade` page, who-sold-what) - old rows attribute merged trades to one representative buyer/seller/item. * Unit prices computed as CurrencyAmount / NumberOfItems on a rollup row may divide mismatched aggregates. * `fair_price` / market history consumers of the same CSV inherit the distortion. * Needs source verification of which TradeAction fields carry `SumInAggregateGrouping` (summed on merge - valid) vs representative-only. Fix shape mirrors #131: treat Count>1 rows as rollups, exclude them from per-item/per-party attribution (or bucket them as "aggregated older trades"), keep whatever fields are genuinely summed, and label the UI accordingly. Note the gather actions (HarvestOrHunt / ChopTree / DigOrMine) have NO per-row discriminator (their Count is biomass even on detail rows), so `by_gathered` species labels on old rows carry the same residual risk - documented here rather than fixed.

coilyco-ops referenced this issue from a commit

2026-07-23 04:19:55 +00:00

fix: quarantine aggregated trade ledger rows

coilyco-ops closed this issue

2026-07-23 04:19:55 +00:00

coilyco-ops commented

2026-07-23 04:20:23 +00:00

Author

Member

WARDED_WORKFLOW: done ✅

details

Review: detailed-only attribution for CurrencyTrade hourly rollups; merged event and currency totals remain.
workflow: merge-remote-main; review summary: in-container review gate intentionally skipped (temporary ward default pending brokered QA)
Felt: a narrow shared-boundary correction that carried cleanly through downstream consumers.
Confidence: high — lint, 529 pytest, and MCP smoke passed.
Surprise: frontend install/test could not run because pnpm is unavailable or non-executable in this container.
Follow-up: gather-action species labels retain their documented rollup risk.

WARDED_WORKFLOW: done ✅ <details><summary>details</summary> Review: detailed-only attribution for CurrencyTrade hourly rollups; merged event and currency totals remain. workflow: merge-remote-main; review summary: in-container review gate intentionally skipped (temporary ward default pending brokered QA) Felt: a narrow shared-boundary correction that carried cleanly through downstream consumers. Confidence: high — lint, 529 pytest, and MCP smoke passed. Surprise: frontend install/test could not run because pnpm is unavailable or non-executable in this container. Follow-up: gather-action species labels retain their documented rollup risk. </details>

coilyco-ops commented

2026-07-23 04:20:28 +00:00

Author

Member

WARDED_WORKFLOW: reservation-released

release details

Run finished with WARDED_WORKFLOW: done ✅.

ward container reap released container engineer-codex-eco-app-132 (--harness codex): the terminal outcome supersedes the reservation, so a later redispatch no longer needs --override-reservation.

— Codex, via ward agent

WARDED_WORKFLOW: reservation-released <details><summary>release details</summary> Run finished with `WARDED_WORKFLOW: done ✅`. `ward container reap` released container `engineer-codex-eco-app-132` (`--harness codex`): the terminal outcome supersedes the reservation, so a later redispatch no longer needs `--override-reservation`. </details>  — Codex, via `ward agent`

coilyco-ops referenced this issue

2026-07-23 04:50:06 +00:00

Retrigger image publish for current main after Forgejo recovery #146

coilyco-ops referenced this issue

2026-08-12 23:46:13 +00:00

get_market returns markets: [] for every query, filtered or not #218

coilyco-ops referenced this issue

2026-08-12 23:46:18 +00:00

Trade totals disagree across get_economy, get_market, get_stores and get_trades (1,699 vs 22,891 vs 3,672 vs 526) #221