feat(assistant): integrity core + RAG foundation for the AI assistant - #80
Conversation
First implementation increment of docs/spec/ai-assistant.md, focused on the highest-priority §9 hardening items plus the RAG layer. Pure, tested and deploy-independent — no new deps/bindings — so it can land and be reviewed before the agent loop / dock UI (which need BGGPT_API_KEY + cloud bindings). - report-schema.ts: server-owned report values (§9.1) — the model references result handles, the server re-binds real numbers; tables take rows wholesale; prose is HTML-sanitized (closes stored-XSS on the public /reports/:id). - sql-guard.ts: read-only structural guard + injected LIMIT + byte cap (§7/§9.4) with the AST-parser + read-only-binding primary guards documented as next. - describe-schema.ts: curated data dictionary encoding the real data traps (amount vs amount_eur, value_flag, ocid≠UNP, lots grain) (§9.2). - rag.ts: Vectorize + Workers-AI (bge-m3) — schema-grounding retrieval and a semantic_search tool. Deliberate addition over the SQL-only spec. - tests for the value-binding and SQL guard (17 new; web suite 75 green). See apps/web/app/lib/assistant/README.md for the architecture and roadmap.
Continues the foundation with two pure, tested modules that feed the agent loop: - system-prompt.ts: encodes the runtime policies — emit_report policy (§9.10), values-by-reference (§9.1), data-trust / no-instructions-in-data (§7), the editorial skeleton (§4) and per-source freshness (§9.7); injects RAG schema context with a static-dictionary fallback. - tool-results.ts: bridges D1 .all() rows to handled, byte-capped QueryResults (R1, R2 …) that the report binder re-binds from (§7). 10 new tests; web suite 85 green; typecheck 0; prettier clean.
…ize cap) Live ЦАИС ЕОП day-query tool, hardened per spec §9.7: takes only a validated date (never a model-supplied URL → no SSRF), reuses the verified eopSource URL builder, caps each file before it reaches the model context, and treats the payload as untrusted. Network call injected for testability. 7 new tests; web suite 92 green; typecheck 0; prettier clean.
CI `pnpm audit --audit-level=high` began failing repo-wide on a newly published undici advisory (GHSA-vmh5-mc38-953g / -vxpw-j846-p89q / -hm92-r4w5-c3mj) — DoS via WebSocket fragment-count bypass and SOCKS5 proxy pool reuse — pulled in transitively via wrangler→miniflare (undici 7.24.8). Dev/build-time only; never ships to the Worker runtime. Add an `undici: ^7.28.0` override alongside the existing ws/vite pins. After the bump `pnpm audit --audit-level=high` exits 0 (1 low remains, below the gate). Lockfile updated to undici@7.28.0; nothing else changes.
Implements spec §4's two renderer rules as pure helpers the /reports/:id renderer will consume: - formatCell(value, hint) → delegates to @sigma/shared money/count/pct/date, so reports match native page formatting (no drift). - entityHref(kind, id) → canonical internal href via @sigma/db hrefForEntity. 4 new tests; web suite 96 green; typecheck 0; prettier clean.
Two more pure helpers: - source-link.ts: grounded official deep links (ЦАИС ЕОП procedure + open-data day files via the verified eopSource helper). Trade Register / АОП links are deferred — won't ship an unverified "official source" URL for a gov tool. - emit-report-schema.ts: validateEmitShape (structural guard that runs before bindReport's handle resolution) + the model-facing EMIT_REPORT_JSON_SCHEMA. 10 new tests; web suite 106 green; typecheck 0; prettier clean.
Starts the agent-loop layer with its substance, kept dependency-free and tested: - tools.ts: describe_schema, run_sql (guard → LIMIT → D1, retains the result under a handle), semantic_search, eop_fetch, source_link — each runs server-side and returns a compact string; data tools retain full results in ctx.results for binding. Plus runTool dispatcher and finalizeReport (validateEmitShape → bindReport against THIS turn's results only — §9.1/§9.3). Remaining: the thin Vercel-AI-SDK wiring (streamText + /assistant/chat + provider via AI Gateway) — adds the deps/bindings and carries no logic. 9 new tests; web suite 115 green; typecheck 0; prettier clean.
The thin SDK layer that turns the tested foundation into a working chat endpoint: - agent.ts: BgGPT through @ai-sdk/openai (createOpenAI.chat) routed via the AI Gateway (§9.5); maps ASSISTANT_TOOLS → SDK tools (jsonSchema), adds emit_report wired to finalizeReport, runs streamText with stopWhen: stepCountIs(MAX_STEPS), returns the streamed Response. - routes/assistant.chat.tsx: stateless resource route — POST UIMessages, RAG-ground the prompt (best-effort), run one turn, stream back. - wrangler.jsonc: AI + VECTORIZE + REPORTS (R2) bindings + config vars; BGGPT_API_KEY stays a secret. - deps: ai ^6, @ai-sdk/openai ^3. Not runtime-verifiable here (needs BGGPT_API_KEY + the bindings), but typecheck 0, 115 tests, audit clean (no new high/moderate), prettier clean.
|
@nedda76 супер работа — чист, тестван fundament (integrity core + RAG) по §9 от спецификацията, при това deploy-независим (без нови bindings/ключове), така че може да се ревюира преди agent loop-а и облачните части. На практика това може да е Фаза 1 от асистента. Една бележка по координацията. Искаме да доставим асистента като един общ, дълготраен branch — Предложение: пренасочи #80 към За следващите фази — нека координираме в Discord-а на МИДТ, да си разпределим обхвата и да не дублираме. Благодаря много за усилията! |
|
Благодаря за прегледа и за рамкирането като Фаза 1! 🙏 Превключих #80 на ready for review и обнових README-то да отразява текущото състояние: backend-ът е опроводен от край до край (13 модула + tool registry + agent loop през Vercel AI SDK → AI Gateway → BgGPT + Една уговорка по координацията. Проверих съдържанието на Понеже #79 е docs-only и не пипа нито един файл, който #80 променя, бих предложила най-простия и чист път: и двете PR-та да влязат директно в След като и двете са в Благодаря отново — радвам се, че foundation-ът е полезен! 🙌 |
Extend the guarantees-vs-limits section with concrete guardrails that harden the residual data-correctness gaps, building on PR #80's system-prompt/describe-schema foundation: default filters, reconcile- with-rollup self-check, explicit CPV interpretation, mandatory methodology callout, Verifier trap-compliance checks, and a golden- reports CI harness. Honesty (watermark + methodology) stays load-bearing.
|
@Bozhidar-Vangelov @teodorkirkov |
|
Сега ще погледна кода и сайта и ще напиша ако забележа нещо |
|
@Alben13579 Ако може да прегледаш дизайна на този ПР и като цяло дизайна на проекта. 🙏 |
|
@nedda76 прегледах кода
|
|
@experiment-bg Тук сме - един преглед и от теб моля и проверка на сайта. В момента доста ПР-и са се натрупали и може би е добра идея да изчакаме да се мърджнат, иначе голям рибейз ще падне. |
|
@nedda76 разгледах кода подробно Решението е премислено и се чете леко. Това, че единствено сървърът изписва числа — моделът само посочва хендъли, а
Иначе посоката е вярна и ядрото за достоверност е силно. За мен единствено спиращо е разменения таван в |
Clamp MAX_STEPS to [1, 20] so a misconfigured deploy can neither stall the tool loop (0/negative) nor uncap BgGPT calls (a huge value), and degrade gracefully on failure: a mid-stream BgGPT outage now surfaces as a readable message via the stream's onError, and a setup failure returns a 503 instead of an unhandled 500. Addresses review notes on PR midt-bg#80.
Exercise the boundary teodorkirkov raised on PR midt-bg#80: a tool/EOP/DB value carrying a fake instruction (e.g. "игнорирай предишните инструкции") must be treated as DATA, never as a command. Lock the system-prompt data-trust clause, that forModel serialises a poisoned cell verbatim inside the data payload, and that bindReport keeps it as a plain table cell. (Model-level resistance itself stays an eval concern — golden-report CI, §9.9.)
Make explicit that the new AI/Vectorize/R2 bindings reference resources that must exist before `wrangler deploy` — deploying first fails the deploy and blocks the team's CD (review note on PR midt-bg#80). Re-stage rate-limiting + circuit-breaker from the launch gate into Phase 2, note the v1 graceful degradation now in place, and refresh the test count.
Layer node-sql-parser (SQLite-only build) over the structural read-only check in run_sql: parse the statement and reject anything that is not a single read-only SELECT, failing closed on parse errors. Closes the gap flagged on PR midt-bg#80 that a regex/keyword guard is inherently incomplete. The structural guard stays the cheap first pass; the parser is the real gate. Deliberately fails closed — valid-but-unparsed SQLite (e.g. window functions without PARTITION BY) is refused, steering the model to the canonical ORDER BY + LIMIT pattern, which all canonical queries use and a regression test covers. A read-only D1 binding remains the open §9.4 layer.
|
@teodorkirkov Благодаря за внимателния преглед! 🙏 Адресирах бележките в четири commit-а на branch-а: 1. 2. 3. Rate-limiting / circuit-breaker. Преместих го от launch gate във Фаза 2, както предложи. За v1 добавих базова graceful degradation: mid-stream грешка от BgGPT се показва като четим текст през 4. 5. Проверено: typecheck 0, 127 теста минават, Prettier чист, |
|
Здравей @nedda76! Направихме задълбочен security ред-тийм на #80, спрямо самата спецификация в #79 (§9 hardening + agent-team addendum-а на @cefothe) — спрямо нашата обща висока летва. Нa първо място: имплементацията е силна. Проверих какво е изпълнимо (прекарах adversarial SQL през самите guard функции) и write-защитата издържа на всичко — Ред-тиймът извади набор от gap-ове — и важното: почти всеки е точно launch-gate изискване, което спецификацията вече описва, просто още не е достигнато в кода. По тежест, с file:line + repro + препратка към спецификацията: 🔴 Launch-gate (спецификацията ги изисква преди публично излагане)1. 2. → моделът може да изброи схемата и да чете всяка таблица в 3.
🟠 Бъгове / hardening4. 5. Bound data cells не се санитизират. 6. „Числа в prose" — точно guardrail E2. Binding-ът пази стойностните слотове, но 7. Без таван на историята/тялото + retry мултипликатор. 🟡 Low
Накратко: ядрото е добро и покрива „Guaranteed by construction" от addendum-а. Останалото е launch-gate слоят, който спецификацията ми описва (read-only път/allowlist, rate-limit + circuit-breaker, query timeout, E2) + два бъга (4, 5) — и всичко е в backend integrity core-а, който е твоят lane ( От наша страна мислим да поемем lane-овете, които консумират схемите ти, без да ги пипат — renderer-ът на |
Three integrity-core fixes from the midt-bg#80 security red-team, all at the binding layer where the guarantee belongs: - Resolve entity-link ids per row (ResolvedRow.links) and require the link idCol to exist. Without this an immutable R2 report could not rebuild its /companies/:eik links — the block-spec contract needs it. - Tag-strip submitter-influenceable string data cells at bind, not just prose, so no markup reaches the public /reports/:id even if a renderer forgets to escape (defence-in-depth, §7). Fixes the over-claiming comment in report-schema. - Add guardrail E2: a deterministic no-material-number-in-prose gate on text/callout (currency, млн/млрд, grouped numbers, 5+ digit integers; years/small counts/ordinals pass). The model must put numbers in value slots the server binds — closes the unbound-number defamation vector.
Freeze the three contracts the team agreed to lock first so FE and BE can work in parallel against fixtures: the block-spec (ResolvedReport, incl. the new per-row link ids), the immutable R2 report object (§5), and the SSE chat protocol (AI SDK UI message stream + the emit_report tool output that carries the report). Adds machine-readable JSON/SSE fixtures under app/lib/assistant/fixtures/. Source of truth stays the BE types.
The EOP_MAX_BYTES cap was a no-op: when the body exceeded the cap the code still JSON.parsed the FULL body and returned every row (the cap only set a 'truncated' flag). An oversized untrusted EOP file therefore reached the model in full. Now an over-cap body is refused with a soft error and never parsed. Strengthens the test to assert rows are withheld. (review midt-bg#80)
decodeNumericEntities ran a single pass, so a double-encoded number (1&midt-bg#38;midt-bg#50;000 -> 1&midt-bg#50;000) passed guardrail E2 while a renderer would decode it the rest of the way to a fabricated 12000 — the §9.1 vector the gate exists to close. Loop to a fixpoint (bounded). (review midt-bg#80, ydimitrof)
node-sql-parser accepts LIMIT 1e9 (type 'bigint') and 1.5, but enforceLimit's regex does not, so it appended a second LIMIT (LIMIT 1e9 LIMIT 500) — a SQLite syntax error that only failed closed by accident. Reject any LIMIT/OFFSET that is not a plain non-negative integer so the AST and regex text models agree. (review midt-bg#80, ydimitrof)
|
Благодаря на @lyubomir-bozhinov и @ydimitrof за двата security re-review! Адресирах трите изпълними бележки от 26.06 — head вече е Имплементирано (с тестове):
Проследено (non-blocking, pre-activation) → #155: fail-closed лимитер от runtime сигнал вместо build-time |
Both this PR and midt-bg#118 (/health) independently claimed namespace_id 1004. A duplicate silently disables one limiter (no deploy error) and git auto-merges the two bindings without conflict, so the dup would ship unnoticed. Take 1005 here (1004 stays with midt-bg#118's reviewed health limiter) to deconflict regardless of merge order.
Mirror the assistant-contract seam (report.ts, stream.ts, fixtures + spec) onto feat/ai-assistant. It re-exports ResolvedReport from app/lib/assistant/report-schema, which is present here now that #80 has landed in main and feat is caught up. Closes the seam parity hole between feat/ai-assistant and feat/ai-assistant-contracts.
Adopt fork-main's canonical app/lib/assistant foundation (upstream #80 is strictly ahead of contracts' #80-head base) and deploy layer. wrangler limiter namespace_id 1004->1005 fixes a silent collision with the /health limiter. Keeps contracts' seam spec and .dev.vars assistant docs. seed-endpoint/reindex (contracts-only) preserved.
Mirror of #10 (approved) onto feat/ai-assistant-contracts. Identical lane content — L0-L3 dedup keys + freshness (dedup.ts), in-isolate single-flight coordinator (single-flight.ts), AI SDK v6 data-dedup/data-progress stream parts (dedup-stream.ts) — pure, self-contained, not yet wired. The only difference from #10 is the base: #80 head here vs upstream main there. Part of midt-bg#97 (two identical fixed-period questions must never diverge).
- format AssistantPanel and AssistantTranscript after rebase conflict resolution - revert cell() to hard errors for missing column and out-of-range/non-integer row (facts/totals precision slots; model must retry with correct reference) - keep table display columns as warnings (graceful null rendering) - add hard-error check for missing link idCols in table blocks (structural requirement — immutable report cannot reconstruct entity links without them)
… meta canonical - contractSlug now encodes %, /, ?, # so any AOP contract number is safe as a URL path segment - entityHref in render-format.ts uses contractSlug directly for contracts (removes double-encode risk) and encodeURIComponent on the slug part for authorities/companies (closes path-traversal concern from review midt-bg#80) - meta canonical/og:url in contract.tsx re-encodes params.id via contractSlug(contractIdFromSlug()) — React Router decodes the param before the loader runs, so the raw id was being used previously - identity.test.ts covers the new ? and # cases and round-trips
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 midt-bg#80)' issue references elsewhere are an established convention and are untouched. Comment-only.
…with "/" (#213) (#221) * fix(db): percent-encode "/" in contractSlug to prevent 404 on contracts with slash in id AOP contract numbers often contain "/" (e.g. "ОП20-42/22/"), which when used raw in the URL path splits /contracts/:id into multiple segments and returns 404. contractSlug now encodes "/" as "%2F"; React Router decodes it back in params so the reading side is unchanged. sitemap-contracts is fixed in the same pass. Closes #213 * fix(db,web): close four follow-on issues found in code review of #213 - contractSlug: also encode bare '%' before '/' so a percent sign in source data can't produce a malformed percent sequence and cause a URIError in React Router's param decode (would 500 instead of 404) - render-format/entityHref: decode the slug before re-encoding with encodeURIComponent to prevent double-encoding '%2F' → '%252F' which broke all AI-assistant contract links for AOP ids with '/' in them - contracts CSV export: use the raw id (strip 'c:' prefix only) for the 'id' column so programmatic consumers receive the domain key without URL-encoding artifacts - contract.tsx display text: decode c.id before rendering so users see the human-readable path instead of literal '%2F' * style: prettier format contract.tsx * fix(contracts): encode ? and # in contractSlug, harden entityHref and meta canonical - contractSlug now encodes %, /, ?, # so any AOP contract number is safe as a URL path segment - entityHref in render-format.ts uses contractSlug directly for contracts (removes double-encode risk) and encodeURIComponent on the slug part for authorities/companies (closes path-traversal concern from review #80) - meta canonical/og:url in contract.tsx re-encodes params.id via contractSlug(contractIdFromSlug()) — React Router decodes the param before the loader runs, so the raw id was being used previously - identity.test.ts covers the new ? and # cases and round-trips * refactor(db): extract bareContractId to remove CSV slug duplication CSV export re-inlined contractSlug's c:-prefix strip. Hoist a shared bareContractId(id); contractSlug builds on it before encoding, CSV uses it raw. Addresses review #221 (NO CODE DUPLICATION). * fix(web): guard contract slug display decode against URIError decodeURIComponent(c.id) crashed the whole page render on a malformed escape. Wrap in safeDecodeSlug with a raw fallback. Clarify that c.id is already the encoded slug so the .json href stays path-safe without re-encoding. Addresses review #221. * test(web): prove contract slug decodes end-to-end via React Router Unit tests simulated the reading side with a manual decodeURIComponent. Drive React Router's real matchRoutes pipeline (decodePath + %2F unescape, the same matcher the Workers server build uses) from an encoded URL and assert params.id reconstructs the domain id for /, %, ?, # and Cyrillic. Addresses review #221. * test(web): preview smoke test for %2F + match real routeConfig (review #221) Address the reviewers' remaining testing gap: the fix depends on the encoded slash surviving the Cloudflare edge. Cloudflare preserves %2F by default (RFC 3986), so the only residual risk is a zone-specific url_decode() Transform Rule. - Add scripts/smoke-encoded-slug.mjs (+ pnpm smoke:slug): a real HTTP GET of a "/"-in-id contract against a deployed preview, expecting 200 — discovers the slug from the sitemap (which already routes through contractSlug). Exits non-zero so CI can gate a deploy. This is the deployment-config check the unit tests cannot do. - End-to-end test now matches against the real routeConfig (was a literal [{path:'contracts/:id'}]), so it exercises the actual route table incl. the contracts/:id.json ordering and breaks if the route is renamed. - Refresh the 'not covered' comment with the concrete edge behaviour + pointer to the smoke test. Refs #213 #221 * fix(web): address #221 review — encode whitespace in slugs, copy-safe JSON sub-line - contractSlug: also percent-encode whitespace and C0 control chars (not just /?#%), so a domain id carrying a space (e.g. a contract_number like „ОП 20-42") no longer leaves a literal space in the SSR href / sitemap <loc>, which the sitemap spec rejects. Readable chars (incl. Cyrillic) and the structural `:` stay literal. Adds a round-trip test for the space case. - contract.tsx: the JSON-endpoint sub-line now shows the encoded slug verbatim (c.id) instead of the decoded form, so copying the visible path yields a working URL — a decoded literal „/" would 404. Drops the now-unused safeDecodeSlug helper. - cache-key.ts: document that decoding collapses %2F→/, so the encoded 200-form and a raw-slash 404-form share one cache key — safe only because app.ts gates cache-put on response.ok; note the linkage so a future change can't silently reintroduce the #213 cache-poison regression. * fix(web): tighten slug control-char encoding + guard cache-put invariant (#221 review) - contractSlug: also encode DEL (U+007F) alongside the C0 range. The C0 range (U+0000-U+001F) was already covered (the review misread the class as 0x40-0x5E) but DEL was genuinely missing. Adds a fromCharCode-built test asserting 0x01 and 0x7F round-trip to %01 / %7F, so the "encode control chars" promise is locked and can't be misread again. - app.cache.test.ts: add an invariant test that a non-ok (404) response is never cached (stays BYPASS, never HIT) - the guarantee cache-key.ts relies on so the %2F->/ key collapse can't let a raw-slash 404 poison the encoded 200 entry. - app.ts: cross-reference comment at the response.ok cache-put gate documenting that dependency and pointing at the new test, so the gate can't be dropped silently. - contracts.ts: clarify the CSV export deliberately carries the RAW id (literal /, %) - not the %2F/%25 slug - for joins/lookups. * refactor(db): use p{Cc} for control-char encoding in contractSlug (#221 review) Functionally equivalent to the prior explicit control-point range (CI was green, the %01 / %7F round-trip test passed), but a raw code-point range renders as caret notation in some diff viewers, which led reviewers to twice misread it as the 0x40-0x5E range. The Unicode control property (with the u flag) is unambiguous and also covers the C1 control range for good measure. Cyrillic and the structural colon stay literal; the control-char round-trip test is unchanged and still passes. * docs(db): pin slug invariants in contractSlug/contractIdFromSlug docstrings * fix(db): encode backslash in contractSlug - WHATWG parsers treat \ as / in paths --------- Co-authored-by: Diyana Dimitova <diyanaydimitova@Diyanas-MacBook-Pro-2.local> Co-authored-by: Todor Kolev <tkolev@obecto.com>
… + 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>
…s new home apps/web's assistant tests need meta.rows_read and meta.total_attempts: they drive the rows-read budget that keeps a retried full scan from under-billing the Denial-of-Wallet limit (midt-bg#122, review midt-bg#80). Flattening that to a fixed empty meta would have quietly removed what those two tests assert, so a route can declare its own meta. Default stays `{}`. Moving d1-sqlite.ts here left it with no tests of its own — its callers live in db, ingest and etl, and none of them count toward this workspace. The ratchet caught it at 81% and it is covered directly now, including the case nothing tested anywhere before: batch() rolls back when one statement fails. A half-applied batch would leave a fixture in a state no production path can reach, and whoever met it would be debugging a ghost. While covering it, throwingD1's bind() read `calls.at(-1)` — so binding statement A after preparing B recorded the arguments against B. Same statement-independence bug fakeD1 already had a test against; it captures its own record now, and so does the test. 100% lines, 100% branches, 44 tests.
* test(ci): gate the fake-D1 doubles, and fail on an unmatched query #325: 24 test files hold 36 `as D1Database` casts, one hand-rolled double each. Every one dispatches on `sql.includes('…')` and falls through to `{ results: [] }` when no marker matches, so renaming a CTE or reordering a JOIN leaves the test green against emptiness — asserting nothing. Only details.test.ts throws today. This is the acceptance test for that work, written before the work: outside an explicit allowlist, no file under apps/ or packages/ may type a value as a D1Database. It is red now (24 files, 36 casts) and goes green when the last double moves to the shared helper. The allowlist is by name, never a directory glob — the argument the #254 review already made about the coverage exclusion list. A glob lets a new double leave the gate by where it sits; a named entry means someone had to add it, which is reviewable. A stale entry is an error rather than a no-op, so a renamed double cannot leave the gate widened by a line nobody reads again. That fail-closed branch is what fires right now, since the helper does not exist yet. Matching is over blanked source — comments, strings and regex literals removed, byte positions kept — so a comment describing the old design is not a finding. `as unknown as D1Database` is matched before `as D1Database` because the short spelling is a suffix of the long one and a naive pattern counts one cast twice. `satisfies` is covered too: it is the only other operator that types a literal. Self-test is mutation-checked — dropping the `as unknown as` alternative, the `satisfies` alternative, the comment blanking, the trailing word boundary, or the stale-entry check each kills exactly one named test, and no others. scripts-test.yml needs no edit: its lane already globs scripts/*.test.mjs. * test(test-support): a shared D1 double that throws on an unmatched query The helper #325 asks for, as its own private workspace. `@sigma/db` exports only `.`, and neither apps/etl nor packages/ingest depends on it, so putting the double under db/src/test/ would have meant a subpath export plus two new workspace deps. A separate package also sits outside all six measured workspaces, so it cannot enter their coverage denominators by construction — stronger than the by-name vitest.shared.ts exclusion the issue proposes, and it leaves that file (which #254 rewrites) untouched. A route is a marker set and a response; every marker must appear in the SQL, and the first matching route wins so a specific route can precede a general one. Unmatched throws, naming the offending statement and every registered marker. `{ onUnmatched: 'empty' }` buys emptiness back, at the call site, in writing. Three entry points, one core: fakeD1 for query tests, recordingD1 for the tests of a *wrapper* over D1 (readonlyD1) that must accept arbitrary SQL and assert on a call log, throwingD1 for the error paths. Two design notes worth keeping: - `first` is typed `object | null | (call) => object | null`, not `unknown`. A top type absorbs the union and the callback form silently loses its parameter type — tsc caught it. A D1 row is an object or nothing anyway. - No pagination feature. Keyset slicing is already `all: (call) => rows.filter(r => r.id > call.binds.at(-2))`, which is what the doubles in companies.test.ts do by hand today. Tests written before the code, behaviour by behaviour, and mutation-checked: never throwing on an unmatched all() or first(), matching a route that answers a different method, `some` for `every` over the markers, last-match instead of first, no truncation, dropping the marker list from the message, discarding bind() arguments, not recording prepare() or batch(), and ignoring throwingD1's supplied error — eleven mutations, each killing a specific named test. The `?? []` fallback in all() went away rather than getting a test: the route lookup already guarantees the response is defined, so it was unreachable. Returning the response instead of the route also keeps `first: null` — a route meaning "no such row" — distinct from no route at all. Seventh key in coverage-baseline.json: check-coverage's findTestWorkspaces fails closed on a workspace that has a test script without a baseline entry, and this one should be measured. 100% lines, 100% branches. The six existing workspaces are untouched — they were already reading above their baselines before this branch, which is pre-existing drift and not for a test refactor to ratchet. * test(test-support): keep `sql` live, and fail the throwing double at execution Two defects the first migration batch walked straight into. `sql` was a getter over `calls`, so `const { db, sql } = fake()` — the natural way to use it, and what flows.test.ts and authorities.test.ts already wrote against their hand-rolled spies — captured an empty snapshot that never filled in. Every later assertion then read nothing and passed for the wrong reason, which is the exact failure this helper exists to remove. It is a live array kept in step with `calls` now, and a test pins the destructured form. throwingD1 threw from prepare(). D1's prepare() is lazy and never touches the database: a missing table surfaces on all()/first()/run(). A double that failed earlier would let a test claim it covers an error path it never reaches — and related-persons.test.ts, whose whole point is that an un-migrated environment degrades instead of 500ing, hand-rolled a double that threw at execution for exactly that reason. It now rejects from the three execution methods and records the statement that failed, so the offending SQL stays inspectable. * test(db): route the query doubles through the shared fake Sixteen files in packages/db/src/queries, each of which built its own D1 double that dispatched on `sql.includes('…')` and fell through to no rows. Fixtures and assertions are unchanged — only the double moves. Measured before touching anything, by breaking each marker in the production SQL and running the test: ELEVEN marker paths across nine files stayed green against an emptied result. authorities (FROM authority_totals), companies (ORDER BY bidder_id), competition and trend and flows (FROM sector_totals), contracts (facet_counts), home (bids_received = 1, JOIN), network (FROM company_totals, FROM authority_totals WHERE authority_id), search (sqlite_master). Every one of them now rejects with the marker set it was looking for. Three things the migration turned up that were not in the issue: - regions.test.ts served *region* rows to sectorOptions, which asks a completely different table. It reached the same answer only because sectorOptions reads r.division, the region fixture has no such field, and the filter dropped every row. The route says `all: []` now, and says why. - companies.test.ts registered two facet routes for queries no test in it ever issues — getCompanyFacets is not exercised there. Dropped rather than kept as decoration. - companies' CSV stream and list query both read company_totals, so breaking the stream's ORDER BY quietly fell through to the list route and returned an unpaginated page. They are separated by their own markers now (ORDER BY bidder_id vs AS sort_value), and breaking either one throws. Two markers still survive being broken — authorities' and companies' `FROM <rollup>`. That is the harness, not the tests: `FROM ${src.from}` is composed at runtime, so the literal never appears in the source to be mutated. Mutating the `from:` value itself is caught by both. Route matching is still substring-based, so a query can fall from a specific route to a more general one in the same set. What is gone is the *default* fall-through to emptiness — an unrouted query throws. packages/db: 487 tests pass; coverage unmoved. * test(test-support): one real-SQLite D1 facade instead of four copies d1FromSqlite lived in packages/ingest/src/test/, and packages/db had two byte-identical re-implementations of it (contracts-filter-sql, value-base-sql, differing only in a local variable name) while apps/etl reached the original through ../../../packages/ingest/src/test/d1-sqlite — a relative path across a workspace boundary, which is what a missing shared home looks like. It moves to @sigma/test-support beside the fake. The two are different tools and stay different: this one runs the real SQL against a real node:sqlite database where SQL semantics are what is under test; fakeD1 is for the TypeScript logic around a query. Now they at least live in the same place, and the gate's allowlist names one package instead of two. Slightly wider than #325 asked — the issue scopes itself to the fake doubles and puts real-SQLite tests out of scope. It is here because "one cast everywhere" is one of its own done-when boxes, and two of the four remaining casts were these copies. Reviewer's call; it lifts out cleanly. Side effect worth noting: d1-sqlite.ts leaves packages/ingest's coverage denominator by leaving the workspace, which is the outcome #254 wanted from a by-name exclusion, reached by construction instead. db 487, ingest 84, etl 20 — all pass. * test(db): recording doubles for the two readonly wrapper suites readonly-d1 and readonly-corpus test a *wrapper* over D1, not a query: what matters is which statements reach the handle underneath, not what comes back. Marker dispatch is the wrong shape for that, so both use recordingD1 — answers anything, records everything — with `when: []`, a route that constrains nothing. Two things came out of it, both in the helper: - `when: []` matching every query was already true (every() over no markers), but undocumented and unpinned. Now both. - readonly-d1's hand-rolled log tagged its entries `prepare:` / `exec:`, and flattening that into plain SQL would have cost the test its point: a wrapper that sent an exec down the prepare path emits identical text, and the assertion could no longer tell. FakeD1Call carries `via` now, and the corpus's zero-proxy row survives as the response to a constraint-free route. readonly-corpus also dropped a `raw()` no production path calls. packages/db: 487 tests pass. * test(etl): route the ETL doubles through the shared fake Three doubles. The integrity gate's fake dispatched on eleven markers and fell through to no rows; it now names all eleven as routes and rejects anything else. Its local builder was called `fakeD1`, which is the shared helper's name, so it becomes `servedD1` — which is what it models anyway: a served D1 after precompute, not any old one. eop.test.ts also passed `{} as D1Database` twice, for paths that fail before they reach the database. `fakeD1([])` states that instead of implying it: a route-less double rejects any query, so if one of those paths ever did reach D1 the test would say so rather than throwing an incidental TypeError on an empty object. The freshness double's guard survives as a route that throws its own message — "raw staging should not be read for planning" is a claim worth keeping in the test, rather than degrading to the generic no-route error. apps/etl: 20 tests pass. * test(test-support): per-route meta, and cover the SQLite facade in its new home apps/web's assistant tests need meta.rows_read and meta.total_attempts: they drive the rows-read budget that keeps a retried full scan from under-billing the Denial-of-Wallet limit (#122, review #80). Flattening that to a fixed empty meta would have quietly removed what those two tests assert, so a route can declare its own meta. Default stays `{}`. Moving d1-sqlite.ts here left it with no tests of its own — its callers live in db, ingest and etl, and none of them count toward this workspace. The ratchet caught it at 81% and it is covered directly now, including the case nothing tested anywhere before: batch() rolls back when one statement fails. A half-applied batch would leave a fixture in a state no production path can reach, and whoever met it would be debugging a ghost. While covering it, throwingD1's bind() read `calls.at(-1)` — so binding statement A after preparing B recorded the arguments against B. Same statement-independence bug fakeD1 already had a test against; it captures its own record now, and so does the test. 100% lines, 100% branches, 44 tests. * test(web): route the last two doubles through the shared fake assistant/tools built a double whose only real job was carrying meta; it now declares that meta on a route. csv-export asserted the expected SQL *inside* its fake — that assertion becomes the route's own marker, so a query that no longer matches rejects and names both the statement and what was expected, instead of failing an inline expect from inside a stub. With these two the gate from the first commit goes green: 279 files scanned, no D1Database cast outside @sigma/test-support. It opened at 36 casts in 24 files. apps/web: 493 tests pass. * fix(test-support): route exec() and batch() through the same contract batch() recorded each statement and returned a synthetic success without ever consulting the routes, so a batch of unregistered SQL passed against nothing — the silent green this helper exists to kill, on the one entry point the write paths use exclusively (staging, refresh, fx never call prepare().run()). exec() had the identical hole one method up. Both now look the statement up. They ask only whether it is registered at all, not for a particular response shape the way all()/first()/run() do, and throw naming the SQL and every marker when it is not. Also: run() and batch() carry the `results` key a real D1Result always has — the cast to D1Database was hiding its absence; the header no longer points at the facade's pre-move path; and the second batch() record is documented as a log of entry points rather than a double count. * fix(test-support): carry the full D1Result shape in the SQLite facade all() returned no `meta`, run() neither `meta` nor `results`, batch() no `results`. The cast to D1Database hid every one of them: the first caller to read one would get `undefined` from the facade where real D1 hands back `[]` or `{}`. One D1Shape type spells out all three keys. * test(web): keep the exact-SQL check csv-export had before the migration The hand-rolled double asserted `expect(sql).toBe(...)` on the whole statement. Migrating turned that string into a `when` marker, and markers match by substring — so the one place the refactor loosened a check rather than tightening it. Measured: wrapping the production statement leaves all 34 tests green. The equality moves inside the route, where the callback sees `call.sql`. * test(ci): pin the gate's scan roots and catch an aliased D1Database Two ways past the gate, both reproduced. `type DBAlias = D1Database` and then `as unknown as DBAlias` leaves no D1Database token for the pattern to find; a renamed type import does the same. A second pass treats giving the type another name outside the allowlist as the offence, while leaving ordinary annotations (`db: D1Database`, a field on an Env type) alone. And SCAN_ROOTS was module-private, so deleting 'apps' from it left the self-test 12/12 green while web and etl dropped out of enforcement. Exported and pinned: a pattern applied to half the repo is a gate that passes while enforcing nothing. * fix(test-support): route a batched statement by what it does, not its markers Markers alone are blind to the method, and it is measurable: a route declaring only `all:` answered a batched `DELETE FROM staging` with its rows, because `FROM staging` is a substring of the write. The same SQL through prepare().run() threw. Narrower than an unrouted batch, but the same silent pass. A batched write now needs a `run:` route and a batched SELECT an `all:` one; neither settles for the other, and a SELECT no longer fires a write effect it happens to match. A write still serves rows when it has them, for RETURNING. exec() asks for `run:` too — it hands back no rows, so nothing else means anything to it. That retires `registered()`: every entry point is method-aware. Reading is decided by the leading keyword, so a `WITH … INSERT` reads as a SELECT here. That costs a false rejection, never a false pass. Also: run() carries the route's meta, which batch() already resolved for the same statement, and batch() documents that it is not transactional — real D1 and the d1-sqlite.ts facade roll back, this does not. * test(ci): widen the alias rule to where an alias actually goes The pattern closed three spellings while the comment promised the class. Four more walked past it: `D1Database & {}`, `Pick<D1Database, …>`, a namespaced `import('…').D1Database`, and a heritage list naming it off the first position. The rule is now positional — the mention must sit right of `=`, or inside an intersection, union, type argument or namespace, never where a parameter or a field goes. `type Env = { DB: D1Database }` and the conditional type in readonly-corpus.test.ts stay clean, both pinned. `implements` is deliberately out: ReadonlyD1 implements D1Database in production, and TypeScript forces a complete implementation there, so it is no shortcut to a stub. The comment now says best-effort and means it. --------- Co-authored-by: Todor Kolev <tkolev@obecto.com>
8 participants
Какво е това
Backend основата на AI асистента на Sigma — имплементация на
docs/spec/ai-assistant.mdс хардунирането от §9 (PR #79). За разлика от ранната чернова, бекендът вече е опроводен от край до край: чисти тествани модули → tool registry → agent loop → ресурс route/assistant/chat(BgGPT през Cloudflare AI Gateway). Потребителският слой (dock UI, renderer на справките, глас) и provisioning-ът остават за следващи PR-та.Какво има (имплементирано и проверено)
emit_reportблоковете реферират хендъли към сървърно изпълнени резултати, аbindReport()пре-свързва реалните стойности. Прозата е markdown-санитизирана; guardrail E2 отказва едри числа в прозата (вкл. Unicode цифрови форми).node-sql-parser) с table-allowlist, забрана на TVF/cross-join/recursion, AST-достоверен LIMIT и scalar-fn denylist.bge-m3) — schema grounding +semantic_search.describe_schema,run_sql(+ Denial-of-Wallet rows-read бюджет, [Идея]: Denial-of-Wallet чрез run_sql: D1 таксува прочетени редове, не върнати (§7/§9.4 #122),semantic_search,eop_fetch(валидирана дата, fixed base — без SSRF),source_linkи терминалнияemit_report.abortSignal, drop на client-подадениsystem/toolсъобщения.Нови зависимости:
ai@^6,@ai-sdk/openai@^3,node-sql-parser@^5.Нови bindings/vars (wrangler.jsonc):
AI,VECTORIZE,REPORTS(R2),ASSISTANT_RATE_LIMITER+ config vars (BGGPT_*,MAX_STEPS,D1_ROWS_READ_BUDGET).Проверено:
pnpm --filter web typecheck→ 0; пълният web test suite минава;pnpm audit --audit-level=highчист; Prettier чист. (CI е авторитетът за актуалния брой тестове.)Това PR добавя bindings към Cloudflare ресурси, които трябва да съществуват преди
wrangler deploy. Освен това route-ът изпълняваrun_sqlв момента, в койтоBGGPT_API_KEYе наличен, докато D1 binding-ът е read-write — затова не задавайте продBGGPT_API_KEY, докато (1)run_sqlне работи срещу read-only D1 binding/реплика и (2) не е наложен глобален budget/circuit-breaker. Двуслойният SQL guard е defense-in-depth, не единствената бариера. Пълни детайли:apps/web/app/lib/assistant/README.md.Какво НЕ е тук (следва)
UI слоят (dock,
emit_reportrenderer +/reports/:id, глас), provisioning-ът, и launch-gate hardening: read-only D1 data path, глобален budget/circuit-breaker, freshness wiring, golden-report CI.eop_fetchзасега връща само брой редове на ден, не самите данни (probe за наличие/свежест).Имплементира части от спецификацията в #79.