feat: свързани лица — детерминистична основа за данни за конфликт на интереси - #226
Conversation
Data foundation that links declared ownership stakes of public officials (КПКОНПИ asset-declaration register) to companies winning public procurement (ЦАИС ЕОП): where an official — or an anonymized close relative — holds a stake in a contract-winning firm. Search, leaderboard, and per-person / per-company / per-contract pages, behind a feature flag. Libel-safe by construction: deterministic name→ЕИК match (the normalizer is the sole libel surface, 100% covered with hard negatives), zero over-merge, close relatives never named, only private/family ownership surfaced (ex-officio and management roles excluded), a read-time contemporaneous split so a divested stake is never asserted as current, and all values computed in SQL. Includes the staging/prod CI/CD needed to ship it: the migration 0002 schema-apply step in deploy.yml, the scripts-test workflow (CACBG pipeline + scripts unit tests), and the related-persons-data ETL workflow (crawl → extract → resolve → audit → ship → reindex). Per-PR preview and dev-environment provisioning are deliberately excluded — those stay on the fork.
…ity telemetry The load-time trueOverMerge_LIBEL_GATE was a structural false-zero: it tiebroke a multi-ЕИК bucket on a strictKey that strips a superset of what companyNameKey folds (whitespace + quotes + .,-), so any two names sharing a companyNameKey necessarily shared a strictKey — the second clause could never be true, the counter was hardwired to 0, yet the loader printed '0 over-merges' and could exit(1) on it (review midt-bg#226). Replace it with honest telemetry: report keys that map to >1 distinct valid winner ЕИК (ambiguous_name_keys + examples), tied to NO exit code. These are already quarantined by the resolver (never published), so they carry no libel exposure — on the real corpus all 54 are presentation-only collisions (generic names / feed typos). The sound 0-over-merge proof stays where it belongs: the labelled company-name-key.test.ts. Proven data-neutral: substantive columns of persons/interest_links are byte-identical old-vs-fixed loader (only created_at flutters). Adds ADR-0027, refines the spec's Phase-0 claim, and spells out the ambiguous/generic-name quarantine on the public methodology page.
…ity telemetry The load-time trueOverMerge_LIBEL_GATE was a structural false-zero: it tiebroke a multi-ЕИК bucket on a strictKey that strips a superset of what companyNameKey folds (whitespace + quotes + .,-), so any two names sharing a companyNameKey necessarily shared a strictKey — the second clause could never be true, the counter was hardwired to 0, yet the loader printed '0 over-merges' and could exit(1) on it (review midt-bg#226). Replace it with honest telemetry: report keys that map to >1 distinct valid winner ЕИК (ambiguous_name_keys + examples), tied to NO exit code. These are already quarantined by the resolver (never published), so they carry no libel exposure — on the real corpus all 54 are presentation-only collisions (generic names / feed typos). The sound 0-over-merge proof stays where it belongs: the labelled company-name-key.test.ts. Proven data-neutral: substantive columns of persons/interest_links are byte-identical old-vs-fixed loader (only created_at flutters). Adds ADR-0027, refines the spec's Phase-0 claim, and spells out the ambiguous/generic-name quarantine on the public methodology page.
…ll holes Three false-negative limits the methodology page did not yet state: ownership held through an intermediate company (HoldCo), a winner that ran under a since-changed name, and Cyrillic/Latin script mismatch (we do not transliterate). All are safe recall misses, never false merges — stated up front on the public methodology page.
A declared_eik match (the official wrote the ЕИК inline and the winner's name also appears for cross-check) resolves the company via the national unique identifier, so identity is deterministic regardless of name genericness. Assign it the new tier A_eik and publish on that basis instead of holding it at C_hold; it is exempt from the TR name-uniqueness census (the ЕИК already renders name uniqueness moot). Name-only methods (exact_name_key, extracted_name) are unchanged — their certainty is the name, so they still ride the distinctiveness/seat gate. Real-data effect: 3 links / 2 closely-held companies (АТЕЛИЕ ДУО ЕООД, Файнанс Консулт ЕООД) promoted onto the public surface; the other declared_eik links are ex-officio/management roles and stay internal. Also surface the TR-blocked held subset on the methodology page (~400 name-only matches, ≈€408M nominal contract value, held pending the Trade-Register census) so the gap between declared and shown is transparent. ADR-0028.
…tity search midt-bg#204, amendments midt-bg#165); resolve shared barrel by exporting both company-name-key and search
Ревю на PR #226 — основа за данни „свързани лица"Прегледах черновата с фокус върху защитата срещу клевета (over-merge, публикационните инварианти) и сигурността. Заради обема и чувствителността проверих критичния път на няколко пласта; всяка находка по-долу е сверена срещу кода. Накратко: основата е стабилна — трите обявени гаранции (нула over-merge, host-scoped TLS без глобален bypass, XXE-safe парсване) издържат на adversarial проверка. Намереното е предимно defense-in-depth, плюс една дизайнерска дилема и един реален прод-блокер в CI. Проверено като солидно
Находки🔴 HIGH (CI) — прод-деплоят се къса на празно 🔴 HIGH (дизайнерска дилема, не бъг) — връзката „източник" при семеен дял води до документа, който назовава свързаното лице. Подзаявката за 🟡 MEDIUM (сигурност) — деструктивният data workflow няма предпазителя за прод-имена, който 🟢 LOW (fail-open) — 🟢 LOW / PLAUSIBLE (зависи от шаблона) — fallback-ът за колони в парсера може да сгреши при невиждан вариант на таблицата. 🟢 LOW (defense-in-depth) — ЕИК се приема само по формат (без mod-11 контролна цифра). За да се публикува, ЕИК-ът трябва точно да съвпадне с валиден победител и да мине проверката по име, така че контролната цифра не добавя защита срещу over-merge, каквато cross-check-ът вече не дава. → mod-11 като допълнителен gate. 🟢 LOW (коментар) — ЗаключениеСилна, добре защитена основа — защитата срещу клевета е закалена и издържа adversarial проверка. Преди merge: HIGH-ът в CI е реален блокер за прод-деплой; дилемата със семейния „източник" иска изрично продуктово решение (не е бъг, но е точно инвариантът, който инструментът пази); MEDIUM-ът за guard-а на деструктивния workflow е евтина застраховка. Останалото е defense-in-depth. За чернова — впечатляващо ниво на строгост. |
🇧🇬 Ревю на PR #226 — „свързани лица"
Сигурност (9/10) ✅
Архитектура (8.5/10)
База данни (8.5/10) · Производителност (8.5/10)
Тестове (9/10) · Документация (9.5/10)
Препоръки преди сливане (незадължителни)
Заключение: Отлична, клеветнически безопасна по конструкция основа. Одобрявам за сливане след потвърждаване на Phase-0 числата срещу реалния корпус. 👏 🤖 Генерирано ревю с паралелни специализирани агенти (сигурност, архитектура, БД, фронтенд, производителност, тестове/документация). |
…st_class default Addresses the two HIGH/MED findings from the midt-bg#226 review (nedda76): - deploy.yml: default SIGMA_D1_NAME to `sigma` in the schema-apply step. Prod leaves the var unset (uses the committed default), so a bare "$SIGMA_D1_NAME" ran `d1 execute ""` and aborted the first prod release (HIGH). - related-persons-data.yml: reject the prod D1 name `sigma` on non-prod dispatches. This rebuild wipes+reloads the свързани-лица tables but lacked the non-prod name guard deploy.yml already has — a misconfigured dev/staging run could wipe prod (MEDIUM). - ship-related-persons.mjs: resolveD1Name() refuses the `sigma` default on a --remote ship with an unset SIGMA_D1_NAME (root cause of the prod-wipe footgun; --local keeps the default). Unit-tested. - 0002 migration: interest_class DEFAULT private_ownership -> management_role, a non-surfaced class, so a future writer that sets status but omits the class cannot leak to the public surface (fail-closed; load.mjs always sets it).
…st_class default Addresses the two HIGH/MED findings from the midt-bg#226 review (nedda76): - deploy.yml: default SIGMA_D1_NAME to `sigma` in the schema-apply step. Prod leaves the var unset (uses the committed default), so a bare "$SIGMA_D1_NAME" ran `d1 execute ""` and aborted the first prod release (HIGH). - related-persons-data.yml: reject the prod D1 name `sigma` on non-prod dispatches. This rebuild wipes+reloads the свързани-лица tables but lacked the non-prod name guard deploy.yml already has — a misconfigured dev/staging run could wipe prod (MEDIUM). - ship-related-persons.mjs: resolveD1Name() refuses the `sigma` default on a --remote ship with an unset SIGMA_D1_NAME (root cause of the prod-wipe footgun; --local keeps the default). Unit-tested. - 0002 migration: interest_class DEFAULT private_ownership -> management_role, a non-surfaced class, so a future writer that sets status but omits the class cannot leak to the public surface (fail-closed; load.mjs always sets it).
Bring the upstream PR branch's deploy.yml to parity with the fork on two non-deploy-layer hardening bits it was missing: - echo "::add-mask::$key" before `wrangler secret put LOG_IP_KEY` — registers the generated key with the runner so an accidental set -x / echo in a later edit cannot leak it (masking is not retroactive within a line). - timeout-minutes: 10 on the deploy job — fail fast on a wedged run instead of burning a 6-hour slot; the required-reviewers wait does not count against it. The `dev` environment target + its provisioning docs stay fork-only (that is genuine deploy-layer). Surfaced while reflecting the midt-bg#226 review fixes.
|
@todorkolev — изскача продуктово решение, което искам да маркирам преди merge (не е бъг; @nedda76 го повдигна в ревюто си и е основателно). Въпросът. При семеен дял ( Съзнателно е — отбелязано в кода ( Опции.
Няма технически блокер и в двата случая — това е политически/правен избор за инструмент, който анонимизира близките именно за да не уврежда частни лица. Твоят call; ако е опция 2, влиза в fix batch-а. (HIGH-1 в CI и MEDIUM guard-ът за прод вече са поправени в кода — това тук е само за анонимността.) |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщено ревю на PR: „свързани лица — детерминистична основа за данни за конфликт на интереси"
Какво прави PR-ът
PR-ът изгражда детерминистична основа за данни за конфликт на интереси („свързани лица"): CACBG/TR crawler и loader-и, нормализация на имена, миграция 0002 (interest_links, link_suppressions), параметризирани заявки, precompute/refresh SQL слой, ship-скрипт към D1, фронтенд („Свързани лица", конфликтни карти, търсене), edge-worker логика (rate-limit, noindex, cache-tag), CI workflows и обширна ADR документация (0007–0028 + spec). Подходът е умишлено детерминистичен (без евристики), с publish/held/quarantine нива, anti-libel и anti-де-анонимизационни предпазители, и силна PII дисциплина.
Обща оценка
Качеството е последователно високо през всичките 8 партиди: изчистен и добре документиран код, смислени (не тривиални) тестове, които защитават реални libel/data-integrity инварианти, и умишлено силна поза по сигурност.
Сигурност (Фаза 0) — ЧИСТО във всички партиди: няма твърдо кодирани тайни (SPKI pin-овете и LOG_IP_KEY са коректно третирани), няма злонамерени шаблони/backdoor/обфускация, външните хостове са само легитимни български държавни регистри. SQL е параметризиран (.bind()/prepare(?)) или статичен; няма shell инжекция (execFileSync + argv); XXE е блокиран (assertNoDoctype); path traversal е санитизиран и адверсариално тестван; TLS pinning е по-строг от доверяване на публична CA; FTS5 инжекция е покрита. GitHub Actions са пиновани по SHA.
Най-важни находки
Блокиращи / изискват потвърждение преди merge:
link_keyинвариант (партиди 2 и 4) — най-съществено.eikсе взима директно от пътя без формат-валидация и се вгражда вlink_key. URL-кодиран|(%7C) позволява self-ендпойнтът да сервира family-обхвата, нарушавайки инвариант, който коментарите обявяват за невъзможен. Засяга и defamation-чувствителния път за поправки/сваляния (link_suppressions). Препоръка: валидирайтеeikс^\d{9,13}$(404 иначе) преди строене на ключа; сверете дефиницията на ключа сload.mjs. Партида 4 маркира това като REQUEST_CHANGES.- Нова зависимост
fast-xml-parser@^5.9.3(партиди 4, 5) — изисква човешко одобрение. Lockfile-ът съвпада с upstream, пакетите са first-party (не typosquat), без install-hooks — но веригата нараства от 1 на 6 много нови, слабо разпространени под-пакета, а caret-диапазоните допускат бъдещо издърпване на непрегледан код. Потвърдете нуждата и scope-а (изглежда като root devDependency — грешно място ако ingest го ползва в runtime), пинирайте точни версии/overridesи наложете--frozen-lockfile/pnpm auditв CI.
Препоръчани преди merge (неблокиращи, но важни):
3. Не-атомарен wipe→insert в ship-скрипта (партида 8). Провал по средата оставя обслужващата повърхност празна/частична без rollback. Препоръка: post-ship проверка на брой published връзки и/или документиран rollback в runbook.
За потвърждение (незадължителни):
- Покритие на чистите функции в
conflicts.tsза ≥90% бариера (партида 2). - Дублиран SQL в
search-sql.test.ts(собствено копие вместо четене на продукционния SQL → drift, фалшива увереност) — партида 5. - Fail-closed 503 засяга и некеширана публична
/conflicts/methodology; noindex върху цялата/search*повърхност — потвърдете умисъла (партида 3). .dataблизнаци на страници за компании/лица извън/conflicts— възможна експозиция от същия клас (партида 3).authoritySharesвзима тотала от първия ред;didбез folder-namespace; TLS single-pin с изтичане ~2027-01 (нужен мониторинг); дребни нормализации наeik.
Заключение
Няма блокиращи проблеми по сигурност или цялост в нито една партида. Двата елемента за задължително адресиране/потвърждение преди merge са: (1) валидация на eik/link_key инварианта и (2) човешко одобрение на веригата зависимости fast-xml-parser; силно препоръчително е и (3) rollback/верификация след ship. След тяхното изясняване PR-ът е готов за одобрение. Забележка: acceptance-критериите срещу описанието на тикета не са автоматично сверени (MCP конекторите не са оторизирани) — препоръчва се ръчна сверка преди merge.
Addresses the midt-bg#226 review (ydimitrof). - NOT_REDUNDANT_FAMILY and its mirrors (precompute.sql, refresh-slice.sql, and the search-sql test copy) collapsed the redundant family link by (person_id, bidder_id). That was correct only via the loader's implicit eik->bidder_id 1:1 (winners are keyed id='eik:'||eik). Key the collapse on (person_id, eik) instead, so the libel-critical ADR-0023 de-anonymisation guard is correct-by-construction, independent of the bidders-id scheme. Behaviour-identical on real data; a new adversarial test with a duplicate-eik bidder proves it discriminates (RED on the old bidder_id predicate). - The 0002 link_key comment documented only the self form (pid|eik); a family link's key carries a |family suffix (load.mjs), so the self+family rows are distinct and UNIQUE holds. Corrected the comment and added a family-suppression round-trip test: a takedown keyed pid|eik|family must survive re-import, or a family (defamation-sensitive) suppression silently no-ops.
|
@ydimitrof Благодаря за детайлното ревю — минахме през всяка находка. Какво предприемаме и какво изяснихме: Адресираме (отделни commit-и, първо във форка):
Изяснено — без промяна:
Останалото (не-атомарен ship, ConflictCards error state, oversized-batch guard) — follow-up. Промените минаха adversarial валидация във форка (RED→GREEN + пълна регресия) и ги отразяваме тук. |
Addresses the midt-bg#226 review (ydimitrof). - NOT_REDUNDANT_FAMILY and its mirrors (precompute.sql, refresh-slice.sql, and the search-sql test copy) collapsed the redundant family link by (person_id, bidder_id). That was correct only via the loader's implicit eik->bidder_id 1:1 (winners are keyed id='eik:'||eik). Key the collapse on (person_id, eik) instead, so the libel-critical ADR-0023 de-anonymisation guard is correct-by-construction, independent of the bidders-id scheme. Behaviour-identical on real data; a new adversarial test with a duplicate-eik bidder proves it discriminates (RED on the old bidder_id predicate). - The 0002 link_key comment documented only the self form (pid|eik); a family link's key carries a |family suffix (load.mjs), so the self+family rows are distinct and UNIQUE holds. Corrected the comment and added a family-suppression round-trip test: a takedown keyed pid|eik|family must survive re-import, or a family (defamation-sensitive) suppression silently no-ops.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщено ревю на PR: „свързани лица — детерминистична основа за данни за конфликт на интереси"
Какво прави PR-ът
PR-ът въвежда детерминистична основа за данни за конфликт на интереси на длъжностни лица („свързани лица"). Обхваща целия конвейер — от извличане и парсване до публикуване и представяне:
- ETL/скриптове (
scripts/cacbg/*,scripts/ship-related-persons.mjs): fetch с TLS certificate pinning, XML парсър със защита срещу XXE, санитайзери срещу path traversal, детерминистичен резолвър, census/quarantine логика и отделен ship-скрипт от work-SQLite към обслужваната D1. - Данни (
0002_related_persons_foundation.sql,queries/*,api-contractDTO-та): миграция, параметрични заявки за връзки/идентичност/търсене, precompute/refresh-slice SQL. - Публична повърхност (Cloudflare worker,
ConflictCards.tsx,SmartSearch, CSS): rate-limiting срещу enumeration,noindexза именни страници, достъпен UI компонент, поправка наDEPLOY_TAG. - Инфраструктура/документация: GitHub Actions workflow-и (pinned към commit SHA), обширна верига ADR-и (0007→0025) с ясна fail-closed позиция по libel-риск и PII.
Обща оценка
Висококачествена, добре документирана и добре тествана промяна със силна защитна култура — грижа за сигурност, PII-релси и защита от клевета в целия конвейер. Всичките 8 партиди дадоха вердикт COMMENT — няма открити критични уязвимости и няма блокиращи проблеми.
Сигурност (Phase 0 — CLEAN във всички партиди)
- Няма хардкоднати тайни, backdoor-и или обфускация.
CACBG_SPKI_PINе публичен ключов отпечатък; тайните минават през GitHub Environment secrets. - SQL/FTS/команден инжекшън: продукционният код е изцяло параметризиран;
sqlLiteral/sqlIdentекранират коректно;execFileSync(без shell) предотвратява command injection. - XSS/XXE:
isHttpsUrlотхвърляjavascript:/data:; парсърът отхвърляDOCTYPE/ENTITY. - TLS pinning: ограничено до един host с ръчна SPKI верификация, fail-closed.
- PII/libel: анонимизацията на свързаните лица е структурно наложена; имената на роднини съзнателно се изключват от публичната D1 (покрито с тест); de-anonymization векторът е затворен.
- Нова зависимост
fast-xml-parser@^5.9.3и транзитивните ѝ пакети са проверени — легитимна модуларизация от същия автор, без install-скриптове (некритична бележка: пакетите са нови и single-maintainer).
Най-важни находки за потвърждение преди merge (некритични)
- Cross-folder колизия на имена в
load.mjs(най-съществено): декларацията се ключира само по базово име (decl:${h.xmlFile}). Еднакво име в различни folder-и може приINSERT OR IGNOREда закачи интереси към чуждо лице — точно грешната атрибуция (libel-adjacent), която PR-ът цели да избегне. Препоръка: ключиране поfolder+xmlFile(или controlHash) + тест за колизия. - Съгласуваност на €-сумите: формулата за сумата на „официал" в
precompute.sql(lifetimecontract_value_eur) изглежда различна отrefresh-slice.sql(contemporaneous прозоречна сума). Да се потвърди, че двете изчисляват идентична стойност, за да не се промени публичната цифра след refresh. DEPLOY_TAGв worker бъндъла: да се потвърди, че Vitedefineреално достига worker пасажа, иначеtypeofguard-ът тихо се връща къмDate.now().- Извеждане на rate-limit ключа: поведението при липсващ
CF-Connecting-IPда е потвърдено (споделен vs. празен ключ = заобикаляне), тъй като това е единственият anti-enumeration контрол. - Case/whitespace чувствителност в
parse.mjs: сравнениетоholder === declarantбез нормализация би могло да класифицира СОБСТВЕН дял като „related" → фалшивfamily_ownershipлинк. Препоръка: trim + case-fold преди сравнение.
Дребни забележки
related.jsonlцикълът вload.mjsда приложи empty-name guard-а, ползван при holdings.fetch.mjs: да се валидират--limit/--concurrency; затваряне на write-streams/DB вfinally.- Паразитна дума „ponytail" в коментара на
LINK_SELECTда се махне. - Дублиране на блока за длъжностни лица между
precompute.sqlиrefresh-slice.sql(риск от разминаване); неизползвани JOIN-ове да се документират или премахнат. - Неатомарен ship (
ship-related-persons.mjs): при провал между wipe и re-insert живата D1 остава празна/частична — да се документира процедура за възстановяване при--remote. - Некритични: липса на retry при fetch в
ConflictCards; тестови fixtures интерполират низове в SQL (само тест, анти-паттерн).
Заключение
Няма блокиращи проблеми. Кодът е готов за merge по същество; препоръчва се да се адресира или потвърди списъкът по-горе — с приоритет т.1 (cross-folder колизия) и т.2 (съгласуваност на €-сумите) — преди финален APPROVE. Отлична дисциплина по провенанс, PII и libel-защита.
Addresses the midt-bg#226 review (ydimitrof). parseAssets classified a stake as a relative's (family_ownership) whenever the holder-name cell didn't EXACTLY match <Personal><Name>. But the holder column is hand-typed and, for a self stake, repeats the declarant's own name — any casing or spacing drift ("ИВАН ПЕТРОВ ТЕСТОВ" vs "Иван Петров Тестов") flipped a SELF stake to a fabricated family_ownership link, asserting a non-existent relative's holding on the libel-sensitive public surface (the misattribution this feature exists to avoid; ADR-0023). Compare on a case-folded, whitespace-collapsed nameKey instead. Test-first: a RED test pins that a self stake whose holder repeats the own name with case/space drift stays 'self' with no fabricated family holding.
|
@ydimitrof Благодаря — минахме през целия списък. Едно приех и поправих; за двете „за потвърждение" ето какво показва кодът: Поправено —
Останалото: не-атомарен ship — осъзнат компромис, документиран в кода (recovery = повторен ship), follow-up. noindex за Проверих и живата повърхност (preview): анонимизацията на близките държи (никакво име в DOM/ |
Addresses the midt-bg#226 review (ydimitrof). parseAssets classified a stake as a relative's (family_ownership) whenever the holder-name cell didn't EXACTLY match <Personal><Name>. But the holder column is hand-typed and, for a self stake, repeats the declarant's own name — any casing or spacing drift ("ИВАН ПЕТРОВ ТЕСТОВ" vs "Иван Петров Тестов") flipped a SELF stake to a fabricated family_ownership link, asserting a non-existent relative's holding on the libel-sensitive public surface (the misattribution this feature exists to avoid; ADR-0023). Compare on a case-folded, whitespace-collapsed nameKey instead. Test-first: a RED test pins that a self stake whose holder repeats the own name with case/space drift stays 'self' with no fabricated family holding.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR: „Свързани лица — детерминистична основа за данни за конфликт на интереси"
Обобщение
PR въвежда read-only, детерминистичен слой за „свързани лица" за целите на разкриване на конфликт на интереси. Обхватът включва: DTO-та и миграция 0002, заявки (related-persons.ts, разширение на търсенето, slug-помощници), CACBG pipeline (fetch/parse/load/guard/tls), TR-census, precompute/refresh SQL, ship-скрипт от работна SQLite към обслужваната D1, обширни интеграционни и юнит тестове, седем ADR-а и спецификация. Кодът е с високо инженерно качество, добре документиран, детерминистичен/идемпотентен и с внимателно обмислена libel/PII позиция.
Сигурност (Phase 0 + OWASP) — ЧИСТО във всички партиди
- Няма hardcoded тайни, backdoor-и, обфускация или code injection.
CACBG_SPKI_PINе публичен SPKI отпечатък (не тайна) с документирана out-of-band ротация. - SQL инжекция: всички продукционни заявки са параметризирани (
.bind()/prepare().run()); статичните SQL фрагменти не интерполират потребителски вход; ship-скриптът коректно екранира на границата към D1 (sqlIdent/sqlLiteral, покрити с тестове). Стрингова интерполация има само в тестови фикстури с контролирани стойности. - Няма shell injection (само
execFileSyncс масив от аргументи), path traversal е спрян отguard.mjs, XXE е предотвратен (fast-xml-parserбез DTD/external entities +assertNoDoctype). - TLS pinning е fail-closed и по-строг от доверие към публично CA; глобалната TLS верификация НЕ е изключена.
- PII rail (§8): имената на трети лица/роднини никога не се персистират в публичната повърхност;
interest_classе fail-closed; де-анонимизиращият existence-oracle е затворен (ADR-0023); partial-census libel-gate отказва да фабрикува „false-unique" твърдения. - Нова зависимост
fast-xml-parser@^5.9.3: проверена като легитимна (истински maintainer, без install-скриптове), но въвежда нова верига от малко популярни транзитивни пакети — изисква еднократно човешко одобрение преди merge.
Качество и тестове — силно
Тестовете са смислени, разкриват реални дефекти (libel/false-attribution, де-анонимизация, contemporaneous прозорец, FK ред при reseed с негативни контроли) и не са тривиални. Миграционната верига е приложена коректно.
Открити забележки (не-блокиращи, за адресиране/уточнение преди merge)
- fetch.mjs — circuit breaker пропуска устойчиви HTTP грешки.
consecutiveрасте само при мрежови throw-ове; стена от 403/429/5xx никога не задейства breaker-а. Препоръчва се да се отчита и в non-200 клона. (Най-съществената забележка за устойчивост.) - Мъртви INNER JOIN-ове в amount под-заявката (
precompute.sql,refresh-slice.sql):JOIN tenders/JOIN authoritiesне се използват и могат тихо да отпаднат договори без свързан ред → подценяване на сумата. Да се потвърди референтна цялост или да се премахнат. - Инвариант брой vs. сума на договорите:
contemporaneous_contract_countеCOUNT(*)(включваamount_eur IS NULL), докато сумата пропуска NULL — тестът за инвариант да покрива и брой, за да не се появи „20 от 15 договора". - Обхват по bidder: contemporaneous под-заявките съединяват по
eik_normalized, а главната сума поbidder_id— да се потвърди инвариантът „един ЕИК → един winner ред". - load.mjs: липсва nameless-guard при вмъкване на person от
related.jsonl(риск от сливане на декларанти във вътрешната PII таблица); възможна колизия на declaration id при преизползвано XML име между папки (provenance разминаване); повтарящо сеdb.prepareв related цикъла (дребна перф.). - tr-census.mjs:
readFileSync+JSON.parseвърху пълен TR dump рискува OOM — да се обмисли streaming при full-register run. - Дребни: нисък ReDoS риск в
extract-companies.mjs; булеви footgun-и вarg()парсването на ship-скрипта; lockfile дисциплина (--frozen-lockfileвъв всички workflow-и); зависимост на интеграционните тестове отsqlite3CLI и експерименталнияnode:sqlite.
Заключение
Няма открити блокиращи проблеми по сигурност, инжекция, целостност на данните или зловреден код в нито една партида. Кодът е отбранително написан с ясни PII предпазни механизми. Преди merge се препоръчва: (а) човешко одобрение на новата верига транзитивни зависимости на fast-xml-parser, и (б) адресиране/уточнение на т.1 (circuit breaker) и т.2 (мъртви JOIN-ове); останалите са nits.
Общ вердикт: APPROVE с условия — партидите 4–7 са COMMENT в очакване на кръстосаната проверка на изброените инварианти, а партида 8 е APPROVE. Няма блокери; изброените точки са за потвърждение/дообработка.
…g#226) The crawler skipped a whole set on a non-200 list.xml with a single log line and counted per-declaration errors without ever failing — a transparency platform could publish a silently short list, while the bidders side already fails before the resolver on an export-vs-source mismatch (integrity-checks). Reconcile announced vs obtained per set: a 404 is a legitimate source gap (listed-but-unpublished), but a non-404 miss or a wholesale-skipped set is a real shortfall. Report the numbers and exit non-zero on shortfall, with --allow-incomplete for a knowing operator override. Reported by todorkolev on midt-bg#226 (#2).
…t-bg#226) PRODUCTION_SLOTS was ['sigma','sigma-green'], but the real production slots are sigma-blue and sigma-green (deploy.md) — there is no slot named 'sigma'. Because sigma-blue was absent from the list, the "a non-production run may not name a production slot" guard did not catch it, collapsing two independent defenses (name + id) to one. The code inherited a stale deploy.md note that said to keep a slot named 'sigma'; that was never done. Fix the constant, the workflow env guard, and align the deploy.md note with its own table. Test: both real prod slots are now refused under a non-production ship env. Reported by todorkolev on midt-bg#226 (#3).
|
@todorkolev — и трите са поправени, отразени и на двата PR-а (#226 = #61 byte-parity по feature файловете). Пуснах пълния корпус за числата, които поиска. Числата (пълен пуск)Корпус: 162 005 декларации (135 184 имуществени + 26 821 за интереси — 16,6% са за интереси, което покрива изложеното, което ти измери). Bidders база: 17 669 изпълнителя / 195 147 договора — това е build DB-то
B1 на пълния корпус: per-person хоризонтът е свалял погрешно 69 връзки. 23 от тях са истински публикувани конфликти (22 свои + 1 семейна) — връщат се. Другите 46 минават от „оттеглена“ в „задържана“. Нула нови оттегляния (published→withdrawn): поправката коригира само в безопасната посока — маха невярно оттегляне, никога не въвежда ново. Точно както го описа: „маха вярна, не задържа съмнителна“. Върнах и числото в коментара на Трите поправки
§2 ал.3 канарчето мина зелено на пълния пуск (всички материални семейни дялове от 'assets'). CI е зелено и на двата PR-а. Конкретният случай с точните файлове от регистъра, който спомена — прати ми го, ще го добавя като RED fixture, за да е закован завинаги. |
…idt-bg#226) The pure decision (assessCompleteness) was unit-tested, but run()'s wiring — incomplete crawl → non-zero exit — was only eyeballed. Make run() return the exit code instead of setting process.exitCode internally (a pure, testable decision; the top-level assigns it to process.exitCode), and inject the I/O boundary (httpGet, rawDir, argv, scratch guard). getPinned pins the CACBG host so a fake server is impossible — injection is the only offline seam; the defaults reproduce production 1:1. fetch-gate.test.mjs drives the real run() with a fake getter + a temp raw dir: complete crawl -> 0, a 404 (source gap) -> 0, a non-404 miss -> 1, a wholesale-skipped set -> 1, --allow-incomplete -> 0; plus a subprocess case proving the returned code becomes a real non-zero process exit. Mutation-checked (break the gate / ignore the override → exactly the right cases fail).
…idt-bg#226) The pure decision (assessCompleteness) was unit-tested, but run()'s wiring — incomplete crawl → non-zero exit — was only eyeballed. Make run() return the exit code instead of setting process.exitCode internally (a pure, testable decision; the top-level assigns it to process.exitCode), and inject the I/O boundary (httpGet, rawDir, argv, scratch guard). getPinned pins the CACBG host so a fake server is impossible — injection is the only offline seam; the defaults reproduce production 1:1. fetch-gate.test.mjs drives the real run() with a fake getter + a temp raw dir: complete crawl -> 0, a 404 (source gap) -> 0, a non-404 miss -> 1, a wholesale-skipped set -> 1, --allow-incomplete -> 0; plus a subprocess case proving the returned code becomes a real non-zero process exit. Mutation-checked (break the gate / ignore the override → exactly the right cases fail).
|
@todorkolev — малка добавка по #2: гейтът за пълнота вече е покрит с интеграционен тест, не само на око.
Mutation-проверен: счупя ли гейта ( Fork-first, и на двата PR-а, CI зелено. |
todorkolev
left a comment
todorkolev
left a comment
There was a problem hiding this comment.
Проверих последните два комита (9821207 и bb824a1): гейтът за пълнота вече е и интеграционно тестван, а run() връща изходния код вместо да пипа process.exitCode отвътре - поведението е същото, seam-ът е само за тестове. Трите находки от ревюто ми са поправени, CI е зелен, нишките са затворени.
Мърджваме. Отделно ще отворя issue с описание на следващата стъпка (справки в Търговския регистър като доказателствен слой) и мини-PR за една находка в parseList, която открихме при независима проверка на корпуса.
conflictHeadline summed contractValueEur/contemporaneousValueEur per link. NOT_REDUNDANT_FAMILY collapses only a single official's own+family stake in one winner, so two different officials linked to the same contractor both reach the array and that winner's money was counted twice (+8.1% / ~7.9M EUR on the full corpus, midt-bg#226). Aggregate money per eik instead: contract_value_eur is constant within an eik (exact dedup); contemporaneous_value_eur is a per-link window subset that can differ between officials on the same winner, so take the max per eik (deterministic, never overstated). linkCount/officialCount stay per-link and per-official. Refs midt-bg#226
conflictHeadline summed contractValueEur/contemporaneousValueEur per link. NOT_REDUNDANT_FAMILY collapses only a single official's own+family stake in one winner, so two different officials linked to the same contractor both reach the array and that winner's money was counted twice (+8.1% / ~7.9M EUR on the full corpus, midt-bg#226). Aggregate money per eik instead: contract_value_eur is constant within an eik (exact dedup); contemporaneous_value_eur is a per-link window subset that can differ between officials on the same winner, so take the max per eik (deterministic, never overstated). linkCount/officialCount stay per-link and per-official. Refs midt-bg#226
load.mjs's end-of-load JSON summary summed the per-eik contract_value_eur across published links, double-counting a winner whose stake is declared by two different officials — the ETL-summary twin of the conflictHeadline fix (midt-bg#226). Dedup all five money fields per eik via MAX (exact, constant within an eik): published_contract_value_eur, the private/family value totals, the by_interest_class value, and the own-institution total (per eik+authority). Counts stay per-link/per-official. The load test adds two officials on one winner and asserts the reported total equals the per-eik-deduped sum (naive minus dedup equals the shared winner's value; mutation-proven). Refs midt-bg#226
load.mjs's end-of-load JSON summary summed the per-eik contract_value_eur across published links, double-counting a winner whose stake is declared by two different officials — the ETL-summary twin of the conflictHeadline fix (midt-bg#226). Dedup all five money fields per eik via MAX (exact, constant within an eik): published_contract_value_eur, the private/family value totals, the by_interest_class value, and the own-institution total (per eik+authority). Counts stay per-link/per-official. The load test adds two officials on one winner and asserts the reported total equals the per-eik-deduped sum (naive minus dedup equals the shared winner's value; mutation-proven). Refs midt-bg#226
todorkolev
left a comment
todorkolev
left a comment
There was a problem hiding this comment.
Одобрението ми беше отменено от пуша с поправките, затова го подавам наново върху e3bd701.
Прегледах двата нови комита: дедупликацията по ЕИК в conflictHeadline е точна за contract_value_eur (константна в рамките на ЕИК) и консервативна за contemporaneous_value_eur през MAX - правилният избор, понеже прозорецът е по декларираните години на връзката и наистина може да се различава между две лица за един и същ изпълнител. Близнакът в обобщението на load.mjs беше пропуск от моя страна, благодаря че го хвана.
Всички нишки са затворени, CI е зелен.
Отменено: тялото на това ревю е Not logged in · Please run /login, тоест артефакт от прекъсната сесия на инструмента, а не преценка по същество. Блокиращият проблем от предходното съдържателно ревю (валидация на eik в conflict.contracts.tsx) е поправен на e3bd701 - /^\d+$/ плюс валидация на обхвата. Останалите бележки са пренесени от самия автор в #279.
|
Затварям последните пет нишки от ревюто ми на 30 юли. Проверих всяка срещу
С това нерешени нишки няма. Одобрението ми стои върху |
…ersons) midt-bg#226 (свързани лица — the conflict-of-interest data foundation) landed on main, adding a large new surface: packages/db related-persons queries, the /conflicts routes + ConflictCards, the apps/web conflicts lib and rate limiter, shared company-name-key, and the scripts/cacbg pipeline. The merge itself was clean — no conflicts; both overlapping test files (identity.test.ts, search.test.ts) auto-merged. The coverage ratchet did fail, though: midt-bg#226 ships its own tests but to a lower bar than this branch's floors, dropping apps/web to 98.86% lines / 93.01% branches (floors 99.4/96) and packages/db branches to 97.77% (floor 98.3). Per the never-lower rule this is fixed with tests — NO baseline floor is touched. Every case below targets a genuinely reachable branch; nothing is faked and no source is edited to suit a test. - ui.test.tsx (new): the shared editorial primitives had no direct test, only incidental page coverage of whichever variant that page used. Covers every optional-prop branch — Chip tones, ЕИК URL-encoding + rel=noopener, the OwnershipChip null/three-kind split, Flag variants, ShareBar clamping outside 0–1, Callout h3-default vs explicit h2 (heading order) and its variant class, Section with and without a hint. - conflicts.render.test.tsx: an undated + unnumbered + authority-less contract (no timeline, „—" body, generic „договор" label, never „№ null"), a sub-threshold „под 0,1%" share with no plotted bar, a no-denominator „—" share, an empty drill-down, and an all-outside-the-window contract set. - conflicts.test.ts: unknown temporal tag falls back to the raw value, year-0 / non-numeric signing dates are refused by parseYear, and authority-share ordering with unknown denominators (plottable first, money breaks a tie). - conflict.pages.render.test.tsx: meta() on both conflict pages with the loader data absent — the error-boundary render must degrade to the generic noun, not interpolate "undefined" into a public page title. - conflicts.loaders.test.ts: route params absent entirely, not merely blank. - conflicts-rate-limit.test.ts: non-GET/HEAD methods bypass the budget (it guards read scraping, and a write is not that vector). - related-persons.test.ts: a NULL joined authority maps to '' so the card never renders a literal null, plus isMissingConflictTableError against a non-Error rejection and against a core (non-0003) missing table. Gate green with no floor lowered: web 99.16/95.84, db 99.82/97.96, etl 100/97.46, ingest 100/98.36, config 100/100, shared 98.87/98.33. Full suite: config 27, shared 65, ingest 119, etl 40, db 505, web 592.
…acbg gate midt-bg#281) Two commits landed on main while the midt-bg#226 sync was in flight — an undici override bump to ^7.29.0 and a cacbg completeness-gate fix (phantom rows / --limit). Clean merge; the surface is pnpm-workspace.yaml + scripts/cacbg, none of it inside the six measured workspaces, so the coverage baseline is untouched and the gate stays green. Verified against the merged tree: scripts tests 51 pass, cacbg pipeline tests 84 pass (run via the register-ts loader, as the CI job does), coverage ratchet green for all six workspaces.
Индексът го отбелязва като заменен от ADR-0030 от PR midt-bg#226 насам, но заглавието на файла продължаваше да казва „Accepted". Разминаване заглавие/индекс, забелязано при синхронизирането на индекса за ADR-0033.
The index has marked it superseded by ADR-0030 since PR midt-bg#226, but the file header still read "Accepted". A header/index divergence, spotted while synchronising the index for ADR-0033.
The index has marked it superseded by ADR-0030 since PR midt-bg#226, but the file header still read "Accepted". A header/index divergence, spotted while synchronising the index for ADR-0033.
Clean automatic merge: no conflicts. Upstream advanced 3 commits since the PR's previous rebase (related-persons midt-bg#226, undici bump midt-bg#282, cacbg fix midt-bg#281); none of those touch the PR's test-only surface (apps/web/test/integration/*, docs/spec/integration-testing.md, apps/web/vitest.integration.config.ts). The single auto-merged file is docs/README.md, which gained a new ADR entry in upstream (0032); the merge preserves the alphabetical/numerical ordering without re-flowing the PR's content. Verification (local): - pnpm typecheck → 7/7 packages clean - pnpm --filter @sigma/web test → 530 passing (52 files, 8 integration files, 41 integration tests, 0 .skip) - pnpm lint → (run separately)
5 participants
Накратко
Детерминистична основа за данни за слоя „свързани лица": свързва
изпълнители по обществени поръчки с длъжностни лица (и, анонимизирано,
техни близки), декларирали финансов интерес в тях. Само публични данни.
Свързаност „по собственост" от Търговския регистър е извън обхвата (виж #60).
Този слой я заобикаля с друг публичен източник — декларациите за имущество
и интереси по ЗПК, публикувани в Публичния регистър на Сметната палата
(чл. 75 ЗСП). Съпоставя се само деклариран дял (свой или на свързано лице),
никога изведена свързаност по ТР.
Обхватът тук е контингентен, а не „отключващ" #60/#128 — доставя данните и
инвариантите, върху които стъпват тези дискусии, но не ги затваря.
Спецификация:
docs/spec/related-persons-foundation.md.Какво съдържа
ETL (
scripts/,scripts/cacbg/,packages/ingest/)register.cacbg.bg) — resumable, кеширан поxml_file+ControlHash, host-scoped TLS (без глобален bypass).фирма + правна форма, град, стойност, вид на връзката).
единственият слой за клевета. Нула over-merge на ръчно етикетиран набор
(form-only разлики като „АЛФА" ЕООД срещу „АЛФА" АД остават разделени).
съвпадение с целия
bidders→ сдвоеният ЕИК (уникален по ЗТРРЮЛНЦ чл.21 т.7);(2) когато деклараторът сам е изписал ЕИК заедно с фирмата (двойна проверка
име+ЕИК), ЕИК-ът — националният уникален идентификатор — разрешава фирмата
дори при родово име (ниво
A_eik, ADR-0028).(contemporaneous), не за целия живот на връзката — ADR-0024.
Заявки и представяне (
packages/db/,apps/web/)собственост).
private_ownership/family_ownershipсъсстатус
published; ex-officio/управление никога не се показва.GitOps / CI-CD
related-persons-data.yml—workflow_dispatch(dev/staging/prod, full_crawltoggle): Extract→Hydrate→Resolve→Audit→Apply schema→Ship→Reindex.
scripts-test.yml— node:test за standalone скриптовете + CACBG pipeline.deploy.ymlприлага схемата0002_related_persons_foundation.sqlкъм целеватасреда преди деплой на Worker-а (идемпотентно,
CREATE … IF NOT EXISTS).Решения (ADR)
(име, ведомство).доказателството е етикетираният тест.
A_eik),освободен от ТР-преброяването.
Инварианти (защита срещу неоснователно обвинение)
с декларирания период.
когато деклараторът сам е изписал ЕИК заедно с фирмата; тогава основанието е
ЕИК-ът (националният уникален идентификатор), не името (ADR-0028).
Обхват на промяната
Тази чернова е самостоятелна и напълно съвместима с main — не носи никаква
ephemeral/preview инфраструктура; всяка стъпка от CI/CD работи за staging и prod
както е.
Мащаб: 99 файла, ~9,9k реда, 22 ADR-а (0007–0028), ~200 теста.
Свързани: #60 (дискусия), #128 (картели — отделен слой).