Skip to content

fix(assistant): integrity, grounding & guard fixes from the PR #79 review - #223

Open
nedda76 wants to merge 12 commits into
midt-bg:mainfrom
nedda76:fix/assistant-integrity-grounding
Open

fix(assistant): integrity, grounding & guard fixes from the PR #79 review#223
nedda76 wants to merge 12 commits into
midt-bg:mainfrom
nedda76:fix/assistant-integrity-grounding

Conversation

@nedda76

@nedda76 nedda76 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Отстранява набор от конкретни дефекти в асистента, открити при ревю на PR #79 (насочено към петте области, които екипът поиска: интегритет на стойностите, SQL guard-а, RAG/grounding-а, hardening-а и provisioning-а). Всяка находка е проверена срещу кода/миграцията и покрита с тест.

Какво влиза (по комит)

1. Речник на данните ↔ миграция (describe-schema.ts) — dictionary-ът, който моделът третира като твърд факт, беше се разминал с packages/db/migrations/0000_init.sql:

  • amendments няма contract_id — връзката е през unp/contract_number;
  • parties няма role — реалните колони са party_key, eik, ocid, party_id, name…;
  • value_flag enum-ът беше без value_low;
  • amount_eur IS NULL беше описано като „= value_suspect“, а има няколко причини (чужда валута без курс / value_suspect без оценка / липсва подписана+текуща); броят „непотвърдени“ е value_flag='value_suspect' (home_totals.suspect), не редовете с NULL;
  • data_freshness е таблица, не view.

2. RAG grounding (rag.ts, system-prompt.ts) — затваря случая, в който RAG ход остава по-слабо ограничен от no-RAG fallback-а:

  • твърдите DATA_TRAPS вече се инжектират безусловно (RAG само добавя релевантните таблици/примерни заявки), така че пропуск в retrieval-а не може да изхвърли правилото SUM(amount_eur);
  • добавен е праг на релевантност (MIN_SCHEMA_SCORE) — под него връщаме по-малко/нула чънкове, а нула връща пълния речник (безопасният изход).

3. SQL scalar guard (sql-guard.ts) — блокира group_concat / json_group_array / json_group_object: колабират цял table scan в една огромна клетка, която се материализира преди capRows — същият клас memory-amplification, който вече е блокиран за printf/randomblob, едно ниво по-нагоре.

4. Интегритет на справката (report-schema.ts, emit-report-schema.ts):

  • prose gate-ът пропускаше трилион/билион/квадрилион — „3 трилиона лева“ минаваше целия gate (цифрата не стига до „лева“ през кирилската дума), необвързана стойност с порядък по-висока от „12 млрд.“; добавени към шаблона на изписаните величини;
  • align на колона вече се валидира срещу left|right, а resolved колоните се строят изрично (без spread), за да не стигне неизвестно свойство до renderer-а;
  • добавени тавани на дължините на масивите (blocks/items/columns) във validateEmitShape.

Съзнателно ИЗВЪН обхвата (за отделно обсъждане/PR)

Това са архитектурни решения, не еднолинейни поправки — държа ги настрана, за да остане PR-ът ревюируем, и ги описвам в коментара към PR #79:

  • read-only D1 път / реален opcode(EXPLAIN) слой за run_sql — днешните два слоя стоят пред read-write binding;
  • account-wide circuit-breaker за платените BgGPT ходове (декларираният DO още не е закачен);
  • first-query cost bound (DoW: първата заявка на хода тече без таван);
  • cross-check на link.kind спрямо префикса на id-то и съгласуваност format↔стойност.

Проверка

  • pnpm test (apps/web): 342 passed
  • pnpm typecheck: чисто
  • prettier --check: чисто

Базирано на main, защото кодът на асистента вече е там; ако предпочитате да влезе през feat/ai-assistant, лесно пренасочвам.

nedda76 added 4 commits July 9, 2026 16:33
The curated dictionary the model treats as hard fact had drifted from
packages/db/migrations/0000_init.sql:
- amendments: no contract_id column — it links via unp/contract_number
- parties: no role column — real cols are party_key, eik, ocid, party_id, name…
- value_flag enum was missing value_low
- amount_eur IS NULL was described as meaning value_suspect; it actually has
  several causes (FX-rateless foreign / value_suspect w/o estimate / no
  signing+current), and the unconfirmed count is value_flag='value_suspect'
  (home_totals.suspect), not NULL-amount rows
- data_freshness is a table, not a view

Drift here misleads a weak model into wrong joins or a wrong integrity KPI.
…e retrieval

Two grounding gaps that could leave a RAG turn LESS constrained than the
no-RAG fallback:
- buildSystemPrompt used the retrieved chunks INSTEAD of the dictionary, so a
  retrieval that missed the money-sum trap dropped the SUM(amount_eur) rule
  entirely. Inject the short imperative DATA_TRAPS unconditionally; RAG now only
  selects the extra tables/example-queries for the question.
- retrieveSchemaContext had no relevance floor — top-K returned its K
  least-distant chunks even when all were off-topic. Add MIN_SCHEMA_SCORE; below
  it we return fewer/zero chunks, and zero falls back to the full dictionary
  (the safe outcome).
group_concat / json_group_array / json_group_object collapse an entire
full-table scan into one huge cell that materialises in Worker memory before
capRows can measure it (and capRows keeps the first row whole) — the same
memory-amplification class already blocked for printf/format/randomblob, one
level up. Add them to the scalar blocklist.
- Prose number-gate missed трилион/билион/квадрилион: '3 трилиона лева' slipped
  the whole gate (the digit can't reach 'лева' across the Cyrillic word), an
  unbound order-up figure on a public report — the '12 млрд.' vector one
  magnitude higher. Add them to the spelled-magnitude stem.
- Validate the optional column align against a left|right whitelist, and build
  resolved table columns explicitly instead of spreading the model object, so no
  unknown/unvalidated property reaches the renderer.
- Cap model-emitted array lengths (blocks, items, columns) in validateEmitShape.
…h prompt paths

renderTraps() now owns the numbered-list rendering that describeSchema (full
dictionary) and the RAG hard-traps block duplicated, so the two paths cannot
drift, and the full-dictionary heading is harmonised to match the RAG block
("Задължителни правила за данните"). No behaviour change — string assembly only.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Преглед на PR: fix(assistant): integrity, grounding & guard fixes

Обобщение

PR-ът е фокусиран follow-up от прегледа на #79 и затяга няколко защитни слоя в асистента без разширяване на обхвата. Промените са атомарни, добре мотивирани в коментарите и всяка е придружена от смислени тестове (не тривиални „cheater" тестове).

Фаза 0 — Сигурност (ЗАДЪЛЖИТЕЛНА проверка): ✅ ЧИСТО

  • Няма хардкоднати тайни, ключове, пароли или токени.
  • Няма нови/променени URL адреси, нито промени в зависимостите.
  • Няма злонамерени шаблони (backdoor, code injection, обфускация).
  • Всички промени всъщност засилват защитата: разширен guard за SQL агрегати, capping на масивите от модела, whitelist за align, floor за релевантност при RAG.

По измерения

Сигурност (agent-level): 1.0/1.0

  • sql-guard.ts: добавени group_concat/json_group_array/json_group_object към блокираните функции — коректно затваря клас memory-amplification (една клетка от пълно сканиране преди capRows). Регексът изисква \s*\(, така че колона с такова име не се блокира по грешка.
  • report-schema.ts: явното изграждане на колоните вместо { ...c } премахва passthrough на непознати свойства към рендера — добра defensive промяна.
  • emit-report-schema.ts: whitelist isAlign (само left|right|undefined) предотвратява попадане на out-of-enum стойност в атрибут/style.

Тестове: 3.0/3.0

  • Всяка нова пътека има целеви тест: capping на масиви, align whitelist, floor при RAG (над/под прага и празен резултат), spelled трилион/билион, string-building агрегати, безусловни hard-traps под RAG. Тестовете са проектирани да разкриват дефекти, не да минават формално.

Качество на кода: 2.0/2.0

  • Извеждането на renderTraps() премахва дублиране между describeSchema и system-prompt (single source of truth) — точно според „NO CODE DUPLICATION".
  • Стилът е консистентен с останалата част на модула.

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

  • Няма регресии; caps-овете (MAX_BLOCKS/ITEMS/COLUMNS) са безопасни горни граници далеч над реален отчет.

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

  • Коментарите обясняват „защо" на всяка граница; актуализирани доки за таблиците amendments/parties и коригиран grain на data_freshness (view→таблица).

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

Без частични имплементации, без TODO/dead code, без смесени концерни, без ресурсни течове. Промените са атомарни и в рамките на обхвата.

Заключение

Висококачествен, добре тестван защитен PR. Нямам блокиращи забележки. Оставям два незадължителни инлайн коментара за проверка (grounding към home_totals.suspect и робъстност при липсваща score), които не блокират сливането.

Композитна оценка: 9.5/10 — препоръка: одобрение с малки уточнения.

Comment thread apps/web/app/lib/assistant/describe-schema.ts Outdated
Comment thread apps/web/app/lib/assistant/rag.ts Outdated
retrieveSchemaContext relied on `m.score` always being numeric. If an index
backend ever returns a match without a `score`, the comparison was falsy and the
chunk was dropped — the correct, safe outcome, but only incidentally. Make it
explicit with `(m.score ?? 0) >= minScore` and a comment so a future refactor
can't strip the guard, and cover it with a test. Addresses the review note on
rag.ts robustness (ydimitrof).

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Преглед на PR: fix(assistant): integrity, grounding & guard fixes (follow-up на PR #79)

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

Фаза 0 — Скенер за сигурност (задължителен, изпълнен пръв)

  • Твърдо кодирани тайни: няма (0 API ключа/пароли/токени). ✅
  • Промени по URL адреси: няма нови/променени URL адреси. ✅
  • Зловредни модели (backdoor / инжекция на код / обфускация): няма. ✅
  • Промени по зависимости: няма нови пакети. ✅
  • Заключение на Фаза 0: ЧИСТО → продължавам към същинския преглед.

Важно: този PR е серия от защитни (hardening) поправки — той затяга SQL guard-а, ограничава размерите на масивите от модела, въвежда праг за релевантност при RAG и разширява детектора на изписани числа. Т.е. промените намаляват риска, а не го увеличават.

Анализ по файлове

sql-guard.ts — Добавени са group_concat / json_group_array / json_group_object към черния списък. Правилно: това са агрегати, които колабират цял table scan в една огромна клетка, преди capRows да я измери (memory amplification, същият клас като printf). Регулярният израз е коректен (\b…\s*\(); коментарите се премахват преди проверката, така че group_concat/*x*/( няма да заобиколи гарда. Fail-closed — правилен избор. (Виж инлайн бележка за остатъчен вектор с рекурсивни CTE.)

emit-report-schema.ts — Въведени горни граници MAX_BLOCKS=100, MAX_ITEMS=50, MAX_COLUMNS=50 и whitelist валидатор isAlign (само undefined|left|right). Затваря DoS през неограничена дължина на масив и предотвратява out-of-enum стойност за align да достигне рендерер, който я интерполира в атрибут/стил (защита срещу attribute/HTML injection — OWASP A03). Тестовете покриват center, "><b> и препълнените масиви. ✅

report-schema.ts — Добавени трилион|билион|квадрилион към стема за изписани величини. Коректно затваря пропуска „3 трилиона лева“ (дефамационен вектор един порядък над „12 млрд.“). Преминаването към явно построяване на колоните (без spread { ...c }) е важно подобрение: спира пренасянето на непознати, подадени от модела свойства към рендерера, тъй като validateEmitShape не отхвърля непознати ключове. Много добра защита в дълбочина. ✅

rag.ts — Въведен праг за релевантност MIN_SCHEMA_SCORE=0.35 с (m.score ?? 0). Логиката е правилна: под прага се връщат по-малко или нула чънкове, а нула кара buildSystemPrompt да падне обратно към пълния статичен речник — безопасният изход. ?? 0 защитава срещу backend, който пропуска score (не се инжектира като „контекст“ без ранг). ✅

system-prompt.ts / describe-schema.ts — Твърдите MUST/NEVER капани (SUM само amount_eur, ocid ≠ УНП, …) вече се инжектират безусловно чрез споделения renderTraps(), дори при RAG. Това коригира реалния дефект, при който извличането е заменяло капаните и е оставяло RAG-хода с по-малко ограничения от no-RAG отстъплението (пропускът, довел до SUM(amount)). Споделеният рендерер премахва риска от дрейф между двата пътя. Актуализирани са и enum-ите (value_flag + value_low) и документацията на таблиците amendments/parties. ✅

OWASP съответствие

  • A03 (Injection): SQL guard е fail-closed и SELECT-only; изписаните числа и подаваните от модела полета се санитизират/валидират. Няма конкатенация на непроверен вход в SQL в този diff.
  • A04 (Insecure Design) / DoS: новите горни граници и блокирането на string-агрегатите адресират точно ресурсното изчерпване.
  • Няма нови вектори за A01/A02/A05/A08.

Тестове и качество

Всяка промяна е придружена от смислен, целенасочен тест (align whitelist, лимити на масивите, RAG праг, трилион/билион, SQL агрегати, безусловни капани). Тестовете са проектирани да разкриват дефекти, не да минават тривиално. Именуването и стилът са консистентни с останалата кодова база. Няма частична имплементация, TODO-та, дублиран или мъртъв код.

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

  1. sql-guard.ts — Остатъчен вектор: рекурсивен CTE, който строи низ чрез || (напр. WITH RECURSIVE r(s) AS (SELECT 'x' UNION ALL SELECT s||'x' FROM r LIMIT 1e6) SELECT s), може да материализира огромен низ, без нито една от блокираните функции. Извън обхвата на този PR (съществуващ пропуск), но си струва отделен follow-up.
  2. report-schema.ts — Стемата покрива до квадрилион, но не и квинтилион/секстилион. Отворен край с ниска вероятност; евентуално по-общ шаблон в бъдеще.
  3. describe-schema.ts — Grain на data_freshness е сменен от „view“ на „таблица“. Моля потвърдете, че обектът реално е таблица, а не изглед — това е документация, която моделът чете при писане на SQL.

Обща оценка: висококачествен, добре тестван защитен PR. Препоръчвам одобрение след вашата ръчна проверка.

Comment thread apps/web/app/lib/assistant/sql-guard.ts Outdated
Comment thread apps/web/app/lib/assistant/report-schema.ts Outdated
Comment thread apps/web/app/lib/assistant/report-schema.ts
Comment thread apps/web/app/lib/assistant/emit-report-schema.ts
Comment thread apps/web/app/lib/assistant/rag.ts
…vers квинтилион+)

The prose-number gate listed magnitudes explicitly and stopped at квадрилион, so
"3 квинтилиона лева" slipped. Match the shared suffixes instead — милион⊃"илион",
милиард⊃"илиард" — which covers the whole family (милион…секстилион…, милиард…)
and closes the row upward for good rather than chasing an endless list. Addresses
the review note on report-schema.ts (ydimitrof).

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ревю на PR: fix(assistant): integrity, grounding & guard fixes from the PR #79 review

Здравей, Явор! Прегледах промените изключително подробно, с фокус върху сигурност и цялост на данните (SQL инжекции, обход на guard-а, XSS през рендер, изтичане на ресурси). Ето обобщението.

ВЕРДИКТ: COMMENT — не блокиращо. Одобрявам след потвърждение по единствената бележка по-долу (string_agg). Не съм публикувал коментари — това е чернова за твоя преглед.

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

  • Твърдо кодирани тайни (API ключове/пароли/токени): няма.
  • Промени по URL/крайни точки: няма.
  • Нови/променени зависимости: няма.
  • Зловредни шаблони (бекдор, инжекция на код, обфускация): няма. Всяка промяна затяга съществуваща защита, а не я отслабва.
  • Резултат: CLEAN — продължавам към прегледа по същество.

Оценка по файлове

  • sql-guard.ts — Добавени group_concat / json_group_array / json_group_object към забранените функции. Правилно — това е същият клас усилване на паметта като printf/format (цял скан се събира в една клетка преди capRows). Виж единствената бележка за string_agg.
  • report-schema.ts — (1) Регексът за изписани величини е сменен от точен списък (милиард|милион|хиляд) на суфиксите (илион|илиард|хиляд), което затваря дупката „3 квинтилиона лева" нагоре без регресия — тестовете го потвърждават, старите величини минават през суфиксите. (2) Колоните вече се строят с явни полета вместо spread — това спира пренасянето на непознати, подадени от модела свойства към рендера (validateEmitShape не отхвърля непознати ключове). Много добра защита.
  • emit-report-schema.ts — Тавани MAX_BLOCKS/ITEMS/COLUMNS и whitelist за align (left|right). align валидирането спира атрибут/style инжекция при рендер (тестът с '"><b>' го покрива). Капацитетите провалят валидацията, така че скъпият път в bindReport не се изпълнява — коректно.
  • rag.ts — Праг за релевантност MIN_SCHEMA_SCORE = 0.35 с ?? 0 защита за липсващ score. Правилна посока: под прага → падане към пълния речник (по-силна заземеност от частичен RAG). Тестовете покриват и трите случая.
  • system-prompt.ts / describe-schema.ts — Твърдите MUST/NEVER капани се инжектират безусловно и под RAG чрез споделен renderTraps(), така че двата пътя не могат да се разминат. Затваря първопричината (RAG turn с по-малко ограничения от no-RAG, който пусна SUM(amount)). Обновените описания на amendments/parties/data_freshness и разграничението amount_eur IS NULLvalue_suspect са коректни пояснения (само документация в промпта).

OWASP / CLAUDE.md

  • Няма инжекция (SQL guard е allowlist за SELECT + денилист за опасни функции; рендерът получава само whitelisted align). Валидацията на вход е засилена. Няма изтичане на ресурси. Няма мъртъв код, дублиране или частична имплементация. Наименованията са консистентни. Тестовете са смислени (проверяват отхвърляне, не тривиално минаване).

Единствена бележка (не блокираща)

Виж inline коментара в sql-guard.ts за string_agg. Понеже подходът е денилист, той е по природа непълен — заслужава да се обмисли allowlist на агрегатните функции в бъдеще.

Много добра, дисциплинирана работа по затягане на цялостта и заземеността. 👍

Comment thread apps/web/app/lib/assistant/sql-guard.ts Outdated
… the SQL guard

string_agg(X, sep) is the official SQLite 3.44 synonym of group_concat and reaches
the same code path on D1's modern SQLite, so it bypassed the scalar/aggregate
denylist and achieved the same memory amplification (whole scan into one cell
before capRows) the guard just closed for group_concat. Add it to the regex and
the adversarial test. Addresses the review note on sql-guard.ts (ydimitrof).

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ревю на PR: fix(assistant): integrity, grounding & guard fixes from the PR #79 review

ВЕРДИКТ: COMMENT — одобрим по същество, без установени блокери; финално одобрение след зелена CI (тестове + покритие). Не съм публикувал inline коментари по твое изрично искане.


Обобщение

PR-ът е серия защитни (security-hardening) корекции по AI асистента като follow-up на ревюто на PR #79. Промените са атомарни, добре мотивирани в коментарите и придружени с целеви тестове. Не открих зловреден код, задни вратички (backdoors), обфускация, инжекции или изтичане на тайни. Промяната стеснява повърхността на атака — не я разширява.

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

  • Твърдо кодирани тайни (secrets/API ключове/пароли): няма.
  • Промени в URL/домейни: няма.
  • Зловредни шаблони (backdoor/eval/обфускация/code injection): няма.
  • Нови зависимости: няма промени в package.json/lockfile — нулев риск от dependency-supply-chain.
  • SQL експлойти: промяната всъщност засилва SQL пазача (виж по-долу).

Преглед по файлове

sql-guard.ts — Добавени в denylist-а group_concat, string_agg (SQLite ≥3.44 синоним, релевантен за D1), json_group_array, json_group_object. Това затваря клас „memory-amplification" — string-building агрегати, които сгъват цял table scan в една огромна клетка, материализирана преди capRows да я измери (потенциален OOM на изолата). Проверих локално: пазачът блокира изброените функции и коректно пропуска легитимни sum(amount_eur) / concat(...). Забележка: това остава denylist (игра на догонване); коментарът честно признава, че устойчивото решение е позитивен allowlist — приемам го като tracked-separately.

report-schema.ts — (1) Регексът за „изписани с думи" величини мина от изричен списък (милиард|милион|хиляд) към суфиксите илион|илиард|хиляд, което затваря скалата нагоре (трилион/квадрилион/квинтилион/секстилион…). Потвърдих локално, че „3 трилиона лева", „три квинтилиона" и старите случаи се флагват; „Илион" (Троя) се over-флагва — приемливо, тъй като гейтът трябва да fail-toward-flagging. (2) Изричното изграждане на колоните (без { ...c } spread) е правилно — пречи неизвестни, подадени от модела полета да достигнат до renderer-а. Добра защита в дълбочина.

emit-report-schema.ts — Горни граници MAX_BLOCKS=100 / MAX_ITEMS=50 / MAX_COLUMNS=50 върху дължините на масивите (преди беше ограничен само размерът в байтове на редовете, не броят елементи) + isAlign whitelist (undefined|left|right). Проверих renderer-а (ReportBlockRenderer.tsx:180): align се сравнява с 'right' и се мапва към контролиран клас 'num', т.е. не се интерполира сурово в атрибут/стил — whitelist-ът е коректна защита в дълбочина. Границите са доста над реален отчет — без функционален риск.

rag.ts — Въведен праг MIN_SCHEMA_SCORE=0.35 за релевантност. Логиката е правилна: под прага се връщат по-малко/нула чънкове, а нула кара buildSystemPrompt да падне обратно към пълния статичен речник — т.е. по-безопасният изход. (m.score ?? 0) е коректна защита срещу backend без score (чете се като „под прага", отпада). Покрито с тестове (включително случая без score).

system-prompt.ts / describe-schema.ts — Твърдите MUST/NEVER капани (SUM само amount_eur, ocid≠УНП, …) вече се инжектират безусловно, дори при RAG. Това коригира реален integrity-дефект: RAG turn можеше да остане с по-малко ограничения от no-RAG fallback-а (пътят, който пропускаше SUM(amount)). renderTraps() е споделен между двата пътя — премахва дрифт. Актуализациите на речника (value_low в enum-а, колони на amendments/parties, data_freshness като таблица) са документационни и консистентни.

Тестове

Всеки промяна носи целеви тестове (align whitelist, cap-ове на масиви, relevance floor вкл. missing-score, суфиксите на величините, string-building агрегати, безусловни hard-traps). Тестовете изглеждат смислени (не тривиални). Не мога да потвърдя точен процент покритие/100% pass от diff-а — това остава за CI.

OWASP

  • A03 Injection — SQL пазачът е усилен; prose-gate-ът и sanitizeProse ограничават материални числа в публичния отчет. ОК.
  • A04 Insecure Design / DoS — cap-овете на масивите и блокирането на amplification-агрегати адресират ресурсно изчерпване. ОК.
  • A05 Misconfiguration — без промени в конфигурация/тайни. ОК.
  • Няма разширяване на повърхността на атака.

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

  1. sql-guard: приоритизирайте позитивния function-allowlist (вече споменат в коментара) — denylist-ите ще изостават от нови алиаси.
  2. prose-regex: over-flag на „Илион"/„милиони" контексти може да увеличи retry-та на модела; приемливо, но си струва да се следи като UX сигнал.
  3. Уверете се, че validateEmitShape винаги предхожда bindReport в реалния път (иначе align разчита само на renderer-а — който все пак е безопасен).

Заключение: качествен, добре тестван защитен PR. Препоръчвам сливане след зелена CI.

@nedda76

nedda76 commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Благодаря за подробното ревю! Кратко по трите незадължителни препоръки:

  1. Позитивен function-allowlist за SQL пазача — съгласна, денилистът е игра на догонване. Проследено е в run_sql: durable memory-amplification защита (allowlist на функции + cell-size cap преди capRows) #227 (позитивен allowlist + таван на размера на клетката преди capRows). L2 на feat(ai-assistant): conversational analytic layer (BgGPT) #79 вече има такъв allowlist, така че при мърджа на feat(ai-assistant): conversational analytic layer (BgGPT) #79 в main тази посока се затваря по-устойчиво и L1 остава cheap backstop.

  2. Over-flag на „Илион"/„милиони" контексти — приемам го като съзнателен fail-toward-flagging компромис: по-добре един retry, отколкото необвързано число на публичен отчет. Ще го следя като UX сигнал; ако retry-тата пораснат, това е още един довод за по-структурирания подход от run_sql: durable memory-amplification защита (allowlist на функции + cell-size cap преди capRows) #227.

  3. validateEmitShape преди bindReport — потвърждавам за реалния път: finalizeReport (tools.ts:222) валидира shape-а, връща рано при грешка (:224) и достига bindReport само при валиден вход (:227). Освен това bindReport е независимо defensive (изричното изграждане на колоните + коментарът на report-schema.ts:380 „Don't rely on validateEmitShape running first"), така че align не разчита единствено на renderer-а.

Локално тестовете (app/lib/assistant), typecheck и prettier минават; чакам зелена CI за финалното одобрение.

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Прегледах стриктно на връх 038d4e3 — силна работа, guard-овете издържаха на доста red-team. Проверих (и всички отхвърлени): stacked ; вътре в '…', CTE кръстен като blocked table, recursive CTE без RECURSIVE, pragma_table_info(...) TVF, json_each/generate_series, tautological JOIN ON 1=1, LIMIT offset,count / отрицателен / 1e9, load_extension/group_concat/string_agg, numeric-entity double-encode, markdown-split число, homoglyph цифри, prompt injection през RAG контекста. Двуслойният guard (L1 текст + L2 AST) е издържан; bindReport explicit-field reconstruction и безусловният hardTraps() (fix за grounding-регресията) са точно правилните решения.

Едно дребно (ново в PR-а, hardening):

  • apps/web/app/lib/assistant/emit-report-schema.ts (~ред 68) — validateEmitShape не прави early-return след MAX_BLOCKS проверката, тъй че обхожда целия (model-emitted) масив. finalizeReport gate-ва bindReport зад shape.ok, тъй че няма downstream щета — но добавете return { ok:false, errors } веднага след push-а (както за !Array.isArray по-горе), също и за MAX_ITEMS.

Прерогатив за прод (не са scope на този PR, вече tracked — само маркирам): || string-concat memory-amplification (#227) и cost-ът на първата run_sql заявка (D1 таксува scanned rows, #122). Струва си да са hard-prerequisite за provision, не просто roadmap.

Одобрявам.

nedda76 added 4 commits July 22, 2026 11:34
An over-cap blocks/items/columns array is exactly the unbounded structure
the ceilings guard against, yet validateEmitShape recorded the length error
and then walked the whole array anyway — doing the very scan the cap exists
to refuse. Return before the per-block scan on oversized blocks, and skip the
per-element scan on oversized items/columns. Behaviour is unchanged for valid
reports (ok:false either way); this only stops the wasted walk. Test asserts a
single cap error with no per-element errors, proving the array is not scanned.

Addresses lyubomir-bozhinov's review note on PR midt-bg#223.
The Dependency-audit step (osv-scanner) fails on ANY known vuln and, per its
own comment, expects intentional exceptions in osv-scanner.toml — which did not
exist yet. sharp@0.34.5 (High, GHSA-f88m-g3jw-g9cj: inherited libvips decoder
CVEs) has no in-range upstream fix: miniflare pins sharp ^0.34.5 and its latest
release still ships 0.34.5, so 0.35.0 is unreachable without a forced override.
sharp is a transitive dev-only dep (miniflare dev server / test runtime), absent
from the deployed Worker, and the vuln needs decoding an untrusted image the
toolchain never handles. Record a dated (ignoreUntil 2026-10-22) exception so
the audit gate goes green and the entry auto-resurfaces for revisit.

Verified locally with osv-scanner 2.4.0: fails without the config, passes with.
…ty-grounding

# Conflicts:
#	apps/web/app/lib/assistant/describe-schema.ts
#	osv-scanner.toml
@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Благодаря за red-team прегледа! По бележката за validateEmitShape: адресирана е в ab2e247 — ранен return { ok:false, errors } веднага след MAX_BLOCKS проверката, плюс пропускане на per-element скана и при over-cap items/columns (същият принцип, приложен и на трите тавана). Тестът твърди точно един cap error без per-element грешки — доказва, че масивът не се обхожда.

Клонът е обновен и с актуалния main (merge, без пренаписване на историята) — конфликтите в describe-schema.ts (речникът се разви на main; запазих уникалния trap „непотвърдени → home_totals.suspect") и osv-scanner.toml (запазени са и двете изключения) са разрешени. Локално: 491/491 web теста, typecheck 7/7, prettier чист. PR-ът вече е MERGEABLE.

@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

@ydimitrof @lyubomir-bozhinov @todorkolev един бърз преглед моля.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants