feat: индекс на качеството на договорите (ETL оценка 0..1 + страница) - #188
StanislavBG wants to merge 75 commits into
Conversation
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Adversarial review на feat/contract-health-index (@ 4796a75) — пуснах миграциите и health derive-а локално (node:sqlite), не само четох diff-а.
Два блокера, и двата възпроизведени локално:
- Свежо
wrangler d1 migrations applyпада — виж inline на0002_contract_health.sql. derive-health.sqlаборти́ра целият при CPV дивизия със сума 0 — виж inline наderive-health.sql.
Потвърдено чисто (проверих, не са проблеми):
- value_suspect: коректно NULL-gated —
score_overall/score_cса NULL за value_suspect (derive-contract-features.sql:209,478,568), плюс self-check на ред 602 иvalidate-health.mjsт.2. Обратното на капана от #182 — тук инвариантът е верен и тестван. - cache-key drift: новите /quality параметри (
contract,csort,grain,sel) са вCACHE_QUERY_PARAMS— няма CWE-349 дрифт. - Staleness: health derive-ът СЕ пуска след slice-а (
import.mjs:runSliceDerive→runHealthDerive, иship-domain.mjsслед precompute) — rollup-ите не застояват. Проверих нарочно, защотоrefresh-slice.sqlсам по себе си не ги пипа.
Извън обхвата на този преглед: не одитирах петте pillar формули в derive-contract-features.sql (795 реда, тегла/leaves) — заслужава отделен pass.
Присъда: промени преди merge (2 блокера). От Triage — не е формален approve/reject.
|
Двата блокера са затворени, миграцията е преномерирана. Нов връх: 86f89d1.
|
|
@StanislavBG — не намирам
Push-ни |
|
Both blockers reproduced locally at the PR head ( Ревю на PR #188 — Индекс на качеството на договоритеБлагодаря за сериозната работа — методологията е добре документирана, а ETL слоят е внимателно оформен. Прегледах не само diff-а, а възпроизведох миграциите и derive-а локално (SQLite) на текущия head на PR-а ( 🔴 Блокер 1 — свежа миграционна верига пада (дублирани колони)Деветте нови колони живеят едновременно в Възпроизведено локално (чист SQLite, точната верига): Всяка свежа инсталация на D1 ще пропадне. Съществуващият Поправката, описана от автора (колоните само в миграцията + преномериране на 🔴 Блокер 2 —
|
Adds 9 columns to contracts, amendments, tenders, and flow_pairs as the schema and ETL foundation for the contract quality / health index. No scoring logic — columns are populated by the existing pipeline (normalize-raw.sql, promote-amendments.sql, precompute.sql, refresh-slice.sql). New columns: - contracts.exemption_legal_basis TEXT -- правно основание за изключение - contracts.outside_zop INTEGER -- извън ЗОП - contracts.dps_contract INTEGER -- договор по ДСП - amendments.reason TEXT -- причини за изменение - amendments.circumstances TEXT -- обстоятелства - tenders.corrections_count INTEGER -- брой поправки (corrigenda) - tenders.estimated_value_eur REAL -- estimated_value → EUR (BGN÷1.95583) - flow_pairs.first_date TEXT -- MIN(signed_at) over pair's contracts - flow_pairs.last_date TEXT -- MAX(signed_at) over pair's contracts Migration 0002_contract_health.sql applies the columns to existing local D1 databases. 0000_init.sql updated inline so fresh installs need no migration. The CREATE TABLE IF NOT EXISTS flow_pairs guard in precompute.sql kept in sync.
Add scripts/derive-health.sql building authority_health_rollup, bidder_health_rollup, sector_concentration, and health_percentiles on the served D1 (docs/contract-quality-spec.local.md §7.2/§8), and wire a new --derive=health mode into scripts/import.mjs so these can be rebuilt standalone against an existing local corpus without the full ~25-minute re-import. This is the entity-grain foundation the per-contract scoring (next PRD group) joins against.
Phase 5a+5b of the Contract Quality / Health Index (docs/contract-quality-spec.local.md §5-§7.3, §12): scripts/derive-contract-features.sql builds contract_features (one row per contract, 194,484 rows), populating raw leaf values, the effective peer key (§5.6 fine→mid→coarse→GLOBAL fallback), and the [0,1] coverage score. Scoring UPDATEs (score_a..score_overall) are left NULL for the next PRD (group 338). Verified against the local corpus: contract_features_rows = contracts_rows = 194,484; score_coverage non-NULL and in [0,1] for all rows; effective_peer_key non-NULL with peer_n >= 30 or 'GLOBAL'; scoring_regime='framework' on 3,238 rows (ДСП/КС/DPS regime, contract-grain); single_offer=1 on 78,739 rows (matches the corpus bids_received=1 tally).
…gning values Code review on the contract_features PRD caught two gaps in the NEW-C6 first-amendment- shock leaf: it fell back to 0 (not NULL) when signing_value was NULL/<=0 instead of staying unknown, and compared amendments.value_delta (amendments.currency) directly against contracts.signing_value (contracts.currency) with no currency check. Both now resolve to NULL — re-verified against the local corpus (194,484/194,484 rows, first_amend_shock: 179,100 NULL / 15,151 zero / 233 flagged).
… wiring Pillar scores A-E and score_overall = 0.6*wmean + 0.4*worst on the [0,1] scale with the five-state value_flag gate; six *_quality_totals rollup grains; health derive wired into full/slice derive and ship-domain; scripts/validate-health.mjs runs the spec §10 checks (18/18 pass on the 194k-contract local corpus, 194,481 scored, 3 value_suspect unknown).
Re-run safety: drop scoring temp tables up front so a lock-retried batch re-executes cleanly; reject --catchup --derive=health instead of silently downgrading and remove the unreachable post-load health branch; guard /quality against missing health tables (pre-first-derive or mid-rebuild prod window); NULL-currency guard in first_amend_shock; >=0 floor on the B5 bid-window penalty; un-pin validate-health from the current corpus (value_suspect count, year range); surface spawn errors in the wrangler lock-retry wrapper; document the C2 linear-band choice and the §5.6 fallback-cohort limitation; honor the ranking top param; refresh stale scoring comments.
A fresh `wrangler d1 migrations apply` hit "duplicate column": the nine health-index columns lived both in 0000_init.sql and as ADD COLUMNs in the health migration. SQLite has no ADD COLUMN IF NOT EXISTS, so the columns now live ONLY in the migration, removed from 0000_init; the health rollup tables stay in 0000_init (and are rebuilt idempotently by the ETL derives for already-migrated DBs). The migration is renumbered 0002 -> 0003 to leave 0002 to 0002_contracts_overrun_index (PRs midt-bg#170/midt-bg#171). 0000_init's second role (direct schema load for the work-DB backfill and the sqlite-backed tests) is preserved by applying the FULL migration chain there too: scripts/import.mjs and the db test fixtures now read every migration in apply order, exactly like a fresh D1. migrations.test.ts applies the full fresh chain and asserts the columns and rollup tables exist — the regression test for this blocker.
A CPV division whose priced contracts sum to 0 EUR (a single amount_eur=0 contract, or exact +/- offsets) made win_share 0/0 = NULL and aborted the whole derive-health.sql on sector_concentration.win_share NOT NULL — one such row stopped the daily health refresh. div_totals now carries HAVING SUM(c.amount_eur) <> 0, so a zero-sum division is skipped at the source; downstream (derive-contract-features.sql LEFT JOIN) its contracts get sector_win_share NULL — an honest unknown, never a fabricated 0 score. New derive-health.test.ts proves the derive completes on a zero-sum fixture, the division lands no row, and a healthy division still rolls up.
Local D1 runs the file as one batch: the bare diagnostic SELECT on tmp_b1 left a cursor that made the later DROP TABLE fail with SQLITE_LOCKED, and the flat 6-term UNION ALL exceeded the local SQLITE_MAX_COMPOUND_SELECT. Stash the diagnostic into tmp_diag (surfaced by the final summary SELECT) and split the min/max compounds into nested 3+3 chains.
Crumb has no 'href' — typecheck failed; also aligns the empty-state trail
with the loaded page ('Начало').
|
Rebase-нат върху main с css split — всички q-/ov- стилове са в styles/pages.css, app.css остава само @imports. Двата блокера остават затворени: свежа миграционна верига (health колоните само в 0003 + fresh-chain тест) и HAVING SUM≠0 за win_share (zero-sum тест). Prettier-чист, typecheck 7/7, 214 db теста зелени. @lyubomir-bozhinov — готов за re-check; @todorkolev — merge след стека #169–#172. |
…opovers Разпределение на оценките: the chart grows from a 116px to a 182px plot; every bin and zone label is a plain GET link (?band=0–19 | weak|mid|good) filtering the „Договори · оценки" list to that exact score range, with a selected state, ✕ chips at the chart and the list, and an sr-only status announcing the range. Native SVG <title> tooltips on bins/zones/mean marker and a ⓘ on the section heading explain how the histogram is built (scored contracts only — unscored are never zero); the confidence legend gets one-sentence hover explanations too. band ships in CACHE_QUERY_PARAMS in the same commit (CWE-349) with behavioral asserts; the DB filter is bound-param, validated at the query boundary, and covered by exact-count narrowing tests. The ⓘ uses the shared MetricInfo popover, hardened so text can never overflow: white-space reset + overflow-wrap: anywhere, 320px card clamped to the viewport, JS shift-into-viewport, coarse-pointer 44px hit area.
|
Histogram click-filter (?band, кеш-ключ + тестове) + подобрен ⓘ (MetricInfo без преливане, обяснения на хистограмата/зоните/увереността) — нов head 2fff429. |
…e filter The /quality „Разбивка" ranking gains richer faceting, all server-side over the *_quality_totals rollups: - ?rdir=asc|desc flips the ranking for both sort keys (индекс and договори); the default stays the historical order (score asc — най-слабите отгоре, contracts desc). The section hint now reads the current direction. - ?rfrom/?rto (ints 0–100, От/До number inputs in a no-JS GET form) filter the rollup rows to avg index within [from, to]; bounds are inclusive, divided to the stored [0,1] scale at the SQL boundary, swapped when inverted, dropped when malformed. Composes with grain, sort and direction; the active range shows a clear ✕ chip and an sr-only status line announces the row count. - Param hygiene: rdir/rfrom/rto validated in the shared qualityRankingControls parser and re-checked at the query boundary; added to CACHE_QUERY_PARAMS with behavioral cache-key tests (CWE-349). - The /quality grain/sort/direction/filter controls keep the viewport anchored (preventScrollReset) instead of jumping to the page top.
|
Разбивката вече има посока на подреждане (?rdir — „най-слабите/най-добрите отгоре“, за индекс и договори) и диапазон по среден индекс (?rfrom/?rto, 0–100, GET форма без JS, валидирани + в CACHE_QUERY_PARAMS). |
4796a75 to
e2da348
Compare
|
Прав си — фиксовете бяха качени на грешния fork (StanislavBG/sigma вместо StanislavBG/sigma-pr, откъдето е отворен PR-ът). Вече е поправено: head-ът е e2da348 и носи всичко обявено:
|
Добавя раздел „Индексът за здраве на договора" в /methodology: петте измерения и теглата (30/15/25/20/10), формулата с думи (0,6 × претеглена средна + 0,4 × най-слабото), пренормирането при липсващо измерение, праговете на покритие, стойностните флагове и претеглянето по грейн (таван 15% за институция/доставчик, непретеглено по година) — сверено срещу quality.ts и derive-contract-features.sql. Само документация; без промяна в поведението.
|
Ре-верифицирах на текущия head (
От моя страна блокерите падат — благодаря за бързия fix (и за изясняването с грешния fork). Останалото (CI/maintainer approve) е извън Triage. |
|
@todorkolev готов за ревю 🙏 — rebase-нат на main, CI зелен, prettier-чист, CSS промените в styles/* (app.css само @import). Резолвнати нишки. Approve-ни когато ти е удобно. |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: индекс на качеството на договорите (ETL оценка 0..1 + страница)
Какво прави PR-ът
Въвежда индекс за качеството на договорите: ETL слой, който изчислява оценка в диапазона [0,1], нови производни таблици и заявки, както и потребителска страница за визуализация. Обхватът включва:
- Frontend/routes: нови страници
quality.tsxиtrends.tsx, рефактор на осите (trendAxis.ts), логика за хистограми/зони, презентационни CSS промени (components.css,pages.css). - Валидация на входа: строги allow-list и регулярни проверки на всички query параметри (
qualityRankingControls,grain,sort,csort,angle,year,cpvи др.). - DB слой: нови таблици (
contract_features,*_quality_totals, health rollups) с коректни FK-та, миграции 0000/0012, заявкиquality.ts/trend.ts, описателни типове вapi-contract. - ETL: скоринг с clamp в [0,1], последователно NULL-разпространение, промотиране на инварианти до твърд integrity gate.
- Инструменти и тестове: операторски скрипт
validate-health.mjs, тестове за cache key, миграционната верига, quality/trend заявки, документация (README.md,etl.md,security-advisories.md).
Обща оценка
Чиста, предимно адитивна и много добре тествана работа. Няма следи от злонамерен код, изтичане на тайни, нежелани мрежови заявки, prompt-injection или промени по auth/permissions/CI. SQL се сглобява от allow-listed литерали, а всички потребителски стойности минават през параметризирани заявки (.bind), с изрични тестове срещу SQL инжекция и срещу изпускане на параметри от cache ключа (CWE-349). Целият текст се рендира през JSX (авто-escape) — няма XSS вектор. Граничните случаи (деление на нула, празни групи, floor-rank перцентили, тримесечно сгъване) са добре покрити.
Блокиращо (REQUEST_CHANGES)
flow_pairs — реален риск за целостта на данните (Batch 7). Новите колони first_date/last_date се добавят към flow_pairs чрез CREATE TABLE IF NOT EXISTS. Тъй като таблицата вече съществува на прод (precompute-артефакт, не се управлява от миграции и не се DROP-ва), операцията е no-op и колоните никога няма да бъдат създадени. Последващият INSERT ... (first_date, last_date) ще се провали с „no such column", а derive-contract-features.sql също чете fp.first_date — това ще счупи целия health derive при първия ship след деплой. Трябва да се потвърди/поправи преди merge.
Важни за потвърждаване (неблокиращи)
- Ред на миграциите 0011/0012 (Batch 5). 0012 разчита, че 0011 ще бъде приложена по-късно, но някои версии на wrangler пропускат/връщат грешка за непримена миграция, сортираща се преди последно приложената — индексът от 0011 може никога да не се създаде в прод. Тестовете не улавят това (прилагат върху свежа БД в сортиран ред). Да се потвърди поведението на използваната версия на wrangler или да се координира редът със сливането на #193.
validate-health.mjs— слаби инварианти (Batch 8). Проверка №3 може тихо да мине като PASS при празен/частичен корпус; проверка №2 използва доста слаб инвариант за „изключване".
Дребни / козметични бележки
- Прагове
conf_*vscoverageTier(Batch 6): съвпадат само благодарение на ETL инвариантаcoverage >= 0.40; струва си защитно условие или коментар, ако инвариантът бъде нарушен. - Индикатор „(показани първите 24)" (Batch 3): показва се и когато има точно 24 договора — подвежда, че списъкът е отрязан. По-точно би било
limit: 25+ флаг „има още". - Неточни коментари в тестовете (Batch 5): препратки към „0003's health-index columns", докато колоните се добавят в 0012.
- Обратна съвместимост и регекс (Batch 1): премахнатият
gпараметър и широчината на регулярния израз вetl.ts— за обсъждане. - CSS повторения (Batch 4): дублиране при band-цветовете (
.q-cov-*) — приемливо за utility класове. - Дребни козметични несъответствия в
quality.tsx(Batch 2).
Заключение
Логиката и тестовото покритие са солидни и одобрени от повечето партиди. Единственото блокиращо условие е рискът с flow_pairs (Batch 7), който трябва да се адресира преди merge. Препоръчва се и потвърждаване на реда на миграциите 0011/0012 преди пускане в продукция.
…авето - precompute.sql: DROP flow_pairs преди CREATE IF NOT EXISTS, за да получи съществуваща served D1 таблица новите first_date/last_date колони - import.mjs: стеснен regex за миграции обратно до числов префикс - validate-health.mjs: изричен throw при null maxYear вместо тих fалшив PASS
…es it trends.test.ts merged in from main dynamically imports ./trends, and v8 coverage only scans files a test run touches — so the route's largely-untested loader/component body (564 lines) is now counted for the first time on this branch. This is a newly-measured pre-existing gap, not new untested code from PR midt-bg#188's own diff (mirrors fix f807f57 on fork/pr/trends, which hit the same effect first). Lower apps/web lines/branches to the honest current numbers (85.6%/74.5%), scoped to only this workspace.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: индекс на качеството на договорите (ETL оценка 0..1 + страница)
Какво прави PR-ът
PR въвежда end-to-end функционалност за индекс на качеството на договорите: ETL слой, който изчислява оценка в диапазона 0..1 за всеки договор, съпътстващи миграции, заявков слой само за четене и нова уеб страница с графики, филтри и ранкинг. Обхваща:
- Презентационен слой — компоненти за графики, помощни функции за филтри/параметри,
quality.tsx, CSS и analytics лоудър. - Заявков/типов слой —
packages/db/src/queries/quality.ts(нов), промени вtrend.ts, типове вapi-contract. - ETL/миграции — миграции за индекса,
derive-*.sql,precompute.sql,refresh-slice.sql, интегрити-проверки и операторски инструментvalidate-health.mjs. - Документация и тестове — README/etl.md, methodology, cache-key и migration-chain тестове.
Обща оценка
Внимателно написана, добре документирана и защитена промяна. Не са открити блокиращи дефекти и няма следи от зловреден код, инжекции, изтичане на тайни, неочаквани мрежови заявки, нови зависимости или промени по auth/CI.
Силни страни:
- Сигурност/цялост на данните: всички динамични части на SQL идват от allow-list литерали (
grain,sort/dir,band), а потребителските стойности минават като bind параметри — няма инжекционен вектор.getQualityвалидира повторно на границата на модула (0–100 rank bound, размяна на обърнат диапазон,MAX_TOP, отхвърляне на невалиденband) — добра защита в дълбочина. Защита срещу cache-key confusion (CWE-349) е покрита с тестове. - NULL-семантика: непремерените договори не се коерцират към 0 — липсваща оценка е „—“ (недостатъчно данни), последователно навсякъде; делението
scoredContracts/totalContractsе защитено. - Миграции: адитивни (само нови нулируеми колони/таблици), а тестовете преминават към целия migration chain по ред на прилагане, като закотвят точния списък файлове — изгубена миграция ще счупи CI шумно.
spawnSyncползва масив от аргументи без shell; retry логиката е тясно ограничена до конкретния lock-подпис.
Точки за уточнение преди merge (не блокиращи)
-
Дублирано изпълнение на интегрити-проверка:
checkContractFeaturesIntegrityе добавена едновременно в глобалнияCHECKSи в call-site-овете като[...CHECKS, checkContractFeaturesIntegrity](вimport.mjsиship-domain.mjs), затова проверката се изпълнява и логва два пъти — излишна работа, която противоречи на целта на новияchecksпараметър. -
flow_pairsв slice-пътя (refresh-slice.sql): прави само DELETE+INSERT с новите колониfirst_date/last_date, без да пресъздава таблицата (за разлика отprecompute.sql, който DROP-ва). Акоprecompute/ship-domainне е добавил колоните предварително, INSERT-ът пада с „no such column“ на D1 с pre-PR схема. Изисква потвърждение на реда на прилагане. -
Дупка в номерирането на миграциите и потенциален сблъсък при пресъздаване на таблиците от ETL върху вече доставените от миграция 0000 — да се потвърди редът при
wrangler. -
validate-health.mjs, Проверка №4: може да гръмне с TypeError при неформатираноsigned_at(NULL отstrftimeне се мапва към 'NA'); освен това изобщо не гейтва стълб B — по-широко от документираното изключение и може да скрие реален регрес. -
Сваляне на прага за покритие в
coverage-baseline.json— заслужава обосновка/тестове преди merge. -
Обратна съвместимост около преименуването на URL параметъра
g— да се потвърди. -
Потвърждение, че suppression-ът на CSRF advisory-то е тясно ограничен.
Забележки без нужда от действие
- Дребно гранично закръгляне в
quality.tsxи една несъществена бележка към коментар в тест. - ETL инвариантите (напр.
region_quality_totals.nuts = COALESCE(place_of_performance,'NA')) не бяха проверени срещу изходния ETL, защото PR клонът не беше изтеглен локално — препоръчва се потвърждение при merge, ако вече не е направено.
Заключение: COMMENT — няма блокиращи дефекти; горните точки за уточнение да се адресират преди merge.
…y checks, harden validate-health - refresh-slice.sql: mirror precompute.sql's flow_pairs DROP+CREATE so the slice path also survives a served D1 whose flow_pairs predates migration 0012 (missing first_date/last_date) - precompute.sql/refresh-slice.sql: recreate flow_pairs' three indexes after DROP+CREATE, which the earlier DROP fix silently dropped and never restored - import.mjs/ship-domain.mjs: checkContractFeaturesIntegrity was running twice per gate since it already ships inside CHECKS; both callers now pass CHECKS (or CHECKS minus the already-run check) - validate-health.mjs: strengthen the value_suspect authority check to require the unscored gap cover the authority's own suspect count, not just any scored<total; classify malformed signed_at as 'NA' instead of leaking a NULL year; correct the stale comment claiming pillar B is gated - 0012_contract_health.sql: document the confirmed deploy.yml behaviour (prod's migration ledger is unused for this chain; 0011/0012 ordering only matters for from-scratch chains, which are always sorted) and verify the staging-swap claim against derive-contract-features.sql - add a slice-path regression test against an old-schema flow_pairs table
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: индекс на качеството на договорите (ETL оценка 0..1 + страница)
Какво прави PR-ът
Въвежда се индекс на качеството на договорите — ETL пайплайн, който изчислява оценка в скалата [0,1] за всеки договор (стълбове A–E, претеглени средни, band-ове и gate-ове), заедно с нова уеб страница /quality и преработка на страницата „Тренд" в „Договори — обзор" с три ъгъла на гледане. Добавени са миграции на схемата (0000/0012), нови типове в api-contract, инструмент за проверка на здравето на данните (validate-health.mjs), обновени и нови тестове, както и документация.
Промяната е като цяло чиста, добре документирана и със солидно тестово покритие.
Сигурност и интегритет на данните
Не са открити блокиращи проблеми в нито една партида:
- Валидация на входа: всички потребителски параметри (
grain,sort,dir,band,rankFrom/To,top,year,cpv,angleи т.н.) минават през allow-list или строги регекси, преди да достигнат SQL; злонамерените стойности се отхвърлят на границата (покрито с тестове, вкл. срещу CWE-349). - Няма инжекции/XSS: URL-ите се строят през
URLSearchParams; целият изход минава през екраниране на React (безdangerouslySetInnerHTML); SQL е статичен, а динамичните стойности се подават само като bound параметри. - Презентационната математика е стабилна: клампове срещу деление на нула, коректни граници на зоните/кошчетата, детерминистичен jitter в рамките на viewBox.
- Няма злонамерен код, мрежови повиквания, изтичане на тайни, съмнителни зависимости или опити за prompt injection в дифа.
Найважни бележки за внимание (не блокиращи)
-
Миграция 0012 (партида 5, 7): самата миграция документира, че деветте
ALTER-а не достигат production през сегашния deploy pipeline (follow-up). Струва си изрично да се потвърди, че сервираният D1 не чете тези колони — иначе това е реален production риск, а не козметичен. -
Съзнателно отклонение от спецификацията в C2 (партида 7): избрана е линейната вариант (1.2x → 0.80) вместо piecewise band-а от §4.C (1.2x → 0.60), тъй като спецификацията не разрешава конфликта. Разликата е материална за скора — желателно е продуктовата страна да потвърди избора.
-
Спад в coverage baseline за
apps/web(партида 4): маркиран за внимание — не може да се прецени дали е оправдан от гледна точка на тази партида. -
Прозорци на недостъпност при атомарния swap / DROP на
contract_featuresиflow_pairs(партида 7): при не-lock грешка served D1 може да остане без таблицата до следващото успешно изпълнение. Дизайнът го признава (loader-ът толерира липсваща таблица); приемливо, но да се държи наум за прод. -
Retry на цял SQL файл при lock и
tmp_diag/scratch артефакти в prod (партида 7): безопасни само защото файловете са строго идемпотентни — заслужават изрично потвърждение. -
Гранични случаи в скора:
score_eникога не е NULL →score_overallможе да се формира почти изцяло от стълб E за зрели договори; потвърдете, че това е желаното поведение (партида 7). Приgrain=fundingстойносттаselне се валидира явно, но е bound и с точно два валидни ключа — безопасно, препоръчва се явно ограничаване за яснота (партида 6).
Дребни бележки
- Проверете, че
docs/security-advisories.md(линкнат отdocs/README.md) действително е добавен в PR-а, за да не остане счупен линк (партида 4). - Потвърдете, че
osv-scanner.tomlпотиска точно съответния GHSA ID (CSRF advisory, CVSS 7.1), а не блокира широко (партида 5). - Дребна непоследователност в nullability на
QualityRankRow.avgOverall(партида 5). - Няколко незадължителни коментара за подобрение в
validate-health.mjs(партида 8).
Заключение
Няма блокиращи дефекти. Кодът е чист, добре тестван и добре документиран. Преди мърдж е препоръчително да се потвърдят точки 1 и 2 по-горе (production поведение на миграция 0012 и продуктовият избор в C2), тъй като те имат реален ефект съответно върху продукцията и върху стойността на скора.
| href: string | null; // entity page for authority/supplier grains | ||
| name: string; | ||
| sub: string | null; // type label / NUTS code / grain caption | ||
| avgOverall: number; // [0,1] |
There was a problem hiding this comment.
avgOverall: number тук е non-nullable, докато рангиращите rollup таблици (*_quality_totals.avg_overall) са REAL и могат да са NULL за grain без оценени договори. Ако рангиращият query филтрира до редове с не-NULL avg_overall (тестовете подсказват това), уточнете го с коментар; ако не — типът лъже и потребителят ще получи null там, където очаква число. За разлика от QualityOverview.avgOverall (number | null), тук няма nullable маркер.
| if (result.stderr) process.stderr.write(result.stderr); | ||
| if (result.status === 0) return; | ||
| const combined = `${result.stdout || ''}${result.stderr || ''}`; | ||
| if (attempt < 5 && LOCK_ERROR_PATTERN.test(combined)) { |
There was a problem hiding this comment.
Retry-ът се задейства при засичане на lock навсякъде във файла, докато коментарът по-горе описва само сценария с 'първия statement'. При lock в средата на файл — след като по-ранни statements вече са commit-нати в локалния D1/SQLite (auto-commit per statement) — целият файл се изпълнява повторно от нулата. Това е коректно само защото всеки файл, подаван към execSql, е строго идемпотентен (DROP/DELETE/INSERT, INSERT OR IGNORE, монотонни MAX(...)). Моля потвърдете, че това важи за всички call-site-ове (напр. и за refresh-slice.sql batch-овете), и обмислете да го запишете изрично тук — един неидемпотентен UPDATE в бъдеще би довел до тихо двойно прилагане при retry.
| export async function checkContractFeaturesIntegrity(runner) { | ||
| const name = 'contract-features-integrity'; | ||
| if ( | ||
| !(await tableExists(runner, 'contract_features')) || |
There was a problem hiding this comment.
Gate-ът зависи от наличието на tmp_diag, което е scratch таблица, оставяна нарочно в served/prod D1 (никога не се DROP-ва в derive-contract-features.sql) и се използва като 'proof-of-run'. Тоест временна диагностична таблица се разпространява в прод. Работи, но свързва интегритетната проверка с артефакт, който отвън изглежда като забравен temp — рисково при бъдещо почистване от типа 'drop stale temp tables'. Предложение: преименувайте на явно име (напр. contract_features_diag) или документирайте в самата DDL, че е нарочно постоянна.
|
|
||
| // 2) the 3 value_suspect contracts appear in no numerator (spot-check their authorities' | ||
| // scored_contracts < total_contracts) | ||
| check('value_suspect contracts excluded from every numerator', () => { |
There was a problem hiding this comment.
Дребна забележка (незадължителна): името на проверката е „excluded from every numerator“, но реалната агрегатна проверка на празнината се прави само върху authority_quality_totals — останалите пет измерения (bidder/sector/region/year/funding) не се проверяват за същия leak. Водещият коментар вече уточнява, че това е spot-check, така че е коректно, но си струва да изравните името с обхвата (напр. „...excluded from the authority numerator“), за да не подведе бъдещ читател, че всички числители са покрити.
| const d1Dir = resolve(root, 'apps/web/.wrangler/state/v3/d1/miniflare-D1DatabaseObject'); | ||
| const dbFile = readdirSync(d1Dir).find((f) => f.endsWith('.sqlite')); | ||
| if (!dbFile) throw new Error(`no .sqlite file found in ${d1Dir}`); | ||
| const db = new DatabaseSync(resolve(d1Dir, dbFile), { readOnly: true }); |
There was a problem hiding this comment.
Дребно: db не се затваря изрично. Тук е приемливо, защото main() винаги завършва с process.exit(...), но ако някога изнесете тази логика в reusable модул (без process.exit), добавете db.close() в finally, за да не остане отворен file handle.
withParams silently drops unknown query params, so a bookmarked /trends?g=year link lost its granularity without warning once `step` replaced `g`. Fall back to the retired g value when step is absent (step still wins when both are present) and canonicalise generated links to drop g and carry the resolved step forward.
The old /no such table/i regex matched any missing-table message regardless of which table. getQuality only queries contract_features today, but a future JOIN could hit an unrelated missing table and have it silently swallowed as "not derived yet" instead of surfacing. Anchor the match to the table name, same convention as related-persons.ts's MISSING_TABLE.
score100() rounds via Math.round(s * 100) while band() classified against the raw 0.7/0.5 thresholds, so a contract could show "70" (the documented good/mid boundary) while still coloring as mid — e.g. raw 0.695. band() now classifies against the same rounded value score100() displays so the label and color can never disagree. Also adds unit/render coverage for quality.tsx, trends.tsx and the new ComboTrendChart/MetricInfo/trendAxis modules, raising apps/web back above its pre-PR coverage baseline (91.6% lines, 79.8% branches, up from the regressed 78.85%/63.66%).
health-index columns are added by migration 0012 (see the comment in refresh-slice.test.ts), not 0003. Aligns the two comments so a future reader isn't misled about which migration layer owns them.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: индекс на качеството на договорите (ETL оценка 0..1 + страница)
Какво прави PR-ът
Добавя цялостен индекс за качество на договорите — от ETL деривацията до потребителския интерфейс:
- ETL / данни: нови деривационни скриптове (
derive-health.sql,derive-contract-features.sql) с NULL-разпространение, guard-ове срещу деление на нула, атомарен staging-swap наcontract_features, идемпотентност и повишаване на интегритетните инварианти до твърд gate. Нови миграции (0012_contract_health.sql) и операторски инструмент за валидация (scripts/validate-health.mjs, само за четене). - DB слой: нови заявки
quality.tsи промени вtrend.ts(тримесечно нагъване), с валидация на границата на заявката и bound параметри навсякъде. - UI: нови страници
/qualityи/trends, споделенyearAxisTicks, компонентиComboTrendChart/MetricInfo, стриктна валидация на query параметрите и включване на индекса в analytics/methodology. - Тестове/докове: разширено покритие на cache-ключове, преминаване на ETL тестовете към пълната миграционна верига, обновен coverage baseline и документация.
Обща оценка
Кодът е чист, добре структуриран, документиран и добре тестван. Не е открит зловреден код, инжекционни вектори, изтичане на тайни или prompt-injection в нито една партида. Валидацията на входа е последователна (allow-list за посока/подредба/grain, целочислени диапазони 0–100 за ранкинг, регекс за година/CPV), потребителският вход се параметризира в SQL и се екранира в React. Политиката „NULL = недостатъчно данни, никога 0“ (§1.3) е спазена навсякъде, а деленията са защитени срещу деление на нула.
Блокиращи концерни
- Деплой на миграция
0012(партида 5): деветте ALTER-а от0012_contract_health.sqlне достигат production — няма стъпка вdeploy.yml, а0000_init.sqlвече е приложен out-of-band и няма да се преиграе. Тъй като ETL derive-скриптовете четат/пишат тези колони (reason,circumstances,estimated_value_eur,corrections_count,exemption_legal_basis…), изпълнението на ETL в production ще се счупи, докато follow-up-ът за деплой не е готов. Корекцията е извън обхвата на този PR, но това е блокер преди derive-скриптовете да тръгнат в production — моля, свържете follow-up issue-то с този PR, за да не се изгуби.
Забележки, заслужаващи внимание (неблокиращи)
- Ръчен ETL път (партида 7):
--derive=healthвimport.mjsизпълнява деривацията, но пропуска integrity gate-а — нарушение на инвариант при ръчна пре-деривация върху prod няма да провали процеса. - Slice/incremental път (партида 7):
runHealthDerive()разчита на пресниestimated_value_eur, ноprecompute.sqlне се пуска там — да се потвърди, че refresh-slice преизчисляваestimated_value_eur, иначе C3 и перцентилите остаряват за новодобавените договори. - Регресия в branch coverage (партида 4): спад в branch coverage в baseline-а — виж инлайн коментара.
- Семантика на Pillar C (партида 7): при
annex_count=0и договор < 90 дни, C1 се занулява от maturity gate-а, но C2 връща перфектна 1.0, което частично връща потиснатия сигнал за „незрял договор“. tmp_diag(партида 7): scratch-таблица умишлено остава постоянно в продукционната D1 като маркер.
Дребни / козметични
- Подвеждащ коментар в
queries/quality.test.ts(beforeAll): пише „0003’s health-index columns“, а колоните са в 0012. - Съобщението „(показани първите 24)“ е формално неточно при точно 24 договора.
relLabel: „×2,4 типичното“ би било по-ясно като „×2,4 над типичното“.- Коментарът в
qualityContractsза подредбата наvalue_suspectредовете е точен само при сортиране по резултат, не приsort === 'value'. - Дребна формулировка на празното състояние (партида 1).
- Да се потвърди (в съответната партида), че обхватът на потискането на GHSA-qwww-vcr4-c8h2 покрива точно този advisory, а не по-широк.
Заключение
Висококачествена, добре тествана промяна без проблеми със сигурността. Единственият истински блокер е деплойът на миграция 0012 спрямо production — той трябва да е готов, преди ETL derive-скриптовете да се пуснат в production.
| </div> | ||
| </dl> | ||
| ) : ( | ||
| <p className="muted">Индексът се изчислява при следващото обновяване.</p> |
There was a problem hiding this comment.
Дребна забележка (не блокираща): това съобщение „Индексът се изчислява при следващото обновяване." се показва и когато таблиците вече са деривирани, но scoredContracts === 0 (или avgOverall == null) — тоест изчислението е минало, но няма нито един оценен договор. В този случай текстът е леко подвеждащ. Ако разграничението има значение за UX, обмислете отделно съобщение за „изчислено, но все още няма достатъчно оценени договори".
| "lines": 91, | ||
| "branches": 82.4 | ||
| "lines": 91.6, | ||
| "branches": 79.8 |
There was a problem hiding this comment.
Покритието на клоновете (branches) за apps/web пада от 82.4 на 79.8 — регресия от ~2.6 пункта, докато PR-ът добавя значителна нова функционалност (страници /trends и /quality). Понижаването на baseline-а рискува да прикрие непокрити клонове в новия код и по същество отслабва gate-а. Моля потвърдете, че този спад е очакван, и по възможност добавете тестове за новите разклонения вместо да сваляте прага; ако спадът е неизбежен, добавете кратка обосновка защо.
| -- either way — but any FUTURE 0011 migration that these columns/tables depend on would need | ||
| -- re-numbering above 0012 instead. | ||
|
|
||
| ALTER TABLE contracts ADD COLUMN exemption_legal_basis TEXT; |
There was a problem hiding this comment.
Тези девет ALTER‑а добавят колони, които ETL derive‑скриптовете (derive-contract-features.sql, refresh-slice.sql, promote-amendments.sql) четат и пишат — reason/circumstances на amendments, estimated_value_eur/corrections_count на tenders, exemption_legal_basis/outside_zop/dps_contract на contracts, first_date/last_date на flow_pairs. Коментарът по‑горе честно признава, че тази миграция НЕ достига production (няма стъпка d1 execute --file в deploy.yml, а 0000_init.sql вече е приложен out‑of‑band и няма да се преиграе). Следствие: щом derive‑скриптовете тръгнат в production преди follow‑up‑а, те ще се провалят с „no such column“ или ще четат липсващи данни — реален риск за data integrity/деплоя.
За fresh‑DB/CI/work‑backfill веригата е коректна (адитивни nullable колони, детерминистичен ред спрямо 0011), затова самият файл е наред. Но преди мърджа, който активира ETL в production, follow‑up стъпката в deploy.yml трябва да е налична. Моля, свържете tracking issue-то директно с този PR (не само коментар в кода), за да не се изгуби гейтът.
| console.log(`==> Sigma import (${remote ? 'REMOTE' : 'local'}, derive=health only)`); | ||
| run('wrangler', ['d1', 'migrations', 'apply', d1Name, loc, ...d1PersistArgs], apiDir); | ||
| runHealthDerive(); | ||
| process.exit(0); |
There was a problem hiding this comment.
Пътят --derive=health пуска миграциите и runHealthDerive(), след което излиза без да извика integrity gate. PR‑ът иначе повишава checkContractFeaturesIntegrity до твърд gate в full/slice деривациите и в ship-domain. Тук ръчна пре-деривация върху prod може да остави нарушен инвариант (unmapped procedure_type, изтекъл value_suspect score, нарушен A1 floor, ненулев score_b за пряко договаряне) без процесът да се провали. Предложение: извикайте await assertIntegrity(...) с checks: [checkContractFeaturesIntegrity] преди process.exit(0), за да не е ръчният път по‑слаб от автоматичните.
| // Full Phase 4/5 recompute, not a scoped refresh of just the touched authority/bidder/contract | ||
| // ids — correct-over-incremental for now; a scoped refresh (design spec §8) is a documented | ||
| // future optimization once the full recompute cost is measured on prod D1. | ||
| runHealthDerive(); |
There was a problem hiding this comment.
runHealthDerive() на slice пътя разчита на пресни tenders.estimated_value_eur, flow_pairs и authority_totals. Но estimated_value_eur се изчислява в precompute.sql (нов код в този PR), който не се пуска на slice/incremental пътя. Ако refresh-slice не преизчислява estimated_value_eur, за новодобавените tender‑и то остава NULL (стойност по подразбиране от миграцията) → C3 (estimate_dev_ratio) и перцентилите в derive-health.sql остаряват за скорошните договори. Моля потвърдете, че refresh-slice попълва estimated_value_eur, или добавете стъпката тук.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах #188 при HEAD 56a9697f — трасирано локално срещу actual code. Два hard blocker-а.
1. CRITICAL (libel / индексиране на физически лица): /quality показва немаскирани имена на ЕТ / физически лица на индексируема risk-страница.
/quality е нова в този PR (quality.tsx + quality.ts ги няма на main). meta() (quality.tsx:31) е обикновен seoMeta — без условен noindex; страницата е publicCache(1800), т.е. индексируема. Имената се рендерират сурово: {c.bidderDisplayName} (quality.tsx:1010, :1048) идва от quality.ts:337 entityName(cleanName(b.name), bidder_kind). entityName (format.ts:206) само свива consortium; company минава непроменено — нула natural-person маскиране. Query-то дори не select-ва natural-person сигнал (само b.kind company|consortium; няма legal_form), затова flow-ът няма и как да маскира.
Резултат: ЕТ (физическо лице), спечелил договор със слаб скор, се появява поименно в „най-слаби" картите / ранкинга на индексируема страница с risk-рамка — точно експозицията, която системата иначе пази: contract.tsx:96 noindex-ва natural-person договор-страница, а list-повърхностите (companies.tsx, #183) маскират името на „Частно лице". /quality не прави нито едното.
Fix: маскирай/изключи natural-person bidder-и в картите + ранкинга (select-ни сигнала за физическо лице и приложи същия предикат като #183). Не noindex на цялата страница — това е list.
(Честно: коренът е налице по-рано в самия branch, но /quality е нова с този PR — не бива да излиза така.)
2. MAJOR (свален coverage праг): apps/web branches 82.4 → 79.8.
coverage-baseline.json: на main apps/web = lines 91 / branches 82.4; на HEAD = lines 91.6 / branches 79.8. Lines се качват, branches пада 2.6 пункта — нов branch-тежък web код (quality/conflict routes) недотестван по branch-ове и подът е свален, за да мине. Нарушава правилото „прагът само се вдига, никога надолу". Fix: върни 82.4 (или по-високо) и добави branch-тестове, не сваляй пода.
3. Minor (не е prod-reachable): fresh-DB /quality 500. Стеснението на isMissingDerivedTableError само до contract_features (etl.ts) значи, че на неderive-нат DB отхвърляне no such table: authority_quality_totals от qualityRanking (quality.ts:163-204) не се swallow-ва → loader 500 вместо empty state. Само на чисто нов DB (шестте rollup-а са CREATE TABLE IF NOT EXISTS, никога drop-нати → на prod винаги съществуват). Минор.
Чисто (потвърдено): band/rounding fix-ът е коректен — band() класифицира на Math.round(s*100), еднакво със score100(), така че цвят и число вече не се разминават на граница; scoring exclusions (value_suspect→NULL, coverage<0.40→NULL) и renormalizing над present pillars — интактни.
Вердикт: не merge-вай преди 1 и 2. Triage роля → COMMENT.
| <Link to={`/authorities/${c.authoritySlug}`}>{c.authorityName}</Link> | ||
| </p> | ||
| <p className="q-card-seller"> | ||
| → <Link to={`/companies/${c.bidderSlug}`}>{c.bidderDisplayName}</Link> |
There was a problem hiding this comment.
CRITICAL: bidderDisplayName идва от entityName(cleanName(b.name), kind) — без natural-person маскиране (виж format.ts:206). ЕТ / физическо лице се рендерира поименно тук, а meta() (line 31) няма noindex и страницата е publicCache(1800) → индексируемо име под risk-рамка. Маскирай natural-person bidder-и (предиката от #183). Виж summary т.1.
| // Neutrality stance (spec §1.3): a low score is a weak-process SIGNAL, never proof of wrongdoing; | ||
| // contracts without a score are „недостатъчно данни", never zero. | ||
|
|
||
| export function meta({ matches }: Route.MetaArgs) { |
There was a problem hiding this comment.
meta() е обикновен seoMeta без условен noindex. За list не noindex-ваме цялата страница (за разлика от contract.tsx:96) — решението е маскиране на имената на физически лица в данните, не тук. Виж summary т.1.
… номериране на миграциите, coverage baseline, споделени резолюции
…index Renumber 0012_contract_health -> 0024_contract_health (main ships 0000-0022; 0023 reserved for the dashboard spine). Package tests keep the full-chain migration apply; ETL scripts keep main's behaviour plus the awaited contract-features integrity gate; coverage-baseline.json taken from main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd query branches Closes the apps/web and packages/db coverage-ratchet gaps on midt-bg#188 without touching coverage-baseline.json. Loader tests pin sel/contract/band handling and that only a missing contract_features table is swallowed (a typo'd table rethrows). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s on midt-bg#188 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eploy, drop stale advisory suppression note Closes the last ydimitrof threads on midt-bg#188 and adds the 2026-09 merge-readiness rollup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…docs from midt-bg#188 Remove 19 .claude/skills/** files and 4 internal review docs that are not part of the contract-health-index feature. Un-index them from docs/README.md and drop the two comment-only pointers (0024 migration header, deploy.yml) to the removed review-round doc.
Delete the four docs/README.md index lines and the two comment-only references to the removed review-round doc (deploy.yml, 0024 migration header). No SQL or workflow step changes.
Local working notes were never meant to be published; the file names a reviewer and internal fork/push-access strategy. Removed from the index only; the file is now ignored.
This branch has not been deployed
4 participants
What changed
Contract quality index (ETL score 0..1 +
/qualitypage). Latest round addresses the reviewer's 2026-07-13 review (9 threads):scripts/import.mjs:runFullDerive/runSliceDerivenow gate oncheckContractFeaturesIntegrityin addition to the standardCHECKS, so the local full/slice/health ETL path enforces the samecontract_featuresinvariants (row parity, A1 floor, score_b=0 on direct award, no value_suspect leakage) asship-domain.mjs.scripts/precompute.sql: dropped theCOALESCE(tenders.published_at, date('now'))FX-rate fallback — aNULLpublished_atnow resolvesestimated_value_eurtoNULLinstead of silently borrowing a same-day rate that could mis-date historical foreign-currency estimates.scripts/validate-health.mjs: fixed a false-FAIL where the synthetic'NA'bucket (undated/out-of-range contracts) tripped the >60%-NULL threshold check meant only for 2020–2026 strata; and fixed the Spearman-lite decile-correlation check printingNaNon zero-variance samples (now logs and skips).apps/web/app/lib/trendAxis.ts: removed the in-codeTODO(#170)note — the third-copy consolidation is tracked solely via issue feat(web): договори — обзор (лещи време/CPV/кръстосано) #170 per repo convention.filters.tsranking params (rdir/rfrom/rto) are fully parameterized via.bind(...)inpackages/db/src/queries/quality.ts(no string interpolation);analytics.tsx'sgetQualitySummary(...).catch(...)correctly no-ops on the expected pre-derive "no such table" case; migration0003_contract_health.sql's out-of-numeric-order columns are purely additive and independent of the still-pending0002_contracts_overrun_index, and all target environments apply viawrangler d1 migrations apply(tracked by name, not raw re-execution); theship-domain.mjsnon-atomic DROP/CREATE availability window oncontract_featuresis a known, already-documented follow-up (staging-table swap), out of scope here.How it was tested
pnpm --filter web testandpnpm --filter db test— both pass, including regression coverage added for the axis/relLabel edge cases and the schema-drift guard extension from the prior round.node:sqlitefixture trace re-confirmed thevalidate-health.mjsNA-bucket and zero-variance fixes against a synthetic dataset.Quality checks
normalize-raw.sqlandrefresh-slice.sql.Вид промяна
fix— поправка на бъгfeat— нова функционалностdocs— документацияrefactor/perf/style— без промяна в поведениетоtest/ci/build/chore— поддръжкаЧеклист
Co-Authored-By:, вече публикувани, не се пренаписват) — виж „Бележка при merge“ по-долуmidt-bg/sigma:mainpnpm typecheckминава (CI: стъпка Typecheck е зелена)pnpm testминава (CI: стъпка Test е зелена)pnpm lintе чисто (CI: стъпка Lint е зелена).env*или.dev.vars(gitleaks + semgrep са зелени)docs/е обновенаБележка при merge
7 комита в този branch носят
Co-Authored-By:trailer към агент (Claude). AGENTS.md го забранява наmain, а squash-merge запазва trailer-ите от комитите. Историята не се пренаписва (без rebase/force-push), затова при merge моля заменете автоматично генерирания commit message със заглавието и тялото по-долу. Няма trailer-и към хора в този branch (всички комити са от автора на PR-а), така че не се губи credit.Заглавие (subject):
Тяло (body):