Skip to content

feat(anomalies): add automated price-anomaly screen - #239

Open
Mupaky wants to merge 5 commits into
midt-bg:mainfrom
Mupaky:feat/anomaly-screen
Open

feat(anomalies): add automated price-anomaly screen#239
Mupaky wants to merge 5 commits into
midt-bg:mainfrom
Mupaky:feat/anomaly-screen

Conversation

@Mupaky

@Mupaky Mupaky commented Jul 14, 2026

Copy link
Copy Markdown

Closes #41 — the price_outlier signal is the cohort price-deviation flag from that issue (thresholds: ≥ 5× the full-CPV-code median, ≥ 10 peers, ≥ €50k). This PR is also a prerequisite for the composite risk indicator in #229, which aggregates exactly these elementary flags.

What this PR does

Adds a new /anomalies page ("Аномалии" in the nav) with automated
price-signal checks over every contract in the corpus.

Three price signals

Signal Rule Count in corpus
Над прогнозата (over-estimate) Signed ≥ +10% above the authority's own announced estimate (framework/DPS call-offs excluded; only comparable estimates) 8 203
Ръст чрез анекси (annex growth) Current value ≥ +20% over the signing value via annexes (ЗОП чл. 116 caps most changes at 10–50%) 2 155
Далеч над типичното (price outlier) ≥ 5× the median of contracts with the same full CPV code (peers ≥ 10, value ≥ €50k) 25 076

Единствена оферта (single bid) and без обявление (direct/no-notice) add points to the 0–100 risk score but never create a row alone.

Verified on the full corpus: 32 107 anomalies, €37.4 bn flagged value.

Coordination (per review triage)

Changes

  • packages/db/migrations/0005_anomalies.sql — new numbered migration (per current convention) with cpv_price_stats + contract_anomalies + indexes; CREATE … IF NOT EXISTS on purpose, since precompute/refresh create the tables defensively on databases migrated before this landed. migrations.test.ts applies the whole chain in filename order
  • scripts/precompute.sql §7 — builds both tables (median via window functions, scoring via sub-select); runs on every full import and via ship-domain
  • scripts/refresh-slice.sql@refresh-batch anomalies scoped to refresh_touched_contracts. Refresh semantics: the slice re-derives touched contracts against the last computed medianscpv_price_stats refreshes on full import only, so untouched contracts keep their flags and a cohort newly crossing peers ≥ 10 starts flagging on the next full import. Documented in the SQL header and on /methodology#flags
  • Duplication guard: the derive/scoring block is intentionally duplicated between precompute and the slice; packages/db/src/anomaly-parity.test.ts pins the marked regions byte-identical and pins score/rank_value to the same weights — a threshold changed in one file fails the build
  • SQL behaviour tests: packages/db/src/anomaly-precompute.test.ts runs §7 verbatim against a seeded SQLite database — cohort median (even-n + self-inclusion), each threshold tier, the framework awards > lots exclusion, and the context-signals-never-create-a-row rule
  • packages/configANOMALY_SIGNALS taxonomy; packages/api-contractAnomalyListItem, AnomaliesSummary, AnomalyFacets, AnomalySignals DTOs
  • packages/db/src/queries/anomalies.tslistAnomalies (keyset; list + summary in Promise.all), anomaliesSummary, getAnomalyFacets; all filter predicates on the small anomaly table
  • apps/web/app/lib/anomaly-badges.ts — the badge copy/formatting as a pure, unit-tested module (also removes a latent toLocaleString('bg-BG') — workerd carries no bg-BG Intl data); apps/web/app/routes/anomalies.tsx renders from it
  • routes.ts, nav, sitemap-pages.tsx, PAGE_SIZE, PARAM_ORDER updated
  • apps/web/app/routes/methodology.tsx — section 9 documents thresholds, exclusions, the peer-set self-inclusion, the median refresh cadence, and the "indicator, not verdict" framing
  • README.md — new route row, updated status/roadmap copy

Tests

  • 180 pure-TS tests green (112 db + 5 config + 68 web left green locally); SQL-driven suites (migrations, anomaly-precompute, refresh-slice) run in CI/devcontainer — the anomaly-precompute assertions were verified against the same engine locally
  • Typecheck 7/7 packages
  • HTTP smoke-test: 200, working filters (373 annex-growth contracts in 2024, €535.9 M)

Methodology / security

Signals are indicators for public scrutiny, not verdicts — documented on /methodology#flags and in the precompute.sql §7 header. All SQL uses bound parameters (zero string concatenation); unknown filter keys fail closed (1=0); under-threshold ratios stay server-side.

Review response (r1)

Note Status
Closes #41 + #229 prerequisite link ✅ description updated
#210 / #171 collision ⚠️ coordination section above — needs a maintainer call on who leads the shared cohort stats
F1 "converge identically" overstated ✅ rephrased here + refresh-slice header + /methodology note on median refresh cadence
F2 DDL in 0000_init ✅ moved to 0005_anomalies.sql
F3 duplicated scoring SQL ✅ byte-identity parity test (anomaly-parity.test.ts)
F4 getDb noted — will switch the loader to getDb(env) once #225 merges
SQL scoring tested only indirectly anomaly-precompute.test.ts
UI layer untested ✅ badge logic extracted to anomaly-badges.ts + unit tests
list/summary sequential awaits Promise.all
README table alignment ✅ fixed
Median includes the contract itself ✅ noted in methodology + asserted in the SQL test

@lyubomir-bozhinov lyubomir-bozhinov added data-quality Качество/коректност на данните (ETL, регистри) enhancement Нова функционалност или предложение web Област: web labels Jul 14, 2026

@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.

Прегледах стриктно — солидна, внимателна работа. Верифицирах емпирично срещу дифа и по-важното: как се връзва с останалите отворени PR-и и Issue-та.

Каквото е коректно (проверено):

  • Медианата е вярна — трикът rn IN ((n+1)/2, (n+2)/2) дава коректна медиана и при нечетно, и при четно n.
  • Framework/ДСП изключването е коректно и в slice-аaw брои договорите на целия tender (не само touched), тъй че awards ≤ lots гейтът не се чупи при scoping.
  • Bound параметри навсякъде (без конкатенация), филтрите са върху малката anomaly таблица (без 190k скан), а рамката „индикатор, не присъда" е документирана (methodology §9) — правилно за тази чувствителна повърхност.

Триаж / колизии (най-същественото):

  1. Closes #41. Този PR имплементира отворения #41 („Рисков флаг: цена-отклонение спрямо съпоставима кохорта (CPV)") — това е сигналът price_outlier. Липсва линк; добавете Closes #41 в описанието. Надолу по веригата #229 (композитен рисков индикатор) агрегира точно тези елементарни флагове — този PR е негова предпоставка.

  2. Колизия с #210 (най-важна). #210 („ценови ориентир по CPV кохорта") строи паралелна CPV-кохортна ценова база — 0004_cpv_division_stats.sql на ниво дивизия — със свои секции в precompute.sql + refresh-slice.sql. Този PR строи cpv_price_stats (пълен CPV код). Две паралелни изчисления на по същество една и съща база, редактиращи едни и същи файлове (precompute.sql, refresh-slice.sql, methodology.tsx, api-contract) → сигурни merge конфликти + логика, която ще дрейфне. Нужно е едно общо CPV-кохортно stats изчисление, обслужващо и контракт-страницата (#210), и outlier флага (тук). Съгласувайте кой води.

  3. Припокриване с #171 (overruns). „Преразходи" = сигналите over_estimate + annex_growth като табло. И двата PR-а пипат filters.ts, routes.ts, api-contract, config → конфликти; двете повърхности може да показват едни и същи договори с различни прагове. Координирайте.

Технически бележки (нито една не блокира):

  • F1 — „converge identically" е пресилено. Slice-ът съзнателно НЕ преизчислява cpv_price_stats (медианите се опресняват на пълен импорт). Значи непокътнати peer-и в CPV с дневен churn пазят остарели outlier флагове/ratio-та, а CPV, който току-що минава peers ≥ 10, няма да флагне до следващия пълен импорт. Разумна ETL апроксимация — но не е „идентична конвергенция"; смекчете формулировката и отбележете в /methodology, че медианите се опресняват на пълен импорт, не в реално време.
  • F2 — DDL в 0000_init.sql. Двете нови таблици са инертни на вече инициализирана прод/staging D1 (wrangler d1 migrations apply пропуска вече приложения 0000_init). Материализират се само защото precompute.sql/refresh-slice.sql ги правят с CREATE TABLE IF NOT EXISTS — което сте направили коректно (за разлика от #188). Но текущата конвенция е нов номериран файл (#2100004_, #2260002_); преместете DDL-а в нов номериран migration (напр. 0005_anomalies.sql). Бележка: migrations.test.ts валидира fresh-apply пътя, не persistent-prod реалността.
  • F3 — дублиран scoring SQL. ~50 реда деривация/scoring са копирани дословно между precompute.sql §7 и refresh-slice.sql, пазени само от коментар „keep in sync" → тих drift риск. #203 въвежда @include фрагмент codegen точно за това; алтернативно — тест за byte-идентичност на двата блока (както в #203).
  • F4 — координация getDb. Loader-ът ползва суров env.DB — консистентно с текущия main (contracts.tsx е същото; #199/getDb още не е merge-нат), тъй че не е дефект тук. Бележка: щом #225 влезе, anomalies.tsx трябва да мине на getDb(env), иначе ../../** chokepoint скенерът ще го флагне.

Иначе — чиста работа. Коментар, не блокирам.

@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: feat(anomalies): add automated price-anomaly screen

Здравейте! Направих задълбочен преглед с приоритет върху сигурността и целостта на данните (SQL експлойти, инжекции, зловреден код), следван от коректност, производителност, тестове и съответствие с правилата на проекта. По-долу е обобщението; конкретни бележки няма да публикувам като inline коментари, за да можете първо да прегледате изводите, както поискахте.


ВЕРДИКТ: COMMENT — няма открити блокиращи проблеми по сигурност или цялост на данните. Преди пълно одобрение (APPROVE) препоръчвам няколко второстепенни подобрения: покриване на дублирания SQL с тест за паритет, тестове за UI слоя и дребна корекция в README.

Фаза 0 — Задължително сканиране за сигурност

  • Твърдо кодирани тайни (API ключове, пароли, токени): няма. ✅
  • Промени по URL / нови адреси: само вътрешни маршрути (/anomalies в навигацията, routes.ts, sitemap-pages.tsx). Няма външни адреси. ✅
  • Зловредни шаблони (бекдори, инжекция на код, обфускация): няма. ✅
  • Нови зависимости: няма. ✅
  • SQL инжекция: проверих внимателно packages/db/src/queries/anomalies.ts. Всички стойности от потребителя (years, sectors, valueBucket, authority, bidder, cursor) се подават като обвързани параметри (? + .bind(...)). Динамичните части, които влизат директно в текста на заявката (имена на колони за сортиране и за сигнали), идват изключително от статични lookup(...) таблици, а не от вход на потребителя. Защитата срещу prototype pollution (toString, constructor) е потвърдена от тестовете (sort=toString → default; signals=['constructor']1=0). Не открих път за SQL инжекция. ✅

Резултат от Фаза 0: ЧИСТО — продължавам с пълния преглед.

Сигурност (ниво код / OWASP)

  • XSS: всички стойности (предмет, имена, УНП) се рендират като текст през React (авто-екраниране); няма dangerouslySetInnerHTML. ✅
  • Валидиране на вход: непознати ключове за сигнал дават празен резултат (1=0), а не нефилтриран списък — коректно поведение „по подразбиране забранено“. ✅
  • Изтичане на данни: добре обмислено — съотношенията под праг, съхранени в базата, се показват само когато съответният flag_* е активен (toItem в anomalies.ts), така че под-праговите числа остават на сървъра. ✅

Цялост на данните / коректност на изчисленията

Прегледах scripts/precompute.sql §7 и scripts/refresh-slice.sql ред по ред:

  • Медиана (cpv_price_stats): WHERE rn IN ((n+1)/2, (n+2)/2) с AVG(eur) дава правилна медиана и за нечетно (една средна стойност), и за четно n (средно на двете средни). ✅
  • Изключване на рамкови споразумения/ДСП: aw.n <= MAX(COALESCE(t.num_lots,0),1) коректно изключва случаите с повече договори от лотове. ✅
  • Защита от деление на нула: flag_over изисква est_eur >= 1000; ratio изисква median_eur > 0; growth изисква signing_value_eur > 0. Няма деление на нула. ✅
  • Валутна конверсия: BGN→EUR (/1.95583) с ясен COALESCE(currency,'BGN') IN ('BGN','EUR') филтър — чуждите валути без курс се пропускат. ✅
  • Тегла на риска: стойностите в SQL (over 25/35/45, annex 20/30, outlier 15/25, single_bid 10, no_notice 5, MIN(100, …)) съвпадат точно с описаното в methodology.tsx и packages/config. ✅
  • Инкрементален refresh: refresh-slice.sql пресмята само засегнатите договори, но под-заявката aw брои всички договори на засегнатите поръчки — коректно за откриване на рамкови. Съзнателното непрестрояване на cpv_price_stats е документирано и приемливо. ✅

Производителност

  • Денормализираната таблица contract_anomalies + индексите (rank_value, amount_eur, signed_at, authority_id, bidder_id) държат филтрирането върху малка таблица и избягват сканиране на 190k реда. ✅
  • Кеширане: Cache-Control 1800s + withDbRetry. ✅
  • Второстепенно: в listAnomalies заявката за списъка и anomaliesSummary се изпълняват последователно (await след await). Могат да се обединят с Promise.all за един по-малко кръг към базата.

Тестове

  • Добро покритие на слоя със заявки (мапване на редове, филтри, facets, гранични reserved ключове) и на миграциите. ✅
  • Пропуски: няма тестове за UI компонента anomalies.tsx / SignalFlags, нито за самата SQL логика за скоринг (тя се тества само косвено). Това поставя под въпрос целта за ≥90% покритие върху новия код.

Съответствие с CLAUDE.md

  • NO CODE DUPLICATION: основната бележка. Целият блок за флагове и скоринг е дублиран дословно между scripts/precompute.sql §7 и scripts/refresh-slice.sql. Коментарът „keep in sync“ признава риска, но при бъдеща промяна на праг лесно ще се получи разминаване. Препоръка: тест за паритет на праговете/теглата между двата файла (или генериране от общ източник), който да проваля билда при разминаване.
  • Останалите правила (без частична имплементация, без TODO, без мъртъв код, последователно именуване, разделени отговорности) са спазени. ✅

Второстепенни бележки

  • README.md: редът „Договори“ в таблицата е с променено подравняване на | и вече не е подравнен с останалите редове — чисто козметично.
  • cpv_price_stats: медианата включва и самия договор в множеството съпоставими; при праг peers ≥ 10 ефектът е пренебрежим, но си струва да се спомене в методологията.

Готовност за деплой

  • Миграциите добавят таблици с CREATE TABLE IF NOT EXISTS (в refresh) — безопасно за стари бази. ✅
  • Няма разрушителни промени по съществуващи таблици; не блокира main. ✅
  • Сравнение с описанието на тикета: не ми е налично тук, затова не мога да потвърдя формално всички приемни критерии — моля потвърдете, че праговете (10% / 20% / 5×) отговарят на изискванията.

Като цяло PR-ът е много добре изпипан, документиран и защитен срещу инжекции. Нямам блокиращи забележки по сигурност или цялост на данните.

Mupaky added a commit to Mupaky/sigma that referenced this pull request Jul 14, 2026
Review triage (lyubomir-bozhinov, ydimitrof) — none blocking, all applied:

- move the anomaly DDL out of 0000_init.sql into a numbered migration
  (0005_anomalies.sql) per current convention; IF NOT EXISTS on purpose,
  since precompute/refresh-slice create the tables defensively on
  databases migrated before the file landed. migrations.test.ts now
  applies the whole chain in filename order (F2)
- guard the intentionally duplicated derive/scoring SQL with
  @anomaly-derive markers + anomaly-parity.test.ts: the shared blocks in
  precompute.sql §7 and refresh-slice.sql must stay byte-identical, and
  score/rank_value must carry the same weights (F3 / no-duplication note)
- add anomaly-precompute.test.ts: behavioural tests of §7 against a real
  SQLite database — cohort median (incl. even-n and self-inclusion),
  each threshold tier, framework awards>lots exclusion, and the
  context-signals-never-create-a-row rule (test-coverage note)
- clarify convergence semantics (F1): the daily slice re-derives touched
  contracts against the LAST computed medians; cpv_price_stats refreshes
  on full import only. Stated in the refresh-slice header and on
  /methodology#flags, alongside the peer-set self-inclusion note
- extract the badge mapping into apps/web/app/lib/anomaly-badges.ts with
  unit tests (UI-coverage note); this also removes a latent
  toLocaleString('bg-BG') dependency — workerd does not carry the bg-BG
  Intl data, so formatting is hand-rolled like @sigma/shared
- run the list page and its summary with Promise.all in listAnomalies
  (one fewer sequential D1 round-trip)
- realign the README route-table pipes

@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 — feat(anomalies): екран за автоматични ценови аномалии (партида 1 от 2)

Прегледът е направен само върху файловете от тази партида. SQL логиката за извеждане/скориране (scripts/precompute.sql §7 и scripts/refresh-slice.sql) е в другата партида и трябва да се провери там — там живее реалният риск за целостта на данните; тази партида само консумира вече изчислените флагове.


Сигурност (Фаза 0 — критичен скан)

  • Няма зашити тайни (API ключове, пароли, токени).
  • Няма нови външни URL адреси, нови зависимости или обфускиран/бекдор код.
  • Няма SQL injection в тази партида. Всички потребителски стойности минават през параметризирани ? плейсхолдъри (bind(...)). Имената на колони/таблици идват единствено от бели списъци (SORTS, SIGNAL_COLUMNS, VALUE_BUCKETS), изградени през lookup(...). Тестовете изрично покриват отравяне на прототип (toString, constructor, 1=0 вместо нефилтриран списък) — това е добра OWASP A03 защита.
  • Няма XSS: текстът на чиповете се генерира от форматери (money, count, signedPct), а React екранира по подразбиране.

Оценка сигурност: чисто за тази партида (условно на прегледа на SQL в партида 2).

Тестове

Много силно покритие: unit за баджовете (anomaly-badges.test.ts), query-shape spy тестове, поведенчески тестове върху реален SQLite (anomaly-precompute.test.ts), паритет-гард че блокът е байт-идентичен между precompute и refresh-slice, и разширен тест на миграционната верига. Тестовете търсят дефекти (прагове, изключване на рамкови/DPS, контекстни сигнали, които не създават ред), а не минават тривиално.

Архитектура и производителност

Добро разделяне: чистата display логика е изнесена в lib/anomaly-badges.ts и е unit-тествана. Филтри/сортиране/summary са изцяло върху малката денормализирана таблица contract_anomalies; join-овете към домейн таблиците са само за 15-те показани реда. Добавени са подходящи индекси. Публичен HTTP кеш (1800s).

Документация

README и методологията (/methodology#flags) са обновени последователно; праговете и ограниченията са документирани публично.


Незадължителни забележки (не блокиращи)

  1. rank_value = score×1e12 + amount_eur като REAL (double). При магнитуд ~1e14 се губи под-евровата точност; за подредба по еднакъв скор ефектът е пренебрежим, но си струва да се знае, ако сумите някога надхвърлят ~9e15.
  2. date-asc съществува в AnomalySort/SORTS, но не се предлага в ListControls — достъпен само чрез ръчно нагласен URL. Безвреден, но е малко „мъртъв“ от UI гледна точка.
  3. formatTimes: съотношение в [99.95, 100) минава по клона <100"100" без разделител на хилядите, докато >=100 ползва count(). Само козметично.
  4. getAnomalyFacets прави 3 нефилтрирани агрегата на всяка заявка — приемливо предвид кеша и малката таблица.

Заключение

Партида 1 е с висок клас: чиста, добре тествана, без открити уязвимости по сигурност/цялост. Одобрението на целия PR зависи от прегледа на SQL логиката (прагове, изключвания, скориране) в партида 2.


English draft (за избор коя версия да изпратиш)

PR review — feat(anomalies): automated price-anomaly screen (batch 1 of 2)

Reviewed only this batch. The derive/scoring SQL (scripts/precompute.sql §7, scripts/refresh-slice.sql) is in the other batch and must be verified there — that is where the real data-integrity risk lives; batch 1 only consumes the precomputed flags.

Security (Phase 0): no hardcoded secrets, no new external URLs/dependencies, no backdoor/obfuscation. No SQL injection in this batch — all user values are parameterized (bind), and column/table identifiers come only from whitelists (SORTS, SIGNAL_COLUMNS, VALUE_BUCKETS) built via lookup(...), with tests explicitly covering prototype-pollution keys (toString, constructor, 1=0). No XSS (formatter-derived text + React escaping).

Tests: excellent coverage — unit, query-shape spies, behavioural SQLite tests of §7, a byte-identical parity guard between precompute and refresh-slice, and an extended migration-chain test. Tests are designed to reveal flaws, not pass trivially.

Architecture/perf: clean separation (display mapping isolated and unit-tested); all filters/sort/summary run on the small denormalized table, joins only for the 15 displayed rows, indexes added, public HTTP cache.

Minor, non-blocking notes: (1) rank_value stored as REAL loses sub-euro precision at ~1e14 magnitude — negligible for tie-break ordering; (2) date-asc sort exists but isn't exposed in the UI; (3) formatTimes boundary [99.95,100) skips the thousands separator — cosmetic; (4) facet query runs 3 unfiltered aggregates per request — fine given caching.

Verdict below.


VERDICT: COMMENT — партида 1 е чиста и готова; финалното одобрение зависи от прегледа на SQL логиката в партида 2. / batch 1 is clean; final approval pending the SQL derive review in batch 2.

Mupaky added 2 commits July 14, 2026 16:45
Add /anomalies page that surfaces procurement contracts with fired
price signals from the full 193k-contract corpus:

  • над прогнозата — signed >= +10% above the authority's own estimate
    (framework/DPS call-offs excluded; comparable estimates only)
  • ръст чрез анекси — current value >= +20% over signing via annexes
  • далеч над типичното — >= 5x the CPV-code median (peers >= 10, >= €50k)

Context signals (единствена оферта / без обявление) raise the 0-100
risk score but never create a row alone.

Implementation:
- packages/db/migrations/0000_init.sql: add cpv_price_stats and
  contract_anomalies tables + indexes; schema-parity test updated
- scripts/precompute.sql §7: build both tables (median via window
  functions, scoring via sub-select); verified 32 107 anomalies, €37.4bn
- scripts/refresh-slice.sql: add @refresh-batch anomalies (scoped to
  refresh_touched_contracts; validated convergence with full precompute)
- packages/config: ANOMALY_SIGNALS taxonomy (key / label / description /
  qualifying flag)
- packages/api-contract: AnomalyListItem, AnomaliesSummary, AnomalyFacets,
  AnomalySignals DTOs
- packages/db/src/queries/anomalies.ts: listAnomalies (keyset), summary,
  getAnomalyFacets — all predicates on the small anomaly table, not
  the 190k-row contracts scan
- packages/db/src/queries/anomalies.test.ts: 8 pure-TS tests
- apps/web/app/routes/anomalies.tsx: full list page with FilterRail
  (signal / sector / year / value), score badge, ratio badges inline
- routes.ts, SiteHeader nav, sitemap-pages, PAGE_SIZE, PARAM_ORDER updated
- methodology.tsx: new section 9 documenting thresholds and caveats
- README: new route row, updated status/roadmap copy

All 176 pure-TS tests pass; typecheck 7/7 packages; page smoke-tested
at HTTP 200 with working filters (373 annex-growth contracts in 2024).
Review triage (lyubomir-bozhinov, ydimitrof) — none blocking, all applied:

- move the anomaly DDL out of 0000_init.sql into a numbered migration
  (0005_anomalies.sql) per current convention; IF NOT EXISTS on purpose,
  since precompute/refresh-slice create the tables defensively on
  databases migrated before the file landed. migrations.test.ts now
  applies the whole chain in filename order (F2)
- guard the intentionally duplicated derive/scoring SQL with
  @anomaly-derive markers + anomaly-parity.test.ts: the shared blocks in
  precompute.sql §7 and refresh-slice.sql must stay byte-identical, and
  score/rank_value must carry the same weights (F3 / no-duplication note)
- add anomaly-precompute.test.ts: behavioural tests of §7 against a real
  SQLite database — cohort median (incl. even-n and self-inclusion),
  each threshold tier, framework awards>lots exclusion, and the
  context-signals-never-create-a-row rule (test-coverage note)
- clarify convergence semantics (F1): the daily slice re-derives touched
  contracts against the LAST computed medians; cpv_price_stats refreshes
  on full import only. Stated in the refresh-slice header and on
  /methodology#flags, alongside the peer-set self-inclusion note
- extract the badge mapping into apps/web/app/lib/anomaly-badges.ts with
  unit tests (UI-coverage note); this also removes a latent
  toLocaleString('bg-BG') dependency — workerd does not carry the bg-BG
  Intl data, so formatting is hand-rolled like @sigma/shared
- run the list page and its summary with Promise.all in listAnomalies
  (one fewer sequential D1 round-trip)
- realign the README route-table pipes
@Mupaky
Mupaky force-pushed the feat/anomaly-screen branch from 5cae5dc to 44224d7 Compare July 14, 2026 13:48

@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: feat(anomalies) — автоматичен екран за ценови аномалии (партида 1 от 2)

ВЕРДИКТ: COMMENT — качествена реализация; един непреднамерен артефакт в app.css за изчистване, без блокиращи проблеми по сигурността или коректността в тази партида.

Обобщение

Реализацията е чиста, добре структурирана и с много силно тестово покритие. Дисплей-логиката (anomaly-badges.ts) е изнесена от route компонента и е unit-тествана; заявките (queries/anomalies.ts) четат от предварително изчислена таблица и джойнват доменните таблици само за 15-те показани реда; методологията и README са синхронизирани с новата функционалност.

Сигурност (Phase 0 + ниво агент) — ЧИСТО

  • SQL инжекции: Няма. Всички потребителски стойности минават като bind параметри (? + .bind). Имена на колони/сортиране идват само от фиксирани allow-list таблици (SORTS, SIGNAL_COLUMNS, VALUE_BUCKETS), изградени чрез lookup(...). Тестовете потвърждават, че резервирани ключове (toString, constructor) не достигат до SQL, а водят до безопасен 1=0 или подразбиращо се сортиране, а не до нефилтриран списък. Това е коректно OWASP-съобразено поведение (fail-closed).
  • Тайни / URL-и / зависимости: Няма добавени тайни, променени URL адреси, нови пакети, обфускация или бекдор шаблони.
  • Валидиране на вход: authority се bind-ва като 'auth:' + стойност; невалиден bidder slug → 1=0 (празен резултат вместо изтичане на данни). Добре.

Забележки за изчистване (незадължителни)

  1. apps/web/app/app.css — на ред 1 е въведен BOM символ (U+FEFF) преди @import 'tailwindcss', а на последния ред е премахнат крайният нов ред. И двете промени са несвързани с функционалността (лек scope creep / вероятно случаен артефакт от редактора). Виж инлайн коментара.
  2. anomaly-badges.ts / formatTimes — съотношения в интервала ~99,95–99,99 се закръглят до 100 в клона „< 100" и се изписват като „×100" без разделител за хиляди, за разлика от клона „≥ 100". Козметичен граничен случай, не е дефект.
  3. queries/anomalies.tsCOALESCE(an.amount_eur, ...) в value-asc/value-desc, докато amount_eur е NOT NULL в схемата (0005_anomalies.sql). Безобиден защитен код.

Съответствие с CLAUDE.md / обхват

  • Без частична реализация, без TODO, без дублиране (парити тестът пази срещу разминаване между precompute.sql §7 и refresh-slice.sql).
  • Изчерпателни, смислени тестове (поведенчески срещу реален SQLite, парити, query-shape) — не са „cheater" тестове.
  • Именуването и шаблоните следват съществуващите route/query конвенции.

Важна бележка за обхвата на прегледа

Праговете и логиката за скоринг живеят в scripts/precompute.sql §7 и scripts/refresh-slice.sql, които са в другата партида и нямат показан patch тук. Парити тестът (anomaly-parity.test.ts) и поведенческият тест (anomaly-precompute.test.ts) дават добра увереност, но самият SQL (напр. изчисление на медиана, изключване на рамкови споразумения/ДПС, value_flag = 'ok' филтъра) трябва да бъде верифициран в онази партида, тъй като там е реалната повърхност за коректност/инжекции при генериране на SQL.

Отлична работа по сигурността и тестовете.

Comment thread apps/web/app/app.css Outdated
@@ -1,4 +1,4 @@
@import 'tailwindcss' source('.');
@import 'tailwindcss' source('.');

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.

На този ред е въведен BOM символ (U+FEFF) непосредствено преди @import 'tailwindcss' (виждан в diff-а като невидим знак между + и @import). Освен това на последния ред на файла е премахнат крайният нов ред (\ No newline at end of file).

И двете промени изглеждат непреднамерени и несвързани с функционалността за аномалии. BOM в началото на CSS файл обикновено се толерира от браузърите, но е добре да се премахне, за да се избегнат евентуални проблеми с CSS парсера/Tailwind v4 и за да остане diff-ът фокусиран. Молбата ми е да върнете реда без BOM и с краен нов ред:

@import 'tailwindcss' source('.');
...
@import './styles/pages.css';\n

PowerShell Set-Content wrote a UTF-8 BOM (U+FEFF) at the start of
app.css and dropped the trailing newline during the conflict resolution
of the modular-CSS rewrite. Both are unrelated to the anomalies feature.
Fixed by reading and writing the raw bytes.
@Mupaky
Mupaky force-pushed the feat/anomaly-screen branch from dfc5748 to d4969b5 Compare July 15, 2026 07:38
The assistant feature (merged upstream) added Vectorize + AI bindings to
wrangler.jsonc. These cannot be emulated by miniflare and the @cloudflare/
vite-plugin@1.3 auto-started a remote proxy session, requiring a Cloudflare
login even for local dev. Set remoteBindings: false (a top-level CloudflarePlugin
option, not inside experimental) so the local server starts offline; the
assistant route falls back to a 503 without Vectorize/AI available.

@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.

Прегледах стриктно наново на връх 174888a (делтата от предишното ми ревю е несубстанциална — само BOM/newline в app.css + remoteBindings:false в vite.config.ts за offline local dev). Всичките ми бележки са адресирани коректно — снемам предишния COMMENT и одобрявам.

  • F2 (migration). DDL-ът е преместен в нов номериран 0005_anomalies.sql; append-ът към 0000_init е върнат. Header-ът показва вярно разбиране на applied-migration модела (и на 0002/0004 cross-PR колизията): IF NOT EXISTS, защото таблиците може вече да съществуват на жива D1, а на прод новият номериран файл реално се прилага през migrations apply.
  • F3 (дублиран scoring SQL). anomaly-parity.test.ts assert-ва, че @anomaly-derive регионите са byte-идентични между precompute §7 и refresh-slice, с non-vacuity инварианти (праговете) + pin-ване на score ≡ rank_value. Точно drift-guard-ът, който исках; no cheater tests.
  • F1 („converge identically"). Смекчено до вярното — /methodology вече казва „не в реално време… само за променените договори", а refresh-slice коментарите — че медианите се опресняват само на пълен импорт.

Остават само cross-PR координации, вече отбелязани в описанието: дедуп на CPV-кохортната статистика с #210 (най-важното — две паралелни изчисления пипат същите файлове и ще дрейфнат) и getDb миграция за loader-а, щом #225 влезе. Отлична работа по обратната връзка.

Closes #41. Одобрено; blocked на required CI.

@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.

Преглед — batch 2/2: scripts/precompute.sql и scripts/refresh-slice.sql

Прегледът обхваща само двата SQL файла от тази партида (аномалиен скрийн: cpv_price_stats + contract_anomalies).

ВЕРДИКТ: COMMENT

Сигурност (Phase 0 — критичен скан): CLEAN

  • Няма твърдо кодирани тайни (API ключове, пароли, токени). Единствената константа е фиксираният курс BGN→EUR 1.95583 — коректен и очакван.
  • Няма нови/променени URL адреси и няма нови зависимости.
  • Няма риск от SQL injection: това са статични скриптове, изпълнявани през wrangler d1 execute; няма динамично конкатениране на низове или потребителски вход. Стойностите на procedure_type са твърдо изброени константи.
  • Няма бекдори, обфускация или инжектиране на код. OWASP е практически неприложим (няма входна повърхност).

Коректност и цялостност на данните — детайлен анализ

Логиката е добре обмислена, а NULL-обработката е коректна навсякъде:

  • Медианата в cpv_price_stats (WHERE rn IN ((n+1)/2,(n+2)/2) + AVG(eur)) е коректна за четно и нечетно n. MAX(n) = peers (n е константен в партицията). ✔
  • MIN(100, …) и MAX(COALESCE(...),1) са скаларни функции на SQLite (2+ аргумента), не агрегати — коректно. ✔
  • Праговете за флаговете гарантират, че over_ratio/growth/ratio не са NULL, когато съответният флаг е 1. ✔
  • Изключването на рамкови/DPS договори чрез aw.n <= MAX(COALESCE(t.num_lots,0),1) е коректно; в refresh-slice aw брои ВСИЧКИ договори на засегнатите поръчки (не само touched) — правилно за пълния award-count. ✔
  • Корпусът за медианата и за derive е консистентен (value_flag='ok' AND amount_eur>0). ✔
  • single_bid/no_notice са само контекст и не създават ред (WHERE flag_over=1 OR flag_annex=1 OR flag_outlier=1) — съответства на документацията. ✔

Основни забележки (виж inline коментарите)

  1. Дублирана скоринг-логика в рамките на един и същ SELECT (score и rank_value повтарят идентичен MIN(100, CASE…) блок). Parity тестът пази идентичност МЕЖДУ файловете, но не и вътрешната консистентност между score и rank_value. Ако някой промени теглата само на едното копие, скорът и подредбата се разминават мълчаливо. Препоръка: изчислете rank_value от вече изчисления score във външен SELECT.
  2. Разминаване в маркерните коментари между двата файла (begin/end редовете съдържат различен текст — „here/there“ и различно име на файл). Ако parity тестът извлича включително маркерните редове, ще се счупи. Моля потвърдете, че извличането е ексклузивно спрямо маркерите.
  3. Точно съвпадение на низове за procedure_type (кирилица) — всяка вариация в изходните данни тихо изпуска контекстния скор. Ниска тежест, но си струва нормализация/валидиране.
  4. Схема без миграция: CREATE TABLE IF NOT EXISTS няма да съгласува колони, ако таблицата вече съществува със стара схема. Приемливо сега, но при бъдеща промяна на схемата ще е нужна явна миграция.

Защо COMMENT, а не APPROVE

Кодът е висококачествен и не намирам блокиращ дефект по сигурност или цялостност на данните. Не мога обаче да потвърдя абсолютните качествени гейтове на инструкцията (≥90% покритие, преминаване на anomaly-parity.test.ts, ревюто на другата партида), тъй като репозиторият не е наличен локално и тестовете не могат да бъдат изпълнени. Затова оставям COMMENT — след потвърждаване на parity теста и покритието PR-ът изглежда готов за одобрение.


Забележка: Не са публикувани реални коментари в PR — това е чернова за Ваша проверка, съгласно указанието.

Comment thread scripts/precompute.sql
+ CASE WHEN flag_outlier = 1 THEN CASE WHEN ratio >= 10 THEN 25 ELSE 15 END ELSE 0 END
+ CASE WHEN single_bid = 1 THEN 10 ELSE 0 END
+ CASE WHEN no_notice = 1 THEN 5 ELSE 0 END) * 1e12 + amount_eur AS rank_value,
flag_over, flag_annex, flag_outlier, single_bid, no_notice,

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.

Скоринг-изразът MIN(100, CASE…) е повторен дословно два пъти в един и същ SELECT — веднъж за score (ред 241) и веднъж тук за rank_value. anomaly-parity.test.ts пази байт-идентичност МЕЖДУ файловете, но нищо не пази вътрешната консистентност между score и rank_value. Ако тегло се промени само на едното копие, скорът и подредбата ще се разминат мълчаливо. Препоръка: изчислете score веднъж във вътрешен подзаявка и изведете rank_value = score * 1e12 + amount_eur във външен SELECT, за да елиминирате дублирането (в духа на CLAUDE.md „NO CODE DUPLICATION“).

Comment thread scripts/precompute.sql
CREATE INDEX IF NOT EXISTS idx_anomalies_bidder ON contract_anomalies(bidder_id);
DELETE FROM contract_anomalies;
-- @anomaly-derive begin (byte-identical with refresh-slice.sql; see anomaly-parity.test.ts)
INSERT INTO contract_anomalies (

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.

Маркерният ред @anomaly-derive begin тук съдържа „byte-identical with refresh-slice.sql“, докато в refresh-slice.sql е „…with precompute.sql §7“; аналогично се различават и @anomaly-derive end редовете (here/there). Ако parity тестът извлича блока ВКЛЮЧИТЕЛНО с маркерните редове, ще се провали. Моля потвърдете, че извличането е ексклузивно спрямо маркерите (обикновено така се прави, но си струва да е изрично покрито в теста).

Comment thread scripts/precompute.sql Outdated
substr(t.cpv_code, 1, 2) AS cpv_division, t.authority_id, c.bidder_id,
COALESCE(c.signing_value_eur, c.amount_eur) AS paid_eur,
-- The comparable estimate: only when it covers exactly this award (see header note).
CASE WHEN aw.n <= MAX(COALESCE(t.num_lots, 0), 1) THEN

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.

single_bid/no_notice разчитат на точно съвпадение на низове за procedure_type (кирилица). Всяко разминаване в изходните данни (интервали, регистър, вариант на изписване) тихо изпуска контекстния скор без сигнал. Тежестта е ниска (тези флагове не създават ред), но обмислете нормализация или проверка, че списъкът покрива всички реални стойности в данните.

Comment thread scripts/refresh-slice.sql
+ CASE WHEN flag_outlier = 1 THEN CASE WHEN ratio >= 10 THEN 25 ELSE 15 END ELSE 0 END
+ CASE WHEN single_bid = 1 THEN 10 ELSE 0 END
+ CASE WHEN no_notice = 1 THEN 5 ELSE 0 END) * 1e12 + amount_eur AS rank_value,
flag_over, flag_annex, flag_outlier, single_bid, no_notice,

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.

Същото дублиране на скоринг-израза важи и тук (score спрямо rank_value). Ако приемете рефактора в precompute.sql, приложете идентична промяна и тук, за да остане @anomaly-derive блокът байт-идентичен и да мине parity тестът.

Comment thread scripts/refresh-slice.sql
-- the @anomaly-derive markers is shared verbatim with scripts/precompute.sql §7 and guarded
-- byte-identical by packages/db/src/anomaly-parity.test.ts — edit both files together.
-- cpv_price_stats is intentionally NOT rebuilt here: medians are refreshed only on full import, so
-- between imports touched contracts are re-derived against the LAST computed medians (untouched

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.

Поведението „cpv_price_stats НЕ се преизчислява в слайса“ е ясно документирано и приемливо. Единствено потвърдете, че refresh_touched_contracts винаги е дефинирана преди този блок в пълния файл (извън тази партида), тъй като DELETE … WHERE contract_id IN (SELECT id FROM refresh_touched_contracts) и филтърът в derive зависят от нея.

…provements

Addresses reviewer comments on scripts/precompute.sql §7 and the matching
refresh-slice.sql @refresh-batch anomalies block:

Score/rank_value duplication (CLAUDE.md NO CODE DUPLICATION):
  The MIN(100, …) scoring expression was copy-pasted twice in the same
  SELECT (once for score, once for rank_value). Introduce a scored
  subquery that computes score once; the outer SELECT derives
  rank_value = score * 1e12 + amount_eur. Both files stay byte-identical
  between the @anomaly-derive markers (parity test still passes).

anomaly-parity.test.ts — four new / updated tests:
  - 'computes score once' — replaces the old regex-based rank≡score test;
    asserts exactly one MIN(100, …) occurrence and that rank_value uses
    score directly.
  - 'marker extraction is exclusive' — explicitly asserts that the marker
    lines themselves (which differ between files) are never part of the
    extracted region; proves the parity comparison is not sensitive to
    the intentional between-file differences on those lines.
  - 'covers all competitive procedure_type values' — imports PROCEDURE_GROUPS
    and checks every competitive===true type appears in the single_bid IN
    list, catching any silent Cyrillic mismatch.
  - 'covers all direct/no-notice procedure_type values' — same for
    competitive===false / no_notice clause.
  - 'refresh_touched_contracts created before anomalies batch' — asserts
    the setup batch creates the temp table before any anomalies-batch
    DELETE/WHERE depends on it.

@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: feat(anomalies): add automated price-anomaly screen

ВЕРДИКТ: COMMENT — без блокиращи проблеми по сигурността. Има няколко открити въпроса по целостта на данните и защитата на ресурси, които е добре да се потвърдят преди APPROVE.


Какво прави PR-ът

Добавя автоматизиран екран за ценови аномалии (/anomalies) с три ценови сигнала (над прогнозата ≥10%, ръст чрез анекси ≥20%, ≥5× над медианата по CPV) и два контекстни сигнала (единствена оферта, без обявление), рисков скор 0–100, навигация, sitemap и нов раздел „9. Аномалии и сигнали" в методологията. Праговете в config, методологията и Callout копието са взаимно консистентни. Обхватът е фокусиран, без scope creep.

Сигурност (Фаза 0) — ЧИСТА ✅

Прегледът и на двете партиди не откри проблеми по сигурността:

  • SQL инжекция: всички потребителски входове минават през ?-placeholder-и (.bind(...)); имена на колони/изрази за сортиране идват само от whitelist-ове (SORTS, SIGNAL_COLUMNS, VALUE_BUCKETS); непознати signal-ключове колапсват до 1=0 (празен резултат, не нефилтриран списък).
  • XSS: всички динамични полета се рендират през стандартното escape-ване на React; няма dangerouslySetInnerHTML.
  • Няма твърдо кодирани тайни, нови/променени URL-и, нови зависимости или следи от зловреден код.

Качество на кода — отлично

  • Чисто разделяне: display мапингът (anomaly-badges.ts) е изведен от route компонента и е unit-тестван.
  • Споделеният derive/scoring блок между precompute.sql §7 и refresh-slice.sql е маркиран и се пази byte-identical чрез anomaly-parity.test.ts (добра защита срещу дрейф).
  • Проверено: INSERT↔SELECT колонен мапинг (19↔19) и toItem — коректни; медианата по CPV е математически вярна за четно/нечетно n; деленията са защитени (est_eur > 0, median_eur > 0, peers >= 10).
  • Достъпност: caption.sr-only, scope="col", aria-busy, семантична таблица.
  • Отбранителна миграция (IF NOT EXISTS) с guard тестове върху схемата и rank_value NOT NULL.

Открити въпроси (не блокиращи, за потвърждение)

  1. Цялост на данните — orphan редове. listAnomalies прави INNER JOIN към contracts/tenders/authorities/bidders, докато anomaliesSummary и getAnomalyFacets четат само contract_anomalies. Ако ред сочи изтрит договор или обединена организация, списъкът го крие, но COUNT/SUM и фасетите го броят → total/valueEur може да надвишат видимите редове. refresh-slice.sql трие само редове за refresh_touched_contracts, а FK REFERENCES contracts(id) не се налага автоматично в D1/SQLite.
  2. Защита на ресурси (DoS). pageSize = p.pageSize ?? 15 няма горна граница — ако стойността идва от външен вход, огромен pageSize води до огромен LIMIT. Препоръка за clamp.

Дребни забележки (за изчистване)

  1. Дублиран тестов блок в packages/db/src/anomaly-parity.test.ts — вторият describe(...) в края повтаря дословно тестовете от първия (изглежда като случаен copy-paste). Препоръка: премахнете дублирания блок.
  2. Несъгласуван default за sort: loader-ът ползва ... || 'score-desc' (празен низ → default), а компонентът ... ?? 'score-desc' (запазва празен низ). Няма функционален риск (query слоят валидира), но си струва уеднаквяване.
  3. Мъртъв код: COALESCE(an.amount_eur, ...) в SORTSamount_eur е NOT NULL.
  4. Перф. бележка: филтрите по година (substr(an.signed_at,1,4)) и по сектор (an.cpv_division) не ползват съществуващите индекси. Приемливо при малката contract_anomalies таблица, но заслужава преоценка при разрастване.

Заключение

Кодът е с високо качество, добре тестван и без открити уязвимости. Нищо не блокира merge от гледна точка на сигурността. Преди APPROVE препоръчвам да се потвърдят двата въпроса по целостта на данните (orphan редове) и защитата на ресурси (clamp на pageSize), както и да се изчистят дребните забележки (най-вече дублирания тестов блок).

p: AnomalyListParams,
): Promise<AnomalyListResult> {
const sort = SORTS[p.sort as keyof typeof SORTS] ?? SORTS['score-desc'];
const pageSize = p.pageSize ?? 15;

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.

⚠️ Защита на ресурси / OWASP A04 (неконтролирано потребление): pageSize се взима директно (p.pageSize ?? 15) без горна граница, а после се подава като LIMIT pageSize + 1. Ако стойността достига дотук от външен вход, заявка с много голям pageSize ще извлече целия набор → риск от DoS/натиск върху паметта. Препоръка: clamp, напр. const pageSize = Math.min(Math.max(p.pageSize ?? 15, 1), 100);. Ако валидацията се прави в маршрута (друг batch), моля потвърдете това изрично.


EN: Resource-safety (OWASP A04). pageSize is taken as p.pageSize ?? 15 with no upper bound and fed into LIMIT pageSize + 1. If this value originates from external input, a huge pageSize pulls the whole result set → potential DoS/memory pressure. Please clamp to a sane max, or confirm the route layer validates it.

const filters = buildFilters(p);
const row = await db
.prepare(
`SELECT COUNT(*) AS total, COALESCE(SUM(an.amount_eur), 0) AS eur${FROM_BARE}${filters.sql}`,

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.

Цялост/консистентност на данните: обобщението и фасетите четат само contract_anomalies (FROM_BARE), докато listAnomalies прави INNER JOIN към contracts/tenders/authorities/bidders. Ако ред в contract_anomalies сочи изтрит договор или обединена/преименувана организация, списъкът го отхвърля (INNER JOIN), но COUNT(*)/SUM(amount_eur) и фасетите го броят → total/valueEur могат да надвишат видимите редове. Моля потвърдете, че refresh-конвейерът поддържа референциална цялост (виж бележката в refresh-slice.sql), или подравнете двете страни (напр. еднакъв набор от JOIN-ове/EXISTS).


EN: Data consistency. Summary/facets read the bare contract_anomalies table while the list INNER-JOINs contracts/tenders/authorities/bidders. A row pointing at a deleted contract or merged/renamed entity is dropped from the list but still counted by COUNT/SUM/facets → total/valueEur can exceed the rows a user can actually page through. Please confirm the refresh pipeline preserves referential integrity, or align both sides.

Comment thread scripts/refresh-slice.sql
authority_id TEXT NOT NULL, bidder_id TEXT NOT NULL
);
DELETE FROM contract_anomalies WHERE contract_id IN (SELECT id FROM refresh_touched_contracts);
-- @anomaly-derive begin (byte-identical with precompute.sql §7; see anomaly-parity.test.ts)

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.

Цялост на данните: тук изтриваме аномалии само за refresh_touched_contracts. При пълно преизчисление (precompute.sql) таблицата се чисти изцяло (DELETE FROM contract_anomalies), но в slice-а редовете за договори, които са били ИЗТРИТИ (а не само променени), ще останат като orphan-и, освен ако изтритите id-та влизат в refresh_touched_contracts. Външният ключ REFERENCES contracts(id) не е гаранция — в SQLite/D1 FK не се налага без PRAGMA foreign_keys=ON, и няма ON DELETE CASCADE. Моля потвърдете, че изтриванията на договори се включват в touched-набора; иначе orphan редовете ще изкривят обобщението/фасетите (виж коментара в anomalies.ts).


EN: Integrity. This DELETE only clears rows for refresh_touched_contracts. Full precompute wipes the whole table, but the slice leaves rows for DELETED contracts as orphans unless removed ids are part of refresh_touched_contracts. The REFERENCES contracts(id) FK is not enforced by default in D1/SQLite and has no ON DELETE CASCADE. Please confirm deletions reach the touched set.

const SORTS: Record<AnomalySort, { expr: string; dir: 'asc' | 'desc' }> = lookup({
'score-desc': { expr: 'an.rank_value', dir: 'desc' },
'value-desc': { expr: 'COALESCE(an.amount_eur, -1)', dir: 'desc' },
'value-asc': { expr: 'COALESCE(an.amount_eur, 1e18)', dir: 'asc' },

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.

Дребно (не блокер): COALESCE(an.amount_eur, -1) и COALESCE(an.amount_eur, 1e18) са защита срещу NULL, но amount_eur е деклариран REAL NOT NULL в схемата на contract_anomalies, т.е. NULL-клоновете са мъртъв код. Може да се опрости за яснота (не влияе на коректността).


EN: Minor/dead-code. The COALESCE NULL fallbacks on amount_eur never trigger because the column is NOT NULL. Harmless, but can be simplified.

@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.

Прегледах делтата 174888a→99f35c7 (score-dedup + подсилени тестове). Одобрението ми остава.

Рефакторът е коректен — проверих го емпирично. Дублираният MIN(100, …) (втората копия в rank_value) вече се смята веднъж като score, а rank_value = score * 1e12 + amount_eur реферира към него. Възпроизведох старата и новата вложеност локално в SQLite с adversarial seed (NULL growth, отрицателен amount_eur, гранични ratio под праговете): двете дават идентичен резултат — същите редове, byte-identical rank_value (вкл. отрицателния: 74999999980000). Преместеният WHERE flag_over = 1 OR flag_annex = 1 OR flag_outlier = 1 реферира SELECT-list alias в същия SELECT — SQLite го резолвва коректно (за разлика от Postgres), а filter-ът пази същото множество редове (флаговете не зависят от score, MIN(100,…) е скаларен, не агрегат). Затваря забележката за дублираната scoring-експресия чисто.

Новите тестове са реално подсилване, не запълване: покритието по PROCEDURE_GROUPS (single_bid / no_notice) е ценен guard — хваща дрейф между канoничния source-of-truth и hardcode-натите IN листи; marker-exclusive извличането и подредбата на refresh_touched_contracts преди anomalies-batch-а са добри инварианти. No cheater tests.

Дребно (не блокира): describe('anomaly derive parity …') е деклариран два пъти (редове 29 и 124), а it('keeps the shared derive/scoring block byte-identical') и it('keeps the clean-corpus scope predicate …') се появяват и в двата блока. Vitest ги пуска всичките (безвредно), но е остатък от долепянето на тестове — слей в един describe, запази версията с invariant loop-а на byte-identical теста и махни дубликатите.

Одобрявам.

nikimilenkov added a commit to nikimilenkov/sigma that referenced this pull request Jul 16, 2026
… review)

The 14 subject-risk columns were added by editing 0000_init.sql. The served D1
is persistent and `wrangler d1 migrations apply` is filename-tracked, so an
edit to the already-applied 0000_init reaches the work DB (green local CI) but
never prod — precompute's UPDATE would then fail on missing columns (the
midt-bg#188/midt-bg#239 trap the reviewer reproduced).

Extract the columns into 0006_subject_risk_columns.sql (ALTER TABLE ADD
COLUMN) and remove them from 0000_init (SQLite has no ADD COLUMN IF NOT EXISTS,
so a fresh DB can't carry both). 0006 clears the 0002 claimants
(midt-bg#226/midt-bg#193/midt-bg#172) to avoid a duplicate-version at apply. The precompute
CREATE TABLE IF NOT EXISTS mirror stays a no-op on the existing tables. The
four risk tests now apply 0006 after 0000_init, mirroring the served-D1 chain.

Reword the 0000_init header to distinguish the rebuilt-every-import work DB
from the persistent served D1, so the next person adds objects via a new
migration instead of editing 0000_init. Also document that value_flag='ok' is
the exact complement of the hidden suspect set.
nikimilenkov added a commit to nikimilenkov/sigma that referenced this pull request Jul 17, 2026
… review)

The 14 subject-risk columns were added by editing 0000_init.sql. The served D1
is persistent and `wrangler d1 migrations apply` is filename-tracked, so an
edit to the already-applied 0000_init reaches the work DB (green local CI) but
never prod — precompute's UPDATE would then fail on missing columns (the
midt-bg#188/midt-bg#239 trap the reviewer reproduced).

Extract the columns into 0006_subject_risk_columns.sql (ALTER TABLE ADD
COLUMN) and remove them from 0000_init (SQLite has no ADD COLUMN IF NOT EXISTS,
so a fresh DB can't carry both). 0006 clears the 0002 claimants
(midt-bg#226/midt-bg#193/midt-bg#172) to avoid a duplicate-version at apply. The precompute
CREATE TABLE IF NOT EXISTS mirror stays a no-op on the existing tables. The
four risk tests now apply 0006 after 0000_init, mirroring the served-D1 chain.

Reword the 0000_init header to distinguish the rebuilt-every-import work DB
from the persistent served D1, so the next person adds objects via a new
migration instead of editing 0000_init. Also document that value_flag='ok' is
the exact complement of the hidden suspect set.
@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Този клон е в конфликт с main, тъй че към момента не може да се ревюира — дифът, който GitHub показва, вече не отговаря на това, което би влязло. Ще го пребазираш ли върху актуалния main (или merge на main в клона) и да разрешиш конфликтите? След това веднага го поглеждам. Благодаря! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data-quality Качество/коректност на данните (ETL, регистри) enhancement Нова функционалност или предложение web Област: web

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Рисков флаг: цена-отклонение спрямо съпоставима кохорта (CPV)

4 participants