feat: chatbot Retrieval-Augmented Generation (RAG) phase (v1) - #592
Open
nivek0o0 wants to merge 37 commits into
Open
feat: chatbot Retrieval-Augmented Generation (RAG) phase (v1)#592nivek0o0 wants to merge 37 commits into
nivek0o0 wants to merge 37 commits into
Conversation
…us ingest The RAG branch predated several dependency bumps that landed on main (fastify 5.10, fastify-plugin 6, fastify-type-provider-zod 7, jwks-rsa 4). Take main's versions rather than the branch's older majors, and add pdf-parse, which the corpus ingest CLI uses to extract text from source PDFs.
Installs the `vector` extension and adds a nullable `embedding vector(1024)` column to `chatbot_corpus_chunk`, plus an HNSW index using cosine distance with pgvector's defaults (m=16, ef_construction=64). Search-time `ef_search` is intentionally left at the default so it can be tuned per deployment without a migration. The column is nullable so ingest can insert chunks and embed them in a second pass, and so a chunk whose embedding failed stays queryable.
The corpus migration installs the `vector` extension, which the official
postgres image does not ship, so local compose, the database package's compose,
and the testcontainer all move to pgvector/pgvector:pg18.
Kept digest-pinned to preserve the supply-chain posture adopted for DPG
compliance. Note this image is Debian-based rather than alpine: pgvector
publishes no alpine variant for pg18, and the official postgres image cannot
run the migration at all ("extension \"vector\" is not available").
The Debian base uses glibc collation, which matches the deployed database
(infra/modules/postgres.bicep creates it with collation es_ES.UTF8) more
closely than alpine's musl did.
On Windows hosts `localhost` resolves to `::1` first, but Docker Desktop binds only `0.0.0.0` by default, so Prisma fails the suite with P1001 "Can't reach database server". Rewrite the hostname to 127.0.0.1 so the URL behaves identically on macOS, Linux, and Windows.
SourceCitation describes one cited corpus chunk (source id, chunk id, label, canonical URL) and is carried on the streaming `done` event so the widget can attribute an answer without a second request. GetCurrentConversation is the contract for rehydrating a persisted thread on mount. Both live in packages/types so the API and the web app share one definition.
Adds EMBEDDING_PROVIDER plus the Azure knobs the RAG phase needs (AZURE_OPENAI_API_VERSION, _API_KEY, _REASONING_EFFORT, _EMBEDDING_DEPLOYMENT_NAME, _EMBEDDING_API_VERSION) to parseEnv, so they are validated in the same pure, table-testable place as the rest of the config. The mock-in-production and required-field guards are gated on CHATBOT_ENABLED, matching the existing LLM guards: a deployment that runs with no AI must not be forced to supply embedding configuration. The mock is still rejected for an enabled production chatbot, because its SHA-256-derived vectors have no semantic relation to the text and would silently corrupt retrieval. Enabling the chatbot in production therefore now also requires a real embeddings deployment; the environment tests are updated to reflect that retrieval is part of the chatbot rather than an add-on.
…backends Mirrors the LLMProvider shape: a cached factory selects the backend from EMBEDDING_PROVIDER, so callers depend on the interface rather than on Azure. The mock derives deterministic, L2-normalized 1024-dim vectors from a SHA-256 seed, which keeps the corpus suites reproducible with no network. The Azure backend batches inputs against both an input-count and a token cap, and rejects a single oversized input before issuing any SDK call. The eslint no-network rule now covers both provider mocks, and the test suite sets EMBEDDING_PROVIDER=mock so nothing reaches a live endpoint.
LlmMessage becomes a discriminated union so an assistant turn can carry `toolCalls` and a TOOL turn can carry the `tool_call_id` the OpenAI API requires. The stream gains a `tool_call` event, and options gain `tools`. The Azure provider accumulates tool-call deltas keyed by wire index (the id and name arrive on the first delta, the arguments accumulate as a JSON string) and emits them in index order before the terminal usage event. It also gains optional API-key auth for local development and an optional `reasoning_effort` passthrough for reasoning deployments. Foundation coerced a TOOL message to a user message because it never emitted one; that coercion and its test are replaced by real tool mapping, since the tool round now replays tool results back to the model.
searchKnowledge embeds the query and runs a cosine-distance nearest-neighbour search against the HNSW index, returning chunks with the metadata needed to cite them. Only ACTIVE sources are visible: DRAFT (mid-ingest) and OUTDATED (superseded by an activation) are excluded, so a partially-ingested or retired document can never be quoted to a user. Scope and source-type filters narrow results further. Query validation (empty, oversized, out-of-range or non-integer topK) runs before the embedding call and the SQL, so a bad tool argument costs nothing.
… lint The foundation phase left the corpus tables dormant and asserted they were referenced nowhere in apps/api/src. Retrieval activates them on purpose, so that invariant is gone and the test can no longer pass. Deleting it outright would drop a useful guardrail, so it is narrowed instead: corpus access is a detail of the retrieval module, and no other feature may reach into those tables. The ingest and activate CLIs live outside src and are intentionally out of scope. A second assertion proves the grep still matches something, so the check cannot start passing vacuously if retrieval is ever moved.
… prompt The tool schema is what the model sees, so its arguments are treated as untrusted input and validated before retrieval runs. The system prompt is loaded from a Markdown file rather than inlined, so the wording can be reviewed as prose and adjusted per deployment without touching the handler. It is loaded once at module scope.
The handler peeks the first stream event before hijacking the response. That ordering is what lets a pre-stream failure — a provider error, or a second consecutive tool_call, which violates the single-round invariant — surface as a real HTTP status instead of a terminal SSE error on an already-committed response. A non-tool turn hijacks immediately so streaming stays delta-by-delta. On a tool turn the handler executes retrieval, replays the assistant tool_calls and the tool result into a second invocation, and streams that. Oversized RAG context aborts the round and marks the row truncated. Token accounting uses the second usage event only, never a sum: the first invocation terminates on tool_call and reports no usage. Cited sources are persisted on the assistant row and sent on the `done` event. When the reply opens with the no-corpus-support disclaimer the source list is dropped, so the wire payload and the visible text agree. Main's disconnect and finalization semantics are preserved throughout: the client-disconnect guard, the conditional truncation UPDATE, and the explicit finalizer for a mid-stream error are shared by both branches.
Adds GET /conversations/me/current, which returns the thread pinned by a signed `chatbot_conversation_id` cookie when it is inside its TTL and the request identity matches the row. The cookie is refreshed on every turn so the window slides with use. Identity is enforced, not assumed: an authenticated caller matches on user_id, an anonymous one on session_id with user_id IS NULL. A cookie that is expired or belongs to someone else yields 404 and is cleared in the response, so a stale cookie self-heals rather than leaking another caller's thread. The route registers inside the CHATBOT_ENABLED gate with the other two, so a deployment running without AI still exposes no chatbot surface at all — the route gate test now asserts all three endpoints.
`chatbot:ingest` parses a PDF, chunks it at section boundaries, embeds the chunks, and writes them as a DRAFT source. `chatbot:activate` promotes a DRAFT to ACTIVE and retires the previous version to OUTDATED in one timestamped cutover, so retrieval never sees two live versions of the same document. Ingest is deliberately not gated on CHATBOT_ENABLED: an operator needs to seed a corpus before turning the assistant on. That leaves the CLI as the last line of defence for the mock provider, whose vectors would silently poison retrieval, so it refuses to run with EMBEDDING_PROVIDER=mock under NODE_ENV=production regardless of the flag. Re-ingest collisions are checked inside the transaction that creates the source, so two concurrent runs cannot both pass on the same stale read. An audit row is written even when embedding fails, with NULL completed_at and NULL source_id. eslint gets a scripts/** override: these are CLI entry points, where process.exit and a trailing fire-and-forget main() are idiomatic.
… order These "ordered by name" tests compared a Postgres ORDER BY result against a bare [...names].sort(), which orders by UTF-16 code unit — byte order. That only agreed with the database while the test container was alpine-based (musl, effectively C collation), and it never matched the deployed database, which is created with collation es_ES.UTF8 (infra/modules/postgres.bicep) and therefore applies glibc linguistic collation. Moving the container to pgvector's Debian image exposed the mismatch: the assertions were encoding an alpine artifact rather than production behaviour. The shared comparator is primary-strength with punctuation ignored, because glibc and ICU agree on letter order but not on how they tie-break accents, case, and punctuation — pinning either would recreate the same brittleness. Equal-comparing pairs keep their database order under a stable sort, so the assertion reduces to the property these tests actually care about: the endpoint returns rows in non-decreasing order rather than arbitrary order.
The terminal `done` event now carries the cited corpus chunks, so a turn's attribution arrives with the turn and needs no second request. A malformed payload is logged and ignored rather than failing a turn whose text already streamed. MessageBubble gains a collapsed "Fuentes consultadas" panel below the bubble. Sources are deduplicated by canonical URL because one document usually contributes several retrieved chunks, and the panel lists documents. Inline `[label](url)` markers are stripped from the rendered markdown: the V1 corpus is dominated by a single source, so repeated inline links are noise when the same attribution is available in the panel.
…eting it useConversationRehydrate loads the persisted thread once on mount and seeds it through useChatStream. It is a separate hook on purpose: folding the fetch into useChatStream would make every one of that hook's turn-streaming tests observe an extra mount-time request, shifting their mocked response queues. seedMessages takes the raw rows and mints React keys from useChatStream's own counter — two id sources could collide on `assistant-1` and let React reconcile a fresh bubble onto a seeded node, which is the hazard that counter exists to prevent. "Nueva conversación" now starts a fresh client-side thread instead of deleting history: it drops the conversation cookie, aborts any in-flight turn, and leaves the prior turns persisted. A cancelled turn cannot re-dirty the cleared view, because the abort ref is nulled before aborting and the turn checks that it is still the current one before applying terminal state — while a user Stop leaves the ref in place, so a stopped turn still resolves to truncated. deleteHistory stays available on the hook for data-deletion requests but is deliberately unwired from the UI, which now offers only the non-destructive reset. A persistent footer states that answers are AI-generated and should be verified against the cited sources.
http-proxy-3 emits `proxyRes` before its own `writeHeaders` pass, which is gated on `!res.headersSent`. Flushing headers synchronously there makes that pass skip, so the upstream Content-Type, Cache-Control, X-Accel-Buffering and Set-Cookie headers never reach the browser and the widget's stream never starts. Defer the flush to nextTick so the proxy copies the upstream headers first.
…ccess Adds the embedding and Azure tuning variables to the environment reference, records the pgvector image requirement in local setup, and documents the corpus ingest/activate runbook including the DRAFT → ACTIVE → OUTDATED cutover. Also adds the AI access requirements note covering what the deployment needs from Azure OpenAI (chat and embedding deployments, managed identity) so an operator can provision it without reading the code.
Captures the proposal, design, spec deltas, and task list for the retrieval phase: corpus schema and embeddings, ingest, retrieval, the single tool round, and the widget's citation surface. Also carries the chatbot-web-test-infra proposal from the branch. That one is superseded — main established the web Vitest convention and covered the Chatbot module in #496 and #513 — so it should be archived or dropped rather than implemented.
…equests `chatbot_conversation_id` was hardcoded to SameSite=Lax while its sibling `chatbot_session_id` uses SameSite=None in production, because the deployed web app and API sit on different registrable domains. A Lax cookie is not sent on those cross-site `credentials: "include"` requests, so the rehydrate endpoint would never receive it and conversation persistence would silently do nothing in production while working fine locally behind the Vite proxy. Derive SameSite the same way the session cookie does, and build the clearing cookie from the same options object so the two cannot drift again — a browser only overwrites a cookie whose attributes line up. The branch this came from predated the cross-site cookie change, so Decision 28 never accounted for it.
…nd cookie The foundation-phase docs described the corpus tables as dormant and knew only one chatbot cookie, both of which the retrieval phase changes. sensitive-data.md: corrects the "dormant in foundation" claim, states what `sources_cited` and the corpus tables actually hold (operator-ingested reference material and embeddings derived from it — no user content), and documents `chatbot_conversation_id` in full: signed, 30-day sliding window, SameSite and Secure mirroring the session cookie, and deliberately NOT HttpOnly so the "Nueva conversación" control can detach client-side. That last point is the one worth spelling out in a security doc, so it records why the signature is the security property and that the endpoint re-checks TTL and identity regardless of what the cookie says. Also notes a leftover cookie is harmless after deletion, and that deletion does not touch the corpus. system-architecture.md: adds a retrieval-flow section splitting the offline operator ingest path from the online turn, including why the handler peeks the first stream event before hijacking the response, and why exactly one tool round runs. Records the pgvector column on ChatbotCorpusChunk and why it is Unsupported. tech-stack.md: adds the pgvector extension, the chat and embedding SDK entries, and pdf-parse — noting which are optional and gated behind CHATBOT_ENABLED. codebase-map.md: points at the files a contributor actually needs for prompt wording, retrieval, the tool schema, provider swaps, and the corpus CLIs.
nivek0o0
added
type: feature
16 tasks
# Conflicts: # apps/api/package.json # pnpm-lock.yaml
`chatbot-web-test-infra` existed because `apps/web` had no unit-test toolchain when the RAG work was written, so four widget tests were deferred behind standing one up. That toolchain landed on main independently (#496 / #513): vitest.config, the jsdom environment, RTL, the setup file, `pnpm test:web`, and a `test-web` CI job all exist, and the Chatbot module is already partly covered. Drop the infrastructure half rather than carry a proposal to build what is built, and keep the part still missing: the four tests themselves. Two corrections while retargeting: - Task 10.28 asserted the click mints a fresh `conversation_id` distinct from the prior one. That described an earlier design. The server owns the conversation id and pins it with the signed cookie, so the client mints nothing — the reset drops the cookie instead. The requirement now splits by layer: the widget test covers the control's wiring and that no request is issued, the hook test covers the state effects. - The spec mandated `apps/web/test/features/chatbot/<scenario>/` to mirror the API layout. apps/web co-locates tests beside their source instead, which is what all existing Chatbot test files do, so the scenario now says that explicitly and notes the mirrored layout is API-only.
Implements the four tests deferred to `chatbot-web-test-infra` (tasks 10.22, 10.23, 10.28, 10.34). They cover exactly the widget surface this branch adds and otherwise left untested: `ChatbotWidget` had no test file at all, and `MessageBubble.test.tsx` predates the citations panel. MessageBubble — the panel renders with its source count, each source is an anchor carrying `target="_blank"` and `rel="noopener noreferrer"` (without `noopener` the opened page can reach back through `window.opener`), sources sharing a `cite_url` collapse to one row, and the panel is absent when `sourcesCited` is undefined, empty, or on a user turn. The links are asserted after expanding, because MUI's Collapse hides its contents from the accessibility tree while closed. ChatbotWidget — first test file for the component. The reset empties the rendered list, issues no request of any kind, and is disabled mid-turn; its accessible name is asserted to be "Nueva conversación", with explicit negative checks against the destructive wording of earlier drafts. The disclaimer is asserted byte-for-byte in all six canonical states and shown to carry no interactive affordance. `useChatStream` is stubbed there, but statefully, so "the list empties" is an assertion about the DOM rather than about a mock call. The rehydrate hook is stubbed too — otherwise its mount-time fetch would make "the reset fires zero HTTP requests" impossible to distinguish from the request the widget makes anyway. useChatStream — the reset's state effects against the real hook: the thread clears, state returns to empty, Last-Event-ID does not leak into the next turn, the conversation cookie is dropped, and a turn cancelled by the reset cannot write its terminal state onto the cleared thread. That last one is the mirror of the existing Stop test, which resolves to "truncated" precisely because a stopped turn IS still the current one.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The API image crashed on startup — CI's api image smoke test caught it: the container exited before it could serve /health. Two independent faults, either of which alone is enough to break a deployment: 1. `build` is `tsc` + `tsc-alias`, and neither copies non-TypeScript files, so `prompts/es/system.md` never reached `dist/`. Fixed with a `cpy` step, the same way packages/database ships its generated Prisma client. 2. The loader read that file at module scope. `@fastify/autoload` imports the chatbot route module on every boot — the CHATBOT_ENABLED gate is *inside* the registration function, not around the import — so the read ran even with the chatbot switched off, and an ENOENT took down the whole API. A deployment that runs the platform with no AI could not boot at all, which is precisely the optionality the flag exists to provide. The loader is now a memoized `getSystemPromptEs()` called from the handler, so the prompt is read on first use rather than at import. Fault 1 was the immediate cause; fault 2 is why it was fatal rather than degraded, and fixing only the packaging would leave the API one missing asset away from the same outage. Verified against the built output: `dist` now contains the prompt, and booting `dist/server.js` with CHATBOT_ENABLED unset reaches the database connectivity check instead of dying at import — including when the prompt file is deleted outright.
CI's `Test (base)` leg failed with the activate CLI dying on "Unable to start a transaction in the given time": under the suite's parallel file execution other test files hold connections, and Prisma's default 2s acquisition window elapsed before the subprocess got one. Those defaults — 2s to acquire, 5s to run — are tuned for a request handler, where failing fast is right. These are operator-run batch commands: ingest inserts one row per chunk of a document, and activate performs a DRAFT → ACTIVE → OUTDATED cutover that must not be left half-applied. Waiting is preferable to aborting, so both now pass explicit budgets. This also removes a latent production hazard, not just a CI flake: a large document, or an ingest run against a busy database, could trip the same 5s transaction timeout on a real deployment and leave the operator with a failed cutover for no better reason than an HTTP-shaped default.
…letely CodeQL flagged the quoting helper as js/incomplete-sanitization (high): it escaped `"` but not `\`, so a value ending in a backslash would escape the closing quote and hand the rest of the command line back to the shell. Not theoretical — the helper's own comment cited `OneDrive\Documentos` as a path it had to survive, so backslashes were an expected input. Escaping them is not the fix: /bin/sh treats `\` as an escape inside double quotes and cmd.exe does not, so there is no single escaping rule correct for both, and applying sh's rule would corrupt the Windows paths the quoting existed to protect. Remove the need to escape instead. The fixture is passed as a path relative to the api workspace (which is already the child's working directory under `pnpm --filter=api`) rather than an absolute path carrying whatever the checkout directory happens to contain. The helper now refuses a value containing a quote or a backslash rather than trying to encode it: every caller passes a fixed literal, so hitting that branch means the test was edited into a shape it cannot express safely, and failing loudly there beats emitting an ambiguous command.
Collaborator
Author
CI round 1 found three real bugs — all fixedOpening this against CI was worth it on its own. None of the three were visible locally. 1. The built API image could not boot at all (
The second fault is the more serious one: a deployment running the platform with no AI could not start, which is exactly the optionality the flag exists to guarantee. Fixed both — a 2. Worth stating plainly: 3. CodeQL, 1 new high alert — Also in this push
Local state: |
… CLI tests
Two follow-ups from CI, one a hard failure and one a partial improvement.
**.dockerignore excluded the system prompt.** `**/*.md` dropped
`prompts/es/system.md` from the build context, which is the root reason it never
reached `dist` and the api image crashed on startup. There was already a
precedent negation for seed-data Markdown read at runtime; the prompt is program
input on the same footing, so it gets one too. Verified by building the image:
the file is present at dist/features/chatbot/prompts/es/system.md, and the
container now boots past module load instead of exiting immediately.
Worth noting the `cpy` step added earlier turned this from a silent runtime
crash into a loud build failure ("No files matched the given patterns"), which is
the failure mode to want — an image missing the prompt can no longer be built.
**The CLI tests used the shared template database.** ingest and activate spawn
the CLI as a subprocess and pointed it at `testdb`, holding connections on the
very database every other test file clones from — `CREATE DATABASE ... TEMPLATE`
requires no other sessions on the source, so those clones failed and retried
against us. They now use this file's own cloned database, like every other file.
This is correct isolation on its own merits, and it removes the template
contention, but it does NOT close #591: the full suite still restarted Postgres
on one of two parallel runs afterwards. Not presented as a fix for that.
Collaborator
Author
CI is green — all 24 checksIncluding everything that failed earlier: Round 2 turned up one more root cause, now fixed: The earlier One caveat before merging
This branch does reduce its likelihood: the ingest and activate tests were pointing their subprocess at the shared template database, holding sessions on the exact database every other file clones from. Fixed, and the chatbot directory now passes in parallel where it failed 3 of 3 times before. But the full suite still restarted Postgres on 1 of 2 subsequent local parallel runs, so the contention was aggravating the problem rather than causing it. Practically: expect
|
`seedMessages` replaced the visible thread unconditionally, so a turn started before the mount-time rehydrate settled — a fast typist on a slow API — had its message replaced by the persisted thread. The silent part is worse than the lost message: replacing the array detaches the in-flight assistant bubble, so every subsequent delta fails the identity check in `updateLastAssistant` and is discarded. The answer streams into nothing. The seed now loses every race, applying only when no turn is in flight and the thread is still empty. Both guards are load-bearing: the ref catches a turn whose append has not committed yet, the `prev.length` check catches an already-populated thread. Ids are minted outside the state updater so it stays pure under StrictMode double-invocation. This was already required by the chatbot-widget spec; the race test was confirmed to fail against the pre-fix code before the guard was added.
…onale Two drift fixes in the widget rehydration requirement. The spec assigned the mount-time GET to `useChatStream`, but it lives in a dedicated `useConversationRehydrate` hook — the two have different lifecycles (one settling load vs. per-turn streaming state), with `seedMessages` as the seam. Behaviour is identical either way, so the requirement is restated on the composed widget rather than either hook. The "seed must not overwrite a new message" requirement was justified by unmount cancellation, which does not apply: the hazardous interleaving happens while the widget is still mounted. The implementation inherited that reasoning and no scenario covered the case, so the resulting race shipped untested. Rationale corrected and two scenarios added.
…tion A full audit of the seven chatbot-rag-mvp specs (53 requirements, 195 scenarios) against the code found five places where the spec had fallen behind deliberate implementation decisions. Behaviour is unchanged; only the specs move. The one that mattered: the conversation cookie requirement still mandated `SameSite=Lax` unconditionally. That is the exact defect fixed in b10628f — the deployed web app and API sit on different registrable domains, so a Lax cookie is never sent and persistence silently no-ops in production while working locally. The code and docs/security have been right since that fix; the spec would have walked the next implementer straight back into it. Corrected, with the rationale recorded so it does not get "simplified" again. The rest: - `searchKnowledge` takes an injected `PrismaClient` as its first parameter. That is what makes this spec's own "importable without Fastify" requirement achievable, so the signature is documented rather than the injection removed. - Both embedding boot guards (mock-in-production, Azure deployment name) are gated on `CHATBOT_ENABLED`. Deliberate: a deployment running with no AI must not be forced to provision Azure. Recorded explicitly, including why the ingest CLI guards mock separately and more strictly — that pairing is what keeps mock vectors out of a production corpus. - The pgvector image is digest-pinned and the testcontainer config lives in `testDatabase.ts`, not `testcontainers.ts`. Also notes the Debian/glibc collation consequence and the shared comparator. - The corpus dormancy guard was narrowed to `corpusAccessBoundary`, not simply deleted; and the LLM mock mirrors the Modo B / C.1 literals so mode routing is testable without an Azure deployment.
…tations The widget cast `done.sources` straight to `SourceCitationWire[]` and checked only `Array.isArray`. The chatbot-widget spec requires a malformed payload to be logged and treated as absent; two shapes slipped through instead: - `sources: "broken"` was dropped silently, with no warn — the existing warn only fires when the whole payload fails `JSON.parse`. - `sources: [1, 2, 3]`, or any entry missing `cite_url`, was assigned verbatim and reached MessageBubble, which keys its rows on `cite_url`: an undefined React key and a blank row. Severity is low — the API is the sole producer and validates server-side — so this closes a defensive gap rather than a live bug. Adds `SourceCitationWireSchema` to @repo/types and parses through it. Kept separate from `SourceCitationSchema`, whose id union also admits the server-side bigint branch: a client accepting that branch would be validating a shape the wire cannot carry. `SourceCitationWire` is now inferred from the schema so the type and the validator cannot drift. Adds eight hook tests — the four `done`-sources scenarios the spec defines had no coverage at this layer at all. The five malformed cases were confirmed to fail against the pre-fix parsing.
An audit of the PR's six security-relevant points against docs/ found four documented and two effectively missing — and the two missing ones were the controls most specific to the assistant rather than to its configuration. The cookie story (non-HttpOnly by design, SameSite mirroring chatbot_session_id) was already thorough in sensitive-data.md. What had no home anywhere was: - Tool-call arguments as an untrusted input. Coverage was a single parenthetical in an architecture doc. Nothing recorded what is actually validated, or where. - The prompt-injection contract. Retrieved chunks are wrapped in `Contenido: "…"` specifically so the model reads them as data, and implementations are forbidden from removing that quoting. That rule existed only in the spec and in code comments — precisely the kind of thing a later cleanup removes without knowing why it is there. Adds docs/security/chatbot.md covering both, plus the corpus ACTIVE-only visibility boundary (promoted from one line in system-architecture.md), the keyless Azure OpenAI path, and why the two mock-provider guards are scoped differently — the boot guard is gated on CHATBOT_ENABLED, the ingest CLI's is not, and the pair is what leaves no path for mock vectors into a production corpus. Indexed in both docs/README.md and docs/security/README.md; the security index previously had no chatbot entry at all, so a reviewer would find the cookie material and miss everything else. Stale text corrected along the way: - secrets.md still listed Azure OpenAI as a future integration whose keys "must be added to Key Vault". It is present now, and holds no key — production is keyless via managed identity. AZURE_OPENAI_API_KEY added to the classification table as a dev-only secret. - hardening.md's input-validation section covered only request bodies. Adds model-supplied input, and notes that searchKnowledge's pgvector query is the one raw-SQL exception (fully parameterised). - environment-variables.md described the boot-time mock guard without the ingest CLI's stricter, separately-scoped one.
The redaction section said redacted fields "appear as `[Redacted]` in log output". The logger is configured with `remove: true` (apps/api/src/app.ts), so the paths are dropped from the record entirely and no placeholder is ever emitted. Worth fixing rather than leaving: anyone grepping logs for `[Redacted]` to confirm redaction is working would find nothing and could reasonably conclude it was not configured. Also notes that env-var secrets are out of scope for the redact list by construction — they are never handed to the logger — so their absence from the table is not a gap. Pre-existing; unrelated to the chatbot work.
The chatbot corpus migration runs `CREATE EXTENSION IF NOT EXISTS vector`, which Azure Flexible Server refuses unless the extension is on the `azure.extensions` allowlist. That allowlist was applied by hand to the running server, so the deployed environment worked while the template stayed silent — any environment built fresh from Bicep would have failed the migration with `extension "vector" is not available`, before the app ever started. Applies regardless of the chatbot flag: migrations run on every deployment, so the extension must be allowlisted even where the assistant itself is switched off. Exposed as `allowedExtensions` because the parameter is an allowlist, not an append — adding entries out of band gets reverted by the next deployment. Ordered after the database resource: a server-parameter write puts the server into an updating state, and Flexible Server rejects concurrent child operations while that runs. Separate from the chatbot IaC that follows because this is a latent fresh-deploy failure on its own.
… flag Everything the chatbot needs on Azure was manual: no Cognitive Services module, no role assignment, and no chatbot app settings on the App Service. So even with the resources created by hand, a deployment would boot the API with the chatbot off. Adds, all gated on `enableChatbot` (default false — the platform is a digital public good and must stay fully usable with no AI and no cloud AI dependency, so deployers opt in): - `modules/openai.bicep` — the account plus chat and embedding deployments. Model versions are pinned rather than tracking latest: a silent model swap changes answer quality with no code change and no signal. Embedding capacity is sized above chat because ingest is bursty — one run embeds every chunk of a document. - `modules/openAiRoleAssignment.bicep` — Cognitive Services OpenAI User for the App Service managed identity, following the existing storage/ACR role-assignment pattern. - App Service settings for the chatbot, with `AZURE_OPENAI_API_KEY` deliberately absent. - `COOKIE_SECRET` via Key Vault, mirroring the database password's create-or-preserve contract. Overwriting it would invalidate every signed chatbot cookie in the wild, silently dropping every user's conversation history on redeploy, so deploy.sh refuses to regenerate it when it cannot prove the secret is absent. Two details that are load-bearing rather than stylistic: - The account sets `customSubDomainName`. Without it the resource is only reachable on the regional shared endpoint, which does not accept AAD tokens — managed identity fails at runtime with a 401 that looks exactly like a missing role assignment. - `disableLocalAuth: true` turns off API-key auth at the resource, so the keyless posture the docs describe is enforced by infrastructure rather than by convention. deploy.sh warns when role assignments are disabled while the chatbot is on: the resources deploy and the settings point at them, but every request 401s until someone with User Access Administrator creates the assignment by hand. It also flags the UNDP deny-AI policy up front, since that blocks the deployment before RBAC is even evaluated. Docs updated: the access-requirements doc said to use it "before writing the Bicep modules" and recorded the pgvector allowlist as a manual step; the hardening checklist now records the API-key item as enforced rather than verify-by-hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
main— including theCHATBOT_ENABLEDgate, the stream timeouts, the cross-site cookie policy, and the by-id in-flight bubble fix. Merging it would have silently reverted five merged PRs. This branch re-applies only the RAG delta on top of currentmaininstead.CHATBOT_ENABLEDlike the rest of the chatbot, so a deployment can still run the platform with no AI and no cloud dependency.Linked issues
Supersedes #303.
Follow-ups found during this work, tracked separately rather than folded in: #589 (chatbot routes bypass the
defineRouteconvention), #590 (ER diagram drift), #591 (Postgres testcontainer restarts under parallel test execution).Closes #
Type of change
Country-agnosticism checklist
Corpus content is data, not code: documents are ingested per deployment through the
chatbot:ingestCLI, withscope(GLOBAL|NATIONAL) on each source. The system prompt lives inprompts/es/system.mdso wording can be adjusted without touching the handler. Every new knob is an env var with a safe default.Backward compatibility: the feature is off unless
CHATBOT_ENABLED=true, and the migration only adds a nullable column plus an index. A deployment that leaves the chatbot disabled is unaffected — though note it now requires a pgvector-capable Postgres image to migrate at all (see Reviewer notes).DPG checklist
docs/was updated where relevant.docs/security/sensitive-data.mdandPRIVACY.md.On the dependency point: both the LLM and embedding providers sit behind an interface with a mock implementation selected by
LLM_PROVIDER/EMBEDDING_PROVIDER, so the Azure OpenAI SDK is swappable rather than load-bearing. pgvector is open source.On PII:
sensitive-data.mdwas updated as part of this PR, including the second cookie (below). The corpus tables hold operator-ingested reference material and embeddings derived from it — no user content.Security checklist
Security-relevant points, called out for review. All six are now written up in
docs/security/chatbot.md(new, indexed in both docs indexes) — the security docs previously had no chatbot entry, so a reviewer would find the cookie material insensitive-data.mdand miss the rest:HttpOnly.chatbot_conversation_idis signed withCOOKIE_SECRETand readable by JS so "Nueva conversación" can detach from a thread without a round-trip. The signature is the security property: a forged or edited id cannot turnGET /conversations/me/currentinto an IDOR, and that endpoint re-checks TTL and identity (user_id, orsession_idwithuser_id IS NULL) regardless of what the cookie claims. A mismatch returns 404 and clears the cookie. Documented indocs/security/sensitive-data.md.b10628f24). The conversation cookie was hardcodedSameSite=Laxwhile its siblingchatbot_session_idusesSameSite=Nonein production, because the deployed web app and API sit on different registrable domains. ALaxcookie is not sent on those cross-site requests, so persistence would have worked locally and silently done nothing in production.searchKnowledgearguments come from the model, so they are validated (empty / oversized / out-of-range / non-integertopK) before the embedding call and before any SQL runs.ACTIVEcorpus sources are visible;DRAFT(mid-ingest) andOUTDATED(superseded) are excluded, so a half-ingested or retired document can never be quoted to a user.CHATBOT_ENABLED— because ingest is intentionally runnable with the flag off, to seed a corpus before switching the assistant on.AZURE_OPENAI_API_KEYis a documented dev-only fallback; production uses managed identity.Mandatory local checks
Deliberately left unticked rather than claimed.
format:check,lintandtype-checkall pass. Tests pass in full serially:apps/api— 185 files, 1990 passed, 2 skipped, 0 failed (--no-file-parallelism)apps/web— 38 files, 714 passedopenspec validate --strict— 3 changes, 37 specs, 0 failedBut root
pnpm testrunsvitest runwith the configuredmaxWorkers: 4/fileParallelism: true, and under parallel execution the Postgres testcontainer restarts mid-run, failing an arbitrary set of chatbot tests with57P03 … in recovery mode. That is #591, filed with the container logs and reproduction. It is an environment/test-infra problem, not a defect in this feature — the same files pass 100% serially.Whether it reproduces on CI is unverified: every observation is from macOS/arm64 under Docker Desktop, whereas CI is
ubuntu-lateston native Docker. CI on this PR is therefore the cheapest way to answer the first question #591 asks. If thebaseleg goes green, #591 is macOS-local; if it fails, the logs here will say why.Screenshots / evidence
Backend evidence is the suite results above. The widget gains a collapsed "Fuentes consultadas" panel under assistant answers and a persistent footer stating answers are AI-generated and should be verified against the cited sources — happy to attach screenshots if useful for design review.
Reviewer notes
Read the commits in order. 22 modular commits, dependency-ordered (deps → schema → infra → shared types → API config → providers → retrieval → tool round → endpoints → CLIs → web → docs), so each is reviewable alone. Three files were split at hunk level to keep one logical change per commit:
testDatabase.ts(image vs. IPv4 fix),eslint.config.ts(provider mocks vs.scripts/**override),useChatStream.ts(citations vs. persistence).Infrastructure change deployers must action. Postgres must now provide the
vectorextension. Local compose, the database package compose, and the testcontainer all move to a digest-pinnedpgvector/pgvector:pg18. The official image cannot apply the migration at all (extension "vector" is not available). Note this image is Debian-based: pgvector publishes no Alpine variant for pg18.That base change swapped musl for glibc collation, which broke five unrelated "ordered by name" tests. Those compared a Postgres
ORDER BYagainst a bare[...names].sort()(UTF-16 byte order) — green only because the container was Alpine, and never matching production, which is created withcollation es_ES.UTF8(infra/modules/postgres.bicep:70). So the new image is closer to production than the old one, and the assertions were encoding an Alpine artifact. Fixed with a shared primary-strength comparator (test/helpers/collation.ts); rationale is in that commit.Two deliberate test-policy changes, both worth a look:
noReferencesToCorpusTables.test.tsasserted the corpus tables were referenced nowhere inapps/api/src— an invariant this phase intentionally lifts. Rather than delete the guardrail, it is narrowed tocorpusAccessBoundary.test.ts: corpus access must stay inside the retrieval module, plus a second assertion proving the check cannot pass vacuously.azureOpenAI.test.tsasserted TOOL-role messages were coerced to user messages ("foundation never emits TOOL"). The tool round now replays real tool results, so that test is replaced with propertool_call_idmapping and two new tests.One design call worth confirming. "Nueva conversación" now starts a fresh client-side thread instead of deleting history: prior turns stay persisted server-side and the cookie is dropped.
deleteHistoryremains on the hook for data-deletion requests but is intentionally unwired from the UI, so there is currently no in-UI way to delete history. Chosen deliberately, since wiring "new conversation" to a destructive delete would make the persistence feature pointless — but it is a product call, so flagging it.Rehydration lives in its own hook (
useConversationRehydrate) rather than insideuseChatStream. The two have genuinely different lifecycles — one mount-time load that settles once, versus per-turn streaming state — so they get one hook each, withseedMessagesas the seam between them.ChatbotWidgetcomposes both.The spec was written before that split and still said the fetch lived in
useChatStream; it has been updated to state the requirement on the composed widget, since the observable behaviour is identical either way.A useful side effect, not the reason for the split: the existing turn-streaming tests never see the mount-time request, so they needed no changes to their mocked response queues. Three of
main's four Chatbot test files are byte-identical tomainhere — evidence that no existing test was loosened to accommodate this feature.useChatStream.test.tsis the exception, and only gains tests (see below).A seed/send race was found and fixed while reviewing that seam.
seedMessagesreplaced the thread unconditionally, so a turn started before the rehydrate settled — a fast typist on a slow API — would have its message replaced by the persisted thread. Worse, that detaches the in-flight assistant bubble: every subsequent delta then fails the identity check inupdateLastAssistantand is silently discarded, so the answer streams into nothing. The seed now loses every race, applying only when no turn is in flight and the thread is still empty.This was already normative — the chatbot-widget spec required that sending during an in-flight rehydrate must not overwrite the new message — but the spec justified it with unmount cancellation, which does not apply here: the hazardous interleaving happens while the widget is still mounted. The implementation inherited that faulty reasoning, and no scenario covered the case, so nothing caught it. The spec's rationale is corrected and two scenarios are added.
useChatStream.test.tsgains fourseedMessagestests; the race test was confirmed to fail against the pre-fix code (the live turn['hola', 'parcial']came back as['thread viejo']) and to pass after.A second widget gap, found the same way. The
doneevent'ssourcesfield was cast rather than validated — onlyArray.isArraywas checked.sources: "broken"was dropped with no warning, andsources: [1,2,3](or any entry missingcite_url) was assigned verbatim and reachedMessageBubble, which keys its rows oncite_url: undefined React key, blank row. Low severity — the API is the sole producer and validates server-side — so this closes a defensive gap, not a live bug. Now parsed through a newSourceCitationWireSchemain@repo/types, kept separate fromSourceCitationSchemabecause the latter's id union also admits the server-side bigint branch, which the wire cannot carry.SourceCitationWireis inferred from the schema so the two cannot drift. Eight tests added — the fourdone-sources scenarios the spec defines had no coverage at this layer at all; the five malformed cases were confirmed to fail against the pre-fix parsing.Spec drift audit. Reading the widget spec for the above prompted a full pass over all seven
chatbot-rag-mvpspecs (53 requirements, 195 scenarios) against the code. The implementation is substantially aligned; five requirements had fallen behind deliberate implementation decisions and are corrected ind91eff88b. The one worth a reviewer's attention: the conversation-cookie requirement still mandatedSameSite=Laxunconditionally — the exact defect fixed inb10628f24— so implementing to spec would have reintroduced it. Code anddocs/securitywere right throughout; only the spec was stale.Obsolete artifact.
openspec/changes/chatbot-web-test-infra/is carried from the working branch but is superseded —mainestablished the web Vitest convention and covered the Chatbot module in #496/#513. It should be archived or dropped rather than implemented; kept here only so nothing is silently lost. Happy to drop it in this PR if preferred.8 of the 99 tasks in the RAG openspec change remain open; the rest are complete.