Repository navigation
Conversation
mrivas00
added
priority: medium
mrivas00
left a comment
mrivas00
left a comment
There was a problem hiding this comment.
Review: emission factor year plan
Docs-only change: 8 new files, 2406 insertions, no product code. The only executable artifact is verify-migration.sh (mode 755, 125 lines), and that is where most of the correctness risk sits.
16 findings — 5 alta, 8 media, 3 baja. All of them are posted inline; every file in this change is newly added, so every referenced line is part of the diff and nothing needed to be grouped here.
The three that matter most
design.mdline 163 — consolidation orphans saved capture lines. Step 2 retires the duplicatekg/kgrepresentations by soft-deleting them, but nothing re-pointscarbon_inventory_line_factor.emission_factor_idat the canonical survivor. The methodology payload filters to ACTIVE, so task 7.7 ("restore the selected canonical factor by ID") cannot resolve the saved choice, and the rollout spot-check passes anyway because it only checks snapshot columns.verify-migration.shline 60 — the atomicity check does not check atomicity. Plainpsql -fwithout--single-transactiondoes not reproduce how Prisma applies a migration, so the property rollout.md reports as verified is never exercised.analysis.htmlline 1318 — the artifact contradicts its own change directory. It presents the classification gate and the whole implementation as pending, whiletasks.mdmarks those tasks done andfactor-classification.mdrecords them as approved on 2026-09-01.
Verified as correct, for the record
NULLS NOT DISTINCT is safe here: the repo pins postgres:18.4-alpine everywhere, well above the PG 15 requirement. The counts in factor-classification.md reconcile exactly against the seed (base DEFRA 195 / IPCC 45 / Kool, A. 3 / EcoAct 3 = 246; testing 245), as do all ten family counts and the 16 mass/mass collision groups. Its cross-change claim about the energy-unit classification holds — the two kg/GJ factors are Biodiesel and Bioetanol, disjoint from the ten kg/kWh fuels in stationary combustion. All nine in-page anchors in analysis.html resolve, and the format:check glob excludes .html, so CI stays green.
Verdict
The plan itself is well built, but it should not merge as is: the planned migration loses the factor selection on already-captured lines, and the script meant to catch that asserts nothing and prints success unconditionally. Worth resolving the five alta findings before the schema and server-authority PRs build on this; the media and baja ones can ride along in the same pass.
Triage of the duplicate review roundA second Resolved — same defect, same fix, already tracked in the 20:03Z thread on the same line: the orphaned Left open, genuinely new (7). These are not in the first round at all:
Left open, partly overlapping (4). Each has a reply marking which part is already tracked and which part is not:
No code, branches or pushes were touched by either round. |
Conversation cleaned upFollow-up to the triage note above, which is now out of date. Both review rounds have been worked through. 31 threads → 4 open. The four pure duplicates from the second round were deleted rather than resolved, so there is one thread per issue. Fixed locally (23 threads). 22 commits on
Open, waiting on you (4). All four are decisions rather than defects:
|
PushedThe 22 review fixes are now on the branch — a7c9a32..7d0dad6, fast-forward, no history rewritten and no sibling branch touched. The comment above said "not pushed"; that is no longer true. Every fixed thread carries a reply with what changed and its commit SHA, and all 23 are resolved. Local gates before pushing: Still open, waiting on a decision (4). Nothing else remains.
|
A footprint measured for 2022 is offered the newest factor in the
catalog: the year is not part of a factor's identity, it lives inside
the free-text source ("DEFRA 2025"), and both the uniqueness check and
the one-source-per-subcategory rule reject a second vintage outright.
The proposal makes the year a column of its own (null = transversal,
serves any measurement year), moves it into the uniqueness key, and
turns capture into an explicit vintage selector: exact year, else
transversal, else the most recent earlier year, else the closest later
one with a notice. The applied year and the fallback flag are recorded
on the line so a verifier can reproduce the number.
Definition closed with the Product Owner over 16 questions.
The design records why the year becomes a column on emission_factor rather than a vintage table or one methodology version per year, why a null year encodes "transversal", and why resolution stays on the client that owns the selector. Two decisions are worth flagging on review: the partial unique index gains the year and NULLS NOT DISTINCT, because three of its four columns are nullable and it therefore never prevented the duplicates it was meant to; and validateSourceConsistency is scoped per year instead of dropped. The specs cover the catalog side (the year as an attribute, coexisting vintages, one source per subcategory and year, the year travelling with every published representation) and the capture side (the four-step preselection, the override, the recorded year, and the offer to update when a footprint is re-dated). Open question carried into the tasks: which factors are genuinely transversal needs the methodology team before any vintage is loaded.
Every existing factor has to be classified as dated or transversal before the migration can split provider from reporting year, and a missing year suffix is not evidence of either. This is the reviewed map the migration validates against: DEFRA 2025 and EcoAct 2020 by their recognized suffix, IPCC and Kool, A. confirmed transversal by the methodology owner rather than inferred from their names. Also records the unit-family audit behind the consolidation: 16 collision groups, all of them one IPCC industrial-process factor stored twice as kg/ton and kg/kg, all mathematically equivalent. No group needed methodology review, and re-running the audit with kWh reclassified as energy (the pending fix-energy-unit-classification change) yields the same 16, so the two changes do not conflict on seed data.
…rollout Decision 3 said non-required dimension slots are normalized to null "before persistence". Implementing that discarded a value a maintainer had entered in an optional slot, and would have had the production migration erase it from existing factors. The previous duplicate check already normalized for comparison only while persisting whatever was set, so this clause was the one that changed behaviour rather than preserving it. Normalization is now scoped to the uniqueness comparison in the design, the migration task and the vintages spec, with a scenario pinning that an optional dimension value survives a save without creating a separate identity. The accepted consequence — the application check being marginally stricter than the index, which compares raw columns — is written down where the decision is. Adds a rollout note, because the ordering is not obvious from the code: the migrate profile runs before api and web come up together, and the classification gate has to be re-checked against production before either. It also records what to watch before loading more vintages — methodology payload size, and the fact that a second provider for one activity turns a silent default into an explicit choice for every organization capturing it. Adds verify-migration.sh for the checks vitest structurally cannot make: the harness migrates once against a clean database, so it never exercises a preflight whose whole job is to stop a migration. The script stages throwaway databases from a pre-change one, injects an unclassified source and a same-family value conflict, and asserts each aborts.
stage() moves the migration directory into a temp dir so `migrate deploy` stops at the previous commit, and moves it back two lines later. With `set -euo pipefail` and no trap, anything that failed in between — an unreachable Postgres, a wrong DATABASE_URL, a Ctrl-C — ended the script before the restore, deleting the migration from the checkout. The trap covers the failure, the interrupt and the normal exit, and is cleared once the move back has happened.
Retiring a duplicate same-family representation soft-deletes it, and the methodology payload returns only ACTIVE factors, so every carbon_inventory_line_factor still pointing at the retired row loses its saved selection while its value/source/unit snapshots stay intact. The rollout spot-check only compared those snapshots, so it passed. Make the re-point part of the consolidation step, read applied_factor_year from the surviving row, and have the spot-check assert that no saved line references a DELETED factor.
Decision 4 removes validateSourceConsistency, which is what currently rejects a second spelling of a provider inside a subcategory. After that, source is maintainer-entered free text that carries identity, so "DEFRA ", "Defra" and "DEFRA" become three rows that are physically identical, tie in the same rank, and leave capture asking the organization to choose between options that look the same on screen. Normalize on write (trim, collapse inner whitespace, reject empty) and fold case in the comparison, keying the partial unique index on lower(source).
The design and tasks phrased the tie rule as "factors from several sources" while the capability spec phrased it as "exactly one candidate", leaving a winning rank with two factors from one provider undefined. That case is reachable: Decision 3 keeps a value entered in a non-required dimension slot and task 2.4 forbids the migration from erasing it, so migrated data can carry exactly that pair — and the fallthrough would pick by array order or ID, which the same paragraph forbids. Use the candidate-count wording everywhere and add the scenario.
The CATALOG | CUSTOM | DIRECT discriminator overlaps the already-persisted inputType enum, and nothing said whether inputType is kept, derived or dropped. Both are client-authored, so an implementer could leave two discriminators free to contradict each other, and deriving one from the other is not free either: SIMPLIFIED versus EXPERT is a capture mode the factor split does not carry. Keep inputType as the capture mode, derive the factor path from the variant alone, and reject contradictory pairings.
…ecking aborts psql does not wrap a file in a transaction the way prisma migrate deploy does, so every statement before the failing preflight auto-committed and the check passed regardless of whether the migration was atomic — the very property rollout.md reports as verified. Apply with --single-transaction, assert the added columns are gone after an abort, and stop treating a connection failure as a migration result.
The check printed three query results and then reported success unconditionally, including for the query labelled "must be 0", so a run against a catalog that differs from the reviewed one still exited 0 while rollout.md told the operator the check asserts no duplicate business key survives. Compare each result, add the magnitude-pair and orphaned-line assertions, and report the DELETED count as a delta against a baseline taken before the migration, since a restored production copy already contains soft-deleted factors. Staged databases are kept when a check fails.
Both fixtures were INSERT ... SELECT ... LIMIT 1 with no row-count check, so a predicate that matched nothing inserted zero rows and exited 0: the defect was never injected, the migration then ran cleanly, and expect_abort blamed the migration. Check 2 is the likely one — it needs a subcategory named exactly "Procesos industriales - Vidrio", which is per-deployment catalog data, and it did not filter on ACTIVE. Wrap both in a DO block that raises on ROW_COUNT <> 1, add the ACTIVE filter, and set ON_ERROR_STOP for every psql_db call.
PGPASSWORD was the only connection variable without a default under set -u, so on a host using .pgpass, trust or peer auth the script died with an unbound-variable error inside stage() — after the migration directory had already been moved aside — even though every direct psql call worked. It was also interpolated raw into the Prisma DATABASE_URL, so a password containing @ / : # or % parsed into a different host or database. Percent- encode user and password, and correct the documented PGPORT default to 5432, matching the packages/database compose that supplies the other defaults.
CREATE DATABASE ... TEMPLATE is the first statement in stage(), before the trap is armed, and it fails while anything holds a connection to the template — which the local dev API pool does against testdb by default. stage() runs three times, so the template had to stay idle for the whole run with nothing saying so.
The path hardcoded 20260901120000, a timestamp this change does not create: task 2.3 leaves it as <timestamp>_add_emission_factor_year, stamped when Prisma generates the migration. Any other value made stage() fail with a bare "mv: cannot stat", and the already-armed trap then silently restored a path that had never existed.
The trap existed to put the parked migration back, then swallowed both stderr and the exit status with 2>/dev/null || true — so in exactly the case it guarded, a failing restore, the operator was told nothing and walked away with the migration gone from the working tree. Install one cleanup trap that reports a failed restore with both paths and exits non-zero, removes the temp directory each stage created, and names the staged databases it kept when the run ends badly.
The payload block called it rateUnitId while design.md, both capability specs and task 3.4 call it rateMeasurementUnitId — the long form used everywhere else in the repo. Copying from this artifact would land the two halves of the discriminated union with different field names, failing Zod validation at runtime rather than at type-check.
The table covered ties between providers and between transversal providers but not two candidates from one provider, which is the case the design and spec disagreed about.
The page listed tasks 1.1-1.4 and all of sections 2-11 as pending while tasks.md marks them done and factor-classification.md records the classification as approved by the methodology owner on 2026-09-01, and it claimed 4/4 artefacts for a directory that now ships nine. A reader would conclude the classification gate is still open and redo it. Report what is actually closed, point at tasks.md for per-task progress rather than restating it, and list only the deployment steps that remain.
The step described the third check as printing two numbers and asserting one thing. It now reports the retired count as a delta and asserts three conditions, and it needs the generated migration to be present.
The methodology seed schema lives in tools/seed/src/scripts/shared.ts. tools/seed/src/scripts/seedMethodologyData/ holds seedEmissionFactors.ts but no shared.ts, so the path in task 3.2 pointed at a file that does not exist.
The Migration Plan describes consolidating same-family duplicates, but section 2 of tasks.md never turns it into a step: 2.3 splits provider/year, 2.4 backfills magnitudes, 2.5 creates the unique index, 2.6 adds applied_factor_year. Nothing retires a collision, so 2.5 would run with every audited group still present and fail on real data.
…ions Step 3 told the migration to normalize non-required dimension slots, which contradicts Decision 3, task 2.4 and emission-factor-vintages spec — all of which state that normalization applies to the comparison only and that a value entered in a non-required slot is stored and returned unchanged.
The requirement said both enforce the same key, while design.md Decision 3 documents the opposite on purpose: the index compares raw dimension columns and the application normalizes non-required slots, so the application rejects a little more than the constraint does.
The CATALOG validation list checked only that the applied rate unit shares the factor's magnitude family. kg/kg and kg/ton share mass/mass, so a line whose quantity was captured in tons passes that check against a kg/kg applied unit and the server persists a result wrong by three orders of magnitude — under the snapshot the design calls server-authoritative.
Both commands diverged from the contract in docker-compose.prod.yml: neither passed --env-file, so MIGRATION_DATABASE_URL resolves empty through its soft default and the migrate one-shot fails at validate:version; and the one-shot is invoked with --profile migrate run --rm migrate, not up.
…data The cleanup trap did not catch HUP, so an SSH drop during the multi-minute migrate deploy left the migration directory inside a mktemp temp dir and gone from the working tree. Also adds the .openspec.yaml every other change directory carries. Strict validation already passed without it, but this was the only active change missing the file.
7d0dad6 to
6bf4192
Compare
Rebased onto mainNew tip 6bf4192 (was 7d0dad6). Rebased 29 commits onto CI on the new tip: 22/22 pass. Local gates before pushing: The branch now carries the 38 new IPCC factors from main (Ferroaleaciones 28, Cal 6, Aluminio 4), so PR 627 can rebase on top and give them an explicit year. One consequence worth flagging: this turns the open |
…logue main added three industrial-process subcategories with factors — Ferroaleaciones y otros metales, Cal and Aluminio, 38 factors, all IPCC — so every count in this file was measured against a catalogue that no longer exists: 246 factors, IPCC 45 and 16 collision groups, versus 284, 83 and 35 today. Recomputed all of them. The classification map itself is unchanged, since the four source values are the same and every added factor is IPCC, already classified transversal. The conclusions hold too: the 35 groups are still all mass/mass, still all kg/ton against kg/kg, and zero disagree after conversion, so none needs methodology review. Only the scope grew, from four subcategories to seven.
…gories The consolidation now covers 35 collision groups across seven industrial-process subcategories, not 16 across four: main added Ferroaleaciones y otros metales, Cal and Aluminio, which contribute the other 19 kg/ton against kg/kg twin pairs. The migration detects collisions from the data rather than from a list of subcategory names, so the step itself needed no change — only the plan was describing a narrower scope than the code. Marks 2.8, 2.10 and 6.7 done: the schema PR re-points carbon_inventory_line_factor.emission_factor_id and retires the duplicates, and the server-authority PR rejects a factor variant that contradicts inputType.
tasks.md had every box from 1.1 to 11.2 checked while nothing was merged, which read as a claim about this PR and made openspec status report the change complete. The boxes track the change across its implementation branches, not this planning PR, so say that where the boxes are and repeat it in the analysis page. Also refreshes the audit numbers on that page to match the re-audited classification: 284 and 283 factors, and 35 collision groups across seven industrial subcategories.
verify-migration.sh is a 125-line operational script an operator runs against a restored copy of production, and no gate covered it: format:check excludes .sh, the docs-link job does not reach openspec/**, and the shellcheck glob was infra-only. That was the last ungated script location in the repo. Decided to keep it beside the change it documents — it is meaningless once the migration ships — and to widen the gate instead of moving the file. The glob now covers openspec/changes/**/*.sh, with nullglob so a change that ships no script keeps the job green. The script is clean at warning severity today, and at style severity too.
All threads closedTip ad0161e, CI 22/22 pass, mergeable. Four new commits on top of the rebase, pushed fast-forward — no history rewritten, so no force was needed. All 27 review threads are now resolved, 0 open.
On the discriminator thread: no design change was made or needed. The server-authority PR already keeps One caveat for anyone reading older replies: the rebase onto main rewrote this branch, so SHAs cited before that point no longer exist on it. The four commits above are current. |
| four `source` values are the same and every new factor is `IPCC`, already | ||
| classified transversal. What changed is the size of the sets it applies to, and | ||
| every count in this file has been recomputed. The conclusions all still hold; | ||
| the expanded consolidation set is the part worth the methodology owner's |
There was a problem hiding this comment.
Open on purpose: the expanded consolidation set has not been re-approved.
This file is signed "Approved by the methodology owner on 2026-09-01", and that approval covered 16 collision groups across four industrial-process subcategories. The catalogue on main has since grown, so the set the migration will actually consolidate is 35 groups across seven — Ferroaleaciones y otros metales, Vidrio, Acero, Cal, Cinc, Aluminio and Cemento.
What I did in 410dbef is mechanical only: I recomputed every number from the seed and re-ran the equivalence check. Zero of the 35 groups disagree after conversion, the classification map is unchanged because all 38 added factors are IPCC and IPCC was already classified transversal, and the canonical kg/ton choice still holds because all seven subcategories allow g, kg and ton. So nothing contradicts the original decision.
But recomputing is not approving. The 19 groups from the three new subcategories were never reviewed by a person, and the migration soft-deletes a catalog row for each of them. That is a methodology sign-off, not a code review, so I am leaving this thread open rather than resolving it on the strength of my own arithmetic. Rollout step 1 is the backstop — it re-checks the production catalogue against this map before the migration runs — but it validates sources, not consolidation groups.
Resolve this once the methodology owner has confirmed the 35-group set, and update the approval line with the new date.
There was a problem hiding this comment.
Making this fully explicit, since it is the one thread that stays open and it is a sign-off, not a code change.
What is missing. Nothing in the file or the code. The numbers were already refreshed in 410dbef and the migration handles the wider set on its own, because it detects collisions from the data rather than from a list of subcategory names. What is missing is a person re-approving the consolidation set. The approval line still reads 2026-09-01, and that approval was given over a catalogue that no longer exists.
What changed against what the approval covered. The file today documents the current catalogue; the 2026-09-01 sign-off covered the left-hand column:
| Approved 2026-09-01 | Documented today | |
|---|---|---|
Factors, base / testing |
246 / 245 | 284 / 283 |
IPCC factors |
45 | 83 |
mass/mass family |
130 | 168 |
| Collision groups | 16 | 35 |
| Subcategories affected | 4 | 7 |
The four new industrial-process subcategories with collisions are Ferroaleaciones y otros metales (14 groups), Cal (3), Aluminio (2) — 19 groups the owner has never seen — on top of Vidrio (8), Acero (4), Cinc (3) and Cemento (1), which are the originally reviewed 16.
What does not change, and so is not part of the ask: the source-to-vintage map is identical, because all 38 added factors are IPCC and IPCC was already classified transversal; all 35 groups are still kg/ton against kg/kg inside mass/mass; zero groups disagree after unit conversion, so none is a methodology question; all seven subcategories allow g, kg and ton, so kg/ton remains the canonical unit; and the fix-energy-unit-classification cross-change note still holds, yielding the same 35 groups with no new collisions.
Why it needs the owner and not me. The migration soft-deletes one catalog row per group. For 19 of them that deletion would rest on my arithmetic alone. Recomputing is not approving, and rollout step 1 does not cover it — it re-checks production source values against the map, not the consolidation groups.
Waiting on the methodology owner. Deliberately left open and not resolved. When the 35-group set is confirmed, update the approval line with the new date and close this thread.
1 participant
What & why
source("DEFRA 2025"), so factor identity, vintage selection and year-mismatch warnings could not be expressed at all.Key changes & decisions — the ones that shape every later PR:
1.
year = NULLmeans transversal, never "unknown"specs/emission-factor-vintages/spec.md,factor-classification.md· c5b52b3, 69ba58cNULLis a positive claim ("confirmed applicable to every reporting year"); an unclassified factor is a preflight error, not an inferred null.IPCCorKool, A.would silently mis-key the catalog, and a wrong null is unrecoverable once rows start colliding on the new unique index. Failing the migration is the only reversible option.2. Factor identity keys on unit family, not exact unit
design.md,specs/emission-factor-vintages/spec.md· d1e9772, 5f325b9kg/kgandkg/tonare the same factor expressed differently, so an exact-unit key would let mathematically equal rows coexist forever.3. Mismatch is derived at read time, never stored
design.md,specs/emission-factor-year-selection/spec.md· ec8dfe0, 5f325b9isFallback/appliedFactorYearMatchesInventorycolumns.applied_factor_yearsnapshot is stored, and mismatch is computed against the current inventory year.4. Dimension normalization applies to the comparison, not to the data
design.md,tasks.md(2.4) · 5f325b95. No bulk re-resolution after a year change
specs/emission-factor-year-selection/spec.md(8.5) · ec8dfe0Deliberate deviations
tasks.mdships with every item already checked[x]. The work is done — it lives on the three chained PRs below, not on this one. Reading this PR as "plan complete, code pending" is correct; reading a checked box as "merged" is not.verify-migration.shis added here but not executed by this PR — it is exercised by the schema PR (feat/mati/emission-factor-year-schema).Test plan
pnpm format:check && pnpm lint && pnpm type-check— green (no code touched; run to prove the docs don't break the gates).proposal.md→design.md→ the two capability specs; challenge decisions 1-5 here rather than on the code PRs.Chain — merge in order:
UNCHANGEDvariant + batched catalog lookupsOut of scope