Skip to content

fix(db,web): единна стойностна база за обобщенията и страниците (#98) - #259

Merged
todorkolev merged 2 commits into
mainfrom
fix/canonical-value-base
Jul 23, 2026
Merged

fix(db,web): единна стойностна база за обобщенията и страниците (#98)#259
todorkolev merged 2 commits into
mainfrom
fix/canonical-value-base

Conversation

@todorkolev

Copy link
Copy Markdown
Collaborator

Какво и защо

Обобщенията (authority_totals, home_totals, company_totals, flow_pairs, sector_totals) сумират всеки ред с известна стойност в евро - amount_eur IS NOT NULL, независимо от value_flag. Това включва review, annex_suspect, value_low и поправените value_suspect.

Две заявки на страници обаче филтрираха строго по value_flag = 'ok' и затова подценяваха спрямо обобщенията. Най-осезаемо при home.ts, където се сумират пари: един и същ показател даваше различно число според това къде го гледаш - точно проблемът, описан в #98.

Промени

  • Изравнени са двете заявки за „една оферта" към каноничната база, като филтърът bids_received = 1 е запазен - той дефинира самия показател.
  • Инвариантът е документиран в docs/etl.md, в речника на данните за асистента и в коментарите на precompute.sql / схемата.
  • Добавен е регресионен тест, който сравнява живите агрегации по институция, фирма и сектор срещу построените обобщения и пада при разминаване.

Проверка на останалата част от заявките

Прегледах целия слой: няма друга парична сума, която да се разминава. Ограниченията за положителни стойности в competition.ts (дял „една оферта", HHI, дялове по процедура) са умишлени - те трябва да останат ограничени - и вече носят изричен коментар защо са изключение. Използването на value_low в contractsSummary е броене за значка за качество на данните, не филтър върху пари.

Тестове

Пълният @sigma/db пакет минава локално: 29 файла / 201 теста, включително тестовете, които минават през sqlite3 и Wrangler. Typecheck на packages/db и apps/web е чист.

Closes #98

🤖 Generated with Claude Code

Rollups sum every row with a known EUR value (`amount_eur IS NOT NULL`,
regardless of value_flag), but two single-offer page queries restricted to
`value_flag = 'ok'` and therefore under-reported against them - the same
figure differed depending on where you read it.

- align both single-offer queries (the contracts.ts list and the home.ts
  money sum) to the canonical base, keeping the `bids_received = 1`
  competition filter that defines the metric
- document the invariant in docs/etl.md, the assistant data dictionary and
  the precompute/schema comments
- add a regression test comparing live authority/company/sector page
  aggregations against the production-built rollups, failing on divergence
- swept the rest of the query layer: no other money sum diverges; the
  positive-only guards in competition.ts (single-offer share, HHI) are
  deliberate and now carry an explicit exception comment

Closes #98

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR #98 — „единна стойностна база за обобщенията и страниците"

Бележка: прегледът е написан на български съгласно конфигурацията на задачата. Не са публикувани коментари в PR — това е чернова за вашата проверка.

Verdict

COMMENT — няма блокиращи проблеми; преди merge потвърдете двете точки по-долу.

Резюме на промяната

PR-ът уеднаквява „стойностната база" за паричните агрегати: страничните заявки вече сумират amount_eur IS NOT NULL независимо от value_flag, вместо стария филтър value_flag = 'ok' AND amount_eur > 0. Така страниците се изравняват с rollup-таблиците (home_totals, company_totals, authority_totals, sector_totals), които вече използват тази база. Реалните кодови промени са само в contracts.ts (listSingleOfferContracts, contractsSummary) и home.ts; всичко останало са коментари/документация плюс нов тест. Промяната отговаря на описанието на задачата (обща канонична база за суми).

Сигурност (Phase 0 + agent-level, OWASP)

  • CLEAN. Няма твърдо кодирани тайни, промени по URL или нови зависимости (само вградени модули на Node + съществуващ vitest).
  • SQL injection: променените заявки са безопасни — LIMIT ? е bind-нат, order е избор между два хардкоднати литерала, а заявката в home.ts е изцяло статична. Няма конкатенация на потребителски вход. directPlaceholders/where.join в competition.ts не са пипани от този PR.
  • Целостност на данните: промените са read-only заявки + коментари + тест; няма миграции, които променят данни.

Качество и тестове

  • Новият тест value-base-sql.test.ts е смислен, не е „cheater" — изгражда реалните rollup-и от миграциите + precompute.sql и сравнява живите агрегации с тях. Покрива всички варианти на value_flag, включително поправен value_suspect и NULL value_suspect (който правилно е изключен). Проверих числата ръчно: сума 1060 и 5 договора за single-offer, подредба [500, 300, 200, 100, -40] — коректни.
  • Документацията (docs/etl.md, коментарите в SQL и describe-schema.ts) е обновена консистентно с новата дефиниция.

Точки за потвърждение преди merge

  1. Отрицателни/value_low редове на публични страници. Премахването на amount_eur > 0 вкарва value_low редове (напр. -40 в теста) в списъците и сумите на началната страница за single-offer договори. За rollup-сравнимост това е правилно, но потвърдете, че показването на отрицателна/ниска стойност публично е желано (в режим „highest value" тя ще е накрая на списъка, така че на практика ще излиза само при много малък корпус).
  2. Пълнота на уеднаквяването. Не можах да grep-на цялото хранилище (локално е наличен само review-ботът, не проектът automated-review). Моля потвърдете, че не е останал друг паричен агрегат, който още филтрира по value_flag = 'ok' или amount_eur > 0 (напр. sector/authority/company заявки извън диффа), за да не се получи нова несъгласуваност. Тестът покрива authority/company/sector/home пътищата — ако има други, добавете ги.

CLAUDE.md / gates

Няма частична имплементация, дублиран или мъртъв код, нито смесени концерни. Промяната е атомична и в обхвата на задачата. Единствената резерва за score >9/10 е невъзможността да потвърдя 100% покритие и липсата на пропуснат агрегат без достъп до пълното хранилище — оттам и verdict-ът COMMENT вместо APPROVE.

Comment thread packages/db/src/queries/contracts.ts
Comment thread packages/db/src/queries/home.ts

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверих срещу самия diff. Реалната промяна в поведението е точно две заявки на странициlistSingleOfferContracts (contracts.ts) и паричната сума за „една оферта" в home.ts — минаващи от value_flag = 'ok' AND amount_eur > 0 към каноничната база amount_eur IS NOT NULL, с което се изравняват с обобщенията (точно #98). Останалото (competition.ts, precompute.sql, 0000_init.sql) е само коментари/документация — потвърждава, че агрегатите вече бяха на каноничната база, и — важно — няма промяна по схемата, тъй че няма миграционна дупка.

Ортогонално на #257: той пипа конверсията на анекс-валутата, ти пипаш филтъра — не се застъпват (координирайте само реда на merge заради общите файлове).

Регресионният тест е реален: сее по един ред от всеки value_flag (review/annex_suspect/value_low вкл. −40) и заковава, че тоталите на страниците .toBe() обобщенията по институция/фирма/сектор, а двете home single-offer заявки дават 1060 на не-NULL базата. Забелязвам, че single-offer списъкът вече включва value_low −40 ред — нарочно е (тестът го заковава изрично), не пропуск.

Одобрявам.

@midt-admin midt-admin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Одобрено - каноничната стойностна база е потвърдена (нула остатъчни value_flag филтъра в паричните агрегати, проверено с grep), нишките резолвнати, CI зелен.

@todorkolev
todorkolev merged commit 463e22a into main Jul 23, 2026
1 check passed
todorkolev added a commit that referenced this pull request Jul 23, 2026
todorkolev added a commit that referenced this pull request Jul 23, 2026
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 26, 2026
Resolves the DIRTY conflict with main after the last upstream batch.
- Renumber migration 0002_related_persons_foundation.sql -> 0003 (midt-bg#261 took
  0002 for current_value_currency); update all refs; add 0002_current_value_currency
  to the test migration chains.
- Route the 4 conflict loaders through getDb(env) — the midt-bg#199/midt-bg#225 read-only D1
  chokepoint (no web source may read env.DB directly).
- deploy.yml: keep BOTH the amendment-currency backfill step (upstream midt-bg#245) and
  the свързани-лица schema step (ours, now 0003).
- docs/adr/README.md: keep our 0007-0028 + upstream's 0029.
- Officials €-block unchanged: sums the canonical amount_eur base (midt-bg#259), distinct
  from the current_value_currency conversion — no silent €-change.
- Lockfile regenerated (react-router 7.18.0 already pinned).
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 29, 2026
Pulls the euro-annex conversion fix (midt-bg#245/midt-bg#261), canonical value base (midt-bg#259),
identity canonicalization + Bulstat checksum + joint procurement (midt-bg#251-253),
app-layer read-only D1 guard (midt-bg#225), JSON-LD escaping (midt-bg#212), react-router 7.18.0.

Non-trivial resolutions:
- normalize-raw.sql: kept upstream's amendment_winner currency CTE + our
  is_synthetic column (both additive, one column-list collision).
- integrity-checks.mjs: upstream's (await rows()) wrapper carrying our
  is_synthetic != 1 filter on auth/bidder attribution.
- Migration collision: our 0002_contracts_is_synthetic renumbered to 0006
  (upstream took 0002 for current_value_currency); tests load all migrations.
- refresh-slice.test seedReattrContract: upstream's authority params + our
  real-tender-header seed so reattr contracts stay non-synthetic and reconcile.
- root.tsx/assistant.chat.tsx: adopted getDb read-only chokepoint + kept dock.
- describe-schema DATA_TRAPS: upstream canonical-base rule + our NULL detail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Данни: гаранция за единна стойностна база (rollups ↔ страници)

4 participants