feat(web): „Подобни договори" - ценови ориентир по CPV кохорта на страницата на договора - #210
Conversation
|
Проверих PR #210 из основи локално — целия diff, SQL пътищата, заявките в РезюмеЧиста, добре обхваната функционалност: предизчислен rollup Сигурност / OWASP
Цялост на данните
Съответствие с описаниетоИмплементацията отговаря на PR описанието изцяло: rollup в двата пътя, четене на един ред вместо сканиране на дивизия (D1 таксува прочетени редове), праг 12, широки стъпала без фалшива точност, методология. Няма пряк issue — допълва посоката на #127/#188. Дребни бележки (незадължителни, не блокират)
ВердиктApprove на същество — сигурност и цялост на данните чисти, SQL параметризиран, покритието е силно; двете бележки са козметични и не блокират мърджа. |
Ревю на PR #210 — „Подобни договори“ (ценови ориентир по CPV кохорта)Благодаря за спретнатата и добре мотивирана промяна. Прегледах я обстойно с фокус върху сигурност, интегритет на данните и съответствие с OWASP; също прекарах дифа през локална проверка за SQL/XSS и злонамерени примеси. По-долу са наблюденията. Сигурност (OWASP)
Интегритет на данните
Тестове / качествоПокритие на нулевите пътища (suspect / NULL / нула / липсващ CPV / малка кохорта), гранична инклузивност на бандовете, DTO мапинг и парити между двата SQL пътя. Миграция Дребни бележки (не блокери)
Няма намерени уязвимости, изтичане на ресурси или следи от злонамерен код. Промяната е готова за merge. Вердикт: APPROVE (на същество) — сигурност и интегритет на данните чисти, OWASP-съвместимо; дребните бележки са по желание. |
|
Ре-проверих ( Неточност (medium): §4c на Low: „сред N договора" брои чистата кохорта, а линкът „Виж договорите в сектора →" води към Cross-PR: |
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Прегледах — PR-ът е внимателно направен: коректна nearest-rank статистика (буферът +0.9999999 безопасно поема IEEE-754 грешката), филтриране само по каноничен EUR (amount_eur IS NOT NULL AND > 0 AND value_flag='ok', което точно съвпада с rollup-а), параметризиран SQL, стабилен ROW_NUMBER() ... ORDER BY amount_eur, c.id, и неутрална формулировка („контекст на мащаба, не оценка за нередност"). Три бележки:
-
(потвърдено) Колапс на горните ленти при малка кохорта. Стълбицата стига до топ 1%/5%, но подът е
MIN_COHORT=12. При N≈12 nearest-rank даваp95 = p99 = max, тоест най-скъпият от 12 договора получава етикет „в най-горния 1%" — фалшива точност, точно обратното на целта на PR-а („никога фалшива точност"). Показвай фините ленти (топ 1%/5%) само когато N е достатъчно голямо, за да ги различи (anomaly-report.mjsсъщо отбелязва, че кохорти 12–20 са крехки в горния край). -
(вероятно) Кохортата е само по 2-цифрен CPV раздел. Раздел 45 „Строителство" слага ремонт за 5 хил. до автомагистрала за 50 млн. в една група — процентилната мрежа смесва структурно различна работа. Съзнателен компромис (цена на precompute/D1 rows-read) и UI-ят е внимателен, но „подобни договори" обещава по-тясна група, отколкото реално има. Пълен CPV код / CPV клас би бил по-тесен, но взривява кардиналността на rollup-а.
-
(вероятно) Излишно четене на договора на SSR hot path —
getContractCohortпречита реда на договора (amount_eur,value_flag, CPV), койтоgetContractвече е взел. ВPromise.allса, тоест латентността е скрита, но D1 таксува прочетени редове (192k страници + sitemap). Подай вече заредения договор на cohort helper-а вместо второ четене.
Координация: #210 и #212 добавят по един 0002_* migration — който се мерджне втори, трябва да се преномерира на 0003, иначе един от двата тихо няма да се приложи в прод D1.
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Допълнение към прегледа (дълбок пас). Няколко находки, по-сериозни от първоначалната ми бележка за „фалшива точност при N≈12" — те я включват и я заменят.
Критични — грешни етикети върху обичайни данни
-
Колапс при равни стойности → всичко е „в най-горния 1%". Каскадата в
cohortBandе с>=, затова при равни стойности на/над p99 order statistic всеки такъв договор получава „в най-горния 1% по стойност". Раздел от 200 стандартизирани договора на еднаква цена (100 000 €) етикетира и 200-те като „топ 1%". Стреля при всяко N — а еднакви цени при рамкови/типови поръчки са често срещани. По-драматично от познатия N≈12 колапс. -
Най-скъпият в кохортата винаги е „топ 1%" (self-inclusion) + фалшива точност при пода. Договорът е в собствената си кохорта (не leave-one-out), тоест максимумът е по дефиниция „топ 1%"; при N=12 това е ~топ 8%, показано като „топ 1%". Собственият
anomaly-report.mjsдокументира, че self-inclusion е ненадежден точно при 12–20 реда. Уговорката живее само в коментарите в кода — в UI-я я няма (обратно на твърдението в PR описанието), затова читателят не знае, че договорът се сравнява със себе си. -
Грешен етикет за медианата. Nearest-rank медианата е долно-средната стойност; с
>=договор в долната половина при четно N получава „над медианата", а показаната „медиана" е изместена надолу с една стъпка. Тестът го фиксира като очаквано поведение.
Висок приоритет
- Най-тежкото изчисление е в споделения
globalsrefresh batch. Пълният ~150k-редов window sort е добавен към@refresh-batch globals(който вече правиdata_freshness/home_totals/sector_totals/facet_counts), вместо в собствен@refresh-batch cohort-stats, както другите тежки rollup-и. Рискува да пробие CPU бюджета на едната D1 заявка и да провали целия globals step.
Средно
- Две несъвместими дефиниции на процентил за едни и същи CPV раздели: тук nearest-rank + self-inclusion; в
anomaly-report.mjsлинейна интерполация + leave-one-out. Една и съща „медиана"/„p95" дава различни числа на два продуктови екрана. PR описанието признава само разликата leave-one-out, не и nearest-rank vs интерполация.
Чисто (проверено, за фокус)
Параметризиран SQL; division идва от DB реда, не от заявката; предикатът за членство в кохортата е byte-еднакъв между precompute и read-side (показан договор винаги е реален член); ceil емулацията е коректна; refresh parity (INSERT) е фиксиран с тест; Promise.all — без латентностна регресия; stats четенето е O(1) по PK върху ≤46-редова таблица.
Вердикт: Request Changes
#1–#3 карат водещата функция да показва уверено грешни етикети върху обичайни данни — най-лошият режим за платформа за прозрачност, и нарушава собственото обещание „никога фалшива точност". Всички са евтини за поправка: показвай фините ленти (топ 1%/5%/медиана) само когато съответните персентили реално се различават и кохортата е достатъчно голяма; ползвай строго > / leave-one-out където е редно; премести recompute-а в собствен @refresh-batch. Основата (parity, сигурност, дизайн на precompute) е стабилна — това е поправка на логиката за етикети и на ETL разположението, не пренаписване.
(Заменя по-меката ми бележка „фалшива точност при N≈12" — тя е частен случай на #1/#2.)
…iew) Addresses the Request-Changes review on the „Подобни договори" benchmark: Label correctness (cohortBand, was `>=` cascade over a shared, self-inclusive grid): - midt-bg#1 ties: strict `>` for the top bands + require each anchor to be strictly above the next coarser one, so a tie-collapsed cohort (p99=p95=…=median) or a value merely equal to an anchor never reads as „top 1%". Symmetric guard on „bottom 25%". - midt-bg#2 tiny cohorts: per-band minimum cohort sizes (top1≥100, top5≥40, top10≥20, top25≥12), so the single most-expensive contract in a 12-row cohort is not „top 1%". The self-inclusion caveat is now stated in the UI + methodology, not only in code. - midt-bg#3 median: a value at the nearest-rank median maps to a new `at-median` band instead of „above median"; strict comparisons on both sides. - midt-bg#5: UI + methodology note that this uses a different method than the anomaly report, so the two figures may differ. Redundant read (review comment 3): the cohort is now computed inside getContract from the row it already read (stats load in the existing Promise.all), returned as `ContractDetail.cohort` — no second contract scan, no added latency. ETL (midt-bg#4): the ~150k-row window recompute moved out of the shared `globals` batch into its own `@refresh-batch cohort-stats` step, so its CPU cost can't fail the whole globals step. Coordination: renumbered this migration 0002 → 0003 to de-conflict with the 0002 in the list-sort-indexes branch. Regression tests cover each labelling case (ties, tiny cohort, at-median, non-distinct anchors) and the pure contractCohort gate.
|
Благодаря за задълбочения преглед - адресирах всичко в Критични — етикети (
Висок — #4 ETL — тежкият ~150k-редов recompute е изведен от споделения Среден — #5 — UI-ят и методологията вече казват изрично, че методът се различава от отчета за аномалии, затова числата може леко да се разминават. (Пълно уеднаквяване не е възможно с предизчислена per-division мрежа - leave-one-out е per-row; затова документирам разликата.) Първо ревю, т.3 — излишно четене — cohort-ът вече се смята вътре в Координация — преномерирах миграцията Нови регресионни тестове покриват всеки случай (равни стойности, малка кохорта, at-median, неразличими прагове) и чистата |
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
✅ Всички находки от двата ми предишни прегледа са адресирани коректно и с тестове — сваля предишния Request Changes.
- Колапс при равни стойности → фините ленти вече изискват различим anchor (
p99Eur > p95Eurи т.н.) + строго>, тоест еднаква/tie-heavy кохорта пада доat-medianвместо да етикетира всичко „топ 1%". - Най-скъпият винаги „топ 1%" / N≈12 →
BAND_MIN_COHORT { top1:100, top5:40, top10:20, top25:12 }; 12-редова кохорта стига най-много доtop25. Self-inclusion вече е обявен в UI + методологията („включващи и самия договор"). - Медиана → нова лента
at-median; точната медиана вече не се етикетира „над медианата". - ETL разположение → recompute-ът е в собствен
-- @refresh-batch cohort-stats(изолиран CPU бюджет), а migration-ът е преномериран на0003— край на колизията с #212. - Излишно четене →
contractCohortвече е чиста функция върху вече заредения ред; остава само O(1) четене на stats (без второ четене на договора).
cohortBand е коректна и без пролуки (проверено), и всеки сценарий, който повдигнах, има реален тест. Сигурността (параметризиран SQL, division от DB реда) и parity на предиката за членство са запазени.
Незадължаващо follow-up (не блокира merge): parity тестът фиксира само INSERT — CREATE TABLE cpv_division_stats е в три копия (migration / precompute / refresh-slice) и не е закотвен, тоест промяна в колона в едно от тях няма да провали тест. Струва си малък отделен issue.
Approve. 🚀
|
Проверих отново (
Cross-PR преди merge: Nit: Едно за потвърждение: Одобрявам по същество; renumber + потвърждение за cron budget преди merge. |
|
Допълнение след по-внимателна проверка на Кохортният INSERT е Medium (accuracy, преди merge): или добави Percentile математиката и стойностната база ( |
… label The cpv_division_stats cohort has no signed_at bound — it spans every priced clean-value contract in the division (matching sector_totals and the /contracts list, which are also not date-bounded), incl. pre-2020-signed and undated rows. The UI hint and methodology claimed „(2020 г. - днес)" / „от 2020 г. насам", promising a window the query never enforces (review lyubomir-bozhinov). Fix the copy rather than add a date bound: bounding only the cohort would break the cohort↔rollup membership invariant (a shown contract must be a real member of the cohort it is compared against) and diverge from the site-wide value basis. The methodology now states the comparison is over the full corpus, same scope as the sector totals.
|
Прав си - оправено в Избрах да поправя надписа, не да добавя дата-граница, защото кохортният
Затова:
Секцията „Покрит период" в методологията си остава мястото, където обхватът на корпуса (2020+) е описан коректно. |
|
Потвърдено на Съгласен с избора ти — да пазиш кохортния Остава само cross-PR renumber: |
De-conflict with the 0003 already claimed by midt-bg#188 (contract_health). 0002 is taken by the list-sort-indexes branch, so the next free number is 0004. Final merge order stays the maintainer's call; this just removes the known 0003 clash.
|
Преномерирах миграцията
Миграцията е адитивна и идемпотентна ( |
De-conflict the migration number: 0002 is claimed by the contracts_overrun_index family (midt-bg#169/midt-bg#170/midt-bg#171/midt-bg#172), 0003 by midt-bg#188 (contract_health), and 0004 by midt-bg#210 (cpv_division_stats). 0005 is the next free number. Additive/idempotent, so final merge order stays the maintainer's call; this just removes the known 0002 clash.
…ctor label The „Подобни договори" section opened with „ (U+201E) but closed with a straight " (U+0022). Match the Bulgarian pair „…“ (review ydimitrof). Cosmetic only.
|
Благодаря за одобрението! По двете дребни бележки: Sector линк (потвърждение):
Тоест връзката води до договорите точно от този CPV раздел, подредени по стойност. Не се налага промяна. Типография: поправено в
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: „Подобни договори" — ценови ориентир по CPV кохорта
Фаза 0 — Сигурност (задължителна проверка): ЧИСТО ✅
- Няма твърдо кодирани тайни (API ключове, пароли, токени).
- Няма нови или променени URL адреси (единственото споменаване на
storage.eop.bgе в съществуващ коментар, не е нова стойност). - Няма зловредни модели (backdoors, инжектиране на код, обфускация).
- Няма нови зависимости.
Обща оценка: 9.5/10 — препоръка: APPROVE
Това е добре обмислена, атомарна промяна с висока инженерна дисциплина. Логиката, документацията и тестовете са в отлично съответствие.
Силни страни:
- Коректност на кохортните ленти.
cohortBandизползва строго>/<, изисква различни персентилни котви и минимален размер на кохортата за всяка фина лента (BAND_MIN_COHORT). Това елегантно предотвратява фалшива точност при изравнени по цена или малки кохорти — точно проблемите от прегледа на nedda76. Покрито е с прицелни регресионни тестове (#1–#3). - Nearest-rank персентили в SQL. Емулацията на
ceilчрезCAST(n*q + 0.9999999 AS INTEGER)е коректна: маржът от 1e-7 надеждно поглъща float грешката (~1e-16) и не допуска off-by-one при целиk.ROW_NUMBERс tie-break поc.idгарантира точно един ред на ранг. - Производителност. Един O(1) PK прочит на предизчислен rollup вместо сканиране на цялата дивизия за всяка заявка. Гейтът е приложен и в
getContract(пропуска прочита изцяло за неподходяща стойност), а когато се чете — паралелно с останалите detail заявки чрезPromise.all. Тежкото пресмятане е изолирано в собствен@refresh-batchза D1 CPU бюджета. - Консистентност между двата пътя. Тестът
keeps the refresh-slice rebuild identical to the precompute oneпредпазваprecompute.sqlиrefresh-slice.sqlот тихо разминаване — WHERE условията им точно съвпадат с read-side гейта, така че показаният договор винаги е член на кохортата, срещу която се сравнява. - Честна комуникация. UI и методологията ясно посочват, че сравнението е self-inclusive, приблизително (широки стъпала), контекст за мащаба, а не оценка за нередност, и че методът се различава от отчета за аномалии.
- Документация.
methodology.tsx,etl-pipeline-state.mdиcompare-served-sqlite.mjsса актуализирани заедно с промяната — няма пропуски.
Спазване на CLAUDE.md: Няма частична имплементация, TODO-та, дублиран или мъртъв код. Наименованията са консистентни, разделението на отговорностите е чисто (чист cohort.ts, детайлите остават в details.ts). Няма изтичане на ресурси.
Тестове: Обхватът е много добър — happy path, всички ленти, крайни случаи (равно на котва, напълно изравнена кохорта, малка кохорта), DTO мапинг, null пътища, и end-to-end гейтът в details.test.ts, включващ проверка че suspect договор изобщо не заявява cpv_division_stats.
Единствената ми бележка е дребна (виж инлайн коментара) и не блокира сливането.
|
Благодаря за одобрението! Потвърждавам бележката с конкретни редове - линкът е валиден и следва съществуваща конвенция: Съществуваща препратка със същия формат (условието, което сам посочи като решаващо): authority.tsx:270 вече линква Веригата на валидация на
Тоест линкът „Виж договорите в сектора" отваря списъка, филтриран по същата дивизия, от която е сметната кохортата, подреден по стойност. Същият въпрос беше повдигнат и в по-ранното ревю - отговорът съвпада; не се налага промяна в кода. |
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Издържано. Cohort lookup-ът е O(1) по PRIMARY KEY (cpv_division_stats.division) и стартира само при hasCleanValue && cpv_code. contractCohort връща null при amountEur == null | <= 0 | valueFlag !== 'ok' | !division | pricedContracts < MIN_COHORT (cohort.ts:99); няма аритметично деление в пътя → няма div-by-zero/NULL капан. MIN_COHORT = 12 + band праговете + distinct-anchor проверките пазят от подвеждащ бенчмарк при малка кохорта, а derived-стойността е разкрита в UI текста. Няма забележки.
# Conflicts: # packages/api-contract/src/index.ts # packages/db/src/queries/details.test.ts # packages/db/src/queries/details.ts
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ре-ревю на връх c76a9fe, само по оста точност/интегритет на данните (a11y/CSS не са пипани този пас). Проследих аритметиката и SQL-а емпирично — чисто, няма блокери.
- Кохортата = CPV дивизия (първите 2 цифри) и в rollup-а (
substr(t.cpv_code,1,2)), и в per-contract gate-а — съвпадат. - Праг за малка извадка е налице и коректен.
MIN_COHORT=12подова всяко показване; фините ленти искат по-голяма кохорта (BAND_MIN_COHORTtop1=100/top5=40/top10=20/top25=12). Кохорта n=1/2 не показва нищо. Adversarial случаите (най-скъпият в кохорта от 12 →top25, неtop1; напълно tie-collapse-ната кохорта →at-median; стойност точно на anchor → по-груба лента) всички се разрешават към по-грубата лента — без фалшива точност. - Няма value_flag контаминация — двете страни ползват идентичен филтър. Rollup-ът (
precompute.sql:157) еWHERE amount_eur IS NOT NULL AND amount_eur > 0 AND value_flag = 'ok' AND cpv_code <> ''; eligibility gate-ът (cohort.ts:99) отхвърляamountEur<=0 || valueFlag!=='ok' || !division. Показан договор винаги е член на кохортата, спрямо която се сравнява; suspect/low/review/annex редове са извън и перцентилите, и допустимостта. - EUR-нормализация (EUR идентитет / BGN peg 1.95583 / bounded FX) консистентна през кохортата. Nearest-rank перцентилите (
k=ceil(q·n)) са коректни. UI показва груба лента + медиана, не фалшиво „top 4.7%".
Една дребна, заварена бележка (не е от този PR): tenders.cancelled/status не се филтрират в rollup-а — но това е конвенцията на целия сайт (същото в home/authority/flow rollups), тъй че #210 е консистентен, не разминат. Не прави показано число грешно спрямо обявената база.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR: „Подобни договори" — ценови ориентир по CPV кохорта
Прегледах целия diff с приоритет върху сигурност и цялост на данните, както е поискано. По-долу е обобщението по направления. (Забележка: по инструкция от средата текстът е само на български; мога да предоставя и английска версия при поискване.)
Фаза 0 — Сигурност (блокиращ скан): ЧИСТО ✅
- SQL инжекция: Няма. Единствената динамична заявка (
getCpvCohortStats) използва параметризиран bind:SELECT * FROM cpv_division_stats WHERE division = ?с.bind(division). SQL скриптовете (precompute.sql,refresh-slice.sql) са статични, без конкатенация на вход. - Твърдо кодирани тайни / ключове / токени: Няма.
- URL промени: Единственият нов линк е вътрешен и относителен (
/contracts?sector=...&sort=value-desc). Няма нови външни домейни. - Malicious patterns (backdoor, eval, обфускация, code injection): Няма.
execFileSync('sqlite3', [...])в теста подава аргументите като масив (без shell), пътищата идват отimport.meta.url, не от външен вход. - Нови зависимости: Няма. Използват се само
node:вградени модули и вече наличниятvitest. - XSS: React екранира по подразбиране; всички стойности минават през форматери (
money,count,plural); нямаdangerouslySetInnerHTML.
Цялост на данните и коректност
- Съответствие кохорта ↔ показан договор: Гейтът в
contractCohort(amount_eur > 0,value_flag === 'ok', налично CPV) съвпада точно сWHEREв rollup-а (amount_eur IS NOT NULL AND amount_eur > 0 AND value_flag = 'ok' AND COALESCE(cpv_code,'') <> ''), затова показаният договор винаги е член на сравняваната кохорта. Self-inclusive поведението е явно документирано в UI и в методологията. ✅ - Персентили (nearest-rank): Емулацията на
ceil(q*n)чрезCAST(n*q + 0.9999999 AS INTEGER)е коректна. Проверих граничните случаи — при цялоn*qдава точния ранг, а рискът от off-by-one изисква дробна част в (0, 1e-7), какъвто при множители 0.25/0.5/0.75/0.9/0.95/0.99 не възниква (float грешката е далеч под този праг). ✅ - Праговете за ленти са принципни:
top1 (n≥100),top5 (n≥40),top10 (n≥20),top25 (n≥12)реално отговарят на 1/5/10/25% от кохортата при nearest-rank — добра защита срещу фалшива точност. Строгото>и проверката за различими опорни персентили коректно предотвратяват „топ 1%" при tie-collapsed или дребна кохорта. Регресионните тестове (#1–#3 на nedda76) го покриват. - Ред на
Promise.allвdetails.ts: Деструктурирането[authTotals, compTotals, lotRows, cohortStats, amendmentRows]съвпада с новия ред на масива — тестътdetails.test.tsизрично пази това. ✅
Тестове
Много добро покритие: чист SQL тест за персентилите (вкл. изключване на suspect/NULL/zero/без-CPV редове и идемпотентност), тест че refresh-slice и precompute INSERT-ите не се разминават, unit тестове за всяка лента и всички null-гейтове, и end-to-end през getContract (вкл. че suspect стойност изобщо не удря cpv_division_stats). Тестовете са смислени, не тривиални.
Производителност
Пълният window-sort върху ~150k реда е изолиран в собствен @refresh-batch cohort-stats, за да не надхвърли CPU бюджета на едно D1 заявка — обосновано. Страницата на договора чете един ред по PK вместо да сканира цялата дивизия. Без регресии.
Документация
methodology.tsx, docs/etl-pipeline-state.md и inline коментарите са изчерпателни и точни; методологичната разлика спрямо отчета за аномалии е честно оповестена на потребителя.
Дребни забележки (не блокиращи)
SELECT *вgetCpvCohortStats— четенето разчита на именуван mapping, така че е безопасно, но явен списък с колони е малко по-устойчив на бъдещи промени в схемата.- Долните ленти са асиметрични спрямо горните (само
bottom25/below-median) — това е съзнателен дизайн, ок; струва си само да се отбележи. - Ако някога се появи CPV код с < 2 значещи символа, групирането по
substr(...,1,2)остава консистентно между SQL иdetails.ts, така че няма разминаване — само за сведение.
Съответствие с гейтовете (CLAUDE.md, OWASP)
Без частична имплементация, без TODO/мъртъв код, без дублиране (общият INSERT е тестово защитен срещу разминаване), без изтичане на ресурси (тестът чисти tmp с try/finally). OWASP: инжекция, XSS и конфигурационни рискове проверени — чисто.
ПРЕПОРЪЧИТЕЛНА ОЦЕНКА: ОДОБРЕНИЕ (Approve) — 9.5/10. Няма блокиращи проблеми по сигурност, цялост на данните или коректност; забележките са козметични.
Не публикувам коментари/финално решение по ваше указание — това е чернова за вашата проверка преди изпращане.
|
Прегледах #210 на head Проверих независимо носещия инвариант: cpv_division_stats се пълни с точно същия предикат като gate-а —
Дребно (яснота): коментарът „matches the rollup's WHERE exactly" — money-rollup-ите всъщност са по-широки ( Ред на миграциите: виж бележката ми на #212. Sign-off от мен. |
… + hardening) (#212) * perf(db): ordering indexes for the non-default list sorts The list pages keyset-paginate with ORDER BY <sortExpr> <dir>, <id> <dir> LIMIT N. Six user-selectable sorts had no matching index, so the planner fell back to a full table SCAN + temp-B-tree ORDER BY on every page (D1 bills rows scanned): /contracts date-desc, date-asc (idx_contracts_signed is on the bare column, not the COALESCE(signed_at, ...) expr the query uses) /companies count, authorities /authorities count, avg Add one index per missing sort, matching the exact ORDER BY expression plus the keyset id tiebreak, so SQLite walks the index and stops at LIMIT. Additive, idempotent; rollup tables are DELETE+INSERT-refreshed so the indexes survive ships. A sqlite3 EXPLAIN QUERY PLAN test proves each sort full-scans before and index-walks after. * fix(web): escape < in the JSON-LD data island (defense-in-depth) root.tsx embeds JSON-LD via dangerouslySetInnerHTML with a raw JSON.stringify. JSON.stringify does not escape '<', so a '</script>' in any string value would close the <script> element early (stored XSS) — the exact sink the project's own review standard (docs/review-security.md) requires be escaped. Today only the request origin reaches the graph (new URL() cannot make it carry '</script>'), so this is not currently exploitable; the jsonLdScript helper closes the sink pre-emptively for any DB/user-derived field added later. A unit test proves '<' is escaped, U+2028/U+2029 are escaped, and the output stays JSON-equivalent. * chore(db): renumber list-sort-indexes migration 0002 → 0005 De-conflict the migration number: 0002 is claimed by the contracts_overrun_index family (#169/#170/#171/#172), 0003 by #188 (contract_health), and 0004 by #210 (cpv_division_stats). 0005 is the next free number. Additive/idempotent, so final merge order stays the maintainer's call; this just removes the known 0002 clash. * test(db): apply all migrations + cover keyset pages; guard jsonLdScript(undefined) Address the review notes on the list-sort-indexes PR: 1. The sort-index test now applies EVERY migration on the branch (discovered from the migrations dir), not a hardcoded 0000/0001/000N subset. The "BEFORE" base is exactly the real served schema minus this PR's index, and the test survives any renumbering. (Confirmed: company_totals/authority_totals are created in 0000 and nothing between affects these sort plans.) 2. Each sort now asserts the plan on the keyset page too - the real paginated path `WHERE (expr <cmp> ? OR (expr = ? AND id <cmp> ?))`, not only the first page. Full-scans BEFORE and index-walks (no temp B-tree) AFTER, on both pages. 3. jsonLdScript now returns "null" when JSON.stringify yields undefined (undefined / function / symbol) instead of throwing on the following .replace - defense-in-depth for the documented "safe for any future field" helper. Covered by a test. * refactor(web): rename jsonLdScript → serializeJsonForScript + document sort-index sync Address the (non-blocking) review nits: - Rename jsonLdScript to serializeJsonForScript: the helper returns a serialized JSON string safe to embed in an inline <script>, not a <script> element (review ydimitrof). Updates root.tsx and the test. - Document the sentinel sync: the COALESCE defaults in queries/contracts.ts SORTS ('' / '9999-99') must stay byte-identical to the expression indexes, or SQLite silently drops the index and falls back to a full scan + temp-B-tree sort. Added reciprocal SYNC comments in the migration and the SORTS map, both noting that list-sort-indexes.test.ts's EXPLAIN assertions catch a drift. * docs(db): state the boundaries of the sort-index guarantee (review) Document the two known limits of the EXPLAIN-plan proof, per review: (1) the local sqlite3 CLI planner is not version-identical to Cloudflare D1's (a strong indication, not a bit-exact production proof; the binary itself is a pre-existing suite-wide dependency), and (2) the index-walk guarantee covers the UNFILTERED sort paths - with an active filter the planner may prefer the filter's index and temp-sort the much smaller filtered set, which is the correct trade. Comment-only. * chore: drop internal review-marker traces from code comments Remove the '(review ydimitrof)' attribution artifacts from json-ld.ts and list-sort-indexes.test.ts comments; the explanations stay. Comment-only. * refactor(web): share one JSON-for-script serializer between the JSON-LD island and .json route The .json contract endpoint had its own safeJson escaper, a second implementation of the same <script>/separator escaping as serializeJsonForScript - a DRY smell the comment itself admitted, and a drift risk (one could add a U+2028 escape the other lacks). Route it through the shared serializer instead. It escapes every `<` (vs the old `</`-only form) - JSON-equivalent, harmless for the JSON body, strictly safer. Also document, in the shared helper, why `>` and `&` are deliberately left unescaped (only `<` can start a token in a script raw-text context), with a test that locks it. * fix(web): set nosniff on the .json route; add planner-independent sentinel-sync test - contract.json.tsx: the actual MIME-sniffing defense is X-Content-Type-Options: nosniff, not the content escaping. The worker already sets it globally (baseSecurityHeaders); set it explicitly on this resource route too so it is safe on its own, and correct the comment that over-credited the escaping (review). - Add sort-index-sentinel-sync.test.ts: the date-sort index only matches while its COALESCE sentinel is byte-identical to SORTS in queries/contracts.ts. A .sql migration can't import a TS constant, so guard the coupling with a static cross-file check of the sentinels ('' and '9999-99') that fails on drift regardless of the DB engine - independent of the local sqlite3 planner the EXPLAIN test relies on (review). * chore: drop stray review-marker artifacts from this PR's comments Remove the bare '(review ...)' attribution notes I left in contract.json.tsx and sort-index-sentinel-sync.test.ts; the explanations stay. The pre-existing '(review #80)' issue references elsewhere are an established convention and are untouched. Comment-only. * test(db): cover filtered list sorts and guard the sqlite3 dependency Two review follow-ups on the ordering-index test: - Filtered sorts were documented as out of scope, leaving the reader unable to tell whether an active list filter makes the ordering index redundant. It does not: with a sector (tenders.cpv_code) or eu-funded filter the planner still walks idx_contracts_signed_desc and drops the sort step, while the pre-index baseline sorts the whole table. Asserted both directions. - A missing sqlite3 CLI surfaced as an opaque ENOENT. Probe it in beforeAll and fail with the fix. Deliberately not a skip: this is a perf/cost gate, and silently passing it on an image without sqlite3 would retire the gate. --------- Co-authored-by: Rumen Slavov <26761822+B353N@users.noreply.github.com> Co-authored-by: todorkolev <tkolev@obecto.com>
Two breakages surfaced once CI actually ran on this branch (the fork workflow runs were awaiting approval, so neither had been reported): - contract.tsx carried both sides of an earlier main merge, so contractIdFromSlug/getContract/ContractDetail were each imported twice and typecheck failed with TS2300. Keep the main-side @sigma/db import (it has contractSlug and getDb) and the branch-side type import (it adds CohortBand). - precompute-cohort.test.ts pinned the migration list to 0000/0001/0004, skipping 0002_current_value_currency. precompute.sql reads that column, so the test DB no longer matched the served schema and sqlite3 aborted with 'no such column: current_value_currency'. Read every migration from the directory instead, sorted - the same approach the sort-index test uses.
todorkolev
left a comment
todorkolev
left a comment
There was a problem hiding this comment.
Одобрявам. Ценовият ориентир по CPV кохорта е точно вида контекст, който липсваше - обяснява мащаба, без да обвинява, и е ясно отделен от рисковите флагове. Предизчисленият rollup е правилният избор: страницата чете един ред вместо да сканира цялата си дивизия при всеки изглед.
Оценявам предпазните мерки срещу подвеждане - праг от 12 договора, широки стъпала вместо фалшива точност, гейт при непотвърдена стойност, и методология, която описва ограниченията.
Две неща поправих в клона, след като CI най-после тръгна (форк пуските чакаха одобрение, затова досега не се е виждало): дублирани import-и в contract.tsx от по-ранен merge на main (typecheck падаше с TS2300), и списъкът с миграции в precompute-cohort.test.ts, който прескачаше 0002 и затова тестовата база не отговаряше на обслужваната схема - вече чете всички миграции от папката, като теста за индексите.
Миграцията 0004 е приложена върху sigma-stage-green предварително и cpv_division_stats е попълнена (45 дивизии, 189 613 договора с чиста стойност), така че деплоят пада върху готови данни.
…milar-contracts midt-bg#210, leaf-index+JSON-LD midt-bg#212, semgrep midt-bg#255); keep sharp pin + detailed osv notes
…af-index+JSON-LD midt-bg#212, semgrep midt-bg#255, osv midt-bg#271); keep sharp pin + all security overrides
…ilar-contracts midt-bg#210, leaf-index+JSON-LD midt-bg#212, semgrep midt-bg#255)
Resolve the contract.tsx import conflict by keeping both sides: the branch's isNaturalPersonProfileName (used by the natural-person noindex guard) and main's getDb + CohortBand (the „Подобни договори" cohort from midt-bg#210). Also correct an inverted claim in both disclaimer texts: the date flag fires when a contract is signed AFTER its publication date, not before - scripts/normalize-raw.sql:933 sets 'signed_after_publication' on `contract_date > date(published_at, '+2 day')`. The methodology copy now also states the two-day tolerance, so the reader can tell an entry-lag from an unexplained ordering.
midt-bg#216 (coverage harness) merged to main, so the vendored harness reconciles to the canonical one. Also folds in the feature work that landed since the branch base — midt-bg#263 (worker-native FX load, rewritten refresh Workflow), midt-bg#252 (Bulstat EIK control code), midt-bg#210 (similar-contracts cohort) — and re-validates the whole suite against the moved source. Conflict resolutions: - apps/etl/vitest.config.ts: take upstream (needs the SQL text-module plugin + cloudflare:workers alias for midt-bg#263's real-Workflow index.ts). - vitest.shared.ts: keep our fixtures/json/md/d.ts excludes; add **/src/test/** (SQLite/workers stubs are test scaffolding, not product code). - coverage-baseline.json: keep our floors (baseline reconciliation to the merged tree's actuals follows in a separate commit). - apps/etl/src/index.test.ts: take upstream's FX integration test; restore the orchestration coverage it does not cover in a new mock-based control-flow test. - packages/db/src/queries/home.test.ts: union both added fakeDb params (singleOffer + capture). Post-merge fixes for source drift: - search.suggest.test.tsx: stub getDb (the route now wraps env in getDb()). - index.control-flow.test.ts (new): capped-window, zero-ingest, FX-uncovered, integrity-gate logger + failure (Error and non-Error), scheduled — restoring the run()/scheduled() branch coverage displaced by taking upstream's test. Full suite green: config 27, shared 56, ingest 116, etl 40, db 458, web 469.
…fter rebase The merge onto current upstream surfaced three pre-existing issues that need to be addressed for the test suite and lint to pass: - apps/web/app/routes/contract.json.test.ts and contract.data.test.ts: add cohort: null to the makeRecord() fixture. Upstream's ContractRecord type now requires ContractCohortBenchmark | null (the 'Подобни договори' benchmark from the new cohort-band feature in PR midt-bg#210), and the fixtures predated it. - apps/web/app/lib/csv-export.test.ts, packages/db/src/queries/companies.ts, packages/db/src/queries/contracts.ts: prettier format. These three files were reformatted by the upstream prettier version (3.8.3 vs whatever the original PR ran on) — same content, just whitespace. The lint gate is blocking on these, so format fixes are non-optional. Verification: pnpm typecheck (7/7 packages clean), pnpm --filter @sigma/web test (532 passing), pnpm --filter @sigma/shared test (60 passing), pnpm lint (prettier --check clean).
5 participants
Какво и защо
Добавя секция „Подобни договори" на страницата на договора - разбираем ценови контекст на всяка поръчка: „1 000 000 € е в топ 5% по стойност сред 214 договора в сектор „Строителство" (CPV 45); медианата за сектора е 112 000 €." Различно от рисковите флагове (#127) и health индекса (#188): те казват „нещо е съмнително", това отговаря на „голяма ли е тази сума за този вид поръчка".
Как работи:
cpv_division_stats(миграция0002): персентили на стойността (p25/медиана/p75/p90/p95/p99) по двуцифрена CPV дивизия, nearest-rank (k = ceil(q·n)), само върху „чистата" кохорта -value_flag = 'ok',amount_eur > 0, известен CPV. Преизгражда се изцяло и в двата пътя:scripts/precompute.sql(§4c) иscripts/refresh-slice.sql(в batchglobals, приsector_totals/facet_counts) - тест пази двете копия идентични.@sigma/db(queries/cohort.ts):getContractCohortвръща бенчмарк само когато сравнението е честно - чиста стойност, наличен CPV, кохорта от поне 12 договора (същия праг като anomaly report-а). Договори с непотвърдена стойност не получават сравнение.scripts/anomaly-report.mjs, който е праг за флагване, не дисплей). UI-ят изрично казва, че това е контекст за мащаба, не оценка за нередност.scripts/compare-served-sqlite.mjsиdocs/etl-pipeline-state.mdвключват новата таблица.Свързан issue
Няма пряк issue; допълва посоката на #127 (рискови флагове) и #188 (health индекс) с неутрален ценови контекст.
Вид промяна
feat— нова функционалностКак е тествано
pnpm typecheck- минава (всички пакети).pnpm test- минава. Нови тестове: поведенчески SQL тест (precompute-cohort.test.ts) прилага миграциите, зарежда корпус със шум (suspect/NULL/нулеви стойности, липсващ CPV) и проверява nearest-rank персентилите на реален sqlite3 + идемпотентност + парити между precompute и refresh-slice; unit тестове заcohortBand/getContractCohort(нулеви пътища: suspect, малка кохорта, липсващ CPV).pnpm lint- чисто.Чеклист
Co-Authored-By:trailermidt-bg/sigma:mainpnpm typecheckминаваpnpm test(поне за засегнатите пакети) минаваpnpm lintе чисто.env*или.dev.varsdocs/etl-pipeline-state.md)