#158 Sign convention follow-ups: stale #64 comment in gold_consolidated_trial_balance, state the adapter contract once, retire generate_demo_data.py

[Anonymous]

Originally created by: grynn-in

Decision record. Prompted by [#155] and by the decision to discard demo data and load only through the F8 upload path.

There is already one convention. Two of the three places that state it have rotted.

The convention, as the working code implements it:

Signed at the boundary, split once in silver, never inferred again.

1. Every source adapter emits a signed amount: positive = debit, negative = credit. Whatever the source encodes it as — magnitude plus a flag, two columns, a natural sign — is resolved in the adapter, once.
2. silver_gl_entries splits that signed amount into non-negative debit_amount / credit_amount.
3. Everything downstream uses period_debit - period_credit, which is period_net_amount. Signed. Never abs(), never re-derived from anything else.
4. The invariant that makes it checkable: every voucher nets to zero when read signed.

Nothing needs inventing. What needs doing is stating it once and stopping the three statements from disagreeing again.

Where it is stated today, and the state of each

Place	Says	State
models/silver/silver_gl_entries.sql:38-47	"positive = debit, negative = credit … the raw signed amount provably balances (every voucher nets to 0), so the sign is the source of truth"	Correct, but its premise is false for D365. The demo generator writes magnitude + IsCredit, so nothing arrives negative.
models/staging/d365_fo/stg_d365_fo__gl_entries.sql:38	entries.AccountingCurrencyAmount as amount	Broken (#155). Drops IsCredit, so every credit enters as a debit. PR [#157] fixes it.
models/gold/gold_consolidated_trial_balance.sql:43-45	"NOT period_net_amount, which is a positive magnitude (see [#64])"	Stale. konsol's patch update_period_net_measure_expression.py redefined period_net_amount as sum(debit_amount) - sum(credit_amount) — which is exactly period_debit - period_credit. The two are now identical. The comment warns against a hazard that no longer exists and tells the next reader the measure means something it does not.

That third one is the same failure mode as the konsol.currency_sync comment fixed in [#150]: a comment describing a design that was superseded, left in place to mislead the next person.

The upload path already implements the convention correctly

silver_gl_entries.sql, the Trial Balance Submission branch:

{# a TB row carries explicit debit and credit columns — no sign derivation #}
tbs.credit_amount as credit_amount,
tbs.debit_amount  as debit_amount,

A TB CSV states debit and credit explicitly, so there is no sign to infer and #155's bug class is impossible on that path by construction. That is the strongest argument for the upload-only decision, and it is worth saying out loud rather than treating as a side effect.

The demo-data decision is already in motion

• #156 (merged) deleted clickhouse/demo-data.sql and replaced it with raw-schema.sql — DDL only, no rows. Its credits were unsigned, i.e. it was a carrier of [#155].
• konsol#127 (merged) removed the Contoso/Alpine fixtures.
• scripts/generate_demo_data.py still exists but nothing loads its output.
• Existing stacks keep their rows until the volume is wiped.

So the remaining question is not whether, it is what the upload format must carry — and that is the part with a hole in it.

The hole: upload-only means no dimensions at all

Trial Balance Submission hardcodes every dimension to ''. If the entire demo loads through it:

Capability	State under upload-only
Cost centre / department / business unit	Empty on every row, every entity
Allocation engine	No input — it groups gold_trial_balance by cost centre
Dimension harmonization (konsol#111)	No demo case exists
Drill-down	One synthetic line per account per period; transaction count always 1
IC elimination (#148)	Stays at 0 rows — a TB row carries no counterparty

konsol#113 is therefore a prerequisite, not a nice-to-have. Its TB_MGMT file (P&L only, division mandatory) and partner_data_area_id on IC accounts are exactly the two columns that close this. Shipping upload-only demo data before [#113] leaves the product unable to demonstrate allocations, harmonization, drill-down or eliminations.

Proposed

1. Land [#157]. The D365 adapter serves real customers whether or not the demo exercises it. Its test — every voucher nets to zero at staging — is the right enforcement point and should exist per ERP source, so a future adapter cannot repeat [#155].
2. Fix the stale comment in gold_consolidated_trial_balance.sql:43-45. period_net_amount and period_debit - period_credit are the same thing now; say so, and drop the [#64] warning.
3. State the convention once, in the adapter contract (a models/staging/README or the _staging__sources.yml header), and link both the silver split and the per-source balance test to it.
4. Sequence the demo reset after konsol#113, or accept a demo with no dimensional capability and say so explicitly.
5. Retire scripts/generate_demo_data.py once the replacement model lands. A generator nothing loads is a third statement of the convention waiting to rot — it is the one that wrote magnitude-plus-flag in the first place.

Related: [#155], [#157], [#156], konsol#113, konsol#127, konsol#121.

[Anonymous]

Originally posted by: grynn-in

Rescoped on 12 Sep 2026 after an issue review against main.

Step 1 is done in [#157]: tests/assert_d365_gl_vouchers_balance.sql exists, and stg_d365_fo__gl_entries states that the amount arrives signed. Step 4 is overtaken by the demo removal.

Remaining:

• The stale "positive magnitude (see [#64])" comment in gold_consolidated_trial_balance.sql.
• Stating the adapter contract once, with the balance test on every ERP source, not only D365.
• Deciding whether to delete scripts/generate_demo_data.py. Its HER inserts were the last trace of grynn-in/konsol#82.

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed